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.
