April 2014 - Devel - libvirt List Archives

[libvirt] [PATCHv2] storageVolCreateXMLFrom: Allow multiple accesses to origvol
by Michal Privoznik 02 May '14

02 May '14

When creating a new volume, it is possible to copy data into it from another already existing volume (referred to as @origvol). Obviously, the read-only access to @origvol is required, which is thread safe (probably not performance-wise though). However, with current code both @newvol and @origvol are marked as building for the time of copying data from the @origvol to @newvol. The rationale behind is to disallow some operations on both @origvol and @newvol, e.g. vol-wipe, vol-delete, vol-download. While it makes sense to not allow such operations on partly copied mirror, but it doesn't make sense to disallow the operations on the source (@origvol). Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com> --- Diff to v1: -introduced a counter in addition to not marking origvol as building src/conf/storage_conf.h | 1 + src/storage/storage_driver.c | 14 +++++++------- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index 9ad38e1..eae959c 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -64,6 +64,7 @@ struct _virStorageVolDef { int type; /* enum virStorageVolType */ unsigned int building; + unsigned int in_use; virStorageVolSource source; virStorageSource target; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 2cb8347..a953dfd 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1640,7 +1640,7 @@ storageVolDelete(virStorageVolPtr obj, if (virStorageVolDeleteEnsureACL(obj->conn, pool->def, vol) < 0) goto cleanup; - if (vol->building) { + if (vol->building || vol->in_use) { virReportError(VIR_ERR_OPERATION_INVALID, _("volume '%s' is still being allocated."), vol->name); @@ -1912,8 +1912,8 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj, /* Drop the pool lock during volume allocation */ pool->asyncjobs++; - origvol->building = 1; newvol->building = 1; + origvol->in_use++; virStoragePoolObjUnlock(pool); if (origpool) { @@ -1929,7 +1929,7 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj, virStoragePoolObjLock(origpool); storageDriverUnlock(driver); - origvol->building = 0; + origvol->in_use--; newvol->building = 0; allocation = newvol->target.allocation; pool->asyncjobs--; @@ -2010,7 +2010,7 @@ storageVolDownload(virStorageVolPtr obj, if (virStorageVolDownloadEnsureACL(obj->conn, pool->def, vol) < 0) goto cleanup; - if (vol->building) { + if (vol->building || vol->in_use) { virReportError(VIR_ERR_OPERATION_INVALID, _("volume '%s' is still being allocated."), vol->name); @@ -2076,7 +2076,7 @@ storageVolUpload(virStorageVolPtr obj, if (virStorageVolUploadEnsureACL(obj->conn, pool->def, vol) < 0) goto cleanup; - if (vol->building) { + if (vol->building || vol->in_use) { virReportError(VIR_ERR_OPERATION_INVALID, _("volume '%s' is still being allocated."), vol->name); @@ -2167,7 +2167,7 @@ storageVolResize(virStorageVolPtr obj, if (virStorageVolResizeEnsureACL(obj->conn, pool->def, vol) < 0) goto cleanup; - if (vol->building) { + if (vol->building || vol->in_use) { virReportError(VIR_ERR_OPERATION_INVALID, _("volume '%s' is still being allocated."), vol->name); @@ -2474,7 +2474,7 @@ storageVolWipePattern(virStorageVolPtr obj, if (virStorageVolWipePatternEnsureACL(obj->conn, pool->def, vol) < 0) goto cleanup; - if (vol->building) { + if (vol->building || vol->in_use) { virReportError(VIR_ERR_OPERATION_INVALID, _("volume '%s' is still being allocated."), vol->name); -- 1.9.0

2 1

[libvirt] [PATCH] Restore skipping of setting capacity
by John Ferlan 02 May '14

02 May '14

