Re: [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 jfer...@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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [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 jfer...@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
Re: [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.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [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 jfer...@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
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
