4:45 p.m.
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
4:45 p.m.
New subject: [libvirt] [PATCH v2 1/5] storage: Check for valid fc_host parent at startup
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>
---
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
6:21 a.m.
New subject: [libvirt] [PATCH v2 1/5] storage: Check for valid fc_host parent at startup
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>
---
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
6:38 a.m.
New subject: [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>
> ---
> 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
9:05 a.m.
New subject: [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>
>> ---
>> 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
9:21 a.m.
New subject: [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>
>>> ---
>>> 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;
8:31 a.m.
New subject: [libvirt] [PATCH v2 1/5] storage: Check for valid fc_host parent at startup
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(a)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
4:45 p.m.
New subject: [libvirt] [PATCH v2 2/5] storage: Ensure fc_host parent matches wwnn/wwpn
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(a)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<...
- 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
4:45 p.m.
New subject: [libvirt] [PATCH v2 3/5] storage: Don't use a stack copy of the adapter
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(a)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
4:45 p.m.
New subject: [libvirt] [PATCH v2 4/5] storage: Introduce virStoragePoolSaveConfig
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(a)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
4:45 p.m.
New subject: [libvirt] [PATCH v2 5/5] storage: Introduce 'managed' for the fchost parent
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(a)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
6:06 a.m.
New subject: [libvirt] [PATCH v2 5/5] storage: Introduce 'managed' for the fchost parent
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(a)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...>
8:31 a.m.
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
9:53 a.m.
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
3700
days inactive
3702
days old
13 comments
2 participants
participants (2)
-
John Ferlan -
Michal Privoznik