Commit id 'ac9a0963' refactored out the 'withCapacity' for the virStorageBackendUpdateVolInfo() API. See: http://www.redhat.com/archives/libvir-list/2014-April/msg00043.html This resulted in a difference in how 'virsh vol-info --pool <poolName> <volume>' or 'virsh vol-list vol-list --pool <poolName> --details' outputs the capacity information for a directory pool with a qcow2 sparse file. For example, using the following XML mkdir /home/TestPool cat testpool.xml <pool type='dir'> <name>TestPool</name> <uuid>6bf80895-10b6-75a6-6059-89fdea2aefb7</uuid> <source> </source> <target> <path>/home/TestPool</path> <permissions> <mode>0755</mode> <owner>0</owner> <group>0</group> </permissions> </target> </pool> virsh pool-create testpool.xml virsh vol-create-as --pool TestPool temp_vol_1 \ --capacity 1048576 --allocation 1048576 --format qcow2 virsh vol-info --pool TestPool temp_vol_1 Results in listing a Capacity value. Prior to the commit, the value would be '1.0 MiB' (1048576 bytes). However, after the commit the output would be (for example) '192.50 KiB', which for my system was the size of the volume in my file system (eg 'ls -l TestPool/temp_vol_1' results in '197120' bytes or 192.50 KiB). While perhaps technically correct, it's not necessarily what the user expected (certainly virt-test didn't expect it). This patch restores the code to not update the target capacity for this path Signed-off-by: John Ferlan <jferlan(a)redhat.com> --- src/storage/storage_backend.c | 22 +++++++++++++++------- src/storage/storage_backend.h | 5 ++++- src/storage/storage_backend_disk.c | 2 +- src/storage/storage_backend_fs.c | 11 +++++++---- src/storage/storage_backend_gluster.c | 2 +- src/storage/storage_backend_logical.c | 2 +- src/storage/storage_backend_mpath.c | 2 +- src/storage/storage_backend_scsi.c | 2 +- 8 files changed, 31 insertions(+), 17 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 946196b..afedbf5 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1403,6 +1403,7 @@ virStorageBackendVolOpen(const char *path, struct stat *sb, int virStorageBackendUpdateVolTargetInfo(virStorageSourcePtr target, + bool updateCapacity, bool withBlockVolFormat, unsigned int openflags) { @@ -1413,7 +1414,8 @@ virStorageBackendUpdateVolTargetInfo(virStorageSourcePtr target, goto cleanup; fd = ret; - if ((ret = virStorageBackendUpdateVolTargetInfoFD(target, fd, &sb)) < 0) + if ((ret = virStorageBackendUpdateVolTargetInfoFD(target, fd, &sb, + updateCapacity)) < 0) goto cleanup; if (withBlockVolFormat) { @@ -1429,18 +1431,21 @@ virStorageBackendUpdateVolTargetInfo(virStorageSourcePtr target, int virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol, + bool updateCapacity, bool withBlockVolFormat, unsigned int openflags) { int ret; if ((ret = virStorageBackendUpdateVolTargetInfo(&vol->target, + updateCapacity, withBlockVolFormat, openflags)) < 0) return ret; if (vol->backingStore.path && (ret = virStorageBackendUpdateVolTargetInfo(&vol->backingStore, + updateCapacity, withBlockVolFormat, VIR_STORAGE_VOL_OPEN_DEFAULT)) < 0) return ret; @@ -1453,15 +1458,15 @@ virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol, * @target: target definition ptr of volume to update * @fd: fd of storage volume to update, via virStorageBackendOpenVol*, or -1 * @sb: details about file (must match @fd, if that is provided) - * @allocation: If not NULL, updated allocation information will be stored - * @capacity: If not NULL, updated capacity info will be stored + * @updateCapacity: If true, updated capacity info will be stored * * Returns 0 for success, -1 on a legitimate error condition. */ int virStorageBackendUpdateVolTargetInfoFD(virStorageSourcePtr target, int fd, - struct stat *sb) + struct stat *sb, + bool updateCapacity) { #if WITH_SELINUX security_context_t filecon = NULL; @@ -1477,10 +1482,12 @@ virStorageBackendUpdateVolTargetInfoFD(virStorageSourcePtr target, /* Regular files may be sparse, so logical size (capacity) is not same * as actual allocation above */ - target->capacity = sb->st_size; + if (updateCapacity) + target->capacity = sb->st_size; } else if (S_ISDIR(sb->st_mode)) { target->allocation = 0; - target->capacity = 0; + if (updateCapacity) + target->capacity = 0; } else if (fd >= 0) { off_t end; /* XXX this is POSIX compliant, but doesn't work for CHAR files, @@ -1496,7 +1503,8 @@ virStorageBackendUpdateVolTargetInfoFD(virStorageSourcePtr target, return -1; } target->allocation = end; - target->capacity = end; + if (updateCapacity) + target->capacity = end; } if (!target->perms && VIR_ALLOC(target->perms) < 0) diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index 5997077..456b9d7 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -137,14 +137,17 @@ int virStorageBackendVolOpen(const char *path, struct stat *sb, ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); int virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol, + bool updateCapacity, bool withBlockVolFormat, unsigned int openflags); int virStorageBackendUpdateVolTargetInfo(virStorageSourcePtr target, + bool updateCapacity, bool withBlockVolFormat, unsigned int openflags); int virStorageBackendUpdateVolTargetInfoFD(virStorageSourcePtr target, int fd, - struct stat *sb); + struct stat *sb, + bool updateCapacity); char *virStorageBackendStablePath(virStoragePoolObjPtr pool, const char *devpath, diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c index 9cebcca..13336fc 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -113,7 +113,7 @@ virStorageBackendDiskMakeDataVol(virStoragePoolObjPtr pool, } /* Refresh allocation/capacity/perms */ - if (virStorageBackendUpdateVolInfo(vol, false, + if (virStorageBackendUpdateVolInfo(vol, true, false, VIR_STORAGE_VOL_OPEN_DEFAULT) < 0) return -1; diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 3694c26..5a2add8 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -84,7 +84,8 @@ virStorageBackendProbeTarget(virStorageSourcePtr target, goto error; /* Take care to propagate ret, it is not always -1 */ fd = ret; - if ((ret = virStorageBackendUpdateVolTargetInfoFD(target, fd, &sb)) < 0) { + if ((ret = virStorageBackendUpdateVolTargetInfoFD(target, fd, + &sb, true)) < 0) { goto error; } @@ -913,7 +914,7 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn ATTRIBUTE_UNUSED, vol->backingStore.format = backingStoreFormat; ignore_value(virStorageBackendUpdateVolTargetInfo( - &vol->backingStore, false, + &vol->backingStore, true, false, VIR_STORAGE_VOL_OPEN_DEFAULT)); /* If this failed, the backing file is currently unavailable, * the capacity, allocation, owner, group and mode are unknown. @@ -1190,8 +1191,10 @@ virStorageBackendFileSystemVolRefresh(virConnectPtr conn, { int ret; - /* Refresh allocation / permissions info in case its changed */ - ret = virStorageBackendUpdateVolInfo(vol, false, + /* Refresh allocation / permissions info in case its changed + * don't update the capacity value for this pass + */ + ret = virStorageBackendUpdateVolInfo(vol, false, false, VIR_STORAGE_VOL_FS_OPEN_FLAGS); if (ret < 0) return ret; diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index e0a25df..28db909 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -267,7 +267,7 @@ virStorageBackendGlusterRefreshVol(virStorageBackendGlusterStatePtr state, if (VIR_ALLOC(vol) < 0) goto cleanup; - if (virStorageBackendUpdateVolTargetInfoFD(&vol->target, -1, st) < 0) + if (virStorageBackendUpdateVolTargetInfoFD(&vol->target, -1, st, true) < 0) goto cleanup; if (virStorageBackendGlusterSetMetadata(state, vol, name) < 0) diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index ed3a012..a597e67 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -149,7 +149,7 @@ virStorageBackendLogicalMakeVol(char **const groups, if (!vol->key && VIR_STRDUP(vol->key, groups[2]) < 0) goto cleanup; - if (virStorageBackendUpdateVolInfo(vol, false, + if (virStorageBackendUpdateVolInfo(vol, true, false, VIR_STORAGE_VOL_OPEN_DEFAULT) < 0) goto cleanup; diff --git a/src/storage/storage_backend_mpath.c b/src/storage/storage_backend_mpath.c index f0ed189..8c3b0df 100644 --- a/src/storage/storage_backend_mpath.c +++ b/src/storage/storage_backend_mpath.c @@ -60,7 +60,7 @@ virStorageBackendMpathNewVol(virStoragePoolObjPtr pool, if (virAsprintf(&vol->target.path, "/dev/%s", dev) < 0) goto cleanup; - if (virStorageBackendUpdateVolInfo(vol, true, + if (virStorageBackendUpdateVolInfo(vol, true, true, VIR_STORAGE_VOL_OPEN_DEFAULT) < 0) { goto cleanup; } diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index c448d7f..71bcf56 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -199,7 +199,7 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool, goto free_vol; } - if (virStorageBackendUpdateVolInfo(vol, true, + if (virStorageBackendUpdateVolInfo(vol, true, true, VIR_STORAGE_VOL_OPEN_DEFAULT) < 0) { retval = -1; goto free_vol; -- 1.9.0

