[libvirt] rebased multipath patch

[Daniel P. Berrange]

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

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

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

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



Allowing the removal of all the retval = -1 assignments
during the method.

> +    }
> +
> +    if (!(strcmp(target_type, "multipath"))) {

Can use STRNEQ() here


ACK if those minor things are cleaned up


Daniel
-- 
|: Red Hat, Engineering, London   -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 :|