Re: [libvirt] [PATCH 2/3] fdstream: avoid child process leak on error
On 07/14/2011 11:42 AM, Matthias Bolte wrote:
2011/7/12 Eric Blake <eblake(a)redhat.com>:
> By requesting the pid in virCommandRunAsync, fdstream was claiming
> that it would manually wait for the process. But on the failure
> path, the child process was being leaked.
This difference in behavior between virCommandRunAsync(..., NULL) and
virCommandRunAsync(..., &pid) is not documented on virCommandRunAsync
itself but only on virCommandFree.
That was the case in libvirt.git right now, but I thought I addressed
that properly in patch 1/3.
Also virCommandFree refers to
waitpid instead of you're newly added virPidWait.
Oh, good point. I'll tweak patch 1 to fix that before I push.
Finally I noticed
that the comments in command.h and command.c are out of sync (at least
for virCommandFree). Why are you documenting in two places making it
prone to get out of sync?
Which file is better, the .c or the .h? I'm fine with dropping docs
from one of the two locations, if we have a preference on which file is
more likely to be referenced.
.c:
PRO - the documentation is next to the implementation, and hopefully
easier to keep the two in sync
PRO - matches how we do things in libvirt.h vs. libvirt.c
CON - much larger file to search through when you are looking up the
reference
.h:
PRO - smaller file
CON - further away from implementation, could go stale
so I'm leaning towards .c only.
ACK, to this patch.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 619 bytes)