Create/use virGetSCSIHostNumber to replace the static getHostNumber Removed the "if (result &&" since result is now required to be non NULL on input. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/libvirt_private.syms | 1 + src/storage/storage_backend_scsi.c | 40 +++------------------------ src/util/virutil.c | 56 ++++++++++++++++++++++++++++++++++++++ src/util/virutil.h | 4 +++ 4 files changed, 65 insertions(+), 36 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d6265ac..189e07c 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2151,6 +2151,7 @@ virGetGroupList; virGetGroupName; virGetHostname; virGetListenFDs; +virGetSCSIHostNumber; virGetSelfLastChanged; virGetUnprivSGIOSysfsPath; virGetUserCacheDirectory; diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index 16ed328..d9f3ff2 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -511,38 +511,6 @@ virStorageBackendSCSITriggerRescan(uint32_t host) return retval; } -static int -getHostNumber(const char *adapter_name, - unsigned int *result) -{ - char *host = (char *)adapter_name; - - /* Specifying adapter like 'host5' is still supported for - * back-compat reason. - */ - if (STRPREFIX(host, "scsi_host")) { - host += strlen("scsi_host"); - } else if (STRPREFIX(host, "fc_host")) { - host += strlen("fc_host"); - } else if (STRPREFIX(host, "host")) { - host += strlen("host"); - } else { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Invalid adapter name '%s' for SCSI pool"), - adapter_name); - return -1; - } - - if (result && virStrToLong_ui(host, NULL, 10, result) == -1) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Invalid adapter name '%s' for SCSI pool"), - adapter_name); - return -1; - } - - return 0; -} - static char * getAdapterName(virStoragePoolSourceAdapter adapter) { @@ -610,7 +578,7 @@ createVport(virStoragePoolSourceAdapter adapter) return -1; } - if (getHostNumber(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, @@ -643,7 +611,7 @@ deleteVport(virStoragePoolSourceAdapter adapter) adapter.data.fchost.wwpn))) return -1; - if (getHostNumber(adapter.data.fchost.parent, &parent_host) < 0) + if (virGetSCSIHostNumber(adapter.data.fchost.parent, &parent_host) < 0) goto cleanup; if (virManageVport(parent_host, adapter.data.fchost.wwpn, @@ -683,7 +651,7 @@ virStorageBackendSCSICheckPool(virConnectPtr conn ATTRIBUTE_UNUSED, } } - if (getHostNumber(name, &host) < 0) + if (virGetSCSIHostNumber(name, &host) < 0) goto cleanup; if (virAsprintf(&path, "%s/host%d", @@ -712,7 +680,7 @@ virStorageBackendSCSIRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED, if (!(name = getAdapterName(pool->def->source.adapter))) return -1; - if (getHostNumber(name, &host) < 0) + if (virGetSCSIHostNumber(name, &host) < 0) goto out; VIR_DEBUG("Scanning host%u", host); diff --git a/src/util/virutil.c b/src/util/virutil.c index 5197969..1c23d7c 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -1838,6 +1838,54 @@ virFindSCSIHostByPCI(const char *sysfs_prefix, return ret; } +/* virGetSCSIHostNumber: + * @adapter_name: Name of the host adapter + * @result: Return the entry value as unsigned int + * + * Convert the various forms of scsi_host names into the numeric + * host# value that can be used in order to scan sysfs looking for + * the specific host. + * + * 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 + * + * Returns 0 on success, and @result has the host number. + * Otherwise returns -1. + */ +int +virGetSCSIHostNumber(const char *adapter_name, + unsigned int *result) +{ + char *host = (char *)adapter_name; + + /* Specifying adapter like 'host5' is still supported for + * back-compat reason. + */ + if (STRPREFIX(host, "scsi_host")) { + host += strlen("scsi_host"); + } else if (STRPREFIX(host, "fc_host")) { + host += strlen("fc_host"); + } else if (STRPREFIX(host, "host")) { + host += strlen("host"); + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid adapter name '%s' for SCSI pool"), + adapter_name); + return -1; + } + + if (virStrToLong_ui(host, NULL, 10, result) == -1) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid adapter name '%s' for SCSI pool"), + adapter_name); + return -1; + } + + return 0; +} + /* virReadFCHost: * @sysfs_prefix: "fc_host" sysfs path, defaults to SYSFS_FC_HOST_PATH * @host: Host number, E.g. 5 of "fc_host/host5" @@ -2209,6 +2257,14 @@ virFindSCSIHostByPCI(const char *sysfs_prefix ATTRIBUTE_UNUSED, } int +virGetSCSIHostNumber(const char *adapter_name ATTRIBUTE_UNUSED, + unsigned int *result ATTRIBUTE_UNUSED) +{ + virReportSystemError(ENOSYS, "%s", _("Not supported on this platform")); + return NULL; +} + +int virReadFCHost(const char *sysfs_prefix ATTRIBUTE_UNUSED, int host ATTRIBUTE_UNUSED, const char *entry ATTRIBUTE_UNUSED, diff --git a/src/util/virutil.h b/src/util/virutil.h index 54f1148..442c998 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -173,6 +173,10 @@ char * virFindSCSIHostByPCI(const char *sysfs_prefix, const char *parentaddr, unsigned int unique_id); +int +virGetSCSIHostNumber(const char *adapter_name, + unsigned int *result) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); int virReadFCHost(const char *sysfs_prefix, int host, const char *entry, -- 1.9.3

On 10/28/2014 07:36 PM, Michal Privoznik wrote: You'll see it's just a cut-n-paste/copy of the former getHostNumber function above. I see your point though - it's not a 'const char const *', so adjusting the adapter_name pointer is perfectly reasonable and what I went with. John

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 In order to resolve this, adjust the code to get the 'host#' to be used by the storage scsi backend in order to check/start the pool and make sure the incoming definition doesn't match any of the existing pool defs. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/conf/storage_conf.c | 69 ++++++++++++++++++++++++++++++------------------- 1 file changed, 42 insertions(+), 27 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 36696a4..19c452b 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -2062,26 +2062,37 @@ virStoragePoolObjIsDuplicate(virStoragePoolObjListPtr pools, return ret; } -static bool -matchSCSIAdapterParent(virStoragePoolObjPtr pool, - virStoragePoolDefPtr def) +static int +getSCSIHostNumber(virStoragePoolSourceAdapter adapter, + unsigned int *hostnum) { - virDevicePCIAddressPtr pooladdr = - &pool->def->source.adapter.data.scsi_host.parentaddr; - virDevicePCIAddressPtr defaddr = - &def->source.adapter.data.scsi_host.parentaddr; - int pool_unique_id = - pool->def->source.adapter.data.scsi_host.unique_id; - int def_unique_id = - def->source.adapter.data.scsi_host.unique_id; - if (pooladdr->domain == defaddr->domain && - pooladdr->bus == defaddr->bus && - pooladdr->slot == defaddr->slot && - pooladdr->function == defaddr->function && - pool_unique_id == def_unique_id) { - return true; - } - return false; + int ret = -1; + unsigned int num; + char *name = NULL; + + if (adapter.data.scsi_host.has_parent) { + virDevicePCIAddress addr = adapter.data.scsi_host.parentaddr; + unsigned int unique_id = adapter.data.scsi_host.unique_id; + + if (!(name = virGetSCSIHostNameByParentaddr(addr.domain, + addr.bus, + addr.slot, + addr.function, + unique_id))) + goto cleanup; + if (virGetSCSIHostNumber(name, &num) < 0) + goto cleanup; + } else { + if (virGetSCSIHostNumber(adapter.data.scsi_host.name, &num) < 0) + goto cleanup; + } + + *hostnum = num; + ret = 0; + + cleanup: + VIR_FREE(name); + return ret; } int @@ -2130,14 +2141,14 @@ 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; - } + unsigned int pool_hostnum, def_hostnum; + + if (getSCSIHostNumber(pool->def->source.adapter, + &pool_hostnum) < 0 || + getSCSIHostNumber(def->source.adapter, &def_hostnum) < 0) + goto error; + if (pool_hostnum == def_hostnum) + matchpool = pool; } break; case VIR_STORAGE_POOL_ISCSI: @@ -2177,6 +2188,10 @@ virStoragePoolSourceFindDuplicate(virStoragePoolObjListPtr pools, ret = -1; } return ret; + + error: + virStoragePoolObjUnlock(pool); + return -1; } void -- 1.9.3

On 10/29/2014 07:31 AM, Ján Tomko wrote: I think I'm missing your point. The finding of duplicate sources for storage pools and disallowing them to be defined (or created) has been around since commit id '5a1f27287". I'm not sure what you meant by your first sentence - perhaps an example would help me especially in the accepted, but invalid condition. John