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

[Daniel Veillard]

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.

[...]
> 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

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

[...]
> +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.

> +    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 ?

[...]
> +        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

> +    /* These are props of the physical device */
> +    HAL_GET_PROPERTY(devname, "scsi.host", &derr, int, host);
> +    HAL_GET_PROPERTY(devname, "scsi.bus", &derr, int, bus);
> +    HAL_GET_PROPERTY(devname, "scsi.target", &derr, int, target);
> +    HAL_GET_PROPERTY(devname, "scsi.lun", &derr, int, lun);
> +
> +    /* These are props of the logic device */
> +    HAL_GET_PROPERTY(strdevs[0], "block.device", &derr, string, dev);
> +    /*
> +     * XXX storage.serial is not actually unique if they have
> +     * multipath on the fibre channel adapter
> +     */
> +    HAL_GET_PROPERTY(strdevs[0], "storage.serial", &derr, string, key);
> +    HAL_GET_PROPERTY(strdevs[0], "storage.size", &derr, uint64, size);
> +
> +#undef HAL_GET_PROPERTY

Looks good, and will make easier to test the simple storage management 
on developpers machines, +1

  thanks !

Daniel

-- 
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard      | virtualization library  http://libvirt.org/
veillard at redhat.com  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine  http://rpmfind.net/