Re: [libvirt] mass create vm errors
On 18.07.2015 10:29, Vasiliy Tolstov wrote: > Hi again =). I have another problem then testing libvirt with massive vm > start: > 2015-07-18 08:25:21.687+: 36893: error : qemuMonitorIO:750 : > internal error: early end of file from monitor: possible problem: > 2015-07-18T08:25:21.586487Z qemu-system-x86_64: -vnc [::]:4,password: > Failed to start VNC server: Failed to bind socket: Address already in > use > > As i understand, when libvirt try to detect free port for vnc it > fails, because another process already binds to it. How can i avoid > this errors? Does it fixed in never libvirt releases? > > libvirt 1.216 > qemu 2.4.0-rc0 > There has not been much movement in that particular area since 1.2.16. There's inherent race though: by the time that libvirt detects that a certain port is free and qemu binds to it. During this window another process may just allocate the port which will result in the error message you're seeing. One of the solutions may be to use static ports, or make sure that other programs don't interfere with the range that's reserved for graphical sessions (the range can be configured too in qemu.conf). Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] rpc: RH1026137: Fix slow volume download (virsh vol-download)
On Mon, Jul 20, 2015 at 05:42:11PM +0300, Ossi Herrala wrote: On Sat, Jun 06, 2015 at 07:36:48PM +, Ossi Herrala wrote: Sorry to miss this mail, it got buried somehow and I haven't got to it until now since nobody pinged it. Sorry for the long wait then. No worries and thank you for taking time to review my patch. See new patch attached as well as comments on the memory usage below. The patch is tested on latest master (e46791e003444ce825feaf5bb2a16f778ee951e5). The only thing I would mention wrt to how it works after this patch is that it will consume some memory that's not needed, precisely (sizeof(struct iovec) + sizeof(void *)) * unreadMBs. It might be worth it to do: memmove(st->incomingVec, st->incomingVec + st->readVec, st->writeVec - st->readVec); VIR_SHRINK_N(st->incomingVec, st->readVec); st->writeVec -= st->readVec; st->readVec = 0; Apart from that it's definitely *way* better approach. What do you think of such modification? This would go either instead 'st->readVec++', but rather at the end of this function, so it's not done after each MB read. I totally agree and implemented freeing of the memory in the new patch: if (st->readVec >= 16) { memmove(st->incomingVec, st->incomingVec + st->readVec, sizeof(*st->incomingVec)*(st->writeVec - st->readVec)); VIR_SHRINK_N(st->incomingVec, st->writeVec, st->readVec); VIR_DEBUG("shrink removed %zu, left %zu", st->readVec, st->writeVec); st->readVec = 0; } now it only frees memory in chunks of 16 to avoid doing memmove() all the time. The iovec is 16 bytes (in 64 bit Linux) so this frees 256 bytes at a time and in my testing usually everything or almost everything that is allocated. Thanks again for taking time to review and commit, and I hope the new patch below is formatted ok. I managed to apply it as needed. I like how it works now and the feeing is fine, too. I tested it and it seems it's working perfectly. Without this patch I've got stuck on 1MiB/s locally after 11GiB in circa 1 minute, with this patch I managed to download 20GiB in less than a minute. ACK && will push after release since we're way after rc2 already. Again, sorry for the delays and thanks for the contribution! Have a nice day, Martin signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2] rpc: Remove keepalive_required option
Since its introduction in 2011 (particularly in commit f4324e329275), the option doesn't work. It just effectively disables all incoming connections. That's because the client private data that contain the 'keepalive_supported' boolean, are initialized to zeroes so the bool is false and the only other place where the bool is used is when checking whether the client supports keepalive. Thus, according to the server, no client supports keepalive. Removing this instead of fixing it is better because a) apparently nobody ever tried it since 2011 (4 years without one month) and b) we cannot know whether the client supports keepalive until we get a ping or pong keepalive packet. And that won't happen untile after we dispatched the ConnectOpen call. Another two reasons would be c) the keepalive_required was tracked on the server level, but keepalive_supported was in private data of the client as well as the check that was made in the remote layer, thus making all other instances of virNetServer miss this feature unless they all implemented it for themselves and d) we can always add it back in case there is a request and a use-case for it. Signed-off-by: Martin Kletzander --- v2: - Added a new daemontest file with current input/output. daemon/libvirtd-config.c | 4 daemon/libvirtd-config.h | 2 -- daemon/libvirtd.aug| 2 -- daemon/libvirtd.c | 2 -- daemon/libvirtd.conf | 7 -- daemon/libvirtd.h | 1 - daemon/remote.c| 8 +-- daemon/test_libvirtd.aug.in| 2 -- src/libvirt_remote.syms| 1 - src/locking/lock_daemon.c | 2 +- src/lxc/lxc_controller.c | 2 +- src/rpc/virnetserver.c | 25 +- src/rpc/virnetserver.h | 3 --- json => input-data-no-keepalive-required.json} | 2 -- .../virnetdaemondata/output-data-admin-nomdns.json | 2 -- .../virnetdaemondata/output-data-anon-clients.json | 1 - .../output-data-initial-nomdns.json| 1 - tests/virnetdaemondata/output-data-initial.json| 1 - ...json => output-data-no-keepalive-required.json} | 2 -- tests/virnetdaemontest.c | 2 +- 20 files changed, 5 insertions(+), 67 deletions(-) copy tests/virnetdaemondata/{input-data-admin-nomdns.json => input-data-no-keepalive-required.json} (96%) copy tests/virnetdaemondata/{input-data-admin-nomdns.json => output-data-no-keepalive-required.json} (96%) diff --git a/daemon/libvirtd-config.c b/daemon/libvirtd-config.c index 10dcc423d2db..c31c8b2e9ab5 100644 --- a/daemon/libvirtd-config.c +++ b/daemon/libvirtd-config.c @@ -292,7 +292,6 @@ daemonConfigNew(bool privileged ATTRIBUTE_UNUSED) data->keepalive_interval = 5; data->keepalive_count = 5; -data->keepalive_required = 0; data->admin_min_workers = 5; data->admin_max_workers = 20; @@ -302,7 +301,6 @@ daemonConfigNew(bool privileged ATTRIBUTE_UNUSED) data->admin_keepalive_interval = 5; data->admin_keepalive_count = 5; -data->admin_keepalive_required = 0; localhost = virGetHostname(); if (localhost == NULL) { @@ -471,11 +469,9 @@ daemonConfigLoadOptions(struct daemonConfig *data, GET_CONF_INT(conf, filename, keepalive_interval); GET_CONF_UINT(conf, filename, keepalive_count); -GET_CONF_UINT(conf, filename, keepalive_required); GET_CONF_INT(conf, filename, admin_keepalive_interval); GET_CONF_UINT(conf, filename, admin_keepalive_count); -GET_CONF_UINT(conf, filename, admin_keepalive_required); return 0; diff --git a/daemon/libvirtd-config.h b/daemon/libvirtd-config.h index 9cdae1a0cb59..3e1971d67f05 100644 --- a/daemon/libvirtd-config.h +++ b/daemon/libvirtd-config.h @@ -81,7 +81,6 @@ struct daemonConfig { int keepalive_interval; unsigned int keepalive_count; -int keepalive_required; int admin_min_workers; int admin_max_workers; @@ -91,7 +90,6 @@ struct daemonConfig { int admin_keepalive_interval; unsigned int admin_keepalive_count; -int admin_keepalive_required; }; diff --git a/daemon/libvirtd.aug b/daemon/libvirtd.aug index a70aa1dddf90..7c7992dd0568 100644 --- a/daemon/libvirtd.aug +++ b/daemon/libvirtd.aug @@ -79,11 +79,9 @@ module Libvirtd = let keepalive_entry = int_entry "keepalive_interval" | int_entry "keepalive_count" - | bool_entry "keepalive_required" let admin_keepalive_entry = int_entry "admin_keepalive_interval" | int_entry "admin_keepalive_count" - | bool_entry "admin_keepalive_required" let misc_entry = str_entry "host_uuid" diff --git a/daemon
Re: [libvirt] tc ingress rule of VM B disappear when reboot VM A
On 20.07.2015 10:43, ychen wrote: > hi: > when I use openstack devstack to test QOS, wired phenomenon appeared, > I set qos ingress rule in tapB, but when I reboot tapA, the ingress rule of > tapB automatically removed, but the egress rule is still exist. > Test enviroment: > Linux: ubuntu 14.04.1 LTS > kernel: 3.13.0-32-generic > libvirt: 1.2.2 > openstack: havana > > > 1. use nova to create vm A and vm B. for the configuration of the libvirt > xml, see the last paragraph in the end. > 2. use tc cmd to create qos rule for vm A and vm B >tc qdisc add dev tap3d0d2c4a-0b ingress //vmA >tc qdisc add dev tap896d5066-69 ingress //vmB > > > 3. then use cmd > "sudo virsh destory 142a08db-6e25-4a03-be13-7073104b0745 " to first > shutdown vm1 > then I see ingress rule of vmB disappeared :( > Interesting. With the latest libvirt I am unable to reproduce - there's a check and QoS is cleared out only if set in domain XML. Since your domain XML lacks it, I guess it's either already fixed or not libvirt fault. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Libvirt Application Development Guide
On 28.07.2015 09:52, Kevin Walker wrote: > Hi All > > > > I am currently getting up to speed with the libvirt api, but have noticed > there are some big gaps in the Application Development Guide available on > the libvirt website. > > > > Is there a more recent version of this document available? I am afraid you are looking at it. There was no development in that area for last 2-3 years. As always, patches are welcome :-) Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/4] conf: introduce seclabels in shmem device element
On 07/30/2015 06:28 PM, Daniel P. Berrange wrote: On Thu, Jul 23, 2015 at 06:13:46PM +0800, Luyao Huang wrote: Introduce a new element in shmem device element, this could help users to change the shm label to a specified label. Signed-off-by: Luyao Huang --- docs/formatdomain.html.in | 7 ++ docs/schemas/domaincommon.rng | 3 +++ src/conf/domain_conf.c| 55 ++- src/conf/domain_conf.h| 5 4 files changed, 59 insertions(+), 11 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index d0c1741..e02c67c 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -6098,6 +6098,13 @@ qemu-kvm -net nic,model=? /dev/null vectors. The ioeventd attribute enables/disables (values "on"/"off", respectively) ioeventfd. +seclabel + + The optional seclabel to override the way that labelling + is done on the shm object path or shm server path. If this + element is not present, the security label is inherited + from the per-domain setting. + Memory devices diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 1120003..f58e8de 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3323,6 +3323,9 @@ + + + So in the XML we have an explicit element to indicate whether the device is intended to be shared across multiple guests. I think we need to have the same flag added to the shm device too, so that we sanity check whether a particular shm was intended to be shared or whether it is a mistake when multiple guests use it. This will also allow us to integrate with the virtlockd to acquire exclusive locks against the shm device to actively prevent admin mistakes starting 2 guests with the same shm. It will also let us automatically choose the right default SELinux label ie svirt_image_t:s0:c214,c3242 for exclusive access vs svirt_image_t:s0 for shared access Good idea! i will introduce this new element in next version. Thanks a lot for your advise. Regards, Daniel Luyao -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/4] qemu: call the helpers in virshm.c to manage shmem device
On 07/30/2015 06:25 PM, Daniel P. Berrange wrote: On Thu, Jul 23, 2015 at 06:13:49PM +0800, Luyao Huang wrote: Signed-off-by: Luyao Huang --- src/qemu/qemu_conf.h| 3 + src/qemu/qemu_driver.c | 4 ++ src/qemu/qemu_process.c | 158 3 files changed, 165 insertions(+) +static int +qemuPrepareShmemDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainShmemDefPtr shmem) +{ +int ret = -1; +virShmObjectPtr tmp; +virShmObjectListPtr list = driver->shmlist; +bool othercreate = false; +char *path = NULL; +bool teardownlabel = false; +bool teardownshm = false; +int type, fd; + +virObjectLock(list); + +if ((tmp = virShmObjectFindByName(list, shmem->name))) { +if (shmem->size > tmp->size) { +virReportError(VIR_ERR_INTERNAL_ERROR, + _("Shmem object %s is already exists and " + "size is smaller than require size"), + tmp->name); +goto cleanup; +} + +if (virShmSetUsedDomain(tmp, QEMU_DRIVER_NAME, vm->def->name) < 0) +goto cleanup; + +if (virShmObjectSaveState(tmp, list->stateDir) < 0) +goto cleanup; + +virObjectUnlock(list); +return 0; +} + +if (!shmem->server.enabled) { +if ((fd = virShmCreate(shmem->name, shmem->size, false, &othercreate, 0600)) < 0) +goto cleanup; +VIR_FORCE_CLOSE(fd); + +if ((ret = virShmBuildPath(shmem->name, &path)) == -1) { +ignore_value(virShmUnlink(shmem->name)); +goto cleanup; +} else if (ret == -2 && !othercreate) { +ignore_value(virShmUnlink(shmem->name)); Why are you treating -1 differentl from -2 - in both cases we should abort creation as that indicates the method either failed or is not supported in this platform. What i thought when i wrote this is : when ret = -2 this means we do not support virShmBuildPath in that platform (this could only happened on some other platform which support shm_open/shm_unlink ,just like solaris, freebsd) but we could use shm_open, on that platform the shm obj is not in /dev/shm or not exist in the file system, if we help to create that shm obj but cannot find a way to relabel it, qemu will cannot use the shm in that case, so unlink the shm and let qemu create it will make guest can start success, and we could unlink the qemu create shm obj if there is no guest use it. I am not sure this is a good idea right now, since i am not sure this will work as except on different platform. Maybe i should remove it and make virShmBuildPath return -1 if not support on that platform. Regards, Daniel Luyao -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] rpc: Remove keepalive_required option
On Mon, Aug 03, 2015 at 09:05:53AM +0200, Martin Kletzander wrote: > Since its introduction in 2011 (particularly in commit f4324e329275), > the option doesn't work. It just effectively disables all incoming > connections. That's because the client private data that contain the > 'keepalive_supported' boolean, are initialized to zeroes so the bool is > false and the only other place where the bool is used is when checking > whether the client supports keepalive. Thus, according to the server, > no client supports keepalive. > > Removing this instead of fixing it is better because a) apparently > nobody ever tried it since 2011 (4 years without one month) and b) we > cannot know whether the client supports keepalive until we get a ping or > pong keepalive packet. And that won't happen untile after we dispatched > the ConnectOpen call. > > Another two reasons would be c) the keepalive_required was tracked on > the server level, but keepalive_supported was in private data of the > client as well as the check that was made in the remote layer, thus > making all other instances of virNetServer miss this feature unless they > all implemented it for themselves and d) we can always add it back in > case there is a request and a use-case for it. > > Signed-off-by: Martin Kletzander > --- > v2: > - Added a new daemontest file with current input/output. > > daemon/libvirtd-config.c | 4 > daemon/libvirtd-config.h | 2 -- > daemon/libvirtd.aug| 2 -- > daemon/libvirtd.c | 2 -- > daemon/libvirtd.conf | 7 -- > daemon/libvirtd.h | 1 - > daemon/remote.c| 8 +-- > daemon/test_libvirtd.aug.in| 2 -- > src/libvirt_remote.syms| 1 - > src/locking/lock_daemon.c | 2 +- > src/lxc/lxc_controller.c | 2 +- > src/rpc/virnetserver.c | 25 > +- > src/rpc/virnetserver.h | 3 --- > json => input-data-no-keepalive-required.json} | 2 -- > .../virnetdaemondata/output-data-admin-nomdns.json | 2 -- > .../virnetdaemondata/output-data-anon-clients.json | 1 - > .../output-data-initial-nomdns.json| 1 - > tests/virnetdaemondata/output-data-initial.json| 1 - > ...json => output-data-no-keepalive-required.json} | 2 -- > tests/virnetdaemontest.c | 2 +- > 20 files changed, 5 insertions(+), 67 deletions(-) > copy tests/virnetdaemondata/{input-data-admin-nomdns.json => > input-data-no-keepalive-required.json} (96%) > copy tests/virnetdaemondata/{input-data-admin-nomdns.json => > output-data-no-keepalive-required.json} (96%) > > diff --git a/daemon/libvirtd.aug b/daemon/libvirtd.aug > index a70aa1dddf90..7c7992dd0568 100644 > --- a/daemon/libvirtd.aug > +++ b/daemon/libvirtd.aug > @@ -79,11 +79,9 @@ module Libvirtd = > > let keepalive_entry = int_entry "keepalive_interval" > | int_entry "keepalive_count" > - | bool_entry "keepalive_required" > > let admin_keepalive_entry = int_entry "admin_keepalive_interval" > | int_entry "admin_keepalive_count" > - | bool_entry "admin_keepalive_required" > > let misc_entry = str_entry "host_uuid" > We should not remove entries from this file in general as it will cause augeas to fail to load any existing files with the config option present. 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
[libvirt] [python PATCH] Check return value of PyList_Append
libvirt_virDomainGetSecurityLabelList called PyList_Append without checking its return value. While looking at it I noticed the function did not properly check several other return values either so I fixed them all. https://bugzilla.redhat.com/show_bug.cgi?id=1249511 Signed-off-by: Jiri Denemark --- libvirt-override.c | 46 ++ 1 file changed, 38 insertions(+), 8 deletions(-) diff --git a/libvirt-override.c b/libvirt-override.c index 45c8afc..95061e8 100644 --- a/libvirt-override.c +++ b/libvirt-override.c @@ -3142,32 +3142,62 @@ libvirt_virDomainGetSecurityLabel(PyObject *self ATTRIBUTE_UNUSED, PyObject *arg #if LIBVIR_CHECK_VERSION(0, 10, 0) static PyObject * -libvirt_virDomainGetSecurityLabelList(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) { +libvirt_virDomainGetSecurityLabelList(PyObject *self ATTRIBUTE_UNUSED, + PyObject *args) +{ PyObject *py_retval; int c_retval; virDomainPtr dom; PyObject *pyobj_dom; -virSecurityLabel *labels; +virSecurityLabel *labels = NULL; size_t i; if (!PyArg_ParseTuple(args, (char *)"O:virDomainGetSecurityLabel", &pyobj_dom)) return NULL; + dom = (virDomainPtr) PyvirDomain_Get(pyobj_dom); LIBVIRT_BEGIN_ALLOW_THREADS; c_retval = virDomainGetSecurityLabelList(dom, &labels); LIBVIRT_END_ALLOW_THREADS; + if (c_retval < 0) return VIR_PY_NONE; -py_retval = PyList_New(0); + +if (!(py_retval = PyList_New(0))) +goto error; + for (i = 0 ; i < c_retval ; i++) { -PyObject *entry = PyList_New(2); -PyList_SetItem(entry, 0, libvirt_constcharPtrWrap(&labels[i].label[0])); -PyList_SetItem(entry, 1, libvirt_boolWrap(labels[i].enforcing)); -PyList_Append(py_retval, entry); +PyObject *entry; +PyObject *value; + +if (!(entry = PyList_New(2)) || +PyList_Append(py_retval, entry) < 0) { +Py_XDECREF(entry); +goto error; +} + +if (!(value = libvirt_constcharPtrWrap(&labels[i].label[0])) || +PyList_SetItem(entry, 0, value) < 0) { +Py_XDECREF(value); +goto error; +} + +if (!(value = libvirt_boolWrap(labels[i].enforcing)) || +PyList_SetItem(entry, 1, value) < 0) { +Py_XDECREF(value); +goto error; +} } -free(labels); + + cleanup: +VIR_FREE(labels); return py_retval; + + error: +Py_XDECREF(py_retval); +py_retval = NULL; +goto cleanup; } #endif /* LIBVIR_CHECK_VERSION(0, 10, 0) */ -- 2.5.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/2] qemu: Improve memory alignment handling and fix upcoming powerpc
On Fri, 2015-07-31 at 16:49 +0200, Peter Krempa wrote: > > Peter Krempa (2): > qemu: Make memory alignment helper more universal > qemu: ppc64: Align memory sizes to 256MiB > > src/qemu/qemu_domain.c | 33 > -- > src/qemu/qemu_domain.h | 3 +- > src/qemu/qemu_hotplug.c| 4 +-- > .../qemuxml2argv-pseries-cpu-compat.args | 2 +- > 4 files changed, 30 insertions(+), 12 deletions(-) Looks okay overall and works as expected from my limited testing on ppc64. One thing, though: the domain XML is not updated with the rounded-up value, which I think might be a little bit confusing to the user. Any chance the rounding up could be reflected there as well? Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] lxc / docker containers gives option to inherit the namespaces. Example lxc-start has option [ --share-[net|ipc|uts] name|pid ] where --share-net name|pid means Inherit a network
On 21.07.2015 17:06, Imran Khan wrote: > Gentle reminder. Humble request for another round of review. > > thanks > imran I'm sorry it took so long but I was on a vacation. I'd like to review, but the patch is mangled. Can you resend with git send-email? Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] lxc / docker containers gives option to inherit the namespaces. Example lxc-start has option [ --share-[net|ipc|uts] name|pid ] where --share-net name|pid means Inherit a network
On Mon, Aug 03, 2015 at 11:18:51AM +0200, Michal Privoznik wrote: > On 21.07.2015 17:06, Imran Khan wrote: > > Gentle reminder. Humble request for another round of review. > > > > thanks > > imran > > I'm sorry it took so long but I was on a vacation. I'd like to review, > but the patch is mangled. Can you resend with git send-email? Look at subject line from the same date: "[libvirt] [PATCH] Inherit namespace feature" I believe that is a clean posting that can be applied 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] lxc / docker containers gives option to inherit the namespaces. Example lxc-start has option [ --share-[net|ipc|uts] name|pid ] where --share-net name|pid means Inherit a network
On 03.08.2015 11:25, Daniel P. Berrange wrote: > On Mon, Aug 03, 2015 at 11:18:51AM +0200, Michal Privoznik wrote: >> On 21.07.2015 17:06, Imran Khan wrote: >>> Gentle reminder. Humble request for another round of review. >>> >>> thanks >>> imran >> >> I'm sorry it took so long but I was on a vacation. I'd like to review, >> but the patch is mangled. Can you resend with git send-email? > > Look at subject line from the same date: > > "[libvirt] [PATCH] Inherit namespace feature" > > I believe that is a clean posting that can be applied Ah, right. And that one you've reviewed yesterday. I still haven't fully caught up with the list. Sorry for the noise then. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [python PATCH] Check return value of PyList_Append
On Mon, Aug 03, 2015 at 10:30:20AM +0200, Jiri Denemark wrote: > libvirt_virDomainGetSecurityLabelList called PyList_Append without > checking its return value. While looking at it I noticed the function > did not properly check several other return values either so I fixed > them all. > > https://bugzilla.redhat.com/show_bug.cgi?id=1249511 > > Signed-off-by: Jiri Denemark > --- > libvirt-override.c | 46 ++ > 1 file changed, 38 insertions(+), 8 deletions(-) ACK Pavel -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] rpc: Remove keepalive_required option
On Mon, Aug 03, 2015 at 09:27:37AM +0100, Daniel P. Berrange wrote: On Mon, Aug 03, 2015 at 09:05:53AM +0200, Martin Kletzander wrote: Since its introduction in 2011 (particularly in commit f4324e329275), the option doesn't work. It just effectively disables all incoming connections. That's because the client private data that contain the 'keepalive_supported' boolean, are initialized to zeroes so the bool is false and the only other place where the bool is used is when checking whether the client supports keepalive. Thus, according to the server, no client supports keepalive. Removing this instead of fixing it is better because a) apparently nobody ever tried it since 2011 (4 years without one month) and b) we cannot know whether the client supports keepalive until we get a ping or pong keepalive packet. And that won't happen untile after we dispatched the ConnectOpen call. Another two reasons would be c) the keepalive_required was tracked on the server level, but keepalive_supported was in private data of the client as well as the check that was made in the remote layer, thus making all other instances of virNetServer miss this feature unless they all implemented it for themselves and d) we can always add it back in case there is a request and a use-case for it. Signed-off-by: Martin Kletzander --- v2: - Added a new daemontest file with current input/output. daemon/libvirtd-config.c | 4 daemon/libvirtd-config.h | 2 -- daemon/libvirtd.aug| 2 -- daemon/libvirtd.c | 2 -- daemon/libvirtd.conf | 7 -- daemon/libvirtd.h | 1 - daemon/remote.c| 8 +-- daemon/test_libvirtd.aug.in| 2 -- src/libvirt_remote.syms| 1 - src/locking/lock_daemon.c | 2 +- src/lxc/lxc_controller.c | 2 +- src/rpc/virnetserver.c | 25 +- src/rpc/virnetserver.h | 3 --- json => input-data-no-keepalive-required.json} | 2 -- .../virnetdaemondata/output-data-admin-nomdns.json | 2 -- .../virnetdaemondata/output-data-anon-clients.json | 1 - .../output-data-initial-nomdns.json| 1 - tests/virnetdaemondata/output-data-initial.json| 1 - ...json => output-data-no-keepalive-required.json} | 2 -- tests/virnetdaemontest.c | 2 +- 20 files changed, 5 insertions(+), 67 deletions(-) copy tests/virnetdaemondata/{input-data-admin-nomdns.json => input-data-no-keepalive-required.json} (96%) copy tests/virnetdaemondata/{input-data-admin-nomdns.json => output-data-no-keepalive-required.json} (96%) diff --git a/daemon/libvirtd.aug b/daemon/libvirtd.aug index a70aa1dddf90..7c7992dd0568 100644 --- a/daemon/libvirtd.aug +++ b/daemon/libvirtd.aug @@ -79,11 +79,9 @@ module Libvirtd = let keepalive_entry = int_entry "keepalive_interval" | int_entry "keepalive_count" - | bool_entry "keepalive_required" let admin_keepalive_entry = int_entry "admin_keepalive_interval" | int_entry "admin_keepalive_count" - | bool_entry "admin_keepalive_required" let misc_entry = str_entry "host_uuid" We should not remove entries from this file in general as it will cause augeas to fail to load any existing files with the config option present. So you are suggesting leaving them just here, but removing them from everywhere else, right? I'll send a v3 if that's the case. 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: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] rpc: Remove keepalive_required option
On Mon, Aug 03, 2015 at 11:43:35AM +0200, Martin Kletzander wrote: > On Mon, Aug 03, 2015 at 09:27:37AM +0100, Daniel P. Berrange wrote: > >On Mon, Aug 03, 2015 at 09:05:53AM +0200, Martin Kletzander wrote: > >>Since its introduction in 2011 (particularly in commit f4324e329275), > >>the option doesn't work. It just effectively disables all incoming > >>connections. That's because the client private data that contain the > >>'keepalive_supported' boolean, are initialized to zeroes so the bool is > >>false and the only other place where the bool is used is when checking > >>whether the client supports keepalive. Thus, according to the server, > >>no client supports keepalive. > >> > >>Removing this instead of fixing it is better because a) apparently > >>nobody ever tried it since 2011 (4 years without one month) and b) we > >>cannot know whether the client supports keepalive until we get a ping or > >>pong keepalive packet. And that won't happen untile after we dispatched > >>the ConnectOpen call. > >> > >>Another two reasons would be c) the keepalive_required was tracked on > >>the server level, but keepalive_supported was in private data of the > >>client as well as the check that was made in the remote layer, thus > >>making all other instances of virNetServer miss this feature unless they > >>all implemented it for themselves and d) we can always add it back in > >>case there is a request and a use-case for it. > >> > >>Signed-off-by: Martin Kletzander > >>--- > >>v2: > >> - Added a new daemontest file with current input/output. > >> > >> daemon/libvirtd-config.c | 4 > >> daemon/libvirtd-config.h | 2 -- > >> daemon/libvirtd.aug| 2 -- > >> daemon/libvirtd.c | 2 -- > >> daemon/libvirtd.conf | 7 -- > >> daemon/libvirtd.h | 1 - > >> daemon/remote.c| 8 +-- > >> daemon/test_libvirtd.aug.in| 2 -- > >> src/libvirt_remote.syms| 1 - > >> src/locking/lock_daemon.c | 2 +- > >> src/lxc/lxc_controller.c | 2 +- > >> src/rpc/virnetserver.c | 25 > >> +- > >> src/rpc/virnetserver.h | 3 --- > >> json => input-data-no-keepalive-required.json} | 2 -- > >> .../virnetdaemondata/output-data-admin-nomdns.json | 2 -- > >> .../virnetdaemondata/output-data-anon-clients.json | 1 - > >> .../output-data-initial-nomdns.json| 1 - > >> tests/virnetdaemondata/output-data-initial.json| 1 - > >> ...json => output-data-no-keepalive-required.json} | 2 -- > >> tests/virnetdaemontest.c | 2 +- > >> 20 files changed, 5 insertions(+), 67 deletions(-) > >> copy tests/virnetdaemondata/{input-data-admin-nomdns.json => > >> input-data-no-keepalive-required.json} (96%) > >> copy tests/virnetdaemondata/{input-data-admin-nomdns.json => > >> output-data-no-keepalive-required.json} (96%) > >> > > > >>diff --git a/daemon/libvirtd.aug b/daemon/libvirtd.aug > >>index a70aa1dddf90..7c7992dd0568 100644 > >>--- a/daemon/libvirtd.aug > >>+++ b/daemon/libvirtd.aug > >>@@ -79,11 +79,9 @@ module Libvirtd = > >> > >>let keepalive_entry = int_entry "keepalive_interval" > >>| int_entry "keepalive_count" > >>- | bool_entry "keepalive_required" > >> > >>let admin_keepalive_entry = int_entry "admin_keepalive_interval" > >> | int_entry "admin_keepalive_count" > >>- | bool_entry "admin_keepalive_required" > >> > >>let misc_entry = str_entry "host_uuid" > >> > > > >We should not remove entries from this file in general as it will > >cause augeas to fail to load any existing files with the config > >option present. > > > > So you are suggesting leaving them just here, but removing them from > everywhere else, right? I'll send a v3 if that's the case. Yes, that's what we did with the 'log_buffer' setting we removed in the past. Actually we left it in libvirtd.conf too with a comment saying it was no longer used, for sake of historical reference 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] [PATCHv3 01/13] conf: add new subelement with name attribute to
On Sat, Jul 25, 2015 at 03:58:25PM -0400, Laine Stump wrote: This new subelement is used in PCI controllers: the toplevel *attribute* "model" of a controller denotes what kind of PCI controller is being described, e.g. a "dmi-to-pci-bridge", "pci-bridge", or "pci-root". But in the future there will be different implementations of some of those types of PCI controllers, which behave similarly from libvirt's point of view (and so should have the same model), but use a different device in qemu (and present themselves as a different piece of hardware in the guest). In an ideal world we (i.e. "I") would have thought of that back when the pci controllers were added, and used some sort of type/class/model notation (where class was used in the way we are now using model, and model was used for the actual manufacturer's model number of a particular family of PCI controller), but that opportunity is long past, so as an alternative, this patch allows selecting a particular implementation of a pci controller with the "name" attribute of the subelement, e.g.: In this case, "dmi-to-pci-bridge" is the kind of controller (one that has a single PCIe port upstream, and 32 standard PCI ports downstream, which are not hotpluggable), and the qemu device to be used to implement this kind of controller is named "i82801b11-bridge". Implementing the above now will allow us in the future to add a new kind of dmi-to-pci-bridge that doesn't use qemu's i82801b11-bridge device, but instead uses something else (which doesn't yet exist, but qemu people have been discussing it), all without breaking existing configs. (note that for the existing "pci-bridge" type of PCI controller, both the model attribute and name are 'pci-bridge'. This is just a coincidence, since it turns out that in this case the device name in qemu really is a generic 'pci-bridge' rather than being the name of some real-world chip) --- change from V2: * attribute is now called "name" instead of "type" * different possible model names are stored internally as an enum value rather than a string. * more than one occurence of in a single controller is now considered an error * docs say "1.2.18" instead of "1.3.0" docs/formatdomain.html.in | 13 +++ docs/schemas/domaincommon.rng | 13 +++ src/conf/domain_conf.c | 46 +++-- src/conf/domain_conf.h | 16 + src/libvirt_private.syms| 2 ++ tests/qemuxml2argvdata/qemuxml2argv-q35.xml | 8 +++-- tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml | 8 +++-- 7 files changed, 100 insertions(+), 6 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6b557d1..d58e679 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7809,6 +7817,15 @@ virDomainControllerDefParseXML(xmlNodePtr node, queues = virXMLPropString(cur, "queues"); cmd_per_lun = virXMLPropString(cur, "cmd_per_lun"); max_sectors = virXMLPropString(cur, "max_sectors"); +} else if (xmlStrEqual(cur->name, BAD_CAST "model")) { +if (processedModel) { Unnecessary variable, you could just use 'modelName', but I have no opinion on that, so ACK either way. signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv3 02/13] qemu: implement subelement to
[I reduced the Cc list so I don't need to hear jtomko's whining again] On Sat, Jul 25, 2015 at 03:58:26PM -0400, Laine Stump wrote: This patch provides qemu support for the contents of in for the two existing PCI controller types that need it (i.e. the two controller types that are backed by a device that must be specified on the qemu commandline): 1) pci-bridge - sets name attribute default as "pci-bridge" 2) dmi-to-pci-bridge - sets name attribute default as "i82801b11-bridge". These both match current hardcoded practice. The defaults are set at the end of qemuDomainAssignPCIAddresses(), It can't be done earlier because some of the options that will be autogenerated need full PCI address info for the controller and because qemuDomainAssignPCIAddresses() might create extra controllers which would need default settings added, and that hasn't been done at the time the PostParse callbacks are being run. qemuDomainAssignPCIAddresses() is still prior to the XML being written to disk, though, so the autogenerated defaults are persistent. qemu capabilities bits aren't checked until the commandline is actually created (so the domain can possibly be defined on a host that doesn't yet have support for the give n device, or a host different from the one where it will eventually be run). At that time we compare the type strings to known qemu device names and check for the capabilities bit for that device. --- Changes from V2: use enum instead of string model name. src/qemu/qemu_command.c | 81 ++--- 1 file changed, 77 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 09f30c4..6a19d15 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2251,11 +2251,33 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, virDomainControllerDefPtr cont = def->controllers[i]; int idx = cont->idx; virDevicePCIAddressPtr addr; +virDomainPCIControllerOptsPtr options; if (cont->type != VIR_DOMAIN_CONTROLLER_TYPE_PCI) continue; addr = &cont->info.addr.pci; +options = &cont->opts.pciopts; + +/* set defaults for any other auto-generated config + * options for this controller that haven't been + * specified in config. + */ +switch ((virDomainControllerModelPCI)cont->model) { +case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: +if (options->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) +options->modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCI_BRIDGE; +break; +case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: +if (options->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) +options->modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_I82801B11_BRIDGE; +break; +case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: +case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: +case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: +break; +} + /* check if every PCI bridge controller's ID is greater than * the bus it is placed onto */ @@ -4505,6 +4527,7 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef, { virBuffer buf = VIR_BUFFER_INITIALIZER; int model = def->model; +const char *modelName = NULL; if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) { if ((qemuSetSCSIControllerModel(domainDef, qemuCaps, &model)) < 0) @@ -4626,17 +4649,67 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef, } switch (def->model) { case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: -virBufferAsprintf(&buf, "pci-bridge,chassis_nr=%d,id=%s", - def->idx, def->info.alias); +if (def->opts.pciopts.modelName +== VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) { +virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("autogenerated pci-bridge options not set")); +goto error; +} + +modelName = virDomainControllerPCIModelNameTypeToString(def->opts.pciopts.modelName); +if (!modelName) { +virReportError(VIR_ERR_INTERNAL_ERROR, + _("unknown pci-bridge model name value %d"), + def->opts.pciopts.modelName); +goto error; +} +if (def->opts.pciopts.modelName +!= VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCI_BRIDGE) { +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("PCI controller model name '%s' " +
Re: [libvirt] [PATCHv3 01/13] conf: add new subelement with name attribute to
On Sat, Jul 25, 2015 at 03:58:25PM -0400, Laine Stump wrote: This new subelement is used in PCI controllers: the toplevel *attribute* "model" of a controller denotes what kind of PCI controller is being described, e.g. a "dmi-to-pci-bridge", "pci-bridge", or "pci-root". But in the future there will be different implementations of some of those types of PCI controllers, which behave similarly from libvirt's point of view (and so should have the same model), but use a different device in qemu (and present themselves as a different piece of hardware in the guest). In an ideal world we (i.e. "I") would have thought of that back when the pci controllers were added, and used some sort of type/class/model notation (where class was used in the way we are now using model, and model was used for the actual manufacturer's model number of a particular family of PCI controller), but that opportunity is long past, so as an alternative, this patch allows selecting a particular implementation of a pci controller with the "name" attribute of the subelement, e.g.: In this case, "dmi-to-pci-bridge" is the kind of controller (one that has a single PCIe port upstream, and 32 standard PCI ports downstream, which are not hotpluggable), and the qemu device to be used to implement this kind of controller is named "i82801b11-bridge". Implementing the above now will allow us in the future to add a new kind of dmi-to-pci-bridge that doesn't use qemu's i82801b11-bridge device, but instead uses something else (which doesn't yet exist, but qemu people have been discussing it), all without breaking existing configs. (note that for the existing "pci-bridge" type of PCI controller, both the model attribute and name are 'pci-bridge'. This is just a coincidence, since it turns out that in this case the device name in qemu really is a generic 'pci-bridge' rather than being the name of some real-world chip) --- change from V2: * attribute is now called "name" instead of "type" * different possible model names are stored internally as an enum value rather than a string. * more than one occurence of in a single controller is now considered an error * docs say "1.2.18" instead of "1.3.0" docs/formatdomain.html.in | 13 +++ docs/schemas/domaincommon.rng | 13 +++ src/conf/domain_conf.c | 46 +++-- src/conf/domain_conf.h | 16 + src/libvirt_private.syms| 2 ++ tests/qemuxml2argvdata/qemuxml2argv-q35.xml | 8 +++-- tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml | 8 +++-- 7 files changed, 100 insertions(+), 6 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index d0c1741..9abceee 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3042,6 +3042,19 @@ (set to 0). Since 1.1.2 (QEMU only) + PCI controllers also have an optional + subelementwith an attribute + name. The name attribute holds the name of the + specific device that qemu is emulating (e.g. "i82801b11-bridge") + rather than simply the class of device ("dmi-to-pci-bridge", + "pci-bridge"), which is set in the controller element's + model attribute. In almost all cases, you should not + manually add a subelement to a + controller, nor should you modify one that is automatically + generated by libvirt. Since 1.2.18 (QEMU Oh, I forgot to ask you to change this to 1.2.19 ^^, sorry about missing not just the rc1, but the whole release. But I was partly offline for the whole week as well and I hoped someone else might pick this up... Martin signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv3 03/13] conf: add new subelement with chassisNr attribute to
On Sat, Jul 25, 2015 at 03:58:27PM -0400, Laine Stump wrote: There are some configuration options to some types of pci controllers that are currently automatically derived from other parts of the controller's configuration. For example, in qemu a pci-bridge controller has an option that is called "chassis_nr"; up until now libvirt has always set chassis_nr to the index of the pci-bridge. So this: will always result in: -device pci-bridge,chassis_nr=2,... on the qemu commandline. In the future we may decide there is a better way to derive that option, but even in that case we will need for existing domains to retain the same chassis_nr they were using in the past - that is something that is visible to the guest so it is part of the guest ABI and changing it would lead to problems for migrating guests (or just guests with very picky OSes). The subelement has been added as a place to put the new "chassisNr" attribute that will be filled in by libvirt when it auto-generates the chassisNr; it will be saved in the config, then reused any time the domain is started: The one oddity of all this is that if the controller configuration is changed (for example to change the index or the pci address where the controller is plugged in), the items in will *not* be re-generated, which might lead to conflict. I can't really see any way around this, but fortunately if there is a material conflict qemu will let us know and we will pass that on to the user. --- changes from V2: * check chassisNr for 0-255 range * multiple s in a single controller is now an error * 1.3.0 -> 1.2.18 docs/formatdomain.html.in | 24 + docs/schemas/domaincommon.rng | 10 ++ src/conf/domain_conf.c | 45 +++-- src/conf/domain_conf.h | 10 +- tests/qemuxml2argvdata/qemuxml2argv-q35.xml | 1 + tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml | 1 + 6 files changed, 88 insertions(+), 3 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 9abceee..fdf7e82 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3055,6 +3055,30 @@ only). + PCI controllers also have an optional + subelementwith the attributes + listed below. These are configurable items that 1) are visible + to the guest OS so must be preserved for guest ABI + compatibility, and 2) are usually left to default values or + derived automatically by libvirt. In almost all cases, you + should not manually add a subelement + to a controller, nor should you modify the values in the those + that are automatically generated by + libvirt. Since 1.2.18 (QEMU only). s/18/19/ + + + chassisNr + +PCI controllers that have attribute model="pci-bridge", can +also have a chassisNr attribute in +the subelement, which is used to +control QEMU's "chassis_nr" option for the pci-bridge device +(normally libvirt automatically sets this to the same value as +the index attribute of the pci controller). If set, chassisNr +must be between 0 and 255. + + + For machine types which provide an implicit PCI bus, the pci-root controller with index=0 is auto-added and required to use PCI devices. pci-root has no address. diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d58e679..fbad7e9 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7826,6 +7830,15 @@ virDomainControllerDefParseXML(xmlNodePtr node, } modelName = virXMLPropString(cur, "name"); processedModel = true; +} else if (xmlStrEqual(cur->name, BAD_CAST "target")) { +if (processedTarget) { +virReportError(VIR_ERR_XML_ERROR, "%s", + _("Multiple elements in " + "controller definition not allowed")); +goto error; +} +chassisNr = virXMLPropString(cur, "chassisNr"); +processedTarget = true; Similarly to the previous patch you could omit the boolean here. Also there will be way more similarities in following patches that could be coupled in a function. But that's more like suggestion for future since it wouldn't go with the rest of the file, the whole parsing could use a re-factor :) ACK signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv3 06/13] conf: new pci controller model "pcie-root-port"
On Sat, Jul 25, 2015 at 03:58:30PM -0400, Laine Stump wrote: This controller can be connected (at domain startup time only - not hotpluggable) only to a port on the pcie root complex ("pcie-root" in libvirt config), hence the new connect type VIR_PCI_CONNECT_TYPE_PCIE_ROOT. It provides a hotpluggable port that will accept any PCI or PCIe device. New attributes must be added to the controller subelement for this - chassis and port are guest-visible option values that will be set by libvirt with values derived from the controller's index and pci address information. --- change from V2: * check chassis/port for 0-255 range * 1.3.0 -> 1.2.18 docs/formatdomain.html.in | 33 +- docs/schemas/domaincommon.rng | 13 src/conf/domain_addr.c | 10 ++- src/conf/domain_addr.h | 5 +- src/conf/domain_conf.c | 75 +- src/conf/domain_conf.h | 8 ++- src/qemu/qemu_command.c| 1 + .../qemuxml2argv-pcie-root-port.xml| 36 +++ tests/qemuxml2xmltest.c| 1 + 9 files changed, 173 insertions(+), 9 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pcie-root-port.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index fdf7e82..a9db924 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3123,6 +3141,17 @@ auto-determined by libvirt will be placed on this pci-bridge device. (since 1.1.2). + + Domains with an implicit pcie-root can also add controllers + with model='pcie-root-port'. This is a simple type of + bridge device that can connect only to one of the 31 slots on + the pcie-root bus on the upstream side, and makes a single + (PCIe, hotpluggable) port (at slot='0') available on the + downstream side. This controller can be used to provide a single + slot to later hotplug a PCIe device (but is not itself + hotpluggable - it must be in the configuration when the domain + is started). (since 1.2.18) s/18/19/ diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index fbad7e9..f1723c0 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7972,6 +7980,57 @@ virDomainControllerDefParseXML(xmlNodePtr node, goto error; } } +if (chassis) { +if (virStrToLong_i(chassis, NULL, 0, + &def->opts.pciopts.chassis) < 0) { +virReportError(VIR_ERR_XML_ERROR, + _("Invalid chassis '%s' in PCI controller"), + chassis); +goto error; +} +if (def->opts.pciopts.chassis < 0 || +def->opts.pciopts.chassisNr > 255) { +virReportError(VIR_ERR_XML_ERROR, + _("PCI controller chassis '%s' out of range " + "- must be 0-255"), + chassis); +goto error; +} +} This gets parsed twice, up here ^^ and down here vv, copy-paste error? Remove one of those, please. I, personally don't care much which one, none of them is my favourite :) ACK with that changed +if (chassis) { +if (virStrToLong_i(chassis, NULL, 0, + &def->opts.pciopts.chassis) < 0) { +virReportError(VIR_ERR_XML_ERROR, + _("Invalid chassis '%s' in PCI controller"), + chassis); +goto error; +} +if (def->opts.pciopts.chassis < 0 || +def->opts.pciopts.chassis > 255) { +virReportError(VIR_ERR_XML_ERROR, + _("PCI controller chassis '%s' out of range " + "- must be 0-255"), + chassis); +goto error; +} +} +if (port) { +if (virStrToLong_i(port, NULL, 0, + &def->opts.pciopts.port) < 0) { +virReportError(VIR_ERR_XML_ERROR, + _("Invalid port '%s' in PCI controller"), + port); +goto error; +} +if (def->opts.pciopts.port < 0 || +def->opts.pciopts.port > 255) { +virReportError(VIR_ERR_XML_ERROR, + _("PCI controller port '%s' out of range " + "- must be 0-255"), + port); +goto error; +} +} break; default: signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.co
Re: [libvirt] [PATCHv3 12/13] conf: new pcie-controller model "pcie-switch-downstream-port"
On Sat, Jul 25, 2015 at 03:58:36PM -0400, Laine Stump wrote: This controller can be connected only to a port on a pcie-switch-upstream-port. It provides a single hotpluggable port that will accept any PCI or PCIe device, as well as any device requiring a pcie-*-port (the only current example of such a device is the pcie-switch-upstream-port). --- change for V2: * 1.3.0 -> 1.2.18 * possibly reworded documention a bit. docs/formatdomain.html.in | 49 +++--- docs/schemas/domaincommon.rng | 3 ++ src/conf/domain_addr.c | 11 + src/conf/domain_conf.c | 6 ++- src/conf/domain_conf.h | 2 + src/qemu/qemu_command.c| 1 + .../qemuxml2argv-pcie-switch-downstream-port.xml | 44 +++ tests/qemuxml2xmltest.c| 1 + 8 files changed, 100 insertions(+), 17 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pcie-switch-downstream-port.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index d033542..2f61ac8 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3030,11 +3030,12 @@ PCI controllers have an optional model attribute with possible values pci-root, pcie-root, pcie-root-port, pci-bridge, - dmi-to-pci-bridge, or pcie-switch-upstream-port. + dmi-to-pci-bridge, pcie-switch-upstream-port, or + pcie-switch-downstream-port. (pci-root and pci-bridge since 1.0.5, pcie-root and dmi-to-pci-bridge since - 1.1.2, pcie-root-port and - pcie-switch-upstream-port since 1.2.18) + 1.1.2, pcie-root-port, pcie-switch-upstream-port, and + pcie-switch-downstream-port since 1.2.18) s/18/19/ everywhere diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index 8bd4ac3..561d8cc 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -213,6 +213,17 @@ virDomainPCIAddressBusSetModel(virDomainPCIAddressBusPtr bus, bus->minSlot = 0; bus->maxSlot = 31; break; +case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: This could be merged with VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT, what's the difference then? ACK if this is just a readability enhancement and is supposed to be this way. +/* provides one slot which is pcie, can be used by devices + * that must connect to some type of "pcie-*-port", and + * is hotpluggable + */ +bus->flags = VIR_PCI_CONNECT_TYPE_PCIE + | VIR_PCI_CONNECT_TYPE_PCIE_PORT + | VIR_PCI_CONNECT_HOTPLUGGABLE; +bus->minSlot = 0; +bus->maxSlot = 0; +break; default: virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid PCI controller model %d"), model); signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] Release of libvirt-1.2.18
It's out ! Tagged in git and with signed tarball and rpms on the server at the usual place: ftp://libvirt.org/libvirt/ I also made a libvirt-python release found at: ftp://libvirt.org/libvirt/python/ This is a rather small size release, the libxl support for dom0 being the main feature from an user POV, but there is as usual a number of bug fixes and improvements across the code: Features: Documentation: - fix typo in qemu_monitor (Cao jin) - docs: bhyve: document clock configuration (Roman Bogorodskiy) - viraccessperm.h: Fix some typos (Michal Privoznik) - docs: Document how libvirt handles companion controllers (Martin Kletzander) - daemonRunStateInit: Fix a typo on a comment (Michal Privoznik) Portability: - netdev: fix build on FreeBSD (Roman Bogorodskiy) - spec: Fix polkit dep on F23 (Cole Robinson) - nodeinfo: fix build on FreeBSD (Roman Bogorodskiy) - Escape left brace as new perl suggests (Martin Kletzander) Bug Fixes: - qemu: Do not reset labels when migration fails (Jiri Denemark) - qemu: Reject migration with memory-hotplug if destination doesn't support it (Peter Krempa) - Load nbd module before running qemu-nbd (Cédric Bosdonnat) - lxc: Don't accidentaly reset autostart flag in virLXCProcessCleanup (Peter Krempa) - remote: fix typo in remoteDomainOpenGraphicsFD (Daniel P. Berrange) - qemu: Check for iotune_max support properly (Martin Kletzander) - Renamed deconfigured-cpus to allow make dist (Daniel Veillard) - storage: allow zero capacity with non-backing file to be created (Chris J Arges) - nodeinfo: Check for SYSFS_INFINIBAND_DIR before open (John Ferlan) - qemu: fix the error cover issue in SetMemoryParameters (Luyao Huang) - nodeinfo: Fix nodeGetCPUBitmap()'s fallback code path (Andrea Bolognani) - tests: Restore links in deconfigured-cpus nodeinfo test (Andrea Bolognani) - cgroup: Drop resource partition from virSystemdMakeScopeName (Peter Krempa) - qemu: Reject updating unsupported disk information (Martin Kletzander) - storage: Fix pool building when directory already exists (Christophe Fergeau) - rpc: ensure daemon is spawn even if dead socket exists (Daniel P. Berrange) - rbd: Return error from rbd_create for message processing (John Ferlan) - qemuMigrationRun: Don't leak @fd (Michal Privoznik) - Fix qemu-nbd cleanup crashes (Cédric Bosdonnat) - network: Add another collision check into networkCheckRouteCollision (Martin Kletzander) - nodeinfo: fix to parse present cpus rather than possible cpus (Kothapally Madhu Pavan) - libxl: set dom0 state to running (Jim Fehlig) - rpc: Rework timerActive logic in daemon (Martin Kletzander) - qemu: Check duplicate WWNs also for hotplugged disks (Peter Krempa) - Fix cloning of raw, sparse volumes (Prerna Saxena) - qemu: don't use initialized ret in qemuRemoveSharedDevice (Guido Günther) - qemu: report error for non-existing disk in blockjobinfo (Luyao Huang) - conf: Don't allow duplicated target names regardless of bus (John Ferlan) - storage: Revert volume obj list updating after volume creation (4749d82a) (Erik Skultety) - qemu: Fix integer/boolean logic in qemuSetUnprivSGIO (John Ferlan) - qemu: report error when shmem has an invalid address (Luyao Huang) - qemu: Auto assign pci addresses for shared memory devices (Luyao Huang) - vz: use PRL_USE_VNET_NAME_FOR_BRIDGE_NAME (Maxim Nestratov) - Explicitly format the isa-fdc controller for newer q35 machines (Ján Tomko) - virt-aa-helper: add unix channels for nserials as well (Serge Hallyn) - storage: Fix regression in storagePoolUpdateAllState (Erik Skultety) - better patch for the XSS search issue (Daniel Veillard) - Avoid XSS vulnerability on the search engine (Daniel Veillard) - vz: fix SDK event dispatching (Nikolay Shirokovskiy) - util: Avoid Coverity FORWARD_NULL (John Ferlan) - virsh: report error if vcpu number exceed the guest maxvcpu number (Luyao Huang) - virt-aa-helper: Fix permissions for vhost-user socket files (Michal Dubiel) Improvements: - qemu: Properly check for incoming migration job (Jiri Denemark) - Fix syntax-check: missing "%s" (Cédric Bosdonnat) - qemu: Adjust VM id allocation (Erik Skultety) - admin: Tiny cleanups (Martin Kletzander) - tests: Finish rename of the long nodeinfo test case (Martin Kletzander) - conf: add virDomainControllerDefNew() (Laine Stump) - qemu: reorganize loop in qemuDomainAssignPCIAddresses (Laine Stump) - conf: pay attention to bus minSlot/maxSlot when autoassigning PCI addresses (Laine Stump) - tests: add vol-qcow2-zerocapacity test to storagevolxml2argvtest (Chris J Arges) - Revert "qemu: Use heads parameter for QXL driver" (Martin Kletzander) - test: introduce a function in test driver to check get vcpupin info (Luyao Huang) - conf: reorganize virNetworkDHCPDefParseXML (Laine Stump) - nodeinfo: Check for errors when reading core_id (Andrea Bolognani) - bhyve: add UTC clock support (Roman Bogorodskiy) - configure: clarify rationale for checking pkcheck (Daniel P. Berrange) - nodeinfo: Calculate present and online CPUs only
Re: [libvirt] [PATCHv3 06/13] conf: new pci controller model "pcie-root-port"
On Mon, Aug 03, 2015 at 12:17:45PM +0200, Martin Kletzander wrote: On Sat, Jul 25, 2015 at 03:58:30PM -0400, Laine Stump wrote: This controller can be connected (at domain startup time only - not hotpluggable) only to a port on the pcie root complex ("pcie-root" in libvirt config), hence the new connect type VIR_PCI_CONNECT_TYPE_PCIE_ROOT. It provides a hotpluggable port that will accept any PCI or PCIe device. New attributes must be added to the controller subelement for this - chassis and port are guest-visible option values that will be set by libvirt with values derived from the controller's index and pci address information. --- change from V2: * check chassis/port for 0-255 range * 1.3.0 -> 1.2.18 docs/formatdomain.html.in | 33 +- docs/schemas/domaincommon.rng | 13 src/conf/domain_addr.c | 10 ++- src/conf/domain_addr.h | 5 +- src/conf/domain_conf.c | 75 +- src/conf/domain_conf.h | 8 ++- src/qemu/qemu_command.c| 1 + .../qemuxml2argv-pcie-root-port.xml| 36 +++ tests/qemuxml2xmltest.c| 1 + 9 files changed, 173 insertions(+), 9 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pcie-root-port.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index fdf7e82..a9db924 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3123,6 +3141,17 @@ auto-determined by libvirt will be placed on this pci-bridge device. (since 1.1.2). + + Domains with an implicit pcie-root can also add controllers + with model='pcie-root-port'. This is a simple type of + bridge device that can connect only to one of the 31 slots on + the pcie-root bus on the upstream side, and makes a single + (PCIe, hotpluggable) port (at slot='0') available on the + downstream side. This controller can be used to provide a single + slot to later hotplug a PCIe device (but is not itself + hotpluggable - it must be in the configuration when the domain + is started). (since 1.2.18) s/18/19/ diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index fbad7e9..f1723c0 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7972,6 +7980,57 @@ virDomainControllerDefParseXML(xmlNodePtr node, goto error; } } +if (chassis) { +if (virStrToLong_i(chassis, NULL, 0, + &def->opts.pciopts.chassis) < 0) { +virReportError(VIR_ERR_XML_ERROR, + _("Invalid chassis '%s' in PCI controller"), + chassis); +goto error; +} +if (def->opts.pciopts.chassis < 0 || +def->opts.pciopts.chassisNr > 255) { +virReportError(VIR_ERR_XML_ERROR, + _("PCI controller chassis '%s' out of range " + "- must be 0-255"), + chassis); +goto error; +} +} This gets parsed twice, up here ^^ and down here vv, copy-paste error? Remove one of those, please. I, personally don't care much which one, none of them is my favourite :) Well, John has a favourite, though. And it makes sense as well... The upper one is comparing chassisNr > 255 but it should be 'chassis' instead, I totally missed that. Thanks John! ACK with that changed +if (chassis) { +if (virStrToLong_i(chassis, NULL, 0, + &def->opts.pciopts.chassis) < 0) { +virReportError(VIR_ERR_XML_ERROR, + _("Invalid chassis '%s' in PCI controller"), + chassis); +goto error; +} +if (def->opts.pciopts.chassis < 0 || +def->opts.pciopts.chassis > 255) { +virReportError(VIR_ERR_XML_ERROR, + _("PCI controller chassis '%s' out of range " + "- must be 0-255"), + chassis); +goto error; +} +} +if (port) { +if (virStrToLong_i(port, NULL, 0, + &def->opts.pciopts.port) < 0) { +virReportError(VIR_ERR_XML_ERROR, + _("Invalid port '%s' in PCI controller"), + port); +goto error; +} +if (def->opts.pciopts.port < 0 || +def->opts.pciopts.port > 255) { +virReportError(VIR_ERR_XML_ERROR, + _("PCI controller port '%s' out of range " +
Re: [libvirt] [PATCHv3 00/13] Add new PCIe controllers
On Sat, Jul 25, 2015 at 03:58:24PM -0400, Laine Stump wrote: Since the first 3 patches of V2 were ACKed and uncontroversial, I fixed the small problems pointed out in the reviews and pushed them. Thus, Patches 01-13 here correspond to Patches 04-17 in V2. 4 patches with the DHCP rework that gotten there by mistake :-) It took me a while to find that out. Most of these patches were already ACKed in V2 (pending my making small fixes pointed out in review), but the main things that need review are: 1) changing of model name from a char* to an enum in Patch 01, and corresponding blowback in patches 02, 07, 10, and 13 2) range checking of chassisNr in Patch 03 and chassis+port in patch 06. 3) check for duplicate in patch 01, and duplicate in patch 03. I did add one new negative test, and reworded some documentation, but I'm I haven't found one, but it's not needed, that was just a suggestion from some ignorant guy I guess (me). about to go mostly offline for 10 days, and would rather not have these patches bitrotting during that time if they are okay other than that. (also, I see both of those tasks as having no practical end, but do give my word to add more to both in later followups). If by chance everything is ACKed before DV freezes for RC1, but after I'm already offline (which will happen Sunday morning U.S. east coast time), I would appreciate if the reviewer could push the patches so they'll get the RC testing and be in the 1.2.18 release. (If not, I'll take care of it when I return). Unfortunately, I was mostly away for the whole week as well, I got to reading my mail for few minutes a day. And I haven't managed to go through this over the weekend, I figured since rc2 was out already, this needs to wait anyway. Only patches 06 and 12 have some needed work in, all other mails are just either suggestions for future re-factors or random rants. ACK series with a) all 1.2.18 occurrences changed to 1.2.19 (I've probably missed most of them) and b) reviews for 6 and 12 worked in. Have a nice day, Martin signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Remove double unlock for domains
On Wed, Jul 15, 2015 at 09:17:46AM +0200, Martin Kletzander wrote: The virDomainObjListRemove() function unlocks a domain that it's given due to legacy code. And because of that code, which should be refactored, that last virObjectUnlock() cannot be just removed. So instead, lock it right back for qemu for now. All calls to qemuDomainRemoveInactive() are followed by code that unlocks the domain again, plus the domain should be locked during qemuDomainObjEndJob(), so the right place to lock it is right after virDomainObjListRemove(). The only place where this would cause a problem is the autodestroy callback, so we need to get another reference there and uref+unlock it afterwards. Luckily, returning NULL from that function doesn't mean an error, and only means that it doesn't need to be unlocked anymore. Signed-off-by: Martin Kletzander --- Post-release ping src/qemu/qemu_domain.c | 7 +++ src/qemu/qemu_process.c | 7 --- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 8b050a043995..d6d723112638 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2593,6 +2593,13 @@ qemuDomainRemoveInactive(virQEMUDriverPtr driver, virObjectRef(vm); virDomainObjListRemove(driver->domains, vm); +/* + * virDomainObjListRemove() leaves the domain unlocked so it can + * be unref'd for other drivers that depend on that, but we still + * need to reset a job and we have a reference from the API that + * called this function. So we need to lock it back. + */ +virObjectLock(vm); virObjectUnref(cfg); if (haveJob) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 1c0c734c3811..df38dacdca92 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5696,6 +5696,8 @@ qemuProcessAutoDestroy(virDomainObjPtr dom, VIR_DEBUG("vm=%s, conn=%p", dom->def->name, conn); +virObjectRef(dom); + if (priv->job.asyncJob) { VIR_DEBUG("vm=%s has long-term job active, cancelling", dom->def->name); @@ -5718,15 +5720,14 @@ qemuProcessAutoDestroy(virDomainObjPtr dom, qemuDomainObjEndJob(driver, dom); -if (!dom->persistent) { +if (!dom->persistent) qemuDomainRemoveInactive(driver, dom); -dom = NULL; -} if (event) qemuDomainEventQueue(driver, event); cleanup: +virDomainObjEndAPI(&dom); return dom; } -- 2.4.5 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/3] qemu: Return true pinning info
On Sun, Jul 26, 2015 at 06:57:03PM +0200, Martin Kletzander wrote: First two patches just prepare the ground for the third one that explains what needs to be fixed and ho it's done. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1162947 Post-release ping Martin Kletzander (3): conf: Pass private data to Parse function of XML options qemu: Keep numad hint after reboot qemu: Return true pining info when using numad src/conf/domain_conf.c | 2 +- src/conf/domain_conf.h | 17 + src/libxl/libxl_domain.c | 3 ++- src/lxc/lxc_domain.c | 3 ++- src/qemu/qemu_domain.c | 33 - src/qemu/qemu_driver.c | 11 +++ 6 files changed, 57 insertions(+), 12 deletions(-) -- 2.4.6 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v3] rpc: Remove keepalive_required option
Since its introduction in 2011 (particularly in commit f4324e329275), the option doesn't work. It just effectively disables all incoming connections. That's because the client private data that contain the 'keepalive_supported' boolean, are initialized to zeroes so the bool is false and the only other place where the bool is used is when checking whether the client supports keepalive. Thus, according to the server, no client supports keepalive. Removing this instead of fixing it is better because a) apparently nobody ever tried it since 2011 (4 years without one month) and b) we cannot know whether the client supports keepalive until we get a ping or pong keepalive packet. And that won't happen untile after we dispatched the ConnectOpen call. Another two reasons would be c) the keepalive_required was tracked on the server level, but keepalive_supported was in private data of the client as well as the check that was made in the remote layer, thus making all other instances of virNetServer miss this feature unless they all implemented it for themselves and d) we can always add it back in case there is a request and a use-case for it. Signed-off-by: Martin Kletzander --- v3: - Kept the config option in place the same way we did with log_buffer_size - Moved the deprecated options together v2: - Added a new daemontest file with current input/output. daemon/libvirtd-config.c | 4 - daemon/libvirtd-config.h | 2 - daemon/libvirtd.c | 2 - daemon/libvirtd.conf | 9 +- daemon/libvirtd.h | 1 - daemon/remote.c| 8 +- daemon/test_libvirtd.aug.in| 2 +- src/libvirt_remote.syms| 1 - src/locking/lock_daemon.c | 2 +- src/lxc/lxc_controller.c | 2 +- src/rpc/virnetserver.c | 25 + src/rpc/virnetserver.h | 3 - tests/libvirtdconftest.c | 4 +- .../input-data-no-keepalive-required.json | 124 + .../virnetdaemondata/output-data-admin-nomdns.json | 2 - .../virnetdaemondata/output-data-anon-clients.json | 1 - .../output-data-initial-nomdns.json| 1 - tests/virnetdaemondata/output-data-initial.json| 1 - .../output-data-no-keepalive-required.json | 124 + tests/virnetdaemontest.c | 2 +- 20 files changed, 262 insertions(+), 58 deletions(-) create mode 100644 tests/virnetdaemondata/input-data-no-keepalive-required.json create mode 100644 tests/virnetdaemondata/output-data-no-keepalive-required.json diff --git a/daemon/libvirtd-config.c b/daemon/libvirtd-config.c index 10dcc423d2db..c31c8b2e9ab5 100644 --- a/daemon/libvirtd-config.c +++ b/daemon/libvirtd-config.c @@ -292,7 +292,6 @@ daemonConfigNew(bool privileged ATTRIBUTE_UNUSED) data->keepalive_interval = 5; data->keepalive_count = 5; -data->keepalive_required = 0; data->admin_min_workers = 5; data->admin_max_workers = 20; @@ -302,7 +301,6 @@ daemonConfigNew(bool privileged ATTRIBUTE_UNUSED) data->admin_keepalive_interval = 5; data->admin_keepalive_count = 5; -data->admin_keepalive_required = 0; localhost = virGetHostname(); if (localhost == NULL) { @@ -471,11 +469,9 @@ daemonConfigLoadOptions(struct daemonConfig *data, GET_CONF_INT(conf, filename, keepalive_interval); GET_CONF_UINT(conf, filename, keepalive_count); -GET_CONF_UINT(conf, filename, keepalive_required); GET_CONF_INT(conf, filename, admin_keepalive_interval); GET_CONF_UINT(conf, filename, admin_keepalive_count); -GET_CONF_UINT(conf, filename, admin_keepalive_required); return 0; diff --git a/daemon/libvirtd-config.h b/daemon/libvirtd-config.h index 9cdae1a0cb59..3e1971d67f05 100644 --- a/daemon/libvirtd-config.h +++ b/daemon/libvirtd-config.h @@ -81,7 +81,6 @@ struct daemonConfig { int keepalive_interval; unsigned int keepalive_count; -int keepalive_required; int admin_min_workers; int admin_max_workers; @@ -91,7 +90,6 @@ struct daemonConfig { int admin_keepalive_interval; unsigned int admin_keepalive_count; -int admin_keepalive_required; }; diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 71db4a042c7f..250094bd21dd 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -1389,7 +1389,6 @@ int main(int argc, char **argv) { config->max_anonymous_clients, config->keepalive_interval, config->keepalive_count, -!!config->keepalive_required, config->mdns_adv ? config->mdns_name : NULL,
Re: [libvirt] [PATCH v2 00/12] Adjust SCSI generated device address checks
On 07/22/2015 10:54 AM, John Ferlan wrote: > v1 here: > http://www.redhat.com/archives/libvir-list/2015-June/msg01104.html > > Some followups into July resulted in the request to move the Hostdev > and Disk default (or _NONE) address creation/assignment into domain/ > device post processing rather than during XML parsing. > > Changes in v2 are numerous, quite a bit of patch and code motion in order > to accomplish the requested task in small enough and reviewable chunks > > John Ferlan (12): > conf: Remove extraneous check in virDomainHostdevAssignAddress > conf: Add 'bus' and 'target' to SCSI address conflict checks > conf: Move hostdev and disk address validations > conf: Add xmlopt to virDomainDeviceDefPostParseInternal > conf: Add check for host address type while checking in use > conf: Try controller add when searching hostdev bus for unit > conf: Change when virDomainHostdevAssignAddress is called > conf: Remove unused param from virDomainHostdevDefParseXML > conf: Add SCSI hostdev check for disk drive address already in use > conf: Change when virDomainDiskDefAssignAddress is called > conf: Create locals for virDomainDiskDefAssignAddress > conf: Check for hostdev conflicts when assign default disk address > > docs/formatdomain.html.in | 4 +- > src/conf/domain_conf.c| 396 > +++--- > src/conf/domain_conf.h| 3 +- > src/qemu/qemu_command.c | 4 +- > src/vmx/vmx.c | 22 +-- > src/vmx/vmx.h | 3 +- > 6 files changed, 253 insertions(+), 179 deletions(-) > ping Tks, John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/2] Adjustments for configuring volume lun device
On 07/18/2015 07:43 AM, John Ferlan wrote: > Resolve a couple of issues regarding failures seen configuring a disk > to be a type='volume' device='lun'. The doc patch just indicates that > using an NPIV storage/source pool is a valid option. The second patch > allows for a "clearer" error message to be reported. > > > John Ferlan (2): > docs: Add Fibre Channel NPIV supported option for volume lun config > conf: Allow error reporting in virDomainDiskSourceIsBlockType > > docs/formatdomain.html.in | 6 -- > src/conf/domain_conf.c| 21 ++--- > src/conf/domain_conf.h| 2 +- > src/lxc/lxc_cgroup.c | 2 +- > src/lxc/lxc_driver.c | 6 ++ > src/qemu/qemu_command.c | 5 + > src/qemu/qemu_conf.c | 6 +++--- > 7 files changed, 30 insertions(+), 18 deletions(-) > ping tks, John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv3 00/13] Add new PCIe controllers
On 07/25/2015 03:58 PM, Laine Stump wrote: > Since the first 3 patches of V2 were ACKed and uncontroversial, I > fixed the small problems pointed out in the reviews and pushed > them. Thus, Patches 01-13 here correspond to Patches 04-17 in V2. > > Most of these patches were already ACKed in V2 (pending my making > small fixes pointed out in review), but the main things that need > review are: > > 1) changing of model name from a char* to an enum in Patch 01, and >corresponding blowback in patches 02, 07, 10, and 13 > > 2) range checking of chassisNr in Patch 03 and chassis+port in patch >06. > > 3) check for duplicate in patch 01, and duplicate in >patch 03. > > I did add one new negative test, and reworded some documentation, but I'm > about to go mostly offline for 10 days, and would rather not have > these patches bitrotting during that time if they are okay other than > that. (also, I see both of those tasks as having no practical end, but > do give my word to add more to both in later followups). > > If by chance everything is ACKed before DV freezes for RC1, but after > I'm already offline (which will happen Sunday morning U.S. east coast > time), I would appreciate if the reviewer could push the patches so > they'll get the RC testing and be in the 1.2.18 release. (If not, I'll > take care of it when I return). > > P.S. I ran "git rebase -i master -x "make -j8 check && make -j8 > syntax-check" before sending. > Like Martin noted, I've been looking through this series as well. I too haven't had a ton of time to devote to it over the last week or so since I'm in the middle of a move. Also being so close to freeze - it just didn't feel right to shimmy it in there late. This should go in as soon as possible for 1.2.19 to hopefully weed out any weird issues! Some more specific/general comments... The 1.2.18 -> 1.2.19 everywhere will need to be done. The commit message in 2/13 seems to have incurred a disjoint thought or perhaps a forgotten edit in the paragraph that starts "The defaults are set..." - the last sentance just doesn't read well. I had some concern over confusion with 'chassis' and 'chassisNr'... I also had concern over whether the parser could confuse chassis values depending on order seen - as in was a parse of 'chassis' an exact match or a prefix match... I think it's ok especially since one is used for pci-bridge and the other for pcie-root-port... I'm still not sure if it's "best" to set chassisNr or make it some sort of tristate where if we know it wasn't read in from the XML, then we default or use the 'cont->idx' internally when formulating the qemu command. That way at least we don't run into the situation where if "index" changes, then we don't run into some issue since we don't follow on with the chassisNr change (because we cannot be sure who set it). This is patch 4/13 stuff. I think it's fine as a followup to adjust that logic if necessary - hopefully something that can be done in the 1.2.19 timeframe though. This would change the logic in qemuBuildControllerDevStr in the def->model switch for VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE to perhaps set a local chassisNr value based on opts.pciopts.chassisNr == -1, then use index, else use what was set. Beyond that and the duplicate 'chassis' parse in patch 6, things were find and ready to push with a few adjustments. Since I know you're on the road too, I'll watch to see if you get to it - if not, I will make a couple of adjustments for you. John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] ambiguous ret of qemuDomainDetachVirtioDiskDevice
On Fri, Jul 31, 2015 at 08:12:01AM +0800, zhang bo wrote: > On 2015/7/30 17:41, zhang bo wrote: > > > On 2015/7/28 16:33, Ján Tomko wrote: > > > >> On Tue, Jul 28, 2015 at 04:25:13PM +0800, zhang bo wrote: > >>> static int > >>> qemuDomainDetachVirtioDiskDevice(virQEMUDriverPtr driver, > >>> virDomainObjPtr vm, > >>> virDomainDiskDefPtr detach) > >>> { > >>> ... > >>> > >>> rc = qemuDomainWaitForDeviceRemoval(vm); > >>> if (rc == 0 || rc == 1) > >>> ret = qemuDomainRemoveDiskDevice(driver, vm, detach); > >>> else > >>> ret = 0; /*the return value of 2 is dismissed here, which refers > >>> to ETIMEOUT.*/ > >>> > >>> } > >>> > >>> > >>> > >>> If it timeouts when qemu tries to del the device, the return value would > >>> be modified from 2 to 0 in > >>> function qemuDomainDetachVirtioDiskDevice(), which means that, the users > >>> would be misleaded that > >>> the device has been deleted, however, the device maybe probably failed to > >>> be detached after timeout and > >>> still in use. > >>> > >> > >> This is intentional and documented: > >> http://libvirt.org/html/libvirt-libvirt-domain.html#virDomainDetachDeviceFlags > >> > >> Unplugging a disk requires guest cooperation, so the best we can do is > >> ask qemu to unplug it and wait for a while. > >> > >>> That is to say, the function qemuDomainDetachVirtioDiskDevice()'s return > >>> value is ambiguous when it's 0, > >>> maybe successful, or timeout. Will it be better to pass ETIMEOUT to user? > >>> or any other advises? for example, > >>> let users themselves dumpxml the guest to check whether the device has > >>> been actually detached or not? > >> > >> Either dump the XML, or wait for the VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED > >> event, as the API documentation suggests. > >> > >> Jan > > > > > > It seems to have fixed the problem by dumping the XML or wait for the > > DEVICE_REMOVED event. However, it seems to > > make nova or other apps to do more checking work, they need to dump XML or > > wait the event even if the device has > > already been successfully removed. which is unnecessary. > > > > I think, it's better to return ETIMEOUT and let nova to dumpxml or wait the > > event at this situation, rather than always > > doing that job. > > The app would have to listen to the event before issuing the API - otherwise the event might arrive after the client app processes the ETIMEOUT return, but before it registers the event. I think in this case it's simpler to process the event regardless of the return value. > > It maybe a better design, what's your opinion? > > > > After thinking twice, it's an async job, thus returning 0 is acceptable, > right? > Yes, that was the intention of the patch adding waiting for the event. But for the most cases, where the guest unplugs the device under 5 seconds, the API is synchronnous. Before that, the API returned 0 regardless of whether the device was unplugged or not, so this did not make matters any worse. Jan signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 2/3] Avoid starting a PowerPC VM with floppy disk
On Thu, Jul 30, 2015 at 07:55:36AM -0400, Kothapally Madhu Pavan wrote: > PowerPC pseries based VMs do not support a floppy disk controller. Here the commit message mentions a controller, but the code only checks for the device, not for Should we forbid that one too? AFAIK we've auto-added it to the XML only if there was at least floppy. Either way, the series looks good to me. Jan > This prohibits libvirt from creating qemu command with floppy device. > > Signed-off-by: Kothapally Madhu Pavan > --- > src/qemu/qemu_command.c |8 > 1 file changed, 8 insertions(+) > > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index 09f30c4..501c7df 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -9767,6 +9767,14 @@ qemuBuildCommandLine(virConnectPtr conn, > continue; > } > > +/* PowerPC pseries based VMs do not support floppy device */ > +if ((disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) && > +ARCH_IS_PPC64(def->os.arch) && STRPREFIX(def->os.machine, > "pseries")) { > +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("PowerPC pseries machines do not support > floppy device")); > +goto error; > +} > + > switch (disk->device) { > case VIR_DOMAIN_DISK_DEVICE_CDROM: > bootindex = bootCD; > > -- > libvir-list mailing list > libvir-list@redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/2] qemu: Improve memory alignment handling and fix upcoming powerpc
On Mon, Aug 03, 2015 at 11:02:49 +0200, Andrea Bolognani wrote: > On Fri, 2015-07-31 at 16:49 +0200, Peter Krempa wrote: > > > > Peter Krempa (2): > > qemu: Make memory alignment helper more universal > > qemu: ppc64: Align memory sizes to 256MiB > > > > src/qemu/qemu_domain.c | 33 > > -- > > src/qemu/qemu_domain.h | 3 +- > > src/qemu/qemu_hotplug.c| 4 +-- > > .../qemuxml2argv-pseries-cpu-compat.args | 2 +- > > 4 files changed, 30 insertions(+), 12 deletions(-) > > Looks okay overall and works as expected from my limited > testing on ppc64. > > One thing, though: the domain XML is not updated with the > rounded-up value, which I think might be a little bit > confusing to the user. Any chance the rounding up could > be reflected there as well? The rounding will be reflected once you start the VM in the live XML. This is also the case with the 1MiB rounding that is currently used, but it's not as prominent as the 256MiB granularity. Peter signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Remove double unlock for domains
On 07/15/2015 03:17 AM, Martin Kletzander wrote: > The virDomainObjListRemove() function unlocks a domain that it's given > due to legacy code. And because of that code, which should be > refactored, that last virObjectUnlock() cannot be just removed. So > instead, lock it right back for qemu for now. All calls to > qemuDomainRemoveInactive() are followed by code that unlocks the domain > again, plus the domain should be locked during qemuDomainObjEndJob(), so > the right place to lock it is right after virDomainObjListRemove(). > Looked through callers - seems qemuProcessHandleMonitorEOF may need a tweak too as it skips around the virObjectUnlock(vm) My only other comment would be perhaps the entry comment into qemuDomainRemoveInactive could use a tweak on the wording and some additional text to indicate on exit it should still hold the lock (or just move the blob you added... Perhaps an XXX to force a revisit in the future based on your note above "which should be refactored" Not that anyone reads those anyway ;-) ACK with the double check on the *EOF function... John > The only place where this would cause a problem is the autodestroy > callback, so we need to get another reference there and uref+unlock it > afterwards. Luckily, returning NULL from that function doesn't mean an > error, and only means that it doesn't need to be unlocked anymore. > > Signed-off-by: Martin Kletzander > --- > src/qemu/qemu_domain.c | 7 +++ > src/qemu/qemu_process.c | 7 --- > 2 files changed, 11 insertions(+), 3 deletions(-) > > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c > index 8b050a043995..d6d723112638 100644 > --- a/src/qemu/qemu_domain.c > +++ b/src/qemu/qemu_domain.c > @@ -2593,6 +2593,13 @@ qemuDomainRemoveInactive(virQEMUDriverPtr driver, > virObjectRef(vm); > > virDomainObjListRemove(driver->domains, vm); > +/* > + * virDomainObjListRemove() leaves the domain unlocked so it can > + * be unref'd for other drivers that depend on that, but we still > + * need to reset a job and we have a reference from the API that > + * called this function. So we need to lock it back. > + */ > +virObjectLock(vm); > virObjectUnref(cfg); > > if (haveJob) > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c > index 1c0c734c3811..df38dacdca92 100644 > --- a/src/qemu/qemu_process.c > +++ b/src/qemu/qemu_process.c > @@ -5696,6 +5696,8 @@ qemuProcessAutoDestroy(virDomainObjPtr dom, > > VIR_DEBUG("vm=%s, conn=%p", dom->def->name, conn); > > +virObjectRef(dom); > + > if (priv->job.asyncJob) { > VIR_DEBUG("vm=%s has long-term job active, cancelling", >dom->def->name); > @@ -5718,15 +5720,14 @@ qemuProcessAutoDestroy(virDomainObjPtr dom, > > qemuDomainObjEndJob(driver, dom); > > -if (!dom->persistent) { > +if (!dom->persistent) > qemuDomainRemoveInactive(driver, dom); > -dom = NULL; > -} > > if (event) > qemuDomainEventQueue(driver, event); > > cleanup: > +virDomainObjEndAPI(&dom); > return dom; > } > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 07/12] conf: Change when virDomainHostdevAssignAddress is called
On Wed, Jul 22, 2015 at 10:54:29AM -0400, John Ferlan wrote: > Rather than calling virDomainHostdevAssignAddress during the parsing > of the XML, move the setting of a default hostdev address to domain/ > device post processing. > > Since the parse code no longer generates an address, we can remove > the virDomainDefMaybeAddHostdevSCSIcontroller since the call to > virDomainHostdevAssignAddress will attempt to add the controllers > that were not already defined in the XML. > > This patch will also enforce that the address type is type 'drive' > when a SCSI subsystem element is provided with an . > > Signed-off-by: John Ferlan > --- > docs/formatdomain.html.in | 4 ++-- > src/conf/domain_conf.c| 29 +++-- > 2 files changed, 21 insertions(+), 12 deletions(-) > > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in > index d0c1741..e78fb26 100644 > --- a/docs/formatdomain.html.in > +++ b/docs/formatdomain.html.in > @@ -3355,8 +3355,8 @@ >(starting with 0x) or octal (starting with 0) form. >For PCI devices the element carries 4 attributes allowing to designate >the device as can be found with the lspci or > - with virsh > - nodedev-list. See above for > + with virsh nodedev-list. For SCSI devices a 'drive' > + address type must be used. See above for >more details on the address element. >driver > > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index 9c6c739..eabba68 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -3997,7 +3997,7 @@ static int > virDomainDeviceDefPostParseInternal(virDomainDeviceDefPtr dev, > const virDomainDef *def, > virCapsPtr caps ATTRIBUTE_UNUSED, > -virDomainXMLOptionPtr xmlopt > ATTRIBUTE_UNUSED) > +virDomainXMLOptionPtr xmlopt) > { > if (dev->type == VIR_DOMAIN_DEVICE_CHR) { > virDomainChrDefPtr chr = dev->data.chr; > @@ -4085,6 +4085,18 @@ > virDomainDeviceDefPostParseInternal(virDomainDeviceDefPtr dev, > video->vram = VIR_ROUND_UP_POWER_OF_TWO(video->vram); > } > > +if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV) { > +virDomainHostdevDefPtr hdev = dev->data.hostdev; > + > +if (hdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && > +hdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI > && > +hdev->info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && > +virDomainHostdevAssignAddress(xmlopt, def, hdev) < 0) { > +virReportError(VIR_ERR_XML_ERROR, "%s", > + _("Cannot assign SCSI host devices address > ")); host device's? There's also a trailing space. Jan signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v11 0/5] nodeinfo: Add support for subcores
On 07/30/2015 05:37 AM, Andrea Bolognani wrote: > Only patch 1/5 has been updated, all the other patches > are the same as v8: > > 2/5 https://www.redhat.com/archives/libvir-list/2015-July/msg01045.html > 3/5 https://www.redhat.com/archives/libvir-list/2015-July/msg01048.html > 4/5 https://www.redhat.com/archives/libvir-list/2015-July/msg01049.html > 5/5 https://www.redhat.com/archives/libvir-list/2015-July/msg01050.html > > I'm including the full history below. > > Cheers. > > > Changes in v11: > > * don't declare variables only used inside a code block > guarded by #ifdef outside of said code block > > * use virBitmapNextSetBit() instead of going through > all possible index values and calling > virBitmapIsBitSet() for each one > > Changes in v10: > > * don't attempt to close a file that wasn't opened > > * report a detailed error to help with diagnosis > > Changes in v9: > > * take into account the fact that kvm might not be loaded > or even installed > > Changes in v8: > > * shortened test names so that make dist doesn't > stop working again > > Changes in v7: > > * rebased on top of master now that the series this one > builds on have been merged > > Changes in v6: > > * updated to work on top of > > [PATCH v2 00/10] nodeinfo: Various cleanups > > Changes in v5: > > * streamlined the logic used to decide whether the subcore > configuration is valid and moved it to a separate function > > * split the tests into separate commits for easier review and > to hopefully avoid having trouble with the list due to the > message size > > Changes in v4: > > * removed a printf() statement > > * fixed typo in a commit message > > Changes in v3: > > * the function to get the number of threads per subcore > has been moved to the from virarch.c, which deals with > architecture names only and is therefore not the right > place to read host configuration, to nodeinfo.c where > the rest of this stuff lives > > * said function has also been given a shorter name > > * the "valid subcore mode" boolean has been removed: > threads_per_subcore will be a positive number if > subcores should be taken into account, and if that's > not the case (x86 host, tainted configuration) it > will simply be zero, so now the code needs to keep > track of a single variable instead of two > > * the test case has been renamed to be more > descriptive > > * the test data has been cleaned up by removing all > cpu/cpu*/node* links, which prevented 'make dist' > from working due to recursive linking > > > Andrea Bolognani (3): > tests: Add subcores1 nodeinfo test > tests: Add subcores2 nodeinfo test > tests: Add subcores3 nodeinfo test > > Shivaprasad G Bhat (2): > nodeinfo: Fix output on PPC64 KVM hosts > tests: Prepare for subcore tests > > src/libvirt_private.syms | 1 + > src/nodeinfo.c | 153 > - > src/nodeinfo.h | 1 + > tests/Makefile.am | 6 + > [...] > tests/nodeinfomock.c | 35 + > tests/nodeinfotest.c | 8 +- > 1348 files changed, 2134 insertions(+), 6 deletions(-) > [...] > create mode 100644 tests/nodeinfomock.c > Pulled in 2-5 from v8 series, rebuilt, checked, and pushed. Tks, John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 08/12] conf: Remove unused param from virDomainHostdevDefParseXML
On Wed, Jul 22, 2015 at 10:54:30AM -0400, John Ferlan wrote: > Remove unused xmlopt param > > Signed-off-by: John Ferlan > --- > src/conf/domain_conf.c | 11 +-- > 1 file changed, 5 insertions(+), 6 deletions(-) > ACK to patches 1 to 8. Jan signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 09/12] conf: Add SCSI hostdev check for disk drive address already in use
On Wed, Jul 22, 2015 at 10:54:31AM -0400, John Ferlan wrote: > https://bugzilla.redhat.com/show_bug.cgi?id=1210587 (partial) > > If a SCSI subsystem element address is provided, we need to > make sure the address provided doesn't conflict with an existing or > libvirt generated address for a SCSI element. > > This will fix the issue where the domain XML provided an for > the , but not the element where the address provided > ends up being the same address used for the . A address > is generated using it's assigned 'dev' name prior to the > check/validation of the address value. > > Signed-off-by: John Ferlan > --- > src/conf/domain_conf.c | 22 +- > 1 file changed, 21 insertions(+), 1 deletion(-) > > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index 44ce71b..eba264d 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -11872,7 +11872,7 @@ virDomainVideoDefParseXML(xmlNodePtr node, > } > > static virDomainHostdevDefPtr > -virDomainHostdevDefParseXML(const virDomainDef *vmdef ATTRIBUTE_UNUSED, > +virDomainHostdevDefParseXML(const virDomainDef *vmdef, > xmlNodePtr node, > xmlXPathContextPtr ctxt, > virHashTablePtr bootHash, > @@ -11939,6 +11939,26 @@ virDomainHostdevDefParseXML(const virDomainDef > *vmdef ATTRIBUTE_UNUSED, > _("SCSI host device must use 'drive' " > "address type")); > goto error; > +} else if (def->info->type == > + VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) { > +/* Ensure provided address doesn't conflict with existing > + * scsi disk drive address > + */ > +virDomainDeviceDriveAddressPtr addr = &def->info->addr.drive; > +if (virDomainDriveAddressIsUsedByDisk(vmdef, > + > VIR_DOMAIN_DISK_BUS_SCSI, > + addr->controller, > + addr->bus, > + addr->target, > + addr->unit)) { This check seems out of place in HostdevDefParse. It also does not check for conflicts with other hostdevs. Jan > +virReportError(VIR_ERR_XML_ERROR, > + _("SCSI host address controller='%u' " > + "bus='%u' target='%u' unit='%u' in " > + "use by a SCSI disk"), > + addr->controller, addr->bus, > + addr->target, addr->unit); > +goto error; > +} > } > > if (virXPathBoolean("boolean(./readonly)", ctxt)) > -- > 2.1.0 > > -- > libvir-list mailing list > libvir-list@redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 10/12] conf: Change when virDomainDiskDefAssignAddress is called
On Wed, Jul 22, 2015 at 10:54:32AM -0400, John Ferlan wrote: > Rather than calling virDomainDiskDefAssignAddress during the parsing of > the XML, moving the setting of disk addresses into the domain/device post > processing. > > Commit id '37588b25' which introduced VIR_DOMAIN_DEF_PARSE_DISK_SOURCE > in order to avoid generating the address which wasn't required will not > be affected by this as all it cared about was processing the source XML. > > Signed-off-by: John Ferlan > --- > src/conf/domain_conf.c | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) > ACK Jan > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index eba264d..1560823 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -4077,6 +4077,10 @@ > virDomainDeviceDefPostParseInternal(virDomainDeviceDefPtr dev, > disk->dst); > return -1; > } > + > +if (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && > +virDomainDiskDefAssignAddress(xmlopt, disk) < 0) > +return -1; > } > > if (dev->type == VIR_DOMAIN_DEVICE_VIDEO) { > @@ -7439,10 +7443,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, > } > > if (!(flags & VIR_DOMAIN_DEF_PARSE_DISK_SOURCE)) { > -if (def->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE > -&& virDomainDiskDefAssignAddress(xmlopt, def) < 0) > -goto error; > - > if (virDomainDiskBackingStoreParse(ctxt, def->src) < 0) > goto error; > } > -- > 2.1.0 > > -- > libvir-list mailing list > libvir-list@redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 12/12] conf: Check for hostdev conflicts when assign default disk address
On Wed, Jul 22, 2015 at 10:54:34AM -0400, John Ferlan wrote: > https://bugzilla.redhat.com/show_bug.cgi?id=1210587 (completed) > > When generating the default drive address for a SCSI device, > check the generated address to ensure it doesn't conflict with a SCSI > address. The address generation algorithm uses the > "dev" name in order to determine which controller and unit > in order to place the device. Since a SCSI device doesn't > require a target device name, its placement on the guest SCSI address > "could" conflict. For instance, if a SCSI exists at > controller=0 unit=0 and an attempt to hotplug 'sda' into the guest > made, there would be a conflict if the is already using > /dev/sda. > > Signed-off-by: John Ferlan > --- > src/conf/domain_conf.c | 16 ++-- > src/conf/domain_conf.h | 3 ++- > src/qemu/qemu_command.c | 4 ++-- > src/vmx/vmx.c | 22 -- > src/vmx/vmx.h | 3 ++- > 5 files changed, 32 insertions(+), 16 deletions(-) > > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index 39a4cf8..0dac60c 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -4079,7 +4079,7 @@ > virDomainDeviceDefPostParseInternal(virDomainDeviceDefPtr dev, > } > > if (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && > -virDomainDiskDefAssignAddress(xmlopt, disk) < 0) > +virDomainDiskDefAssignAddress(xmlopt, disk, def) < 0) > return -1; > } > > @@ -5860,7 +5860,8 @@ virDomainDiskDiffersSourceOnly(virDomainDiskDefPtr disk, > > int > virDomainDiskDefAssignAddress(virDomainXMLOptionPtr xmlopt, > - virDomainDiskDefPtr def) > + virDomainDiskDefPtr def, > + const virDomainDef *vmdef) This function does not really do any 'real' assignment, it just translates the target name to a fixed address. Unlike PCI address assignment, where we find unoccupied slots based on the domain defition, the domain definition is not really needed here. Moving the address check conflict to virDomainDefPostParse, after the addresses have been filled out by virDomainDeviceDefPostParse, would remove the need to change all the function prototypes and it would be a good place to check for address conflicts between disks too - after this series, two drives with the same addresses are allowed, as long as they have different target names. Jan > { > int idx = virDiskNameToIndex(def->dst); > if (idx < 0) { > @@ -5896,6 +5897,17 @@ virDomainDiskDefAssignAddress(virDomainXMLOptionPtr > xmlopt, > unit = idx % 7; > } > > +if (virDomainDriveAddressIsUsedByHostdev(vmdef, > + > VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI, > + controller, 0, 0, unit)) { > +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("using disk target name '%s' conflicts with " > + "SCSI host device address controller='%u' " > + "bus='%u' target='%u' unit='%u"), > + def->dst, controller, 0, 0, unit); > +return -1; > +} > + > def->info.addr.drive.controller = controller; > def->info.addr.drive.bus = 0; > def->info.addr.drive.target = 0; signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Remove double unlock for domains
On Mon, Aug 03, 2015 at 09:21:51AM -0400, John Ferlan wrote: On 07/15/2015 03:17 AM, Martin Kletzander wrote: The virDomainObjListRemove() function unlocks a domain that it's given due to legacy code. And because of that code, which should be refactored, that last virObjectUnlock() cannot be just removed. So instead, lock it right back for qemu for now. All calls to qemuDomainRemoveInactive() are followed by code that unlocks the domain again, plus the domain should be locked during qemuDomainObjEndJob(), so the right place to lock it is right after virDomainObjListRemove(). Looked through callers - seems qemuProcessHandleMonitorEOF may need a tweak too as it skips around the virObjectUnlock(vm) My only other comment would be perhaps the entry comment into qemuDomainRemoveInactive could use a tweak on the wording and some additional text to indicate on exit it should still hold the lock (or just move the blob you added... Perhaps an XXX to force a revisit in the future based on your note above "which should be refactored" Not that anyone reads those anyway ;-) ACK with the double check on the *EOF function... Would it be enough if I squashed this in? diff --git i/src/qemu/qemu_domain.c w/src/qemu/qemu_domain.c index d6d723112638..6ba8087c108b 100644 --- i/src/qemu/qemu_domain.c +++ w/src/qemu/qemu_domain.c @@ -2597,7 +2597,13 @@ qemuDomainRemoveInactive(virQEMUDriverPtr driver, * virDomainObjListRemove() leaves the domain unlocked so it can * be unref'd for other drivers that depend on that, but we still * need to reset a job and we have a reference from the API that - * called this function. So we need to lock it back. + * called this function. So we need to lock it back. This is + * just a workaround for the qemu driver. + * + * XXX: Ideally, the global handling of domain objects and object + * lists would be refactored so we don't need hacks like + * this, but since that requires refactor of all drivers, + * it's a work for another day. */ virObjectLock(vm); virObjectUnref(cfg); diff --git i/src/qemu/qemu_process.c w/src/qemu/qemu_process.c index 5a9f97bc8921..ddce248635b8 100644 --- i/src/qemu/qemu_process.c +++ w/src/qemu/qemu_process.c @@ -323,10 +323,8 @@ qemuProcessHandleMonitorEOF(qemuMonitorPtr mon ATTRIBUTE_UNUSED, qemuProcessStop(driver, vm, stopReason, stopFlags); virDomainAuditStop(vm, auditReason); -if (!vm->persistent) { +if (!vm->persistent) qemuDomainRemoveInactive(driver, vm); -goto cleanup; -} unlock: virObjectUnlock(vm); -- signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Remove double unlock for domains
On Mon, Aug 03, 2015 at 04:00:32PM +0200, Martin Kletzander wrote: On Mon, Aug 03, 2015 at 09:21:51AM -0400, John Ferlan wrote: On 07/15/2015 03:17 AM, Martin Kletzander wrote: The virDomainObjListRemove() function unlocks a domain that it's given due to legacy code. And because of that code, which should be refactored, that last virObjectUnlock() cannot be just removed. So instead, lock it right back for qemu for now. All calls to qemuDomainRemoveInactive() are followed by code that unlocks the domain again, plus the domain should be locked during qemuDomainObjEndJob(), so the right place to lock it is right after virDomainObjListRemove(). Looked through callers - seems qemuProcessHandleMonitorEOF may need a tweak too as it skips around the virObjectUnlock(vm) My only other comment would be perhaps the entry comment into qemuDomainRemoveInactive could use a tweak on the wording and some additional text to indicate on exit it should still hold the lock (or just move the blob you added... Perhaps an XXX to force a revisit in the future based on your note above "which should be refactored" Not that anyone reads those anyway ;-) ACK with the double check on the *EOF function... Would it be enough if I squashed this in? Sorry, incomplete diff, here's the proper one, it's cleaner and looks better (and compiles): diff --git i/src/qemu/qemu_domain.c w/src/qemu/qemu_domain.c index d6d723112638..6ba8087c108b 100644 --- i/src/qemu/qemu_domain.c +++ w/src/qemu/qemu_domain.c @@ -2597,7 +2597,13 @@ qemuDomainRemoveInactive(virQEMUDriverPtr driver, * virDomainObjListRemove() leaves the domain unlocked so it can * be unref'd for other drivers that depend on that, but we still * need to reset a job and we have a reference from the API that - * called this function. So we need to lock it back. + * called this function. So we need to lock it back. This is + * just a workaround for the qemu driver. + * + * XXX: Ideally, the global handling of domain objects and object + * lists would be refactored so we don't need hacks like + * this, but since that requires refactor of all drivers, + * it's a work for another day. */ virObjectLock(vm); virObjectUnref(cfg); diff --git i/src/qemu/qemu_process.c w/src/qemu/qemu_process.c index 5a9f97bc8921..505778ec2f05 100644 --- i/src/qemu/qemu_process.c +++ w/src/qemu/qemu_process.c @@ -295,12 +295,12 @@ qemuProcessHandleMonitorEOF(qemuMonitorPtr mon ATTRIBUTE_UNUSED, if (priv->beingDestroyed) { VIR_DEBUG("Domain is being destroyed, EOF is expected"); -goto unlock; +goto cleanup; } if (!virDomainObjIsActive(vm)) { VIR_DEBUG("Domain %p is not active, ignoring EOF", vm); -goto unlock; +goto cleanup; } if (priv->monJSON && !priv->gotShutdown) { @@ -323,15 +323,11 @@ qemuProcessHandleMonitorEOF(qemuMonitorPtr mon ATTRIBUTE_UNUSED, qemuProcessStop(driver, vm, stopReason, stopFlags); virDomainAuditStop(vm, auditReason); -if (!vm->persistent) { +if (!vm->persistent) qemuDomainRemoveInactive(driver, vm); -goto cleanup; -} - - unlock: -virObjectUnlock(vm); cleanup: +virObjectUnlock(vm); if (event) qemuDomainEventQueue(driver, event); } -- Martin signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/2] qemu: Improve memory alignment handling and fix upcoming powerpc
On Mon, 2015-08-03 at 15:13 +0200, Peter Krempa wrote: > > > Looks okay overall and works as expected from my limited > > testing on ppc64. > > > > One thing, though: the domain XML is not updated with the > > rounded-up value, which I think might be a little bit > > confusing to the user. Any chance the rounding up could > > be reflected there as well? > > The rounding will be reflected once you start the VM in the live XML. > This is also the case with the 1MiB rounding that is currently used, > but > it's not as prominent as the 256MiB granularity. Let's file updating the on-disk XML under "would be nice to have" then. ACK series. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] docs: Add Fibre Channel NPIV supported option for volume lun config
On Sat, Jul 18, 2015 at 07:43:09AM -0400, John Ferlan wrote: "Further" clarification (and testing) shows that using a SCSI Fibre Channel NPIV device/lun from a storage pool as a will work. So just add that to the allowable options Related to: https://bugzilla.redhat.com/show_bug.cgi?id=1230179 Signed-off-by: John Ferlan --- docs/formatdomain.html.in | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) Since I have no way how to test this I must trust you. So ACK to that. diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index d0c1741..cf697f3 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1972,8 +1972,10 @@ Using "lun" (since 0.9.10) is only valid when the type is "block" or "network" for protocol='iscsi' or when the type -is "volume" when using an iSCSI source pool -for mode "host". +is "volume" when using an iSCSI storage pool +for mode "host" or as an +http://wiki.libvirt.org/page/NPIV_in_libvirt";>NPIV +virtual Host Bus Adapter (vHBA) using a Fibre Channel storage pool. Configured in this manner, the LUN behaves identically to "disk", except that generic SCSI commands from the guest are accepted and passed through to the physical device. Also note that -- 2.1.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] conf: Allow error reporting in virDomainDiskSourceIsBlockType
On Sat, Jul 18, 2015 at 07:43:10AM -0400, John Ferlan wrote: Rather than provide a somewhat generic error message when the API returns false, allow the caller to supply a "report = true" option in order to cause virReportError's to describe which of the 3 paths that can cause failure. Some callers don't care about what caused the failure, they just want to have a true/false - for those, calling with report = false should be sufficient. Signed-off-by: John Ferlan --- src/conf/domain_conf.c | 21 ++--- src/conf/domain_conf.h | 2 +- src/lxc/lxc_cgroup.c| 2 +- src/lxc/lxc_driver.c| 6 ++ src/qemu/qemu_command.c | 5 + src/qemu/qemu_conf.c| 6 +++--- 6 files changed, 26 insertions(+), 16 deletions(-) diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 81bb711..913e007 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -4069,11 +4069,9 @@ lxcDomainAttachDeviceDiskLive(virLXCDriverPtr driver, goto cleanup; } -if (!virDomainDiskSourceIsBlockType(def->src)) { -virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("Can't setup disk for non-block device")); +if (!virDomainDiskSourceIsBlockType(def->src, false)) Even though some don't like this reporting booleans, I think this one is used properly. But you probably meant "true" in this hunk. ACK with that changed. signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Remove double unlock for domains
On 08/03/2015 10:04 AM, Martin Kletzander wrote: > On Mon, Aug 03, 2015 at 04:00:32PM +0200, Martin Kletzander wrote: >> On Mon, Aug 03, 2015 at 09:21:51AM -0400, John Ferlan wrote: >>> >>> >>> On 07/15/2015 03:17 AM, Martin Kletzander wrote: The virDomainObjListRemove() function unlocks a domain that it's given due to legacy code. And because of that code, which should be refactored, that last virObjectUnlock() cannot be just removed. So instead, lock it right back for qemu for now. All calls to qemuDomainRemoveInactive() are followed by code that unlocks the domain again, plus the domain should be locked during qemuDomainObjEndJob(), so the right place to lock it is right after virDomainObjListRemove(). >>> >>> Looked through callers - seems qemuProcessHandleMonitorEOF may need a >>> tweak too as it skips around the virObjectUnlock(vm) >>> >>> My only other comment would be perhaps the entry comment into >>> qemuDomainRemoveInactive could use a tweak on the wording and some >>> additional text to indicate on exit it should still hold the lock (or >>> just move the blob you added... Perhaps an XXX to force a revisit in the >>> future based on your note above "which should be refactored" >>> >>> Not that anyone reads those anyway ;-) >>> >>> ACK with the double check on the *EOF function... >>> >> >> Would it be enough if I squashed this in? >> > > Sorry, incomplete diff, here's the proper one, it's cleaner and looks > better (and compiles): Sure, this works for me ACK John > > diff --git i/src/qemu/qemu_domain.c w/src/qemu/qemu_domain.c > index d6d723112638..6ba8087c108b 100644 > --- i/src/qemu/qemu_domain.c > +++ w/src/qemu/qemu_domain.c > @@ -2597,7 +2597,13 @@ qemuDomainRemoveInactive(virQEMUDriverPtr driver, > * virDomainObjListRemove() leaves the domain unlocked so it can > * be unref'd for other drivers that depend on that, but we still > * need to reset a job and we have a reference from the API that > - * called this function. So we need to lock it back. > + * called this function. So we need to lock it back. This is > + * just a workaround for the qemu driver. > + * > + * XXX: Ideally, the global handling of domain objects and object > + * lists would be refactored so we don't need hacks like > + * this, but since that requires refactor of all drivers, > + * it's a work for another day. > */ > virObjectLock(vm); > virObjectUnref(cfg); > diff --git i/src/qemu/qemu_process.c w/src/qemu/qemu_process.c > index 5a9f97bc8921..505778ec2f05 100644 > --- i/src/qemu/qemu_process.c > +++ w/src/qemu/qemu_process.c > @@ -295,12 +295,12 @@ qemuProcessHandleMonitorEOF(qemuMonitorPtr mon > ATTRIBUTE_UNUSED, > > if (priv->beingDestroyed) { > VIR_DEBUG("Domain is being destroyed, EOF is expected"); > -goto unlock; > +goto cleanup; > } > > if (!virDomainObjIsActive(vm)) { > VIR_DEBUG("Domain %p is not active, ignoring EOF", vm); > -goto unlock; > +goto cleanup; > } > > if (priv->monJSON && !priv->gotShutdown) { > @@ -323,15 +323,11 @@ qemuProcessHandleMonitorEOF(qemuMonitorPtr mon > ATTRIBUTE_UNUSED, > qemuProcessStop(driver, vm, stopReason, stopFlags); > virDomainAuditStop(vm, auditReason); > > -if (!vm->persistent) { > +if (!vm->persistent) > qemuDomainRemoveInactive(driver, vm); > -goto cleanup; > -} > - > - unlock: > -virObjectUnlock(vm); > > cleanup: > +virObjectUnlock(vm); > if (event) > qemuDomainEventQueue(driver, event); > } > -- > > Martin -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/5] Make sure internal blkiotune values are in sync
We were blindly setting blkiotune values for devices, but kernel can throw some of them away. This series reworks the logic the same wayother tuning values are updated. That is, after the value gets set, it is read back again to make sure internal structures are in sync and we can return the values from them. Little example should clear up everything. Before: === $ virsh blkiotune dummy --device-read-bytes-sec /dev/sda,18446744073709551614 $ virsh blkiotune dummy weight : 500 device_weight : device_read_iops_sec: device_write_iops_sec: device_read_bytes_sec: /dev/sda,18446744073709551614 device_write_bytes_sec: $ cat /sys/fs/cgroup/blkio/machine/dummy.libvirt-qemu/blkio.throttle.read_bps_device 8:0 18446744073709551614 $ virsh blkiotune dummy --device-read-bytes-sec /dev/sda,18446744073709551615 $ virsh blkiotune dummy weight : 500 device_weight : device_read_iops_sec: device_write_iops_sec: device_read_bytes_sec: /dev/sda,18446744073709551615 device_write_bytes_sec: $ cat /sys/fs/cgroup/blkio/machine/dummy.libvirt-qemu/blkio.throttle.read_bps_device $ After: == $ virsh blkiotune dummy --device-read-bytes-sec /dev/sda,18446744073709551614 $ virsh blkiotune dummy weight : 500 device_weight : device_read_iops_sec: device_write_iops_sec: device_read_bytes_sec: /dev/sda,18446744073709551614 device_write_bytes_sec: $ cat /sys/fs/cgroup/blkio/machine/dummy.libvirt-qemu/blkio.throttle.read_bps_device 8:0 18446744073709551614 $ virsh blkiotune dummy --device-read-bytes-sec /dev/sda,18446744073709551615 $ virsh blkiotune dummy weight : 500 device_weight : device_read_iops_sec: device_write_iops_sec: device_read_bytes_sec: device_write_bytes_sec: $ cat /sys/fs/cgroup/blkio/machine/dummy.libvirt-qemu/blkio.throttle.read_bps_device $ Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1165580 Martin Kletzander (5): util: Add virStringGetFirstWithPrefix util: Add virCgroupGetBlockDevString util: Add getters for cgroup block device I/O throttling lxc: Sync BlkioDevice values when setting them in cgroups qemu: Sync BlkioDevice values when setting them in cgroups src/libvirt_private.syms | 6 + src/lxc/lxc_cgroup.c | 20 ++- src/lxc/lxc_driver.c | 25 ++- src/qemu/qemu_cgroup.c | 20 ++- src/qemu/qemu_driver.c | 25 ++- src/util/vircgroup.c | 457 +++ src/util/vircgroup.h | 20 +++ src/util/virstring.c | 17 ++ src/util/virstring.h | 2 + 9 files changed, 459 insertions(+), 133 deletions(-) -- 2.5.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/5] util: Add virStringGetFirstWithPrefix
That function takes string list and returns first string in that list that starts with the @prefix parameter with that prefix being skipped as the caller knows what it starts with (also for easier manipulation in future). Signed-off-by: Martin Kletzander --- src/libvirt_private.syms | 1 + src/util/virstring.c | 17 + src/util/virstring.h | 2 ++ 3 files changed, 20 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index ff322d61083e..d22bde70e17d 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2170,6 +2170,7 @@ virStrdup; virStringArrayHasString; virStringFreeList; virStringFreeListCount; +virStringGetFirstWithPrefix; virStringHasControlChars; virStringIsEmpty; virStringJoin; diff --git a/src/util/virstring.c b/src/util/virstring.c index 5794f968406e..31f77cd1a656 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -238,6 +238,23 @@ virStringArrayHasString(char **strings, const char *needle) return false; } +char * +virStringGetFirstWithPrefix(char **strings, const char *prefix) +{ +size_t i = 0; + +if (!strings) +return NULL; + +while (strings[i]) { +if (STRPREFIX(strings[i], prefix)) +return strings[i] + strlen(prefix); +i++; +} + +return NULL; +} + /* Like strtol, but produce an "int" result, and check more carefully. Return 0 upon success; return -1 to indicate failure. When END_PTR is NULL, the byte after the final valid digit must be NUL. diff --git a/src/util/virstring.h b/src/util/virstring.h index e6dcb32e998c..f65a12652961 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -47,6 +47,8 @@ void virStringFreeListCount(char **strings, size_t count); size_t virStringListLen(const char **strings); bool virStringArrayHasString(char **strings, const char *needle); +char *virStringGetFirstWithPrefix(char **strings, const char *prefix) +ATTRIBUTE_NONNULL(2); char *virArgvToString(const char *const *argv); -- 2.5.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/5] util: Add virCgroupGetBlockDevString
This function translates device paths to "major:minor " string, and all virCgroupSetBlkioDevice* functions are modified to use it. It's a cleanup with no functional change. Signed-off-by: Martin Kletzander --- src/util/vircgroup.c | 180 --- 1 file changed, 70 insertions(+), 110 deletions(-) diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 0599ba587809..afa85ded9061 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -705,6 +705,35 @@ virCgroupDetect(virCgroupPtr group, } +static char * +virCgroupGetBlockDevString(const char *path) +{ +char *ret = NULL; +struct stat sb; + +if (stat(path, &sb) < 0) { +virReportSystemError(errno, + _("Path '%s' is not accessible"), + path); +return NULL; +} + +if (!S_ISBLK(sb.st_mode)) { +virReportSystemError(EINVAL, + _("Path '%s' must be a block device"), + path); +return NULL; +} + +/* Automatically append space after the string since all callers + * use it anyway */ +if (virAsprintf(&ret, "%d:%d ", major(sb.st_rdev), minor(sb.st_rdev)) < 0) +return NULL; + +return ret; +} + + static int virCgroupSetValueStr(virCgroupPtr group, int controller, @@ -1966,7 +1995,6 @@ virCgroupGetBlkioIoDeviceServiced(virCgroupPtr group, long long *requests_write) { char *str1 = NULL, *str2 = NULL, *str3 = NULL, *p1, *p2; -struct stat sb; size_t i; int ret = -1; @@ -1983,20 +2011,6 @@ virCgroupGetBlkioIoDeviceServiced(virCgroupPtr group, requests_write }; -if (stat(path, &sb) < 0) { -virReportSystemError(errno, - _("Path '%s' is not accessible"), - path); -return -1; -} - -if (!S_ISBLK(sb.st_mode)) { -virReportSystemError(EINVAL, - _("Path '%s' must be a block device"), - path); -return -1; -} - if (virCgroupGetValueStr(group, VIR_CGROUP_CONTROLLER_BLKIO, "blkio.throttle.io_service_bytes", &str1) < 0) @@ -2007,7 +2021,7 @@ virCgroupGetBlkioIoDeviceServiced(virCgroupPtr group, "blkio.throttle.io_serviced", &str2) < 0) goto cleanup; -if (virAsprintf(&str3, "%d:%d ", major(sb.st_rdev), minor(sb.st_rdev)) < 0) +if (!(str3 = virCgroupGetBlockDevString(path))) goto cleanup; if (!(p1 = strstr(str1, str3))) { @@ -2116,33 +2130,22 @@ virCgroupSetBlkioDeviceReadIops(virCgroupPtr group, const char *path, unsigned int riops) { -char *str; -struct stat sb; -int ret; - -if (stat(path, &sb) < 0) { -virReportSystemError(errno, - _("Path '%s' is not accessible"), - path); -return -1; -} +char *str = NULL; +char *blkstr = NULL; +int ret = -1; -if (!S_ISBLK(sb.st_mode)) { -virReportSystemError(EINVAL, - _("Path '%s' must be a block device"), - path); +if (!(blkstr = virCgroupGetBlockDevString(path))) return -1; -} -if (virAsprintf(&str, "%d:%d %u", major(sb.st_rdev), -minor(sb.st_rdev), riops) < 0) -return -1; +if (virAsprintf(&str, "%s%u", blkstr, riops) < 0) +goto error; ret = virCgroupSetValueStr(group, VIR_CGROUP_CONTROLLER_BLKIO, "blkio.throttle.read_iops_device", str); - + error: +VIR_FREE(blkstr); VIR_FREE(str); return ret; } @@ -2161,33 +2164,22 @@ virCgroupSetBlkioDeviceWriteIops(virCgroupPtr group, const char *path, unsigned int wiops) { -char *str; -struct stat sb; -int ret; - -if (stat(path, &sb) < 0) { -virReportSystemError(errno, - _("Path '%s' is not accessible"), - path); -return -1; -} +char *str = NULL; +char *blkstr = NULL; +int ret = -1; -if (!S_ISBLK(sb.st_mode)) { -virReportSystemError(EINVAL, - _("Path '%s' must be a block device"), - path); +if (!(blkstr = virCgroupGetBlockDevString(path))) return -1; -} -if (virAsprintf(&str, "%d:%d %u", major(sb.st_rdev), -minor(sb.st_rdev), wiops) < 0) -return -1; +if (virAsprintf(&str, "%s%u", blkstr, wiops) < 0) +goto error; ret = virCgroupSetValueStr(group,
[libvirt] [PATCH 3/5] util: Add getters for cgroup block device I/O throttling
Since now they were not needed, but I sense they will be in a short while. Signed-off-by: Martin Kletzander --- src/libvirt_private.syms | 5 + src/util/vircgroup.c | 277 ++- src/util/vircgroup.h | 20 3 files changed, 299 insertions(+), 3 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d22bde70e17d..8803777cde80 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1175,6 +1175,11 @@ virCgroupDenyDeviceMajor; virCgroupDenyDevicePath; virCgroupDetectMountsFromFile; virCgroupFree; +virCgroupGetBlkioDeviceReadBps; +virCgroupGetBlkioDeviceReadIops; +virCgroupGetBlkioDeviceWeight; +virCgroupGetBlkioDeviceWriteBps; +virCgroupGetBlkioDeviceWriteIops; virCgroupGetBlkioIoDeviceServiced; virCgroupGetBlkioIoServiced; virCgroupGetBlkioWeight; diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index afa85ded9061..9d527fb8b99a 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -804,6 +804,36 @@ virCgroupGetValueStr(virCgroupPtr group, static int +virCgroupGetValueForBlkDev(virCgroupPtr group, + int controller, + const char *key, + const char *path, + char **value) +{ +char *prefix = NULL; +char *str = NULL; +char **lines = NULL; +int ret = -1; + +if (virCgroupGetValueStr(group, controller, key, &str) < 0) +goto error; + +if (!(prefix = virCgroupGetBlockDevString(path))) +return -1; + +if (!(lines = virStringSplit(str, "\n", -1))) +goto error; + +ret = VIR_STRDUP(*value, virStringGetFirstWithPrefix(lines, prefix)); + error: +VIR_FREE(str); +VIR_FREE(prefix); +virStringFreeList(lines); +return ret; +} + + +static int virCgroupSetValueU64(virCgroupPtr group, int controller, const char *key, @@ -2259,9 +2289,6 @@ virCgroupSetBlkioDeviceWriteBps(virCgroupPtr group, * @weight: The new device weight (100-1000), * (10-1000) after kernel 2.6.39, or 0 to clear * - * device_weight is treated as a write-only parameter, so - * there isn't a getter counterpart. - * * Returns: 0 on success, -1 on error */ int @@ -2289,6 +2316,196 @@ virCgroupSetBlkioDeviceWeight(virCgroupPtr group, return ret; } +/** + * virCgroupGetBlkioDeviceReadIops: + * @group: The cgroup to gather block io setting for + * @path: The path of device + * @riops: Returned device read iops throttle, 0 if there is none + * + * Returns: 0 on success, -1 on error + */ +int +virCgroupGetBlkioDeviceReadIops(virCgroupPtr group, +const char *path, +unsigned int *riops) +{ +char *str = NULL; +int ret = -1; + +if (virCgroupGetValueForBlkDev(group, + VIR_CGROUP_CONTROLLER_BLKIO, + "blkio.throttle.read_iops_device", + path, + &str) < 0) +goto error; + +if (!str) { +*riops = 0; +} else if (virStrToLong_ui(str, NULL, 10, riops) < 0) { +virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to parse '%s' as an integer"), + str); +goto error; +} + +ret = 0; + error: +VIR_FREE(str); +return ret; +} + +/** + * virCgroupGetBlkioDeviceWriteIops: + * @group: The cgroup to gather block io setting for + * @path: The path of device + * @wiops: Returned device write iops throttle, 0 if there is none + * + * Returns: 0 on success, -1 on error + */ +int +virCgroupGetBlkioDeviceWriteIops(virCgroupPtr group, + const char *path, + unsigned int *wiops) +{ +char *str = NULL; +int ret = -1; + +if (virCgroupGetValueForBlkDev(group, + VIR_CGROUP_CONTROLLER_BLKIO, + "blkio.throttle.write_iops_device", + path, + &str) < 0) +goto error; + +if (!str) { +*wiops = 0; +} else if (virStrToLong_ui(str, NULL, 10, wiops) < 0) { +virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to parse '%s' as an integer"), + str); +goto error; +} + +ret = 0; + error: +VIR_FREE(str); +return ret; +} + +/** + * virCgroupGetBlkioDeviceReadBps: + * @group: The cgroup to gather block io setting for + * @path: The path of device + * @rbps: Returned device read bps throttle, 0 if there is none + * + * Returns: 0 on success, -1 on error + */ +int +virCgroupGetBlkioDeviceReadBps(virCgroupPtr group, + const char *path, + unsigned long long *rbps) +{ +char *str = NULL; +int ret = -
[libvirt] [PATCH 4/5] lxc: Sync BlkioDevice values when setting them in cgroups
The problem here is that there are some values that kernel accepts, but does not set them, for example 18446744073709551615 which acts the same way as zero. Let's do the same thing we do with other tuning options and re-read them right after they are set in order to keep our internal structures up-to-date. Signed-off-by: Martin Kletzander --- src/lxc/lxc_cgroup.c | 20 +++- src/lxc/lxc_driver.c | 25 - 2 files changed, 35 insertions(+), 10 deletions(-) diff --git a/src/lxc/lxc_cgroup.c b/src/lxc/lxc_cgroup.c index 507d56759f6e..0d5a91cfcb92 100644 --- a/src/lxc/lxc_cgroup.c +++ b/src/lxc/lxc_cgroup.c @@ -117,27 +117,37 @@ static int virLXCCgroupSetupBlkioTune(virDomainDefPtr def, if (dev->weight && (virCgroupSetBlkioDeviceWeight(cgroup, dev->path, - dev->weight) < 0)) + dev->weight) < 0 || + virCgroupGetBlkioDeviceWeight(cgroup, dev->path, + &dev->weight) < 0)) return -1; if (dev->riops && (virCgroupSetBlkioDeviceReadIops(cgroup, dev->path, - dev->riops) < 0)) + dev->riops) < 0 || + virCgroupGetBlkioDeviceReadIops(cgroup, dev->path, + &dev->riops) < 0)) return -1; if (dev->wiops && (virCgroupSetBlkioDeviceWriteIops(cgroup, dev->path, - dev->wiops) < 0)) + dev->wiops) < 0 || + virCgroupGetBlkioDeviceWriteIops(cgroup, dev->path, + &dev->wiops) < 0)) return -1; if (dev->rbps && (virCgroupSetBlkioDeviceReadBps(cgroup, dev->path, -dev->rbps) < 0)) +dev->rbps) < 0 || + virCgroupGetBlkioDeviceReadBps(cgroup, dev->path, +&dev->rbps) < 0)) return -1; if (dev->wbps && (virCgroupSetBlkioDeviceWriteBps(cgroup, dev->path, - dev->wbps) < 0)) + dev->wbps) < 0 || + virCgroupGetBlkioDeviceWriteBps(cgroup, dev->path, + &dev->wbps) < 0)) return -1; } } diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 81bb71186cfc..f734d251a23b 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -2603,7 +2603,10 @@ lxcDomainSetBlkioParameters(virDomainPtr dom, for (j = 0; j < ndevices; j++) { if (virCgroupSetBlkioDeviceWeight(priv->cgroup, devices[j].path, - devices[j].weight) < 0) { + devices[j].weight) < 0 || +virCgroupGetBlkioDeviceWeight(priv->cgroup, + devices[j].path, + &devices[j].weight) < 0) { ret = -1; break; } @@ -2612,7 +2615,10 @@ lxcDomainSetBlkioParameters(virDomainPtr dom, for (j = 0; j < ndevices; j++) { if (virCgroupSetBlkioDeviceReadIops(priv->cgroup, devices[j].path, -devices[j].riops) < 0) { +devices[j].riops) < 0 || +virCgroupGetBlkioDeviceReadIops(priv->cgroup, +devices[j].path, +&devices[j].riops) < 0) { ret = -1; break; } @@ -2621,7 +2627,10 @@ lxcDomainSetBlkioParameters(virDomainPtr dom, for (j = 0; j < ndevices; j++) { if (virCgroupSetBlkioDeviceWriteIops(priv->cgroup, devices[j].path, - devices[j].wiops) < 0) { + devices[j].wiops) < 0 || +virCgroupGetBlkioDeviceWriteIops
[libvirt] [PATCH 5/5] qemu: Sync BlkioDevice values when setting them in cgroups
The problem here is that there are some values that kernel accepts, but does not set them, for example 18446744073709551615 which acts the same way as zero. Let's do the same thing we do with other tuning options and re-read them right after they are set in order to keep our internal structures up-to-date. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1165580 Signed-off-by: Martin Kletzander --- src/qemu/qemu_cgroup.c | 20 +++- src/qemu/qemu_driver.c | 25 - 2 files changed, 35 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index e1a28272ee7b..0da6c0238696 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -436,27 +436,37 @@ qemuSetupBlkioCgroup(virDomainObjPtr vm) virBlkioDevicePtr dev = &vm->def->blkio.devices[i]; if (dev->weight && (virCgroupSetBlkioDeviceWeight(priv->cgroup, dev->path, - dev->weight) < 0)) + dev->weight) < 0 || + virCgroupGetBlkioDeviceWeight(priv->cgroup, dev->path, + &dev->weight) < 0)) return -1; if (dev->riops && (virCgroupSetBlkioDeviceReadIops(priv->cgroup, dev->path, - dev->riops) < 0)) + dev->riops) < 0 || + virCgroupGetBlkioDeviceReadIops(priv->cgroup, dev->path, + &dev->riops) < 0)) return -1; if (dev->wiops && (virCgroupSetBlkioDeviceWriteIops(priv->cgroup, dev->path, - dev->wiops) < 0)) + dev->wiops) < 0 || + virCgroupGetBlkioDeviceWriteIops(priv->cgroup, dev->path, + &dev->wiops) < 0)) return -1; if (dev->rbps && (virCgroupSetBlkioDeviceReadBps(priv->cgroup, dev->path, -dev->rbps) < 0)) +dev->rbps) < 0 || + virCgroupGetBlkioDeviceReadBps(priv->cgroup, dev->path, +&dev->rbps) < 0)) return -1; if (dev->wbps && (virCgroupSetBlkioDeviceWriteBps(priv->cgroup, dev->path, - dev->wbps) < 0)) + dev->wbps) < 0 || + virCgroupGetBlkioDeviceWriteBps(priv->cgroup, dev->path, + &dev->wbps) < 0)) return -1; } } diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 7a984a86ab3a..aa3e05e4a258 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9191,7 +9191,10 @@ qemuDomainSetBlkioParameters(virDomainPtr dom, for (j = 0; j < ndevices; j++) { if (virCgroupSetBlkioDeviceWeight(priv->cgroup, devices[j].path, - devices[j].weight) < 0) { + devices[j].weight) < 0 || +virCgroupGetBlkioDeviceWeight(priv->cgroup, + devices[j].path, + &devices[j].weight) < 0) { ret = -1; break; } @@ -9200,7 +9203,10 @@ qemuDomainSetBlkioParameters(virDomainPtr dom, for (j = 0; j < ndevices; j++) { if (virCgroupSetBlkioDeviceReadIops(priv->cgroup, devices[j].path, -devices[j].riops) < 0) { +devices[j].riops) < 0 || +virCgroupGetBlkioDeviceReadIops(priv->cgroup, +devices[j].path, +&devices[j].riops) < 0) { ret = -1; break; } @@ -9209,7 +9215,10 @@ qemuDomainSetBlkioParameters(virDomainPtr dom, for (j = 0; j < ndevices; j++) { if (virCgroupSetBlkioDeviceWriteIops(priv->cgroup, devices[j].path, -
[libvirt] [PATCH] nodeinfo: Fix build failure when KVM headers are not available
Compiler error: ../../src/nodeinfo.c: In function 'nodeGetThreadsPerSubcore': ../../src/nodeinfo.c:2393: error: label 'out' defined but not used [-Wunused-label] ../../src/nodeinfo.c:2352: error: unused parameter 'arch' [-Wunused-parameter] --- src/nodeinfo.c | 17 ++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 7da055c..6ccada5 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -2341,6 +2341,8 @@ nodeAllocPages(unsigned int npages, return ret; } +#if HAVE_LINUX_KVM_H && defined(KVM_CAP_PPC_SMT) + /* Get the number of threads per subcore. * * This will be 2, 4 or 8 on POWER hosts, depending on the current @@ -2352,8 +2354,6 @@ int nodeGetThreadsPerSubcore(virArch arch) { int threads_per_subcore = 0; - -#if HAVE_LINUX_KVM_H && defined(KVM_CAP_PPC_SMT) const char *kvmpath = "/dev/kvm"; int kvmfd; @@ -2388,8 +2388,19 @@ nodeGetThreadsPerSubcore(virArch arch) VIR_FORCE_CLOSE(kvmfd); } -#endif /* HAVE_LINUX_KVM_H && defined(KVM_CAP_PPC_SMT) */ out: return threads_per_subcore; } + +#else + +/* Fallback for nodeGetThreadsPerSubcore() used when KVM headers + * are not available on the system */ +int +nodeGetThreadsPerSubcore(virArch arch ATTRIBUTE_UNUSED) +{ +return 0; +} + +#endif /* HAVE_LINUX_KVM_H && defined(KVM_CAP_PPC_SMT) */ -- 2.4.3 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] nodeinfo: Fix build failure when KVM headers are not available
On Mon, Aug 03, 2015 at 05:02:39PM +0200, Andrea Bolognani wrote: Compiler error: ../../src/nodeinfo.c: In function 'nodeGetThreadsPerSubcore': ../../src/nodeinfo.c:2393: error: label 'out' defined but not used [-Wunused-label] ../../src/nodeinfo.c:2352: error: unused parameter 'arch' [-Wunused-parameter] --- ACK && Pushed signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v11 0/5] nodeinfo: Add support for subcores
On Mon, 2015-08-03 at 09:27 -0400, John Ferlan wrote: > > Pulled in 2-5 from v8 series, rebuilt, checked, and pushed. The new code caused the build to fail[1] if the KVM headers were not available on the system, but Martin has already pushed the fix so we're good now. Thanks. [1] https://ci.centos.org/view/libvirt-project/job/libvirt-daemon-build /478/systems=libvirt-centos-6/ -- Andrea Bolognani Software Engineer - Virtualization Team -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Inherit namespace feature
On Mon, Aug 03, 2015 at 10:00:29PM +0530, Imran Khan wrote: > Thanks Daniel and Michal again for your valuable inputs. Please check my > reply with text for some of your comments. And request you to help > on those. > > BTW: should i reply with rework in the new patch. or i should reply to > this thread itself? Sorry i am new to the community so yet to get familiar > with etiquette. Different communities often have different rules on this. For libvirt reply to the comments in the thread, but when posting a new version of a patch post the patch as a new top level thread. ie do *not* use '--in-reply-to' with git send-email > > > +ns_fd[VIR_DOMAIN_NAMESPACE_SHARENET] = open(path, O_RDONLY); > > > +VIR_FREE(path); > > > +if (ns_fd[VIR_DOMAIN_NAMESPACE_SHARENET] < 0) { > > > +virReportSystemError(errno, > > > + _("failed to open netns %s"), > > lxcDef->ns_val[VIR_DOMAIN_NAMESPACE_SHARENET]); > > > +return -1; > > > +} > > > +} > > > +for (i = 0; i < VIR_DOMAIN_NAMESPACE_LAST; i++) { > > > +/* If not yet intialized by above: netns*/ > > > +if (lxcDef->ns_type[i] && ns_fd[i] == -1) { > > > +pid = strtol(lxcDef->ns_val[i], &eptr, 10); > > > +if (*eptr != '\0' || pid < 1) { > > > +/* check if the domain is running, then set the > > namespaces > > > + * to that container > > > + */ > > > +const char *ns[] = { "user", "ipc", "uts", "net", > > "pid", "mnt" }; > > > +conn = virConnectOpen("lxc:///"); > > > +if (!conn) { > > > +virReportError(virGetLastError()->code, > > > + _("unable to get connect to lxc %s"), > > lxcDef->ns_val[i]); > > > +rc = -1; > > > +goto cleanup; > > > +} > > > +dom = virDomainLookupByName(conn, lxcDef->ns_val[i]); > > > +if (!dom) { > > > +virReportError(virGetLastError()->code, > > > + _("Unable to lookup peer containeri > > %s"), > > > + lxcDef->ns_val[i]); > > > +rc = -1; > > > +goto cleanup; > > > +} > > > +if ((nfdlist = virDomainLxcOpenNamespace(dom, &fdlist, > > 0)) < 0) { > > > +virReportError(virGetLastError()->code, > > > + _("Unable to open %s"), > > lxcDef->ns_val[i]); > > > +rc = -1; > > > +goto cleanup; > > > +} > > > > I really hate the idea of the libvirt_lxc program opening a connection > > back to libvirtd using virConnectOpen, as that creates a circular > > dependancy. It also risks deadlock, because libvirtd will be holding > > locks while starting up the container, and you're calling back into > > the driver which may then end up acquiring the same lock. > > > > : This is where i am finding problem to code. All the driver > functions are static and to access them i might have to change the static > to non-static.which will not be inline with current design. i don't see > circular dependency with this approach as the internal connection is just > used to get the fd's. please share any approach to handle this or hope i > can keep the current code. The code you are interested in for virDomainLxcOpenNamespace is in lxc_driver.c, which is static. This though, simply calls the global virProcessGetNamespaces() method which is non-static. So when you put the code in lxc_process.c you can directly call virProcssGetNamespaces > > I think a better approach in general is for libvirtd lxc_process.c > > code to be responsible for getting access to all the namespace > > file descriptors. We can then pass those pre-opened file descrpitors > > down into libvirt_lxc using command line args, in the sme way that > > we pass down file descriptors for pre-opened TAP devices. > > > > eg so we end up running > > > > libvirt_lxc --netns 23 --pidns 24 --utsns 25 > > > > or something like that. > > > > : And useability wise its easier to provide names instead of fds. as > if the shared container goes down. the fds wont be valid. > with name a simple restart and again get the new fds with names > automatically. The libvirt_lxc program is not something that eend users ever run. It is launched by libvirtd itself, so we don't need to care about usability in this particular place. The users still use XML which allows names as you have in your patch already. 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/
Re: [libvirt] [PATCH v2 2/3] Avoid starting a PowerPC VM with floppy disk
On 08/03/2015 06:23 PM, Ján Tomko wrote: On Thu, Jul 30, 2015 at 07:55:36AM -0400, Kothapally Madhu Pavan wrote: PowerPC pseries based VMs do not support a floppy disk controller. Here the commit message mentions a controller, but the code only checks for the device, not for Should we forbid that one too? AFAIK we've auto-added it to the XML only if there was at least floppy. We need not forbid fdc, as checking for floppy device will fulfill the need. Yes, fdc is added only if there was at least one floppy device. Either way, the series looks good to me. Jan This prohibits libvirt from creating qemu command with floppy device. Signed-off-by: Kothapally Madhu Pavan --- src/qemu/qemu_command.c |8 1 file changed, 8 insertions(+) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 09f30c4..501c7df 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -9767,6 +9767,14 @@ qemuBuildCommandLine(virConnectPtr conn, continue; } +/* PowerPC pseries based VMs do not support floppy device */ +if ((disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) && +ARCH_IS_PPC64(def->os.arch) && STRPREFIX(def->os.machine, "pseries")) { +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("PowerPC pseries machines do not support floppy device")); +goto error; +} + switch (disk->device) { case VIR_DOMAIN_DISK_DEVICE_CDROM: bootindex = bootCD; -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list Thanks, Madhu Pavan. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 00/12] Adjust SCSI generated device address checks
On 07/22/2015 10:54 AM, John Ferlan wrote: > v1 here: > http://www.redhat.com/archives/libvir-list/2015-June/msg01104.html > > Some followups into July resulted in the request to move the Hostdev > and Disk default (or _NONE) address creation/assignment into domain/ > device post processing rather than during XML parsing. > > Changes in v2 are numerous, quite a bit of patch and code motion in order > to accomplish the requested task in small enough and reviewable chunks > > John Ferlan (12): > conf: Remove extraneous check in virDomainHostdevAssignAddress > conf: Add 'bus' and 'target' to SCSI address conflict checks > conf: Move hostdev and disk address validations > conf: Add xmlopt to virDomainDeviceDefPostParseInternal > conf: Add check for host address type while checking in use > conf: Try controller add when searching hostdev bus for unit > conf: Change when virDomainHostdevAssignAddress is called > conf: Remove unused param from virDomainHostdevDefParseXML > conf: Add SCSI hostdev check for disk drive address already in use > conf: Change when virDomainDiskDefAssignAddress is called > conf: Create locals for virDomainDiskDefAssignAddress > conf: Check for hostdev conflicts when assign default disk address > > docs/formatdomain.html.in | 4 +- > src/conf/domain_conf.c| 396 > +++--- > src/conf/domain_conf.h| 3 +- > src/qemu/qemu_command.c | 4 +- > src/vmx/vmx.c | 22 +-- > src/vmx/vmx.h | 3 +- > 6 files changed, 253 insertions(+), 179 deletions(-) > I fixed the error message in patch 7 and pushed patches 1-8 and 10. Adjustments to the other patches will take a bit more time and testing - I don't think throwing away the existing patches will completely work for the hotplug cases. It's been a few weeks since I had this all fresh in my mind though. John I'll rework and post the duplicate address check -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list