Re: [libvirt] [PATCH v2 04/14] nodedev: Use switch for virNodeDeviceObjHasCap and virNodeDeviceCapMatch
On 05/26/2017 08:36 AM, Peter Krempa wrote:
On Fri, May 26, 2017 at 08:22:26 -0400, John Ferlan wrote:
>
>
> On 05/26/2017 03:14 AM, Peter Krempa wrote:
>> On Thu, May 25, 2017 at 15:57:01 -0400, John Ferlan wrote:
>>> In order to ensure that whenever something is added to virNodeDevCapType
>>> that both functions are considered for processing of a new capability,
>>> change the if-then-else construct into a switch statement.
>>>
>>> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
>>> ---
>>> src/conf/virnodedeviceobj.c | 80
+++++++++++++++++++++++++++++++++------------
>>> 1 file changed, 60 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c
>>> index bbb6eeb..913cdda 100644
>>> --- a/src/conf/virnodedeviceobj.c
>>> +++ b/src/conf/virnodedeviceobj.c
>>> @@ -48,19 +48,41 @@ virNodeDeviceObjHasCap(const virNodeDeviceObj *dev,
>>> while (caps) {
>>> if (STREQ(cap, virNodeDevCapTypeToString(caps->data.type))) {
>>> return 1;
>>> - } else if (caps->data.type == 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 1;
>>> - } else if (caps->data.type == VIR_NODE_DEV_CAP_PCI_DEV) {
>>> - if ((STREQ(cap, mdev_types)) &&
>>> - (caps->data.pci_dev.flags &
>>> - VIR_NODE_DEV_CAP_FLAG_PCI_MDEV))
>>> - return 1;
>>> + } 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))
>>
>> Since you are touching this, put this on a single line. It looks very
>> ugly this way. It also fits into the 80 col boundary, so I don't see a
>> reaosn for this.
>
> For MDEV - it can fit, for SCSI_HOST, not as clean, but it could be:
>
> 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)))
That is WAY worse. The binary mask should be on a single line since it's
semantically connected.
I don't disagree! I wouldn't take that route, but it keeps everything
inside 80 cols...
Also the 80 col rule is not really strict. Especially if it hinders
readability of the code. The above suggestion is a very good example
where you'd completely destroy readability.
I'll just go beyond the 80 cols. I personally don't like that, but
that's just a personal preference thing and readability wise it's I
think better than the multiline condition just because of 80 cols.
Tks -
John
> return 1;
>
> Although I'm not sure I like the way that looks.
[...]
>
> But does that "violate" the too many changes at once "guideline"?
If you feel so, leave it as-is.