Re: [libvirt] [PATCH 03/15] conf: nodedev: Convert virNodeDevObjHasCapStr to a simple wrapper

On Fri, Jan 26, 2018 at 12:40:34PM +0100, Michal Privoznik wrote: > On 01/25/2018 10:23 AM, Erik Skultety wrote: >> This patch drops the capability matching redundancy by simply converting >> the string input to our internal types which are then in turn used for >> the actual capability matching. >> >> Signed-off-by: Erik Skultety <eskultet(a)redhat.com&gt; >> --- >> src/conf/virnodedeviceobj.c | 50 +-------------------------------------------- >> 1 file changed, 1 insertion(+), 49 deletions(-) >> >> diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c >> index ccad59a4b..5360df805 100644 >> --- a/src/conf/virnodedeviceobj.c >> +++ b/src/conf/virnodedeviceobj.c >> @@ -128,55 +128,7 @@ static bool >> virNodeDeviceObjHasCapStr(const virNodeDeviceObj *obj, >> const char *cap) >> { >> - virNodeDevCapsDefPtr caps = obj->def->caps; >> - const char *fc_host_cap = >> - virNodeDevCapTypeToString(VIR_NODE_DEV_CAP_FC_HOST); >> - const char *vports_cap = >> - virNodeDevCapTypeToString(VIR_NODE_DEV_CAP_VPORTS); >> - const char *mdev_types = >> - virNodeDevCapTypeToString(VIR_NODE_DEV_CAP_MDEV_TYPES); >> - >> - while (caps) { >> - if (STREQ(cap, virNodeDevCapTypeToString(caps->data.type))) { >> - return true; >> - } else { >> - switch (caps->data.type) { >> - case VIR_NODE_DEV_CAP_PCI_DEV: >> - if ((STREQ(cap, mdev_types)) && >> - (caps->data.pci_dev.flags & VIR_NODE_DEV_CAP_FLAG_PCI_MDEV)) >> - return true; >> - break; >> - >> - case VIR_NODE_DEV_CAP_SCSI_HOST: >> - if ((STREQ(cap, fc_host_cap) && >> - (caps->data.scsi_host.flags & VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST)) || >> - (STREQ(cap, vports_cap) && >> - (caps->data.scsi_host.flags & VIR_NODE_DEV_CAP_FLAG_HBA_VPORT_OPS))) >> - return true; >> - break; >> - >> - case VIR_NODE_DEV_CAP_SYSTEM: >> - case VIR_NODE_DEV_CAP_USB_DEV: >> - case VIR_NODE_DEV_CAP_USB_INTERFACE: >> - case VIR_NODE_DEV_CAP_NET: >> - case VIR_NODE_DEV_CAP_SCSI_TARGET: >> - case VIR_NODE_DEV_CAP_SCSI: >> - case VIR_NODE_DEV_CAP_STORAGE: >> - case VIR_NODE_DEV_CAP_FC_HOST: >> - case VIR_NODE_DEV_CAP_VPORTS: >> - case VIR_NODE_DEV_CAP_SCSI_GENERIC: >> - case VIR_NODE_DEV_CAP_DRM: >> - case VIR_NODE_DEV_CAP_MDEV_TYPES: >> - case VIR_NODE_DEV_CAP_MDEV: >> - case VIR_NODE_DEV_CAP_CCW_DEV: >> - case VIR_NODE_DEV_CAP_LAST: >> - break; >> - } >> - } >> - >> - caps = caps->next; >> - } >> - return false; >> + return virNodeDeviceObjHasCap(obj, virNodeDevCapTypeFromString(cap)); > > I wonder if we should check for the TypeFromString() conversion rather > than pass it to the other function directly. Well, since the conversion function returns -1 on unknown types and none of our device cap enum types can ever be equal to -1, since that would make it non-deterministic, but I agree that by adding a check explicitly we can save a few cycles, may I assume this to be an ACK with the following squashed in? diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index 2f37c4a05..ccea10793 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -124,7 +124,12 @@ static bool virNodeDeviceObjHasCapStr(const virNodeDeviceObj *obj, const char *cap) { - return virNodeDeviceObjHasCap(obj, virNodeDevCapTypeFromString(cap)); + int type; + + if ((type = virNodeDevCapTypeFromString(cap)) < 0) + return false; + + return virNodeDeviceObjHasCap(obj, type); }