On Fri, Feb 17, 2017 at 11:42:54AM -0500, John Ferlan wrote:
>
>
> 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.
We don't need to be that strict :) renaming function is OK. This was more
like a general note, after reading the code it was actually pretty straight
forward. I was afraid that there is more changes.
> 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@xxxxxxxxxx>
> >> ---
>
> [...]
>
> >> +/* 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...
I should have been more specific, the "caller must free @result after" sounds
kind of odd, like it's missing the end of the sentence.
How about "caller is responsible for freeing the @result".
>
> ;-) Should the reworded comment be a separate commit...
Well, if you mention in commit message that the comments where improved
that's good enough and there is no need for 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 you adjust the comment it would be better, the event itself is not used
while creating new VHBA device so it was a little bit confusing to me.
>
> >> + 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...
The only benefit is to make it consistent. If you write some new function
you can just simply copy those lines and you don't have to adjust the
indentation and another benefit is if the return type is some type
with really long name it makes it nicer.
I don't mean this as an issue, it's just my attempt to make the change
and use the same construct in header and source files.
ACK with the comments fixed.
Pavel
>
> 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@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
