2:24 a.m.
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(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/