[libvirt] [PATCH v2] storage: Disallow usage of the HBA for a fc_host backing

[Erik Skultety]

On Tue, Jul 25, 2017 at 07:36:48AM -0400, John Ferlan wrote:
>
>
> On 07/25/2017 03:45 AM, Erik Skultety wrote:
> >> +/**
> >> + * @name: Name from a wwnn/wwpn lookup
> >> + *
> >> + * Validate that the @name fetched from the wwnn/wwpn is a vHBA and not
> >> + * an HBA as that should be a configuration error. It's only possible to
> >> + * use an existing wwnn/wwpn of a vHBA because that's what someone would
> >> + * have created using the node device create functionality. Using the
> >> + * HBA "just because" it has a wwnn/wwpn and the characteristics of a
> >> + * vHBA is just not valid
> >> + *
> >> + * Returns the @scsi_host_name to be used or NULL on errror

s/errror/error

...

> >> @@ -275,6 +316,7 @@ createVport(virConnectPtr conn,
> >>              virStorageAdapterFCHostPtr fchost)
> >>  {
> >>      char *name = NULL;
> >> +    char *scsi_host_name = NULL;
> >>      virStoragePoolFCRefreshInfoPtr cbdata = NULL;
> >>      virThread thread;
> >>      int ret = -1;
> >> @@ -288,6 +330,9 @@ createVport(virConnectPtr conn,
> >>       * this pool and we don't have to create the vHBA
> >>       */
> >>      if ((name = virVHBAGetHostByWWN(NULL, fchost->wwnn, fchost->wwpn))) {
> >> +        if (!(scsi_host_name = checkName(name)))
> >> +            goto cleanup;
> >
> > Ok, so I learn from my mistakes, do you plan on building upon this in the
> > foreseeable future with a follow-up series you haven't sent yet, but have on a
> > local branch? Because if not, I don't really see a point in returning a string
> > which only gets free'd on the function exit - checkName should probably become
> > something like "isVHBA" and return boolean. And even if you do have a follow-up
> > on a local branch, I still think that converting the return value should be
> > part of that series, solely because until then, @scsi_host_name wouldn't have
> > any meaning.
> >
> > Erik
> >
>
> No this is it - I want to stop thinking about NPIV for a while... I
> returned the 'scsi_host_name' string because in my mind I had passed it
> to checkParent, but apparently I didn't do that, sigh.
>
> Does that make more sense now?

Indeed, to be honest I missed the connection between name and scsi_host_name
and thought, yeah ok makes sense (probably have seen quite a lot NPIV lately
myself)....now that I see it a bit more clearly, it still left me wondering if
it wouldn't make more sense to move the scsi_host_name formatting part to
createVport (you free it here after all) right after you pass @name to
checkName/isVHBA (whatever we settle on):
    - you don't need to format it prior to your checkName, since backwards
      compatibility takes care of eating "hostX" nicely
    - you also don't need to report any error when virSCSIHostGetNumber fails,
      since one gets formatted already and it didn't bring much value

-> then, right after that call you actually format the "scsi_" name since you
really need it to traverse the list of devices and find the parent in
checkParent.

Erik

>
> John