- Devel - libvirt List Archives

[libvirt] [PATCH] qemuSetupMemoryCgroup: Handle hard_limit properly
by Michal Privoznik 20 Aug '13

20 Aug '13

Since 16bcb3 we have a regression. The hard_limit is set unconditionally. By default, the limit is zero. Hence, if user hasn't configured any, we set the zero in cgroup subsystem making the kernel kill the corresponding qemu process immediately. The proper fix is to set hard_limit iff user has configure any. --- src/qemu/qemu_cgroup.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 9673e8e..e27945e 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -428,7 +428,8 @@ qemuSetupMemoryCgroup(virDomainObjPtr vm) } } - if (virCgroupSetMemoryHardLimit(priv->cgroup, vm->def->mem.hard_limit) < 0) + if (vm->def->mem.hard_limit != 0 && + virCgroupSetMemoryHardLimit(priv->cgroup, vm->def->mem.hard_limit) < 0) return -1; if (vm->def->mem.soft_limit != 0 && -- 1.8.1.5

4 5

[libvirt] [PATCH 0/3] Clean up usage of vshStringToArray
by Peter Krempa 20 Aug '13

20 Aug '13

Usage of vshStringToArray in virsh was problematic in a few places. Clean it up to avoid a few memleaks. Also improve an error message. Peter Krempa (3): virsh: modify vshStringToArray to duplicate the elements too virsh-pool: Improve error message in cmdPoolList virsh: Don't leak list of volumes when undefining domain with storage tools/virsh-domain.c | 121 ++++++++++++++++++++++++------------------------- tools/virsh-nodedev.c | 18 ++------ tools/virsh-pool.c | 12 ++--- tools/virsh-snapshot.c | 10 +--- tools/virsh.c | 8 ++-- tools/virsh.h | 1 + 6 files changed, 76 insertions(+), 94 deletions(-) -- 1.8.3.2

2 10

[libvirt] [RFC] QCOW2 version defaults in qemu-img and libvirt
by Ján Tomko 20 Aug '13

20 Aug '13

