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

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..