Re: [libvirt] [PATCH 06/15] util: Create a new virvhba module and move/rename API's

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(a)redhat.com&gt; > ---

> +/* 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. > + */

> +/* 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; > +} > + > +

> --- /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