On Thu, Sep 03, 2009 at 06:03:00PM +0100, Daniel P. Berrange wrote:
> On Wed, Sep 02, 2009 at 11:28:27AM -0400, Dave Allan wrote:
> > @@ -1177,6 +1180,26 @@ if test "$with_storage_scsi" = "check"; then
> > fi
> > AM_CONDITIONAL([WITH_STORAGE_SCSI], [test "$with_storage_scsi" = "yes"])
> >
> > +if test "$with_storage_mpath" = "check"; then
> > + with_storage_mpath=yes
> > +
> > + AC_DEFINE_UNQUOTED([WITH_STORAGE_MPATH], 1,
> > + [whether mpath backend for storage driver is enabled])
> > +fi
> > +AM_CONDITIONAL([WITH_STORAGE_MPATH], [test "$with_storage_mpath" = "yes"])
> > +
> > +if test "$with_storage_mpath" = "yes"; then
> > + DEVMAPPER_REQUIRED=0.0
> > + DEVMAPPER_CFLAGS=
> > + DEVMAPPER_LIBS=
> > + PKG_CHECK_MODULES(DEVMAPPER, devmapper >= $DEVMAPPER_REQUIRED,
> > + [], [
> > + AC_MSG_ERROR(
> > + [You must install device-mapper-devel >= $DEVMAPPER_REQUIRED to compile libvirt])
> > + ])
> > +fi
> > +AC_SUBST([DEVMAPPER_CFLAGS])
> > +AC_SUBST([DEVMAPPER_LIBS])
>
>
> Need to update livbvirt.spec.in with a dependancy on device mapper for this
> tool, both Requires & BuildRequries
Okay, I will add this as a separate patch
> > diff --git a/src/Makefile.am b/src/Makefile.am
> > index bedeb84..cf3420b 100644
> > --- a/src/Makefile.am
> > +++ b/src/Makefile.am
> > @@ -199,6 +199,9 @@ STORAGE_DRIVER_ISCSI_SOURCES = \
> > STORAGE_DRIVER_SCSI_SOURCES = \
> > storage_backend_scsi.h storage_backend_scsi.c
> >
> > +STORAGE_DRIVER_MPATH_SOURCES = \
> > + storage_backend_mpath.h storage_backend_mpath.c
> > +
> > STORAGE_DRIVER_DISK_SOURCES = \
> > storage_backend_disk.h storage_backend_disk.c
> >
> > @@ -478,6 +481,10 @@ if WITH_STORAGE_SCSI
> > libvirt_driver_storage_la_SOURCES += $(STORAGE_DRIVER_SCSI_SOURCES)
> > endif
> >
> > +if WITH_STORAGE_MPATH
> > +libvirt_driver_storage_la_SOURCES += $(STORAGE_DRIVER_MPATH_SOURCES)
> > +endif
> > +
> > if WITH_STORAGE_DISK
> > libvirt_driver_storage_la_SOURCES += $(STORAGE_DRIVER_DISK_SOURCES)
> > endif
> > @@ -539,6 +546,7 @@ EXTRA_DIST += \
> > $(STORAGE_DRIVER_LVM_SOURCES) \
> > $(STORAGE_DRIVER_ISCSI_SOURCES) \
> > $(STORAGE_DRIVER_SCSI_SOURCES) \
> > + $(STORAGE_DRIVER_MPATH_SOURCES) \
> > $(STORAGE_DRIVER_DISK_SOURCES) \
> > $(NODE_DEVICE_DRIVER_SOURCES) \
> > $(NODE_DEVICE_DRIVER_HAL_SOURCES) \
> > @@ -607,6 +615,7 @@ libvirt_la_LDFLAGS = $(VERSION_SCRIPT_FLAGS)libvirt.syms \
> > $(COVERAGE_CFLAGS:-f%=-Wc,-f%) \
> > $(LIBXML_LIBS) $(SELINUX_LIBS) \
> > $(XEN_LIBS) $(DRIVER_MODULE_LIBS) \
> > + $(DEVMAPPER_LIBS) \
> > @CYGWIN_EXTRA_LDFLAGS@ @MINGW_EXTRA_LDFLAGS@
> > libvirt_la_CFLAGS = $(COVERAGE_CFLAGS) -DIN_LIBVIRT
> > libvirt_la_DEPENDENCIES = $(libvirt_la_LIBADD) libvirt.syms
>
>
> I think you probably need a $(DEVMAPPER_CFLAGS) somewhere in here too,
> even if it happens to be empty for default Fedora installs.
Added in
if WITH_STORAGE_MPATH
libvirt_driver_storage_la_SOURCES += $(STORAGE_DRIVER_MPATH_SOURCES)
libvirt_driver_storage_la_CFLAGS += $(DEVMAPPER_CFLAGS)
endif
> > +static int
> > +virStorageBackendMpathUpdateVolTargetInfo(virConnectPtr conn,
> > + virStorageVolTargetPtr target,
> > + unsigned long long *allocation,
> > + unsigned long long *capacity)
> > +{
> > + int ret = 0;
> > + int fd = -1;
> > +
> > + if ((fd = open(target->path, O_RDONLY)) < 0) {
> > + virReportSystemError(conn, errno,
> > + _("cannot open volume '%s'"),
> > + target->path);
> > + ret = -1;
> > + goto out;
> > + }
> > +
> > + if (virStorageBackendUpdateVolTargetInfoFD(conn,
> > + target,
> > + fd,
> > + allocation,
> > + capacity) < 0) {
> > +
> > + VIR_ERROR(_("Failed to update volume target info for %s"),
> > + target->path);
> > +
> > + ret = -1;
> > + goto out;
> > + }
> > +
> > + if (virStorageBackendUpdateVolTargetFormatFD(conn,
> > + target,
> > + fd) < 0) {
> > + VIR_ERROR(_("Failed to update volume target format for %s"),
> > + target->path);
> > +
> > + ret = -1;
> > + goto out;
> > + }
>
> These two VIR_EROR calls should be virRaiseError() or similar
> if we're going to return a fail status.
Done using the proper virStorageReportError()
> > +static int
> > +virStorageBackendMpathNewVol(virConnectPtr conn,
> > + virStoragePoolObjPtr pool,
> > + const int devnum,
> > + const char *dev)
> > +{
> > + virStorageVolDefPtr vol;
> > + int retval = 0;
> > +
> > + if (VIR_ALLOC(vol) < 0) {
> > + virReportOOMError(conn);
> > + retval = -1;
> > + goto out;
> > + }
> > +
> > + vol->type = VIR_STORAGE_VOL_BLOCK;
> > +
> > + if (virAsprintf(&(vol->name), "dm-%u", devnum) < 0) {
> > + virReportOOMError(conn);
> > + retval = -1;
> > + goto free_vol;
> > + }
> > +
> > + if (virAsprintf(&vol->target.path, "/dev/%s", dev) < 0) {
> > + virReportOOMError(conn);
> > + retval = -1;
> > + goto free_vol;
> > + }
> > +
> > + if (virStorageBackendMpathUpdateVolTargetInfo(conn,
> > + &vol->target,
> > + &vol->allocation,
> > + &vol->capacity) < 0) {
> > +
> > + virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR,
> > + _("Failed to update volume for '%s'"),
> > + vol->target.path);
> > + retval = -1;
> > + goto free_vol;
> > + }
> > +
> > + /* XXX should use logical unit's UUID instead */
> > + vol->key = strdup(vol->target.path);
> > + if (vol->key == NULL) {
> > + virReportOOMError(conn);
> > + retval = -1;
> > + goto free_vol;
> > + }
> > +
> > + pool->def->capacity += vol->capacity;
> > + pool->def->allocation += vol->allocation;
> > +
> > + if (VIR_REALLOC_N(pool->volumes.objs,
> > + pool->volumes.count + 1) < 0) {
> > + virReportOOMError(conn);
> > + retval = -1;
> > + goto free_vol;
> > + }
> > + pool->volumes.objs[pool->volumes.count++] = vol;
> > +
>
> If 'retval' is declared as -1 initially, then
>
> > + goto out;
> > +
> > +free_vol:
> > + virStorageVolDefFree(vol);
> > +out:
> > + return retval;
> > +}
>
> could be simplified with
>
>
> ret = 0;
>
> cleanup:
> if (ret != 0)
> virStorageVolDefFree(vol);
>
> return ret;
I also moved
pool->def->capacity += vol->capacity;
pool->def->allocation += vol->allocation;
just before exiting to avoid incleasing the capacity in case the
VIR_REALLOC_N(pool->volumes.objs) could fail.
>
>
> Allowing the removal of all the retval = -1 assignments
> during the method.
>
> > + }
> > +
> > + if (!(strcmp(target_type, "multipath"))) {
>
> Can use STRNEQ() here
I would assume that virStorageBackendIsMultipath() would
return 1 if target_type is actually "multipath", and hence
use
if (STREQ(target_type, "multipath")) {
ret = 1;
}
ret being initialized to 0 at the beginning :-)
>
> ACK if those minor things are cleaned up
I also fixed the Copyrights on the new code
Applied and commited, thanks !
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list