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

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