Re: [PATCH v4 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest

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

On 17/04/13 11:13, Ian Campbell wrote:
> On Wed, 2013-04-17 at 11:02 +0100, George Dunlap wrote:
>> On 17/04/13 10:58, Ian Campbell wrote:
>>> On Tue, 2013-04-16 at 19:00 +0100, Roger Pau Monné wrote:
>>>
>>>>> +    libxl_usb_path = libxl__sprintf(gc, "%s/usb", libxl_path);
>>>> I would recommend to use GCSPRINTF instead, it already checks for errors
>>> GCSPRINTF and libxl__sprintf have the same amount of error checking.
>>>
>>> The more important point is that the libxl memory allocation
>>> functions/macros all panic on ENOMEM so there is no need for code in
>>> general to check for allocation errors.
>>>
>>> i.e. This block...
>>>>> +    if (!libxl_usb_path) {
>>>>> +        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "cannot allocate create paths");
>>>>> +        rc = ERROR_FAIL;
>>>>> +        goto out;
>>>>> +    }
>>> ...is unneeded. I didn't check the rest of the patch but I imagine you
>>> are in the habits of checking errors and can now remove a bunch of
>>> code ;-)
>> OK -- well in that block in particular, it's following suit with the
>> rest of the surrounding code which does exactly the same thing.
> That code predates the panic on ENOMEM behaviour.
>
>>    As I
>> said in another e-mail to roger about error checking in this function, I
>> think that it's more important for programmers to be able to see this is
>> exactly the same as all the rest.  At some point there should probably
>> be a clean-up to remove the check from all of them, but I don't think
>> that's worth the risk at this point in the release cycle.
> Rather than having a massive flag day we are taking the approach of
> fixing code as it is changed and part of that is to not introduce new
> "incorrect" code.

I think that makes sense on a function-by-function, or perhaps 
file-by-file basis, but I personally think it's a bad idea to mix 
"templates" in one function like that.  But if that's the plan, I'll go 
with it.

  -George


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel