September 2009 - Devel - libvirt List Archives

[libvirt] Free name in xenDaemonLookupByID(), if virGetDomain() fails
by Matthias Bolte 09 Sep '09

09 Sep '09

Subject says it all. Matthias

2 3

[libvirt] rebased multipath patch
by Dave Allan 09 Sep '09

09 Sep '09

Here's the multipath patch, rebased against the current head. Dave >From 52cb0ff30dd5a52c5a246f3a2022f933cd2a0eab Mon Sep 17 00:00:00 2001 From: David Allan <dallan(a)redhat.com> Date: Tue, 21 Jul 2009 12:43:01 -0400 Subject: [PATCH] Add a simple pool type for multipath devices This pool type contains volumes for the multipath devices that are present on the host. It does not (yet) support any sort of multipath configuration, so that aspect of system administration must still be done at host build time. --- configure.in | 24 +++ po/POTFILES.in | 1 + src/Makefile.am | 9 + src/storage_backend.c | 82 ++++++++++ src/storage_backend.h | 5 +- src/storage_backend_mpath.c | 352 +++++++++++++++++++++++++++++++++++++++++++ src/storage_backend_mpath.h | 31 ++++ src/storage_conf.c | 7 +- src/storage_conf.h | 1 + 9 files changed, 510 insertions(+), 2 deletions(-) create mode 100644 src/storage_backend_mpath.c create mode 100644 src/storage_backend_mpath.h diff --git a/configure.in b/configure.in index d28c44a..7c93ab7 100644 --- a/configure.in +++ b/configure.in @@ -1046,6 +1046,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-scsi], [ --with-storage-scsi with SCSI backend for the storage driver (on)],[],[with_storage_scsi=check]) +AC_ARG_WITH([storage-mpath], +[ --with-storage-mpath with mpath backend for the storage driver (on)],[],[with_storage_mpath=check]) AC_ARG_WITH([storage-disk], [ --with-storage-disk with GPartd Disk backend for the storage driver (on)],[],[with_storage_disk=check]) @@ -1056,6 +1058,7 @@ if test "$with_libvirtd" = "no"; then with_storage_lvm=no with_storage_iscsi=no with_storage_scsi=no + with_storage_mpath=no with_storage_disk=no fi if test "$with_storage_dir" = "yes" ; then @@ -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]) LIBPARTED_CFLAGS= LIBPARTED_LIBS= @@ -1677,6 +1700,7 @@ AC_MSG_NOTICE([ NetFS: $with_storage_fs]) AC_MSG_NOTICE([ LVM: $with_storage_lvm]) AC_MSG_NOTICE([ iSCSI: $with_storage_iscsi]) AC_MSG_NOTICE([ SCSI: $with_storage_scsi]) +AC_MSG_NOTICE([ mpath: $with_storage_mpath]) AC_MSG_NOTICE([ Disk: $with_storage_disk]) AC_MSG_NOTICE([]) AC_MSG_NOTICE([Security Drivers]) diff --git a/po/POTFILES.in b/po/POTFILES.in index 0a53dab..1586368 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -38,6 +38,7 @@ src/storage_backend_fs.c src/storage_backend_iscsi.c src/storage_backend_logical.c src/storage_backend_scsi.c +src/storage_backend_mpath.c src/storage_conf.c src/storage_driver.c src/storage_encryption_conf.c 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 diff --git a/src/storage_backend.c b/src/storage_backend.c index c818142..4dec06b 100644 --- a/src/storage_backend.c +++ b/src/storage_backend.c @@ -59,6 +59,9 @@ #if WITH_STORAGE_SCSI #include "storage_backend_scsi.h" #endif +#if WITH_STORAGE_MPATH +#include "storage_backend_mpath.h" +#endif #if WITH_STORAGE_DISK #include "storage_backend_disk.h" #endif @@ -89,6 +92,9 @@ static virStorageBackendPtr backends[] = { #if WITH_STORAGE_SCSI &virStorageBackendSCSI, #endif +#if WITH_STORAGE_MPATH + &virStorageBackendMpath, +#endif #if WITH_STORAGE_DISK &virStorageBackendDisk, #endif @@ -812,6 +818,82 @@ virStorageBackendUpdateVolTargetInfoFD(virConnectPtr conn, return 0; } + +struct diskType { + int part_table_type; + unsigned short offset; + unsigned short length; + unsigned long long magic; +}; + + +static struct diskType const disk_types[] = { + { VIR_STORAGE_POOL_DISK_LVM2, 0x218, 8, 0x31303020324D564CULL }, + { VIR_STORAGE_POOL_DISK_GPT, 0x200, 8, 0x5452415020494645ULL }, + { VIR_STORAGE_POOL_DISK_DVH, 0x0, 4, 0x41A9E50BULL }, + { VIR_STORAGE_POOL_DISK_MAC, 0x0, 2, 0x5245ULL }, + { VIR_STORAGE_POOL_DISK_BSD, 0x40, 4, 0x82564557ULL }, + { VIR_STORAGE_POOL_DISK_SUN, 0x1fc, 2, 0xBEDAULL }, + /* + * NOTE: pc98 is funky; the actual signature is 0x55AA (just like dos), so + * we can't use that. At the moment I'm relying on the "dummy" IPL + * bootloader data that comes from parted. Luckily, the chances of running + * into a pc98 machine running libvirt are approximately nil. + */ + /*{ 0x1fe, 2, 0xAA55UL },*/ + { VIR_STORAGE_POOL_DISK_PC98, 0x0, 8, 0x314C5049000000CBULL }, + /* + * NOTE: the order is important here; some other disk types (like GPT and + * and PC98) also have 0x55AA at this offset. For that reason, the DOS + * one must be the last one. + */ + { VIR_STORAGE_POOL_DISK_DOS, 0x1fe, 2, 0xAA55ULL }, + { -1, 0x0, 0, 0x0ULL }, +}; + + +int +virStorageBackendUpdateVolTargetFormatFD(virConnectPtr conn, + virStorageVolTargetPtr target, + int fd) +{ + int i; + off_t start; + unsigned char buffer[1024]; + ssize_t bytes; + + /* make sure to set the target format "unknown" to begin with */ + target->format = VIR_STORAGE_POOL_DISK_UNKNOWN; + + start = lseek(fd, 0, SEEK_SET); + if (start < 0) { + virReportSystemError(conn, errno, + _("cannot seek to beginning of file '%s'"), + target->path); + return -1; + } + bytes = saferead(fd, buffer, sizeof(buffer)); + if (bytes < 0) { + virReportSystemError(conn, errno, + _("cannot read beginning of file '%s'"), + target->path); + return -1; + } + + for (i = 0; disk_types[i].part_table_type != -1; i++) { + if (disk_types[i].offset + disk_types[i].length > bytes) + continue; + if (memcmp(buffer+disk_types[i].offset, &disk_types[i].magic, + disk_types[i].length) == 0) { + target->format = disk_types[i].part_table_type; + break; + } + } + + return 0; +} + + void virStorageBackendWaitForDevices(virConnectPtr conn) { virWaitForDevices(conn); diff --git a/src/storage_backend.h b/src/storage_backend.h index e80b05d..eb5bf87 100644 --- a/src/storage_backend.h +++ b/src/storage_backend.h @@ -91,7 +91,10 @@ int virStorageBackendUpdateVolTargetInfoFD(virConnectPtr conn, int fd, unsigned long long *allocation, unsigned long long *capacity); - +int +virStorageBackendUpdateVolTargetFormatFD(virConnectPtr conn, + virStorageVolTargetPtr target, + int fd); void virStorageBackendWaitForDevices(virConnectPtr conn); char *virStorageBackendStablePath(virConnectPtr conn, diff --git a/src/storage_backend_mpath.c b/src/storage_backend_mpath.c new file mode 100644 index 0000000..41dadf1 --- /dev/null +++ b/src/storage_backend_mpath.c @@ -0,0 +1,352 @@ +/* + * storage_backend_mpath.c: storage backend for multipath handling + * + * Copyright (C) 2007-2008 Red Hat, Inc. + * Copyright (C) 2007-2008 Daniel P. Berrange + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + * Author: Daniel P. Berrange <berrange redhat com> + */ + +#include <config.h> + +#include <unistd.h> +#include <stdio.h> +#include <dirent.h> +#include <fcntl.h> + +#include <libdevmapper.h> + +#include "virterror_internal.h" +#include "storage_conf.h" +#include "storage_backend.h" +#include "memory.h" +#include "logging.h" + +#define VIR_FROM_THIS VIR_FROM_STORAGE + +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; + } + +out: + if (fd != -1) { + close(fd); + } + return ret; +} + + +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; + + goto out; + +free_vol: + virStorageVolDefFree(vol); +out: + return retval; +} + + +static int +virStorageBackendIsMultipath(const char *devname) +{ + int ret = 0; + struct dm_task *dmt = NULL; + void *next = NULL; + uint64_t start, length; + char *target_type = NULL; + char *params = NULL; + + dmt = dm_task_create(DM_DEVICE_TABLE); + if (dmt == NULL) { + ret = -1; + goto out; + } + + if (dm_task_set_name(dmt, devname) == 0) { + ret = -1; + goto out; + } + + dm_task_no_open_count(dmt); + + if (!dm_task_run(dmt)) { + ret = -1; + goto out; + } + + next = dm_get_next_target(dmt, next, &start, &length, + &target_type, &params); + + if (target_type == NULL) { + ret = -1; + goto out; + } + + if (!(strcmp(target_type, "multipath"))) { + ret = 1; + } + +out: + if (dmt != NULL) { + dm_task_destroy(dmt); + } + return ret; +} + + +static int +virStorageBackendGetMinorNumber(const char *devname, uint32_t *minor) +{ + int ret = -1; + struct dm_task *dmt; + struct dm_info info; + + if (!(dmt = dm_task_create(DM_DEVICE_INFO))) { + goto out; + } + + if (!dm_task_set_name(dmt, devname)) { + goto out; + } + + if (!dm_task_run(dmt)) { + goto out; + } + + if (!dm_task_get_info(dmt, &info)) { + goto out; + } + + *minor = info.minor; + ret = 0; + +out: + if (dmt != NULL) { + dm_task_destroy(dmt); + } + + return ret; +} + + +static int +virStorageBackendCreateVols(virConnectPtr conn, + virStoragePoolObjPtr pool, + struct dm_names *names) +{ + int retval = 0, is_mpath = 0; + char *map_device = NULL; + uint32_t minor = -1; + + do { + is_mpath = virStorageBackendIsMultipath(names->name); + + if (is_mpath < 0) { + retval = -1; + goto out; + } + + if (is_mpath == 1) { + + if (virAsprintf(&map_device, "mapper/%s", names->name) < 0) { + virReportOOMError(conn); + retval = -1; + goto out; + } + + if (virStorageBackendGetMinorNumber(names->name, &minor) < 0) { + retval = -1; + goto out; + } + + virStorageBackendMpathNewVol(conn, + pool, + minor, + map_device); + + VIR_FREE(map_device); + } + + /* Given the way libdevmapper returns its data, I don't see + * any way to avoid this series of casts. */ + names = (struct dm_names *)(((char *)names) + names->next); + + } while (names->next); + +out: + return retval; +} + + +static int +virStorageBackendGetMaps(virConnectPtr conn, + virStoragePoolObjPtr pool) +{ + int retval = 0; + struct dm_task *dmt = NULL; + struct dm_names *names = NULL; + + if (!(dmt = dm_task_create(DM_DEVICE_LIST))) { + retval = 1; + goto out; + } + + dm_task_no_open_count(dmt); + + if (!dm_task_run(dmt)) { + retval = 1; + goto out; + } + + if (!(names = dm_task_get_names(dmt))) { + retval = 1; + goto out; + } + + if (!names->dev) { + /* No devices found */ + goto out; + } + + virStorageBackendCreateVols(conn, pool, names); + +out: + if (dmt != NULL) { + dm_task_destroy (dmt); + } + return retval; +} + + +static int +virStorageBackendMpathRefreshPool(virConnectPtr conn, + virStoragePoolObjPtr pool) +{ + int retval = 0; + + VIR_ERROR(_("in %s"), __func__); + + pool->def->allocation = pool->def->capacity = pool->def->available = 0; + + virStorageBackendWaitForDevices(conn); + + virStorageBackendGetMaps(conn, pool); + + return retval; +} + + +virStorageBackend virStorageBackendMpath = { + .type = VIR_STORAGE_POOL_MPATH, + + .refreshPool = virStorageBackendMpathRefreshPool, +}; diff --git a/src/storage_backend_mpath.h b/src/storage_backend_mpath.h new file mode 100644 index 0000000..9de2b79 --- /dev/null +++ b/src/storage_backend_mpath.h @@ -0,0 +1,31 @@ +/* + * storage_backend_scsi.h: storage backend for SCSI handling + * + * Copyright (C) 2007-2008 Red Hat, Inc. + * Copyright (C) 2007-2008 Daniel P. Berrange + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + * Author: Daniel P. Berrange <berrange redhat com> + */ + +#ifndef __VIR_STORAGE_BACKEND_MPATH_H__ +#define __VIR_STORAGE_BACKEND_MPATH_H__ + +#include "storage_backend.h" + +extern virStorageBackend virStorageBackendMpath; + +#endif /* __VIR_STORAGE_BACKEND_MPATH_H__ */ diff --git a/src/storage_conf.c b/src/storage_conf.c index c446069..af04080 100644 --- a/src/storage_conf.c +++ b/src/storage_conf.c @@ -53,7 +53,7 @@ VIR_ENUM_IMPL(virStoragePool, VIR_STORAGE_POOL_LAST, "dir", "fs", "netfs", "logical", "disk", "iscsi", - "scsi") + "scsi", "mpath") VIR_ENUM_IMPL(virStoragePoolFormatFileSystem, VIR_STORAGE_POOL_FS_LAST, @@ -198,6 +198,11 @@ static virStoragePoolTypeInfo poolTypeInfo[] = { .formatToString = virStoragePoolFormatDiskTypeToString, } }, + { .poolType = VIR_STORAGE_POOL_MPATH, + .volOptions = { + .formatToString = virStoragePoolFormatDiskTypeToString, + } + }, { .poolType = VIR_STORAGE_POOL_DISK, .poolOptions = { .flags = (VIR_STORAGE_POOL_SOURCE_DEVICE), diff --git a/src/storage_conf.h b/src/storage_conf.h index bcf9b93..421d305 100644 --- a/src/storage_conf.h +++ b/src/storage_conf.h @@ -119,6 +119,7 @@ enum virStoragePoolType { VIR_STORAGE_POOL_DISK, /* Disk partitions */ VIR_STORAGE_POOL_ISCSI, /* iSCSI targets */ VIR_STORAGE_POOL_SCSI, /* SCSI HBA */ + VIR_STORAGE_POOL_MPATH, /* Multipath devices */ VIR_STORAGE_POOL_LAST, }; -- 1.6.0.6

3 3

Re: [libvirt] [PATCH 04/10] Secret manipulation step 7: Local driver
by Miloslav Trmac 08 Sep '09

08 Sep '09

----- "Daniel Veillard" <veillard(a)redhat.com> wrote: > On Mon, Sep 07, 2009 at 04:12:39PM +0200, Miloslav Trmač wrote: > > + if ((size_t)st.st_size != st.st_size) { > > shouldn't we chaeck against SECRET_MAX_XML_FILE instead ? No, this code reads the secret value, not the XML, and there's little reason to impose an arbitrary limit on the size. SECRET_MAX_XML_FILE is a left-over from an earlier version, the attached updated patch removes the definition. Mirek

1 0

Re: [libvirt] [PATCH 03/10] Add an internal <secret> XML handling API
by Miloslav Trmac 08 Sep '09

08 Sep '09

----- "Daniel Veillard" <veillard(a)redhat.com> wrote: > > + default: > > + VIR_ERROR(_("unexpected secret usage type %d"), def->usage_type); > > + break; > > Hum, since the virSecretDefPtr is allocated by our own code, it's > probably better to remove the default so that the compiler can tell us > we missed one enum case if new ones gets added. > > + type_str = virXPathString(conn, "string(./usage/@type)", ctxt); > > + if (type_str == NULL) { > > + virSecretReportError(conn, VIR_ERR_XML_ERROR, "%s", > > + _("unknown secret usage type")); > > _("missing secret usage type") would be more appropriate I guess > > + case VIR_SECRET_USAGE_TYPE_VOLUME: > > + def->usage.volume = virXPathString(conn, "string(./usage/volume)", > > + ctxt); > > + break; > > + > > + default: > > Again default: here means a mismatch between > virSecretUsageTypeTypeFromString and this function, best handled > statically IMHO. Thanks for the review, attached is an updated patch. Mirek

1 0

Re: [libvirt] [PATCH 02/10] Add VIR_SECRET_GET_VALUE_INTERNAL_CALL.
by Miloslav Trmac 08 Sep '09

08 Sep '09

----- "Daniel Veillard" <veillard(a)redhat.com> wrote: > > +/* Make sure ... INTERNAL_CALL can not be set by the caller */ > > +verify((VIR_SECRET_GET_VALUE_INTERNAL_CALL & > > + VIR_SECRET_GET_VALUE_FLAGS_MASK) == 0); > > ??? what's that ? an assert at compile time ? I don't know that construct > I would rather avoid it if it's not portable. Yes, a compile-time assert, provided by gnulib. The mechanism depends only on ISO C, and gnulib/lib/verify.h contains detailed comments about why the particular variant was chosen. It seems safe enough to me. Mirek

1 0

[libvirt] [PATCH]: Fixed minor bugs in display and added OOM checks.
by Pritesh Kothari 08 Sep '09

08 Sep '09

Hi All, There was a minor bug in selecting the graphics type. if the graphics type was desktop it was assumed that display is set for it, and thus crashed on strdup, so now checking if display is present before setting it. The second bug was while setting the 3d acceleration parameter, VIR_ALLOC() returns 0 or greater if success and not the other way round. Lastly added OOM checks in few places which were missed earlier. Regards, Pritesh

3 4

[libvirt] [PATCH] 6 dead-store patches
by Jim Meyering 08 Sep '09

08 Sep '09

>From 129dc57243df4e73daa24aac671087bcd25f51ad Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering(a)redhat.com> Date: Fri, 4 Sep 2009 16:49:37 +0200 Subject: [PATCH 1/6] xm_internal.c: remove dead stores of local, "type" * src/xm_internal.c (xenXMDomainConfigParse): Remove declaration and useless containing if-block, too. --- src/xm_internal.c | 9 +-------- 1 files changed, 1 insertions(+), 8 deletions(-) diff --git a/src/xm_internal.c b/src/xm_internal.c index 9627ffb..661172b 100644 --- a/src/xm_internal.c +++ b/src/xm_internal.c @@ -1,7 +1,7 @@ /* * xm_internal.h: helper routines for dealing with inactive domains * - * Copyright (C) 2006-2007 Red Hat + * Copyright (C) 2006-2007, 2009 Red Hat * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -995,7 +995,6 @@ xenXMDomainConfigParse(virConnectPtr conn, virConfPtr conf) { if (list && list->type == VIR_CONF_LIST) { list = list->list; while (list) { - int type = -1; char script[PATH_MAX]; char model[10]; char ip[16]; @@ -1031,7 +1030,6 @@ xenXMDomainConfigParse(virConnectPtr conn, virConfPtr conf) { mac[len] = '\0'; } else if (STRPREFIX(key, "bridge=")) { int len = nextkey ? (nextkey - data) : sizeof(bridge)-1; - type = 1; if (len > (sizeof(bridge)-1)) len = sizeof(bridge)-1; strncpy(bridge, data, len); @@ -1069,11 +1067,6 @@ xenXMDomainConfigParse(virConnectPtr conn, virConfPtr conf) { key = nextkey; } - /* XXX Forcing to pretend its a bridge */ - if (type == -1) { - type = 1; - } - if (VIR_ALLOC(net) < 0) goto cleanup; -- 1.6.4.2.419.gab238 >From b8b1ea4894ac9bf2a59472958a8bb0749526847f Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering(a)redhat.com> Date: Fri, 4 Sep 2009 17:22:19 +0200 Subject: [PATCH 2/6] xm_internal.c: remove two ret=... dead stores * src/xm_internal.c (xenXMDomainCreate): Remove dead stores. --- src/xm_internal.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/xm_internal.c b/src/xm_internal.c index 661172b..de3aca9 100644 --- a/src/xm_internal.c +++ b/src/xm_internal.c @@ -1850,10 +1850,10 @@ int xenXMDomainCreate(virDomainPtr domain) { goto error; domain->id = ret; - if ((ret = xend_wait_for_devices(domain->conn, domain->name)) < 0) + if (xend_wait_for_devices(domain->conn, domain->name) < 0) goto error; - if ((ret = xenDaemonDomainResume(domain)) < 0) + if (xenDaemonDomainResume(domain) < 0) goto error; xenUnifiedUnlock(priv); -- 1.6.4.2.419.gab238 >From a74d747d27bde38ece90df9a14acb648d83b9993 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering(a)redhat.com> Date: Fri, 4 Sep 2009 17:27:34 +0200 Subject: [PATCH 3/6] domain_conf.c: remove two dead stores * src/domain_conf.c (virDomainSaveXML): Remove use and decl of "err". (virDomainDefParseXML): Likewise. --- src/domain_conf.c | 6 ++---- 1 files changed, 2 insertions(+), 4 deletions(-) diff --git a/src/domain_conf.c b/src/domain_conf.c index 8dde5dd..050cf50 100644 --- a/src/domain_conf.c +++ b/src/domain_conf.c @@ -2520,8 +2520,7 @@ static virDomainDefPtr virDomainDefParseXML(virConnectPtr conn, /* Extract domain uuid */ tmp = virXPathString(conn, "string(./uuid[1])", ctxt); if (!tmp) { - int err; - if ((err = virUUIDGenerate(def->uuid))) { + if (virUUIDGenerate(def->uuid)) { virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, "%s", _("Failed to generate UUID")); goto error; @@ -4456,12 +4455,11 @@ int virDomainSaveXML(virConnectPtr conn, char *configFile = NULL; int fd = -1, ret = -1; size_t towrite; - int err; if ((configFile = virDomainConfigFile(conn, configDir, def->name)) == NULL) goto cleanup; - if ((err = virFileMakePath(configDir))) { + if (virFileMakePath(configDir)) { virReportSystemError(conn, errno, _("cannot create config directory '%s'"), configDir); -- 1.6.4.2.419.gab238 >From b9c2d697d732b4c740fd9e1f87ad135c52e02c34 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering(a)redhat.com> Date: Fri, 4 Sep 2009 18:53:20 +0200 Subject: [PATCH 4/6] util.c: avoid dead store to "flag" * src/util.c (virExecDaemonize): Change flag |= VAR to "flag | VAR". --- src/util.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/util.c b/src/util.c index 2529837..af50028 100644 --- a/src/util.c +++ b/src/util.c @@ -663,7 +663,7 @@ int virExecDaemonize(virConnectPtr conn, ret = virExecWithHook(conn, argv, envp, keepfd, retpid, infd, outfd, errfd, - flags |= VIR_EXEC_DAEMON, + flags | VIR_EXEC_DAEMON, hook, data, pidfile); /* __virExec should have set an error */ -- 1.6.4.2.419.gab238 >From 6470cb9312a13fd018fb83b79e66cb50190b1f4f Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering(a)redhat.com> Date: Fri, 4 Sep 2009 19:36:17 +0200 Subject: [PATCH 5/6] iptables.c: remove dead store to "s" * src/iptables.c (iptablesAddRemoveRule): Remove dead store. --- src/iptables.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/iptables.c b/src/iptables.c index 73b39d1..4562800 100644 --- a/src/iptables.c +++ b/src/iptables.c @@ -398,7 +398,7 @@ iptablesAddRemoveRule(iptRules *rules, int action, const char *arg, ...) 1; /* arg */ va_start(args, arg); - while ((s = va_arg(args, const char *))) + while (va_arg(args, const char *)) n++; va_end(args); -- 1.6.4.2.419.gab238 >From 65c10b030fb43d6678be9bd5eeeb326a024d6902 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering(a)redhat.com> Date: Fri, 4 Sep 2009 19:37:37 +0200 Subject: [PATCH 6/6] network_driver.c: remove dead store to "err" * src/network_driver.c (networkSetAutostart): ...and its decl. --- src/network_driver.c | 4 +--- 1 files changed, 1 insertions(+), 3 deletions(-) diff --git a/src/network_driver.c b/src/network_driver.c index 84910ab..49855bf 100644 --- a/src/network_driver.c +++ b/src/network_driver.c @@ -1428,9 +1428,7 @@ static int networkSetAutostart(virNetworkPtr net, goto cleanup; if (autostart) { - int err; - - if ((err = virFileMakePath(driver->networkAutostartDir))) { + if (virFileMakePath(driver->networkAutostartDir)) { virReportSystemError(net->conn, errno, _("cannot create autostart directory '%s'"), driver->networkAutostartDir); -- 1.6.4.2.419.gab238

2 3

[libvirt] [PATCH] xm_internal.c: remove dead store and associated leak
by Jim Meyering 08 Sep '09

08 Sep '09

This one took a little thinking. I'm not 100% sure that the comment in my log is sufficient justification for removing the virGetDomain call. If we leave it, we'll have to free it and remove the XXX comment. >From 324e07f17e6a2ed6df236af955adc693670d1b16 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering(a)redhat.com> Date: Fri, 4 Sep 2009 19:46:59 +0200 Subject: [PATCH] xm_internal.c: remove dead store and associated leak * src/xm_internal.c (xenXMDomainDefineXML): Dead store to "olddomain" -- and comment ;-) highlighted that the virGetDomain call represented a leak. It was also useless, seeing as how preceding call to virHashLookup(priv->nameConfigMap found def->name. --- src/xm_internal.c | 5 ----- 1 files changed, 0 insertions(+), 5 deletions(-) diff --git a/src/xm_internal.c b/src/xm_internal.c index de3aca9..e7f6a55 100644 --- a/src/xm_internal.c +++ b/src/xm_internal.c @@ -2532,7 +2532,6 @@ cleanup: */ virDomainPtr xenXMDomainDefineXML(virConnectPtr conn, const char *xml) { virDomainPtr ret; - virDomainPtr olddomain; char filename[PATH_MAX]; const char * oldfilename; virDomainDefPtr def = NULL; @@ -2578,10 +2577,6 @@ virDomainPtr xenXMDomainDefineXML(virConnectPtr conn, const char *xml) { goto error; } - /* XXX wtf.com is this line for - it appears to be amemory leak */ - if (!(olddomain = virGetDomain(conn, def->name, entry->def->uuid))) - goto error; - /* Remove the name -> filename mapping */ if (virHashRemoveEntry(priv->nameConfigMap, def->name, NULL) < 0) { xenXMError(conn, VIR_ERR_INTERNAL_ERROR, -- 1.6.4.2.419.gab238

2 1

[libvirt] [PATCH] network_conf.c: remove dead store to "err"
by Jim Meyering 08 Sep '09

08 Sep '09

>From 66133e3b9d80e9fadefd810786b0ff2f5c8e6875 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering(a)redhat.com> Date: Fri, 4 Sep 2009 19:15:51 +0200 Subject: [PATCH] network_conf.c: remove dead store to "err" * src/network_conf.c (virNetworkDefParseXML): ...and its decl. --- src/network_conf.c | 5 ++--- 1 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/network_conf.c b/src/network_conf.c index 58a4f32..3764bb4 100644 --- a/src/network_conf.c +++ b/src/network_conf.c @@ -1,7 +1,7 @@ /* * network_conf.c: network XML handling * - * Copyright (C) 2006-2008 Red Hat, Inc. + * Copyright (C) 2006-2009 Red Hat, Inc. * Copyright (C) 2006-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -329,8 +329,7 @@ virNetworkDefParseXML(virConnectPtr conn, /* Extract network uuid */ tmp = virXPathString(conn, "string(./uuid[1])", ctxt); if (!tmp) { - int err; - if ((err = virUUIDGenerate(def->uuid))) { + if (virUUIDGenerate(def->uuid)) { virNetworkReportError(conn, VIR_ERR_INTERNAL_ERROR, "%s", _("Failed to generate UUID")); goto error; -- 1.6.4.2.409.g85dc3

2 1

[libvirt] merge-commit push prohibition now in place, for master
by Jim Meyering 08 Sep '09

08 Sep '09

Just today we noticed that it was possible to push accidental merge commits to "master". While the commit hook to prevent that was in place, the config setting to enable the hook for "master" was missing. I've just done this on the server: git --git-dir /git/libvirt.git config hooks.denymerge.master true so *now*, we should be protected from ourselves.

2 1