From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH v2 for 4.5] xl: Return proper error codes for block-attach and block-detach Date: Mon, 17 Nov 2014 12:36:53 +0000 Message-ID: <5469EBE5.4070209@eu.citrix.com> References: <1415905446-32168-1-git-send-email-george.dunlap@eu.citrix.com> <1415963574.7113.7.camel@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1415963574.7113.7.camel@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: Ian Campbell , Ian Jackson Cc: Wei Liu , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 11/14/2014 11:12 AM, Ian Campbell wrote: > On Thu, 2014-11-13 at 19:04 +0000, George Dunlap wrote: >> Return proper error codes on failure so that scripts can tell whether >> the command completed properly or not. >> >> Also while we're at it, normalize init and dispose of >> libxl_device_disk structures. This means calling init and dispose in >> the actual functions where they are declared. >> >> This in turn means changing parse_disk_config_multistring() to not >> call libxl_device_disk_init. And that in turn means changes to >> callers of parse_disk_config(). >> >> It also means removing libxl_device_disk_init() from >> libxl.c:libxl_vdev_to_device_disk(). This is only called from >> xl_cmdimpl.c. >> >> Signed-off-by: George Dunlap >> --- >> CC: Ian Campbell >> CC: Ian Jackson >> CC: Wei Liu >> CC: Konrad Wilk >> >> Release justification: This is a bug fix. It's a fairly minor one, >> but it's also a very simple one. >> >> v2: >> - Restructure functions to make sure init and dispose are properly >> called. > Sadly this bit has somewhat reduced the truth of the second half of your > release justification, since the patch is a fair bit more subtle though. > Although IMHO it hasn't invalidated the case for taking the patch for > 4.5 (modulo comments below). Well, I think we can take the hacky short-term fix I posted before, which is simple, and do a proper fix after the 4.6 dev window opens up; or we can do a more complete fix now. Or, if the valgrind thing is really important, I could use the change you suggested, and do "return rc ? 1 : 0;" but I really think that's going in the wrong direction... -George > >> --- >> tools/libxl/libxl.c | 2 -- >> tools/libxl/xl_cmdimpl.c | 28 +++++++++++++++++++++------- >> 2 files changed, 21 insertions(+), 9 deletions(-) >> >> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c >> index f7961f6..d9c4ce7 100644 >> --- a/tools/libxl/libxl.c >> +++ b/tools/libxl/libxl.c >> @@ -2611,8 +2611,6 @@ int libxl_vdev_to_device_disk(libxl_ctx *ctx, uint32_t domid, >> if (devid < 0) >> return ERROR_INVAL; >> >> - libxl_device_disk_init(disk); > _init functions are idempotent, so this is harmless, I think. Existing > callers (including things which aren't xl) might be relying on it. > Outside of a freeze period that might be acceptable (those callers are > buggy) but since you are asking for a freeze exception I think this may > be going a step too far in cleaning things up. > > In terms of other callers you've missed > tools/ocaml/libs/xl/xenlight_stubs.c (which appears buggy to me) in > tree, plus whatever use e.g. libvirt makes of it. > >> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c >> index 3c9f146..25af715 100644 >> --- a/tools/libxl/xl_cmdimpl.c >> +++ b/tools/libxl/xl_cmdimpl.c >> @@ -485,8 +485,6 @@ static void parse_disk_config_multistring(XLU_Config **config, >> { >> int e; >> >> - libxl_device_disk_init(disk); > Likewise here, although to a lesser extent since this is xl not libxl. >> >> @@ -6378,13 +6382,17 @@ int main_blockattach(int argc, char **argv) >> printf("disk: %s\n", json); >> free(json); >> if (ferror(stdout) || fflush(stdout)) { perror("stdout"); exit(-1); } >> - return 0; > You should set rc = 0 here rather than initing it at declaration to > catch error paths which don't set the result. (we aren't very consistent > about this in the code today so I'm only mentioning it because other > changes seem to be needed, if that turns out not to be the case and > there's no need for v3 then this shouldn't block acceptance) > >> + goto out; > I'm not 100% convinced by the use of the goto out error handling style > for a success path, but it's probably better than duplicating the exit > path or adding !dryrun checks to all the following operations. Ian, > since you recently posted updated coding style for things around this, > what do you think? > > Ian. >