[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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 redhat com>
> >> ---
> >>  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 redhat com
> >> https://www.redhat.com/mailman/listinfo/libvir-list
> 
> --
> libvir-list mailing list
> libvir-list redhat com
> https://www.redhat.com/mailman/listinfo/libvir-list

Attachment: signature.asc
Description: Digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]