Re: [libvirt] [PATCH 07/15] util: Move/rename virStoragePoolGetVhbaSCSIHostParent to virvhba

On 02/17/2017 11:41 AM, Pavel Hrdina wrote: > On Wed, Jan 25, 2017 at 03:27:33PM -0500, John Ferlan wrote: >> Move the function and rename to simply virVHBAGetParent >> >> Since I'm changing related code, rather than have a Vhba named function >> in storage_backend_scsi rename it to simply checkParent as it's already >> within the scope of checking VHBA related features. > > This is Vhba named function but it isn't only VHBA related code. > With some modification it could be generic node device function to > get a parent of a device specified by it's name. The only VHBA related > code in this function is construction of the node device name. > > Even without this context we should not import anything from src/conf/*.h > in the src/util/* code. While I agree it's doesn't appear to be VHBA related since it's not looking at the sysfs file structure for the answer, but there is a reason for it. One could argue that virStoragePoolGetVhbaSCSIHostParent doesn't really belong in storage_conf.c either. IIRC it was placed there because of that awful matchFCHostToSCSIHost function (which theoretically could be moved too). There are other util functions that query a driver to get an answer (auth, closecallbacks), so that part isn't something new. It also

doesn't "do" something _conf specific such as format/alter - it's just getting an answer from the node device driver. There's nothing storage_conf specific about it. It is VHBA related if you consider that it's a way to take a name ("scsi_hostN") that's already been verified as a VHBA by some other virVHBA* sysfs perusing API and return its parent. Since one doesn't get the answer of parent via the file system, one must ask the driver. I'll have to look at my local branches to recall whether or not the "next" stages will need this. The next stages being the ability to add/define a VHBA to a domain directly with the ultimate goal being to add all the LUNs from the VHBA to the domain automagically. I believe the reason I made this generic is because it would be "duplicitous" to have a domain_conf.c function that does the same thing...

John > > NACK > > Pavel > >> >> Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; >> --- >> src/conf/storage_conf.c | 61 +---------------------------------- >> src/conf/storage_conf.h | 5 --- >> src/libvirt_private.syms | 2 +- >> src/storage/storage_backend_scsi.c | 12 +++---- >> src/util/virvhba.c | 65 ++++++++++++++++++++++++++++++++++++++ >> src/util/virvhba.h | 4 +++ >> 6 files changed, 77 insertions(+), 72 deletions(-) >> >> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c >> index f8b5228..5e13bbf 100644 >> --- a/src/conf/storage_conf.c >> +++ b/src/conf/storage_conf.c >> @@ -2264,64 +2264,6 @@ virStoragePoolObjIsDuplicate(virStoragePoolObjListPtr pools, >> return ret; >> } >> >> -/* >> - * virStoragePoolGetVhbaSCSIHostParent: >> - * >> - * Using the Node Device Driver, find the host# name found via wwnn/wwpn >> - * lookup in the fc_host sysfs tree (e.g. virGetFCHostNameByWWN) to get >> - * the parent 'scsi_host#'. >> - * >> - * @conn: Connection pointer (must be non-NULL on entry) >> - * @name: Pointer a string from a virGetFCHostNameByWWN (e.g., "host#") >> - * >> - * Returns a "scsi_host#" string of the parent of the vHBA >> - */ >> -char * >> -virStoragePoolGetVhbaSCSIHostParent(virConnectPtr conn, >> - const char *name) >> -{ >> - char *nodedev_name = NULL; >> - virNodeDevicePtr device = NULL; >> - char *xml = NULL; >> - virNodeDeviceDefPtr def = NULL; >> - char *vhba_parent = 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; >> - >> - /* The caller checks whether the returned value is NULL or not >> - * before continuing >> - */ >> - ignore_value(VIR_STRDUP(vhba_parent, def->parent)); >> - >> - cleanup: >> - VIR_FREE(nodedev_name); >> - virNodeDeviceDefFree(def); >> - VIR_FREE(xml); >> - virObjectUnref(device); >> - return vhba_parent; >> -} >> >> static int >> getSCSIHostNumber(virStoragePoolSourceAdapter adapter, >> @@ -2404,8 +2346,7 @@ matchFCHostToSCSIHost(virConnectPtr conn, >> * have a match. >> */ >> if (conn && !fc_adapter.data.fchost.parent) { >> - parent_name = virStoragePoolGetVhbaSCSIHostParent(conn, name); >> - if (parent_name) { >> + if ((parent_name = virVHBAGetParent(conn, name))) { >> if (virGetSCSIHostNumber(parent_name, &fc_hostnum) == 0 && >> scsi_hostnum == fc_hostnum) { >> VIR_FREE(parent_name); >> diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h >> index b35471d..426e8b8 100644 >> --- a/src/conf/storage_conf.h >> +++ b/src/conf/storage_conf.h >> @@ -30,7 +30,6 @@ >> # include "virbitmap.h" >> # include "virthread.h" >> # include "device_conf.h" >> -# include "node_device_conf.h" >> # include "object_event.h" >> >> # include <libxml/tree.h> >> @@ -417,10 +416,6 @@ int virStoragePoolObjIsDuplicate(virStoragePoolObjListPtr pools, >> virStoragePoolDefPtr def, >> unsigned int check_active); >> >> -char *virStoragePoolGetVhbaSCSIHostParent(virConnectPtr conn, >> - const char *name) >> - ATTRIBUTE_NONNULL(1); >> - >> int virStoragePoolSourceFindDuplicate(virConnectPtr conn, >> virStoragePoolObjListPtr pools, >> virStoragePoolDefPtr def); >> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms >> index 429c133..b58742d 100644 >> --- a/src/libvirt_private.syms >> +++ b/src/libvirt_private.syms >> @@ -887,7 +887,6 @@ virStoragePoolFormatDiskTypeToString; >> virStoragePoolFormatFileSystemNetTypeToString; >> virStoragePoolFormatFileSystemTypeToString; >> virStoragePoolFormatLogicalTypeToString; >> -virStoragePoolGetVhbaSCSIHostParent; >> virStoragePoolLoadAllConfigs; >> virStoragePoolLoadAllState; >> virStoragePoolObjAssignDef; >> @@ -2744,6 +2743,7 @@ virVHBAFindVportHost; >> virVHBAGetConfig; >> virVHBAGetHostByFabricWWN; >> virVHBAGetHostByWWN; >> +virVHBAGetParent; >> virVHBAIsVportCapable; >> virVHBAManageVport; >> virVHBAPathExists; >> diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c >> index e037a93..2da15dd 100644 >> --- a/src/storage/storage_backend_scsi.c >> +++ b/src/storage/storage_backend_scsi.c >> @@ -214,9 +214,9 @@ getAdapterName(virStoragePoolSourceAdapter adapter) >> * sysfs tree to get the parent 'scsi_host#' to ensure it matches. >> */ >> static bool >> -checkVhbaSCSIHostParent(virConnectPtr conn, >> - const char *name, >> - const char *parent_name) >> +checkParent(virConnectPtr conn, >> + const char *name, >> + const char *parent_name) >> { >> char *vhba_parent = NULL; >> bool retval = false; >> @@ -227,7 +227,7 @@ checkVhbaSCSIHostParent(virConnectPtr conn, >> if (!conn) >> return true; >> >> - if (!(vhba_parent = virStoragePoolGetVhbaSCSIHostParent(conn, name))) >> + if (!(vhba_parent = virVHBAGetParent(conn, name))) >> goto cleanup; >> >> if (STRNEQ(parent_name, vhba_parent)) { >> @@ -276,7 +276,7 @@ createVport(virConnectPtr conn, >> * retrieved has the same parent >> */ >> if (adapter->data.fchost.parent && >> - checkVhbaSCSIHostParent(conn, name, adapter->data.fchost.parent)) >> + checkParent(conn, name, adapter->data.fchost.parent)) >> ret = 0; >> >> goto cleanup; >> @@ -416,7 +416,7 @@ deleteVport(virConnectPtr conn, >> if (virGetSCSIHostNumber(adapter.data.fchost.parent, &parent_host) < 0) >> goto cleanup; >> } else { >> - if (!(vhba_parent = virStoragePoolGetVhbaSCSIHostParent(conn, name))) >> + if (!(vhba_parent = virVHBAGetParent(conn, name))) >> goto cleanup; >> >> if (virGetSCSIHostNumber(vhba_parent, &parent_host) < 0) >> diff --git a/src/util/virvhba.c b/src/util/virvhba.c >> index e5637d7..8c4da3a 100644 >> --- a/src/util/virvhba.c >> +++ b/src/util/virvhba.c >> @@ -18,6 +18,8 @@ >> >> #include <config.h> >> >> +#include "node_device_conf.h" >> + >> #include "viralloc.h" >> #include "virerror.h" >> #include "virfile.h" >> @@ -530,3 +532,66 @@ virVHBAGetHostByFabricWWN(const char *sysfs_prefix ATTRIBUTE_UNUSED, >> } >> >> #endif /* __linux__ */ >> + >> + >> +/* The following will be more generic - using the Node Device driver in >> + * order to perform the lookup rather than looking through the file system >> + * in order to find the answer */ >> +/* >> + * virVHBAGetParent: >> + * >> + * Using the Node Device Driver, find the host# name found via wwnn/wwpn >> + * lookup in the fc_host sysfs tree (e.g. virGetFCHostNameByWWN) to get >> + * the parent 'scsi_host#'. >> + * >> + * @conn: Connection pointer (must be non-NULL on entry) >> + * @name: Pointer a string from a virGetFCHostNameByWWN (e.g., "host#") >> + * >> + * Returns a "scsi_host#" string of the parent of the vHBA >> + */ >> +char * >> +virVHBAGetParent(virConnectPtr conn, >> + const char *name) >> +{ >> + char *nodedev_name = NULL; >> + virNodeDevicePtr device = NULL; >> + char *xml = NULL; >> + virNodeDeviceDefPtr def = NULL; >> + char *vhba_parent = 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; >> + >> + /* The caller checks whether the returned value is NULL or not >> + * before continuing >> + */ >> + ignore_value(VIR_STRDUP(vhba_parent, def->parent)); >> + >> + cleanup: >> + VIR_FREE(nodedev_name); >> + virNodeDeviceDefFree(def); >> + VIR_FREE(xml); >> + virObjectUnref(device); >> + return vhba_parent; >> +} >> diff --git a/src/util/virvhba.h b/src/util/virvhba.h >> index e338f96..b8d73ab 100644 >> --- a/src/util/virvhba.h >> +++ b/src/util/virvhba.h >> @@ -52,4 +52,8 @@ char *virVHBAGetHostByFabricWWN(const char *sysfs_prefix, >> const char *fabric_wwn) >> ATTRIBUTE_NONNULL(2); >> >> +char *virVHBAGetParent(virConnectPtr conn, >> + const char *name) >> + ATTRIBUTE_NONNULL(1); >> + >> #endif /* __VIR_VBHA_H__ */ >> -- >> 2.7.4 >> >> -- >> libvir-list mailing list >> libvir-list(a)redhat.com >> https://www.redhat.com/mailman/listinfo/libvir-list -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list