Re: [libvirt] [PATCH] LXC: create a bind mount for sysfs when enable userns but disable netns
ping > -Original Message- > From: libvir-list-boun...@redhat.com [mailto:libvir-list-boun...@redhat.com] > On Behalf Of Chen Hanxiao > Sent: Monday, July 14, 2014 6:02 PM > To: libvir-list@redhat.com > Subject: [libvirt] [PATCH] LXC: create a bind mount for sysfs when enable > userns > but disable netns > > kernel commit 7dc5dbc879bd0779924b5132a48b731a0bc04a1e > forbid us doing a fresh mount for sysfs > when enable userns but disable netns. > This patch will create a bind mount in this senario. > > Signed-off-by: Chen Hanxiao > --- > src/lxc/lxc_container.c | 44 +--- > 1 file changed, 33 insertions(+), 11 deletions(-) > > diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c > index 4d89677..8a27215 100644 > --- a/src/lxc/lxc_container.c > +++ b/src/lxc/lxc_container.c > @@ -815,10 +815,13 @@ static int lxcContainerSetReadOnly(void) > } > > > -static int lxcContainerMountBasicFS(bool userns_enabled) > +static int lxcContainerMountBasicFS(bool userns_enabled, > +bool netns_disabled) > { > size_t i; > int rc = -1; > +char* mnt_src = NULL; > +int mnt_mflags; > > VIR_DEBUG("Mounting basic filesystems"); > > @@ -826,8 +829,25 @@ static int lxcContainerMountBasicFS(bool userns_enabled) > bool bindOverReadonly; > virLXCBasicMountInfo const *mnt = &lxcBasicMounts[i]; > > +/* When enable userns but disable netns, kernel will > + * forbid us doing a new fresh mount for sysfs. > + * So we had to do a bind mount for sysfs instead. > + */ > +if (userns_enabled && netns_disabled && > +STREQ(mnt->src, "sysfs")) { > +if (VIR_STRDUP(mnt_src, "/sys") < 0) { > +goto cleanup; > +} > +mnt_mflags = MS_NOSUID|MS_NOEXEC|MS_NODEV|MS_RDONLY|MS_BIND; > +} else { > +if (VIR_STRDUP(mnt_src, mnt->src) < 0) { > +goto cleanup; > +} > +mnt_mflags = mnt->mflags; > +} > + > VIR_DEBUG("Processing %s -> %s", > - mnt->src, mnt->dst); > + mnt_src, mnt->dst); > > if (mnt->skipUnmounted) { > char *hostdir; > @@ -856,7 +876,7 @@ static int lxcContainerMountBasicFS(bool userns_enabled) > if (virFileMakePath(mnt->dst) < 0) { > virReportSystemError(errno, > _("Failed to mkdir %s"), > - mnt->src); > + mnt_src); > goto cleanup; > } > > @@ -867,24 +887,24 @@ static int lxcContainerMountBasicFS(bool userns_enabled) > * we mount the filesystem in read-write mode initially, and then do > a > * separate read-only bind mount on top of that. > */ > -bindOverReadonly = !!(mnt->mflags & MS_RDONLY); > +bindOverReadonly = !!(mnt_mflags & MS_RDONLY); > > VIR_DEBUG("Mount %s on %s type=%s flags=%x", > - mnt->src, mnt->dst, mnt->type, mnt->mflags & ~MS_RDONLY); > -if (mount(mnt->src, mnt->dst, mnt->type, mnt->mflags & ~MS_RDONLY, > NULL) > < 0) { > + mnt_src, mnt->dst, mnt->type, mnt_mflags & ~MS_RDONLY); > +if (mount(mnt_src, mnt->dst, mnt->type, mnt_mflags & ~MS_RDONLY, > NULL) > < 0) { > virReportSystemError(errno, > _("Failed to mount %s on %s type %s > flags=%x"), > - mnt->src, mnt->dst, NULLSTR(mnt->type), > - mnt->mflags & ~MS_RDONLY); > + mnt_src, mnt->dst, NULLSTR(mnt->type), > + mnt_mflags & ~MS_RDONLY); > goto cleanup; > } > > if (bindOverReadonly && > -mount(mnt->src, mnt->dst, NULL, > +mount(mnt_src, mnt->dst, NULL, >MS_BIND|MS_REMOUNT|MS_RDONLY, NULL) < 0) { > virReportSystemError(errno, > _("Failed to re-mount %s on %s flags=%x"), > - mnt->src, mnt->dst, > + mnt_src, mnt->dst, > MS_BIND|MS_REMOUNT|MS_RDONLY); > goto cleanup; > } > @@ -893,6 +913,7 @@ static int lxcContainerMountBasicFS(bool userns_enabled) > rc = 0; > > cleanup: > +VIR_FREE(mnt_src); > VIR_DEBUG("rc=%d", rc); > return rc; > } > @@ -1643,7 +1664,8 @@ static int lxcContainerSetupPivotRoot(virDomainDefPtr > vmDef, > goto cleanup; > > /* Mounts the core /proc, /sys, etc filesystems */ > -if (lxcContainerMountBasicFS(vmDef->idmap.nuidmap) < 0) > +if (lxcContainerMountBasicFS(vmDef->idmap.nuidmap, > + !vmDef->nnets) < 0) > goto cleanup; > > /* Ensure entire roo
[libvirt] [PATCH 2/2] remove error message when virMutexInit and virCondInit failed
Signed-off-by: Jincheng Miao --- daemon/remote.c | 1 - src/conf/interface_conf.c | 2 -- src/conf/network_conf.c | 2 -- src/conf/node_device_conf.c | 2 -- src/conf/nwfilter_conf.c| 2 -- src/conf/object_event.c | 2 -- src/conf/storage_conf.c | 2 -- src/conf/virchrdev.c| 2 -- src/esx/esx_vi.c| 15 +++ src/fdstream.c | 2 -- src/libxl/libxl_driver.c| 2 -- src/locking/lock_daemon.c | 5 - src/node_device/node_device_udev.c | 1 - src/nwfilter/nwfilter_learnipaddr.c | 2 -- src/parallels/parallels_driver.c| 5 + src/qemu/qemu_agent.c | 2 -- src/qemu/qemu_capabilities.c| 2 -- src/qemu/qemu_driver.c | 1 - src/qemu/qemu_monitor.c | 6 ++ src/remote/remote_driver.c | 2 -- src/rpc/virnetclient.c | 5 + src/test/test_driver.c | 4 src/util/vireventpoll.c | 5 + src/util/virlockspace.c | 4 src/util/virobject.c| 2 -- src/xen/xen_driver.c| 2 -- tests/commandtest.c | 4 +--- tests/qemumonitortestutils.c| 2 -- 28 files changed, 9 insertions(+), 79 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index ea16789..d3cf8a8 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -1164,7 +1164,6 @@ void *remoteClientInitHook(virNetServerClientPtr client, if (virMutexInit(&priv->lock) < 0) { VIR_FREE(priv); -virReportSystemError(errno, "%s", _("unable to init mutex")); return NULL; } diff --git a/src/conf/interface_conf.c b/src/conf/interface_conf.c index 103e878..87dc79e 100644 --- a/src/conf/interface_conf.c +++ b/src/conf/interface_conf.c @@ -1250,8 +1250,6 @@ virInterfaceObjPtr virInterfaceAssignDef(virInterfaceObjListPtr interfaces, if (VIR_ALLOC(iface) < 0) return NULL; if (virMutexInit(&iface->lock) < 0) { -virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("cannot initialize mutex")); VIR_FREE(iface); return NULL; } diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index ce4d4d8..574727a 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -388,8 +388,6 @@ virNetworkAssignDef(virNetworkObjListPtr nets, if (VIR_ALLOC(network) < 0) return NULL; if (virMutexInit(&network->lock) < 0) { -virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("cannot initialize mutex")); VIR_FREE(network); return NULL; } diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 30aa477..ed4cd2a 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -183,8 +183,6 @@ virNodeDeviceObjPtr virNodeDeviceAssignDef(virNodeDeviceObjListPtr devs, return NULL; if (virMutexInit(&device->lock) < 0) { -virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("cannot initialize mutex")); VIR_FREE(device); return NULL; } diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index 52f24e4..b662e02 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -3104,8 +3104,6 @@ virNWFilterObjAssignDef(virNWFilterObjListPtr nwfilters, return NULL; if (virMutexInitRecursive(&nwfilter->lock) < 0) { -virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("cannot initialize mutex")); VIR_FREE(nwfilter); return NULL; } diff --git a/src/conf/object_event.c b/src/conf/object_event.c index beef3e1..86d0eb8 100644 --- a/src/conf/object_event.c +++ b/src/conf/object_event.c @@ -577,8 +577,6 @@ virObjectEventStateNew(void) goto error; if (virMutexInit(&state->lock) < 0) { -virReportSystemError(errno, "%s", - _("unable to initialize state mutex")); VIR_FREE(state); goto error; } diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 69333f5..1e8edff 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1694,8 +1694,6 @@ virStoragePoolObjAssignDef(virStoragePoolObjListPtr pools, return NULL; if (virMutexInit(&pool->lock) < 0) { -virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("cannot initialize mutex")); VIR_FREE(pool); return NULL; } diff --git a/src/conf/virchrdev.c b/src/conf/virchrdev.c index 022fe71..a659b62 100644 --- a/src/conf/virchrdev.c +++ b/src/conf/virchrdev.c @@ -272,8 +272,6 @@ virChrdevsPtr virChrdevAlloc(void) return NULL; if (virMutexInit(&devs->lock) < 0) { -virReportSystemError(errno, "%s", - _("Unable to init device stream
[libvirt] [PATCH 1/2] Refactor virMutexInit virRWLockInit and virCondInit
Implement InitInternal functions for Mutex, RWLock and Cond, these functions contain error report using virReportSystemErrorFull, it is controlled by an argument 'quite'. The related macros are Init and InitQuite, they call InitInternal function by passing 'false' or 'true' to quite argument. Signed-off-by: Jincheng Miao --- src/libvirt_private.syms | 7 ++-- src/util/virthread.c | 95 src/util/virthread.h | 57 ++--- 3 files changed, 94 insertions(+), 65 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 8d3671c..b1c05d3 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2015,18 +2015,17 @@ virSystemdTerminateMachine; # util/virthread.h virCondBroadcast; virCondDestroy; -virCondInit; +virCondInitInternal; virCondSignal; virCondWait; virCondWaitUntil; virMutexDestroy; -virMutexInit; -virMutexInitRecursive; +virMutexInitInternal; virMutexLock; virMutexUnlock; virOnce; virRWLockDestroy; -virRWLockInit; +virRWLockInitInternal; virRWLockRead; virRWLockUnlock; virRWLockWrite; diff --git a/src/util/virthread.c b/src/util/virthread.c index 7e841d1..ceed1bf 100644 --- a/src/util/virthread.c +++ b/src/util/virthread.c @@ -31,6 +31,16 @@ #include "viralloc.h" +#define VIR_THREAD_ERR_EXIT(str) do { \ +errno = ret;\ +if (!quite) { \ +virReportSystemErrorFull(VIR_FROM_NONE, errno, \ + filename, funcname, linenr,\ + "%s", _(str)); \ +} \ +return -1; \ +} while (0) + /* Nothing special required for pthreads */ int virThreadInitialize(void) @@ -48,92 +58,81 @@ int virOnce(virOnceControlPtr once, virOnceFunc init) } -int virMutexInit(virMutexPtr m) +int virMutexInitInternal(virMutexPtr mutex, bool recursive, bool quite, + const char *filename, const char *funcname, + size_t linenr) { int ret; pthread_mutexattr_t attr; +int type = recursive ? PTHREAD_MUTEX_RECURSIVE : PTHREAD_MUTEX_NORMAL; pthread_mutexattr_init(&attr); -pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_NORMAL); -ret = pthread_mutex_init(&m->lock, &attr); +pthread_mutexattr_settype(&attr, type); +ret = pthread_mutex_init(&mutex->lock, &attr); pthread_mutexattr_destroy(&attr); -if (ret != 0) { -errno = ret; -return -1; -} -return 0; -} -int virMutexInitRecursive(virMutexPtr m) -{ -int ret; -pthread_mutexattr_t attr; -pthread_mutexattr_init(&attr); -pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE); -ret = pthread_mutex_init(&m->lock, &attr); -pthread_mutexattr_destroy(&attr); -if (ret != 0) { -errno = ret; -return -1; -} +if (ret != 0) +VIR_THREAD_ERR_EXIT("unable to init Mutex"); + return 0; } -void virMutexDestroy(virMutexPtr m) +void virMutexDestroy(virMutexPtr mutex) { -pthread_mutex_destroy(&m->lock); +pthread_mutex_destroy(&mutex->lock); } -void virMutexLock(virMutexPtr m) +void virMutexLock(virMutexPtr mutex) { -pthread_mutex_lock(&m->lock); +pthread_mutex_lock(&mutex->lock); } -void virMutexUnlock(virMutexPtr m) +void virMutexUnlock(virMutexPtr mutex) { -pthread_mutex_unlock(&m->lock); +pthread_mutex_unlock(&mutex->lock); } -int virRWLockInit(virRWLockPtr m) +int virRWLockInitInternal(virRWLockPtr rwlock, bool quite, + const char *filename, const char *funcname, + size_t linenr) { -int ret; -ret = pthread_rwlock_init(&m->lock, NULL); -if (ret != 0) { -errno = ret; -return -1; -} +int ret = pthread_rwlock_init(&rwlock->lock, NULL); +if (ret != 0) +VIR_THREAD_ERR_EXIT("unable to init RWLock"); + return 0; } -void virRWLockDestroy(virRWLockPtr m) +void virRWLockDestroy(virRWLockPtr rwlock) { -pthread_rwlock_destroy(&m->lock); +pthread_rwlock_destroy(&rwlock->lock); } -void virRWLockRead(virRWLockPtr m) +void virRWLockRead(virRWLockPtr rwlock) { -pthread_rwlock_rdlock(&m->lock); +pthread_rwlock_rdlock(&rwlock->lock); } -void virRWLockWrite(virRWLockPtr m) +void virRWLockWrite(virRWLockPtr rwlock) { -pthread_rwlock_wrlock(&m->lock); +pthread_rwlock_wrlock(&rwlock->lock); } -void virRWLockUnlock(virRWLockPtr m) +void virRWLockUnlock(virRWLockPtr rwlock) { -pthread_rwlock_unlock(&m->lock); +pthread_rwlock_unlock(&rwlock->lock); } -int virCondInit(virCondPtr c) +int virCondInitInternal(virCondPtr cond, bool
[libvirt] [PATCH 0/2] Refactor virMutexInit virRWLockInit and virCondInit
Implement virMutexInitInternal, virRWLockInitInternal and virCondInitInternal which include error message reporting. int virMutexInitInternal(virMutexPtr mutex, bool recursive, bool quite, const char *filename, const char *funcname, size_t linenr) virMutexInit calls virMutexInitInternal with quite == false; virMutexInitQuite calls virMutexInitInternal with quite == true. Same for virRWLockInit and virCondInit. And remove redundant error messages after virMutexInit from other source files. Jincheng Miao (2): Refactor virMutexInit virRWLockInit and virCondInit remove error message when virMutexInit and virCondInit failed daemon/remote.c | 1 - src/conf/interface_conf.c | 2 - src/conf/network_conf.c | 2 - src/conf/node_device_conf.c | 2 - src/conf/nwfilter_conf.c| 2 - src/conf/object_event.c | 2 - src/conf/storage_conf.c | 2 - src/conf/virchrdev.c| 2 - src/esx/esx_vi.c| 15 ++ src/fdstream.c | 2 - src/libvirt_private.syms| 7 ++- src/libxl/libxl_driver.c| 2 - src/locking/lock_daemon.c | 5 -- src/node_device/node_device_udev.c | 1 - src/nwfilter/nwfilter_learnipaddr.c | 2 - src/parallels/parallels_driver.c| 5 +- src/qemu/qemu_agent.c | 2 - src/qemu/qemu_capabilities.c| 2 - src/qemu/qemu_driver.c | 1 - src/qemu/qemu_monitor.c | 6 +-- src/remote/remote_driver.c | 2 - src/rpc/virnetclient.c | 5 +- src/test/test_driver.c | 4 -- src/util/vireventpoll.c | 5 +- src/util/virlockspace.c | 4 -- src/util/virobject.c| 2 - src/util/virthread.c| 95 ++--- src/util/virthread.h| 57 +- src/xen/xen_driver.c| 2 - tests/commandtest.c | 4 +- tests/qemumonitortestutils.c| 2 - 31 files changed, 103 insertions(+), 144 deletions(-) -- 1.8.3.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] CPU model API (v3)
On Tue, Jul 15, 2014 at 11:42 PM, Zeeshan Ali (Khattak) wrote: > v3: Two classes for CPU model: > * CapabilitiesCpuModel > * DomainCpuModel that derives from CapabilitiesCpuModel. Ping! I need these patches to fix the broken case of non-kvm in Boxes in a nice way: https://bugzilla.gnome.org/show_bug.cgi?id=732680 -- Regards, Zeeshan Ali (Khattak) Befriend GNOME: http://www.gnome.org/friends/ -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] libxl: Implement basic video device selection
Stefan Bader wrote: > On 16.07.2014 23:05, Jim Fehlig wrote: > >> While testing this, I noticed that libvirt will set vram to 9216 if not >> specified. E.g. >> >> # cat test.xml >> ... >> >> >> >> ... >> # virsh define test.xml >> # virsh dumpxml test >> ... >> >> >> >> ... >> >> With type='vga', libxl will then fail to start the domain >> >> libxl: error: libxl_create.c:253:libxl__domain_build_info_setdefault: >> videoram must be at least 16 MB for STDVGA on QEMU_XEN >> > > Heh, thats "funny". At least this was the same thing I observed when creating > new guests with virt-manager. Maybe I blamed the wrong part of the stack. Or > they both do it. This lead the the second part of my changes which was not so > clear about whether it should or should not go upstream. > > So from my side a fixup of some kind would be good but we can work on this in > a > follow-up. > The problem is, some domains may not start after applying this patch. Consider a domain with the following video config This would work pre-patch since libxl_domain_build_info->video_memkb would be initialized to LIBXL_MEMKB_DEFAULT when calling libxl_domain_build_info_init(). But the domain will fail to start post-patch since video_memkb is set to an invalid value. I know the current behavior is not correct either, but would be nice to fix everything up in one series. I started on a second patch that would validate input or set sane defaults in libxlDomainDeviceDefPostParse(), but while testing realized that sane defaults depend on which qemu is used. E.g. the minimum video_memkb values have doubled with QEMU_XEN vs QEMU_XEN_TRADITIONAL - see $xen_root/tools/libxl/libxl_create.c, libxl__domain_build_info_setdefault(). [I'll mention again that it is unfortunate that QEMU_XEN vs QEMU_XEN_TRADITIONAL leaked through the public API.] I'm thinking of writing a utility function to detect the old vs new qemu, hoping there is something in the help output or similar to determine which one is. Such a function would be useful for David Scott's old patch to support arbitrary user-provided https://www.redhat.com/archives/libvir-list/2013-April/msg02119.html But alas, this will have to wait until I return from vacation. Regards, Jim -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/2] Disallow wiping a sparse logical volume
On 07/17/2014 02:10 PM, John Ferlan wrote: > John Ferlan (2): > storage: Convert 'building' into a bool > storage: Disallow vol_wipe for sparse logical volumes > > src/conf/storage_conf.h | 2 +- > src/storage/storage_backend_logical.c | 39 > ++- > src/storage/storage_driver.c | 8 +++ > src/util/virstoragefile.h | 1 + > 4 files changed, 44 insertions(+), 6 deletions(-) > Thanks for the review - I adjusted the completely... funny when I typed it and looked at it I thought it looked funny but my fingers were more insistent that they had done the right thing :-) Not sure lvremove/lvresize will do the trick here - read the 'lvmthin' man page... Also online I found here was a reading or two online that suggested one cannot reduce the size of a thin pool due to some lvm bug. There's another doc that says in order to reduce the size use trim, but I'm not quite sure how to access that. In my "example" case - my extent size is 4096 and my thin/sparse size is 4096. Attemping to lvreduce results in a "New size given (xxx) not less than existing size (1 extents)." So even though I've filled half of it with 0's, there doesn't seem to be a way to do what we want. Maybe someone has more "experience" dealing with these In any case, I pushed what I have... John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 1/2] Introduce virTristateBool enum type
On 07/16/2014 04:34 AM, Ján Tomko wrote: > Replace all three-state (default/yes/no) enums with it: > virDomainBootMenu > virDomainPMState > virDomainGraphicsSpiceClipboardCopypaste > virDomainGraphicsSpiceAgentFileTransfer > virNetworkDNSForwardPlainNames virDomainBIOSUseserial Too. > --- > src/conf/domain_conf.c | 57 > - > src/conf/domain_conf.h | 43 -- > src/conf/network_conf.c | 11 ++--- > src/conf/network_conf.h | 15 +--- > src/libvirt_private.syms| 10 ++-- > src/network/bridge_driver.c | 3 +-- > src/qemu/qemu_command.c | 22 - > src/qemu/qemu_driver.c | 4 ++-- > src/util/virutil.c | 5 > src/util/virutil.h | 11 + > 10 files changed, 50 insertions(+), 131 deletions(-) > > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index 1d83f13..e374604 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -134,11 +134,6 @@ VIR_ENUM_IMPL(virDomainBoot, VIR_DOMAIN_BOOT_LAST, >"hd", >"network") > > -VIR_ENUM_IMPL(virDomainBootMenu, VIR_DOMAIN_BOOT_MENU_LAST, > - "default", > - "yes", > - "no") > - > VIR_ENUM_IMPL(virDomainFeature, VIR_DOMAIN_FEATURE_LAST, >"acpi", >"apic", > @@ -180,11 +175,6 @@ VIR_ENUM_IMPL(virDomainLockFailure, > VIR_DOMAIN_LOCK_FAILURE_LAST, >"pause", >"ignore") > > -VIR_ENUM_IMPL(virDomainPMState, VIR_DOMAIN_PM_STATE_LAST, > - "default", > - "yes", > - "no") > - > VIR_ENUM_IMPL(virDomainDevice, VIR_DOMAIN_DEVICE_LAST, >"none", >"disk", > @@ -576,18 +566,6 @@ VIR_ENUM_IMPL(virDomainGraphicsSpiceStreamingMode, >"all", >"off"); > > -VIR_ENUM_IMPL(virDomainGraphicsSpiceClipboardCopypaste, > - VIR_DOMAIN_GRAPHICS_SPICE_CLIPBOARD_COPYPASTE_LAST, > - "default", > - "yes", > - "no"); > - > -VIR_ENUM_IMPL(virDomainGraphicsSpiceAgentFileTransfer, > - VIR_DOMAIN_GRAPHICS_SPICE_AGENT_FILE_TRANSFER_LAST, > - "default", > - "yes", > - "no"); > - > VIR_ENUM_IMPL(virDomainHostdevMode, VIR_DOMAIN_HOSTDEV_MODE_LAST, >"subsystem", >"capabilities") > @@ -8789,7 +8767,7 @@ virDomainGraphicsDefParseXML(xmlNodePtr node, > } > > if ((copypasteVal = > - > virDomainGraphicsSpiceClipboardCopypasteTypeFromString(copypaste)) <= 0) { > + virTristateBoolTypeFromString(copypaste)) <= 0) { > virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > _("unknown copypaste value '%s'"), > copypaste); > VIR_FREE(copypaste); > @@ -8809,7 +8787,7 @@ virDomainGraphicsDefParseXML(xmlNodePtr node, > } > > if ((enableVal = > - > virDomainGraphicsSpiceAgentFileTransferTypeFromString(enable)) <= 0) { > + virTristateBoolTypeFromString(enable)) <= 0) { > virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > _("unknown enable value '%s'"), > enable); > VIR_FREE(enable); > @@ -9916,7 +9894,7 @@ virDomainPMStateParseXML(xmlXPathContextPtr ctxt, > int ret = -1; > char *tmp = virXPathString(xpath, ctxt); > if (tmp) { > -*val = virDomainPMStateTypeFromString(tmp); > +*val = virTristateBoolTypeFromString(tmp); > if (*val < 0) { > virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > _("unknown PM state value %s"), tmp); > @@ -10879,14 +10857,14 @@ virDomainDefParseBootXML(xmlXPathContextPtr ctxt, > > tmp = virXPathString("string(./os/bootmenu[1]/@enable)", ctxt); > if (tmp) { > -def->os.bootmenu = virDomainBootMenuTypeFromString(tmp); > +def->os.bootmenu = virTristateBoolTypeFromString(tmp); > if (def->os.bootmenu <= 0) { > /* In order not to break misconfigured machines, this > * should not emit an error, but rather set the bootmenu > * to disabled */ > VIR_WARN("disabling bootmenu due to unknown option '%s'", > tmp); > -def->os.bootmenu = VIR_DOMAIN_BOOT_MENU_DISABLED; > +def->os.bootmenu = VIR_TRISTATE_BOOL_NO; > } > VIR_FREE(tmp); > } > @@ -10901,9 +10879,9 @@ virDomainDefParseBootXML(xmlXPathContextPtr ctxt, > "for useserial")); > goto cleanup; > } > -def->os.bios.useserial = VIR_DOMAIN_BIOS_USESERIAL_YES
Re: [libvirt] [PATCH] examples: Introduce domtop
On 07/16/2014 08:27 AM, Eric Blake wrote: > On 07/16/2014 07:53 AM, Michal Privoznik wrote: >> There's this question on the list that is asked over and over again. >> How do I get {cpu, memory, ...} usage in percentage? Or its modified >> version: How do I plot nice graphs like virt-manager does? >> >> It would be nice if we have an example to inspire people. And that's >> what domtop should do. Yes, it could be written in different ways, but >> I've chosen this one as I think it show explicitly what users need to >> implement in order to imitate virt-manager's graphing. >> >> Signed-off-by: Michal Privoznik >> --- >> .gitignore | 1 + >> Makefile.am | 2 +- >> cfg.mk | 2 +- >> configure.ac| 1 + >> examples/domtop/Makefile.am | 27 +++ >> examples/domtop/domtop.c| 388 >> >> libvirt.spec.in | 2 +- >> 7 files changed, 420 insertions(+), 3 deletions(-) >> create mode 100644 examples/domtop/Makefile.am >> create mode 100644 examples/domtop/domtop.c >> > > [first round review - I still plan to compile and examine what happens > when actually running the program, which may result in more comments...] I was a bit confused that the usage depends on whether I supply a domain name or not (it looks like without a name, you just list available names and quit immediately; with a name, you show stats on just that named domain). Not sure if the help text could be enhanced to explain that. I noticed that even though my guest only has one vcpu assigned, the stats shown listed four cpus (for my test machine), so this is reporting the cumulative usage of each host cpu, and not of the guest vcpu. Definitely worth mentioning what perspective the stats are taken from. The output is hard to visually break apart - either put a blank line between output spurts (with the current one line per cpu during the spurt), or put all cpu stats on a single line per spurt. (Oh, I see Jan also suggested single line per spurt, so it becomes easier to track columns for usage patterns). Looking forward to v2. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] esx: Fix a comment about VSphere versions
2014-07-17 0:25 GMT+02:00 Geoff Hickey : > Update the VSphere version comment in esx_vi.c for ESX 5.1 and 5.5. > --- > src/esx/esx_vi.c | 14 -- > 1 file changed, 8 insertions(+), 6 deletions(-) ACK and pushed! -- Matthias Bolte http://photron.blogspot.com -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] storage: Disallow vol_wipe for sparse logical volumes
On 07/17/2014 12:10 PM, John Ferlan wrote: > https://bugzilla.redhat.com/show_bug.cgi?id=1091866 > > Add a new boolean 'sparse'. This will be used by the logical backend > storage driver to determine whether the target volume is sparse or not > (also known by a snapshot or thin logical volume). Although setting sparse > to true at creation could be seen as duplicitous to setting during > virStorageBackendLogicalMakeVol() in case there are ever other code paths > between Create and FindLVs that need to know about the volume be sparse. > > Use the 'sparse' in a new virStorageBackendLogicalVolWipe() to decide whether > to attempt to wipe the logical volume or not. For now, I have found no > means to wipe the volume without writing to it. Writing to the sparse > volume causes it to be filled. A sparse logical volume is not complely s/complely/completely/ > writeable as there exists metadata which if overwritten will cause the > sparse lv to go INACTIVE which means pool-refresh will not find it. > Access to whatever lvm uses to manage data blocks is not provided by > any API I could find. > I wonder if lvm developers could point us to an alternative; maybe even lvreduce (shrink the allocated size) followed by lvresize (regrow it, but the new growth starts life empty) might behave semantically like a wipe. But yeah, we don't need to solve that today. > Signed-off-by: John Ferlan > --- > src/storage/storage_backend_logical.c | 39 > ++- > src/util/virstoragefile.h | 1 + > 2 files changed, 39 insertions(+), 1 deletion(-) > ACK -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] storage: Convert 'building' into a bool
On 07/17/2014 12:10 PM, John Ferlan wrote: > Rather than a unsigned int, use a 'bool' since that's how it was used. > > Signed-off-by: John Ferlan > --- > src/conf/storage_conf.h | 2 +- > src/storage/storage_driver.c | 8 > 2 files changed, 5 insertions(+), 5 deletions(-) > ACK -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/2] Disallow wiping a sparse logical volume
The first patch just changes 'building' into a bool as that's how I found it used when going to add a new bool for patch 2 Patch 2 addresses the following bz: https://bugzilla.redhat.com/show_bug.cgi?id=1091866 Essentially, wiping the lv caused it to disappear after a vol-refresh. This was because the lv that was created was a sparse (or thin or snapshot via --virtualsize of -V) logical volume. The multiple names are the ways I found the feature described. When the sparse volume was written to by the wipe algorithms it actually filled the volume beyond it's capacity rendering it INACTIVE which causes pool-refresh to ignore it. As it turns out a sparse lv created of size (for example) 4 MiB has metadata contained within the size created. While we could adjust the size of the metadata, control over it's location becomes the issue. It seems there is also some guard data at each end of the sparse lv as when the wipe was done lost I/O messages were sent to the system log. After much searching it just seems that writing a sparse lv with some sort of wipe algorithm will not work. Although if anyone has suggestions I'm willing to try. I have found writing '0x00''s, the scrub utility, and a "dd if=/dev/zero of=/dev/lv_pool/lv_test bs=4096 count=1024" doesn't work. It's also of interest to note that the 'scrub' utility doesn't seem to recognize the logical volume format properly as none of the nonzero algorithms are run - each fails to open the file - for example: $ /usr/sbin/lvcreate --name lv_test -L 4096K lv_pool $ /usr/bin/scrub -f -p bsi /dev/lv_pool/lv_test scrub: using BSI patterns scrub: warning: /dev/lv_pool/lv_test is zero length $ John Ferlan (2): storage: Convert 'building' into a bool storage: Disallow vol_wipe for sparse logical volumes src/conf/storage_conf.h | 2 +- src/storage/storage_backend_logical.c | 39 ++- src/storage/storage_driver.c | 8 +++ src/util/virstoragefile.h | 1 + 4 files changed, 44 insertions(+), 6 deletions(-) -- 1.9.3 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/2] storage: Convert 'building' into a bool
Rather than a unsigned int, use a 'bool' since that's how it was used. Signed-off-by: John Ferlan --- src/conf/storage_conf.h | 2 +- src/storage/storage_driver.c | 8 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index badf7a3..54c978d 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -63,7 +63,7 @@ struct _virStorageVolDef { char *key; int type; /* virStorageVolType */ -unsigned int building; +bool building; unsigned int in_use; virStorageVolSource source; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 441da21..3830867 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1691,7 +1691,7 @@ storageVolCreateXML(virStoragePoolPtr obj, /* Drop the pool lock during volume allocation */ pool->asyncjobs++; -voldef->building = 1; +voldef->building = true; virStoragePoolObjUnlock(pool); buildret = backend->buildVol(obj->conn, pool, buildvoldef, flags); @@ -1700,7 +1700,7 @@ storageVolCreateXML(virStoragePoolPtr obj, virStoragePoolObjLock(pool); storageDriverUnlock(driver); -voldef->building = 0; +voldef->building = false; pool->asyncjobs--; if (buildret < 0) { @@ -1858,7 +1858,7 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj, /* Drop the pool lock during volume allocation */ pool->asyncjobs++; -newvol->building = 1; +newvol->building = true; origvol->in_use++; virStoragePoolObjUnlock(pool); @@ -1876,7 +1876,7 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj, storageDriverUnlock(driver); origvol->in_use--; -newvol->building = 0; +newvol->building = false; allocation = newvol->target.allocation; pool->asyncjobs--; -- 1.9.3 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/2] storage: Disallow vol_wipe for sparse logical volumes
https://bugzilla.redhat.com/show_bug.cgi?id=1091866 Add a new boolean 'sparse'. This will be used by the logical backend storage driver to determine whether the target volume is sparse or not (also known by a snapshot or thin logical volume). Although setting sparse to true at creation could be seen as duplicitous to setting during virStorageBackendLogicalMakeVol() in case there are ever other code paths between Create and FindLVs that need to know about the volume be sparse. Use the 'sparse' in a new virStorageBackendLogicalVolWipe() to decide whether to attempt to wipe the logical volume or not. For now, I have found no means to wipe the volume without writing to it. Writing to the sparse volume causes it to be filled. A sparse logical volume is not complely writeable as there exists metadata which if overwritten will cause the sparse lv to go INACTIVE which means pool-refresh will not find it. Access to whatever lvm uses to manage data blocks is not provided by any API I could find. Signed-off-by: John Ferlan --- src/storage/storage_backend_logical.c | 39 ++- src/util/virstoragefile.h | 1 + 2 files changed, 39 insertions(+), 1 deletion(-) diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index ab1e64b..ed62c2f 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -131,6 +131,15 @@ virStorageBackendLogicalMakeVol(char **const groups, goto cleanup; } +/* Mark the (s) sparse/snapshot lv, e.g. the lv created using + * the --virtualsize/-V option. We've already ignored the (t)hin + * pool definition. In the manner libvirt defines these, the + * thin pool is hidden to the lvs output, except as the name + * in brackets [] described for the groups[1] (backingStore). + */ +if (attrs[0] == 's') +vol->target.sparse = true; + /* Skips the backingStore of lv created with "--virtualsize", * its original device "/dev/$vgname/$lvname_vorigin" is * just for lvm internal use, one should never use it. @@ -752,6 +761,7 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn, VIR_DIV_UP(vol->target.allocation ? vol->target.allocation : 1, 1024)); virCommandAddArg(cmd, "--virtualsize"); +vol->target.sparse = true; } virCommandAddArgFormat(cmd, "%lluK", VIR_DIV_UP(vol->target.capacity, 1024)); @@ -829,6 +839,33 @@ virStorageBackendLogicalBuildVolFrom(virConnectPtr conn, return build_func(conn, pool, vol, inputvol, flags); } +static int +virStorageBackendLogicalVolWipe(virConnectPtr conn, +virStoragePoolObjPtr pool, +virStorageVolDefPtr vol, +unsigned int algorithm, +unsigned int flags) +{ +if (!vol->target.sparse) +return virStorageBackendVolWipeLocal(conn, pool, vol, algorithm, flags); + +/* The wiping algorithms will write something to the logical volume. + * Writing to a sparse logical volume causes it to be filled resulting + * in the volume becoming INACTIVE because there is some amount of + * metadata contained within the sparse lv. Choosing to only write + * a wipe pattern to the already written portion lv based on what + * 'lvs' shows in the "Data%" column/field for the sparse lv was + * considered. However, there is no guarantee that sparse lv could + * grow or shrink outside of libvirt's knowledge and thus still render + * the volume INACTIVE. Until there is some sort of wipe function + * implemented by lvm for one of these sparse lv, we'll just return + * unsupported. + */ +virReportError(VIR_ERR_NO_SUPPORT, + _("logical volue '%s' is sparse, volume wipe not supported"), + vol->target.path); +return -1; +} virStorageBackend virStorageBackendLogical = { .type = VIR_STORAGE_POOL_LOGICAL, @@ -846,5 +883,5 @@ virStorageBackend virStorageBackendLogical = { .deleteVol = virStorageBackendLogicalDeleteVol, .uploadVol = virStorageBackendVolUploadLocal, .downloadVol = virStorageBackendVolDownloadLocal, -.wipeVol = virStorageBackendVolWipeLocal, +.wipeVol = virStorageBackendLogicalVolWipe, }; diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index c840aa0..eccbf4e 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -248,6 +248,7 @@ struct _virStorageSource { virBitmapPtr features; char *compat; bool nocow; +bool sparse; virStoragePermsPtr perms; virStorageTimestampsPtr timestamps; -- 1.9.3 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 07/16] numatune: Encapsulate numatune configuration in order to unify results
Eric Blake wrote: > On 07/17/2014 10:57 AM, Roman Bogorodskiy wrote: > > Roman Bogorodskiy wrote: > > > > >> Looks like this breaks build with clang: > >> > >> gmake[3]: Entering directory `/usr/home/novel/code/libvirt/src' > >> CC util/libvirt_util_la-virclosecallbacks.lo > >> In file included from util/virclosecallbacks.c:28: > >> In file included from ../src/util/virclosecallbacks.h:28: > >> ../src/conf/domain_conf.h:70:35: error: redefinition of typedef > >> 'virDomainNumatune' is a C11 feature [-Werror,-Wtypedef-redefinition] > >> typedef struct _virDomainNumatune virDomainNumatune; > >> ^ > >> ../src/conf/numatune_conf.h:43:35: note: previous definition is here > >> typedef struct _virDomainNumatune virDomainNumatune; > >> ^ > > > > > I got it fixed by the following diff: > > > > diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h > > index 4c9b7e8..e4d7988 100644 > > --- a/src/conf/domain_conf.h > > +++ b/src/conf/domain_conf.h > > @@ -67,9 +67,6 @@ typedef virDomainFSDef *virDomainFSDefPtr; > > typedef struct _virDomainNetDef virDomainNetDef; > > typedef virDomainNetDef *virDomainNetDefPtr; > > > > -typedef struct _virDomainNumatune virDomainNumatune; > > -typedef virDomainNumatune *virDomainNumatunePtr; > > - > > typedef struct _virDomainInputDef virDomainInputDef; > > typedef virDomainInputDef *virDomainInputDefPtr; > > ACK to this hunk. > > > > > @@ -1854,8 +1851,6 @@ struct _virDomainResourceDef { > > * NB: if adding to this struct, virDomainDefCheckABIStability > > * may well need an update > > */ > > -typedef struct _virDomainDef virDomainDef; > > -typedef virDomainDef *virDomainDefPtr; > > struct _virDomainDef { > > But this hunk feels fishy. Why does numatune_conf.h need virDomainDef? > It's a leaky abstraction - virDomainNumatuneNodeParseXML is accessing > def->cpu directly instead of limiting itself to just def->numatune. > Also, virDomainNumatuneParseXML is accessing def->placement_mode, which > argues that placement_mode should be part of def->numatune rather than > an independent variable. > > Yes, this hunk solves the compiler fix, but it means that we are just > cementing that we didn't abstract things into a single object. That is, > my ideal setup would be that numatune has access to all the pieces that > it reads/modifies as parameters, but not access the entire virDomainDef, > and then we don't have a circular referencing situation and don't need > numatune_conf.h to be the source of our typedef declaration of virDomainDef. > > > I didn't check it beyond build and check/syntax-check though. Anyway, it > > doesn't look quite clean to have typedefs in numatune_conf.h for the > > struct declared in domain_conf.h. > > I'd be fine with your patch going in as a stop-gap compilation fix, even > if I still think that we could restructure the code better to make > numatune_conf.h self-contained and move the typedef for virDomainDef > back to domain_conf.h. Good, so I pushed the patch. Roman Bogorodskiy pgpyRYI2JAm6Q.pgp Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/2] virsh: negative numbers for specific commands
On 07/16/2014 07:35 PM, John Ferlan wrote: > Following up to the recently restarted discussion: > > http://www.redhat.com/archives/libvir-list/2014-July/msg00686.html > > regarding negative values for certain virsh commands - these patches > will document the "feature" of using a negative value to indicate > the largest value *and* for the vol-{upload|download} change 'offset' > to not accept a negative value. > > John Ferlan (2): > virsh vol-upload/download disallow negative offset > virsh: Document bandwidth maximum more clearly > > tools/virsh-volume.c | 6 +++--- > tools/virsh.pod | 33 +++-- > 2 files changed, 30 insertions(+), 9 deletions(-) > Thanks - pushed. John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 00/16] Support for per-guest-node binding
On 07/17/2014 01:37 AM, Ján Tomko wrote: > On 07/16/2014 04:42 PM, Martin Kletzander wrote: >> v3 of https://www.redhat.com/archives/libvir-list/2014-July/msg00372.html >> > > This series broke the build for me (clang 3.4.1): > In file included from util/virclosecallbacks.c:28: > In file included from ../src/util/virclosecallbacks.h:28: > ../src/conf/domain_conf.h:70:35: error: redefinition of typedef > 'virDomainNumatune' is a C11 feature [-Werror,-Wtypedef-redefinition] > typedef struct _virDomainNumatune virDomainNumatune; > ^ > ../src/conf/numatune_conf.h:43:35: note: previous definition is here > typedef struct _virDomainNumatune virDomainNumatune; Roman hit the same; see the sub-thread at 7/16. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 07/16] numatune: Encapsulate numatune configuration in order to unify results
On 07/17/2014 10:57 AM, Roman Bogorodskiy wrote: > Roman Bogorodskiy wrote: > >> Looks like this breaks build with clang: >> >> gmake[3]: Entering directory `/usr/home/novel/code/libvirt/src' >> CC util/libvirt_util_la-virclosecallbacks.lo >> In file included from util/virclosecallbacks.c:28: >> In file included from ../src/util/virclosecallbacks.h:28: >> ../src/conf/domain_conf.h:70:35: error: redefinition of typedef >> 'virDomainNumatune' is a C11 feature [-Werror,-Wtypedef-redefinition] >> typedef struct _virDomainNumatune virDomainNumatune; >> ^ >> ../src/conf/numatune_conf.h:43:35: note: previous definition is here >> typedef struct _virDomainNumatune virDomainNumatune; >> ^ > > I got it fixed by the following diff: > > diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h > index 4c9b7e8..e4d7988 100644 > --- a/src/conf/domain_conf.h > +++ b/src/conf/domain_conf.h > @@ -67,9 +67,6 @@ typedef virDomainFSDef *virDomainFSDefPtr; > typedef struct _virDomainNetDef virDomainNetDef; > typedef virDomainNetDef *virDomainNetDefPtr; > > -typedef struct _virDomainNumatune virDomainNumatune; > -typedef virDomainNumatune *virDomainNumatunePtr; > - > typedef struct _virDomainInputDef virDomainInputDef; > typedef virDomainInputDef *virDomainInputDefPtr; ACK to this hunk. > > @@ -1854,8 +1851,6 @@ struct _virDomainResourceDef { > * NB: if adding to this struct, virDomainDefCheckABIStability > * may well need an update > */ > -typedef struct _virDomainDef virDomainDef; > -typedef virDomainDef *virDomainDefPtr; > struct _virDomainDef { But this hunk feels fishy. Why does numatune_conf.h need virDomainDef? It's a leaky abstraction - virDomainNumatuneNodeParseXML is accessing def->cpu directly instead of limiting itself to just def->numatune. Also, virDomainNumatuneParseXML is accessing def->placement_mode, which argues that placement_mode should be part of def->numatune rather than an independent variable. Yes, this hunk solves the compiler fix, but it means that we are just cementing that we didn't abstract things into a single object. That is, my ideal setup would be that numatune has access to all the pieces that it reads/modifies as parameters, but not access the entire virDomainDef, and then we don't have a circular referencing situation and don't need numatune_conf.h to be the source of our typedef declaration of virDomainDef. > I didn't check it beyond build and check/syntax-check though. Anyway, it > doesn't look quite clean to have typedefs in numatune_conf.h for the > struct declared in domain_conf.h. I'd be fine with your patch going in as a stop-gap compilation fix, even if I still think that we could restructure the code better to make numatune_conf.h self-contained and move the typedef for virDomainDef back to domain_conf.h. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 07/16] numatune: Encapsulate numatune configuration in order to unify results
Roman Bogorodskiy wrote: > Martin Kletzander wrote: > > > There were numerous places where numatune configuration (and thus > > domain config as well) was changed in different ways. On some > > places this even resulted in persistent domain definition not to be > > stable (it would change with daemon's restart). > > > > In order to uniformly change how numatune config is dealt with, all > > the internals are now accessible directly only in numatune_conf.c and > > outside this file accessors must be used. > > > > Signed-off-by: Martin Kletzander > > --- > > po/POTFILES.in | 1 + > > src/conf/domain_conf.c | 159 ++- > > src/conf/domain_conf.h | 8 +- > > src/conf/numatune_conf.c | 318 > > + > > src/conf/numatune_conf.h | 72 - > > src/libvirt_private.syms | 11 + > > src/lxc/lxc_cgroup.c | 19 +- > > src/lxc/lxc_controller.c | 5 +- > > src/lxc/lxc_native.c | 15 +- > > src/parallels/parallels_driver.c | 7 +- > > src/qemu/qemu_cgroup.c | 23 +- > > src/qemu/qemu_driver.c | 84 +++--- > > src/qemu/qemu_process.c| 8 +- > > src/util/virnuma.c | 48 ++-- > > src/util/virnuma.h | 2 +- > > .../qemuxml2argv-numatune-auto-prefer.xml | 29 ++ > > .../qemuxml2xmlout-numatune-auto-prefer.xml| 29 ++ > > tests/qemuxml2xmltest.c| 2 + > > 18 files changed, 555 insertions(+), 285 deletions(-) > > create mode 100644 > > tests/qemuxml2argvdata/qemuxml2argv-numatune-auto-prefer.xml > > create mode 100644 > > tests/qemuxml2xmloutdata/qemuxml2xmlout-numatune-auto-prefer.xml > > Looks like this breaks build with clang: > > gmake[3]: Entering directory `/usr/home/novel/code/libvirt/src' > CC util/libvirt_util_la-virclosecallbacks.lo > In file included from util/virclosecallbacks.c:28: > In file included from ../src/util/virclosecallbacks.h:28: > ../src/conf/domain_conf.h:70:35: error: redefinition of typedef > 'virDomainNumatune' is a C11 feature [-Werror,-Wtypedef-redefinition] > typedef struct _virDomainNumatune virDomainNumatune; > ^ > ../src/conf/numatune_conf.h:43:35: note: previous definition is here > typedef struct _virDomainNumatune virDomainNumatune; > ^ > In file included from util/virclosecallbacks.c:28: > In file included from ../src/util/virclosecallbacks.h:28: > ../src/conf/domain_conf.h:71:28: error: redefinition of typedef > 'virDomainNumatunePtr' is a C11 feature [-Werror,-Wtypedef-redefinition] > typedef virDomainNumatune *virDomainNumatunePtr; >^ > ../src/conf/numatune_conf.h:44:28: note: previous definition is here > typedef virDomainNumatune *virDomainNumatunePtr; >^ > In file included from util/virclosecallbacks.c:28: > In file included from ../src/util/virclosecallbacks.h:28: > ../src/conf/domain_conf.h:1857:30: error: redefinition of typedef > 'virDomainDef' is a C11 feature [-Werror,-Wtypedef-redefinition] > typedef struct _virDomainDef virDomainDef; > ^ > ../src/conf/numatune_conf.h:39:30: note: previous definition is here > typedef struct _virDomainDef virDomainDef; > ^ > In file included from util/virclosecallbacks.c:28: > In file included from ../src/util/virclosecallbacks.h:28: > ../src/conf/domain_conf.h:1858:23: error: redefinition of typedef > 'virDomainDefPtr' is a C11 feature [-Werror,-Wtypedef-redefinition] > typedef virDomainDef *virDomainDefPtr; > ^ > ../src/conf/numatune_conf.h:40:23: note: previous definition is here > typedef virDomainDef *virDomainDefPtr; > ^ > 4 errors generated. > > Should we probably drop the repeating definitions from domain_conf.h as > we're including numatune_conf.h anyway? I got it fixed by the following diff: diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 4c9b7e8..e4d7988 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -67,9 +67,6 @@ typedef virDomainFSDef *virDomainFSDefPtr; typedef struct _virDomainNetDef virDomainNetDef; typedef virDomainNetDef *virDomainNetDefPtr; -typedef struct _virDomainNumatune virDomainNumatune; -typedef virDomainNumatune *virDomainNumatunePtr; - typedef struct _virDomainInputDef virDomainInputDef; typedef virDomainInputDef *virDomainInputDefPtr; @@ -1854,8 +1851,6 @@ struct _virDomainResourceDef { * NB: if adding to this struct, virDomainDefCheckABIStability * may well need an update */ -typedef struct _virDomai
Re: [libvirt] [PATCH v3 07/16] numatune: Encapsulate numatune configuration in order to unify results
Martin Kletzander wrote: > There were numerous places where numatune configuration (and thus > domain config as well) was changed in different ways. On some > places this even resulted in persistent domain definition not to be > stable (it would change with daemon's restart). > > In order to uniformly change how numatune config is dealt with, all > the internals are now accessible directly only in numatune_conf.c and > outside this file accessors must be used. > > Signed-off-by: Martin Kletzander > --- > po/POTFILES.in | 1 + > src/conf/domain_conf.c | 159 ++- > src/conf/domain_conf.h | 8 +- > src/conf/numatune_conf.c | 318 > + > src/conf/numatune_conf.h | 72 - > src/libvirt_private.syms | 11 + > src/lxc/lxc_cgroup.c | 19 +- > src/lxc/lxc_controller.c | 5 +- > src/lxc/lxc_native.c | 15 +- > src/parallels/parallels_driver.c | 7 +- > src/qemu/qemu_cgroup.c | 23 +- > src/qemu/qemu_driver.c | 84 +++--- > src/qemu/qemu_process.c| 8 +- > src/util/virnuma.c | 48 ++-- > src/util/virnuma.h | 2 +- > .../qemuxml2argv-numatune-auto-prefer.xml | 29 ++ > .../qemuxml2xmlout-numatune-auto-prefer.xml| 29 ++ > tests/qemuxml2xmltest.c| 2 + > 18 files changed, 555 insertions(+), 285 deletions(-) > create mode 100644 > tests/qemuxml2argvdata/qemuxml2argv-numatune-auto-prefer.xml > create mode 100644 > tests/qemuxml2xmloutdata/qemuxml2xmlout-numatune-auto-prefer.xml Looks like this breaks build with clang: gmake[3]: Entering directory `/usr/home/novel/code/libvirt/src' CC util/libvirt_util_la-virclosecallbacks.lo In file included from util/virclosecallbacks.c:28: In file included from ../src/util/virclosecallbacks.h:28: ../src/conf/domain_conf.h:70:35: error: redefinition of typedef 'virDomainNumatune' is a C11 feature [-Werror,-Wtypedef-redefinition] typedef struct _virDomainNumatune virDomainNumatune; ^ ../src/conf/numatune_conf.h:43:35: note: previous definition is here typedef struct _virDomainNumatune virDomainNumatune; ^ In file included from util/virclosecallbacks.c:28: In file included from ../src/util/virclosecallbacks.h:28: ../src/conf/domain_conf.h:71:28: error: redefinition of typedef 'virDomainNumatunePtr' is a C11 feature [-Werror,-Wtypedef-redefinition] typedef virDomainNumatune *virDomainNumatunePtr; ^ ../src/conf/numatune_conf.h:44:28: note: previous definition is here typedef virDomainNumatune *virDomainNumatunePtr; ^ In file included from util/virclosecallbacks.c:28: In file included from ../src/util/virclosecallbacks.h:28: ../src/conf/domain_conf.h:1857:30: error: redefinition of typedef 'virDomainDef' is a C11 feature [-Werror,-Wtypedef-redefinition] typedef struct _virDomainDef virDomainDef; ^ ../src/conf/numatune_conf.h:39:30: note: previous definition is here typedef struct _virDomainDef virDomainDef; ^ In file included from util/virclosecallbacks.c:28: In file included from ../src/util/virclosecallbacks.h:28: ../src/conf/domain_conf.h:1858:23: error: redefinition of typedef 'virDomainDefPtr' is a C11 feature [-Werror,-Wtypedef-redefinition] typedef virDomainDef *virDomainDefPtr; ^ ../src/conf/numatune_conf.h:40:23: note: previous definition is here typedef virDomainDef *virDomainDefPtr; ^ 4 errors generated. Should we probably drop the repeating definitions from domain_conf.h as we're including numatune_conf.h anyway? Roman Bogorodskiy pgprEV7Y1QBNm.pgp Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] leaseshelper: add enhancements to support all events
On 07/17/2014 09:18 AM, Peter Krempa wrote: >>> A second issue that comes into my mind is compatibility with already >>> existing files. Does this work when you already have a lease file? (I >>> didn't try it, I'm just asking). >> >> If we use the --leasefile-ro option, then although this method will >> work, but no, the lease file will *not* be read/written at all. So >> even if an old one exists, its of no use. >> But then again, the use of --leasefile-ro is mandatory, so that all >> events are captured by our helper program. For example, renew of a >> lease. > > My concerns are whether this will work in the case you already used the > leases helper as the patch is adding a few fields to the stored format. In particular, this scenario MUST work: A user installs libvirt 1.2.6, and starts a guest (which creates a leases file and longrunning helper app). Then the user upgrades to libvirt 1.2.7 and restarts libvirtd. The new libvirt MUST be able to interact with the running guest that is still using the older-style leases file and helper app, with no loss of behavior to the guest. Also, this scenario MUST work: A user installs libvirt 1.2.6. Then they upgrade to libvirt 1.2.7, but without restarting libvirtd. Then they start a guest. Note that the upgrade means that the leaseshelper application that will be started is the 1.2.7 build, but the command line to start it will be triggered by the 1.2.6 libvirtd. The new helper must do everything that the old helper did, so that the old libvirtd does not have any hiccups in operation. The easiest way to ensure that both directions of version mismatch are well-behaved is to make sure that either side can be newer than the other, that the changes are purely additive in nature, and that there are sane defaults in place in the newer code when dealing with the older data that lacks the additions. Also, while upgrade is a required scenario, downgrade generally is not (going from 1.2.7 back to 1.2.6 while a guest is still running, or expecting 1.2.7 libvirtd to be able to use 1.2.6 leaseshelper, is nice if it works but not a showstopper if it doesn't). -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] virsh vol-upload/download disallow negative offset
On 07/17/2014 09:25 AM, Peter Krempa wrote: > On 07/17/14 01:35, John Ferlan wrote: >> https://bugzilla.redhat.com/show_bug.cgi?id=1087104 >> >> Commit id 'c6212539' explicitly allowed a negative value to be used for >> offset and length as a shorthand for the largest value after commit id >> 'f18c02ec' modified virStrToLong_ui() to essentially disallow a negative >> value. >> >> However, allowing a negative value for offset ONLY worked if the negative >> value was -1 since the eventual lseek() does allow a -1 to mean the end >> of the file. Providing other negative values resulted in errors such as: lseek to -1 is not necessarily the end of the file. But I'm not sure what a better wording of this part of the commit message would be. >> >> $ virsh vol-download --pool default qcow3-vol2 /home/vm-images/raw \ >> --offset -2 --length -1000 >> error: cannot download from volume qcow3-vol2 >> error: Unable to seek /home/vm-images/qcow3-vol2 to 18446744073709551614: >> Invalid argument >> >> $ >> >> Thus, it seems unreasonable to expect or allow a negative value for offset >> since the only benefit is to lseek() to the end of the file and then only >> take advantage of how the OS would handle such a seek. For the purposes of >> upload or download of volume data, that seems to be a no-op. Therefore, >> disallow a negative value for offset. >> >> Additionally, modify the man page for vol-upload and vol-download to provide >> more details regarding the valid values for both offset and length. >> >> Signed-off-by: John Ferlan >> --- >> tools/virsh-volume.c | 6 +++--- >> tools/virsh.pod | 10 -- >> 2 files changed, 11 insertions(+), 5 deletions(-) >> > > ACK, although I'd prefer Eric stating his opinion on this too. > At any rate, the code and documentation of this patch are fine with me - keeping the negative length as shorthand to catch the rest of the file is fine. And if we ever want to add negative offset as distance from the end of the file, we can still do that in the future; but disabling it for now is fine. I think the ACK stands. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v1 3/7] qemu: Utilize virFileFindHugeTLBFS
Use better detection of hugetlbfs mount points. Yes, there can be multiple mount points each serving different huge page size. Since we already have ability to override the mount point in the qemu.conf file, this crazy backward compatibility code is brought in. Now we allow multiple mount points, so the "hugetlbfs_mount" option must take an list of strings (mount points). But previously, it was just a string, so we must accept both types now. Signed-off-by: Michal Privoznik --- src/Makefile.am | 1 + src/qemu/qemu_command.c | 20 src/qemu/qemu_conf.c | 122 +-- src/qemu/qemu_conf.h | 9 +++- src/qemu/qemu_driver.c | 39 +++ src/qemu/qemu_process.c | 21 +--- tests/qemuxml2argvtest.c | 10 ++-- 7 files changed, 166 insertions(+), 56 deletions(-) diff --git a/src/Makefile.am b/src/Makefile.am index 982f63d..85e61c2 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -1212,6 +1212,7 @@ libvirt_driver_qemu_impl_la_LIBADD = $(CAPNG_LIBS) \ $(GNUTLS_LIBS) \ $(LIBNL_LIBS) \ $(LIBXML_LIBS) \ + libvirt_util.la \ $(NULL) libvirt_driver_qemu_impl_la_SOURCES = $(QEMU_DRIVER_SOURCES) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 8062510..b14ce83 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7333,14 +7333,12 @@ qemuBuildCommandLine(virConnectPtr conn, def->mem.max_balloon = VIR_DIV_UP(def->mem.max_balloon, 1024) * 1024; virCommandAddArgFormat(cmd, "%llu", def->mem.max_balloon / 1024); if (def->mem.hugepage_backed) { -if (!cfg->hugetlbfsMount) { +char *mem_path; + +if (!cfg->nhugetlbfs) { virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("hugetlbfs filesystem is not mounted")); -goto error; -} -if (!cfg->hugepagePath) { -virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("hugepages are disabled by administrator config")); + "%s", _("hugetlbfs filesystem is not mounted " + "or disabled by administrator config")); goto error; } if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MEM_PATH)) { @@ -7349,8 +7347,14 @@ qemuBuildCommandLine(virConnectPtr conn, def->emulator); goto error; } + +if (!(mem_path = qemuGetDefaultHugepath(cfg->hugetlbfs, +cfg->nhugetlbfs))) +goto error; + virCommandAddArgList(cmd, "-mem-prealloc", "-mem-path", - cfg->hugepagePath, NULL); + mem_path, NULL); +VIR_FREE(mem_path); } if (def->mem.locked && !virQEMUCapsGet(qemuCaps, QEMU_CAPS_MLOCK)) { diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index e62bec0..cf5ce97 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -230,19 +230,13 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) cfg->migrationPortMin = QEMU_MIGRATION_PORT_MIN; cfg->migrationPortMax = QEMU_MIGRATION_PORT_MAX; -#if defined HAVE_MNTENT_H && defined HAVE_GETMNTENT_R -/* For privileged driver, try and find hugepage mount automatically. +/* For privileged driver, try and find hugetlbfs mounts automatically. * Non-privileged driver requires admin to create a dir for the - * user, chown it, and then let user configure it manually */ + * user, chown it, and then let user configure it manually. */ if (privileged && -!(cfg->hugetlbfsMount = virFileFindMountPoint("hugetlbfs"))) { -if (errno != ENOENT) { -virReportSystemError(errno, "%s", - _("unable to find hugetlbfs mountpoint")); -goto error; -} -} -#endif +virFileFindHugeTLBFS(&cfg->hugetlbfs, &cfg->nhugetlbfs) < 0) +goto error; + if (VIR_STRDUP(cfg->bridgeHelperName, "/usr/libexec/qemu-bridge-helper") < 0) goto error; @@ -293,8 +287,11 @@ static void virQEMUDriverConfigDispose(void *obj) VIR_FREE(cfg->spicePassword); VIR_FREE(cfg->spiceSASLdir); -VIR_FREE(cfg->hugetlbfsMount); -VIR_FREE(cfg->hugepagePath); +while (cfg->nhugetlbfs) { +cfg->nhugetlbfs--; +VIR_FREE(cfg->hugetlbfs[cfg->nhugetlbfs].mnt_dir); +} +VIR_FREE(cfg->hugetlbfs); VIR_FREE(cfg->bridgeHelperName); VIR_FREE(cfg->saveImageFormat); @@ -307,6 +304,26 @@ static void virQEMUDriverConfigDispose(void *obj) } +static int +virQEMUDriverConfigHugeTLBFSInit(virHugeTLBFSPtr hugetlbfs, + const char *path, + bool deflt) +{ +int ret = -1; + +
[libvirt] [PATCH v1 6/7] qemu: Implement ./hugepages/page/[@size, @unit, @nodeset]
Signed-off-by: Michal Privoznik --- src/qemu/qemu_capabilities.c | 2 + src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c| 91 +++--- .../qemuxml2argv-hugepages-pages.args | 16 tests/qemuxml2argvdata/qemuxml2argv-hugepages.args | 2 +- tests/qemuxml2argvtest.c | 10 ++- 6 files changed, 109 insertions(+), 13 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages.args diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 07306e5..f69c4d0 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -263,6 +263,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "memory-backend-ram", /* 170 */ "numa", + "memory-backend-file", ); @@ -1481,6 +1482,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = { { "pvpanic", QEMU_CAPS_DEVICE_PANIC }, { "usb-kbd", QEMU_CAPS_DEVICE_USB_KBD }, { "memory-backend-ram", QEMU_CAPS_OBJECT_MEMORY_RAM }, +{ "memory-backend-file", QEMU_CAPS_OBJECT_MEMORY_FILE }, }; static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsVirtioBlk[] = { diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 4332633..e80a377 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -211,6 +211,7 @@ typedef enum { QEMU_CAPS_CHANGE_BACKING_FILE = 169, /* change name of backing file in metadata */ QEMU_CAPS_OBJECT_MEMORY_RAM = 170, /* -object memory-backend-ram */ QEMU_CAPS_NUMA = 171, /* newer -numa handling with disjoint cpu ranges */ +QEMU_CAPS_OBJECT_MEMORY_FILE = 172, /* -object memory-backend-file */ QEMU_CAPS_LAST, /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 0b8cef5..cb35727 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6381,24 +6381,36 @@ qemuBuildSmpArgStr(const virDomainDef *def, } static int -qemuBuildNumaArgStr(const virDomainDef *def, +qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg, +const virDomainDef *def, virCommandPtr cmd, virQEMUCapsPtr qemuCaps) { -size_t i; +size_t i, j; virBuffer buf = VIR_BUFFER_INITIALIZER; +virDomainHugePagePtr master_hugepage = NULL; char *cpumask = NULL, *tmpmask = NULL, *next = NULL; char *nodemask = NULL; +char *mem_path = NULL; int ret = -1; if (virDomainNumatuneHasPerNodeBinding(def->numatune) && -!virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_RAM)) { +!(virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_RAM) || + virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_FILE))) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Per-node memory binding is not supported " "with this QEMU")); goto cleanup; } +if (def->mem.nhugepages && def->mem.hugepages[0].size && +!virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_FILE)) { +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("huge pages pre NUMA node are not " + "supported with this QEMU")); +goto cleanup; +} + for (i = 0; i < def->cpu->ncells; i++) { int cellmem = VIR_DIV_UP(def->cpu->cells[i].mem, 1024); def->cpu->cells[i].mem = cellmem * 1024; @@ -6417,15 +6429,74 @@ qemuBuildNumaArgStr(const virDomainDef *def, goto cleanup; } -if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_RAM)) { +if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_RAM) || +virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_FILE)) { virDomainNumatuneMemMode mode; +virDomainHugePagePtr hugepage = NULL; const char *policy = NULL; mode = virDomainNumatuneGetMode(def->numatune, i); policy = qemuNumaPolicyTypeToString(mode); -virBufferAsprintf(&buf, "memory-backend-ram,size=%dM,id=ram-node%zu", - cellmem, i); +/* Find the huge page size we want to use */ +for (j = 0; j < def->mem.nhugepages; j++) { +bool thisHugepage = false; + +hugepage = &def->mem.hugepages[j]; + +if (!hugepage->nodemask) { +master_hugepage = hugepage; +continue; +} + +if (virBitmapGetBit(hugepage->nodemask, i, &thisHugepage) < 0) { +/* Ignore this error. It's not an error after all. Well, + * the nodemask for this can contain lower NUMA + * nodes than we are querying in here. */ +
[libvirt] [PATCH v1 5/7] domain: Introduce ./hugepages/page/[@size, @unit, @nodeset]
Signed-off-by: Michal Privoznik --- docs/formatdomain.html.in | 18 +- docs/schemas/domaincommon.rng | 19 +- src/conf/domain_conf.c | 197 +++-- src/conf/domain_conf.h | 13 +- src/parallels/parallels_driver.c | 2 +- src/qemu/qemu_command.c| 2 +- src/qemu/qemu_conf.c | 20 ++- src/qemu/qemu_process.c| 2 +- .../qemuxml2argv-hugepages-pages.xml | 45 + tests/qemuxml2xmltest.c| 1 + 10 files changed, 288 insertions(+), 31 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 3c85fc5..f4362e6 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -617,7 +617,9 @@... - @@ -632,7 +634,19 @@ hugepages This tells the hypervisor that the guest should have its memory -allocated using hugepages instead of the normal native page size. + allocated using hugepages instead of the normal native page size. + Since 1.2.5 it's possible to set hugepages + more specifically per numa node. The page element is + introduced. It has one compulsory attribute size which + specifies which hugepages should be used (especially useful on systems + supporting hugepages of different sizes). The default unit for the + size attribute is kilobytes (multiplier of 1024). If you + want to use different unit, use optional unit attribute. + For systems with NUMA, the optional nodeset attribute may + come handy as it ties given guest's NUMA nodes to certain hugepage + sizes. From the example snippet, one gigabyte hugepages are used for + every NUMA node except node number four. For the correct syntax see + this. nosharepages Instructs hypervisor to disable shared pages (memory merge, KSM) for this domain. Since 1.0.6 diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 2caeef9..d9da0bc 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -567,7 +567,24 @@ - + + + + + + + + + + + + + + + + + + diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a1ef374..b49bcb0 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11258,6 +11258,57 @@ virDomainParseMemory(const char *xpath, xmlXPathContextPtr ctxt, } +static int +virDomainHugepagesParseXML(xmlNodePtr node, + xmlXPathContextPtr ctxt, + virDomainHugePagePtr hugepage) +{ +int ret = -1; +xmlNodePtr oldnode = ctxt->node; +unsigned long long bytes, max; +char *unit = NULL, *nodeset = NULL; + +ctxt->node = node; + +/* On 32-bit machines, our bound is 0x * KiB. On 64-bit + * machines, our bound is off_t (2^63). */ +if (sizeof(unsigned long) < sizeof(long long)) +max = 1024ull * ULONG_MAX; +else +max = LLONG_MAX; + +if (virXPathULongLong("string(./@size)", ctxt, &bytes) < 0) { +virReportError(VIR_ERR_XML_DETAIL, "%s", + _("unable to parse size attribute")); +goto cleanup; +} + +unit = virXPathString("string(./@unit)", ctxt); + +if (virScaleInteger(&bytes, unit, 1024, max) < 0) +goto cleanup; + +if (!(hugepage->size = VIR_DIV_UP(bytes, 1024))) { +virReportError(VIR_ERR_XML_DETAIL, "%s", + _("hugepage size can't be zero")); +goto cleanup; +} + +if ((nodeset = virXMLPropString(node, "nodeset"))) { +if (virBitmapParse(nodeset, 0, &hugepage->nodemask, + VIR_DOMAIN_CPUMASK_LEN) < 0) +goto cleanup; +} + +ret = 0; + cleanup: +VIR_FREE(unit); +VIR_FREE(nodeset); +ctxt->node = oldnode; +return ret; +} + + static virDomainResourceDefPtr virDomainResourceDefParse(xmlNodePtr node, xmlXPathContextPtr ctxt) @@ -11325,7 +11376,7 @@ virDomainDefParseXML(xmlDocPtr xml, { xmlNodePtr *nodes+ + +
[libvirt] [PATCH v1 7/7] tests: Some testing of hugepages mapping
Signed-off-by: Michal Privoznik --- .../qemuxml2argv-hugepages-pages2.args | 10 ++ .../qemuxml2argv-hugepages-pages2.xml | 38 ++ .../qemuxml2argv-hugepages-pages3.args | 9 + .../qemuxml2argv-hugepages-pages3.xml | 38 ++ tests/qemuxml2argvtest.c | 4 +++ tests/qemuxml2xmltest.c| 2 ++ 6 files changed, 101 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages2.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages2.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages3.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages3.xml diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages2.args b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages2.args new file mode 100644 index 000..9211bc6 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages2.args @@ -0,0 +1,10 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ +/usr/bin/qemu -S -M pc -m 1024 -smp 2 \ +-object memory-backend-file,prealloc=yes,\ +mem-path=/dev/hugepages2M/libvirt/qemu,size=256M,id=ram-node0 \ +-numa node,nodeid=0,cpus=0,memdev=ram-node0 \ +-object memory-backend-file,prealloc=yes,\ +mem-path=/dev/hugepages2M/libvirt/qemu,size=768M,id=ram-node1 \ +-numa node,nodeid=1,cpus=1,memdev=ram-node1 \ +-nographic -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c \ +-usb -hda /dev/HostVG/QEMUGuest1 -net none -serial none -parallel none diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages2.xml b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages2.xml new file mode 100644 index 000..3df870b --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages2.xml @@ -0,0 +1,38 @@ + + SomeDummyHugepagesGuest + ef1bdff4-27f3-4e85-a807-5fb4d58463cc + 1048576 + 1048576 + + + + + + 2 + +hvm + + + + + + + + + + destroy + restart + destroy + +/usr/bin/qemu + + + + + + + + + + + diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages3.args b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages3.args new file mode 100644 index 000..27b3f8e --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages3.args @@ -0,0 +1,9 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ +/usr/bin/qemu -S -M pc -m 1024 -smp 2 \ +-object memory-backend-ram,size=256M,id=ram-node0 \ +-numa node,nodeid=0,cpus=0,memdev=ram-node0 \ +-object memory-backend-file,prealloc=yes,\ +mem-path=/dev/hugepages1G/libvirt/qemu,size=768M,id=ram-node1 \ +-numa node,nodeid=1,cpus=1,memdev=ram-node1 \ +-nographic -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb \ +-hda /dev/HostVG/QEMUGuest1 -net none -serial none -parallel none diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages3.xml b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages3.xml new file mode 100644 index 000..35aa2cf --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages3.xml @@ -0,0 +1,38 @@ + + SomeDummyHugepagesGuest + ef1bdff4-27f3-4e85-a807-5fb4d58463cc + 1048576 + 1048576 + + + + + + 2 + +hvm + + + + + + + + + + destroy + restart + destroy + +/usr/bin/qemu + + + + + + + + + + + diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 63c9c4b..b4505a9 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -669,6 +669,10 @@ mymain(void) DO_TEST("hugepages", QEMU_CAPS_MEM_PATH); DO_TEST("hugepages-pages", QEMU_CAPS_MEM_PATH, QEMU_CAPS_OBJECT_MEMORY_RAM, QEMU_CAPS_OBJECT_MEMORY_FILE); +DO_TEST("hugepages-pages2", QEMU_CAPS_MEM_PATH, QEMU_CAPS_OBJECT_MEMORY_RAM, +QEMU_CAPS_OBJECT_MEMORY_FILE); +DO_TEST("hugepages-pages3", QEMU_CAPS_MEM_PATH, QEMU_CAPS_OBJECT_MEMORY_RAM, +QEMU_CAPS_OBJECT_MEMORY_FILE); DO_TEST("nosharepages", QEMU_CAPS_MACHINE_OPT, QEMU_CAPS_MEM_MERGE); DO_TEST("disk-cdrom", NONE); DO_TEST("disk-cdrom-network-http", QEMU_CAPS_KVM, QEMU_CAPS_DEVICE, diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 09cb228..8d46b40 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -198,6 +198,8 @@ mymain(void) DO_TEST("hugepages"); DO_TEST("hugepages-pages"); +DO_TEST("hugepages-pages2"); +DO_TEST("hugepages-pages3"); DO_TEST("nosharepages"); DO_TEST("disk-aio"); DO_TEST("disk-cdrom"); -- 1.8.5.5 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v1 4/7] virbitmap: Introduce virBitmapDoesIntersect
This internal API just checks if two bitmaps intersect or not. Signed-off-by: Michal Privoznik --- src/libvirt_private.syms | 1 + src/util/virbitmap.c | 20 src/util/virbitmap.h | 3 +++ tests/virbitmaptest.c| 26 ++ 4 files changed, 50 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 44403fd..256edd5 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1016,6 +1016,7 @@ virBitmapClearBit; virBitmapCopy; virBitmapCountBits; virBitmapDataToString; +virBitmapDoesIntersect; virBitmapEqual; virBitmapFormat; virBitmapFree; diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c index 1029635..9f82eb9 100644 --- a/src/util/virbitmap.c +++ b/src/util/virbitmap.c @@ -732,3 +732,23 @@ virBitmapDataToString(void *data, virBitmapFree(map); return ret; } + +bool +virBitmapDoesIntersect(virBitmapPtr b1, + virBitmapPtr b2) +{ +size_t i; + +if (b1->max_bit > b2->max_bit) { +virBitmapPtr tmp = b1; +b1 = b2; +b2 = tmp; +} + +for (i = 0; i < b1->map_len; i++) { +if (b1->map[i] & b2->map[i]) +return true; +} + +return false; +} diff --git a/src/util/virbitmap.h b/src/util/virbitmap.h index 142a218..063516a 100644 --- a/src/util/virbitmap.h +++ b/src/util/virbitmap.h @@ -114,5 +114,8 @@ size_t virBitmapCountBits(virBitmapPtr bitmap) char *virBitmapDataToString(void *data, int len) ATTRIBUTE_NONNULL(1); +bool virBitmapDoesIntersect(virBitmapPtr b1, +virBitmapPtr b2) +ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); #endif diff --git a/tests/virbitmaptest.c b/tests/virbitmaptest.c index 048946f..09b40d7 100644 --- a/tests/virbitmaptest.c +++ b/tests/virbitmaptest.c @@ -510,6 +510,30 @@ test9(const void *opaque ATTRIBUTE_UNUSED) } static int +test10(const void *opaque ATTRIBUTE_UNUSED) +{ +int ret = -1; +virBitmapPtr b1 = NULL, b2 = NULL, b3 = NULL; + +if (virBitmapParse("0-3,5-8,11-15", 0, &b1, 20) < 0 || +virBitmapParse("4,9,10,16-19", 0, &b2, 20) < 0 || +virBitmapParse("15", 0, &b3, 20) < 0) +goto cleanup; + +if (virBitmapDoesIntersect(b1, b2) || +virBitmapDoesIntersect(b2, b3) || +!virBitmapDoesIntersect(b1, b3)) +goto cleanup; + +ret = 0; + cleanup: +virBitmapFree(b1); +virBitmapFree(b2); +virBitmapFree(b3); +return ret; +} + +static int mymain(void) { int ret = 0; @@ -532,6 +556,8 @@ mymain(void) ret = -1; if (virtTestRun("test9", test9, NULL) < 0) ret = -1; +if (virtTestRun("test10", test10, NULL) < 0) +ret = -1; return ret; } -- 1.8.5.5 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v1 1/7] configure: Check for statfs
The statfs(2) gets filesystem statistics. Currently, we use it only on linux, and leave stub to implement on other platforms. But hey, other platforms (like FreeBSD) have statfs() too. If we check it in configure we can wider platforms supported. Speaking of FreeBSD, the headers to include are of course different: sys/param.h and sys/mount.h on the FreeBSD and sys/statfs.h on the Linux. The header files are checked too. Signed-off-by: Michal Privoznik --- configure.ac | 4 ++-- src/util/virfile.c | 21 ++--- 2 files changed, 16 insertions(+), 9 deletions(-) diff --git a/configure.ac b/configure.ac index 8001e24..68ebf75 100644 --- a/configure.ac +++ b/configure.ac @@ -275,7 +275,7 @@ dnl and various less common threadsafe functions AC_CHECK_FUNCS_ONCE([cfmakeraw fallocate geteuid getgid getgrnam_r \ getmntent_r getpwuid_r getuid kill mmap newlocale posix_fallocate \ posix_memalign prlimit regexec sched_getaffinity setgroups setns \ - setrlimit symlink sysctlbyname getifaddrs]) + setrlimit symlink sysctlbyname getifaddrs statfs]) dnl Availability of pthread functions. Because of $LIB_PTHREAD, we dnl cannot use AC_CHECK_FUNCS_ONCE. LIB_PTHREAD and LIBMULTITHREAD @@ -316,7 +316,7 @@ dnl Availability of various common headers (non-fatal if missing). AC_CHECK_HEADERS([pwd.h paths.h regex.h sys/un.h \ sys/poll.h syslog.h mntent.h net/ethernet.h linux/magic.h \ sys/un.h sys/syscall.h sys/sysctl.h netinet/tcp.h ifaddrs.h \ - libtasn1.h sys/ucred.h sys/mount.h]) + libtasn1.h sys/ucred.h sys/mount.h sys/param.h sys/statfs.h]) dnl Check whether endian provides handy macros. AC_CHECK_DECLS([htole64], [], [], [[#include ]]) diff --git a/src/util/virfile.c b/src/util/virfile.c index 463064c..699b9f8 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -43,11 +43,15 @@ # include #endif -#ifdef __linux__ -# if HAVE_LINUX_MAGIC_H -# include -# endif +#if HAVE_LINUX_MAGIC_H +# include +#endif + +#ifdef HAVE_SYS_STATFS_H # include +#elif defined HAVE_SYS_PARAM_H && defined HAVE_SYS_MOUNT_H +# include +# include #endif #if defined(__linux__) && HAVE_DECL_LO_FLAGS_AUTOCLEAR @@ -2817,7 +2821,7 @@ int virFilePrintf(FILE *fp, const char *msg, ...) } -#ifdef __linux__ +#ifdef HAVE_STATFS # ifndef NFS_SUPER_MAGIC # define NFS_SUPER_MAGIC 0x6969 @@ -2909,14 +2913,17 @@ virFileIsSharedFSType(const char *path, return 0; } -#else + +#else /* ! HAVE_STATFS */ + int virFileIsSharedFSType(const char *path ATTRIBUTE_UNUSED, int fstypes ATTRIBUTE_UNUSED) { /* XXX implement me :-) */ return 0; } -#endif + +#endif /* HAVE_STATFS */ int virFileIsSharedFS(const char *path) { -- 1.8.5.5 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v1 2/7] Introduce virFileFindHugeTLBFS
This should iterate over mount tab and search for hugetlbfs among with looking for the default value of huge pages. Signed-off-by: Michal Privoznik --- src/libvirt_private.syms | 1 + src/util/virfile.c | 155 +++ src/util/virfile.h | 12 3 files changed, 168 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 8d3671c..44403fd 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1297,6 +1297,7 @@ virFileDirectFdFlag; virFileExists; virFileFclose; virFileFdopen; +virFileFindHugeTLBFS; virFileFindMountPoint; virFileFindResource; virFileFindResourceFull; diff --git a/src/util/virfile.c b/src/util/virfile.c index 699b9f8..e1034b7 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -2841,6 +2841,9 @@ int virFilePrintf(FILE *fp, const char *msg, ...) # ifndef CIFS_SUPER_MAGIC # define CIFS_SUPER_MAGIC 0xFF534D42 # endif +# ifndef HUGETLBFS_MAGIC +# define HUGETLBFS_MAGIC 0x958458f6 +# endif int virFileIsSharedFSType(const char *path, @@ -2914,6 +2917,33 @@ virFileIsSharedFSType(const char *path, return 0; } +int +virFileGetHugepageSize(const char *path, + unsigned long long *size) +{ +int ret = -1; +struct statfs fs; + +if (statfs(path, &fs) < 0) { +virReportSystemError(errno, + _("cannot determine filesystem for '%s'"), + path); +goto cleanup; +} + +if (fs.f_type != HUGETLBFS_MAGIC) { +virReportError(VIR_ERR_INTERNAL_ERROR, + _("not a hugetlbfs mount: '%s'"), + path); +goto cleanup; +} + +*size = fs.f_bsize / 1024; /* we are storing size in KiB */ +ret = 0; + cleanup: +return ret; +} + #else /* ! HAVE_STATFS */ int virFileIsSharedFSType(const char *path ATTRIBUTE_UNUSED, @@ -2923,6 +2953,17 @@ int virFileIsSharedFSType(const char *path ATTRIBUTE_UNUSED, return 0; } +int +virFileGetHugepageSize(const char *path, + unsigned long long *size) +{ +/* XXX implement me :-) */ +VIR_WARN("Trying to get huge page size on %s is not implemented yet.", + path); +*size = 2048; /* Pure guess */ +return 0; +} + #endif /* HAVE_STATFS */ int virFileIsSharedFS(const char *path) @@ -2935,3 +2976,117 @@ int virFileIsSharedFS(const char *path) VIR_FILE_SHFS_SMB | VIR_FILE_SHFS_CIFS); } + + +#if defined __linux__ && defined HAVE_MNTENT_H && defined HAVE_GETMNTENT_R + +# define PROC_MEMINFO "/proc/meminfo" +# define HUGEPAGESIZE_STR "Hugepagesize:" + +static int +virFileGetDefaultHugepageSize(unsigned long long *size) +{ +int ret = -1; +char *meminfo, *c, *n, *unit; + +if (virFileReadAll(PROC_MEMINFO, 4096, &meminfo) < 0) +goto cleanup; + +if (!(c = strstr(meminfo, HUGEPAGESIZE_STR))) { +virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to parse %s"), + PROC_MEMINFO); +goto cleanup; +} +c += strlen(HUGEPAGESIZE_STR); + +if ((n = strchr(c, '\n'))) { +/* Cut off the rest of the meminfo file */ +*n = '\0'; +} + +if (virStrToLong_ull(c, &unit, 10, size) < 0 || STRNEQ(unit, " kB")) { +virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to parse %s %s"), + HUGEPAGESIZE_STR, c); +goto cleanup; +} + +ret = 0; + cleanup: +VIR_FREE(meminfo); +return ret; +} + +# define PROC_MOUNTS "/proc/mounts" + +int +virFileFindHugeTLBFS(virHugeTLBFSPtr *ret_fs, + size_t *ret_nfs) +{ +int ret = -1; +FILE *f = NULL; +struct mntent mb; +char mntbuf[1024]; +virHugeTLBFSPtr fs = NULL; +size_t nfs = 0; +unsigned long long default_hugepagesz; + +if (virFileGetDefaultHugepageSize(&default_hugepagesz) < 0) +goto cleanup; + +if (!(f = setmntent(PROC_MOUNTS, "r"))) { +virReportSystemError(errno, + _("Unable to open %s"), + PROC_MOUNTS); +goto cleanup; +} + +while (getmntent_r(f, &mb, mntbuf, sizeof(mntbuf))) { +virHugeTLBFSPtr tmp; + +if (STRNEQ(mb.mnt_type, "hugetlbfs")) +continue; + +if (VIR_REALLOC_N(fs, nfs + 1) < 0) +goto cleanup; + +tmp = &fs[nfs]; +nfs++; + +if (VIR_STRDUP(tmp->mnt_dir, mb.mnt_dir) < 0) +goto cleanup; + +if (virFileGetHugepageSize(tmp->mnt_dir, &tmp->size) < 0) +goto cleanup; + +tmp->deflt = tmp->size == default_hugepagesz; +} + +*ret_fs = fs; +*ret_nfs = nfs; +fs = NULL; +nfs = 0; +ret = 0; + + cleanup: +endmntent(f); +while (nfs) +VIR_FREE(fs[--nfs].mnt_dir); +VIR_FREE(fs); +return r
[libvirt] [PATCH v1 0/7] Hugepages wrt to NUMA
Up to now, domains are either backed by an arbitrary huge page but without any NUMA awareness. This is suboptimal and I'm trying to fix it. Michal Privoznik (7): configure: Check for statfs Introduce virFileFindHugeTLBFS qemu: Utilize virFileFindHugeTLBFS virbitmap: Introduce virBitmapDoesIntersect domain: Introduce ./hugepages/page/[@size,@unit,@nodeset] qemu: Implement ./hugepages/page/[@size,@unit,@nodeset] tests: Some testing of hugepages mapping configure.ac | 4 +- docs/formatdomain.html.in | 18 +- docs/schemas/domaincommon.rng | 19 +- src/Makefile.am| 1 + src/conf/domain_conf.c | 197 +++-- src/conf/domain_conf.h | 13 +- src/libvirt_private.syms | 2 + src/parallels/parallels_driver.c | 2 +- src/qemu/qemu_capabilities.c | 2 + src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c| 111 ++-- src/qemu/qemu_conf.c | 124 +++-- src/qemu/qemu_conf.h | 9 +- src/qemu/qemu_driver.c | 39 ++-- src/qemu/qemu_process.c| 21 ++- src/util/virbitmap.c | 20 +++ src/util/virbitmap.h | 3 + src/util/virfile.c | 176 +- src/util/virfile.h | 12 ++ .../qemuxml2argv-hugepages-pages.args | 16 ++ .../qemuxml2argv-hugepages-pages.xml | 45 + .../qemuxml2argv-hugepages-pages2.args | 10 ++ .../qemuxml2argv-hugepages-pages2.xml | 38 .../qemuxml2argv-hugepages-pages3.args | 9 + .../qemuxml2argv-hugepages-pages3.xml | 38 tests/qemuxml2argvdata/qemuxml2argv-hugepages.args | 2 +- tests/qemuxml2argvtest.c | 18 +- tests/qemuxml2xmltest.c| 3 + tests/virbitmaptest.c | 26 +++ 29 files changed, 884 insertions(+), 95 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages2.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages2.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages3.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages3.xml -- 1.8.5.5 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] leaseshelper: add enhancements to support all events
> My concerns are whether this will work in the case you already used the > leases helper as the patch is adding a few fields to the stored format. In this patch, I am only adding server-duid as an extra JSON object. I am not adding more fields to an existing JSON object. The old leases file will certainly work, even if server-duid is not present. Server-DUID will get added in any event which will get triggered from the ipv6 interface. But all of this will happen assuming dnsmasq and libvirt have been restarted. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] virsh vol-upload/download disallow negative offset
On 07/17/14 01:35, John Ferlan wrote: > https://bugzilla.redhat.com/show_bug.cgi?id=1087104 > > Commit id 'c6212539' explicitly allowed a negative value to be used for > offset and length as a shorthand for the largest value after commit id > 'f18c02ec' modified virStrToLong_ui() to essentially disallow a negative > value. > > However, allowing a negative value for offset ONLY worked if the negative > value was -1 since the eventual lseek() does allow a -1 to mean the end > of the file. Providing other negative values resulted in errors such as: > > $ virsh vol-download --pool default qcow3-vol2 /home/vm-images/raw \ > --offset -2 --length -1000 > error: cannot download from volume qcow3-vol2 > error: Unable to seek /home/vm-images/qcow3-vol2 to 18446744073709551614: > Invalid argument > > $ > > Thus, it seems unreasonable to expect or allow a negative value for offset > since the only benefit is to lseek() to the end of the file and then only > take advantage of how the OS would handle such a seek. For the purposes of > upload or download of volume data, that seems to be a no-op. Therefore, > disallow a negative value for offset. > > Additionally, modify the man page for vol-upload and vol-download to provide > more details regarding the valid values for both offset and length. > > Signed-off-by: John Ferlan > --- > tools/virsh-volume.c | 6 +++--- > tools/virsh.pod | 10 -- > 2 files changed, 11 insertions(+), 5 deletions(-) > ACK, although I'd prefer Eric stating his opinion on this too. Peter signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] virsh: Document bandwidth maximum more clearly
On 07/17/14 01:35, John Ferlan wrote: > Commit id '0e2d7305' modified the code to allow a negative value to be > supplied for the bandwidth argument of the various block virsh commands Technically that commit didn't allow it. It was allowed before that commit kept it in that same state. > and the migrate-setspeed; however, it failed to update the man page to > describe the "feature" whereby a very large value could be interpreted > by the hypervisor to mean maximum value allowed. Although initially > designed to handle a -1 value, the reality is just about any negative > value could be provided and essentially perform the same feature. > > Signed-off-by: John Ferlan > --- > tools/virsh.pod | 23 +++ > 1 file changed, 19 insertions(+), 4 deletions(-) ACK, I like the wording where you specify that the hypervisor may reject that. Peter signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] leaseshelper: add enhancements to support all events
On 07/16/14 17:31, Nehal J Wani wrote: >>> cmd = virCommandNew(dnsmasqCapsGetBinaryPath(caps)); >>> virCommandAddArgFormat(cmd, "--conf-file=%s", configfile); >>> -virCommandAddArgFormat(cmd, "--dhcp-script=%s", leaseshelper_path); >>> +virCommandAddArgFormat(cmd, "--dhcp-script=%s", >>> pseudo_leaseshelper_path); >>> +virCommandAddArgFormat(cmd, "--leasefile-ro"); >> >> Does dnsmasq pass through the rest of the environment we pass to it at >> this point? The leaseshelper extracts quite a lot of stuff from the >> environment variables that are passed by dnsmasq. In case it does we >> could use an env variable to pass the interface name instead of linking >> the helper and using different names. > > If I use the following line of code in the function > networkBuildDhcpDaemonCommandLine ... > setenv("VIR_BRIDGE_NAME", network->def->bridge, 1); > .. then yes, the helper program invoked by dnsmasq does get to see > this variable. (It does so in Fedora20). (Looks like I acted in haste, > should've waited for more replies on RFC, sigh) I wanted to reply to that but I was a bit busy and didn't manage to do it soon enough. We have wrappers for running a separate command with a env variable virCommandAddEnv*... you can use easily for this. > > So, in the next version that I will send, should I first make a > wrapper by the name virSetEnvBlockSUID for setenv, just like we have > virGetEnvBlockSUID for getenv? No, please use the API mentioned above > >> A second issue that comes into my mind is compatibility with already >> existing files. Does this work when you already have a lease file? (I >> didn't try it, I'm just asking). > > If we use the --leasefile-ro option, then although this method will > work, but no, the lease file will *not* be read/written at all. So > even if an old one exists, its of no use. > But then again, the use of --leasefile-ro is mandatory, so that all > events are captured by our helper program. For example, renew of a > lease. My concerns are whether this will work in the case you already used the leases helper as the patch is adding a few fields to the stored format. > > > Regards, > Nehal J Wani > Peter signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] test: metadata: Improve test coverate
On 07/17/14 17:03, Eric Blake wrote: > On 07/17/2014 08:32 AM, Peter Krempa wrote: > > s/coverate/coverage/ in the title > >> Test also the TITLE and DESCRIPTION metadata types. >> --- >> tests/metadatatest.c | 64 >> >> 1 file changed, 64 insertions(+) >> > > ACK - more testing is always good > Fixed && pushed; Thanks. Peter signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] spec: Consolidate with_qemu* definitions
On 07/17/2014 08:22 AM, Jiri Denemark wrote: > Decisions whether qemu driver and libvirt-daemon-{qemu,kvm} packages > should be built on various OS/arch combinations were scattered around > the spec file. Let's make it easier to see where qemu driver is going to > be built. > > Signed-off-by: Jiri Denemark > --- > libvirt.spec.in | 52 +--- > 1 file changed, 25 insertions(+), 27 deletions(-) > > diff --git a/libvirt.spec.in b/libvirt.spec.in > index 9c7b241..472fa4b 100644 > --- a/libvirt.spec.in > +++ b/libvirt.spec.in > @@ -54,15 +54,27 @@ > %define with_vbox 0%{!?_without_vbox:%{server_drivers}} > > %define with_qemu_tcg %{with_qemu} > -# Change if we ever provide qemu-kvm binaries on non-x86 hosts > -%if 0%{?fedora} >= 18 > + > +%define qemu_kvm_arches %{ix86} x86_64 > + > +%if 0%{?fedora} > +%if 0%{?fedora} < 16 > +# Fedora doesn't have any QEMU on ppc64 until FC16 - only ppc > +%ifarch ppc64 > +%define with_qemu_tcg 0 > +%endif > +%endif This may be removed if we come to a conclusion in the other thread about how much we want to support older Fedora. But we can clean it up later, this patch is just moving it. ACK. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] test: metadata: Improve test coverate
On 07/17/2014 08:32 AM, Peter Krempa wrote: s/coverate/coverage/ in the title > Test also the TITLE and DESCRIPTION metadata types. > --- > tests/metadatatest.c | 64 > > 1 file changed, 64 insertions(+) > ACK - more testing is always good -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] test: metadata: Improve test coverate
Test also the TITLE and DESCRIPTION metadata types. --- tests/metadatatest.c | 64 1 file changed, 64 insertions(+) diff --git a/tests/metadatatest.c b/tests/metadatatest.c index 91fc944..a8d8f10 100644 --- a/tests/metadatatest.c +++ b/tests/metadatatest.c @@ -167,6 +167,10 @@ verifyMetadata(virDomainPtr dom, struct metadataTest { virConnectPtr conn; virDomainPtr dom; + +const char *data; +int type; +bool fail; }; @@ -216,6 +220,52 @@ testEraseMetadata(const void *data) } static int +testTextMetadata(const void *data) +{ +const struct metadataTest *test = data; +char *actual = NULL; +int ret = -1; + +if (virDomainSetMetadata(test->dom, test->type, test->data, NULL, NULL, 0) < 0) { +if (test->fail) +return 0; +return -1; +} + +actual = virDomainGetMetadata(test->dom, test->type, NULL, 0); + +if (STRNEQ_NULLABLE(test->data, actual)) { +virReportError(VIR_ERR_INTERNAL_ERROR, + "expected metadata doesn't match actual: " + "expected:'%s'\ngot: '%s'", + NULLSTR(test->data), NULLSTR(actual)); +goto cleanup; +} + +ret = 0; + + cleanup: +VIR_FREE(actual); + +return ret; +} + +#define TEST_TEXT_METADATA(INDEX, TYPE, DATA, FAIL) \ +do {\ +test.type = VIR_DOMAIN_METADATA_ ## TYPE; \ +test.data = DATA; \ +test.fail = FAIL; \ +\ +if (virtTestRun("text metadata: " #TYPE " " INDEX " ", \ +testTextMetadata, &test) < 0) \ +ret = EXIT_FAILURE; \ +} while (0) + +#define TEST_TITLE(INDEX, DATA) TEST_TEXT_METADATA(INDEX, TITLE, DATA, false) +#define TEST_TITLE_FAIL(INDEX, DATA) TEST_TEXT_METADATA(INDEX, TITLE, DATA, true) +#define TEST_DESCR(INDEX, DATA) TEST_TEXT_METADATA(INDEX, DESCRIPTION, DATA, false) + +static int mymain(void) { struct metadataTest test; @@ -238,6 +288,20 @@ mymain(void) if (virtTestRun("Erase metadata ", testEraseMetadata, &test) < 0) ret = EXIT_FAILURE; +TEST_TITLE("1", "qwert"); +TEST_TITLE("2", NULL); +TEST_TITLE("3", "blah"); +TEST_TITLE_FAIL("4", "qwe\nrt"); +TEST_TITLE("5", ""); +TEST_TITLE_FAIL("6", "qwert\n"); +TEST_TITLE_FAIL("7", "\n"); + +TEST_DESCR("1", "qwert\nqwert"); +TEST_DESCR("2", NULL); +TEST_DESCR("3", "qwert"); +TEST_DESCR("4", "\n"); +TEST_DESCR("5", ""); + virDomainFree(test.dom); virConnectClose(test.conn); -- 2.0.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] virsh capabilities vs. domcapabilities
On 07/17/2014 03:05 AM, Michal Privoznik wrote: >> Furthermore, I'm trying to figure out how to advertise whether a given >> domain will support active commit (and similarly, Peter's patches for >> relative backing name preservation). Advertising the feature just >> through 'virsh capabilities' is insufficient, because that only covers >> the default binary; so it seems like the sort of thing that should also >> be in 'virsh domcapabilities'. > > That depends on how's active commit accepted by libvirt. IIUC it's a > flag to an API. (Okay, you got me there, I'm not paying much attention > to snapshot work). You use a flag to trigger it for now (although there is a proposal to eventually allow active commit without a flag); but the main point is that people want to know up front if the flag even stands a chance of working, which depends on the abilities of the qemu binary. > > The best solution would be to introduce another section, where supported > flags to APIs would be enumerated (same way that attribute values are). > But this is sooo much more work than in attribute part (esp. without > introspection) that the resulting code would be unmaintainable. > A list of supported flags may be too hard, compared to just a list of feature names. >> I'm also finding 'virsh domcapabilities' a bit hard to use - even though >> it allows all its arguments to be optional at the RPC level, the qemu >> implementation isn't so happy: >> >> # tools/virsh domcapabilities >> error: failed to get emulator capabilities >> error: virttype_str in qemuConnectGetDomainCapabilities must not be NULL >> >> but how am I supposed to know what --virttype strings are valid? > > By reading the documentation :P > From the virsh manpage: > > The virttype option specifies the virtualization type used. The value to > be used is either from the 'type' attribute of the top level > element from the domain XML or the 'type' attribute found within each > element from the virsh capabilities output. You know, I can think of a couple of UI additions that would make this command really cool: - virsh domcapabilities --domain $dom uses the same name, uuid, or id as all other domain lookups, then calls dumpxml on that domain, scrapes the XML, and uses the parameters it found to pass to the virConnectGetDomainCapabilities call - virsh domcapabilities --xml $file takes $file which contains either or XML (no ambiguity, because the top level element says which style), scrapes it for the right parameters, and passes those parameters (or NULL for missing parameters) to virConnectGetDomainCapabilities Remember, it is possible to define a with the user input XML not listing an , where a later dumpxml will show the default emulator that got chosen. So the same idea applies to the virsh command - if the user supplies a XML file without an explicit , then we use NULL for that parameter to virConnectGetDomainCapabilities, and we should get results for the default emulator that will be used to run that domain. Also, by allowing XML (as a subset of overall 'virsh capabilities') as the input, it is possible to easily transition from the old API (tell me what emulator(s) will be used) to the new (tell me what features/drivers I can use with that emulator). > > I know virsh user is user unfriendly, but I think this could be solved > by wise auto completion (if I find another student to complete it). Or > if you have any idea meanwhile ... There's a pending patch series (now several months old) that improves some auto completion; I need to find time to review and revive it. >> >> /guest/arch/emulator vs. /domainCapabilities/path - why 3 levels vs. 2, >> and different naming? > > This is something provided by callee, so in fact it has no additional > value. Not true. If you take my argument above about allowing NULL emulator to mean "tell me the default emulator and its capabilities", then this output parameter is essential in that case. > >> >> /guest/arch/machine@maxCpus vs. /domainCapabilities/vcpu@max - why 3 >> levels vs. 2, with different naming? > > Because we may want to extend fom domaincaps in the future. Okay, so we are intentionally breaking design from the old XML, and clients cannot share XPath code between the old style and new style. I can live with that; although it might be worth some documentation on the old API how to find the same information in the new. Maybe as simple as just documenting all the mappings that I already identified in this mail. I know - patches welcome :) -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] spec: Consolidate with_qemu* definitions
Decisions whether qemu driver and libvirt-daemon-{qemu,kvm} packages should be built on various OS/arch combinations were scattered around the spec file. Let's make it easier to see where qemu driver is going to be built. Signed-off-by: Jiri Denemark --- libvirt.spec.in | 52 +--- 1 file changed, 25 insertions(+), 27 deletions(-) diff --git a/libvirt.spec.in b/libvirt.spec.in index 9c7b241..472fa4b 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -54,15 +54,27 @@ %define with_vbox 0%{!?_without_vbox:%{server_drivers}} %define with_qemu_tcg %{with_qemu} -# Change if we ever provide qemu-kvm binaries on non-x86 hosts -%if 0%{?fedora} >= 18 + +%define qemu_kvm_arches %{ix86} x86_64 + +%if 0%{?fedora} +%if 0%{?fedora} < 16 +# Fedora doesn't have any QEMU on ppc64 until FC16 - only ppc +%ifarch ppc64 +%define with_qemu_tcg 0 +%endif +%endif +%if 0%{?fedora} >= 18 +%define qemu_kvm_arches %{ix86} x86_64 ppc64 s390x +%endif %if 0%{?fedora} >= 20 -%define qemu_kvm_arches%{ix86} x86_64 ppc64 s390x %{arm} -%else -%define qemu_kvm_arches%{ix86} x86_64 ppc64 s390x +%define qemu_kvm_arches %{ix86} x86_64 ppc64 s390x %{arm} %endif -%else -%define qemu_kvm_arches%{ix86} x86_64 +%endif + +%if 0%{?rhel} +%define with_qemu_tcg 0 +%define qemu_kvm_arches x86_64 %endif %ifarch %{qemu_kvm_arches} @@ -71,6 +83,10 @@ %define with_qemu_kvm 0 %endif +%if ! %{with_qemu_tcg} && ! %{with_qemu_kvm} +%define with_qemu 0 +%endif + # Then the hypervisor drivers that run outside libvirtd, in libvirt.so %define with_openvz0%{!?_without_openvz:1} %define with_vmware0%{!?_without_vmware:1} @@ -191,34 +207,16 @@ %define with_firewalld 1 %endif -# RHEL-5 has restricted QEMU to x86_64 only and is too old for LXC +# RHEL-5 is too old for LXC %if 0%{?rhel} == 5 -%define with_qemu_tcg 0 -%ifnarch x86_64 -%define with_qemu 0 -%define with_qemu_kvm 0 -%endif %define with_lxc 0 %endif -# RHEL-6 has restricted QEMU to x86_64 only, stopped including Xen -# on all archs. Other archs all have LXC available though +# RHEL-6 stopped including Xen on all archs. %if 0%{?rhel} >= 6 -%define with_qemu_tcg 0 -%ifnarch x86_64 -%define with_qemu 0 -%define with_qemu_kvm 0 -%endif %define with_xen 0 %endif -# Fedora doesn't have any QEMU on ppc64 until FC16 - only ppc -%if 0%{?fedora} && 0%{?fedora} < 16 -%ifarch ppc64 -%define with_qemu 0 -%endif -%endif - # Fedora doesn't have new enough Xen for libxl until F18 %if 0%{?fedora} && 0%{?fedora} < 18 %define with_libxl 0 -- 2.0.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Log an error when we fail to set the COW attribute
On 07/17/2014 01:54 PM, John Ferlan wrote: > > > On 07/17/2014 06:22 AM, Ján Tomko wrote: >> Coverity complains about the return value of ioctl not being checked. >> >> Even though we carry on when this fails (just like qemu-img does), >> we can log an error. >> --- >> src/storage/storage_backend.c | 9 +++-- >> 1 file changed, 7 insertions(+), 2 deletions(-) >> >> diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c >> index 5e7aa3c..b8b89ca 100644 >> --- a/src/storage/storage_backend.c >> +++ b/src/storage/storage_backend.c >> @@ -464,9 +464,14 @@ virStorageBackendCreateRaw(virConnectPtr conn >> ATTRIBUTE_UNUSED, >> * The FS_IOC_SETFLAGS ioctl return value will be ignored since any >> * failure of this operation should not block the left work. >> */ >> -if (ioctl(fd, FS_IOC_GETFLAGS, &attr) == 0) { >> +if (ioctl(fd, FS_IOC_GETFLAGS, &attr) < 0) { >> +virReportSystemError(errno, "%s", _("Failed to get fs flags")); >> +} else { >> attr |= FS_NOCOW_FL; >> -ioctl(fd, FS_IOC_SETFLAGS, &attr); >> +if (ioctl(fd, FS_IOC_SETFLAGS, &attr) < 0) { >> +virReportSystemError(errno, "%s", >> + _("Failed to set NOCOW flag")); >> +} >> } >> #endif >> } >> > > Looks like you were already looking at the failure when my Coverity scan > ran... Should have checked if there already was a patch before sending > my response in Chunyan Liu's original series. > > In any case, seems better to me to at least log the error whether or not > the code wants to "block the left work" (any chance to clean that > comment up a bit :-) - hopefully there's not too much "right work" left > either). > > ACK, > I've changed "left work" to "volume creation" and pushed the patch. Jan signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] examples: Introduce domtop
On 07/16/2014 03:53 PM, Michal Privoznik wrote: > There's this question on the list that is asked over and over again. > How do I get {cpu, memory, ...} usage in percentage? Or its modified > version: How do I plot nice graphs like virt-manager does? > > It would be nice if we have an example to inspire people. And that's > what domtop should do. Yes, it could be written in different ways, but > I've chosen this one as I think it show explicitly what users need to > implement in order to imitate virt-manager's graphing. > > Signed-off-by: Michal Privoznik > --- > .gitignore | 1 + > Makefile.am | 2 +- > cfg.mk | 2 +- > configure.ac| 1 + > examples/domtop/Makefile.am | 27 +++ > examples/domtop/domtop.c| 388 > > libvirt.spec.in | 2 +- > 7 files changed, 420 insertions(+), 3 deletions(-) > create mode 100644 examples/domtop/Makefile.am > create mode 100644 examples/domtop/domtop.c > + > +static void > +print_cpu_usage(const char *dom_name, > +size_t cpu, > +size_t ncpus, > +unsigned long long then, > +virTypedParameterPtr then_params, > +size_t then_nparams, > +unsigned long long now, > +virTypedParameterPtr now_params, > +size_t now_nparams) > +{ > +size_t i, j, k; > +size_t nparams = now_nparams; > + > +if (then_nparams != now_nparams) { > +/* this should not happen (TM) */ > +ERROR("parameters counts don't match"); > +return; > +} > + > +for (i = 0; i < ncpus; i++) { > +size_t pos; > +double usage; > + > +/* check if the vCPU is in the maps */ > +if (now_params[i * nparams].type == 0 || > +then_params[i * then_nparams].type == 0) > +continue; > + > +for (j = 0; j < nparams; j++) { > +pos = i * nparams + j; > +if (STREQ(then_params[pos].field, VIR_DOMAIN_CPU_STATS_CPUTIME) > || > +STREQ(then_params[pos].field, VIR_DOMAIN_CPU_STATS_VCPUTIME)) > +break; > +} > + > +if (j == nparams) { > +ERROR("unable to find %s", VIR_DOMAIN_CPU_STATS_CPUTIME); > +return; > +} > + > +DEBUG("now_params=%llu then_params=%llu now=%llu then=%llu", > + now_params[pos].value.ul, then_params[pos].value.ul, now, > then); > + > +/* @now_params and @then_params are in nanoseconds, @now and @then > are > + * in microseconds. In ideal world, we would translate them both into > + * the same scale, divide one by another and multiply by factor of > 100 > + * to get percentage. However, the count of floating point operations > + * performed has a bad affect on the precision, so instead of > dividing s/affect/effect/ > + * @now_params and @then_params by 1000 and then multiplying again by > + * 100, we divide only once by 10 and get the same result. */ > +usage = (now_params[pos].value.ul - then_params[pos].value.ul) / > +(now - then) / 10; > + > +printf("CPU%zu: %.2lf\n", cpu + i, usage); I think printing all the CPUs on one line would look nicer, and it would be easier to see which numbers are changing. ACK Jan signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Log an error when we fail to set the COW attribute
On 07/17/2014 06:22 AM, Ján Tomko wrote: > Coverity complains about the return value of ioctl not being checked. > > Even though we carry on when this fails (just like qemu-img does), > we can log an error. > --- > src/storage/storage_backend.c | 9 +++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c > index 5e7aa3c..b8b89ca 100644 > --- a/src/storage/storage_backend.c > +++ b/src/storage/storage_backend.c > @@ -464,9 +464,14 @@ virStorageBackendCreateRaw(virConnectPtr conn > ATTRIBUTE_UNUSED, > * The FS_IOC_SETFLAGS ioctl return value will be ignored since any > * failure of this operation should not block the left work. > */ > -if (ioctl(fd, FS_IOC_GETFLAGS, &attr) == 0) { > +if (ioctl(fd, FS_IOC_GETFLAGS, &attr) < 0) { > +virReportSystemError(errno, "%s", _("Failed to get fs flags")); > +} else { > attr |= FS_NOCOW_FL; > -ioctl(fd, FS_IOC_SETFLAGS, &attr); > +if (ioctl(fd, FS_IOC_SETFLAGS, &attr) < 0) { > +virReportSystemError(errno, "%s", > + _("Failed to set NOCOW flag")); > +} > } > #endif > } > Looks like you were already looking at the failure when my Coverity scan ran... Should have checked if there already was a patch before sending my response in Chunyan Liu's original series. In any case, seems better to me to at least log the error whether or not the code wants to "block the left work" (any chance to clean that comment up a bit :-) - hopefully there's not too much "right work" left either). ACK, John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH V3 1/2] storagevol: add nocow to vol xml
Coverity flagged this commit see below... On 07/15/2014 04:49 AM, Chunyan Liu wrote: > Add 'nocow' to storage volume xml so that user can have an option > to set NOCOW flag to the newly created volume. It's useful on btrfs > file system to enhance performance. > > Btrfs has low performance when hosting VM images, even more when the guest > in those VM are also using btrfs as file system. One way to mitigate this > bad performance is to turn off COW attributes on VM files. Generally, there > are two ways to turn off COW on btrfs: a) by mounting fs with nodatacow, > then all newly created files will be NOCOW. b) per file. Add the NOCOW file > attribute. It could only be done to empty or new files. > > This patch tries the second way, according to 'nocow' option, it could set > NOCOW flag per file: > for raw file images, handle 'nocow' in libvirt code; for non-raw file images, > pass 'nocow=on' option to qemu-img, and let qemu-img to handle that (requires > qemu-img version >= 2.1). > > Signed-off-by: Chunyan Liu > --- > Changes: > * fix typo > > docs/formatstorage.html.in| 7 +++ > docs/schemas/storagevol.rng | 5 + > src/conf/storage_conf.c | 3 +++ > src/storage/storage_backend.c | 22 ++ > src/util/virstoragefile.h | 1 + > 5 files changed, 38 insertions(+) > > diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in > index 1cd82b4..8d51402 100644 > --- a/docs/formatstorage.html.in > +++ b/docs/formatstorage.html.in > @@ -385,6 +385,7 @@ > > >1.1 > +> > > @@ -424,6 +425,12 @@ > 1.1 is used. If omitted, qemu-img default is used. > Since 1.1.0 > > + nocow > + Turn off COW of the newly created volume. So far, this is only > valid > +for a file image in btrfs file system. It will improve performance > when > +the file image is used in VM. To create non-raw file images, it > +requires QEMU version since 2.1. Since > 1.2.6 > + >features >Format-specific features. Only used for qcow2 now. > Valid sub-elements are: > diff --git a/docs/schemas/storagevol.rng b/docs/schemas/storagevol.rng > index 3798476..1b2d4cc 100644 > --- a/docs/schemas/storagevol.rng > +++ b/docs/schemas/storagevol.rng > @@ -138,6 +138,11 @@ > > > > + > + > + > + > + > > > > diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c > index aa29658..f28a314 100644 > --- a/src/conf/storage_conf.c > +++ b/src/conf/storage_conf.c > @@ -1285,6 +1285,9 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, > virStringFreeList(version); > } > > +if (virXPathNode("./target/nocow", ctxt)) > +ret->target.nocow = true; > + > if (options->featureFromString && virXPathNode("./target/features", > ctxt)) { > if ((n = virXPathNodeSet("./target/features/*", ctxt, &nodes)) < 0) > goto error; > diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c > index 7b17ca4..5f2dc5d 100644 > --- a/src/storage/storage_backend.c > +++ b/src/storage/storage_backend.c > @@ -37,6 +37,9 @@ > #ifdef __linux__ > # include > # include > +# ifndef FS_NOCOW_FL > +# define FS_NOCOW_FL 0x0080 /* Do not cow file */ > +# endif > #endif > > #if WITH_SELINUX > @@ -453,6 +456,21 @@ virStorageBackendCreateRaw(virConnectPtr conn > ATTRIBUTE_UNUSED, > goto cleanup; > } > > +if (vol->target.nocow) { > +#ifdef __linux__ > +int attr; > + > +/* Set NOCOW flag. This is an optimisation for btrfs. > + * The FS_IOC_SETFLAGS ioctl return value will be ignored since any > + * failure of this operation should not block the left work. > + */ > +if (ioctl(fd, FS_IOC_GETFLAGS, &attr) == 0) { > +attr |= FS_NOCOW_FL; > +ioctl(fd, FS_IOC_SETFLAGS, &attr); ^^^ Status is not checked here causing complaint about CHECKED_RETURN John > +} > +#endif > +} > + > if ((ret = createRawFile(fd, vol, inputvol)) < 0) > /* createRawFile already reported the exact error. */ > ret = -1; > @@ -718,6 +736,7 @@ virStorageBackendCreateQemuImgOpts(char **opts, > bool preallocate, > int format, > const char *compat, > + bool nocow, > virBitmapPtr features) > { > virBuffer buf = VIR_BUFFER_INITIALIZER; > @@ -730,6 +749,8 @@ virStorageBackendCreateQemuImgOpts(char **opts, > virBufferAddLit(&buf, "encryption=on,"); > if>
[libvirt] [PATCH] Log an error when we fail to set the COW attribute
Coverity complains about the return value of ioctl not being checked. Even though we carry on when this fails (just like qemu-img does), we can log an error. --- src/storage/storage_backend.c | 9 +++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 5e7aa3c..b8b89ca 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -464,9 +464,14 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED, * The FS_IOC_SETFLAGS ioctl return value will be ignored since any * failure of this operation should not block the left work. */ -if (ioctl(fd, FS_IOC_GETFLAGS, &attr) == 0) { +if (ioctl(fd, FS_IOC_GETFLAGS, &attr) < 0) { +virReportSystemError(errno, "%s", _("Failed to get fs flags")); +} else { attr |= FS_NOCOW_FL; -ioctl(fd, FS_IOC_SETFLAGS, &attr); +if (ioctl(fd, FS_IOC_SETFLAGS, &attr) < 0) { +virReportSystemError(errno, "%s", + _("Failed to set NOCOW flag")); +} } #endif } -- 1.8.5.5 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC] require for suggestions on support for ivshmem device
On Tue, May 20, 2014 at 11:17:32AM +0200, Martin Kletzander wrote: On Wed, May 14, 2014 at 08:23:21AM +, Wangrui (K) wrote: Hi, Libvirt does not support ivshmem(Inter-VM Shared Memory) device recently, thus, I would like to know if there's any plan to support it in the future? If not, I would like to contribute a serial of patches to do so. I came back to this mail right now because I need to have this implemented. Is there any progress on your side with this or should I try hitting this? [...] There are two ways to use ivshmem with qemu (please refer to http://qemu.weilnetz.de/qemu-doc.html#pcsys_005fother_005fdevs ): 1.Guests map a POSIX shared memory region into the guest as a PCI device that enables zero-copy communication to the application level of the guests, The basic syntax is: qemu-system-i386-device ivshmem, size = [, shm = ] 2.If desired, interrupts can be sent between guest VMs accessing the same shared memory region. Interrupt support requires using a shared memory server and using a chardev socket to connect to it. An example syntax when using the shared memory server is: qemu-system-i386-device ivshmem, size = [, chardev = ] [, msi = on] [, ioeventfd = on] [, vectors = n] [, role = peer | master] qemu-system-i386-chardev socket, path = , id = The respective xml configuration for the above 2 qemu command lines are shown as below: Example1: automatically attach device with KVM NOTE: "size" means ivshmem size in unit MB, "name" means shm name "role" is optional, it may be set to "master" or "peer", the default one is "master" What do these roles mean, I mean what's the difference between master and peer and why is it only used with the chardev? Does it mean master can only send interrupts or...? Just curious. @Cam (Cc'd) I was wondering about the role= options, so I looked into the code. It looks like role=peer just effectively disables migration. Did I miss any other difference? From the libvirt's POV I'd have a few more questions if I may. How does the migration work (if there's role=master) WRT other guests using the same shm? I found no shm_unlink call in QEMU sources (but again, I'm not experienced in QEMU's internals), does that mean that cleanup should be done by libvirt? Thank you for any info provided. Martin signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] libxl: Implement basic video device selection
On 16.07.2014 23:05, Jim Fehlig wrote: > Stefan Bader wrote: >> being as bad with timely responses. Ok, so how about the following? >> >> One note: it could be the STRDUP's are not strictly needed. But >> to me it felt wrong to have two places refer to the same strings >> (as MakeVFB copies the struct containing the pointers). > > Agreed. Without the STRDUP's, seems there is a potential for double > free when libxl_device_vfb and libxl_domain_config objects are disposed. > >> If this >> is not needed, then all changes now in MakeVFB probably can be >> dropped (except setting the keyboard layout, maybe; which I >> might miss ;)). >> >> -Stefan >> >> >> >From a95db265fa4c1a231e7c2d70baa360c6a0500e3b Mon Sep 17 00:00:00 2001 >> From: Stefan Bader >> Date: Thu, 27 Mar 2014 16:01:18 +0100 >> Subject: [PATCH] libxl: Implement basic video device selection >> >> This started as an investigation into an issue where libvirt (using the >> libxl driver) and the Xen host, like an old couple, could not agree on >> who is responsible for selecting the VNC port to use. >> >> Things usually (and a bit surprisingly) did work because, just like that >> old couple, they had the same idea on what to do by default. However it >> was possible that this ended up in a big argument. >> >> The problem is that display information exists in two different places: >> in the vfbs list and in the build info. And for launching the device model, >> only the latter is used. But that never gets initialized from libvirt. So >> Xen allows the device model to select a default port while libvirt thinks >> it has told Xen that this is done by libvirt (though the vfbs config). >> >> While fixing that, I made a stab at actually evaluating the configuration >> of the video device. So that it is now possible to at least decide between >> a Cirrus or standard VGA emulation and to modify the VRAM within certain >> limits using libvirt. >> >> [v2: Check return code of VIR_STRDUP and fix indentation] >> [v3: Split out VRAM fixup and return error for unsupported video type] >> [v4: Re-arrange code and move VFB setup into libxlMakeVfbList] >> > > [meta-comment] > libvirt prefers patch version history like this to be below the '---' > following your Signed-off-by, so as to not pollute the commit message. Ah yeah, makes sense. I try to keep it in mind. > >> Signed-off-by: Stefan Bader >> --- >> src/libxl/libxl_conf.c | 63 >> ++-- >> 1 file changed, 61 insertions(+), 2 deletions(-) >> >> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c >> index 8eeaf82..43cabcf 100644 >> --- a/src/libxl/libxl_conf.c >> +++ b/src/libxl/libxl_conf.c >> @@ -1098,10 +1098,21 @@ libxlMakeVfbList(virPortAllocatorPtr graphicsports, >> libxl_domain_build_info *b_info = &d_config->b_info; >> libxl_device_vfb vfb = d_config->vfbs[0]; >> >> -if (libxl_defbool_val(vfb.vnc.enable)) >> +if (libxl_defbool_val(vfb.vnc.enable)) { >> memcpy(&b_info->u.hvm.vnc, &vfb.vnc, sizeof(libxl_vnc_info)); >> -else if (libxl_defbool_val(vfb.sdl.enable)) >> +if (VIR_STRDUP(b_info->u.hvm.vnc.listen, vfb.vnc.listen) < 0) >> +goto error; >> +if (VIR_STRDUP(b_info->u.hvm.vnc.passwd, vfb.vnc.passwd) < 0) >> +goto error; >> +} else if (libxl_defbool_val(vfb.sdl.enable)) { >> memcpy(&b_info->u.hvm.sdl, &vfb.sdl, sizeof(libxl_sdl_info)); >> +if (VIR_STRDUP(b_info->u.hvm.sdl.display, vfb.sdl.display) < 0) >> +goto error; >> +if (VIR_STRDUP(b_info->u.hvm.sdl.xauthority, >> vfb.sdl.xauthority) < 0) >> +goto error; >> +} >> +if (VIR_STRDUP(b_info->u.hvm.keymap, vfb.keymap) < 0) >> +goto error; >> } >> >> return 0; >> @@ -1363,6 +1374,45 @@ libxlMakeCapabilities(libxl_ctx *ctx) >> return NULL; >> } >> >> +static int >> +libxlMakeVideo(virDomainDefPtr def, libxl_domain_config *d_config) >> +{ >> +libxl_domain_build_info *b_info = &d_config->b_info; >> + >> +if (d_config->c_info.type != LIBXL_DOMAIN_TYPE_HVM) >> +return 0; >> + >> +/* >> + * Take the first defined video device (graphics card) to display >> + * on the first graphics device (display). >> + * Right now only type and vram info is used and anything beside >> + * type xen and vga is mapped to cirrus. >> + */ >> +if (def->nvideos) { >> +switch (def->videos[0]->type) { >> +case VIR_DOMAIN_VIDEO_TYPE_VGA: >> +case VIR_DOMAIN_VIDEO_TYPE_XEN: >> +b_info->u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_STD; >> +break; >> +case VIR_DOMAIN_VIDEO_TYPE_CIRRUS: >> +b_info->u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_CIRRUS; >> +break; >> +default: >> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, >
[libvirt] [PATCH] LXC: show used memory as 0 when domain is not active
Before: virsh # dominfo chx3 State: shut off Max memory: 92160 KiB Used memory:92160 KiB After: virsh # dominfo container1 State: shut off Max memory: 92160 KiB Used memory:0 KiB Similar to qemu cases. Signed-off-by: Chen Hanxiao --- src/lxc/lxc_driver.c | 2 +- src/qemu/qemu_driver.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index b7b4b02..f094f86 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -584,7 +584,7 @@ static int lxcDomainGetInfo(virDomainPtr dom, if (!virDomainObjIsActive(vm)) { info->cpuTime = 0; -info->memory = vm->def->mem.cur_balloon; +info->memory = 0; } else { if (virCgroupGetCpuacctUsage(priv->cgroup, &(info->cpuTime)) < 0) { virReportError(VIR_ERR_OPERATION_FAILED, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 33541d3..01107cf 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2534,7 +2534,7 @@ static int qemuDomainGetInfo(virDomainPtr dom, info->memory = vm->def->mem.cur_balloon; } } else { -info->memory = vm->def->mem.cur_balloon; +info->memory = 0; } info->nrVirtCpu = vm->def->vcpus; -- 1.9.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemuConnectGetDomainCapabilities: Use wiser defaults
On Thu, Jul 17, 2014 at 11:12:05AM +0200, Michal Privoznik wrote: > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index 33541d3..7d99435 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -16849,17 +16849,17 @@ qemuConnectGetDomainCapabilities(virConnectPtr conn, > char *ret = NULL; > virQEMUDriverPtr driver = conn->privateData; > virQEMUCapsPtr qemuCaps = NULL; > -int virttype; /* virDomainVirtType */ > +int virttype = VIR_DOMAIN_VIRT_KVM; /* virDomainVirtType */ Isn't this going to always fail when we run inside a guest which does not have nested virt? When we know the QEMU binary, we probe to see whether it can do KVM or not, so could we postpone this decision about virt type until after we've checked if KVM was available ? Regards, 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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] virsh capabilities vs. domcapabilities
On 17.07.2014 11:11, Daniel P. Berrange wrote: On Thu, Jul 17, 2014 at 11:05:08AM +0200, Michal Privoznik wrote: # tools/virsh domcapabilities --virttype kvm error: failed to get emulator capabilities error: invalid argument: at least one of emulatorbin or architecture fields must be present Would it be nicer to behave the same as 'virsh capabilities' and give the details of the default binary in this case? Sure, but in order to get default binary we must know architecture (consider the case where you have both /usr/bin/qemu-system-x86_64 and /usr/bin/qemu-system-i686). Although, having only one qemu binary on the system makes it easy to find the default, doesn't it? Patch on the way. IMHO it would be preferrable to always default to virArchFromHost() if arch is none, since that gives a predictable default value, as opposed to probing emulators which is unpredictable Yep, that's exactly what I've done in my patch: https://www.redhat.com/archives/libvir-list/2014-July/msg00884.html Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] qemuConnectGetDomainCapabilities: Use wiser defaults
Up to now, users have to pass two arguments at least: domain virt type ('qemu' vs 'kvm') and one of emulatorbin or architecture. This is not much user friendly. Nowadays users mostly use KVM and share the host architecture with the guest. So now, the API (and subsequently virsh command) can be called with all NULLs (without any arguments). Before this patch: # virsh domcapabilities error: failed to get emulator capabilities error: virttype_str in qemuConnectGetDomainCapabilities must not be NULL # virsh domcapabilities kvm error: failed to get emulator capabilities error: invalid argument: at least one of emulatorbin or architecture fields must be present After: # virsh domcapabilities /usr/bin/qemu-system-x86_64 kvm pc-i440fx-2.1 x86_64 Signed-off-by: Michal Privoznik --- src/qemu/qemu_driver.c | 24 ++-- 1 file changed, 6 insertions(+), 18 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 33541d3..7d99435 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16849,17 +16849,17 @@ qemuConnectGetDomainCapabilities(virConnectPtr conn, char *ret = NULL; virQEMUDriverPtr driver = conn->privateData; virQEMUCapsPtr qemuCaps = NULL; -int virttype; /* virDomainVirtType */ +int virttype = VIR_DOMAIN_VIRT_KVM; /* virDomainVirtType */ virDomainCapsPtr domCaps = NULL; -int arch = VIR_ARCH_NONE; /* virArch */ +int arch = virArchFromHost(); /* virArch */ virCheckFlags(0, ret); -virCheckNonNullArgReturn(virttype_str, ret); if (virConnectGetDomainCapabilitiesEnsureACL(conn) < 0) return ret; -if ((virttype = virDomainVirtTypeFromString(virttype_str)) < 0) { +if (virttype_str && +(virttype = virDomainVirtTypeFromString(virttype_str)) < 0) { virReportError(VIR_ERR_INVALID_ARG, _("unknown virttype: %s"), virttype_str); @@ -16882,9 +16882,6 @@ qemuConnectGetDomainCapabilities(virConnectPtr conn, arch_from_caps = virQEMUCapsGetArch(qemuCaps); -if (arch == VIR_ARCH_NONE) -arch = arch_from_caps; - if (arch_from_caps != arch) { virReportError(VIR_ERR_INVALID_ARG, _("architecture from emulator '%s' doesn't " @@ -16893,21 +16890,12 @@ qemuConnectGetDomainCapabilities(virConnectPtr conn, virArchToString(arch)); goto cleanup; } -} else if (arch_str) { +} else { if (!(qemuCaps = virQEMUCapsCacheLookupByArch(driver->qemuCapsCache, arch))) goto cleanup; -if (!emulatorbin) -emulatorbin = virQEMUCapsGetBinary(qemuCaps); -/* Deliberately not checking if provided @emulatorbin matches @arch, - * since if @emulatorbin was specified the match has been checked a few - * lines above. */ -} else { -virReportError(VIR_ERR_INVALID_ARG, "%s", - _("at least one of emulatorbin or " - "architecture fields must be present")); -goto cleanup; +emulatorbin = virQEMUCapsGetBinary(qemuCaps); } if (machine) { -- 1.8.5.5 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] virsh capabilities vs. domcapabilities
On Thu, Jul 17, 2014 at 11:05:08AM +0200, Michal Privoznik wrote: > ># tools/virsh domcapabilities --virttype kvm > >error: failed to get emulator capabilities > >error: invalid argument: at least one of emulatorbin or architecture > >fields must be present > > > >Would it be nicer to behave the same as 'virsh capabilities' and give > >the details of the default binary in this case? > > Sure, but in order to get default binary we must know architecture (consider > the case where you have both /usr/bin/qemu-system-x86_64 and > /usr/bin/qemu-system-i686). Although, having only one qemu binary on the > system makes it easy to find the default, doesn't it? Patch on the way. IMHO it would be preferrable to always default to virArchFromHost() if arch is none, since that gives a predictable default value, as opposed to probing emulators which is unpredictable Regards, 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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 4/8] daemon: support passing FDs from the calling process
On Thu, Jul 17, 2014 at 09:40:12AM +0100, Daniel P. Berrange wrote: On Wed, Jul 16, 2014 at 08:29:58PM +0200, Martin Kletzander wrote: First FD is the RW unix socket to listen on, second one (if applicable) is the RO unix socket. Signed-off-by: Martin Kletzander --- daemon/libvirtd.c | 45 +++-- 1 file changed, 27 insertions(+), 18 deletions(-) diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 4c926b3..d20aeae 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -56,6 +56,7 @@ #include "virstring.h" #include "locking/lock_manager.h" #include "viraccessmanager.h" +#include "virutil.h" #ifdef WITH_DRIVER_MODULES # include "driver.h" @@ -476,11 +477,19 @@ static int daemonSetupNetworking(virNetServerPtr srv, int unix_sock_ro_mask = 0; int unix_sock_rw_mask = 0; +unsigned int cur_fd = STDIN_FILENO + 1; Shouldn't that be STDERR not STDIN since the passed FDs start at 3 not 1. Oh, yes, I tried that with the value "3" and changed it right before sending, my bad. +unsigned int nfds = virGetListenFDs(); + Regards, 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 :| signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] virsh capabilities vs. domcapabilities
On 16.07.2014 21:00, Eric Blake wrote: We have some inconsistencies in the node capabilities (which shows guest capabilities for some default binaries) and domcapabilities (which shows guest capabilities for a specified binary). It might be nicer for client applications if the two XML components share a common schema and output layout, so that the same client code can be used to parse either (sub-tree of) XML, modulo the name of the top-most element containing the tree. Furthermore, I'm trying to figure out how to advertise whether a given domain will support active commit (and similarly, Peter's patches for relative backing name preservation). Advertising the feature just through 'virsh capabilities' is insufficient, because that only covers the default binary; so it seems like the sort of thing that should also be in 'virsh domcapabilities'. That depends on how's active commit accepted by libvirt. IIUC it's a flag to an API. (Okay, you got me there, I'm not paying much attention to snapshot work). The best solution would be to introduce another section, where supported flags to APIs would be enumerated (same way that attribute values are). But this is sooo much more work than in attribute part (esp. without introspection) that the resulting code would be unmaintainable. So my suggestion is to come up with yet another section and put arbitrary strings there to represent features like active commit. For instance: ... active-commit {some ordinary commit} Since virConnectGetDomainCapabilities is brand new to 1.2.7, we still have time to change its XML. But I want consensus on whether we need things to match, or whether we intentionally want people to rely only on the newer XML format of the new API call (that is, don't bloat 'virsh capabilities'/virConnectGetCapabilities any further, and learning whether active commit is supported will have to be done via 'virsh domcapabilities'/virConnectGetDomainCapabilities). That is, I'm wondering if should use / as its starting point, rather than completely inventing new XML. I'm also finding 'virsh domcapabilities' a bit hard to use - even though it allows all its arguments to be optional at the RPC level, the qemu implementation isn't so happy: # tools/virsh domcapabilities error: failed to get emulator capabilities error: virttype_str in qemuConnectGetDomainCapabilities must not be NULL but how am I supposed to know what --virttype strings are valid? By reading the documentation :P From the virsh manpage: The virttype option specifies the virtualization type used. The value to be used is either from the 'type' attribute of the top level element from the domain XML or the 'type' attribute found within each element from the virsh capabilities output. I know virsh user is user unfriendly, but I think this could be solved by wise auto completion (if I find another student to complete it). Or if you have any idea meanwhile ... # tools/virsh domcapabilities --virttype kvm error: failed to get emulator capabilities error: invalid argument: at least one of emulatorbin or architecture fields must be present Would it be nicer to behave the same as 'virsh capabilities' and give the details of the default binary in this case? Sure, but in order to get default binary we must know architecture (consider the case where you have both /usr/bin/qemu-system-x86_64 and /usr/bin/qemu-system-i686). Although, having only one qemu binary on the system makes it easy to find the default, doesn't it? Patch on the way. Now, for a comparison between the two XML per binary: 'virsh capabilities' gives me this segment: hvm 64 /usr/bin/qemu-system-alpha clipper while 'virsh domcapabilities --emulatorbin /usr/bin/qemu-system-alpha --virttype kvm' gives this: /usr/bin/qemu-system-alpha kvm clipper alpha disk cdrom floppy lun ide fdc scsi virtio usb subsystem default mandatory requisite optional usb pci scsi I'm okay that the domcapabilites output is longer, and don't think we need to backport any of the new stuff to the older API. But any information present in the old API should be easily retrieved using the new API, so that the information isn't lost, and so that a client can learn the same amount of detail about a non-default binary as they can about the defaults. Look at the difference in XPath to get to some of the same information: /guest/os_type vs. ? - where is os_type in domcapabilities? nowhere yet. /guest/arch@name vs. /domainCapabilities/arch - why is one an attribute and the other an element? /
Re: [libvirt] virsh capabilities vs. domcapabilities
On Wed, Jul 16, 2014 at 01:00:30PM -0600, Eric Blake wrote: > Furthermore, I'm trying to figure out how to advertise whether a given > domain will support active commit (and similarly, Peter's patches for > relative backing name preservation). Advertising the feature just > through 'virsh capabilities' is insufficient, because that only covers > the default binary; so it seems like the sort of thing that should also > be in 'virsh domcapabilities'. My view is that now we have dom capabilities, we shouldn't add anything more to the old capabilities that is related to guest emulators. The main motivation is that the old capabilities is not scalable given the sheer volume of data we need to report per QEMU. > Since virConnectGetDomainCapabilities is brand new to 1.2.7, we still > have time to change its XML. But I want consensus on whether we need > things to match, or whether we intentionally want people to rely only on > the newer XML format of the new API call (that is, don't bloat 'virsh > capabilities'/virConnectGetCapabilities any further, and learning > whether active commit is supported will have to be done via 'virsh > domcapabilities'/virConnectGetDomainCapabilities). That is, I'm > wondering if should use / as > its starting point, rather than completely inventing new XML. I don't think there's really anything of value in the old capabilities XML schema that needs preserving as-is. > I'm also finding 'virsh domcapabilities' a bit hard to use - even though > it allows all its arguments to be optional at the RPC level, the qemu > implementation isn't so happy: > > # tools/virsh domcapabilities > error: failed to get emulator capabilities > error: virttype_str in qemuConnectGetDomainCapabilities must not be NULL > > but how am I supposed to know what --virttype strings are valid? Hmm, I'm pretty sure we agreed virt type was going to be optional and would jsut get a sensible default. So we should fix that. > > # tools/virsh domcapabilities --virttype kvm > error: failed to get emulator capabilities > error: invalid argument: at least one of emulatorbin or architecture > fields must be present > > Would it be nicer to behave the same as 'virsh capabilities' and give > the details of the default binary in this case? My original idea was that either emulator or arch would be required. If we wanted to allow just virtype on its own, I guess we could just default to the current host arch. > Now, for a comparison between the two XML per binary: > > 'virsh capabilities' gives me this segment: > > > hvm > > 64 > /usr/bin/qemu-system-alpha > clipper > > > > > > > > > > while 'virsh domcapabilities --emulatorbin /usr/bin/qemu-system-alpha > --virttype kvm' gives this: > > > /usr/bin/qemu-system-alpha > kvm > clipper > alpha > > > > > disk > cdrom > floppy > lun > > > ide > fdc > scsi > virtio > usb > > > > > subsystem > > > default > mandatory > requisite > optional > > > usb > pci > scsi > > > > > > > > I'm okay that the domcapabilites output is longer, and don't think we > need to backport any of the new stuff to the older API. But any > information present in the old API should be easily retrieved using the > new API, so that the information isn't lost, and so that a client can > learn the same amount of detail about a non-default binary as they can > about the defaults. > > Look at the difference in XPath to get to some of the same information: > > /guest/os_type vs. ? - where is os_type in domcapabilities? Yep, that's an oversight really. > /guest/arch@name vs. /domainCapabilities/arch - why is one an attribute > and the other an element? > > /guest/arch/wordsize vs. nothing - where is wordsize in domcapabilities? > > /guest/arch/emulator vs. /domainCapabilities/path - why 3 levels vs. 2, > and different naming? > > /guest/arch/machine@maxCpus vs. /domainCapabilities/vcpu@max - why 3 > levels vs. 2, with different naming? > > /guest/arch/machine vs. /domainCapabilities/machine - why 3 levels vs. 2? This is just a reflection of the fact that we're doing a different XML schema. We had the extra '/guest' level in the original XML because we were reporting on multiple guest types. I see no reason to make the new XML have this uneccessary extra level of nesting. > /guest/arch/domain@type vs. /domainCapabilities/domain - why attribute > of 3 levels vs. element at 2 levels? And did I expose an error when I > passed --virrtype kvm, even though qemu-system-alpha is only a qemu > emulator on my machine (since kvm emulators is only for hardware-assit > emulation, which is not possible when I'm doing cross architecture)? > > /guest/features vs. ? - where ar
Re: [libvirt] [PATCH v2 4/8] daemon: support passing FDs from the calling process
On Wed, Jul 16, 2014 at 08:29:58PM +0200, Martin Kletzander wrote: > First FD is the RW unix socket to listen on, second one (if > applicable) is the RO unix socket. > > Signed-off-by: Martin Kletzander > --- > daemon/libvirtd.c | 45 +++-- > 1 file changed, 27 insertions(+), 18 deletions(-) > > diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c > index 4c926b3..d20aeae 100644 > --- a/daemon/libvirtd.c > +++ b/daemon/libvirtd.c > @@ -56,6 +56,7 @@ > #include "virstring.h" > #include "locking/lock_manager.h" > #include "viraccessmanager.h" > +#include "virutil.h" > > #ifdef WITH_DRIVER_MODULES > # include "driver.h" > @@ -476,11 +477,19 @@ static int daemonSetupNetworking(virNetServerPtr srv, > int unix_sock_ro_mask = 0; > int unix_sock_rw_mask = 0; > > +unsigned int cur_fd = STDIN_FILENO + 1; Shouldn't that be STDERR not STDIN since the passed FDs start at 3 not 1. > +unsigned int nfds = virGetListenFDs(); > + Regards, 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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Schedule for the next release 1.2.7
On Thu, 2014-07-17 at 14:51 +0800, Daniel Veillard wrote: > Plan is to get the new release on Friday Aug 1st, so > I think this mean entering freeze at some point on the > preceeding week-end, I will be travelling then, but I > should be able to do that at some point saturday 26. > We already have 180 commits in since 1.2.6 so deinitely > makes sense to push this month too, It would be good if someone could push my 3 patches around LXC keep/drop capabilities before 1.2.7 freeze. Those were ACKed by Daniel during 1.2.6 freeze. -- Cedric -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 0/2] storage: Don't wipe volumes on remote filesystems with local tools
On 07/16/14 18:54, John Ferlan wrote: > > > On 07/11/2014 08:20 AM, Peter Krempa wrote: >> Version 2 now splits the stuff into separate driver backend funcs. >> >> Peter Krempa (2): >> storage: wipe: Move helper code into storage backend >> storage: Split out volume wiping as separate backend function >> >> src/storage/storage_backend.c | 203 >> + >> src/storage/storage_backend.h | 12 ++ >> src/storage/storage_backend_disk.c| 1 + >> src/storage/storage_backend_fs.c | 3 + >> src/storage/storage_backend_iscsi.c | 1 + >> src/storage/storage_backend_logical.c | 1 + >> src/storage/storage_backend_mpath.c | 1 + >> src/storage/storage_backend_scsi.c| 1 + >> src/storage/storage_driver.c | 205 >> +- >> 9 files changed, 229 insertions(+), 199 deletions(-) >> > > ACK both as they seem to fulfill the changes requested from > http://www.redhat.com/archives/libvir-list/2014-July/msg00556.html > Thanks; Pushed. > > John > > BTW: Since I was looking at a bug (bz 1091866) in this area - this > caught my attention... Not that these changes fix the problem, but > perhaps they'll help since as it seems "sparse snapshot" logical volumes > are "problematic"... > Well with this stuff pushed, you can easily add a wrapper function for the logical pool that will check whether the volume is sparse and forbid destroying it before relaying to the wiping function. Peter signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Syslog "nested job is dangerous" message
On 07/17/2014 08:51 AM, Jiri Denemark wrote: > On Thu, Jul 17, 2014 at 13:29:52 +1000, Sam Bobroff wrote: >> On 17/07/14 12:50, Eric Blake wrote: >>> On 07/16/2014 07:52 PM, Sam Bobroff wrote: Hello everyone, >>> >>> [Can you configure your mailer to wrap long lines?] >> >> [No problem, done.] >> After performing a migration, the syslog often contains several messages like this: "This thread seems to be the async job owner; entering monitor without asking for a nested job is dangerous" >>> >>> The sign of a bug that we need to fix. What version of libvirt are >>> you using? We may have already fixed it. >> >> I've been testing on 1.2.6, but I will re-test on the latest code from git. > > Hmm, it what were you doing when the message appeared in the log? I > fixed wrong usage of our job tracking APIs long time ago > (e4e28220b572cf4c71d07740c24150411f0704a6 in 1.0.3) so I guess there > must be another place we don't do it correctly. > That commit was essentially reverted by my commit 9846402 when fixing migration crashes: https://bugzilla.redhat.com/show_bug.cgi?id=1018267 I think the message was harmless during migration as we don't allow other jobs (except destroy). Jan signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 00/16] Support for per-guest-node binding
On 07/16/2014 04:42 PM, Martin Kletzander wrote: > v3 of https://www.redhat.com/archives/libvir-list/2014-July/msg00372.html > > v3: > - Michal's suggestions worked in > - rebased on current master > > Martin Kletzander (16): > qemu: purely a code movement > qemu: remove useless error check > conf: purely a code movement > conf, schema: add 'id' field for cells > numatune: create new module for numatune > numatune: unify numatune struct and enum names > numatune: Encapsulate numatune configuration in order to unify results > conf, schema: add support for memnode elements > numatune: add support for per-node memory bindings in private APIs > qemu: allow qmp probing for cmdline options without params > qemu: memory-backend-ram capability probing > qemu: newer -numa parameter capability probing > qemu: enable disjoint numa cpu ranges > qemu: pass numa node binding preferences to qemu > qemu: split out cpuset.mems setting > qemu: leave restricting cpuset.mems after initialization > > docs/formatdomain.html.in | 32 +- > docs/schemas/domaincommon.rng | 22 + > po/POTFILES.in | 1 + > src/Makefile.am| 3 +- > src/conf/cpu_conf.c| 41 +- > src/conf/cpu_conf.h| 3 +- > src/conf/domain_conf.c | 203 ++- > src/conf/domain_conf.h | 10 +- > src/conf/numatune_conf.c | 595 > + > src/conf/numatune_conf.h | 108 > src/libvirt_private.syms | 24 +- > src/lxc/lxc_cgroup.c | 20 +- > src/lxc/lxc_controller.c | 5 +- > src/lxc/lxc_native.c | 15 +- > src/parallels/parallels_driver.c | 7 +- > src/qemu/qemu_capabilities.c | 16 +- > src/qemu/qemu_capabilities.h | 2 + > src/qemu/qemu_cgroup.c | 52 +- > src/qemu/qemu_cgroup.h | 4 +- > src/qemu/qemu_command.c| 98 +++- > src/qemu/qemu_driver.c | 84 ++- > src/qemu/qemu_monitor.c| 6 +- > src/qemu/qemu_monitor.h| 3 +- > src/qemu/qemu_monitor_json.c | 8 +- > src/qemu/qemu_monitor_json.h | 3 +- > src/qemu/qemu_process.c| 12 +- > src/util/virnuma.c | 61 +-- > src/util/virnuma.h | 28 +- > tests/qemucapabilitiesdata/caps_1.6.50-1.caps | 1 + > tests/qemucapabilitiesdata/caps_1.6.50-1.replies | 5 + > tests/qemumonitorjsontest.c| 20 +- > .../qemuxml2argv-cpu-numa-disjoint.args| 6 + > .../qemuxml2argv-cpu-numa-disjoint.xml | 28 + > tests/qemuxml2argvdata/qemuxml2argv-cpu-numa1.xml | 6 +- > tests/qemuxml2argvdata/qemuxml2argv-cpu-numa2.xml | 6 +- > tests/qemuxml2argvdata/qemuxml2argv-cpu-numa3.xml | 25 + > .../qemuxml2argv-numatune-auto-prefer.xml | 29 + > .../qemuxml2argv-numatune-memnode-no-memory.args | 8 + > .../qemuxml2argv-numatune-memnode-no-memory.xml| 30 ++ > .../qemuxml2argv-numatune-memnode-nocpu.xml| 25 + > .../qemuxml2argv-numatune-memnode.args | 11 + > .../qemuxml2argv-numatune-memnode.xml | 33 ++ > .../qemuxml2argv-numatune-memnodes-problematic.xml | 31 ++ > tests/qemuxml2argvtest.c | 12 + > .../qemuxml2xmlout-cpu-numa1.xml | 28 + > .../qemuxml2xmlout-cpu-numa2.xml | 28 + > .../qemuxml2xmlout-numatune-auto-prefer.xml| 29 + > .../qemuxml2xmlout-numatune-memnode.xml| 33 ++ > tests/qemuxml2xmltest.c| 8 + > 49 files changed, 1479 insertions(+), 389 deletions(-) > create mode 100644 src/conf/numatune_conf.c > create mode 100644 src/conf/numatune_conf.h > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-cpu-numa-disjoint.args > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-cpu-numa-disjoint.xml > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-cpu-numa3.xml > create mode 100644 > tests/qemuxml2argvdata/qemuxml2argv-numatune-auto-prefer.xml > create mode 100644 > tests/qemuxml2argvdata/qemuxml2argv-numatune-memnode-no-memory.args > create mode 100644 > tests/qemuxml2argvdata/qemuxml2argv-numatune-memnode-no-memory.xml > create mode 100644 > tests/qemuxml2argvdata/qemuxml2argv-numatune-memnode-nocpu.xml > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-numatune-memnode.args > create mode 100644 tests/qemuxml2argvda