3 5

[libvirt] Libvirt API
by Vikas Kokare 02 May '14

02 May '14

We are seeing issues(data is unavailable) with usage of the following Libvirt APIs, on RHEL5, while they are working Ok with RHEL 6, SLES10/11. CPU dumpxml from Domain.getXMLDesc() returns no xml nodes for CPU model and topology and its sub-elements such as sockets, cores, threads. Domain.memoryStats(int) : added since 0.5.0 Please guide.

2 2

[libvirt] Test failures with newest gnutls
by Martin Kletzander 01 May '14

01 May '14

Hi everyone, after upgrade to gnutls-3.3.0, I discovered (commandtest fails) that any code linked with -lgnutls will have not 3, but 5 open file descriptors upon the entry into main(). I asked on gnutls-help [1] if they know they are leaking file descriptors. The response was, that this is intended with the explanation being that these FDs (pointing to /dev/urandom) are kept open for backward compatibility with programs that may chroot into environment without /dev/urandom as the previous version didn't require to have access to /dev/urandom when calling gnutls code. Does that seem like our bug that we're relying on fixed number of open file descriptors? Or that we're linking to gnutls when we don't need it in commandhelper? Or should this be fixed somewhere else? Have a nice day, Martin [1] http://lists.gnutls.org/pipermail/gnutls-help/2014-April/003415.html

2 3

[libvirt] [PATCH v6 0/6] Expose FSFreeze/FSThaw within the guest as API
by Tomoki Sekiyama 01 May '14

01 May '14

