Re: [libvirt] [PATCH v2 1/5] storage: Check for valid fc_host parent at startup

On 11/11/2014 10:05 AM, Michal Privoznik wrote: > On 11.11.2014 13:38, John Ferlan wrote: >> >> >> On 11/11/2014 07:21 AM, Michal Privoznik wrote: >>> On 10.11.2014 23:45, John Ferlan wrote: >>>> https://bugzilla.redhat.com/show_bug.cgi?id=1160565 >>>> >>>> If a 'parent' attribute is provided for the fchost, then at startup >>>> time check to ensure it is a vport capable scsi_host. If the parent >>>> is not vport capable, then disallow the startup. The following is the >>>> expected results: >>>> >>>> error: Failed to start pool fc_pool >>>> error: XML error: parent 'scsi_host2' specified for vHBA is not vport capable >>>> >>>> where the XML for the fc_pool is: >>>> >>>> <pool type='scsi'> >>>> <name>fc_pool</name> >>>> <source> >>>> <adapter type='fc_host' parent='scsi_host2' wwnn='5001a4aaf3ca174b' wwpn='5001a4a77192b864'/> >>>> </source> >>>> ... >>>> >>>> and 'scsi_host2' is not vport capable. >>>> >>>> Providing an incorrect parent and a correct wwnn/wwpn could lead to >>>> failures at shutdown (deleteVport) where the assumption is the parent >>>> is for the fchost. >>>> >>>> NOTE: If the provided wwnn/wwpn doesn't resolve to an existing scsi_host, >>>> then we will be creating one with code (virManageVport) which >>>> assumes the parent is vport capable. >>>> >>>> Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; >>>> --- >>>> src/storage/storage_backend_scsi.c | 22 ++++++++++++++++++---- >>>> 1 file changed, 18 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c >>>> index 02160bc..549d8db 100644 >>>> --- a/src/storage/storage_backend_scsi.c >>>> +++ b/src/storage/storage_backend_scsi.c >>>> @@ -556,6 +556,20 @@ createVport(virStoragePoolSourceAdapter adapter) >>>> if (adapter.type != VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) >>>> return 0; >>>> >>>> + /* If a parent was provided, then let's make sure it's vhost capable */ >>>> + if (adapter.data.fchost.parent) { >>>> + if (virGetSCSIHostNumber(adapter.data.fchost.parent, &parent_host) < 0) >>>> + return -1; >> >> ^^^ [1] ^^^ >>>> + >>>> + if (!virIsCapableFCHost(NULL, parent_host)) { >>>> + virReportError(VIR_ERR_XML_ERROR, >>>> + _("parent '%s' specified for vHBA " >>>> + "is not vport capable"), >>>> + adapter.data.fchost.parent); >>>> + return -1; >>>> + } >>>> + } >>>> + >>>> /* This filters either HBA or already created vHBA */ >>>> if ((name = virGetFCHostNameByWWN(NULL, adapter.data.fchost.wwnn, >>>> adapter.data.fchost.wwpn))) { >>>> @@ -565,14 +579,14 @@ createVport(virStoragePoolSourceAdapter adapter) >>>> >>>> if (!adapter.data.fchost.parent && >>>> !(adapter.data.fchost.parent = virFindFCHostCapableVport(NULL))) { >>>> - virReportError(VIR_ERR_XML_ERROR, "%s", >>>> + virReportError(VIR_ERR_XML_ERROR, "%s", >>>> _("'parent' for vHBA not specified, and " >>>> "cannot find one on this host")); >>>> return -1; >>>> - } >>>> >>>> - if (virGetSCSIHostNumber(adapter.data.fchost.parent, &parent_host) < 0) >>>> - return -1; >>>> + if (virGetSCSIHostNumber(adapter.data.fchost.parent, &parent_host) < 0) >>>> + return -1; >>>> + } >>> >>> This chunk seems odd. After the error is reported, the control jumps out >>> from the function, so virGetSCSIHostNumer is not called at all. Did you >>> forget to commit something? >>> >> >> The earlier chunk of code where the parent exists, we figure the >> parent_host value. [1] >> >> This chunk is - if a parent wasn't provided, find a capable vport, then >> get the parent_host value. >> >> I moved it inside the if because it makes no sense to call the function >> twice in the event we had a parent value.. > > My point is, when the conditions are met, and the error is reported the > control jumps out of the function right after virReportError(). That's > because there's 'return -1' after that. However, the next line in the > same block is yet another function call. This, however, will never be > called so it's a dead code. Hence my question. > > Michal > Doh! Of course as you'll find in later patches the logic is adjusted and thus where my brain was already at. I'll fix this one though to be: diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_sc index 549d8db..88928c9 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -577,12 +577,13 @@ createVport(virStoragePoolSourceAdapter adapter) return 0; } - if (!adapter.data.fchost.parent && - !(adapter.data.fchost.parent = virFindFCHostCapableVport(NULL))) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("'parent' for vHBA not specified, and " - "cannot find one on this host")); - return -1; + if (!adapter.data.fchost.parent) { + if (!(adapter.data.fchost.parent = virFindFCHostCapableVport(NULL))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("'parent' for vHBA not specified, and " + "cannot find one on this host")); + return -1; + } if (virGetSCSIHostNumber(adapter.data.fchost.parent, &parent_host) < 0) return -1;