[Libvir] PATCH: Add SCSI HBA backend for storage driver

[Daniel P. Berrange]

On Tue, Mar 25, 2008 at 03:24:48AM -0400, Daniel Veillard wrote:
> On Mon, Mar 24, 2008 at 11:15:47PM +0000, Daniel P. Berrange wrote:
> > And this time with the patch included...
> [...]
> >  PARTED_REQUIRED="1.8.0"
> > +HAL_REQUIRED="0.5.0"
> 
>   With HAL being in Solaris now, maybe that could even be made non-linux
> specific
> 
> [...]
> > @@ -584,6 +585,8 @@ AC_ARG_WITH(storage-iscsi,
> >  [  --with-storage-iscsi        with iSCSI backend for the storage driver (on)],[],[with_storage_iscsi=check])
> >  AC_ARG_WITH(storage-disk,
> >  [  --with-storage-disk         with GPartd Disk backend for the storage driver (on)],[],[with_storage_disk=check])
> > +AC_ARG_WITH(storage-scsi,
> > +[  --with-storage-scsi         with HAL SCSI Disk backend for the storage driver (on)],[],[with_storage_scsi=check])
> >  
> >  if test "$with_storage_fs" = "yes" -o "$with_storage_fs" = "check"; then
> >    AC_PATH_PROG(MOUNT, [mount], [], [$PATH:/sbin:/usr/sbin])
> > @@ -741,6 +744,30 @@ AC_SUBST(LIBPARTED_CFLAGS)
> >  AC_SUBST(LIBPARTED_LIBS)
> 
>   maybe a meta option --with-storage giving a default to the group of options
> (that now 5 of them) could be helpful for reduced setups.

Nah, you never need to use any of the --with-storage-XXX options in
normal circumstances. If you leave them out, it will automatically
probe and enable/disable as appropriate. You only need to use the
--with options if you want to force configure to abort if probing
fails.

> [...]
> > diff -N src/storage_backend_scsi.c
> [...]
> > +#define LINUX_SYSFS_SCSI_HOST_PREFIX "/sys/class/scsi_host/"
> > +#define LINUX_SYSFS_SCSI_HOST_POSTFIX "/device"
> 
>   hopefully we can add Solaris equivalent later

Yeah, or change the way the Linux bit works so it sucks less. I'm
still undecided on this particular bit of code. I'm using sysfs
because I'll need to use it anyway when I add NPIV support. Then
again NPIV support may work out differently to how I expect - I'll
be getting access to some real NPIV hardware soon to verify...

> > +    if (strlen(absLink) >= buflen) {
> > +        virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR,
> > +                              "link too long for buffer");
> > +        return -1;
> > +    }
> > +    strcpy(buf, absLink);
> 
>   Even if just checked can we still use strncpy(), i assume at some point
> we will add an automatic check against strcpy 

Sure, will do.

> [...]
> > +virStorageBackendSCSICreateVol(virConnectPtr conn,
> > +                               virStoragePoolObjPtr pool,
> > +                               const char *name,
> > +                               const char *path,
> > +                               const char *key,
> > +                               unsigned long long size)
> > +{
> > +    virStorageVolDefPtr vol;
> > +    char *tmppath;
> > +
> > +    if ((vol = calloc(1, sizeof(*vol))) == NULL) {
> > +        virStorageReportError(conn, VIR_ERR_NO_MEMORY, "%s", _("volume"));
> > +        goto cleanup;
> > +    }
> 
>     maybe check that size is greater than some absolute minimum like 1 MByte
> (and maximum size ?) this could also allow to catch some conversion errors 
> or 0 being passed.

This size value comes straight from HAL, as is the LUN size. The code
will re-probe this value a short while later when we open the actual
device node to lookup permissions/ownership, so I think its overkill
to check size here too.

> > +    tmppath = strdup(path);
> [...]
> > +    if ((vol->target.path = virStorageBackendStablePath(conn,
> > +                                                        pool,
> > +                                                        tmppath)) == NULL)
> > +        goto cleanup;
> > +
> > +    if (tmppath != vol->target.path)
> > +        free(tmppath);
> > +    tmppath = NULL;
> 
>   it's a bit funky, you mean virStorageBackendStablePath could just grab the
> parameter string or not and that need to be checked later to decide if it needs
> freeing ? Even for an internal API it's a bit dangerous IMHO, what if 
> it is called with a constant string path, let's avoid the potential problem
> get virStorageBackendStablePath to always strdup if it reuses the parameter,
> no ?

Yeah, guess I should change the contract of that method. Its a little ugly.
All the other drivers work in this way though, so I'll do that cleanup as
a separate patch.

> > +        goto cleanup;
> > +    }
> > +
> > +#define HAL_GET_PROPERTY(path, name, err, type, value)                  \
> > +    (value) = libhal_device_get_property_ ## type(ctx, (path), (name), (err)); \
> > +    if (dbus_error_is_set((err))) {                                     \
> > +        virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR,             \
> > +                              "unable to lookup '%s' property on %s: %s", \
> > +                              (name), (path), derr.message);            \
> > +        goto cleanup;                                                   \
> > +    }
> 
>   i'm not too fond of macros defined in the function code, move it just before
> Also for token-pasting I though one needed to glue the # and the identifiers
> like foo##bar , i'm surprized foo ## bar actually works

Works for GCC at least, but it could be that's a non-portable GCC extension

Dan.
-- 
|: Red Hat, Engineering, Boston   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|