Hello, This is patchset v6 to add FSFreeze/FSThaw API for custom disk snapshotting. Changes since v5: * replace disks and ndisks parameter with mountpoints and nmountpoints to specify filesystem to be frozen/thawed, so that we can specify non-disk-backed filesystems * add PATCH 6 to support 'mountpoints' argument for 'guest-fsfreeze-freeze' command of qemu-guest-agent. (This requires a patch for qemu-guest-agent, which isn't merged yet: https://lists.gnu.org/archive/html/qemu-devel/2014-04/msg04434.html ) * drop nested fsfreeze request feature * change API version to 1.2.5 (v5: https://www.redhat.com/archives/libvir-list/2014-April/msg00140.html ) === Description === Currently FSFreeze and FSThaw are supported by qemu guest agent and they are used internally in snapshot-create command with --quiesce option. However, when users want to utilize the native snapshot feature of storage devices (such as LVM over iSCSI, enterprise storage appliances, etc.), they need to issue fsfreeze command separately from libvirt-driven snapshots. (OpenStack cinder provides these storages' snapshot feature, but it cannot quiesce the guest filesystems automatically for now.) Although virDomainQemuGuestAgent() API could be used for this purpose, it is only for debugging and is not supported officially. This patchset adds virDomainFSFreeze()/virDomainFSThaw() APIs and virsh domfsfreeze/domfsthaw commands to enable the users to freeze and thaw domain's filesystems cleanly. <updated> The APIs take disks and ndisks parameters, which is a list of mountpoints of filesystems to be frozen/thawed. If the option is not provided, or guest agent doesn't support the 'mountpoints' argument, every mounted filesystem is frozen/thawed. </updated> The APIs have flags option currently unsupported for future extension. --- Tomoki Sekiyama (6): Introduce virDomainFSFreeze() and virDomainFSThaw() public API remote: Implement virDomainFSFreeze and virDomainFSThaw qemu: track quiesced status in qemuDomainSnapshotFSFreeze qemu: Implement virDomainFSFreeze and virDomainFSThaw virsh: Expose new virDomainFSFreeze and virDomainFSThaw API qemu: Support mountpoints option of guest-fsfreeze-freeze include/libvirt/libvirt.h.in | 10 +++ src/access/viraccessperm.c | 2 - src/access/viraccessperm.h | 6 ++ src/driver.h | 14 ++++ src/libvirt.c | 93 ++++++++++++++++++++++++ src/libvirt_public.syms | 6 ++ src/qemu/qemu_agent.c | 47 +++++++++++- src/qemu/qemu_agent.h | 3 + src/qemu/qemu_domain.c | 5 + src/qemu/qemu_domain.h | 2 + src/qemu/qemu_driver.c | 160 ++++++++++++++++++++++++++++++++++++++---- src/remote/remote_driver.c | 2 + src/remote/remote_protocol.x | 38 +++++++++- src/remote_protocol-structs | 18 +++++ src/rpc/gendispatch.pl | 2 + tests/qemuagenttest.c | 8 +- tools/virsh-domain.c | 130 ++++++++++++++++++++++++++++++++++ tools/virsh.pod | 23 ++++++ 18 files changed, 544 insertions(+), 25 deletions(-)

3 17

[libvirt] [PATCH v7 0/4] Handling of undefine and re define snapshots with VirtualBox 4.2 or higher
by Yohan BELLEGUIC 01 May '14

01 May '14

Hello, This is a new series of patches in order to support undefining and redefining snapshots with VirtualBox 4.2 or higher. These patches are based on Manuel Vives' patches, taking into account Daniel P. Berrange's remarks. The VirtualBox API provides only high level operations to manipulate snapshots, so it not possible to support flags like VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE and VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY with only API calls. Following an IRC talk with Eric Blake, the decision was made to emulate these behaviours by manipulating directly the .vbox XML files. The first patch adds extra details in the snapshot XML returned by libvirt. We will need those details in order to redefine the snapshots. The second patch adds a new API to manipulate the VirtualBox XML file. It provides several structs describing the VirtualBox XML file nodes and functions which can manipulate these structs. The third patch adds support of the VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE and VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT flags in virDomainSnapshotCreateXML. The idea is to unregister the machine, add the snapshot in the Virtualbox XML file and re-register the machine. However, VirtualBox forbids a machine to have snapshots but no current snapshot. So, if the real current snapshot has not been redefined yet, we create fake disks, allowing us to have an intermediate state in order to not corrupt the snapshot's read-write disks. These fake disks will be deleted during the next redefine. The fourth and last patch adds support of the VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY flag in virDomainSnapshotDelete. As in the third patch, we also create fake disks to not corrupt the snapshot's read-write disks. The patches were only tested with VirtualBox 4.3.10 and VirtualBox 4.2.24. Regards Yohan BELLEGUIC v7: * Add vbox_snapshot_conf.{h,c} files to (de)serialize VirtualBox XML files * Update the code to use the API exposed by vbox_snapshot_conf.h * Handle the fact that VirtualBox forbids a machine to have snapshots but no current snapshot v6: * Rebased because of a massive change in vbox_tmpl.c due to changes in the handling of different versions of VirtualBox v5: * The patches are modified according to a first review by Laine Stump: * renamed virSearchUuid to virSearchRegex and moved it from viruuid.{c,h} to virstring.{c,h}. * Various fixes. v4: * The code is compliant with Virtualbox 4.3 API * Some minor modifications in order to satisfy "make syntax-check" v3: * Use of STREQ_NULLABLE instead of STREQ in one case * Fix the method for finding uuids according to Ján Tomko review v2: * Fix a licence problem with the method for string replacement Manuel VIVES (1): vbox_tmpl.c: Better XML description for snapshots Yohan BELLEGUIC (3): Add vbox_snapshot_conf struct vbox_tmpl.c: Patch for redefining snapshots vbox_tmpl.c: Add function for undefining snapshot po/POTFILES.in | 1 + src/Makefile.am | 1 + src/vbox/vbox_snapshot_conf.c | 1383 +++++++++++++++++++++++++ src/vbox/vbox_snapshot_conf.h | 100 ++ src/vbox/vbox_tmpl.c | 2225 +++++++++++++++++++++++++++++++++++++---- 5 files changed, 3538 insertions(+), 172 deletions(-) create mode 100644 src/vbox/vbox_snapshot_conf.c create mode 100644 src/vbox/vbox_snapshot_conf.h -- 1.7.10.4

