Re: [PATCH 8/19]: SCST SYSFS interface implementation

[Greg KH]

On Thu, Oct 14, 2010 at 11:48:17PM +0400, Vladislav Bolkhovitin wrote:
> Originally I thought you are asking to make tgt_kobj be not embedded in
> struct scst_tgt, but a pointer in it, so scst_tgtt_release() will
> kfree() tgt_kobj. Hence, all the above I wrote about why we have
> tgt_kobj embedded.
> 
> But now I feel like you are asking that scst_tgtt_release() should
> kfree() tgt, not tgt_kobj.
> 
> Is it correct?

I am asking that ANY kobject release function call kfree to release the
memory that object is embedded in.  That is how kobjects work, please
read the documentation for more details.

> We have a simple and straightforward errors recovery semantic: if
> scst_tgt_sysfs_create() failed, then tgt_kobj returned deinited as if
> scst_tgt_sysfs_create() has never been called. This is a regular
> practice in the kernel: don't return half-initialized objects.

True.

> If we implement freeing tgt in scst_tgtt_release() as you requesting, we
> will need to add in the error recovery path additional recovery code to
> track and delete half-initialized tgt_kobj. Particularly, we will need
> to add additional flag to track that tgt_kobj initialized, hence needs
> deleting. Similar flag and code will be added in all similar to scst_tgt
> SCST objects.
> 
> This code will be quite errors prone as you can see on the example of
> device_register() which on failure requires device_put() to be called
> (http://lkml.org/lkml/2010/9/19/93).

That's not "error prone", it's "people don't read the provided
documentation about how to use the API".

And yes, one could argue to make the API easier to use, and patches are
always welcome to do so.

> (I'm not questioning device_register() implementation, there might be
> very good reasons to implement it this way (I don't know), I mean, it
> is too easy to forget to do the needed recovery of the half-created
> objects as this case demonstrating.)

There are good reasons, but the most important one being, if you pass
off an object, as of that moment in time, you had better handle the
reference counting correctly.  If you think of it this way, cleanup
logic is even simpler as that's the only rule you ever need to think
about, none of thies "half-initialized" stuff.

> Could you confirm if I understand you correctly and need to implement
> freeing tgt in the kobject release() function, please?

Again, yes.  Please read the documentation.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/