On Fri, Feb 17, 2017 at 12:14:51PM -0500, John Ferlan wrote:
>
>
> 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
Yes they also include conf headers and it should not happen in
the first place.
> 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...
It would be way better to make a generic function that simply gets
a parent for node device specified by its name and place this function
into node_device_conf.c. There would be no duplication and the caller
would be responsible for constructing the correct node device name.
Pavel
> John
>
> >
> > NACK
> >
> > Pavel
> >
> >>
> >> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
> >> ---
> >> 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@xxxxxxxxxx
> >> https://www.redhat.com/mailman/listinfo/libvir-list
>
> --
> libvir-list mailing list
> libvir-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
