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 <[email protected]>
>>> ---
>>> 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
>> [email protected]
>> https://www.redhat.com/mailman/listinfo/libvir-list
>>
>
--
libvir-list mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/libvir-list
