Re: [Xen-devel] [PATCH v2 03/10] libxl: Clean up userlookup_helper_getpw* helper

[George Dunlap <George.Dunlap@xxxxxxxxxx>]

> On Dec 12, 2018, at 3:26 PM, Ian Jackson <ian.jackson@xxxxxxxxxx> wrote:
> 
> George Dunlap writes ("[PATCH v2 03/10] libxl: Clean up 
> userlookup_helper_getpw* helper"):
>> Bring conventions more in line with libxl__xs_read_checked():
>> - If found, return 0 and set pointer to non-NULL
>> - If not found, return 0 and set pointer to NULL
>> - On error, return libxl-style error number.
>> 
>> Update documentation to match.
> ...
>> #define DEFINE_USERLOOKUP_HELPER(NAME,SPEC_TYPE,STRUCTNAME,SYSCONF)     \
>>     static int userlookup_helper_##NAME(libxl__gc *gc,                  \
>> @@ -83,7 +89,7 @@ static int libxl__create_qemu_logfile(libxl__gc *gc, char 
>> *name)
>>         struct STRUCTNAME *resultp = NULL;                              \
> 
> git diff has excelled itself in choice of heading line, hasn't it ?
> 
>> @@ -142,14 +147,14 @@ static int 
>> libxl__domain_get_device_model_uid(libxl__gc *gc,
>>                                          &user_pwbuf, &user_base);
>>     if (ret < 0)
>>         return ret;
>> -    if (ret > 0) {
>> +    if (user_base) {
> 
> I would prefer to also:
> 
>  -    if (ret < 0)
>  +    if (ret)
>           return ret;
> 
> which is more conventional for rc and might reduce the impact of bugs
> where the function returned a positive.  (Twice.)

Looks like thrice, but sure.

> 
> With or without that change,
> 
> Acked-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> 
> Thanks for the cleanup!

No problem — it’s nice to have something tractable and satisfying to accomplish 
when my other active project is a seemingly never-ending ball of twine…

 -George


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel