[libvirt] [PATCH v3 01/18] conf: Ensure both parent_wwnn/parent_wwpn provided

[John Ferlan]

On 03/11/2017 09:56 PM, Laine Stump wrote:
> On 03/11/2017 10:34 AM, Laine Stump wrote:
>> On 03/10/2017 04:10 PM, John Ferlan wrote:
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1428209
>>>
>>> Commit id 'bb74a7ffe' neglected to check that both the parent_wwnn
>>> parent_wwpn are in the XML if one or the other is similar to how
>>> the node device code checked (commit id '2b13361bc').
>>>
>>> If only one is provided, the "default" is to use a vHBA capable
>>> adapter (see commit id '78be2e8b'), so the vHBA could start, but
>>> perhaps not on the expected adapter.
>>>
>>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>>> ---
>>>  src/conf/storage_conf.c | 11 +++++++++++
>>>  1 file changed, 11 insertions(+)
>>>
>>> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
>>> index a52eeba..e4d89dd 100644
>>> --- a/src/conf/storage_conf.c
>>> +++ b/src/conf/storage_conf.c
>>> @@ -941,6 +941,17 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt)
>>>              if (!virValidateWWN(ret->source.adapter.data.fchost.wwnn) ||
>>>                  !virValidateWWN(ret->source.adapter.data.fchost.wwpn))
>>>                  goto error;
>>> +
>>> +            if ((ret->source.adapter.data.fchost.parent_wwnn &&
>>> +                 !ret->source.adapter.data.fchost.parent_wwpn) ||
>>> +                (!ret->source.adapter.data.fchost.parent_wwnn &&
>>> +                 ret->source.adapter.data.fchost.parent_wwpn)) {
>> If you want to make this more compact, you could use an XOR. Since there isn't a logical XOR operator in C, you could do this:
>>
>>                   if (!!ret->source.adapter.data.fchost.parent_wwnn ^
>>                        !!ret->source.adapter.data.fchost.parent_wwpn)
>>

Not a real fan of the !! unary and perhaps less so for the ^ unary.  Let
the compiler do that!

>>
>>> +                virReportError(VIR_ERR_XML_ERROR, "%s",
>>> +                               _("must supply both parent_wwnn and "
>>> +                                 "parent_wwpn not just one or the other"));
> 
> While seeing this chunk fly by in patch 7, I realized that this message isn't complete - you must supply both of them *or neither of them* but never just one  - you left out the bit inside the asterisks.
> 

FWIW: From node_device_conf for a similar check:

    if ((def->parent_wwnn && !def->parent_wwpn) ||
        (!def->parent_wwnn && def->parent_wwpn)) {
        virReportError(VIR_ERR_XML_ERROR, "%s",
                       _("must supply both wwnn and wwpn for parent"));
        goto error;
    }

I can alter the message here (and there) to be something like:

_("when used both parent_wwnn('%s') and parent_wwpn('%s') must be
supplied, not just one"), NULLSTR(def->parent_wwnn),
NULLSTR(def->parent_wwpn)...

>>> +                    goto error;
>>> +            }
>>> +
>>
>> Is there any other identifiable info available at this point that could give more useful context to the log message? If nothing else, maybe log whichever of the two values is set so there would be something for the user to search on when looking for the offending xml. (I see the error log in commit  2b13361bc has the same problem).
>>

Nothing springs to mind, but it's still early ;-)

Tks -

John

>>>          } else if (ret->source.adapter.type ==
>>>                     VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) {
>>>              if (!ret->source.adapter.data.scsi_host.name &&
>>
>> ACK, but I'd prefer if the error log gave more identifiable information.
>>
>> --
>> libvir-list mailing list
>> libvir-list at redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list
>>
>