[libvirt] [PATCH v2 0/5] Resolve fc_host startup, shutdown issues
This series will replace: http://www.redhat.com/archives/libvir-list/2014-November/msg00197.html There are two bugs being fixed here and having them reviewed together makes things easier in the long run as the last 3 patches depended upon the first 2 being present. https://bugzilla.redhat.com/show_bug.cgi?id=1160565 Patch 1 is as was in the previous set Patch 2 is essentially the same, except the single function checkVhbaSCSIHostParent was split out to generate a getVhbaSCSIHostParent which gets used later on. https://bugzilla.redhat.com/show_bug.cgi?id=1160926 Patch 3 fixes an issue which took a bit of gdb time in order to figure out exactly what was going wrong. Essentially, the exising createVport code had a path which would find the "best available" fc_host to use and save the parent on return so that the deleteVport code would delete the parent. However, the code used a copy of the adapter not a reference to the adapter and thus the change was lost leaving the vHBA on the system. Patch 4 adds a new function to save the StoragePool XML given a configFile. This will be used in the next patch because while having the in memory fchost copy updated does work - if libvirtd dies in between we're back at square 1 reading storage pool config files and not knowing whence we started. Patch 5 adds a new "managed" attribute to the fchost XML. It does this mainly to let the deleteVport know when it should delete the self created vport. Prior to this code, the reliance was that we didn't have a parent provided. However, this causes an issue where if someone used nodedev-create in order to create a vHBA and then tried to use that vHBA in a storage pool we would delete that vHBA when we were done, which may not be expected. Using the managed attribute at creation time will let libvirt know what to do in this case. NOTE: There's still one more issue in the code, but it's a bit trickier to resolve. A libvirt created vport doesn't seem to want to find the LUN's on the initial PoolRefresh that occurs during startup. However, if one does a refresh immediately after starting the pool, the luns show up. It seems perhaps there's some sort of locking issue that won't allow the udevEventHandleCallback to 'add' the new device. John Ferlan (5): storage: Check for valid fc_host parent at startup storage: Ensure fc_host parent matches wwnn/wwpn storage: Don't use a stack copy of the adapter storage: Introduce virStoragePoolSaveConfig storage: Introduce 'managed' for the fchost parent docs/formatstorage.html.in | 28 ++- docs/schemas/basictypes.rng | 5 + src/conf/storage_conf.c | 70 ++++-- src/conf/storage_conf.h | 4 + src/libvirt_private.syms | 1 + src/storage/storage_backend_scsi.c | 237 ++++++++++++++++++--- .../pool-scsi-type-fc-host-managed.xml | 15 ++ .../pool-scsi-type-fc-host-managed.xml | 18 ++ tests/storagepoolxml2xmltest.c | 1 + 9 files changed, 323 insertions(+), 56 deletions(-) create mode 100644 tests/storagepoolxml2xmlin/pool-scsi-type-fc-host-managed.xml create mode 100644 tests/storagepoolxml2xmlout/pool-scsi-type-fc-host-managed.xml -- 1.9.3
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@redhat.com> --- 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; + + 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; + } if (virManageVport(parent_host, adapter.data.fchost.wwpn, adapter.data.fchost.wwnn, VPORT_CREATE) < 0) -- 1.9.3
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@redhat.com> --- 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; + + 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?
if (virManageVport(parent_host, adapter.data.fchost.wwpn, adapter.data.fchost.wwnn, VPORT_CREATE) < 0)
Michal
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@redhat.com> --- 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.. John
if (virManageVport(parent_host, adapter.data.fchost.wwpn, adapter.data.fchost.wwnn, VPORT_CREATE) < 0)
Michal
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@redhat.com> --- 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
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@redhat.com> --- 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;
On 11.11.2014 16:21, John Ferlan wrote:
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@redhat.com> --- 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;
ACK with this squashed in. Michal
https://bugzilla.redhat.com/show_bug.cgi?id=1160565 The existing code assumed that the configuration of a 'parent' attribute was correct for the createVport path. As it turns out, that may not be the case which leads errors during the deleteVport path because the wwnn/wwpn isn't associated with the parent. With this change the following is reported: error: Failed to start pool fc_pool_host3 error: XML error: Parent attribute 'scsi_host4' does not match parent 'scsi_host3' determined for the 'scsi_host16' wwnn/wwpn lookup. for XML as follows: <pool type='scsi'> <name>fc_pool</name> <source> <adapter type='fc_host' parent='scsi_host4' wwnn='5001a4aaf3ca174b' wwpn='5001a4a77192b864'/> </source> Where 'nodedev-dumpxml scsi_host16' provides: <device> <name>scsi_host16</name> <path>/sys/devices/pci0000:00/0000:00:04.0/0000:10:00.0/host3/vport-3:0-11/host16</path> <parent>scsi_host3</parent> <capability type='scsi_host'> <host>16</host> <unique_id>13</unique_id> <capability type='fc_host'> <wwnn>5001a4aaf3ca174b</wwnn> <wwpn>5001a4a77192b864</wwpn> ... The patch also adjusts the description of the storage pool to describe the restrictions. Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatstorage.html.in | 15 ++++- src/storage/storage_backend_scsi.c | 118 +++++++++++++++++++++++++++++++++++-- 2 files changed, 126 insertions(+), 7 deletions(-) diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in index d3e6f05..9d77c45 100644 --- a/docs/formatstorage.html.in +++ b/docs/formatstorage.html.in @@ -166,7 +166,13 @@ to uniquely identify the device in the Fibre Channel storage fabric (the device can be either a HBA or vHBA). Both wwnn and wwpn should be specified. Use the command 'virsh nodedev-dumpxml' to determine - how to set the values for the wwnn/wwpn of a (v)HBA. + how to set the values for the wwnn/wwpn of a (v)HBA. The wwnn and + wwpn have very specific numerical format requirements based on the + hypervisor being used, thus care should be taken if you decide to + generate your own to follow the standards; otherwise, the pool + will fail to start with an opaque error message indicating failure + to write to the vport_create file during vport create/delete due + to "No such file or directory". <span class="since">Since 1.0.4</span> </dd> </dl> @@ -176,7 +182,12 @@ parent scsi_host device defined in the <a href="formatnode.html">Node Device</a> database as the <a href="http://wiki.libvirt.org/page/NPIV_in_libvirt">NPIV</a> - virtual Host Bus Adapter (vHBA). + virtual Host Bus Adapter (vHBA). The value provided must be + a vport capable scsi_host. The value is not the scsi_host of + the vHBA created by 'virsh nodedev-create', rather it is + the parent of that vHBA. If the value is not provided, libvirt + will determine the parent based either finding the wwnn,wwpn + defined for an existing scsi_host or by creating a vHBA. <span class="since">Since 1.0.4</span> </dd> </dl> diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index 549d8db..5220292 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -34,6 +34,7 @@ #include "virlog.h" #include "virfile.h" #include "vircommand.h" +#include "viraccessapicheck.h" #include "virstring.h" #define VIR_FROM_THIS VIR_FROM_STORAGE @@ -547,8 +548,103 @@ getAdapterName(virStoragePoolSourceAdapter adapter) return name; } +/* + * Using the host# name found via wwnn/wwpn lookup in the fc_host + * sysfs tree to get the parent 'scsi_host#'. On entry we need 'conn' + * set. We won't get here from the autostart path since the caller + * will return true before calling this function. For the shutdown + * path we won't be able to delete the vport. + */ +static char * ATTRIBUTE_NONNULL(1) +getVhbaSCSIHostParent(virConnectPtr conn, + const char *name) +{ + char *nodedev_name = NULL; + virNodeDevicePtr device = NULL; + char *xml = NULL; + virNodeDeviceDefPtr def = NULL; + char *vhba_parent = NULL; + virErrorPtr savedError = NULL; + + VIR_DEBUG("conn=%p, name=%s", conn, name); + + /* We get passed "host#" from the return from virGetFCHostNameByWWN, + * so we need to adjust that to what the nodedev lookup expects + */ + if (virAsprintf(&nodedev_name, "scsi_%s", name) < 0) + goto cleanup; + + /* Compare the scsi_host for the name with the provided parent + * if not the same, then fail + */ + if (!(device = virNodeDeviceLookupByName(conn, nodedev_name))) { + virReportError(VIR_ERR_XML_ERROR, + _("Cannot find '%s' in node device database"), + nodedev_name); + goto cleanup; + } + + if (!(xml = virNodeDeviceGetXMLDesc(device, 0))) + goto cleanup; + + if (!(def = virNodeDeviceDefParseString(xml, EXISTING_DEVICE, NULL))) + goto cleanup; + + ignore_value(VIR_STRDUP(vhba_parent, def->parent)); + + cleanup: + if (!vhba_parent) + savedError = virSaveLastError(); + VIR_FREE(nodedev_name); + virNodeDeviceDefFree(def); + VIR_FREE(xml); + virNodeDeviceFree(device); + if (savedError) { + virSetError(savedError); + virFreeError(savedError); + } + return vhba_parent; +} + +/* + * Using the host# name found via wwnn/wwpn lookup in the fc_host + * sysfs tree to get the parent 'scsi_host#' to ensure it matches. + */ +static bool +checkVhbaSCSIHostParent(virConnectPtr conn, + const char *name, + const char *parent_name) +{ + char *vhba_parent = NULL; + bool retval = false; + + VIR_DEBUG("conn=%p, name=%s, parent_name=%s", conn, name, parent_name); + + /* autostarted pool - assume we're OK */ + if (!conn) + return true; + + if (!(vhba_parent = getVhbaSCSIHostParent(conn, name))) + goto cleanup; + + if (STRNEQ(parent_name, vhba_parent)) { + virReportError(VIR_ERR_XML_ERROR, + _("Parent attribute '%s' does not match parent '%s' " + "determined for the '%s' wwnn/wwpn lookup."), + parent_name, vhba_parent, name); + goto cleanup; + } + + retval = true; + + cleanup: + VIR_FREE(vhba_parent); + return retval; +} + static int -createVport(virStoragePoolSourceAdapter adapter) +createVport(virConnectPtr conn, + virStoragePoolSourceAdapter adapter) { unsigned int parent_host; char *name = NULL; @@ -570,11 +666,23 @@ createVport(virStoragePoolSourceAdapter adapter) } } - /* This filters either HBA or already created vHBA */ + /* If we find an existing HBA/vHBA within the fc_host sysfs + * using the wwnn/wwpn, then a nodedev is already created for + * this pool and we don't have to create the vHBA + */ if ((name = virGetFCHostNameByWWN(NULL, adapter.data.fchost.wwnn, adapter.data.fchost.wwpn))) { + int retval = 0; + + /* If a parent was provided, let's make sure the 'name' we've + * retrieved has the same parent + */ + if (adapter.data.fchost.parent && + !checkVhbaSCSIHostParent(conn, name, adapter.data.fchost.parent)) + retval = -1; + VIR_FREE(name); - return 0; + return retval; } if (!adapter.data.fchost.parent && @@ -704,11 +812,11 @@ virStorageBackendSCSIRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED, } static int -virStorageBackendSCSIStartPool(virConnectPtr conn ATTRIBUTE_UNUSED, +virStorageBackendSCSIStartPool(virConnectPtr conn, virStoragePoolObjPtr pool) { virStoragePoolSourceAdapter adapter = pool->def->source.adapter; - return createVport(adapter); + return createVport(conn, adapter); } static int -- 1.9.3
https://bugzilla.redhat.com/show_bug.cgi?id=1160926 Passing a copy of the storage pool adapter to a function just changes the copy of the fields in the particular function and then when returning to the caller those changes are discarded. While not yet biting us in the storage clean-up case, it did cause an issue for the fchost storage pool startup case, createVport. The issue was at startup, if no parent is found in the XML, the code will search for the 'best available' parent and then store that in the in memory copy of the adapter. Of course, in this case it was a copy, so when returning to the virStorageBackendSCSIStartPool that change was discarded (or lost) from the pool->def->source.adapter which meant at shutdown (deleteVport), the code assumed no adapter was passed and skipped the deletion, leaving the vHBA created by libvirt still defined requiring an additional stop of a nodedev-destroy to remove. Adjusted the createVport to take virStoragePoolDefPtr instead of the adapter copy. Then use the virStoragePoolSourceAdapterPtr when processing. A future patch will need the 'def' anyway, so this just sets up for that. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/storage_conf.c | 16 ++++++++-------- src/conf/storage_conf.h | 1 + src/storage/storage_backend_scsi.c | 32 ++++++++++++++++---------------- 3 files changed, 25 insertions(+), 24 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index afd6cd4..52656e5 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -343,15 +343,15 @@ virStorageVolDefFree(virStorageVolDefPtr def) } static void -virStoragePoolSourceAdapterClear(virStoragePoolSourceAdapter adapter) +virStoragePoolSourceAdapterClear(virStoragePoolSourceAdapterPtr adapter) { - if (adapter.type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) { - VIR_FREE(adapter.data.fchost.wwnn); - VIR_FREE(adapter.data.fchost.wwpn); - VIR_FREE(adapter.data.fchost.parent); - } else if (adapter.type == + if (adapter->type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) { + VIR_FREE(adapter->data.fchost.wwnn); + VIR_FREE(adapter->data.fchost.wwpn); + VIR_FREE(adapter->data.fchost.parent); + } else if (adapter->type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) { - VIR_FREE(adapter.data.scsi_host.name); + VIR_FREE(adapter->data.scsi_host.name); } } @@ -380,7 +380,7 @@ virStoragePoolSourceClear(virStoragePoolSourcePtr source) VIR_FREE(source->devices); VIR_FREE(source->dir); VIR_FREE(source->name); - virStoragePoolSourceAdapterClear(source->adapter); + virStoragePoolSourceAdapterClear(&source->adapter); VIR_FREE(source->initiator.iqn); virStorageAuthDefFree(source->auth); VIR_FREE(source->vendor); diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index 1276ac2..a9b5bdb 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -177,6 +177,7 @@ typedef enum { VIR_ENUM_DECL(virStoragePoolSourceAdapter) typedef struct _virStoragePoolSourceAdapter virStoragePoolSourceAdapter; +typedef virStoragePoolSourceAdapter *virStoragePoolSourceAdapterPtr; struct _virStoragePoolSourceAdapter { int type; /* virStoragePoolSourceAdapterType */ diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index 5220292..41ea67a 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -644,24 +644,25 @@ checkVhbaSCSIHostParent(virConnectPtr conn, static int createVport(virConnectPtr conn, - virStoragePoolSourceAdapter adapter) + virStoragePoolDefPtr def) { + virStoragePoolSourceAdapterPtr adapter = &def->source.adapter; unsigned int parent_host; char *name = NULL; - if (adapter.type != VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) + 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) + if (adapter->data.fchost.parent) { + if (virGetSCSIHostNumber(adapter->data.fchost.parent, &parent_host) < 0) return -1; if (!virIsCapableFCHost(NULL, parent_host)) { virReportError(VIR_ERR_XML_ERROR, _("parent '%s' specified for vHBA " "is not vport capable"), - adapter.data.fchost.parent); + adapter->data.fchost.parent); return -1; } } @@ -670,34 +671,34 @@ createVport(virConnectPtr conn, * using the wwnn/wwpn, then a nodedev is already created for * this pool and we don't have to create the vHBA */ - if ((name = virGetFCHostNameByWWN(NULL, adapter.data.fchost.wwnn, - adapter.data.fchost.wwpn))) { + if ((name = virGetFCHostNameByWWN(NULL, adapter->data.fchost.wwnn, + adapter->data.fchost.wwpn))) { int retval = 0; /* If a parent was provided, let's make sure the 'name' we've * retrieved has the same parent */ - if (adapter.data.fchost.parent && - !checkVhbaSCSIHostParent(conn, name, adapter.data.fchost.parent)) + if (adapter->data.fchost.parent && + !checkVhbaSCSIHostParent(conn, name, adapter->data.fchost.parent)) retval = -1; VIR_FREE(name); return retval; } - if (!adapter.data.fchost.parent && - !(adapter.data.fchost.parent = virFindFCHostCapableVport(NULL))) { + 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 (virGetSCSIHostNumber(adapter.data.fchost.parent, &parent_host) < 0) + if (virGetSCSIHostNumber(adapter->data.fchost.parent, &parent_host) < 0) return -1; } - if (virManageVport(parent_host, adapter.data.fchost.wwpn, - adapter.data.fchost.wwnn, VPORT_CREATE) < 0) + if (virManageVport(parent_host, adapter->data.fchost.wwpn, + adapter->data.fchost.wwnn, VPORT_CREATE) < 0) return -1; virFileWaitForDevices(); @@ -815,8 +816,7 @@ static int virStorageBackendSCSIStartPool(virConnectPtr conn, virStoragePoolObjPtr pool) { - virStoragePoolSourceAdapter adapter = pool->def->source.adapter; - return createVport(conn, adapter); + return createVport(conn, pool->def); } static int -- 1.9.3
https://bugzilla.redhat.com/show_bug.cgi?id=1160926 Introduce the ability to save a configuration of a persistent configuration that may be changed by storage pool backend activity, such as start or stop Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/storage_conf.c | 37 ++++++++++++++++++++++--------------- src/conf/storage_conf.h | 2 ++ src/libvirt_private.syms | 1 + 3 files changed, 25 insertions(+), 15 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 52656e5..4126451 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1884,14 +1884,33 @@ virStoragePoolLoadAllConfigs(virStoragePoolObjListPtr pools, } int -virStoragePoolObjSaveDef(virStorageDriverStatePtr driver, - virStoragePoolObjPtr pool, +virStoragePoolSaveConfig(const char *configFile, virStoragePoolDefPtr def) { char uuidstr[VIR_UUID_STRING_BUFLEN]; char *xml; int ret = -1; + if (!(xml = virStoragePoolDefFormat(def))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to generate XML")); + return -1; + } + + virUUIDFormat(def->uuid, uuidstr); + ret = virXMLSaveFile(configFile, + virXMLPickShellSafeComment(def->name, uuidstr), + "pool-edit", xml); + VIR_FREE(xml); + + return ret; +} + +int +virStoragePoolObjSaveDef(virStorageDriverStatePtr driver, + virStoragePoolObjPtr pool, + virStoragePoolDefPtr def) +{ if (!pool->configFile) { if (virFileMakePath(driver->configDir) < 0) { virReportSystemError(errno, @@ -1912,19 +1931,7 @@ virStoragePoolObjSaveDef(virStorageDriverStatePtr driver, } } - if (!(xml = virStoragePoolDefFormat(def))) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("failed to generate XML")); - return -1; - } - - virUUIDFormat(def->uuid, uuidstr); - ret = virXMLSaveFile(pool->configFile, - virXMLPickShellSafeComment(def->name, uuidstr), - "pool-edit", xml); - VIR_FREE(xml); - - return ret; + return virStoragePoolSaveConfig(pool->configFile, def); } int diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index a9b5bdb..67145a0 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -360,6 +360,8 @@ virStoragePoolObjPtr virStoragePoolObjAssignDef(virStoragePoolObjListPtr pools, virStoragePoolDefPtr def); +int virStoragePoolSaveConfig(const char *configDir, + virStoragePoolDefPtr def); int virStoragePoolObjSaveDef(virStorageDriverStatePtr driver, virStoragePoolObjPtr pool, virStoragePoolDefPtr def); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index b8f35e8..0864618 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -753,6 +753,7 @@ virStoragePoolObjLock; virStoragePoolObjRemove; virStoragePoolObjSaveDef; virStoragePoolObjUnlock; +virStoragePoolSaveConfig; virStoragePoolSourceAdapterTypeFromString; virStoragePoolSourceAdapterTypeToString; virStoragePoolSourceClear; -- 1.9.3
https://bugzilla.redhat.com/show_bug.cgi?id=1160926 Introduce a 'managed' attribute to allow libvirt to decide whether to delete a vHBA vport created via external means such as nodedev-create. The code currently decides whether to delete the vHBA based solely on whether the parent was provided at creation time. However, that may not be the desired action, so rather than delete and force someone to create another vHBA via an additional nodedev-create allow the configuration of the storage pool to decide the desired action. During createVport when libvirt does the VPORT_CREATE, set the managed value to YES if not already set to indicate to the deleteVport code that it should delete the vHBA when the pool is destroyed. If libvirtd is restarted all the memory only state was lost, so for a persistent storage pool, use the virStoragePoolSaveConfig in order to write out the managed value. Because we're now saving the current configuration, we need to be sure to not save the parent in the output XML if it was undefined at start. Saving the name would cause future starts to always use the same parent which is not the expected result when not providing a parent. By not providing a parent, libvirt is expected to find the best available vHBA port for each subsequent (re)start. At deleteVport, use the new managed value to decide whether to execute the VPORT_DELETE. Since we no longer save the parent in memory or in XML when provided, if it was not provided, then we have to look it up. Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatstorage.html.in | 13 +++ docs/schemas/basictypes.rng | 5 ++ src/conf/storage_conf.c | 17 ++++ src/conf/storage_conf.h | 1 + src/storage/storage_backend_scsi.c | 93 +++++++++++++++++----- .../pool-scsi-type-fc-host-managed.xml | 15 ++++ .../pool-scsi-type-fc-host-managed.xml | 18 +++++ tests/storagepoolxml2xmltest.c | 1 + 8 files changed, 143 insertions(+), 20 deletions(-) create mode 100644 tests/storagepoolxml2xmlin/pool-scsi-type-fc-host-managed.xml create mode 100644 tests/storagepoolxml2xmlout/pool-scsi-type-fc-host-managed.xml diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in index 9d77c45..29c1931 100644 --- a/docs/formatstorage.html.in +++ b/docs/formatstorage.html.in @@ -190,6 +190,19 @@ defined for an existing scsi_host or by creating a vHBA. <span class="since">Since 1.0.4</span> </dd> + <dt><code>managed</code></dt> + <dd>An optional attribute to instruct the SCSI storage backend to + manage destroying the vHBA when the pool is destroyed. For + configurations that do not provide an already created vHBA + from a 'virsh nodedev-create', libvirt will set this property + to "yes". For configurations that have already created a vHBA + via 'virsh nodedev-create' and are using the wwnn/wwpn from + that vHBA and optionally the scsi_host parent, setting this + attribute to "yes" will allow libvirt to destroy the node device + when the pool is destroyed. If this attribute is set to "no" or + not defined in the XML, then libvirt will not destroy the vHBA. + <span class="since">Since 1.2.11</span> + </dd> </dl> <dl> <dt><code>parentaddr</code></dt> diff --git a/docs/schemas/basictypes.rng b/docs/schemas/basictypes.rng index 14245c9..9ddd92b 100644 --- a/docs/schemas/basictypes.rng +++ b/docs/schemas/basictypes.rng @@ -389,6 +389,11 @@ <text/> </attribute> </optional> + <optional> + <attribute name='managed'> + <ref name="virYesNo"/> + </attribute> + </optional> <attribute name='wwnn'> <ref name='wwn'/> </attribute> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 4126451..1251b47 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -475,6 +475,7 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, char *name = NULL; char *port = NULL; char *adapter_type = NULL; + char *managed = NULL; int n; relnode = ctxt->node; @@ -578,6 +579,18 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) { source->adapter.data.fchost.parent = virXPathString("string(./adapter/@parent)", ctxt); + managed = virXPathString("string(./adapter/@managed)", ctxt); + if (managed) { + source->adapter.data.fchost.managed = + virTristateBoolTypeFromString(managed); + if (source->adapter.data.fchost.managed < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown fc_host managed setting '%s'"), + managed); + goto cleanup; + } + } + source->adapter.data.fchost.wwnn = virXPathString("string(./adapter/@wwnn)", ctxt); source->adapter.data.fchost.wwpn = @@ -675,6 +688,7 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, VIR_FREE(port); VIR_FREE(nodeset); VIR_FREE(adapter_type); + VIR_FREE(managed); virStorageAuthDefFree(authdef); return ret; } @@ -1076,6 +1090,9 @@ virStoragePoolSourceFormat(virBufferPtr buf, if (src->adapter.type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) { virBufferEscapeString(buf, " parent='%s'", src->adapter.data.fchost.parent); + if (src->adapter.data.fchost.managed) + virBufferAsprintf(buf, " managed='%s'", + virTristateBoolTypeToString(src->adapter.data.fchost.managed)); virBufferAsprintf(buf, " wwnn='%s' wwpn='%s'/>\n", src->adapter.data.fchost.wwnn, src->adapter.data.fchost.wwpn); diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index 67145a0..765f681 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -192,6 +192,7 @@ struct _virStoragePoolSourceAdapter { char *parent; char *wwnn; char *wwpn; + int managed; /* enum virTristateSwitch */ } fchost; } data; }; diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index 41ea67a..b1602ea 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -590,6 +590,9 @@ getVhbaSCSIHostParent(virConnectPtr conn, if (!(def = virNodeDeviceDefParseString(xml, EXISTING_DEVICE, NULL))) goto cleanup; + /* The caller checks whether the returned value is NULL or not + * before continuing + */ ignore_value(VIR_STRDUP(vhba_parent, def->parent)); cleanup: @@ -644,15 +647,21 @@ checkVhbaSCSIHostParent(virConnectPtr conn, static int createVport(virConnectPtr conn, + const char *configFile, virStoragePoolDefPtr def) { virStoragePoolSourceAdapterPtr adapter = &def->source.adapter; unsigned int parent_host; char *name = NULL; + char *parent_hoststr = NULL; if (adapter->type != VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) return 0; + VIR_DEBUG("conn=%p, configFile='%s' parent='%s', wwnn='%s' wwpn='%s'", + conn, NULLSTR(configFile), NULLSTR(adapter->data.fchost.parent), + adapter->data.fchost.wwnn, adapter->data.fchost.wwpn); + /* 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) @@ -686,15 +695,41 @@ createVport(virConnectPtr conn, return retval; } - 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 (!(parent_hoststr = 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) + if (virGetSCSIHostNumber(parent_hoststr, &parent_host) < 0) { + VIR_FREE(parent_hoststr); return -1; + } + + /* NOTE: + * We do not save the parent_hoststr in adapter->data.fchost.parent + * since we could be writing out the 'def' to the saved XML config. + * If we wrote out the name in the XML, then future starts would + * always use the same parent rather than finding the "best available" + * parent. Besides we have a way to determine the parent based on + * the 'name' field. + */ + VIR_FREE(parent_hoststr); + } + + /* Since we're creating the vHBA, then we need to manage removing it + * as well. Since we need this setting to "live" through a libvirtd + * restart, we need to save the persistent configuration. So if not + * already defined as YES, then force the issue. + */ + if (adapter->data.fchost.managed != VIR_TRISTATE_BOOL_YES) { + adapter->data.fchost.managed = VIR_TRISTATE_BOOL_YES; + if (configFile) { + if (virStoragePoolSaveConfig(configFile, def) < 0) + return -1; + } } if (virManageVport(parent_host, adapter->data.fchost.wwpn, @@ -706,30 +741,47 @@ createVport(virConnectPtr conn, } static int -deleteVport(virStoragePoolSourceAdapter adapter) +deleteVport(virConnectPtr conn, + virStoragePoolSourceAdapter adapter) { unsigned int parent_host; char *name = NULL; + char *vhba_parent = NULL; int ret = -1; if (adapter.type != VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) return 0; - /* It must be a HBA instead of a vHBA as long as "parent" - * is NULL. "createVport" guaranteed "parent" for a vHBA - * cannot be NULL, it's either specified in XML, or detected - * automatically. - */ - if (!adapter.data.fchost.parent) + VIR_DEBUG("conn=%p parent='%s', managed='%d' wwnn='%s' wwpn='%s'", + conn, NULLSTR(adapter.data.fchost.parent), + adapter.data.fchost.managed, + adapter.data.fchost.wwnn, + adapter.data.fchost.wwpn); + + /* If we're not managing the deletion of the vHBA, then just return */ + if (adapter.data.fchost.managed != VIR_TRISTATE_BOOL_YES) return 0; + /* Find our vHBA by searching the fc_host sysfs tree for our wwnn/wwpn */ if (!(name = virGetFCHostNameByWWN(NULL, adapter.data.fchost.wwnn, adapter.data.fchost.wwpn))) - return -1; - - if (virGetSCSIHostNumber(adapter.data.fchost.parent, &parent_host) < 0) goto cleanup; + /* If at startup time we provided a parent, then use that to + * get the parent_host value; otherwise, we have to determine + * the parent scsi_host which we did not save at startup time + */ + if (adapter.data.fchost.parent) { + if (virGetSCSIHostNumber(adapter.data.fchost.parent, &parent_host) < 0) + goto cleanup; + } else { + if (!(vhba_parent = getVhbaSCSIHostParent(conn, name))) + goto cleanup; + + if (virGetSCSIHostNumber(vhba_parent, &parent_host) < 0) + goto cleanup; + } + if (virManageVport(parent_host, adapter.data.fchost.wwpn, adapter.data.fchost.wwnn, VPORT_DELETE) < 0) goto cleanup; @@ -737,6 +789,7 @@ deleteVport(virStoragePoolSourceAdapter adapter) ret = 0; cleanup: VIR_FREE(name); + VIR_FREE(vhba_parent); return ret; } @@ -816,15 +869,15 @@ static int virStorageBackendSCSIStartPool(virConnectPtr conn, virStoragePoolObjPtr pool) { - return createVport(conn, pool->def); + return createVport(conn, pool->configFile, pool->def); } static int -virStorageBackendSCSIStopPool(virConnectPtr conn ATTRIBUTE_UNUSED, +virStorageBackendSCSIStopPool(virConnectPtr conn, virStoragePoolObjPtr pool) { virStoragePoolSourceAdapter adapter = pool->def->source.adapter; - return deleteVport(adapter); + return deleteVport(conn, adapter); } virStorageBackend virStorageBackendSCSI = { diff --git a/tests/storagepoolxml2xmlin/pool-scsi-type-fc-host-managed.xml b/tests/storagepoolxml2xmlin/pool-scsi-type-fc-host-managed.xml new file mode 100644 index 0000000..43611ee --- /dev/null +++ b/tests/storagepoolxml2xmlin/pool-scsi-type-fc-host-managed.xml @@ -0,0 +1,15 @@ +<pool type='scsi'> + <name>hba0</name> + <uuid>e9392370-2917-565e-692b-d057f46512d6</uuid> + <source> + <adapter type='fc_host' parent='scsi_host5' managed='yes' wwnn='20000000c9831b4b' wwpn='10000000c9831b4b'/> + </source> + <target> + <path>/dev/disk/by-path</path> + <permissions> + <mode>0700</mode> + <owner>0</owner> + <group>0</group> + </permissions> + </target> +</pool> diff --git a/tests/storagepoolxml2xmlout/pool-scsi-type-fc-host-managed.xml b/tests/storagepoolxml2xmlout/pool-scsi-type-fc-host-managed.xml new file mode 100644 index 0000000..c8bb0db --- /dev/null +++ b/tests/storagepoolxml2xmlout/pool-scsi-type-fc-host-managed.xml @@ -0,0 +1,18 @@ +<pool type='scsi'> + <name>hba0</name> + <uuid>e9392370-2917-565e-692b-d057f46512d6</uuid> + <capacity unit='bytes'>0</capacity> + <allocation unit='bytes'>0</allocation> + <available unit='bytes'>0</available> + <source> + <adapter type='fc_host' parent='scsi_host5' managed='yes' wwnn='20000000c9831b4b' wwpn='10000000c9831b4b'/> + </source> + <target> + <path>/dev/disk/by-path</path> + <permissions> + <mode>0700</mode> + <owner>0</owner> + <group>0</group> + </permissions> + </target> +</pool> diff --git a/tests/storagepoolxml2xmltest.c b/tests/storagepoolxml2xmltest.c index 8a2c0b5..52e2193 100644 --- a/tests/storagepoolxml2xmltest.c +++ b/tests/storagepoolxml2xmltest.c @@ -98,6 +98,7 @@ mymain(void) DO_TEST("pool-scsi"); DO_TEST("pool-scsi-type-scsi-host"); DO_TEST("pool-scsi-type-fc-host"); + DO_TEST("pool-scsi-type-fc-host-managed"); DO_TEST("pool-mpath"); DO_TEST("pool-iscsi-multiiqn"); DO_TEST("pool-iscsi-vendor-product"); -- 1.9.3
On 11/10/2014 05:45 PM, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1160926
Introduce a 'managed' attribute to allow libvirt to decide whether to delete a vHBA vport created via external means such as nodedev-create. The code currently decides whether to delete the vHBA based solely on whether the parent was provided at creation time. However, that may not be the desired action, so rather than delete and force someone to create another vHBA via an additional nodedev-create allow the configuration of the storage pool to decide the desired action.
During createVport when libvirt does the VPORT_CREATE, set the managed value to YES if not already set to indicate to the deleteVport code that it should delete the vHBA when the pool is destroyed.
If libvirtd is restarted all the memory only state was lost, so for a persistent storage pool, use the virStoragePoolSaveConfig in order to write out the managed value.
Because we're now saving the current configuration, we need to be sure to not save the parent in the output XML if it was undefined at start. Saving the name would cause future starts to always use the same parent which is not the expected result when not providing a parent. By not providing a parent, libvirt is expected to find the best available vHBA port for each subsequent (re)start.
At deleteVport, use the new managed value to decide whether to execute the VPORT_DELETE. Since we no longer save the parent in memory or in XML when provided, if it was not provided, then we have to look it up.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatstorage.html.in | 13 +++ docs/schemas/basictypes.rng | 5 ++ src/conf/storage_conf.c | 17 ++++ src/conf/storage_conf.h | 1 + src/storage/storage_backend_scsi.c | 93 +++++++++++++++++----- .../pool-scsi-type-fc-host-managed.xml | 15 ++++ .../pool-scsi-type-fc-host-managed.xml | 18 +++++ tests/storagepoolxml2xmltest.c | 1 + 8 files changed, 143 insertions(+), 20 deletions(-) create mode 100644 tests/storagepoolxml2xmlin/pool-scsi-type-fc-host-managed.xml create mode 100644 tests/storagepoolxml2xmlout/pool-scsi-type-fc-host-managed.xml
<...snip...> Consider the following squished into deleteVport as testing asked about a condition where someone does a 'virsh nodedev-destroy' on the vHBA we've created resulting in the virGetFCHostNameByWWN() rightfully returning NULL (just like it would in the createVport case when the wwnn/wwpn doesn't exist). Allowing virsh nodedev-destroy to remove an "active" vHBA is perhaps a different issue... The desire was to get a real error message instead of: destroy the 'fc_host' pool # virsh pool-destroy fc-pool error: Failed to destroy pool fc-pool error: An error occurred, but the cause is unknown #
diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index 41ea67a..b1602ea 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c <...snip...> static int -deleteVport(virStoragePoolSourceAdapter adapter) +deleteVport(virConnectPtr conn, + virStoragePoolSourceAdapter adapter) { unsigned int parent_host; char *name = NULL; + char *vhba_parent = NULL; int ret = -1;
if (adapter.type != VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) return 0;
- /* It must be a HBA instead of a vHBA as long as "parent" - * is NULL. "createVport" guaranteed "parent" for a vHBA - * cannot be NULL, it's either specified in XML, or detected - * automatically. - */ - if (!adapter.data.fchost.parent) + VIR_DEBUG("conn=%p parent='%s', managed='%d' wwnn='%s' wwpn='%s'", + conn, NULLSTR(adapter.data.fchost.parent), + adapter.data.fchost.managed, + adapter.data.fchost.wwnn, + adapter.data.fchost.wwpn); + + /* If we're not managing the deletion of the vHBA, then just return */ + if (adapter.data.fchost.managed != VIR_TRISTATE_BOOL_YES) return 0;
+ /* Find our vHBA by searching the fc_host sysfs tree for our wwnn/wwpn */ if (!(name = virGetFCHostNameByWWN(NULL, adapter.data.fchost.wwnn, adapter.data.fchost.wwpn))) - return -1; - - if (virGetSCSIHostNumber(adapter.data.fchost.parent, &parent_host) < 0) goto cleanup;
diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_sc index b1602ea..3f61610 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -764,8 +764,12 @@ deleteVport(virConnectPtr conn, /* Find our vHBA by searching the fc_host sysfs tree for our wwnn/wwpn */ if (!(name = virGetFCHostNameByWWN(NULL, adapter.data.fchost.wwnn, - adapter.data.fchost.wwpn))) + adapter.data.fchost.wwpn))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to find fc_host for wwnn='%s' and wwpn='%s'"), + adapter.data.fchost.wwnn, adapter.data.fchost.wwpn); goto cleanup; + } /* If at startup time we provided a parent, then use that to * get the parent_host value; otherwise, we have to determine
+ /* If at startup time we provided a parent, then use that to + * get the parent_host value; otherwise, we have to determine + * the parent scsi_host which we did not save at startup time + */ + if (adapter.data.fchost.parent) { + if (virGetSCSIHostNumber(adapter.data.fchost.parent, &parent_host) < 0) + goto cleanup; + } else { + if (!(vhba_parent = getVhbaSCSIHostParent(conn, name))) + goto cleanup; + + if (virGetSCSIHostNumber(vhba_parent, &parent_host) < 0) + goto cleanup; + } + if (virManageVport(parent_host, adapter.data.fchost.wwpn, adapter.data.fchost.wwnn, VPORT_DELETE) < 0) goto cleanup; @@ -737,6 +789,7 @@ deleteVport(virStoragePoolSourceAdapter adapter) ret = 0; cleanup: VIR_FREE(name); + VIR_FREE(vhba_parent); return ret; }
<...snip...>
On 10.11.2014 23:45, John Ferlan wrote:
This series will replace:
http://www.redhat.com/archives/libvir-list/2014-November/msg00197.html
There are two bugs being fixed here and having them reviewed together makes things easier in the long run as the last 3 patches depended upon the first 2 being present.
https://bugzilla.redhat.com/show_bug.cgi?id=1160565
Patch 1 is as was in the previous set Patch 2 is essentially the same, except the single function checkVhbaSCSIHostParent was split out to generate a getVhbaSCSIHostParent which gets used later on.
https://bugzilla.redhat.com/show_bug.cgi?id=1160926
Patch 3 fixes an issue which took a bit of gdb time in order to figure out exactly what was going wrong. Essentially, the exising createVport code had a path which would find the "best available" fc_host to use and save the parent on return so that the deleteVport code would delete the parent. However, the code used a copy of the adapter not a reference to the adapter and thus the change was lost leaving the vHBA on the system.
Patch 4 adds a new function to save the StoragePool XML given a configFile. This will be used in the next patch because while having the in memory fchost copy updated does work - if libvirtd dies in between we're back at square 1 reading storage pool config files and not knowing whence we started.
Patch 5 adds a new "managed" attribute to the fchost XML. It does this mainly to let the deleteVport know when it should delete the self created vport. Prior to this code, the reliance was that we didn't have a parent provided. However, this causes an issue where if someone used nodedev-create in order to create a vHBA and then tried to use that vHBA in a storage pool we would delete that vHBA when we were done, which may not be expected. Using the managed attribute at creation time will let libvirt know what to do in this case.
NOTE: There's still one more issue in the code, but it's a bit trickier to resolve. A libvirt created vport doesn't seem to want to find the LUN's on the initial PoolRefresh that occurs during startup. However, if one does a refresh immediately after starting the pool, the luns show up. It seems perhaps there's some sort of locking issue that won't allow the udevEventHandleCallback to 'add' the new device.
John Ferlan (5): storage: Check for valid fc_host parent at startup storage: Ensure fc_host parent matches wwnn/wwpn storage: Don't use a stack copy of the adapter storage: Introduce virStoragePoolSaveConfig storage: Introduce 'managed' for the fchost parent
docs/formatstorage.html.in | 28 ++- docs/schemas/basictypes.rng | 5 + src/conf/storage_conf.c | 70 ++++-- src/conf/storage_conf.h | 4 + src/libvirt_private.syms | 1 + src/storage/storage_backend_scsi.c | 237 ++++++++++++++++++--- .../pool-scsi-type-fc-host-managed.xml | 15 ++ .../pool-scsi-type-fc-host-managed.xml | 18 ++ tests/storagepoolxml2xmltest.c | 1 + 9 files changed, 323 insertions(+), 56 deletions(-) create mode 100644 tests/storagepoolxml2xmlin/pool-scsi-type-fc-host-managed.xml create mode 100644 tests/storagepoolxml2xmlout/pool-scsi-type-fc-host-managed.xml
ACK series Michal
On 11/12/2014 09:31 AM, Michal Privoznik wrote:
On 10.11.2014 23:45, John Ferlan wrote:
John Ferlan (5): storage: Check for valid fc_host parent at startup storage: Ensure fc_host parent matches wwnn/wwpn storage: Don't use a stack copy of the adapter storage: Introduce virStoragePoolSaveConfig storage: Introduce 'managed' for the fchost parent
docs/formatstorage.html.in | 28 ++- docs/schemas/basictypes.rng | 5 + src/conf/storage_conf.c | 70 ++++-- src/conf/storage_conf.h | 4 + src/libvirt_private.syms | 1 + src/storage/storage_backend_scsi.c | 237 ++++++++++++++++++--- .../pool-scsi-type-fc-host-managed.xml | 15 ++ .../pool-scsi-type-fc-host-managed.xml | 18 ++ tests/storagepoolxml2xmltest.c | 1 + 9 files changed, 323 insertions(+), 56 deletions(-) create mode 100644 tests/storagepoolxml2xmlin/pool-scsi-type-fc-host-managed.xml create mode 100644 tests/storagepoolxml2xmlout/pool-scsi-type-fc-host-managed.xml
ACK series
Michal
Pushed - thanks! John
participants (2)
-
John Ferlan -
Michal Privoznik