[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