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.
_______________________________________________
Containers mailing list
Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/containers
