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

On 02/17/2017 11:00 AM, Pavel Hrdina wrote: > 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. > While I understand your sentiment, the series was already at 15 patches. Changing the names in a separate patch didn't feel "right" since I was creating a new util module... I suppose an argument could also be made that every changed function name should have it's own separate patch. That was just too tedious and way too much work for the same result.

Patch 9 has a similar move/rename going on... The changes in patch 3 were really innocuous - I think it was combining an "if" construct, fixing the == -1, and adding more comments >> >> 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. > This is the same comment as the former virReadFCHost. I'm open to suggestions as for a rewording...

;-) Should the reworded comment be a separate commit...

>> + * 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. > No it's not necessary, it's a shorthand explanation for what's going on... The "event" posted by the kernel is a udev event which is recognized by udevEventHandleCallback. That in turn calls udevAddOneDevice which eventually calls virNodeDeviceEventLifecycleNew to create the libvirt event that applications can key off of. I can adjust or drop the comment - it doesn't really matter, but for anyone that stumbles headfirst into this code - I believe it provides an explanation regarding what's about to take place. There's then enough bread crumbs for that poor hapless soul to attempt to figure out how this stuff works...

>> + 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. > Doesn't matter to me - I can make it two lines... Guess I'm following what I think I've seen in most header files. In source files we also try to have one argument per line; whereas, for header files that's not always followed. Doing as you suggest just increases the number of lines - I really don't mind either way, but I do believe the majority of the header files follow this construct. I'm not sure I see value in the multiline header construct, but I'm not opposed to it... Of course the same comment will be made in patch 9 I suppose...

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