2 9

[libvirt] [PATCH] conf: handle 'vda[-1]' uniformly
by Eric Blake 01 May '14

01 May '14

Commit f22b7899 stumbled across a difference between 32-bit and 64-bit platforms; on 64-bit, parsing "-1" as a long produces 0xffffffffffffffff, which does not fit in unsigned int; but on 32-bit, it parses as 0xffffffff, which DOES fit. Another patch will tweak virStrToLong_ui to behave the same across platforms, but regardless of which of the two choices it makes, the chain lookup code wants to reject negative numbers rather than treating it as large integers. * src/util/virstoragefile.c (virStorageFileParseChainIndex): Reject negative sign. Signed-off-by: Eric Blake <eblake(a)redhat.com> --- Therefore, although this qualifies as a build-breaker fix on 32-bit, I haven't pushed it yet: I'm still working on my patch to virstring.c for uniform behavior; and that turned up the fact that virstringtest.c doesn't have coverage of integer parsing, so of course, I have to expand that test too... src/util/virstoragefile.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index dcce1ef..5d933a4 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1525,7 +1525,9 @@ virStorageFileParseChainIndex(const char *diskTarget, if (virStringListLength(strings) != 2) goto cleanup; - if (virStrToLong_ui(strings[1], &suffix, 10, &idx) < 0 || + /* Rule out spaces or negative sign */ + if (!c_isdigit(*strings[1]) || + virStrToLong_ui(strings[1], &suffix, 10, &idx) < 0 || STRNEQ(suffix, "]")) goto cleanup; -- 1.9.0

2 2

[libvirt] [PATCH for 1.2.4] storage: Clear all data allocated about backing store before reparsing
by Peter Krempa 30 Apr '14

30 Apr '14

To avoid memory leak of the "backingStoreRaw" field when reparsing backing chains a new function is being introduced by this patch that shall be used to clear backing store information. The memory leak was introduced in commit 8823272d41a259c1246c05d. --- src/libvirt_private.syms | 1 + src/qemu/qemu_domain.c | 8 +++----- src/qemu/qemu_driver.c | 3 +-- src/util/virstoragefile.c | 30 ++++++++++++++++++++++++------ src/util/virstoragefile.h | 1 + 5 files changed, 30 insertions(+), 13 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 55b016d..4cfaefc 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1865,6 +1865,7 @@ virStorageNetHostTransportTypeToString; virStorageNetProtocolTypeToString; virStorageSourceAuthClear; virStorageSourceClear; +virStorageSourceClearBackingStore; virStorageSourceFree; virStorageSourceGetActualType; virStorageSourcePoolDefFree; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 8fa58f3..a3c1b1c 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2421,12 +2421,10 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr driver, goto cleanup; if (disk->src.backingStore) { - if (force) { - virStorageSourceFree(disk->src.backingStore); - disk->src.backingStore = NULL; - } else { + if (force) + virStorageSourceClearBackingStore(&disk->src); + else goto cleanup; - } } qemuDomainGetImageIds(cfg, vm, disk, &uid, &gid); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 75cf8cb..bf19c6e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12734,8 +12734,7 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, * recompute it. Better would be storing the chain ourselves rather than * reprobing, but this requires modifying domain_conf and our XML to fully * track the chain across libvirtd restarts. */ - virStorageSourceFree(disk->src.backingStore); - disk->src.backingStore = NULL; + virStorageSourceClearBackingStore(&disk->src); if (virStorageFileInit(&snap->src) < 0) goto cleanup; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index dcce1ef..5c43665 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1726,6 +1726,29 @@ virStorageSourceGetActualType(virStorageSourcePtr def) } +/** + * virStorageSourceClearBackingStore: + * + * @src: disk source to clear + * + * Clears information about backing store of the current storage file. + */ +void +virStorageSourceClearBackingStore(virStorageSourcePtr def) +{ + if (!def) + return; + + VIR_FREE(def->relPath); + VIR_FREE(def->relDir); + VIR_FREE(def->backingStoreRaw); + + /* recursively free backing chain */ + virStorageSourceFree(def->backingStore); + def->backingStore = NULL; +} + + void virStorageSourceClear(virStorageSourcePtr def) { @@ -1755,12 +1778,7 @@ virStorageSourceClear(virStorageSourcePtr def) virStorageNetHostDefFree(def->nhosts, def->hosts); virStorageSourceAuthClear(def); - VIR_FREE(def->relPath); - VIR_FREE(def->relDir); - VIR_FREE(def->backingStoreRaw); - - /* recursively free backing chain */ - virStorageSourceFree(def->backingStore); + virStorageSourceClearBackingStore(def); } diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 148776e..9e6cdba 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -316,5 +316,6 @@ void virStorageSourcePoolDefFree(virStorageSourcePoolDefPtr def); void virStorageSourceClear(virStorageSourcePtr def); int virStorageSourceGetActualType(virStorageSourcePtr def); void virStorageSourceFree(virStorageSourcePtr def); +void virStorageSourceClearBackingStore(virStorageSourcePtr def); #endif /* __VIR_STORAGE_FILE_H__ */ -- 1.9.1

