[libvirt] [PATCH 06/15] util: Create a new virvhba module and move/rename API's
Pavel Hrdina
phrdina at redhat.com
Fri Feb 17 16:00:18 UTC 2017
On Wed, Jan 25, 2017 at 03:27:32PM -0500, John Ferlan wrote:
> Rather than have them mixed in with the virutil apis, create a separate
> virvhba.c module and move the vHBA related calls into there. Soon there
> will be more added.
>
> Also modify the names of the functions and some arguments to be more
> indicative of what is really happening. Adjust the callers respectively.
>
> While I was changing fchosttest, rather than the non-descriptive names
> test1...test6, rename them to match what the test is doing.
Again, please next time split those changes into separate commit, because
they can be easily overlooked by reviewer. I suspect that the whole series
is affected by this, so I will not repeat it again.
>
> Signed-off-by: John Ferlan <jferlan at redhat.com>
> ---
> po/POTFILES.in | 1 +
> src/Makefile.am | 1 +
> src/conf/storage_conf.c | 5 +-
> src/libvirt_private.syms | 17 +-
> src/node_device/node_device_driver.c | 21 +-
> src/node_device/node_device_linux_sysfs.c | 19 +-
> src/storage/storage_backend_scsi.c | 41 +--
> src/util/virscsi.c | 4 +
> src/util/virutil.c | 459 +-------------------------
> src/util/virutil.h | 27 --
> src/util/virvhba.c | 532 ++++++++++++++++++++++++++++++
> src/util/virvhba.h | 55 +++
> tests/fchosttest.c | 80 ++---
> 13 files changed, 684 insertions(+), 578 deletions(-)
> create mode 100644 src/util/virvhba.c
> create mode 100644 src/util/virvhba.h
[...]
> diff --git a/src/util/virvhba.c b/src/util/virvhba.c
> new file mode 100644
> index 0000000..e5637d7
> --- /dev/null
> +++ b/src/util/virvhba.c
> @@ -0,0 +1,532 @@
> +/*
> + * virvhba.c: Generic vHBA management utility functions
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library. If not, see
> + * <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <config.h>
> +
> +#include "viralloc.h"
> +#include "virerror.h"
> +#include "virfile.h"
> +#include "virlog.h"
> +#include "virstring.h"
> +#include "virvhba.h"
> +
> +#define VIR_FROM_THIS VIR_FROM_NONE
> +
> +VIR_LOG_INIT("util.vhba");
> +
> +#ifdef __linux__
> +
> +# define SYSFS_SCSI_HOST_PATH "/sys/class/scsi_host"
> +# define SYSFS_FC_HOST_PATH "/sys/class/fc_host"
> +# define PORT_STATE_ONLINE "Online"
> +
> +
> +/* virVHBAPathExists:
> + * @sysfs_prefix: "fc_host" sysfs path, defaults to SYSFS_FC_HOST_PATH
> + * @host: Host number, E.g. 5 of "fc_host/host5"
> + *
> + * Check if the "fc_host" to provided host# exists. This path may be either
> + * a vHBA capable path or a vHBA itself.
> + *
> + * Returns true if it does, false if not
> + */
> +bool
> +virVHBAPathExists(const char *sysfs_prefix,
> + int host)
> +{
> + char *sysfs_path = NULL;
> + bool ret = false;
> +
> + if (virAsprintf(&sysfs_path, "%s/host%d",
> + sysfs_prefix ? sysfs_prefix : SYSFS_FC_HOST_PATH,
> + host) < 0)
> + return false;
> +
> + if (virFileExists(sysfs_path))
> + ret = true;
> +
> + VIR_FREE(sysfs_path);
> + return ret;
> +}
> +
> +
> +/* virVHBAIsVportCapable:
> + * @sysfs_prefix: "fc_host" sysfs path, defaults to SYSFS_FC_HOST_PATH
> + * @host: Host number, E.g. 5 of "fc_host/host5"
> + *
> + * Not all vHBA paths can create/delete a vport - only the parent NPIV
> + * capable HBA has the "vport_create" and "vport_delete" functions.
> + * A vHBA created path does not have the function files.
> + *
> + * NB: Checks both the "fc_host" and "scsi_host" paths.
> + *
> + * Returns true if capable, false if not
> + */
> +bool
> +virVHBAIsVportCapable(const char *sysfs_prefix,
> + int host)
> +{
> + char *scsi_host_path = NULL;
> + char *fc_host_path = NULL;
> + bool ret = false;
> +
> + if (virAsprintf(&fc_host_path,
> + "%s/host%d/%s",
> + sysfs_prefix ? sysfs_prefix : SYSFS_FC_HOST_PATH,
> + host,
> + "vport_create") < 0)
> + return false;
> +
> + if (virAsprintf(&scsi_host_path,
> + "%s/host%d/%s",
> + sysfs_prefix ? sysfs_prefix : SYSFS_SCSI_HOST_PATH,
> + host,
> + "vport_create") < 0)
> + goto cleanup;
> +
> + if (virFileExists(fc_host_path) || virFileExists(scsi_host_path))
> + ret = true;
> +
> + cleanup:
> + VIR_FREE(fc_host_path);
> + VIR_FREE(scsi_host_path);
> + return ret;
> +}
> +
> +
> +/* virVHBAGetConfig:
> + * @sysfs_prefix: "fc_host" sysfs path, defaults to SYSFS_FC_HOST_PATH
> + * @host: Host number, E.g. 5 of "fc_host/host5"
> + * @entry: Name of the FC sysfs entry to read
> + *
> + * Read the value of a vHBA sysfs "fc_host" entry (if it exists).
> + *
> + * Returns result as a string on success, caller must free @result after
This could be reworded.
> + * Otherwise returns NULL.
> + */
> +char *
> +virVHBAGetConfig(const char *sysfs_prefix,
> + int host,
> + const char *entry)
> +{
> + char *sysfs_path = NULL;
> + char *p = NULL;
> + char *buf = NULL;
> + char *result = NULL;
> +
> + if (virAsprintf(&sysfs_path, "%s/host%d/%s",
> + sysfs_prefix ? sysfs_prefix : SYSFS_FC_HOST_PATH,
> + host, entry) < 0)
> + goto cleanup;
> +
> + if (!virFileExists(sysfs_path))
> + goto cleanup;
> +
> + if (virFileReadAll(sysfs_path, 1024, &buf) < 0)
> + goto cleanup;
> +
> + if ((p = strchr(buf, '\n')))
> + *p = '\0';
> +
> + if ((p = strstr(buf, "0x")))
> + p += strlen("0x");
> + else
> + p = buf;
> +
> + ignore_value(VIR_STRDUP(result, p));
> +
> + cleanup:
> + VIR_FREE(sysfs_path);
> + VIR_FREE(buf);
> + return result;
> +}
> +
> +
> +/* virVHBAFindVportHost:
> + *
> + * Iterate over the sysfs and find out the first online HBA which
> + * supports vport, and is not saturated. Returns the host name (e.g.
> + * host5) on success, or NULL on failure.
> + *
> + * It's up to the caller to free the returned string.
> + */
> +char *
> +virVHBAFindVportHost(const char *sysfs_prefix)
> +{
> + const char *prefix = sysfs_prefix ? sysfs_prefix : SYSFS_FC_HOST_PATH;
> + DIR *dir = NULL;
> + struct dirent *entry = NULL;
> + char *max_vports = NULL;
> + char *vports = NULL;
> + char *state = NULL;
> + char *ret = NULL;
> +
> + if (virDirOpen(&dir, prefix) < 0)
> + return NULL;
> +
> + while (virDirRead(dir, &entry, prefix) > 0) {
> + unsigned int host;
> + char *p = NULL;
> +
> + p = entry->d_name + strlen("host");
> + if (virStrToLong_ui(p, NULL, 10, &host) == -1) {
> + VIR_DEBUG("Failed to parse host number from '%s'",
> + entry->d_name);
> + continue;
> + }
> +
> + if (!virVHBAPathExists(prefix, host))
> + continue;
> +
> + if (!(state = virVHBAGetConfig(prefix, host, "port_state"))) {
> + VIR_DEBUG("Failed to read port_state for host%d", host);
> + continue;
> + }
> +
> + /* Skip the not online FC host */
> + if (STRNEQ(state, PORT_STATE_ONLINE)) {
> + VIR_FREE(state);
> + continue;
> + }
> + VIR_FREE(state);
> +
> + if (!(max_vports = virVHBAGetConfig(prefix, host, "max_npiv_vports"))) {
> + VIR_DEBUG("Failed to read max_npiv_vports for host%d", host);
> + continue;
> + }
> +
> + if (!(vports = virVHBAGetConfig(prefix, host, "npiv_vports_inuse"))) {
> + VIR_DEBUG("Failed to read npiv_vports_inuse for host%d", host);
> + VIR_FREE(max_vports);
> + continue;
> + }
> +
> + /* Compare from the strings directly, instead of converting
> + * the strings to integers first
> + */
> + if ((strlen(max_vports) >= strlen(vports)) ||
> + ((strlen(max_vports) == strlen(vports)) &&
> + strcmp(max_vports, vports) > 0)) {
> + ignore_value(VIR_STRDUP(ret, entry->d_name));
> + goto cleanup;
> + }
> +
> + VIR_FREE(max_vports);
> + VIR_FREE(vports);
> + }
> +
> + cleanup:
> + VIR_DIR_CLOSE(dir);
> + VIR_FREE(max_vports);
> + VIR_FREE(vports);
> + return ret;
> +}
> +
> +/* virVHBAManageVport:
> + * @sysfs_prefix: "fc_host" sysfs path, defaults to SYSFS_FC_HOST_PATH
> + * @wwnn: world wide node name used to create/delete the vport
> + * @wwpn: world wide port name used to create/delete the vport
> + * @operation: create or delete
> + *
> + * NB: Checks both the "fc_host" and "scsi_host" paths.
> + * Returns true if capable, false if not
> + */
> +int
> +virVHBAManageVport(const int parent_host,
> + const char *wwpn,
> + const char *wwnn,
> + int operation)
> +{
> + int ret = -1;
> + char *operation_path = NULL, *vport_name = NULL;
> + const char *operation_file = NULL;
> +
> + switch (operation) {
> + case VPORT_CREATE:
> + operation_file = "vport_create";
> + break;
> + case VPORT_DELETE:
> + operation_file = "vport_delete";
> + break;
> + default:
> + virReportError(VIR_ERR_OPERATION_INVALID,
> + _("Invalid vport operation (%d)"), operation);
> + goto cleanup;
> + }
> +
> + if (virAsprintf(&operation_path, "%s/host%d/%s",
> + SYSFS_FC_HOST_PATH, parent_host, operation_file) < 0)
> + goto cleanup;
> +
> + if (!virFileExists(operation_path)) {
> + VIR_FREE(operation_path);
> + if (virAsprintf(&operation_path, "%s/host%d/%s",
> + SYSFS_SCSI_HOST_PATH, parent_host, operation_file) < 0)
> + goto cleanup;
> +
> + if (!virFileExists(operation_path)) {
> + virReportError(VIR_ERR_OPERATION_INVALID,
> + _("vport operation '%s' is not supported "
> + "for host%d"),
> + operation_file, parent_host);
> + goto cleanup;
> + }
> + }
> +
> + /* Create/Delete is handle through the file passing the wwpn:wwnn as
s/handle/handled/
> + * a parameter. This results in the kernel managing the port and an
> + * event posted. That event will be recogized and the Node Device driver
> + * will then take over either creating or deleting the vHBA */
Is this comment really necessary? Strictly speaking there is no event posted.
The Node Device driver is simply polling udev whether the requested device
exists or not.
> + if (virAsprintf(&vport_name, "%s:%s", wwpn, wwnn) < 0)
> + goto cleanup;
> +
> + if (virFileWriteStr(operation_path, vport_name, 0) == 0)
> + ret = 0;
> + else
> + virReportSystemError(errno,
> + _("Write of '%s' to '%s' during "
> + "vport create/delete failed"),
> + vport_name, operation_path);
> +
> + cleanup:
> + VIR_FREE(vport_name);
> + VIR_FREE(operation_path);
> + return ret;
> +}
> +
> +
> +/* vhbaReadCompareWWN
> + * @prefix: path to the wwn file
> + * @d_name: name of the current directory
> + * @f_name: file name to read
> + *
> + * Read/compare the on-disk file with the passed wwn value.
> + *
> + * Returns:
> + * -1 : Error
> + * 0 : No match
> + * 1 : Match
> + */
> +static int
> +vhbaReadCompareWWN(const char *prefix,
> + const char *d_name,
> + const char *f_name,
> + const char *wwn)
> +{
> + char *path;
> + char *buf = NULL;
> + char *p;
> + int ret = -1;
> +
> + if (virAsprintf(&path, "%s/%s/%s", prefix, d_name, f_name) < 0)
> + return -1;
> +
> + if (!virFileExists(path)) {
> + ret = 0;
> + goto cleanup;
> + }
> +
> + if (virFileReadAll(path, 1024, &buf) < 0)
> + goto cleanup;
> +
> + if ((p = strchr(buf, '\n')))
> + *p = '\0';
> + if (STRPREFIX(buf, "0x"))
> + p = buf + strlen("0x");
> + else
> + p = buf;
> +
> + if (STRNEQ(wwn, p))
> + ret = 0;
> + else
> + ret = 1;
> +
> + cleanup:
> + VIR_FREE(path);
> + VIR_FREE(buf);
> +
> + return ret;
> +}
> +
> +/* virVHBAGetHostByWWN:
> + *
> + * Iterate over the sysfs tree to get FC host name (e.g. host5)
> + * by the provided "wwnn,wwpn" pair.
> + *
> + * Returns the FC host name which must be freed by the caller,
> + * or NULL on failure.
> + */
> +char *
> +virVHBAGetHostByWWN(const char *sysfs_prefix,
> + const char *wwnn,
> + const char *wwpn)
> +{
> + const char *prefix = sysfs_prefix ? sysfs_prefix : SYSFS_FC_HOST_PATH;
> + struct dirent *entry = NULL;
> + DIR *dir = NULL;
> + char *ret = NULL;
> +
> + if (virDirOpen(&dir, prefix) < 0)
> + return NULL;
> +
> + while (virDirRead(dir, &entry, prefix) > 0) {
> + int rc;
> +
> + if ((rc = vhbaReadCompareWWN(prefix, entry->d_name,
> + "node_name", wwnn)) < 0)
> + goto cleanup;
> +
> + if (rc == 0)
> + continue;
> +
> + if ((rc = vhbaReadCompareWWN(prefix, entry->d_name,
> + "port_name", wwpn)) < 0)
> + goto cleanup;
> +
> + if (rc == 0)
> + continue;
> +
> + ignore_value(VIR_STRDUP(ret, entry->d_name));
> + break;
> + }
> +
> + cleanup:
> + VIR_DIR_CLOSE(dir);
> + return ret;
> +}
> +
> +/* virVHBAGetHostByFabricWWN:
> + *
> + * Iterate over the sysfs tree to get FC host name (e.g. host5)
> + * by the provided "fabric_wwn". This would find a host on a SAN.
> + *
> + * Returns the FC host name which must be freed by the caller,
> + * or NULL on failure.
> + */
> +char *
> +virVHBAGetHostByFabricWWN(const char *sysfs_prefix,
> + const char *fabric_wwn)
> +{
> + const char *prefix = sysfs_prefix ? sysfs_prefix : SYSFS_FC_HOST_PATH;
> + struct dirent *entry = NULL;
> + DIR *dir = NULL;
> + char *vport_create_path = NULL;
> + char *ret = NULL;
> +
> + if (virDirOpen(&dir, prefix) < 0)
> + return NULL;
> +
> + while (virDirRead(dir, &entry, prefix) > 0) {
> + int rc;
> +
> + VIR_FREE(vport_create_path);
> +
> + /* Existing vHBA's will have the same fabric_name, but won't
> + * have the vport_create file - so we check for both */
> + if (virAsprintf(&vport_create_path, "%s/%s/vport_create", prefix,
> + entry->d_name) < 0)
> + goto cleanup;
> +
> + if (!virFileExists(vport_create_path))
> + continue;
> +
> + if ((rc = vhbaReadCompareWWN(prefix, entry->d_name,
> + "fabric_name", fabric_wwn)) < 0)
> + goto cleanup;
> +
> + if (rc == 0)
> + continue;
> +
> + ignore_value(VIR_STRDUP(ret, entry->d_name));
> + break;
> + }
> +
> + cleanup:
> + VIR_DIR_CLOSE(dir);
> + VIR_FREE(vport_create_path);
> + return ret;
> +}
> +
> +#else
> +
> +bool
> +virVHBAPathExists(const char *sysfs_prefix ATTRIBUTE_UNUSED,
> + int host ATTRIBUTE_UNUSED)
> +{
> + virReportSystemError(ENOSYS, "%s", _("Not supported on this platform"));
> + return false;
> +}
> +
> +
> +bool
> +virVHBAVportIsCapable(const char *sysfs_prefix ATTRIBUTE_UNUSED,
> + int host ATTRIBUTE_UNUSED)
> +{
> + virReportSystemError(ENOSYS, "%s", _("Not supported on this platform"));
> + return false;
> +}
> +
> +
> +char *
> +virVHBAGetConfig(const char *sysfs_prefix ATTRIBUTE_UNUSED,
> + int host ATTRIBUTE_UNUSED,
> + const char *entry ATTRIBUTE_UNUSED)
> +{
> + virReportSystemError(ENOSYS, "%s", _("Not supported on this platform"));
> + return NULL;
> +}
> +
> +
> +char *
> +virVHBAFindVportHost(const char *sysfs_prefix ATTRIBUTE_UNUSED)
> +{
> + virReportSystemError(ENOSYS, "%s", _("Not supported on this platform"));
> + return NULL;
> +}
> +
> +
> +int
> +virVHBAManageVport(const int parent_host ATTRIBUTE_UNUSED,
> + const char *wwpn ATTRIBUTE_UNUSED,
> + const char *wwnn ATTRIBUTE_UNUSED,
> + int operation ATTRIBUTE_UNUSED)
> +{
> + virReportSystemError(ENOSYS, "%s", _("Not supported on this platform"));
> + return -1;
> +}
> +
> +
> +char *
> +virVHBAGetHostByWWN(const char *sysfs_prefix ATTRIBUTE_UNUSED,
> + const char *wwnn ATTRIBUTE_UNUSED,
> + const char *wwpn ATTRIBUTE_UNUSED)
> +{
> + virReportSystemError(ENOSYS, "%s", _("Not supported on this platform"));
> + return NULL;
> +}
> +
> +
> +char *
> +virVHBAGetHostByFabricWWN(const char *sysfs_prefix ATTRIBUTE_UNUSED,
> + const char *fabric_wwn ATTRIBUTE_UNUSED)
> +{
> + virReportSystemError(ENOSYS, "%s", _("Not supported on this platform"));
> + return NULL;
> +}
> +
> +#endif /* __linux__ */
> diff --git a/src/util/virvhba.h b/src/util/virvhba.h
> new file mode 100644
> index 0000000..e338f96
> --- /dev/null
> +++ b/src/util/virvhba.h
> @@ -0,0 +1,55 @@
> +/*
> + * virvhba.h: Generic vHBA management utility functions
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library. If not, see
> + * <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef __VIR_VHBA_H__
> +# define __VIR_VHBA_H__
> +
> +# include "internal.h"
> +
> +enum {
> + VPORT_CREATE,
> + VPORT_DELETE,
> +};
> +
> +bool virVHBAPathExists(const char *sysfs_prefix, int host);
I know that we use this format in almost all of our headers. Since you are
moving it to new header it would be nice to follow the same format as for
source files: return value on separate line.
I would like to change int for the whole libvirt some day and we have to
start somewhere :).
I'll leave it up to you.
> +
> +bool virVHBAIsVportCapable(const char *sysfs_prefix, int host);
> +
> +char *virVHBAGetConfig(const char *sysfs_prefix,
> + int host,
> + const char *entry)
> + ATTRIBUTE_NONNULL(3);
> +
> +char *virVHBAFindVportHost(const char *sysfs_prefix);
> +
> +int virVHBAManageVport(const int parent_host,
> + const char *wwpn,
> + const char *wwnn,
> + int operation)
> + ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
> +
> +char *virVHBAGetHostByWWN(const char *sysfs_prefix,
> + const char *wwnn,
> + const char *wwpn)
> + ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
> +
> +char *virVHBAGetHostByFabricWWN(const char *sysfs_prefix,
> + const char *fabric_wwn)
> + ATTRIBUTE_NONNULL(2);
> +
> +#endif /* __VIR_VBHA_H__ */
[...]
Conditional ACK, I would like to see your opinion about the comment.
Pavel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170217/e1d1bf5b/attachment-0001.sig>
More information about the libvir-list
mailing list