Re: [libvirt] [PATCH 2/2] Implementation deficiency in virInitctlSetRunLevel v4

On 12/20/2013 09:24 AM, Reco wrote: This fails to compile, due to [1] below: CCLD libvirt_lxc lxc/lxc_driver.c:2783:1: error: return type defaults to 'int' [-Werror] virDomainRebootCallback(pid_t pid ATTRIBUTE_UNUSED, ^ lxc/lxc_driver.c:2783:1: error: no previous prototype for 'virDomainRebootCallback' [-Werror=missing-prototypes] cc1: all warnings being treated as errors Hmm. The callback's return value being used directly as the _exit() value in patch 1 is not a good idea; either this function must convert -1 to EXIT_FAILURE, or we should fix the framework to turn our normal -1 conventions into the correct exit code (I'm opting for the latter). Also, you are now invoking code in between the fork/exec boundary off of a multithreaded parent, which means we must now audit that all calls in this code path are async-signal-safe (if not, the child will deadlock if the fork happened in the parent while some other thread held a lock that the child wants to obtain). Alas, virInitctlSetRunLevel() calls virAsprintf() calls malloc(), which is not async safe :( [we've gotten away with it in the past; LXC is a Linux-only technology, and glibc's malloc is relatively robust enough that I've never seen it fail multithreaded fork/exec setups, whereas I HAVE seen malloc deadlocks on other systems like Solaris]. It also calls virReportSystemError() on failure, but in a child process, this may not get logged the same was as an error message in the parent. More robust would be having the child be silent and just use distinct error codes, then let the parent convert error codes into actual error messages to be logged. But I think we can wait until an actual report of a deadlock before going to that effort. Aha - you are passing NULL for vroot, which means we could get rid of the malloc() issue. In fact, we could get rid of the vroot argument altogether, since all callers now pass NULL, and since we don't want to be tempted to use the unsafe construct. We could use the opaque argument to pass our desired runlevel constant... [2] [1] you missed the 'static int' line here. [2] ... and have only one static callback helper instead of 2, since they differ only by the constant used. Good - I have enough cleanup to do that I can feel good about claiming authorship of the final version (thus bypassing the full name question) while still giving you credit for the find and initial attempts at the fix. v5 coming up soon! -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org