Re: [libvirt] mass create vm errors

[Michal Privoznik]

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)

[Martin Kletzander]

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

[Martin Kletzander]

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

[Michal Privoznik]

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

[Michal Privoznik]

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

[lhuang]

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

[lhuang]

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

[Daniel P. Berrange]

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

[Jiri Denemark]

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

[Andrea Bolognani]

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

[Michal Privoznik]

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

[Daniel P. Berrange]

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

[Michal Privoznik]

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

[Pavel Hrdina]

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

[Martin Kletzander]

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

[Daniel P. Berrange]

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

[Martin Kletzander]

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

[Martin Kletzander]

[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

[Martin Kletzander]

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
+  subelement  with 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

[Martin Kletzander]

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
+  subelement  with 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"

[Martin Kletzander]

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"

[Martin Kletzander]

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

[Daniel Veillard]

 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"

[Martin Kletzander]

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

[Martin Kletzander]

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

[Martin Kletzander]

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

[Martin Kletzander]

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

[Martin Kletzander]

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

[John Ferlan]

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

[John Ferlan]

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

[John Ferlan]

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

[Ján Tomko]

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

[Ján Tomko]

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

[Peter Krempa]

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

[John Ferlan]

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

[Ján Tomko]

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

[John Ferlan]

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

[Ján Tomko]

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

[Ján Tomko]

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

[Ján Tomko]

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

[Ján Tomko]

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

[Martin Kletzander]

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

[Martin Kletzander]

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

[Andrea Bolognani]

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

[Martin Kletzander]

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

[Martin Kletzander]

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

[John Ferlan]

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

[Martin Kletzander]

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

[Martin Kletzander]

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

[Martin Kletzander]

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

[Martin Kletzander]

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

[Martin Kletzander]

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

[Martin Kletzander]

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

[Andrea Bolognani]

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

[Martin Kletzander]

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

[Andrea Bolognani]

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

[Daniel P. Berrange]

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

[madhu pavan]

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

[John Ferlan]

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