Hello! QEMU is switching the default QCOW2 version from v2 (compat=0.10) to v3 (compat=1.1) [1] Currently, libvirt only specifies the compat=0.10 option if it was explicitly requested (to avoid parsing qemu-img help output [2]) and assumes the format to be v2 when it calls qemu-img without the compat option. With this change in qemu-img a volume with no <features> or <compat> elements will be created as qcow2v3 with the new qemu-img (but the compat level won't be reflected in volume XML until refresh). According to the IRC conversation with Eric Blake and Kevin Wolf (bug I filed: [3]), it seems we should: * always specify the compat option if it's supported by qemu-img (which would solve the problem mentioned above) * provide an option in qemu.conf to set the default compatibility level, defaulting to 1.1 to make it easier to use the new format This would probably require a new storage.conf file, since the storage driver doesn't have access to the qemu driver config, but: does this seem reasonable? Should we add a default feature list (for the only feature) as well? Jan [1] http://lists.nongnu.org/archive/html/qemu-devel/2013-08/msg02549.html [2] https://www.redhat.com/archives/libvir-list/2013-February/msg00301.html [3] https://bugzilla.redhat.com/show_bug.cgi?id=997977

2 1

Re: [libvirt] [PATCH v2]LXC: Helper function for checking ownership of dir when userns enabled
by Chen HanXiao 20 Aug '13

20 Aug '13

Hi Any comments? Thanks > -----Original Message----- > From: libvir-list-bounces(a)redhat.com [mailto:libvir-list-bounces@redhat.com] > On Behalf Of Chen HanXiao > Sent: Wednesday, August 14, 2013 9:30 AM > To: 'Daniel P. Berrange' > Cc: libvir-list(a)redhat.com > Subject: Re: [libvirt] [PATCH v2]LXC: Helper function for checking ownership of > dir when userns enabled > > > > > -----Original Message----- > > From: Daniel P. Berrange [mailto:berrange@redhat.com] > > Sent: Saturday, August 10, 2013 12:54 AM > > To: Chen Hanxiao > > Cc: libvir-list(a)redhat.com > > Subject: Re: [libvirt] [PATCH v2]LXC: Helper function for checking ownership of > > dir when userns enabled > > > > On Fri, Aug 09, 2013 at 04:05:58PM +0800, Chen Hanxiao wrote: > > > From: Chen Hanxiao <chenhanxiao(a)cn.fujitsu.com> > > > > > > If we enable userns, the ownership of dir we provided for containers > > > should match the uid/gid in idmap. > > > Currently, the debug log is very implicit or misleading sometimes. > > > This patch will help clarify this for us when using > > > debug log or virsh. > > > > I do recall hitting some permission issue once, but can't remember > > just what it was. Can you describe exactly how to reproduce the > > problem ? > > > > 1) Enable user namespace in kernel > 2) Add idmap for container > 3) Don't change the ownership of devices/ filesystem/ source dir ( leave > them to 'root' for instance) > 4) Start the container > > Usually I got an input/output error by virsh, which is not a good hint. > > > > > Signed-off-by: Chen Hanxiao <chenhanxiao(a)cn.fujitsu.com> > > > --- > > > src/lxc/lxc_container.c | 46 > > ++++++++++++++++++++++++++++++++++++++++++++++ > > > 1 files changed, 46 insertions(+), 0 deletions(-) > > > > > > diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c > > > index b910b10..2ccdc61 100644 > > > --- a/src/lxc/lxc_container.c > > > +++ b/src/lxc/lxc_container.c > > > @@ -1815,6 +1815,49 @@ lxcNeedNetworkNamespace(virDomainDefPtr > > def) > > > return false; > > > } > > > > > > +/* > > > + * Helper function for helping check > > > + * whether we have enough privilege > > > + * to operate the source dir when userns enabled > > > + * @vmDef: pointer to vm definition structure > > > + * Returns 0 on success or -1 in case of error > > > + */ > > > +static int > > > +lxcContainerUsernsSrcOwnershipCheck(virDomainDefPtr vmDef) > > > +{ > > > + struct stat buf; > > > + size_t i; > > > + uid_t uid; > > > + gid_t gid; > > > + > > > + VIR_DEBUG("vmDef->nfss %d", (int)vmDef->nfss); > > > + for (i = 0; i < vmDef->nfss; i++) { > > > + VIR_DEBUG("dst is %s, src is %s", > > > + vmDef->fss[i]->dst, > > > + vmDef->fss[i]->src); > > > + > > > + uid = vmDef->idmap.uidmap[0].target; > > > + gid = vmDef->idmap.gidmap[0].target; > > > + > > > + if (lstat(vmDef->fss[i]->src, &buf) < 0) { > > > + virReportSystemError(errno, _("Cannot access '%s'"), > > > + vmDef->fss[i]->src); > > > + return -1; > > > + } else if (uid != buf.st_uid || gid != buf.st_gid) { > > > + VIR_DEBUG("In userns uid is %d, gid is %d\n", > > > + uid, gid); > > > + errno = EINVAL; > > > + > > > + virReportSystemError(errno, > > > + _("[userns] Src dir '%s' does not > > belong to uid/gid: %d/%d"), > > > + vmDef->fss[i]->src, uid, gid); > > > + return -1; > > > + } > > > + } > > > + > > > + return 0; > > > +} > > > + > > > /** > > > * lxcContainerStart: > > > * @def: pointer to virtual machine structure > > > @@ -1866,6 +1909,9 @@ int lxcContainerStart(virDomainDefPtr def, > > > if (userns_supported()) { > > > VIR_DEBUG("Enable user namespace"); > > > cflags |= CLONE_NEWUSER; > > > + if (lxcContainerUsernsSrcOwnershipCheck(def) < 0) { > > > + return -1; > > > + } > > > } else { > > > virReportSystemError(VIR_ERR_CONFIG_UNSUPPORTED, > > "%s", > > > _("Kernel doesn't support user > > namespace")); > > > > > > Daniel > > -- > > |: http://berrange.com -o- > > http://www.flickr.com/photos/dberrange/ :| > > |: http://libvirt.org -o- > > http://virt-manager.org :| > > |: http://autobuild.org -o- > > http://search.cpan.org/~danberr/ :| > > |: http://entangle-photo.org -o- > > http://live.gnome.org/gtk-vnc :| > > > > -- > libvir-list mailing list > libvir-list(a)redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list

1 0

[libvirt] [PATCH 0/3] virt-*-validate cleanups
by Eric Blake 20 Aug '13

20 Aug '13

Since I complained about virt-login-shell not having --help or --version, I figured I'd better audit our other shipping executable files. In the process, I found that we have been unable to auto-validate <domainsnapshot> objects, among others. Eric Blake (3): virt-xml-validate: add --help/--version option virt-xml-validate: add missing schemas virt-pki-validate: add --help/--version option tools/Makefile.am | 10 ++++---- tools/virt-pki-validate.in | 45 +++++++++++++++++++++++++++++++++++- tools/virt-xml-validate.in | 57 ++++++++++++++++++++++++++++++++++++++++++---- 3 files changed, 102 insertions(+), 10 deletions(-) -- 1.8.3.1

2 10

[libvirt] [PATCH] docs: Clean 09adfdc62de2b up
by Michal Privoznik 19 Aug '13

19 Aug '13

--- docs/formatdomain.html.in | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 4a927cc..12a756c 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -677,8 +677,9 @@ <dd> The optional <code>hard_limit</code> element is the maximum memory the guest can use. The units for this value are kibibytes (i.e. blocks of 1024 bytes). <strong>However, users of QEMU and KVM are strongly - advised not to set this limit as domain may get killed by the kernel. - To determine the memory needed for a process to run is + advised not to set this limit as domain may get killed by the kernel + if the guess is too low. To determine the memory needed for a process + to run is an <a href="http://en.wikipedia.org/wiki/Undecidable_problem"> undecidable problem</a>.</strong></dd> <dt><code>soft_limit</code></dt> -- 1.8.1.5

2 1

[libvirt] [PATCH] storage: Update pool metadata after adding/removing/resizing volume
by Osier Yang 19 Aug '13

19 Aug '13

RHEL6.5: https://bugzilla.redhat.com/show_bug.cgi?id=965442 One has to refresh the pool to get the correct pool info after adding/removing/resizing a volume, this updates the pool metadata (allocation, available) after those operation are done. v1: https://www.redhat.com/archives/libvir-list/2013-May/msg01957.html --- src/storage/storage_driver.c | 43 ++++++++++++++++++++++++++++++------------- 1 file changed, 30 insertions(+), 13 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 72786dd..7908ba6 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1507,6 +1507,7 @@ storageVolCreateXML(virStoragePoolPtr obj, virStorageBackendPtr backend; virStorageVolDefPtr voldef = NULL; virStorageVolPtr ret = NULL, volobj = NULL; + virStorageVolDefPtr buildvoldef = NULL; virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA, NULL); @@ -1565,20 +1566,19 @@ storageVolCreateXML(virStoragePoolPtr obj, goto cleanup; } - if (backend->buildVol) { - int buildret; - virStorageVolDefPtr buildvoldef = NULL; + if (VIR_ALLOC(buildvoldef) < 0) { + voldef = NULL; + goto cleanup; + } - if (VIR_ALLOC(buildvoldef) < 0) { - voldef = NULL; - goto cleanup; - } + /* Make a shallow copy of the 'defined' volume definition, since the + * original allocation value will change as the user polls 'info', + * but we only need the initial requested values + */ + memcpy(buildvoldef, voldef, sizeof(*voldef)); - /* Make a shallow copy of the 'defined' volume definition, since the - * original allocation value will change as the user polls 'info', - * but we only need the initial requested values - */ - memcpy(buildvoldef, voldef, sizeof(*voldef)); + if (backend->buildVol) { + int buildret; /* Drop the pool lock during volume allocation */ pool->asyncjobs++; @@ -1595,7 +1595,6 @@ storageVolCreateXML(virStoragePoolPtr obj, pool->asyncjobs--; voldef = NULL; - VIR_FREE(buildvoldef); if (buildret < 0) { virStoragePoolObjUnlock(pool); @@ -1606,6 +1605,10 @@ storageVolCreateXML(virStoragePoolPtr obj, } + /* Update pool metadata */ + pool->def->allocation += buildvoldef->allocation; + pool->def->available -= buildvoldef->allocation; + VIR_INFO("Creating volume '%s' in storage pool '%s'", volobj->name, pool->def->name); ret = volobj; @@ -1615,6 +1618,7 @@ storageVolCreateXML(virStoragePoolPtr obj, cleanup: virObjectUnref(volobj); virStorageVolDefFree(voldef); + virStorageVolDefFree(buildvoldef); if (pool) virStoragePoolObjUnlock(pool); return ret; @@ -1770,6 +1774,10 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj, goto cleanup; } + /* Updating pool metadata */ + pool->def->allocation += newvol->allocation; + pool->def->available -= newvol->allocation; + VIR_INFO("Creating volume '%s' in storage pool '%s'", volobj->name, pool->def->name); ret = volobj; @@ -2013,6 +2021,11 @@ storageVolResize(virStorageVolPtr obj, goto out; vol->capacity = abs_capacity; + + /* Update pool metadata */ + pool->def->allocation += (abs_capacity - vol->capacity); + pool->def->available -= (abs_capacity - vol->capacity); + ret = 0; out: @@ -2356,6 +2369,10 @@ storageVolDelete(virStorageVolPtr obj, if (backend->deleteVol(obj->conn, pool, vol, flags) < 0) goto cleanup; + /* Update pool metadata */ + pool->def->allocation -= vol->allocation; + pool->def->available += vol->allocation; + for (i = 0; i < pool->volumes.count; i++) { if (pool->volumes.objs[i] == vol) { VIR_INFO("Deleting volume '%s' from storage pool '%s'", -- 1.8.1.4

6 6

[libvirt] [PATCH 0/8] virsh: More intelligent auto-completion
by Tomas Meszaros 19 Aug '13

19 Aug '13

Hi, this patch series is a prototype for my GSoC project (Michal Privoznik is my mentor). I'm working on virsh auto-completion, trying to make it more "intelligent". At this stage, prototype is capable of command and option completion. Three completer functions are currently implemented so you can test it. If it turns out that this prototype is good enough, I will implement more completer functions. Tomas Meszaros (8): virsh: Add vshCmdCompleter and vshOptCompleter virsh: Add vshDomainCompleter virsh: Add vshSuspendTargetCompleter virsh: Add vshRebootShutdownModeCompleter virsh: Improve readline generators and readline completion virsh: Add completer functions to domManaggementCmds virsh: Add completer functions to snapshotCmds virsh: Add completer functions to domMonitoringCmds tools/virsh-domain-monitor.c | 32 ++- tools/virsh-domain.c | 240 +++++++++++++++----- tools/virsh-snapshot.c | 45 +++- tools/virsh.c | 507 +++++++++++++++++++++++++++++++++++++++++-- tools/virsh.h | 11 + 5 files changed, 742 insertions(+), 93 deletions(-) -- 1.8.3.1

3 14

[libvirt] [PATCH] virBitmapParse: Fix behavior in case of error
by Peter Krempa 19 Aug '13

19 Aug '13

Re-arrange the code so that the returned bitmap is always initialized to NULL even on early failures and return an error message as some callers are already expecting it. --- src/util/virbitmap.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c index 47c678e..5d0655c 100644 --- a/src/util/virbitmap.c +++ b/src/util/virbitmap.c @@ -298,23 +298,21 @@ virBitmapParse(const char *str, size_t bitmapSize) { bool neg = false; - const char *cur; + const char *cur = str; char *tmp; size_t i; int start, last; - if (!str) + if (!(*bitmap = virBitmapNew(bitmapSize))) return -1; - cur = str; - virSkipSpaces(&cur); + if (!str) + goto error; - if (*cur == 0) - return -1; + virSkipSpaces(&cur); - *bitmap = virBitmapNew(bitmapSize); - if (!*bitmap) - return -1; + if (*cur == '\0') + goto error; while (*cur != 0 && *cur != terminator) { /* @@ -384,6 +382,8 @@ virBitmapParse(const char *str, return virBitmapCountBits(*bitmap); error: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to parse bitmap '%s'", str)); virBitmapFree(*bitmap); *bitmap = NULL; return -1; -- 1.8.3.2

2 1

[libvirt] [PATCH] qemu: Drop qemuDomainMemoryLimit
by Michal Privoznik 19 Aug '13

19 Aug '13

This function is to guess the correct limit for maximal memory usage by qemu for given domain. This can never be guessed correctly, not to mention all the pains and sleepless nights this code has caused. Once somebody discovers algorithm to solve the Halting Problem, we can compute the limit algorithmically. But till then, this code should never see the light of the release again. --- src/qemu/qemu_cgroup.c | 3 +-- src/qemu/qemu_command.c | 2 +- src/qemu/qemu_domain.c | 49 ------------------------------------------------- src/qemu/qemu_domain.h | 2 -- src/qemu/qemu_hotplug.c | 2 +- 5 files changed, 3 insertions(+), 55 deletions(-) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index dc949db..9673e8e 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -428,8 +428,7 @@ qemuSetupMemoryCgroup(virDomainObjPtr vm) } } - if (virCgroupSetMemoryHardLimit(priv->cgroup, - qemuDomainMemoryLimit(vm->def)) < 0) + if (virCgroupSetMemoryHardLimit(priv->cgroup, vm->def->mem.hard_limit) < 0) return -1; if (vm->def->mem.soft_limit != 0 && diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index b811e1d..a0a1773 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -9220,7 +9220,7 @@ qemuBuildCommandLine(virConnectPtr conn, } if (mlock) - virCommandSetMaxMemLock(cmd, qemuDomainMemoryLimit(def) * 1024); + virCommandSetMaxMemLock(cmd, def->mem.hard_limit * 1024); virObjectUnref(cfg); return cmd; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 393af6b..7f4d17d 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2306,55 +2306,6 @@ cleanup: return ret; } - -unsigned long long -qemuDomainMemoryLimit(virDomainDefPtr def) -{ - unsigned long long mem; - size_t i; - - if (def->mem.hard_limit) { - mem = def->mem.hard_limit; - } else { - /* If there is no hard_limit set, compute a reasonable one to avoid - * system thrashing caused by exploited qemu. A 'reasonable - * limit' has been chosen: - * (1 + k) * (domain memory + total video memory) + (32MB for - * cache per each disk) + F - * where k = 0.5 and F = 400MB. The cache for disks is important as - * kernel cache on the host side counts into the RSS limit. - * Moreover, VFIO requires some amount for IO space. Alex Williamson - * suggested adding 1GiB for IO space just to be safe (some finer - * tuning might be nice, though). - * - * Technically, the disk cache does not have to be included in - * RLIMIT_MEMLOCK but it doesn't hurt as it's just an upper limit and - * it makes this function and its usage simpler. - */ - mem = def->mem.max_balloon; - for (i = 0; i < def->nvideos; i++) - mem += def->videos[i]->vram; - mem *= 1.5; - mem += def->ndisks * 32768; - mem += 409600; - - for (i = 0; i < def->nhostdevs; i++) { - virDomainHostdevDefPtr hostdev = def->hostdevs[i]; - if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && - hostdev->source.subsys.type == - VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI && - hostdev->source.subsys.u.pci.backend == - VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) { - mem += 1024 * 1024; - break; - } - } - } - - return mem; -} - - int qemuDomainUpdateDeviceList(virQEMUDriverPtr driver, virDomainObjPtr vm) diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 0a4a51e..21f116c 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -365,8 +365,6 @@ extern virDomainXMLPrivateDataCallbacks virQEMUDriverPrivateDataCallbacks; extern virDomainXMLNamespace virQEMUDriverDomainXMLNamespace; extern virDomainDefParserConfig virQEMUDriverDomainDefParserConfig; -unsigned long long qemuDomainMemoryLimit(virDomainDefPtr def); - int qemuDomainUpdateDeviceList(virQEMUDriverPtr driver, virDomainObjPtr vm); diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index c9748d9..fa64dd7 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1030,7 +1030,7 @@ int qemuDomainAttachHostPciDevice(virQEMUDriverPtr driver, */ vm->def->hostdevs[vm->def->nhostdevs++] = hostdev; virProcessSetMaxMemLock(vm->pid, - qemuDomainMemoryLimit(vm->def) * 1024); + vm->def->mem.hard_limit * 1024); vm->def->hostdevs[vm->def->nhostdevs--] = NULL; } -- 1.8.1.5

6 10