Re: [PATCH 2/2] Add a cleanup routine for objhash socket objects

[Oren Laadan <orenl-RdfvBDnrOixBDgjK7y7TUQ@public.gmane.org>]

Dan Smith wrote:
> MH> Does it make sense to add a generic (cleanup) operation when only
> MH> one object type will make use of it?
> 
> In general, I agree with you, but I don't think this is an obscure
> case.
> 
> MH> If we add generic mechanisms for things without having multiple
> MH> uses then are we just obfuscating the special cases of the code?
> 
> MH> As an alternative, would it work if we kept a list of unattached
> MH> sockets in the ckpt context, removed them whenever they become
> MH> attached, and then use the generic "end of restart" deferqueue to
> MH> cleanup unattached sockets?
> 
> It would be more code to do it that way, it would inflate the context
> with another list, and would cause us to iterate these objects more
> than we already do.

I agree with Dan.

So the problem happens because the ref_drop() method cannot tell
whether it's called from restore_obj() to drop an extra reference, or
from obj_hash_clear(), to drop the last reference (from the objhash).

[Actually, the way it is now there is still a leak: if the call to
obj_new() from restore_obj() fails, then the subsequent ref_drop()
*is* intended to drop the last reference, but it will not].

I think that the root of this is that ref_drop() doesn't have enough
information about what's going on. Dan's patch suggested to solve it
by adding a specialized ref_drop() - a cleanup method.

However, now I think that it's probably better to modify the prototype
of ref_drop() to be, e.g.:   void ref_drop(void *ptr, int clean), so
it can be smarter.

>>> +	if (sk->sk_socket && !sk->sk_socket->file) {
> 
> MH> Would it make sense to add a little helper to explain this?
> 
> MH> 	if (!is_sk_attached(sk)) {
> 
> Does that make it more clear?  What's the socket attached to?  Another
> socket?  I could add more words to the helper to make it more obvious
> but IMHO, it's clearer to have it spelled out.
> 

Perhaps a comment to spell it out :)

Oren.