Re: [PATCH] drivers: base: Remove statistics group if encryption group not created

[Justin Tee]

> I think the patch is correct as-is, because the only way to get to the
> err_del_statistics: label
> is from an explicit goto if an error is returned when attempting to
> create the encryption group.
> In order to get there the creation of the statistics group had to have
> been successful first.

In general, a transport_container does not have to implement an
encryption attribute_group.  Also, not every transport_container
implements a statistics attribute_group either.

Only for scsi_transport_fc’s rport_attr_cont transport_container,
because it uses both the encryption and statistics attribute_group,
that the creation of the statistics group had to have been successful
first in order to have reached err_del_statistics.  However, there are
transport_containers that don’t use statistics, for example
target_attrs and vport_attr_cont.  Although not in existence, there’s
nothing stopping a theoretical transport_container that implements
encryption and not the statistics.

> If the encryption group could not be created, transport_add_class_device()
> would return an error, and the function would have called both
> attribute_container_class_device_del() and the tclass->remove() functions.
>
> The basic issue is that if there is an error creating the encryption group,
> we clean everything up except we do not clean up the statistics group
> that was created and is effectively orphaned.
>
> Or so I think.  If the intention was to allow the creation of the encryption
> group to be optional, then the code could be changed to not use a goto
> to return an error.  But I am not sure what other effects this might have.

scsi_transport_fc doesn’t check the return value of
transport_add_device().  Nevertheless, when LLDDs call
fc_remote_port_delete() or fc_remove_host(), fc_rport_final_delete()
will eventually call transport_remove_device(), which does the
sysfs_remove_group() on the statistics group.  The clean up happens
eventually, just not immediately after a sysfs_create_group() error.

But point taken, if there was an error with creating a requested sysfs
group in transport_add_device(), then we should clean up everything
the transport_container asked for and not have it linger until
transport_remove_device().

Therefore, I’m okay with the err_del_statistics label, but I still
think we should qualify it with if (tcont->statistics).