[PATCH libvirt v1 1/6] conf: fix ZPCI address validation on s390

Shalini Chellathurai Saroja shalini at linux.ibm.com
Mon Jun 8 08:13:31 UTC 2020


Hi Andrea,

Thank you for the review.

On 6/3/20 12:09 PM, Andrea Bolognani wrote:
> On Thu, 2020-04-09 at 12:31 +0200, Shalini Chellathurai Saroja wrote:
>> -    if (uid &&
>> -        virStrToLong_uip(uid, NULL, 0, &def.uid) < 0) {
>> -        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> -                       _("Cannot parse <address> 'uid' attribute"));
>> -        goto cleanup;
>> +    if (uid) {
>> +        if (virStrToLong_uip(uid, NULL, 0, &def.uid) < 0) {
>> +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> +                           _("Cannot parse <address> 'uid' attribute"));
>> +            return -1;
>> +        }
>> +        if (!virZPCIDeviceAddressIsValid(&def))
>> +            return -1;
>>       }
> This doesn't seem right.
>
> I understand that you're moving the call to
> virZPCIDeviceAddressIsValid() inside the if(uid) branch so that you
> won't get an error for something like
>
>    <zpci fid='0x00000005'/>
>
> but this is not a good way to do it: in fact, you change it again in
> patch 3/6 and adopt the much better approach of storing directly in
> the struct whether uid and fid have been set.
>
> You should go for that approach right away instead of implementing
> this intermediate solution first.
ok, I will do it.
>
>> - cleanup:
>> -    VIR_FREE(uid);
>> -    VIR_FREE(fid);
>> -    return ret;
>> +    return 0;
> Switching to g_autofree is a nice improvement, but it's a completely
> independent one so please make that a separate patch that doesn't
> touch the logic.
ok, I will make this as a separate patch.
>




More information about the libvir-list mailing list