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@xxxxxxxxxx | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list