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)
>
>
>> + 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.
>> + 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).
>
>> } 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