2 2

[libvirt] [PATCH v2] nwfilter: Tear down temp. filters when tearing all filters
by Stefan Berger 30 Apr '14

30 Apr '14

From: Stefan Berger <stefanb(a)linux.vnet.ibm.com> Refactor the ebiptablesTearNewRules function so that the teardown of temporary filters can also be called by the ebiptablesAllTeardown function. This fixes a problem that leaves temporary filters behind when a VM shuts down while its filters are modified. Signed-off-by: Stefan Berger <stefanb(a)linux.vnet.ibm.com> v1->v2: - test cases adjusted to expect more commands --- src/nwfilter/nwfilter_ebiptables_driver.c | 24 ++++++++---- tests/nwfilterebiptablestest.c | 64 +++++++++++++++++-------------- 2 files changed, 52 insertions(+), 36 deletions(-) diff --git a/src/nwfilter/nwfilter_ebiptables_driver.c b/src/nwfilter/nwfilter_ebiptables_driver.c index 4cbdc0e..5cb0b74 100644 --- a/src/nwfilter/nwfilter_ebiptables_driver.c +++ b/src/nwfilter/nwfilter_ebiptables_driver.c @@ -3536,14 +3536,9 @@ ebiptablesApplyNewRules(const char *ifname, } -static int -ebiptablesTearNewRules(const char *ifname) +static void +ebiptablesTearNewRulesFW(virFirewallPtr fw, const char *ifname) { - virFirewallPtr fw = virFirewallNew(); - int ret = -1; - - virFirewallStartTransaction(fw, VIR_FIREWALL_TRANSACTION_IGNORE_ERRORS); - iptablesUnlinkTmpRootChainsFW(fw, VIR_FIREWALL_LAYER_IPV4, ifname); iptablesRemoveTmpRootChainsFW(fw, VIR_FIREWALL_LAYER_IPV4, ifname); @@ -3555,13 +3550,24 @@ ebiptablesTearNewRules(const char *ifname) ebtablesRemoveTmpSubChainsFW(fw, ifname); ebtablesRemoveTmpRootChainFW(fw, true, ifname); ebtablesRemoveTmpRootChainFW(fw, false, ifname); +} + + +static int +ebiptablesTearNewRules(const char *ifname) +{ + virFirewallPtr fw = virFirewallNew(); + int ret = -1; + + virFirewallStartTransaction(fw, VIR_FIREWALL_TRANSACTION_IGNORE_ERRORS); + + ebiptablesTearNewRulesFW(fw, ifname); ret = virFirewallApply(fw); virFirewallFree(fw); return ret; } - static int ebiptablesTearOldRules(const char *ifname) { @@ -3608,6 +3614,8 @@ ebiptablesAllTeardown(const char *ifname) virFirewallStartTransaction(fw, VIR_FIREWALL_TRANSACTION_IGNORE_ERRORS); + ebiptablesTearNewRulesFW(fw, ifname); + iptablesUnlinkRootChainsFW(fw, VIR_FIREWALL_LAYER_IPV4, ifname); iptablesClearVirtInPostFW(fw, VIR_FIREWALL_LAYER_IPV4, ifname); iptablesRemoveRootChainsFW(fw, VIR_FIREWALL_LAYER_IPV4, ifname); diff --git a/tests/nwfilterebiptablestest.c b/tests/nwfilterebiptablestest.c index df939d5..ff9a9b7 100644 --- a/tests/nwfilterebiptablestest.c +++ b/tests/nwfilterebiptablestest.c @@ -33,11 +33,43 @@ #define VIR_FROM_THIS VIR_FROM_NONE + +#define VIR_NWFILTER_NEW_RULES_TEARDOWN \ + "iptables -D libvirt-out -m physdev --physdev-is-bridged --physdev-out vnet0 -g FP-vnet0\n" \ + "iptables -D libvirt-out -m physdev --physdev-out vnet0 -g FP-vnet0\n" \ + "iptables -D libvirt-in -m physdev --physdev-in vnet0 -g FJ-vnet0\n" \ + "iptables -D libvirt-host-in -m physdev --physdev-in vnet0 -g HJ-vnet0\n" \ + "iptables -F FP-vnet0\n" \ + "iptables -X FP-vnet0\n" \ + "iptables -F FJ-vnet0\n" \ + "iptables -X FJ-vnet0\n" \ + "iptables -F HJ-vnet0\n" \ + "iptables -X HJ-vnet0\n" \ + "ip6tables -D libvirt-out -m physdev --physdev-is-bridged --physdev-out vnet0 -g FP-vnet0\n" \ + "ip6tables -D libvirt-out -m physdev --physdev-out vnet0 -g FP-vnet0\n" \ + "ip6tables -D libvirt-in -m physdev --physdev-in vnet0 -g FJ-vnet0\n" \ + "ip6tables -D libvirt-host-in -m physdev --physdev-in vnet0 -g HJ-vnet0\n" \ + "ip6tables -F FP-vnet0\n" \ + "ip6tables -X FP-vnet0\n" \ + "ip6tables -F FJ-vnet0\n" \ + "ip6tables -X FJ-vnet0\n" \ + "ip6tables -F HJ-vnet0\n" \ + "ip6tables -X HJ-vnet0\n" \ + "ebtables -t nat -D PREROUTING -i vnet0 -j libvirt-J-vnet0\n" \ + "ebtables -t nat -D POSTROUTING -o vnet0 -j libvirt-P-vnet0\n" \ + "ebtables -t nat -L libvirt-J-vnet0\n" \ + "ebtables -t nat -L libvirt-P-vnet0\n" \ + "ebtables -t nat -F libvirt-J-vnet0\n" \ + "ebtables -t nat -X libvirt-J-vnet0\n" \ + "ebtables -t nat -F libvirt-P-vnet0\n" \ + "ebtables -t nat -X libvirt-P-vnet0\n" + static int testNWFilterEBIPTablesAllTeardown(const void *opaque ATTRIBUTE_UNUSED) { virBuffer buf = VIR_BUFFER_INITIALIZER; const char *expected = + VIR_NWFILTER_NEW_RULES_TEARDOWN "iptables -D libvirt-out -m physdev --physdev-is-bridged --physdev-out vnet0 -g FO-vnet0\n" "iptables -D libvirt-out -m physdev --physdev-out vnet0 -g FO-vnet0\n" "iptables -D libvirt-in -m physdev --physdev-in vnet0 -g FI-vnet0\n" @@ -221,34 +253,7 @@ testNWFilterEBIPTablesTearNewRules(const void *opaque ATTRIBUTE_UNUSED) { virBuffer buf = VIR_BUFFER_INITIALIZER; const char *expected = - "iptables -D libvirt-out -m physdev --physdev-is-bridged --physdev-out vnet0 -g FP-vnet0\n" - "iptables -D libvirt-out -m physdev --physdev-out vnet0 -g FP-vnet0\n" - "iptables -D libvirt-in -m physdev --physdev-in vnet0 -g FJ-vnet0\n" - "iptables -D libvirt-host-in -m physdev --physdev-in vnet0 -g HJ-vnet0\n" - "iptables -F FP-vnet0\n" - "iptables -X FP-vnet0\n" - "iptables -F FJ-vnet0\n" - "iptables -X FJ-vnet0\n" - "iptables -F HJ-vnet0\n" - "iptables -X HJ-vnet0\n" - "ip6tables -D libvirt-out -m physdev --physdev-is-bridged --physdev-out vnet0 -g FP-vnet0\n" - "ip6tables -D libvirt-out -m physdev --physdev-out vnet0 -g FP-vnet0\n" - "ip6tables -D libvirt-in -m physdev --physdev-in vnet0 -g FJ-vnet0\n" - "ip6tables -D libvirt-host-in -m physdev --physdev-in vnet0 -g HJ-vnet0\n" - "ip6tables -F FP-vnet0\n" - "ip6tables -X FP-vnet0\n" - "ip6tables -F FJ-vnet0\n" - "ip6tables -X FJ-vnet0\n" - "ip6tables -F HJ-vnet0\n" - "ip6tables -X HJ-vnet0\n" - "ebtables -t nat -D PREROUTING -i vnet0 -j libvirt-J-vnet0\n" - "ebtables -t nat -D POSTROUTING -o vnet0 -j libvirt-P-vnet0\n" - "ebtables -t nat -L libvirt-J-vnet0\n" - "ebtables -t nat -L libvirt-P-vnet0\n" - "ebtables -t nat -F libvirt-J-vnet0\n" - "ebtables -t nat -X libvirt-J-vnet0\n" - "ebtables -t nat -F libvirt-P-vnet0\n" - "ebtables -t nat -X libvirt-P-vnet0\n"; + VIR_NWFILTER_NEW_RULES_TEARDOWN; char *actual = NULL; int ret = -1; @@ -282,6 +287,7 @@ testNWFilterEBIPTablesApplyBasicRules(const void *opaque ATTRIBUTE_UNUSED) { virBuffer buf = VIR_BUFFER_INITIALIZER; const char *expected = + VIR_NWFILTER_NEW_RULES_TEARDOWN "iptables -D libvirt-out -m physdev --physdev-is-bridged --physdev-out vnet0 -g FO-vnet0\n" "iptables -D libvirt-out -m physdev --physdev-out vnet0 -g FO-vnet0\n" "iptables -D libvirt-in -m physdev --physdev-in vnet0 -g FI-vnet0\n" @@ -353,6 +359,7 @@ testNWFilterEBIPTablesApplyDHCPOnlyRules(const void *opaque ATTRIBUTE_UNUSED) { virBuffer buf = VIR_BUFFER_INITIALIZER; const char *expected = + VIR_NWFILTER_NEW_RULES_TEARDOWN "iptables -D libvirt-out -m physdev --physdev-is-bridged --physdev-out vnet0 -g FO-vnet0\n" "iptables -D libvirt-out -m physdev --physdev-out vnet0 -g FO-vnet0\n" "iptables -D libvirt-in -m physdev --physdev-in vnet0 -g FI-vnet0\n" @@ -443,6 +450,7 @@ testNWFilterEBIPTablesApplyDropAllRules(const void *opaque ATTRIBUTE_UNUSED) { virBuffer buf = VIR_BUFFER_INITIALIZER; const char *expected = + VIR_NWFILTER_NEW_RULES_TEARDOWN "iptables -D libvirt-out -m physdev --physdev-is-bridged --physdev-out vnet0 -g FO-vnet0\n" "iptables -D libvirt-out -m physdev --physdev-out vnet0 -g FO-vnet0\n" "iptables -D libvirt-in -m physdev --physdev-in vnet0 -g FI-vnet0\n" -- 1.8.1.4

2 2

[libvirt] [PATCHv2] Device{Attach, Detach}: Document S4 limitations
by Michal Privoznik 30 Apr '14

30 Apr '14

https://bugzilla.redhat.com/show_bug.cgi?id=808463 Well, libvirt doesn't distinguish between domain poweroff and hibernation (S4). It's hard to differentiate these two on a real machine anyway. As a result, any device that is hot(un-)plugged is lost (appears again) when domain is started again as from our POV it is a fresh cold boot. Instead of doing anything wise here, we should just document this as known limitation. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com> --- src/libvirt.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/libvirt.c b/src/libvirt.c index b6c99c5..79071db 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -10319,6 +10319,10 @@ virNodeGetSecurityModel(virConnectPtr conn, virSecurityModelPtr secmodel) * in an existing CDROM/Floppy device, however, applications are * recommended to use the virDomainUpdateDeviceFlag method instead. * + * Be aware that hotplug changes might not persist across a domain going + * into S4 state (also known as hibernation) unless you also modify the + * persistent domain definition. + * * Returns 0 in case of success, -1 in case of failure. */ int @@ -10374,6 +10378,10 @@ virDomainAttachDevice(virDomainPtr domain, const char *xml) * in an existing CDROM/Floppy device, however, applications are * recommended to use the virDomainUpdateDeviceFlag method instead. * + * Be aware that hotplug changes might not persist across a domain going + * into S4 state (also known as hibernation) unless you also modify the + * persistent domain definition. + * * Returns 0 in case of success, -1 in case of failure. */ int @@ -10416,6 +10424,10 @@ virDomainAttachDeviceFlags(virDomainPtr domain, * Destroy a virtual device attachment to backend. This function, * having hot-unplug semantics, is only allowed on an active domain. * + * Be aware that hotplug changes might not persist across a domain going + * into S4 state (also known as hibernation) unless you also modify the + * persistent domain definition. + * * Returns 0 in case of success, -1 in case of failure. */ int @@ -10487,6 +10499,10 @@ virDomainDetachDevice(virDomainPtr domain, const char *xml) * a synchronous removal. In other words, this API may wait a bit for the * removal to complete in case it was not synchronous. * + * Be aware that hotplug changes might not persist across a domain going + * into S4 state (also known as hibernation) unless you also modify the + * persistent domain definition. + * * Returns 0 in case of success, -1 in case of failure. */ int -- 1.9.0

2 2