From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Jackson Subject: Re: [PATCH v2 17/27] tools/libxl: Support converting a legacy stream to a v2 stream Date: Fri, 10 Jul 2015 13:28:47 +0100 Message-ID: <21919.47743.531030.202193@mariner.uk.xensource.com> References: <1436466413-25867-1-git-send-email-andrew.cooper3@citrix.com> <1436466413-25867-18-git-send-email-andrew.cooper3@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1436466413-25867-18-git-send-email-andrew.cooper3@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Andrew Cooper Cc: Wei Liu , Ian Campbell , Xen-devel List-Id: xen-devel@lists.xenproject.org Andrew Cooper writes ("[PATCH v2 17/27] tools/libxl: Support converting a legacy stream to a v2 stream"): > When a legacy stream is found, it needs to be converted to a v2 stream for the > reading logic. This is done by exec()ing the python conversion utility. > > One complication is that the caller of this interface needs to assume > ownership of the output fd, to prevent it being closed while still in use in a > datacopier. Please reformat the commit message to 70 columns at most. (This applies in general.) Commit messages need to be narrow so that `git log' output is <80. AFAICT in this patch there is no call site for the new functionality. > +static void helper_failed(libxl__egc *egc, > + libxl__conversion_helper_state *chs, int rc) > +{ ... > +static void helper_stop(libxl__egc *egc, libxl__ao_abortable *abrt, int rc) > +{ These two functions are almost identical. Please combine them. > +static void helper_exited(libxl__egc *egc, libxl__ev_child *ch, > + pid_t pid, int status) > +{ > + libxl__conversion_helper_state *chs = CONTAINER_OF(ch, *chs, child); > + STATE_AO_GC(chs->ao); > + > + if (status) { > + libxl_report_child_exitstatus(CTX, XTL_ERROR, "conversion helper", > + pid, status); > + chs->rc = ERROR_FAIL; Don't overwrite an existing rc. Also, you probably don't want to report the results of SIGKILL or SIGTERM with XTL_ERROR. So either report with XTL_DEBUG if rc is already nonzero, or (if you prefer more work) have the signaller record the signal sent and check with WIFSIGNALED. > + } > + else > + chs->rc = 0; Style: `} else {' please. But actually, clearing rc here is wrong I think. If rc is already nonzero, but the helper is exiting zero, you should not be clearing rc. > +_hidden void libxl__conversion_helper_init( > + libxl__conversion_helper_state *chs); Existing style would be +_hidden void libxl__conversion_helper_init + (libxl__conversion_helper_state *chs); Cf libxl__domain_suspend_common_switch_qemu_logdirty etc. (There are a few more occurrences of "(\n" in your series; please fix them too.) > +#else > +/* Stubs for non-x86 architecture to reduce code #ifdefary */ > +static inline void libxl__conversion_helper_init( > + libxl__conversion_helper_state *chs) { } Please, no. Can we have a separate file instead ? I'm really not a fan of #ifdefery at all. In header files it is particularly pernicious. > typedef struct libxl__domain_suspend_state libxl__domain_suspend_state; > @@ -3283,6 +3330,7 @@ struct libxl__domain_create_state { > * for the non-stubdom device model. */ > libxl__stream_read_state srs; > libxl__save_helper_state shs; > + libxl__conversion_helper_state chs; Since there is no call site for this functionality, this struct should not be introduced here. In the patch where it /is/ introduced, it should be explicitly initialised, and probably during the domain create exit path its idleness should be asserted. Thanks, Ian.