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

[Michal Privoznik]

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 at 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