On 01/29/2018 09:42 AM, Erik Skultety wrote:
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@redhat.com> --- 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); }
Yup, looks good to me. Michal