Re: [PATCH v2 for 4.5] xl: Return proper error codes for block-attach and block-detach

[George Dunlap <george.dunlap@eu.citrix.com>]

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 <george.dunlap@eu.citrix.com>
>> ---
>> CC: Ian Campbell <ian.campbell@citrix.com>
>> CC: Ian Jackson <ian.jackson@citrix.com>
>> CC: Wei Liu <wei.liu2@citrix.com>
>> CC: Konrad Wilk <konrad.wilk@oracle.com>
>>
>> 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.
>