4:35 p.m.
https://bugzilla.redhat.com/show_bug.cgi?id=1146837
Not for 1.2.9, but may as well get it in the queue to be reviewed for
post 1.2.9
Although possible to do in one patch - I figured it'd be easier to
review if shown in steps.
Essentially the problem is the scsi_host duplicate checks were causing
a libvirtd crash with the parentaddr/unique_id code. During my investigation
I determined that there was another lurking issue with the two ways in
which the 'name=' property is allowed to be either 'host#' or
'scsi_host#'.
John Ferlan (4):
storage_conf: Create function to perform scsi_host dupe check
storage_conf: Refactor arguments to matchSCSIAdapter
storage_conf: Fix the scsi_host.name comparison
storage_conf: Resolve libvirtd crash matching scsi_host
src/conf/storage_conf.c | 111 ++++++++++++++++++++++++++++++++++++++++++------
1 file changed, 97 insertions(+), 14 deletions(-)
--
1.9.3
4:35 p.m.
New subject: [libvirt] [PATCH 1/4] storage_conf: Create function to perform scsi_host dupe check
Create local matchSCSIAdapter() function to handle the duplicate
adapter check. Future patches will expand upon the checks.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/storage_conf.c | 22 ++++++++++++++--------
1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 36696a4..19b6589 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -2084,6 +2084,18 @@ matchSCSIAdapterParent(virStoragePoolObjPtr pool,
return false;
}
+static bool
+matchSCSIAdapter(virStoragePoolObjPtr pool,
+ virStoragePoolDefPtr def)
+{
+ if (pool->def->source.adapter.data.scsi_host.name) {
+ return STREQ(pool->def->source.adapter.data.scsi_host.name,
+ def->source.adapter.data.scsi_host.name);
+ } else {
+ return matchSCSIAdapterParent(pool, def);
+ }
+}
+
int
virStoragePoolSourceFindDuplicate(virStoragePoolObjListPtr pools,
virStoragePoolDefPtr def)
@@ -2130,14 +2142,8 @@ virStoragePoolSourceFindDuplicate(virStoragePoolObjListPtr pools,
VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST &&
def->source.adapter.type ==
VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) {
- if (pool->def->source.adapter.data.scsi_host.name) {
- if (STREQ(pool->def->source.adapter.data.scsi_host.name,
- def->source.adapter.data.scsi_host.name))
- matchpool = pool;
- } else {
- if (matchSCSIAdapterParent(pool, def))
- matchpool = pool;
- }
+ if (matchSCSIAdapter(pool, def))
+ matchpool = pool;
}
break;
case VIR_STORAGE_POOL_ISCSI:
--
1.9.3
4:35 p.m.
New subject: [libvirt] [PATCH 2/4] storage_conf: Refactor arguments to matchSCSIAdapter
Rather than pass the virStoragePoolObjPtr and virStoragePoolDefPtr
for the source adapter checks - just pass the virStoragePoolSourceAdapter's
Cuts down on typing and makes future patches a bit easier
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/storage_conf.c | 27 ++++++++++++++-------------
1 file changed, 14 insertions(+), 13 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 19b6589..ac41307 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -2063,17 +2063,17 @@ virStoragePoolObjIsDuplicate(virStoragePoolObjListPtr pools,
}
static bool
-matchSCSIAdapterParent(virStoragePoolObjPtr pool,
- virStoragePoolDefPtr def)
+matchSCSIAdapterParent(virStoragePoolSourceAdapter poolAdapter,
+ virStoragePoolSourceAdapter defAdapter)
{
virDevicePCIAddressPtr pooladdr =
- &pool->def->source.adapter.data.scsi_host.parentaddr;
+ &poolAdapter.data.scsi_host.parentaddr;
virDevicePCIAddressPtr defaddr =
- &def->source.adapter.data.scsi_host.parentaddr;
+ &defAdapter.data.scsi_host.parentaddr;
int pool_unique_id =
- pool->def->source.adapter.data.scsi_host.unique_id;
+ poolAdapter.data.scsi_host.unique_id;
int def_unique_id =
- def->source.adapter.data.scsi_host.unique_id;
+ defAdapter.data.scsi_host.unique_id;
if (pooladdr->domain == defaddr->domain &&
pooladdr->bus == defaddr->bus &&
pooladdr->slot == defaddr->slot &&
@@ -2085,14 +2085,14 @@ matchSCSIAdapterParent(virStoragePoolObjPtr pool,
}
static bool
-matchSCSIAdapter(virStoragePoolObjPtr pool,
- virStoragePoolDefPtr def)
+matchSCSIAdapter(virStoragePoolSourceAdapter poolAdapter,
+ virStoragePoolSourceAdapter defAdapter)
{
- if (pool->def->source.adapter.data.scsi_host.name) {
- return STREQ(pool->def->source.adapter.data.scsi_host.name,
- def->source.adapter.data.scsi_host.name);
+ if (poolAdapter.data.scsi_host.name) {
+ return STREQ(poolAdapter.data.scsi_host.name,
+ defAdapter.data.scsi_host.name);
} else {
- return matchSCSIAdapterParent(pool, def);
+ return matchSCSIAdapterParent(poolAdapter, defAdapter);
}
}
@@ -2142,7 +2142,8 @@ virStoragePoolSourceFindDuplicate(virStoragePoolObjListPtr pools,
VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST &&
def->source.adapter.type ==
VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) {
- if (matchSCSIAdapter(pool, def))
+ if (matchSCSIAdapter(pool->def->source.adapter,
+ def->source.adapter))
matchpool = pool;
}
break;
--
1.9.3
4:35 p.m.
New subject: [libvirt] [PATCH 3/4] storage_conf: Fix the scsi_host.name comparison
Since the 'scsi_host#' concept was introduced in commit id '9f781da6' it
is possible to provide either "name='host#'" or
"name='scsi_host#'" to
define/name the scsi_host adapter source address. The concept being
that 'name#' is/was legacy and 'scsi_host#' is what's seen in the
'virsh nodedev-list [--cap scsi_host]' output. However, in doing the
comparison for a to be defined scsi_host, a latent bug allows the same
source to be defined twice, e.g. "name='host5'" is not distiguished
from "name='scsi_host5'", although in reality they eventually become
the same thing in commit id 'c1f63a9b' with the introduction of the
getHostNumber() API.
So this change will ensure that no one can use 'host5' and 'scsi_host5'
(or whatever #) to resolve to the same source adapter when going through
the define the source adapter processing and checking for duplicates. Doing
so will now result in an error (assuming that an existing pool is using
'host5' and the to be defined pool tries 'scsi_host5'):
$ virsh pool-define scsi-pool-use-scsi_host.xml
error: Failed to define pool from scsi-pool-use-scsi_host.xml
error: operation failed: Storage source conflict with pool: 'scsi-pool-use-host
$
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/storage_conf.c | 25 +++++++++++++++++++++++--
1 file changed, 23 insertions(+), 2 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index ac41307..74267bd 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -2063,6 +2063,27 @@ virStoragePoolObjIsDuplicate(virStoragePoolObjListPtr pools,
}
static bool
+matchSCSIAdapterName(const char *pool_name,
+ const char *def_name)
+{
+ /* Names can be either "scsi_host#" or just "host#", where
+ * "host#" is the back-compat format, but both equate to
+ * the same source adapter. First check if both pool and def
+ * are using same format (easier) - if so, then compare
+ */
+ if ((STRPREFIX(pool_name, "scsi_") && STRPREFIX(def_name,
"scsi")) ||
+ (STRPREFIX(pool_name, "host") && STRPREFIX(def_name,
"host")))
+ return STREQ(pool_name, def_name);
+
+ /* If pool uses "scsi_host#" and def uses "host#", deal with that
here */
+ if (STRPREFIX(pool_name, "scsi_"))
+ return STREQ(&pool_name[5], def_name);
+
+ /* Otherwise we have pool with "host#" and def with "scsi_host#"
*/
+ return STREQ(pool_name, &def_name[5]);
+}
+
+static bool
matchSCSIAdapterParent(virStoragePoolSourceAdapter poolAdapter,
virStoragePoolSourceAdapter defAdapter)
{
@@ -2089,8 +2110,8 @@ matchSCSIAdapter(virStoragePoolSourceAdapter poolAdapter,
virStoragePoolSourceAdapter defAdapter)
{
if (poolAdapter.data.scsi_host.name) {
- return STREQ(poolAdapter.data.scsi_host.name,
- defAdapter.data.scsi_host.name);
+ return matchSCSIAdapterName(poolAdapter.data.scsi_host.name,
+ defAdapter.data.scsi_host.name);
} else {
return matchSCSIAdapterParent(poolAdapter, defAdapter);
}
--
1.9.3
8:20 a.m.
New subject: [libvirt] [PATCH 3/4] storage_conf: Fix the scsi_host.name comparison
On 09/30/2014 11:35 PM, John Ferlan wrote:
Since the 'scsi_host#' concept was introduced in commit id
'9f781da6' it
is possible to provide either "name='host#'" or
"name='scsi_host#'" to
define/name the scsi_host adapter source address. The concept being
that 'name#' is/was legacy and 'scsi_host#' is what's seen in the
'virsh nodedev-list [--cap scsi_host]' output. However, in doing the
comparison for a to be defined scsi_host, a latent bug allows the same
source to be defined twice, e.g. "name='host5'" is not distiguished
from "name='scsi_host5'", although in reality they eventually become
the same thing in commit id 'c1f63a9b' with the introduction of the
getHostNumber() API.
So this change will ensure that no one can use 'host5' and 'scsi_host5'
(or whatever #) to resolve to the same source adapter when going through
the define the source adapter processing and checking for duplicates. Doing
so will now result in an error (assuming that an existing pool is using
'host5' and the to be defined pool tries 'scsi_host5'):
$ virsh pool-define scsi-pool-use-scsi_host.xml
error: Failed to define pool from scsi-pool-use-scsi_host.xml
error: operation failed: Storage source conflict with pool: 'scsi-pool-use-host
$
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/storage_conf.c | 25 +++++++++++++++++++++++--
1 file changed, 23 insertions(+), 2 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index ac41307..74267bd 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -2063,6 +2063,27 @@ virStoragePoolObjIsDuplicate(virStoragePoolObjListPtr pools,
}
static bool
+matchSCSIAdapterName(const char *pool_name,
+ const char *def_name)
+{
+ /* Names can be either "scsi_host#" or just "host#", where
+ * "host#" is the back-compat format, but both equate to
+ * the same source adapter. First check if both pool and def
+ * are using same format (easier) - if so, then compare
+ */
+ if ((STRPREFIX(pool_name, "scsi_") && STRPREFIX(def_name,
"scsi")) ||
+ (STRPREFIX(pool_name, "host") && STRPREFIX(def_name,
"host")))
+ return STREQ(pool_name, def_name);
+
+ /* If pool uses "scsi_host#" and def uses "host#", deal with
that here */
+ if (STRPREFIX(pool_name, "scsi_"))
+ return STREQ(&pool_name[5], def_name);
+
+ /* Otherwise we have pool with "host#" and def with "scsi_host#"
*/
+ return STREQ(pool_name, &def_name[5]);
+}
fc_host prefix is not handled here, but getHostNumber will allow it. Maybe the
checks should be shared? (as long as we don't error out on unknown prefixes,
since we didn't validate the adapter name in the past).
Jan
6:03 a.m.
New subject: [libvirt] [PATCH 3/4] storage_conf: Fix the scsi_host.name comparison
On 10/03/2014 09:20 AM, Ján Tomko wrote:
On 09/30/2014 11:35 PM, John Ferlan wrote:
> Since the 'scsi_host#' concept was introduced in commit id '9f781da6'
it
> is possible to provide either "name='host#'" or
"name='scsi_host#'" to
> define/name the scsi_host adapter source address. The concept being
> that 'name#' is/was legacy and 'scsi_host#' is what's seen in
the
> 'virsh nodedev-list [--cap scsi_host]' output. However, in doing the
> comparison for a to be defined scsi_host, a latent bug allows the same
> source to be defined twice, e.g. "name='host5'" is not
distiguished
> from "name='scsi_host5'", although in reality they eventually
become
> the same thing in commit id 'c1f63a9b' with the introduction of the
> getHostNumber() API.
>
> So this change will ensure that no one can use 'host5' and
'scsi_host5'
> (or whatever #) to resolve to the same source adapter when going through
> the define the source adapter processing and checking for duplicates. Doing
> so will now result in an error (assuming that an existing pool is using
> 'host5' and the to be defined pool tries 'scsi_host5'):
>
> $ virsh pool-define scsi-pool-use-scsi_host.xml
> error: Failed to define pool from scsi-pool-use-scsi_host.xml
> error: operation failed: Storage source conflict with pool: 'scsi-pool-use-host
> $
>
> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
> ---
> src/conf/storage_conf.c | 25 +++++++++++++++++++++++--
> 1 file changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
> index ac41307..74267bd 100644
> --- a/src/conf/storage_conf.c
> +++ b/src/conf/storage_conf.c
> @@ -2063,6 +2063,27 @@ virStoragePoolObjIsDuplicate(virStoragePoolObjListPtr pools,
> }
>
> static bool
> +matchSCSIAdapterName(const char *pool_name,
> + const char *def_name)
> +{
> + /* Names can be either "scsi_host#" or just "host#", where
> + * "host#" is the back-compat format, but both equate to
> + * the same source adapter. First check if both pool and def
> + * are using same format (easier) - if so, then compare
> + */
> + if ((STRPREFIX(pool_name, "scsi_") && STRPREFIX(def_name,
"scsi")) ||
> + (STRPREFIX(pool_name, "host") && STRPREFIX(def_name,
"host")))
> + return STREQ(pool_name, def_name);
> +
> + /* If pool uses "scsi_host#" and def uses "host#", deal with
that here */
> + if (STRPREFIX(pool_name, "scsi_"))
> + return STREQ(&pool_name[5], def_name);
> +
> + /* Otherwise we have pool with "host#" and def with
"scsi_host#" */
> + return STREQ(pool_name, &def_name[5]);
> +}
fc_host prefix is not handled here, but getHostNumber will allow it. Maybe the
checks should be shared? (as long as we don't error out on unknown prefixes,
since we didn't validate the adapter name in the past).
Not clear what kind of sharing would be expected (perhaps it's my code
myopia)... The previous (and current to this patch) code does validate
the name - at least to the degree that the incoming name isn't already
in use or the name that the incoming definition would resolve to in the
case of parentaddr. It is broken - which is what this set of patches
looks to resolve.
If you go back to patch 1/4 - you will see for a "type='scsi_host'"
pool
we'd previously either simply match the incoming name against name of
the pool (assuming the the incoming def had a name instead of
parentaddr) or we'd match the parentaddr (assuming that if the current
pool def was using a parentaddr that the incoming def would be too).
All this patch does is ensure that someone cannot provide
"name='host3'"
for one pool while providing "name='scsi_host3'" for another pool for
the Create/Define (or vice versa). There is no bug on this - I just
noted this while working on the code.
matchSCSIAdapter[Name|Parent] is called during the Create/Define pool
processing to ensure we don't allow user defined duplicate names of
existing pools for SCSI_HOST pools only (eg, type='scsi_host' instead of
type='fc_host'). A FC_HOST pool would disallow matches for the unique
wwnn/wwpn pairs. Yes it does use "parent='scsi_host#'" as a name, but
that's only to find the scsi_host# defined - see
http://wiki.libvirt.org/page/NPIV_in_libvirt during the start or refresh
processing (in getHostNumber).
The getHostNumber is used by the scsi pool driver during the Check or
Refresh processing in order to fetch which user provided name that was
created/defined for use in finding the on disk
/sys/class/scsi_host/host# directory in order to find either the fc_host
or scsi_host data. The fc_host processing has/uses a
"parent='scsi_host#'" in order to define the vHBA with the the vport
(wwnn/wwpn). I'm not even sure at this point if fc_host could be a
proper prefix, but that's a different issue.
I can rename this set of functions to something like
matchSCSIHostAdapterType[Name|Parent] if it's clearer, but I guess I'm
missing the synergy.
John
7:45 a.m.
New subject: [libvirt] [PATCH 3/4] storage_conf: Fix the scsi_host.name comparison
On 10/06/2014 01:03 PM, John Ferlan wrote:
On 10/03/2014 09:20 AM, Ján Tomko wrote:
> On 09/30/2014 11:35 PM, John Ferlan wrote:
>> Since the 'scsi_host#' concept was introduced in commit id
'9f781da6' it
>> is possible to provide either "name='host#'" or
"name='scsi_host#'" to
>> define/name the scsi_host adapter source address. The concept being
>> that 'name#' is/was legacy and 'scsi_host#' is what's seen in
the
>> 'virsh nodedev-list [--cap scsi_host]' output. However, in doing the
>> comparison for a to be defined scsi_host, a latent bug allows the same
>> source to be defined twice, e.g. "name='host5'" is not
distiguished
>> from "name='scsi_host5'", although in reality they eventually
become
>> the same thing in commit id 'c1f63a9b' with the introduction of the
>> getHostNumber() API.
>>
>> So this change will ensure that no one can use 'host5' and
'scsi_host5'
>> (or whatever #) to resolve to the same source adapter when going through
>> the define the source adapter processing and checking for duplicates. Doing
>> so will now result in an error (assuming that an existing pool is using
>> 'host5' and the to be defined pool tries 'scsi_host5'):
>>
>> $ virsh pool-define scsi-pool-use-scsi_host.xml
>> error: Failed to define pool from scsi-pool-use-scsi_host.xml
>> error: operation failed: Storage source conflict with pool:
'scsi-pool-use-host
>> $
>>
>> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
>> ---
>> src/conf/storage_conf.c | 25 +++++++++++++++++++++++--
>> 1 file changed, 23 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
>> index ac41307..74267bd 100644
>> --- a/src/conf/storage_conf.c
>> +++ b/src/conf/storage_conf.c
>> @@ -2063,6 +2063,27 @@ virStoragePoolObjIsDuplicate(virStoragePoolObjListPtr
pools,
>> }
>>
>> static bool
>> +matchSCSIAdapterName(const char *pool_name,
>> + const char *def_name)
>> +{
>> + /* Names can be either "scsi_host#" or just "host#",
where
>> + * "host#" is the back-compat format, but both equate to
>> + * the same source adapter. First check if both pool and def
>> + * are using same format (easier) - if so, then compare
>> + */
>> + if ((STRPREFIX(pool_name, "scsi_") && STRPREFIX(def_name,
"scsi")) ||
>> + (STRPREFIX(pool_name, "host") && STRPREFIX(def_name,
"host")))
>> + return STREQ(pool_name, def_name);
>> +
>> + /* If pool uses "scsi_host#" and def uses "host#", deal
with that here */
>> + if (STRPREFIX(pool_name, "scsi_"))
>> + return STREQ(&pool_name[5], def_name);
>> +
>> + /* Otherwise we have pool with "host#" and def with
"scsi_host#" */
>> + return STREQ(pool_name, &def_name[5]);
>> +}
>
> fc_host prefix is not handled here, but getHostNumber will allow it. Maybe the
> checks should be shared? (as long as we don't error out on unknown prefixes,
> since we didn't validate the adapter name in the past).
>
Not clear what kind of sharing would be expected (perhaps it's my code
myopia)...
Calling getHostNumber on both pool_name and def_name and comparing the result,
or splitting out the part skipping the prefixes into something in util/virscsi.c.
The previous (and current to this patch) code does validate
the name - at least to the degree that the incoming name isn't already
in use or the name that the incoming definition would resolve to in the
case of parentaddr. It is broken - which is what this set of patches
looks to resolve.
Currently, we don't resolve the parentaddr, just compare it to other addresses.
If you go back to patch 1/4 - you will see for a "type='scsi_host'"
pool
we'd previously either simply match the incoming name against name of
the pool (assuming the the incoming def had a name instead of
parentaddr) or we'd match the parentaddr (assuming that if the current
pool def was using a parentaddr that the incoming def would be too).
All this patch does is ensure that someone cannot provide
"name='host3'"
for one pool while providing "name='scsi_host3'" for another pool for
the Create/Define (or vice versa). There is no bug on this - I just
noted this while working on the code.
matchSCSIAdapter[Name|Parent] is called during the Create/Define pool
processing to ensure we don't allow user defined duplicate names of
existing pools for SCSI_HOST pools only (eg, type='scsi_host' instead of
type='fc_host'). A FC_HOST pool would disallow matches for the unique
wwnn/wwpn pairs. Yes it does use "parent='scsi_host#'" as a name, but
that's only to find the scsi_host# defined - see
http://wiki.libvirt.org/page/NPIV_in_libvirt during the start or refresh
processing (in getHostNumber).
The getHostNumber is used by the scsi pool driver during the Check or
Refresh processing in order to fetch which user provided name that was
created/defined for use in finding the on disk
/sys/class/scsi_host/host# directory in order to find either the fc_host
or scsi_host data. The fc_host processing has/uses a
"parent='scsi_host#'" in order to define the vHBA with the the vport
(wwnn/wwpn). I'm not even sure at this point if fc_host could be a
proper prefix, but that's a different issue.
I haven't actually tried it, but from looking at the code, for a storage pool
with VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST name='fc_host3' would also
be duplicate with name='host3' and name='scsi_host3'. The name is not
validated on definition and the fc_host prefix will be stripped (just as
scsi_host or host) in getHostNumber.
Jan
I can rename this set of functions to something like
matchSCSIHostAdapterType[Name|Parent] if it's clearer, but I guess I'm
missing the synergy.
John
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
4:11 p.m.
New subject: [libvirt] [PATCH 3/4] storage_conf: Fix the scsi_host.name comparison
On 10/06/2014 08:45 AM, Ján Tomko wrote:
On 10/06/2014 01:03 PM, John Ferlan wrote:
<...snip...>
>>>
>>> static bool
>>> +matchSCSIAdapterName(const char *pool_name,
>>> + const char *def_name)
>>> +{
>>> + /* Names can be either "scsi_host#" or just "host#",
where
>>> + * "host#" is the back-compat format, but both equate to
>>> + * the same source adapter. First check if both pool and def
>>> + * are using same format (easier) - if so, then compare
>>> + */
>>> + if ((STRPREFIX(pool_name, "scsi_") &&
STRPREFIX(def_name, "scsi")) ||
>>> + (STRPREFIX(pool_name, "host") &&
STRPREFIX(def_name, "host")))
>>> + return STREQ(pool_name, def_name);
>>> +
>>> + /* If pool uses "scsi_host#" and def uses "host#",
deal with that here */
>>> + if (STRPREFIX(pool_name, "scsi_"))
>>> + return STREQ(&pool_name[5], def_name);
>>> +
>>> + /* Otherwise we have pool with "host#" and def with
"scsi_host#" */
>>> + return STREQ(pool_name, &def_name[5]);
>>> +}
>>
>> fc_host prefix is not handled here, but getHostNumber will allow it. Maybe the
>> checks should be shared? (as long as we don't error out on unknown prefixes,
>> since we didn't validate the adapter name in the past).
>>
>
> Not clear what kind of sharing would be expected (perhaps it's my code
> myopia)...
Calling getHostNumber on both pool_name and def_name and comparing the result,
or splitting out the part skipping the prefixes into something in util/virscsi.c.
Hmm.. true... Although similar other SCSI_HOST and FC_HOST functions
are in util/virutil.c
> The previous (and current to this patch) code does validate
> the name - at least to the degree that the incoming name isn't already
> in use or the name that the incoming definition would resolve to in the
> case of parentaddr. It is broken - which is what this set of patches
> looks to resolve.
Currently, we don't resolve the parentaddr, just compare it to other addresses.
yeah and this makes the getHostNumber a bit more tricky - especially
with respect to virDevicePCIAddress which when added to virutil.{c,h}
created a mess
>
> If you go back to patch 1/4 - you will see for a "type='scsi_host'"
pool
> we'd previously either simply match the incoming name against name of
> the pool (assuming the the incoming def had a name instead of
> parentaddr) or we'd match the parentaddr (assuming that if the current
> pool def was using a parentaddr that the incoming def would be too).
>
> All this patch does is ensure that someone cannot provide
"name='host3'"
> for one pool while providing "name='scsi_host3'" for another pool
for
> the Create/Define (or vice versa). There is no bug on this - I just
> noted this while working on the code.
>
> matchSCSIAdapter[Name|Parent] is called during the Create/Define pool
> processing to ensure we don't allow user defined duplicate names of
> existing pools for SCSI_HOST pools only (eg, type='scsi_host' instead of
> type='fc_host'). A FC_HOST pool would disallow matches for the unique
> wwnn/wwpn pairs. Yes it does use "parent='scsi_host#'" as a name,
but
> that's only to find the scsi_host# defined - see
> http://wiki.libvirt.org/page/NPIV_in_libvirt during the start or refresh
> processing (in getHostNumber).
>
> The getHostNumber is used by the scsi pool driver during the Check or
> Refresh processing in order to fetch which user provided name that was
> created/defined for use in finding the on disk
> /sys/class/scsi_host/host# directory in order to find either the fc_host
> or scsi_host data. The fc_host processing has/uses a
> "parent='scsi_host#'" in order to define the vHBA with the the
vport
> (wwnn/wwpn). I'm not even sure at this point if fc_host could be a
> proper prefix, but that's a different issue.
>
I haven't actually tried it, but from looking at the code, for a storage pool
with VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST name='fc_host3' would also
be duplicate with name='host3' and name='scsi_host3'. The name is not
validated on definition and the fc_host prefix will be stripped (just as
scsi_host or host) in getHostNumber.
In any case, I see what you're driving at - I'm reworking the patches
and will post a new series shortly...
John
FWIW:
It seems commit id 'b52fbad1' (interesting sequence for hash id :-))
should never have allowed 'fc_host' as a value for the name property.
Oh well, what's done is done I guess.
4:35 p.m.
New subject: [libvirt] [PATCH 4/4] storage_conf: Resolve libvirtd crash matching scsi_host
https://bugzilla.redhat.com/show_bug.cgi?id=1146837
Resolve a crash in libvirtd resulting from commit id 'a4bd62ad' (1.0.6)
which added parentaddr and unique_id to allow unique identification of
a scsi_host, but assumed that all the pool entries and the incoming
definition would be similarly defined. If the existing pool uses the
'name' attribute and an incoming pool is using the parentaddr/unique_id,
then the code will attempt to compare the existing name string against
the incoming name string which doesn't exist (is NULL) and results in
a core (STREQ).
Conversely, if the existing pool used the parentaddr/unique_id and the
to be defined pool used the name, then the comparison would be against
the parentaddr, but since the incoming pool doesn't have one - that would
leave the comparison against a parentaddr of all 0's and a unique_id of 0,
which will always comparison to fail. This means someone could define the
same source adapter for two pools
This patch resolves that.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/storage_conf.c | 67 ++++++++++++++++++++++++++++++++++++++++++++-----
1 file changed, 61 insertions(+), 6 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 74267bd..c17565e 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -2062,7 +2062,7 @@ virStoragePoolObjIsDuplicate(virStoragePoolObjListPtr pools,
return ret;
}
-static bool
+static bool ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2)
matchSCSIAdapterName(const char *pool_name,
const char *def_name)
{
@@ -2105,16 +2105,63 @@ matchSCSIAdapterParent(virStoragePoolSourceAdapter poolAdapter,
return false;
}
-static bool
+static int
matchSCSIAdapter(virStoragePoolSourceAdapter poolAdapter,
virStoragePoolSourceAdapter defAdapter)
{
- if (poolAdapter.data.scsi_host.name) {
+ char *name = NULL;
+ char *parentaddr = NULL;
+ const char *cmpname;
+ unsigned int unique_id;
+ virDevicePCIAddressPtr poolPCIptr;
+ bool retval = -1;
+
+ /* simple cases - both use name or both use parentaddr */
+ if (poolAdapter.data.scsi_host.name &&
+ defAdapter.data.scsi_host.name) {
return matchSCSIAdapterName(poolAdapter.data.scsi_host.name,
defAdapter.data.scsi_host.name);
- } else {
+ } else if (poolAdapter.data.scsi_host.has_parent &&
+ defAdapter.data.scsi_host.has_parent) {
return matchSCSIAdapterParent(poolAdapter, defAdapter);
}
+
+ if (poolAdapter.data.scsi_host.has_parent) {
+ /* If the poolAdapter is using parentaddr, but incoming is using the
+ * "scsi_host#" or "host#", then we need to formulate the
"host#"
+ * string of the poolAdapter for comparison. We cannot save that
+ * during startup in the pooldef since when/if we write out the
+ * XML for the pool, if we find the name field filled in we use that
+ * defeating the purpose of having parentaddr/unique_id
+ */
+ cmpname = defAdapter.data.scsi_host.name;
+ unique_id = poolAdapter.data.scsi_host.unique_id;
+ poolPCIptr = &poolAdapter.data.scsi_host.parentaddr;
+ } else {
+ /* Otherwise the pool is using "host#" or "scsi_host#" and
the incoming
+ * def is using the "parentaddr" and "uniqueid" - so
formulate the
+ * host# name of the incoming def and make sure it doesn't match the
+ * current pool definition
+ */
+ cmpname = poolAdapter.data.scsi_host.name;
+ unique_id = defAdapter.data.scsi_host.unique_id;
+ poolPCIptr = &defAdapter.data.scsi_host.parentaddr;
+ }
+
+ if (virAsprintf(&parentaddr, "%04x:%02x:%02x.%01x",
+ poolPCIptr->domain, poolPCIptr->bus,
+ poolPCIptr->slot, poolPCIptr->function) < 0)
+ goto cleanup;
+
+ if (!(name = virFindSCSIHostByPCI(NULL, parentaddr, unique_id)))
+ goto cleanup;
+
+ retval = matchSCSIAdapterName(name, cmpname);
+
+ cleanup:
+ VIR_FREE(name);
+ VIR_FREE(parentaddr);
+ return retval;
}
int
@@ -2163,8 +2210,12 @@ virStoragePoolSourceFindDuplicate(virStoragePoolObjListPtr pools,
VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST &&
def->source.adapter.type ==
VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) {
- if (matchSCSIAdapter(pool->def->source.adapter,
- def->source.adapter))
+ int rc;
+ rc = matchSCSIAdapter(pool->def->source.adapter,
+ def->source.adapter);
+ if (rc < 0)
+ goto error; /* Error reported during matchSCSIAdapter */
+ if (rc)
matchpool = pool;
}
break;
@@ -2205,6 +2256,10 @@ virStoragePoolSourceFindDuplicate(virStoragePoolObjListPtr pools,
ret = -1;
}
return ret;
+
+ error:
+ virStoragePoolObjUnlock(pool);
+ return -1;
}
void
--
1.9.3
8:20 a.m.
New subject: [libvirt] [PATCH 4/4] storage_conf: Resolve libvirtd crash matching scsi_host
On 09/30/2014 11:35 PM, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1146837
Resolve a crash in libvirtd resulting from commit id 'a4bd62ad' (1.0.6)
which added parentaddr and unique_id to allow unique identification of
a scsi_host, but assumed that all the pool entries and the incoming
definition would be similarly defined. If the existing pool uses the
'name' attribute and an incoming pool is using the parentaddr/unique_id,
then the code will attempt to compare the existing name string against
the incoming name string which doesn't exist (is NULL) and results in
a core (STREQ).
Fixing this crash would be nicer in a separate patch.
Conversely, if the existing pool used the parentaddr/unique_id and
the
to be defined pool used the name, then the comparison would be against
the parentaddr, but since the incoming pool doesn't have one - that would
leave the comparison against a parentaddr of all 0's and a unique_id of 0,
which will always comparison to fail. This means someone could define the
same source adapter for two pools
When defining a storage pool, we don't check if the adapter name or
parentaddr/unique_id is valid, so I don't think we should require it to be
valid to detect duplicates.
If there was a pool with invalid parentaddr, no other SCSI pools could be
defined because of that.
Maybe we could check it on pool startup?
Jan
6:23 a.m.
New subject: [libvirt] [PATCH 4/4] storage_conf: Resolve libvirtd crash matching scsi_host
On 10/03/2014 09:20 AM, Ján Tomko wrote:
On 09/30/2014 11:35 PM, John Ferlan wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1146837
>
> Resolve a crash in libvirtd resulting from commit id 'a4bd62ad' (1.0.6)
> which added parentaddr and unique_id to allow unique identification of
> a scsi_host, but assumed that all the pool entries and the incoming
> definition would be similarly defined. If the existing pool uses the
> 'name' attribute and an incoming pool is using the parentaddr/unique_id,
> then the code will attempt to compare the existing name string against
> the incoming name string which doesn't exist (is NULL) and results in
> a core (STREQ).
>
Fixing this crash would be nicer in a separate patch.
This patch does fix the crash and it must fix the side effect to having
that check (e.g. both pool and incoming def use name). The crash is the
condition where incoming definition doesn't use the same XML format as
the already defined pool. Adding in the mismatched definition checks in
a prior or future patch doesn't make sense mainly because all that was
considered previously was matching definitions.
> Conversely, if the existing pool used the parentaddr/unique_id
and the
> to be defined pool used the name, then the comparison would be against
> the parentaddr, but since the incoming pool doesn't have one - that would
> leave the comparison against a parentaddr of all 0's and a unique_id of 0,
> which will always comparison to fail. This means someone could define the
> same source adapter for two pools
When defining a storage pool, we don't check if the adapter name or
parentaddr/unique_id is valid, so I don't think we should require it to be
valid to detect duplicates.
If you mean we don't check that the name starts with 'scsi' or
'scsi_host', then sure I agree, but that would be a different bug or
issue. I can certainly add a check if that's desired to ensure prefix
is correct. Of course, the docs :
http://libvirt.org/formatstorage.html
do provide the rules for the name property (and less so for the parent).
John
If there was a pool with invalid parentaddr, no other SCSI pools could be
defined because of that.
Maybe we could check it on pool startup?
Jan
7:44 a.m.
New subject: [libvirt] [PATCH 4/4] storage_conf: Resolve libvirtd crash matching scsi_host
On 10/06/2014 01:23 PM, John Ferlan wrote:
On 10/03/2014 09:20 AM, Ján Tomko wrote:
> On 09/30/2014 11:35 PM, John Ferlan wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1146837
>>
>> Resolve a crash in libvirtd resulting from commit id 'a4bd62ad' (1.0.6)
>> which added parentaddr and unique_id to allow unique identification of
>> a scsi_host, but assumed that all the pool entries and the incoming
>> definition would be similarly defined. If the existing pool uses the
>> 'name' attribute and an incoming pool is using the parentaddr/unique_id,
>> then the code will attempt to compare the existing name string against
>> the incoming name string which doesn't exist (is NULL) and results in
>> a core (STREQ).
>>
>
> Fixing this crash would be nicer in a separate patch.
>
This patch does fix the crash and it must fix the side effect to having
that check (e.g. both pool and incoming def use name). The crash is the
condition where incoming definition doesn't use the same XML format as
the already defined pool. Adding in the mismatched definition checks in
a prior or future patch doesn't make sense mainly because all that was
considered previously was matching definitions.
>> Conversely, if the existing pool used the parentaddr/unique_id and the
>> to be defined pool used the name, then the comparison would be against
>> the parentaddr, but since the incoming pool doesn't have one - that would
>> leave the comparison against a parentaddr of all 0's and a unique_id of 0,
>> which will always comparison to fail. This means someone could define the
>> same source adapter for two pools
>
> When defining a storage pool, we don't check if the adapter name or
> parentaddr/unique_id is valid, so I don't think we should require it to be
> valid to detect duplicates.
If you mean we don't check that the name starts with 'scsi' or
'scsi_host', then sure I agree, but that would be a different bug or
issue. I can certainly add a check if that's desired to ensure prefix
is correct. Of course, the docs :
http://libvirt.org/formatstorage.html
do provide the rules for the name property (and less so for the parent).
I mean we don't check if a scsi controller is present on the specified PCI
address when the pool is defined (so the definiton does not depend on host
hardware). After this patch, I can successfully define a pool with:
<adapter type='scsi_host'>
<parentaddr unique_id='1'>
<address domain='0x0000' bus='0x00' slot='0x1f'
function='0x0'/>
</parentaddr>
</adapter>
(where 00:1f.0 is some ISA bridge on my system)
But defining another pool with <adapter type='scsi_host'> and name
specified
now depends on the host hardware, i.e. it always fails:
<adapter type='scsi_host' name='host1'/>
error: Failed to define pool from pool.xml
error: operation failed: Storage source conflict with pool: 'scsi-pool'
Depending on the host hardware for duplicate detection during definition seems
weird to me, considering we don't do that for the first definition on the pool
either.
Jan
4:17 p.m.
New subject: [libvirt] [PATCH 4/4] storage_conf: Resolve libvirtd crash matching scsi_host
On 10/06/2014 08:44 AM, Ján Tomko wrote:
On 10/06/2014 01:23 PM, John Ferlan wrote:
<...snip...>
>
> If you mean we don't check that the name starts with 'scsi' or
> 'scsi_host', then sure I agree, but that would be a different bug or
> issue. I can certainly add a check if that's desired to ensure prefix
> is correct. Of course, the docs :
>
> http://libvirt.org/formatstorage.html
>
> do provide the rules for the name property (and less so for the parent).
>
I mean we don't check if a scsi controller is present on the specified PCI
address when the pool is defined (so the definiton does not depend on host
hardware). After this patch, I can successfully define a pool with:
<adapter type='scsi_host'>
<parentaddr unique_id='1'>
<address domain='0x0000' bus='0x00' slot='0x1f'
function='0x0'/>
</parentaddr>
</adapter>
(where 00:1f.0 is some ISA bridge on my system)
But defining another pool with <adapter type='scsi_host'> and name
specified
now depends on the host hardware, i.e. it always fails:
<adapter type='scsi_host' name='host1'/>
error: Failed to define pool from pool.xml
error: operation failed: Storage source conflict with pool: 'scsi-pool'
Depending on the host hardware for duplicate detection during definition seems
weird to me, considering we don't do that for the first definition on the pool
either.
The parentaddr/unique_id are way to make sure the definition of the
target of the pool lives beyond a host reboot or some other "event" that
causes a reorder/renumber of the 'scsi_host#' values.
Validating whether the address provided is actually a valid target for a
scsi_host is a different can-o-worms.
The patches to be posted shortly will at least make sure the various
ways a scsi_host can be named (name=host#, name=scsi_host#, parentaddr &
unique_id) will at least result in non duplicated pools.
John
3734
days inactive
3740
days old
12 comments
2 participants
participants (2)
-
John Ferlan -
Ján Tomko