Re: [libvirt] [PATCH v2 04/14] qemuBuildInterfaceCommandLine: Move vhostuser handling a bit further

[Michal Privoznik]

On 13.10.2016 22:58, John Ferlan wrote:
> 
> 
> On 10/06/2016 09:38 AM, Michal Privoznik wrote:
>> The idea is to have function that does some checking of the
>> arguments at its beginning and then have one big switch for all
>> the interface types it supports. Each one of them generating the
>> corresponding part of the command line.
>>
>> Signed-off-by: Michal Privoznik 
>> ---
>>  src/qemu/qemu_command.c|  9 +++---
>>  .../qemuxml2argv-net-vhostuser-fail.xml| 36 
>> ++
>>  tests/qemuxml2argvtest.c   |  3 ++
>>  3 files changed, 44 insertions(+), 4 deletions(-)
>>  create mode 100644 
>> tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser-fail.xml
>>
> 
> Similarly to patch 3, if a vhost-user entry contains 'filterref' or
> 'backend tap', then bulding will fail.  Although queues is Ok.
> 
> BTW: I just realized cfg isn't even used in this function other than the
> GetConfig call and Unref call.

Yes, I've noticed that too and I'm fixing it somewhere in a later patch.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 03/14] qemuBuildInterfaceCommandLine: Move hostdev handling a bit further

[Michal Privoznik]

On 13.10.2016 22:58, John Ferlan wrote:
> 
> 
> On 10/06/2016 09:38 AM, Michal Privoznik wrote:
>> The idea is to have function that does some checking of the
>> arguments at its beginning and then have one big switch for all
>> the interface types it supports. Each one of them generating the
>> corresponding part of the command line.
>>
>> Signed-off-by: Michal Privoznik 
>> ---
>>  src/qemu/qemu_command.c| 13 
>>  .../qemuxml2argv-net-hostdev-fail.xml  | 39 
>> ++
>>  tests/qemuxml2argvtest.c   |  4 +++
>>  3 files changed, 49 insertions(+), 7 deletions(-)
>>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-hostdev-fail.xml
>>
> 
> Is it safe to assume that previously if the XML had " filter='myfilter'/>,  , and/or " queues='4'/>" that it would be ignored, but now there will be an error
> at command line build time?

Yeah. I mean, we were lying to users before letting them think we set
one of those things you mentioned. We should error out.

> 
> Hopefully we get no complaints ;-)

Well, if we do, we can at least tell them sorry for fooling you :-)

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 00/14] Couple of vhost-user fixes and cleanups

[Michal Privoznik]

On 06.10.2016 21:38, Michal Privoznik wrote:
> v2 of:
> 
> https://www.redhat.com/archives/libvir-list/2016-August/msg00806.html
> 
> The first patches are mostly code movement and cleanups.
> 
> diff to v1:
> - Hopefully all John's review suggestions are worked in now
> 
> Michal Privoznik (14):
>   virDomainNetDefParseXML: Realign
>   virDomainNetGetActualType: Return type is virDomainNetType
>   qemuBuildInterfaceCommandLine: Move hostdev handling a bit further
>   qemuBuildInterfaceCommandLine: Move vhostuser handling a bit further
>   qemuBuildInterfaceCommandLine: Move from if-else forest to switch
>   qemuDomainAttachNetDevice: Move hostdev handling a bit further
>   qemuDomainAttachNetDevice: Explicitly list allowed types for hotplug
>   qemuBuildHostNetStr: Explicitly enumerate net types
>   qemuBuildChrChardevStr: Introduce @nowait argument
>   qemuBuildVhostuserCommandLine: Reuse qemuBuildChrChardevStr
>   qemuBuildInterfaceCommandLine: Pass proper args
>   qemuBuildVhostuserCommandLine: Unify -netdev creation
>   qemuBuildHostNetStr: Support VIR_DOMAIN_NET_TYPE_VHOSTUSER
>   qemu_hotplug: Support interface type of vhost-user hotplug
> 
>  src/bhyve/bhyve_command.c  |   2 +-
>  src/bhyve/bhyve_process.c  |   2 +-
>  src/conf/domain_conf.c |  16 +-
>  src/conf/domain_conf.h |   2 +-
>  src/libxl/libxl_domain.c   |   2 +-
>  src/libxl/libxl_driver.c   |   2 +-
>  src/lxc/lxc_driver.c   |   9 +-
>  src/qemu/qemu_command.c| 182 
> +
>  src/qemu/qemu_hotplug.c| 130 +++
>  src/qemu/qemu_process.c|  13 +-
>  .../qemuxml2argv-net-hostdev-fail.xml  |  39 +
>  .../qemuxml2argv-net-vhostuser-fail.xml|  36 
>  .../qemuxml2argv-net-vhostuser-multiq.args |   6 +-
>  .../qemuxml2argv-net-vhostuser.args|   4 +-
>  tests/qemuxml2argvtest.c   |   7 +
>  15 files changed, 334 insertions(+), 118 deletions(-)
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-hostdev-fail.xml
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser-fail.xml
> 

I've merged patches 10 a 11 as requested, and pushed these.
Thank you John for the review!

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 3/4] vbox: change API (un)initialization logic.

[Michal Privoznik]

On 13.10.2016 23:33, Dawid Zamirski wrote:
> On Thu, 2016-10-13 at 12:04 +0200, Martin Kletzander wrote:
>> On Wed, Oct 12, 2016 at 04:00:52PM -0400, Dawid Zamirski wrote:
>>>
>>> So after my initial attempts at handling this as suggested (create
>>> vboxDriver struct and initialize in VIR_ONCE_GLOBAL_INIT), I've
>>> stumbled upon virStateDriver structs used in other drivers that
>>> apparently has .stateInitialize, .stateCleanup that to me look like
>>> perfect places to call vbox's pfnComInitialize pfnComUnintialize
>>> pairs.
>>> Would this be a good idea to utilize the virStateDriver for this or
>>> should I stick with VIR_ONCE_GLOBAL_INIT way?
>>>
>>
>>
>> I just now realized you said the issue we are talking about crashes
>> the
>> libvirt daemon, right?  And it does not use virStateDriver?  That's
>> weird.  You are connecting to vbox:///session, but the crash happens
>> in
>> the libvirt daemon.  That means we switched the vbox driver to the
>> daemo-side driver, so I'd expect it to be stateful driver.  However
>> it
>> doesn't use virStateDriver.  I'm Cc'ing Michal to let him look at it
>> as
>> he's more handy in the whole vbox area.
>>
>>> PS. Thanks a lot for reviewing my code and guiding me through it
>>> all.
> 
> Yes, it's crashing libvirtd daemon, however the local one that is
> started on behalf of vbox:///session owned by the user that started it
> (not the system-wide one running as root). One thing I realized is that
> putting pfnComInitialize into the .stateInitialize will likely make
> VBoxSVC start together with libvirtd which IMO is not desired. I might
> still utilize the state driver to at least initialize vboxDriver object
> and obtain pFuncs reference and then defer pfnComInitialize to the
> first connection that comes in, similarly as my original patch does.
> That way, I might be able to make vbox driver follow the initialization
> pattern used by other drivers and hopefully get rid of the
> g_pVBoxGlobalData.
> 

Yes, this sounds reasonable. We certainly do not want to have a
permanent 'connection' to vboxsvc from the time daemon starts. It would
be pure waste of resources.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 3/4] vbox: change API (un)initialization logic.

[Michal Privoznik]

On 13.10.2016 18:04, Martin Kletzander wrote:
> On Wed, Oct 12, 2016 at 04:00:52PM -0400, Dawid Zamirski wrote:
>> On Tue, 2016-10-11 at 10:58 -0400, Dawid Zamirski wrote:
>>> On Tue, 2016-10-11 at 16:22 +0200, Martin Kletzander wrote:
>>> > On Wed, Sep 28, 2016 at 01:41:35PM -0400, Dawid Zamirski wrote:
>>> >
>>> > The allocation is not guarded by any lock, so there's still a
>>> > race.  I
>>> > think there should be a global struct that has only some lock in it
>>> > and
>>> > whatever global data you need, the struct will be initialized on
>>> > the
>>> > first call to any function (check out VIR_ONCE_GLOBAL_INIT) and
>>> > then
>>> > the
>>> > connection (or global data or how it's called) would be reference
>>> > counted (just like you have).  It's just that having the reference
>>> > count
>>> > in the object you will be reallocating over and over again for each
>>> > connection is not really good.
>>> >
>>>
>>>
>>> Thanks, I see, I'll address this in v2
>>>
>>
>> So after my initial attempts at handling this as suggested (create
>> vboxDriver struct and initialize in VIR_ONCE_GLOBAL_INIT), I've
>> stumbled upon virStateDriver structs used in other drivers that
>> apparently has .stateInitialize, .stateCleanup that to me look like
>> perfect places to call vbox's pfnComInitialize pfnComUnintialize pairs.
>> Would this be a good idea to utilize the virStateDriver for this or
>> should I stick with VIR_ONCE_GLOBAL_INIT way?
>>
> 
> I just now realized you said the issue we are talking about crashes the
> libvirt daemon, right?  And it does not use virStateDriver?  That's
> weird.  You are connecting to vbox:///session, but the crash happens in
> the libvirt daemon.  That means we switched the vbox driver to the
> daemo-side driver, so I'd expect it to be stateful driver. 

Yes, couple of drivers were switched to server side drivers because of
some licensing nonsense.

> However it
> doesn't use virStateDriver.

That's because vbox is stateless driver. Just like esx for instance.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] util: Alter return value of virReadFCHost and fix mem leak

[John Ferlan]

On 10/12/2016 10:49 AM, Erik Skultety wrote:
> On Wed, Oct 12, 2016 at 09:20:29AM -0400, John Ferlan wrote:
>>
>>
>> On 10/12/2016 06:40 AM, Erik Skultety wrote:
>>> On Tue, Oct 11, 2016 at 05:25:49PM -0400, John Ferlan wrote:
 https://bugzilla.redhat.com/show_bug.cgi?id=1357416

 Rather than return a 0 or -1 and the *result string, return just the result
 string to the caller.  Alter all the callers to handle the different 
 return.

 As a side effect or result of this, it's much clearer that we cannot just
 assign the returned string into the scsi_host wwnn, wwpn, and fabric_wwn
 fields - rather we should fetch a temporary string, then as long as our
 fetch was good, VIR_FREE what may have been there, and STEAL what we just 
 got.
 This fixes a memory leak in the virNodeDeviceCreateXML code path through
 find_new_device and nodeDeviceLookupSCSIHostByWWN which will continually
 call nodeDeviceSysfsGetSCSIHostCaps until the expected wwnn/wwpn is found
 in the device object capabilities.

 Signed-off-by: John Ferlan 
 ---

  I suppose I could have made two patches out of this, but it felt like
  overkill once I realized what the issue was. OK a third one line patch
  could have been added to change the virGetFCHostNameByWWN comment as well,
  but that'd really be overkill.

  src/node_device/node_device_linux_sysfs.c | 55 
 ---
  src/util/virutil.c| 34 ---
  src/util/virutil.h|  9 +++--
  tests/fchosttest.c| 30 ++---
  4 files changed, 49 insertions(+), 79 deletions(-)

 diff --git a/src/node_device/node_device_linux_sysfs.c 
 b/src/node_device/node_device_linux_sysfs.c
 index 549d32c..be99c41 100644
 --- a/src/node_device/node_device_linux_sysfs.c
 +++ b/src/node_device/node_device_linux_sysfs.c
 @@ -44,8 +44,7 @@ VIR_LOG_INIT("node_device.node_device_linux_sysfs");
  int
  nodeDeviceSysfsGetSCSIHostCaps(virNodeDevCapDataPtr d)
  {
 -char *max_vports = NULL;
 -char *vports = NULL;
 +char *tmp = NULL;
  int ret = -1;
  
  if (virReadSCSIUniqueId(NULL, d->scsi_host.host,
 @@ -59,64 +58,53 @@ nodeDeviceSysfsGetSCSIHostCaps(virNodeDevCapDataPtr d)
  if (virIsCapableFCHost(NULL, d->scsi_host.host)) {
  d->scsi_host.flags |= VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST;
  
 -if (virReadFCHost(NULL,
 -  d->scsi_host.host,
 -  "port_name",
 -  >scsi_host.wwpn) < 0) {
 +if (!(tmp = virReadFCHost(NULL, d->scsi_host.host, "port_name"))) 
 {
  VIR_WARN("Failed to read WWPN for host%d", d->scsi_host.host);
  goto cleanup;
  }
 +VIR_FREE(d->scsi_host.wwpn);
 +VIR_STEAL_PTR(d->scsi_host.wwpn, tmp);
  
 -if (virReadFCHost(NULL,
 -  d->scsi_host.host,
 -  "node_name",
 -  >scsi_host.wwnn) < 0) {
 +if (!(tmp = virReadFCHost(NULL, d->scsi_host.host, "node_name"))) 
 {
  VIR_WARN("Failed to read WWNN for host%d", d->scsi_host.host);
  goto cleanup;
  }
 +VIR_FREE(d->scsi_host.wwnn);
 +VIR_STEAL_PTR(d->scsi_host.wwnn, tmp);
  
 -if (virReadFCHost(NULL,
 -  d->scsi_host.host,
 -  "fabric_name",
 -  >scsi_host.fabric_wwn) < 0) {
 +if (!(tmp = virReadFCHost(NULL, d->scsi_host.host, 
 "fabric_name"))) {
  VIR_WARN("Failed to read fabric WWN for host%d",
   d->scsi_host.host);
  goto cleanup;
  }
 +VIR_FREE(d->scsi_host.fabric_wwn);
 +VIR_STEAL_PTR(d->scsi_host.fabric_wwn, tmp);
  }
  
>>>
>>> So if I understand correctly, the problem is basically that we did not call
>>> VIR_FREE on the data that had previously been filled to the 
>>> virNodeDevCapData
>>> structure during replacing it with new data. So, we could just keep the
>>> signature of virReadFCHost as is and just add the VIR_FREE and VIR_STEAL_PTR
>>> to the function, thus there won't be any need to use VIR_FREE+STEAL 
>>> repeatedly
>>> in here.
>>>
>>
>> True - we're overwriting the >scsi_host.{wwnn|wwpn|fabric_wwn} fields
>> and that's the primary issue (e.g. mem leak).
>>
>> While I understand your point, I'm not sure adding a VIR_FREE(*result)
>> to virReadFCHost just prior to the "if VIR_STRDUP(*result, p) < 0)" is a
>> proper solution since virReadFCHost does not "own" that memory. It
>> doesn't state that if *result has 

Re: [libvirt] [PATCH] virLogDefineOutputs: Fix build without syslog.h

[Michal Privoznik]

On 14.10.2016 10:22, John Ferlan wrote:
> 
> 
> On 10/13/2016 10:12 PM, Michal Privoznik wrote:
>> Not every system out there has syslog, that's why we check for it
>> in our configure script. However, in 640b58abdf while fixing
>> another issue, some variables and functions are called that are
>> defined only when syslog.h is present. But these function
>> calls/variables were not guarded by #ifdef-s.
>>
>> Signed-off-by: Michal Privoznik 
>> ---
>>
>> Pushed under build breaker rule.
>>
>>  src/util/virlog.c | 15 +--
>>  1 file changed, 9 insertions(+), 6 deletions(-)
>>
> 
> FYI: There was a patch on list from Eric:
> 
> http://www.redhat.com/archives/libvir-list/2016-October/msg00397.html

D'oh! I'm sorry. For some reason I didn't check the list before starting
to work on this. I could come up with some lousy excuse (like morning
coffee hasn't kicked in yet), but next time I'll check.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 00/12] Add length (duration) params for iotune throttling

[John Ferlan]

ping?  Tks (I have a follow up series for group that I'd like to see
considered before the end of the release cycle too, but this needs to be
done first).

John

On 10/06/2016 06:38 PM, John Ferlan wrote:
> v1: http://www.redhat.com/archives/libvir-list/2016-September/msg01090.html
> 
> Differences since v1:
> 
> Patches 1-5 are new...
>Patch 1 is essentially the former patch 8.5 added after initial review
>   based on a review comment. Erik Skultety and I went back and forth
>   on this one a few times and this is the result
>Patch 2 is a result of being frustrated by a generic error message when
>   I know we could return whatever qemu gives us
>Patch 3 is simple code motion - the conf_disk is closer to usage now
>Patch 4 & 5 create and use the same algorithm to set the default values
>   that weren't provided.
> 
> Patches 6-7 have already been ACK'd... I've been waiting to push because
>they will interfere with libvirt-perl builds and since the 2.3.0 hasn't
>been published yet, I'm still holding onto them.
> 
> Patch 8 is the former patch 8, but reworked mostly in qemu_driver.c, but
>also a change in qemu_monitor_json.c to use "P:" instead of "U:" for
>then length values. For _length a 0 is an invalid value, so we'll use
>it to mean "nothing changed"
> 
> Patch 9 is the former patch 11 and is mostly intact, with major difference
>being the removal of the CHECK_IOTUNE_MAX_LENGTH macro (it was causing
>domain disappearance)
> 
> Patch 10 is the former patch 12 and is unchanged. It also has been ACKd
> 
> Patch 11-12 are new - it's the virsh code.
> 
> John Ferlan (12):
>   qemu: Create a macro to handle setting bytes/iops iotune values
>   qemu: Return real error message for block_set_io_throttle
>   qemu: Move setting of conf_disk in qemuDomainSetBlockIoTune
>   qemu: Introduce qemuDomainSetBlockIoTuneSetDefaults
>   qemu: Use qemuDomainsetBlockIoTuneSetDefaults for config
>   include: Add new definitions for duration for bps/iops throttling
>   caps: Add new capability for the bps/iops throttling length
>   qemu: Add length for bps/iops throttling parameters to driver
>   conf: Add support for blkiotune "_length" options
>   qemu: Add the length options to the iotune command line
>   virsh: Create a macro to add IOTUNE values
>   virsh: Add _length parameters to virsh output
> 
>  docs/formatdomain.html.in  |  40 ++-
>  docs/schemas/domaincommon.rng  |  38 +++
>  include/libvirt/libvirt-domain.h   | 102 ++
>  src/conf/domain_conf.c |  24 +-
>  src/conf/domain_conf.h |   6 +
>  src/qemu/qemu_capabilities.c   |   2 +
>  src/qemu/qemu_capabilities.h   |   1 +
>  src/qemu/qemu_command.c|  21 ++
>  src/qemu/qemu_driver.c | 341 
> +++--
>  src/qemu/qemu_monitor.c|   7 +-
>  src/qemu/qemu_monitor.h|   3 +-
>  src/qemu/qemu_monitor_json.c   |  39 ++-
>  src/qemu/qemu_monitor_json.h   |   3 +-
>  .../caps_2.6.0-gicv2.aarch64.xml   |   1 +
>  .../caps_2.6.0-gicv3.aarch64.xml   |   1 +
>  tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml  |   1 +
>  tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml   |   1 +
>  tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml   |   1 +
>  tests/qemumonitorjsontest.c|  17 +-
>  .../qemuxml2argv-blkdeviotune-max-length.args  |  34 ++
>  .../qemuxml2argv-blkdeviotune-max-length.xml   |  65 
>  tests/qemuxml2argvtest.c   |   4 +
>  .../qemuxml2xmlout-blkdeviotune-max-length.xml |   1 +
>  tests/qemuxml2xmltest.c|   1 +
>  tools/virsh-domain.c   | 196 +---
>  tools/virsh.pod|  21 ++
>  26 files changed, 673 insertions(+), 298 deletions(-)
>  create mode 100644 
> tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune-max-length.args
>  create mode 100644 
> tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune-max-length.xml
>  create mode 12 
> tests/qemuxml2xmloutdata/qemuxml2xmlout-blkdeviotune-max-length.xml
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] virLogDefineOutputs: Fix build without syslog.h

[John Ferlan]

On 10/13/2016 10:12 PM, Michal Privoznik wrote:
> Not every system out there has syslog, that's why we check for it
> in our configure script. However, in 640b58abdf while fixing
> another issue, some variables and functions are called that are
> defined only when syslog.h is present. But these function
> calls/variables were not guarded by #ifdef-s.
> 
> Signed-off-by: Michal Privoznik 
> ---
> 
> Pushed under build breaker rule.
> 
>  src/util/virlog.c | 15 +--
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 

FYI: There was a patch on list from Eric:

http://www.redhat.com/archives/libvir-list/2016-October/msg00397.html

John

> diff --git a/src/util/virlog.c b/src/util/virlog.c
> index 52b0eea..8f831fc 100644
> --- a/src/util/virlog.c
> +++ b/src/util/virlog.c
> @@ -1362,9 +1362,10 @@ virLogFindOutput(virLogOutputPtr *outputs, size_t 
> noutputs,
>  int
>  virLogDefineOutputs(virLogOutputPtr *outputs, size_t noutputs)
>  {
> -int ret = -1;
> +#if HAVE_SYSLOG_H
>  int id;
>  char *tmp = NULL;
> +#endif /* HAVE_SYSLOG_H */
>  
>  if (virLogInitialize() < 0)
>  return -1;
> @@ -1372,6 +1373,7 @@ virLogDefineOutputs(virLogOutputPtr *outputs, size_t 
> noutputs)
>  virLogLock();
>  virLogResetOutputs();
>  
> +#if HAVE_SYSLOG_H
>  /* syslog needs to be special-cased, since it keeps the fd in private */
>  if ((id = virLogFindOutput(outputs, noutputs, VIR_LOG_TO_SYSLOG,
> current_ident)) != -1) {
> @@ -1379,20 +1381,21 @@ virLogDefineOutputs(virLogOutputPtr *outputs, size_t 
> noutputs)
>   * holding the lock so it's safe to call openlog and change the 
> message
>   * tag
>   */
> -if (VIR_STRDUP_QUIET(tmp, outputs[id]->name) < 0)
> -goto cleanup;
> +if (VIR_STRDUP_QUIET(tmp, outputs[id]->name) < 0) {
> +virLogUnlock();
> +return -1;
> +}
>  VIR_FREE(current_ident);
>  current_ident = tmp;
>  openlog(current_ident, 0, 0);
>  }
> +#endif /* HAVE_SYSLOG_H */
>  
>  virLogOutputs = outputs;
>  virLogNbOutputs = noutputs;
>  
> -ret = 0;
> - cleanup:
>  virLogUnlock();
> -return ret;
> +return 0;
>  }
>  
>  
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH] virLogDefineOutputs: Fix build without syslog.h

[Michal Privoznik]

Not every system out there has syslog, that's why we check for it
in our configure script. However, in 640b58abdf while fixing
another issue, some variables and functions are called that are
defined only when syslog.h is present. But these function
calls/variables were not guarded by #ifdef-s.

Signed-off-by: Michal Privoznik 
---

Pushed under build breaker rule.

 src/util/virlog.c | 15 +--
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/src/util/virlog.c b/src/util/virlog.c
index 52b0eea..8f831fc 100644
--- a/src/util/virlog.c
+++ b/src/util/virlog.c
@@ -1362,9 +1362,10 @@ virLogFindOutput(virLogOutputPtr *outputs, size_t 
noutputs,
 int
 virLogDefineOutputs(virLogOutputPtr *outputs, size_t noutputs)
 {
-int ret = -1;
+#if HAVE_SYSLOG_H
 int id;
 char *tmp = NULL;
+#endif /* HAVE_SYSLOG_H */
 
 if (virLogInitialize() < 0)
 return -1;
@@ -1372,6 +1373,7 @@ virLogDefineOutputs(virLogOutputPtr *outputs, size_t 
noutputs)
 virLogLock();
 virLogResetOutputs();
 
+#if HAVE_SYSLOG_H
 /* syslog needs to be special-cased, since it keeps the fd in private */
 if ((id = virLogFindOutput(outputs, noutputs, VIR_LOG_TO_SYSLOG,
current_ident)) != -1) {
@@ -1379,20 +1381,21 @@ virLogDefineOutputs(virLogOutputPtr *outputs, size_t 
noutputs)
  * holding the lock so it's safe to call openlog and change the message
  * tag
  */
-if (VIR_STRDUP_QUIET(tmp, outputs[id]->name) < 0)
-goto cleanup;
+if (VIR_STRDUP_QUIET(tmp, outputs[id]->name) < 0) {
+virLogUnlock();
+return -1;
+}
 VIR_FREE(current_ident);
 current_ident = tmp;
 openlog(current_ident, 0, 0);
 }
+#endif /* HAVE_SYSLOG_H */
 
 virLogOutputs = outputs;
 virLogNbOutputs = noutputs;
 
-ret = 0;
- cleanup:
 virLogUnlock();
-return ret;
+return 0;
 }
 
 
-- 
2.8.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 4/4] schema: smbios: allow any strings

[Peter Krempa]

On Thu, Oct 13, 2016 at 17:23:33 -0400, John Ferlan wrote:
> 
> 
> On 10/10/2016 11:51 AM, Peter Krempa wrote:
> > The smbios docs allow any string to be passed and libvirt does not
> > really do any validation on them. Allow passing any string.
> > 
> > Partially resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1373535
> > ---
> >  docs/schemas/domaincommon.rng | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> > 
> 
> ACK, although couldn't the 's each be
> replaced by , too?

It could probably but I did not want to remove it if we ever revisit it.
On the other hand in such case it will most likely be replaced with
specific entries.


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/4] qemu: command: Don't bother reporting errors in smbios formatters

[Peter Krempa]

On Thu, Oct 13, 2016 at 17:11:54 -0400, John Ferlan wrote:
> 
> 
> On 10/10/2016 11:51 AM, Peter Krempa wrote:
> > qemuBuildSmbiosBiosStr and qemuBuildSmbiosSystemStr return NULL if
> > there's noting to format on the commandline. Reporting errors from
> 
> nothing
> 
> > buffer creation doesn't make sense since it would be ignored.
> 
> Introduced by 54c0237ccb, so it's been this way a long time...
> 
> > ---
> >  src/qemu/qemu_command.c | 14 --
> >  1 file changed, 14 deletions(-)
> > 
> 
> I suppose since it seems the only legitimate error you'll hit is ENOMEM
> and if you ignore it here, some shortly to be run code is sure to run
> into it, then no big deal...
> 
> Although, one could argue the callers should check/return on error, but
> they'd probably lose that argument.

Not really :). I thought the same when writing the patch as it's fully
possible although extremely unlikely that we'd start a VM with invalid
configuration. Given that we were doing it like this for quite some
while and it would require a more invasive refactoring of the code I
just decided to drop the code altogether.

> ACK for what's here

Thanks, I'll go with this version currently, since it keeps the
semantics present for a rather long time.

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] leasetime support per and

[Alberto Ruiz]

I'm attaching the patch because I've been unable to use send-email properly.

2) is a good point, I wanted to discuss it here. Yes 120 is the minimum
allowed by dnsmasq, erroring out seems fine to me, I'll change the patch
accordingly.

On Thu, Oct 13, 2016 at 7:07 AM, Michal Privoznik 
wrote:

> On 13.10.2016 00:03, Alberto Ruiz wrote:
> > Support for custom dhcp wide and per host leasetime.
> >
> > It is specified as a child tag for :
> > 
> >   24h
> >   ...
> > 
> >
> > And as an attribute for :
> > 
> >   
> > 
> >
> > These are the different notations:
> >
> > -1   (infinite/unlimited lease)
> > 120  (seconds are the default unit, 120 seconds is the minimum, if less
> is
> > specified it will use 120)
> > 300s (seconds)
> > 5m   (minutes)
> > 24h  (hours)
> > 7d   (days)
> > ---
>
> I know I'm stepping on a moving train (sorry for that), but I have two
> points to raise:
>
> 1) use git send-email, this patch is mangled by your MTA and does not
> apply.
> 2) 120 seconds is minimum because of dnsmasq? If so, I think we should
> error out instead of silently changing this to a different value behind
> user's back.
>
> Michal
>



-- 
Alberto Ruiz
Associate Engineering Manager - Desktop Management Tools
Red Hat
From d48f957ab78310d864b356b4a25b9a29722ca736 Mon Sep 17 00:00:00 2001
From: Alberto Ruiz 
Date: Thu, 6 Oct 2016 21:29:40 +0100
Subject: [PATCH] leasetime support per  and 

Support for custom dhcp wide and per host leasetime.

It is specified as a child tag for :

  24h
  ...


And as an attribute for :

  


These are the different notations:

-1   (infinite/unlimited lease)
120  (seconds are the default unit, 120 seconds is the minimum, if less is specified it will use 120)
300s (seconds)
5m   (minutes)
24h  (hours)
7d   (days)
---
 docs/schemas/basictypes.rng|   5 +
 docs/schemas/network.rng   |   8 ++
 src/conf/network_conf.c|  86 +-
 src/conf/network_conf.h|   4 +-
 src/libvirt_private.syms   |   1 +
 src/network/bridge_driver.c| 132 +
 src/network/bridge_driver.h|   1 +
 src/util/virdnsmasq.c  | 106 +++--
 src/util/virdnsmasq.h  |   2 +
 .../dhcp6-nat-network.hostsfile|   7 ++
 tests/networkxml2confdata/dhcp6-network.hostsfile  |   5 +
 .../dhcp6host-routed-network.hostsfile |   7 ++
 tests/networkxml2confdata/leasetime-days.conf  |  17 +++
 tests/networkxml2confdata/leasetime-days.xml   |  18 +++
 tests/networkxml2confdata/leasetime-host.conf  |  16 +++
 tests/networkxml2confdata/leasetime-host.hostsfile |   6 +
 tests/networkxml2confdata/leasetime-host.xml   |  22 
 tests/networkxml2confdata/leasetime-hours.conf |  17 +++
 tests/networkxml2confdata/leasetime-hours.xml  |  18 +++
 tests/networkxml2confdata/leasetime-infinite.conf  |  17 +++
 tests/networkxml2confdata/leasetime-infinite.xml   |  18 +++
 tests/networkxml2confdata/leasetime-minutes.conf   |  17 +++
 tests/networkxml2confdata/leasetime-minutes.xml|  18 +++
 tests/networkxml2confdata/leasetime-seconds.conf   |  17 +++
 tests/networkxml2confdata/leasetime-seconds.xml|  18 +++
 tests/networkxml2confdata/leasetime.conf   |  17 +++
 tests/networkxml2confdata/leasetime.xml|  18 +++
 .../nat-network-dns-srv-record-minimal.hostsfile   |   2 +
 .../nat-network-dns-srv-record.hostsfile   |   2 +
 .../nat-network-dns-txt-record.hostsfile   |   2 +
 .../nat-network-name-with-quotes.hostsfile |   2 +
 tests/networkxml2confdata/nat-network.hostsfile|   2 +
 tests/networkxml2conftest.c|  47 ++--
 33 files changed, 597 insertions(+), 78 deletions(-)
 create mode 100644 tests/networkxml2confdata/dhcp6-nat-network.hostsfile
 create mode 100644 tests/networkxml2confdata/dhcp6-network.hostsfile
 create mode 100644 tests/networkxml2confdata/dhcp6host-routed-network.hostsfile
 create mode 100644 tests/networkxml2confdata/leasetime-days.conf
 create mode 100644 tests/networkxml2confdata/leasetime-days.xml
 create mode 100644 tests/networkxml2confdata/leasetime-host.conf
 create mode 100644 tests/networkxml2confdata/leasetime-host.hostsfile
 create mode 100644 tests/networkxml2confdata/leasetime-host.xml
 create mode 100644 tests/networkxml2confdata/leasetime-hours.conf
 create mode 100644 tests/networkxml2confdata/leasetime-hours.xml
 create mode 100644 tests/networkxml2confdata/leasetime-infinite.conf
 create mode 100644 tests/networkxml2confdata/leasetime-infinite.xml
 create mode 100644 tests/networkxml2confdata/leasetime-minutes.conf
 create mode 100644 tests/networkxml2confdata/leasetime-minutes.xml
 create mode 100644 tests/networkxml2confdata/leasetime-seconds.conf
 create mode 100644 

Re: [libvirt] [PATCH 1/1] qemu: host NUMA hugepage policy without guest NUMA

[Sam Bobroff]

On Thu, Oct 13, 2016 at 11:34:43AM +0200, Martin Kletzander wrote:
> On Thu, Oct 13, 2016 at 11:34:16AM +1100, Sam Bobroff wrote:
> >On Wed, Oct 12, 2016 at 10:27:50AM +0200, Martin Kletzander wrote:
> >>On Wed, Oct 12, 2016 at 03:04:53PM +1100, Sam Bobroff wrote:
> >>>At the moment, guests that are backed by hugepages in the host are
> >>>only able to use policy to control the placement of those hugepages
> >>>on a per-(guest-)CPU basis. Policy applied globally is ignored.
> >>>
> >>>Such guests would use  and
> >>>a  block with  but no  >>>.../> elements.
> >>>
> >>>This patch corrects this by, in this specific case, changing the QEMU
> >>>command line from "-mem-prealloc -mem-path=..." (which cannot
> >>>specify NUMA policy) to "-object memory-backend-file ..." (which can).
> >>>
> >>>Note: This is not visible to the guest and does not appear to create
> >>>a migration incompatibility.
> >>>
> >>
> >>It could make sense, I haven't tried yet, though.  However, I still
> >>don't see the point in using memory-backend-file.  Is it that when you
> >>don't have cpuset cgroup the allocation doesn't work well?  Because it
> >>certainly does work for me.
> >
> >Thanks for taking a look at this :-)
> >
> >The point of using a memory-backend-file is that with it, the NUMA policy can
> >be specified to QEMU, but with -mem-path it can't. It seems to be a way to 
> >tell
> >QEMU to apply NUMA policy in the right place. It does seem odd to me to use
> >memory-backend-file without attaching the backend to a guest NUMA node, but 
> >it
> >seems to do the right thing in this case. (If there are guest NUMA nodes, or 
> >if
> >hugepages aren't being used, policy is correctly applied.)
> >
> >I'll describe my test case in detail, perhaps there's something I don't 
> >understand
> >happening.
> >
> >* I set up a machine with two (fake) NUMA nodes (0 and 1), with 2G of 
> >hugepages
> > on node 1, and none on node 0.
> >
> >* I create a 2G guest using virt-install:
> >
> >virt-install --name ppc --memory=2048 --disk ~/tmp/tmp.qcow2 --cdrom 
> >~/tmp/ubuntu-16.04-server-ppc64el.iso --wait 0 --virt-type qemu 
> >--memorybacking hugepages=on --graphics vnc --arch ppc64le
> >
> >* I "virsh destroy" and then "virsh edit" to add this block to the guest XML:
> >
> > 
> >
> > 
> >
> >* "virsh start", and the machine starts (I believe it should fail due to 
> >insufficient memory satasfying the policy).
> >* "numastat -p $(pidof qemu-system-ppc64)" shows something like this:
> >
> >Per-node process memory usage (in MBs) for PID 8048 (qemu-system-ppc)
> >  Node 0  Node 1   Total
> > --- --- ---
> >Huge 0.00 2048.00 2048.00
> >Heap 8.120.008.12
> >Stack0.030.000.03
> >Private 35.806.10   41.90
> >  --- --- ---
> >Total   43.95 2054.10 2098.05
> >
> >So it looks like it's allocated hugepages from node 1, isn't this violating 
> >the
> >policy I set via numatune?
> >
> 
> Oh, now I get it.  We are doing our best to apply that policy to qemu
> even when we don't have this option.  However, using this works even
> better (which is probably* what we want).  And that's the reasoning
> behind this.
> 
> * I'm saying probably because when I was adding numactl binding to be
>   used together with cgroups, I was told that we couldn't change the
>   binding afterwards and it's bad.  I feel like we could do something
>   with that and it would help us in the future, but there needs to be a
>   discussion, I guess.  Because I might be one of the few =)
> 
> So to recapitulate that, there are three options how to affect the
> allocation of qemu's memory:
> 
> 1) numactl (libnuma): it works as expected, but cannot be changed later
> 
> 2) cgroups: so strict it has to be applied after qemu started, due to
>that it doesn't work right, especially for stuff that gets all
>pre-allocated (like hugepages).  it can be changed later, but it
>won't always mean the memory will migrate, so upon change there is
>no guarantee.  If it's unavailable, we fallback to (1) anyway
> 
> 3) memory-backing-file's host-nodes=: this works as expected, but
>cannot be used with older QEMUs, cannot be changed later and in some
>cases (not your particular one) it might screw up migration if it
>wasn't used before.
> 
> Selecting the best option from these, plus making the code work with
> every possibility (erroring out when you want to change the memory node
> and we had to use (1) for example) is a pain.  We should really think
> about that and reorganize these things for the better of the future.
> Otherwise we're going to get overwhelm ourselves.  Cc'ing Peter to get
> his thoughts as well as he 

Re: [libvirt] [PATCH 4/4] test_driver: Implement virNodeAllocPages

[John Ferlan]

On 10/12/2016 10:57 PM, Michal Privoznik wrote:
> On 13.10.2016 05:38, John Ferlan wrote:
>>
>>
>> On 10/05/2016 03:30 AM, Michal Privoznik wrote:
>>> Now that our cells in test driver are huge pages aware, we can
>>> implement virNodeAllocPages. Basically there's just one catch. If
>>> users specify startCell == -1, they want the huge pages change to
>>> be stretched over all the nodes. Therefore we just recalculate
>>> the change they want to make to each node and continue.
>>>
>>> Signed-off-by: Michal Privoznik 
>>> ---
>>>  src/test/test_driver.c | 78 
>>> ++
>>>  1 file changed, 78 insertions(+)
>>>
>>> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
>>> index a3f74f8..d9e408e 100644
>>> --- a/src/test/test_driver.c
>>> +++ b/src/test/test_driver.c
>>> @@ -2867,6 +2867,83 @@ testNodeGetFreePages(virConnectPtr conn,
>>>  return ret;
>>>  }
>>>  
>>> +static int
>>> +testNodeAllocPages(virConnectPtr conn,
>>> +   unsigned int npages,
>>> +   unsigned int *pageSizes,
>>> +   unsigned long long *pageCounts,
>>> +   int startCell,
>>> +   unsigned int cellCount,
>>> +   unsigned int flags)
>>> +{
>>> +testDriverPtr privconn = conn->privateData;
>>> +bool add = !(flags & VIR_NODE_ALLOC_PAGES_SET);
>>> +ssize_t i, ncounts = 0;
>>> +size_t j;
>>> +int lastCell;
>>> +int ret = -1;
>>> +
>>> +virCheckFlags(VIR_NODE_ALLOC_PAGES_SET, -1);
>>> +
>>> +testDriverLock(privconn);
>>> +
>>> +lastCell = privconn->numCells - 1;
>>
>> lastCell would be 1 then.
>>
>>> +
>>> +if (startCell < -1 || startCell > lastCell) {
>>> +virReportError(VIR_ERR_INTERNAL_ERROR,
>>> +   _("start cell %d out of range (0-%d)"),
>>> +   startCell, lastCell);
>>> +goto cleanup;
>>> +}
>>> +
>>> +lastCell = MIN(lastCell, startCell + (int) cellCount - 1);
>>
>> if cellCount is 2, then lastCell = MIN(1, -1 + 2 - 1) ?
> 
> Ah. So I haven't though of the case where user wants both behaviours:
> startcell = -1 cellCount = 2. He/she is basically telling us: in the
> first step I want you to allocate the pages through all NUMA nodes, and
> in the second step, I want you to allocate some additional pages on node
> #0. Damn, this is gonna be twice as complicated. Okay, let me rework
> this and post v2.
> 

Well it is a test of some basic world - I'm unclear of the rules of the
game of parameters, I'm reacting to what I see.

It is complicated as you continue to describe below... Glad I'm not the
expert!


John

>>
>>
>> Does the following hunk belong before the startCell < -1 if?
>>
>>> +
>>> +if (startCell == -1) {
>>> +/* Okay, hold on to your hats because this is gonna get wild.
>>> + * startCell == -1 means that user wants us to allocate
>>> + * pages over all NUMA nodes proportionally. Just
>>> + * recalculate the pageCounts and we should be good. */
>>
>> Color me confounded.
>>
>>> +for (j = 0; j < npages; j++)
>>> +pageCounts[j] /= privconn->numCells;
>>
>> Why does this work?  pageCounts[j] would essentially be divided by 2,
>> but isn't that an input value?  How does that make us ensure we're
>> pulling "equally" from across all cells.  That is what's the difference
>> between supplying 0 or -1 for startCell?  I'm missing something...
> 
> Exactly. I mean, if user tells us the following:
> 
> npages = 1, pageSizes= {2048}, pageCounts = {678}, startCell = -1,
> cellCount = 1
> 
> then they want to allocate 678 of 2M pages across the whole host. They
> don't care whether they will be distributed equally throughout all NUMA
> nodes. For what they know, all 678 pages can be allocated on node #0. Or
> you know, any other combination that adds up to 678.
> If, however, they care where the pages are allocated, they have to
> provide startCell != -1. So for instance, if host has 2 NUMA nodes, and
> they want to allocate 678 pages in total and have them distributed
> equally between the two nodes:
> 
> npages =1, pageSizes = {2048}, pageCounts = {339}, startCell = 0,
> cellCount = 2
> 
> Yup, it is very complicated API, because it allows you to allocate more
> page sizes across multiple NUMA nodes.
> 
> Anyway, because this is all emulated in our test driver (and we
> specifically document that we guarantee no equal distribution for
> startCell = -1 case), we can distribute the allocation as we please. So
> I went with equal distribution.
> 
>>
>>> +startCell = 0;
>>> +lastCell = privconn->numCells - 1;
>>
>> So this doesn't change from our default?
>>
>>> +}
>>> +
>>
>> You could probably explain the following to me 5 different ways and each
>> time I'd probably respond, huh?
>>
>>> +for (i = startCell; i <= lastCell; i++) {
>>> +testCellPtr cell = >cells[i];
>>> +size_t 

Re: [libvirt] [PATCH 3/4] test driver: Store memory per NUMA node

[John Ferlan]

On 10/12/2016 10:30 PM, Michal Privoznik wrote:
> On 13.10.2016 05:12, John Ferlan wrote:
>>
>>
>> On 10/05/2016 03:30 AM, Michal Privoznik wrote:
>>> In d18c7d7124 we have tried to implement virNodeGetFreePages API
>>> to test driver. And for a very limited definition of success we
>>> have succeeded. But, we can do better. Firstly, we can teach our
>>> internal representation of a NUMA cell that there are different
>>> page sizes and that they create a pool (from which apps alloc
>>> some). For the demonstration purposes a half of the pool is taken
>>> by default, therefore only the other half is free.
>>>
>>> This representation described above also makes it perfect for
>>> implementing virNodeAllocPages API (yet to come in a subsequent
>>> commit).
>>>
>>> Signed-off-by: Michal Privoznik 
>>> ---
>>>  src/test/test_driver.c | 100 
>>> +
>>>  1 file changed, 76 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
>>> index b760b4f..a3f74f8 100644
>>> --- a/src/test/test_driver.c
>>> +++ b/src/test/test_driver.c
>>> @@ -72,9 +72,16 @@ VIR_LOG_INIT("test.test_driver");
>>>  
>>>  #define MAX_CPUS 128
>>>  
>>> +struct _testPage {
>>> +unsigned long pagesize; /* in KiB */
>>> +unsigned long pages;/* in total, pagesFree should never
>>> +   be greater than this. */
>>> +unsigned long long pagesFree; /* free pages */
>>> +};
>>> +
>>>  struct _testCell {
>>> -unsigned long mem;
>>> -unsigned long freeMem;
>>> +size_t npages;
>>> +struct _testPage *pages;
>>>  int numCpus;
>>>  virCapsHostNUMACellCPU cpus[MAX_CPUS];
>>>  };
>>> @@ -319,16 +326,17 @@ testBuildCapabilities(virConnectPtr conn)
>>>  if (virCapabilitiesAddHostFeature(caps, "nonpae") < 0)
>>>  goto error;
>>>  
>>> -if (VIR_ALLOC_N(caps->host.pagesSize, 2) < 0)
>>> +if (VIR_ALLOC_N(caps->host.pagesSize, privconn->cells[0].npages) < 0)
>>>  goto error;
>>
>> I have very limited knowledge of NUMA/Cell, but I guess I don't
>> understand why the host.pagesSize is only referencing the cells[0]
>> values here and in the subsequent loop. It's all a mis
> 
> 
> Well, so far I'm assuming that all the NUMA nodes support all the sizes
> of huge pages. IOW all the NUMA nodes are the same from this specific
> POV. So if NUMA node #0 supports say 4K and 2M pages, the rest of the
> nodes do as well. Therefore, I don't have to make this more complex than
> it already is.
> 
>>
>> Shouldn't this be checking the various cells[n] for the page sizes
>> supported and then allocating pagesSize based upon the different sizes?
> 
> Yes, if we were to have different sizes for different NUMA nodes. But we
> don't. And frankly, I've never ever seen a machine out there in the wild
> which does have separate page sizes on NUMA nodes. Maybe there is one.
> But even if there is one, question is whether we want the test driver to
> reflect that. And the next question is whether we want to do it in a
> separate patch (and merging this one meanwhile) or in this one.
> 

Guess I was thinking too hard ;-)  in trying to understand the
algorithm, but you're right - this is a test driver for a simple case
and we're taking a shortcut... Something that you just document - what
you were thinking - although perhaps someone else with a deeper
understand of NUMA would say, well duh why document that...

>>
>> Then when filling things in the nPagesSize[n] would be based on the
>> cells page sizes found?
> 
> You mean pageSize[n]? Yes.
> 
>>
>> It just doesn't look right with the usage of [0].  The config has 2
>> cells that each have 2 pages. The host.pagesSize would then be a list of
>> the page sizes found, right?
> 
> Yes. But then again - since all our virtual NUMA nodes for the test
> driver have the same page sizes, we can do this kind of shortcut.
> 
>>
>> Future math could then find the number of pages and pagesFree for each
>> specific size.
>>
>>>  
>>> -caps->host.pagesSize[caps->host.nPagesSize++] = 4;
>>> -caps->host.pagesSize[caps->host.nPagesSize++] = 2048;
>>> +for (i = 0; i < privconn->cells[i].npages; i++)
>>> +caps->host.pagesSize[caps->host.nPagesSize++] = 
>>> privconn->cells[0].pages[i].pagesize;
>>>  
>>>  for (i = 0; i < privconn->numCells; i++) {
>>>  virCapsHostNUMACellCPUPtr cpu_cells;
>>>  virCapsHostNUMACellPageInfoPtr pages;
>>>  size_t nPages;
>>> +unsigned long mem = 0;
>>>  
>>>  if (VIR_ALLOC_N(cpu_cells, privconn->cells[i].numCpus) < 0 ||
>>>  VIR_ALLOC_N(pages, caps->host.nPagesSize) < 0) {
>>> @@ -341,12 +349,14 @@ testBuildCapabilities(virConnectPtr conn)
>>>  memcpy(cpu_cells, privconn->cells[i].cpus,
>>> sizeof(*cpu_cells) * privconn->cells[i].numCpus);
>>>  
>>
>> I would remove the local nPages it's confusing...
> 
> Ah, I felt the opposite. 

Re: [libvirt] [PATCH 4/4] schema: smbios: allow any strings

[John Ferlan]

On 10/10/2016 11:51 AM, Peter Krempa wrote:
> The smbios docs allow any string to be passed and libvirt does not
> really do any validation on them. Allow passing any string.
> 
> Partially resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1373535
> ---
>  docs/schemas/domaincommon.rng | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 

ACK, although couldn't the 's each be
replaced by , too?

John


> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 6eeb4e9..f1609f9 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -4517,9 +4517,7 @@
>
> 
>
> -
> -  [a-zA-Z0-9/\-_\. \(\)]+
> -
> +
>
> 
>
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 3/4] qemu: command: escape smbios entry strings

[John Ferlan]

On 10/10/2016 11:51 AM, Peter Krempa wrote:
> We pass free-form strings from the users to qemu, thus we need escape
> commas since they are passed to qemu monitor.
> 
> Partially resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1373535
> ---
>  src/qemu/qemu_command.c | 102 
> +++-
>  1 file changed, 66 insertions(+), 36 deletions(-)
> 

ACK (oy vei!)

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] util: Fix build on s390

[Paul Eggert]

On 10/13/2016 12:14 PM, Eric Blake wrote:

I think a configure test based on -Werror=format would be
sufficient


What we have now should work for GCC 2.0 and later, whereas 
-Werror=format is a much-newer GCC feature and so would be less portable.



if they are using some other compiler,
then chances are -Wformat does not work, therefore the problem cannot be
portably observed so there is nothing to work around


I am worried that other compilers might warn about it even without 
-Werror=format. (I'm not worried about GCC, as nobody uses GCC 1 any more.)


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/4] qemu: command: Don't bother reporting errors in smbios formatters

[John Ferlan]

On 10/10/2016 11:51 AM, Peter Krempa wrote:
> qemuBuildSmbiosBiosStr and qemuBuildSmbiosSystemStr return NULL if
> there's noting to format on the commandline. Reporting errors from

nothing

> buffer creation doesn't make sense since it would be ignored.

Introduced by 54c0237ccb, so it's been this way a long time...

> ---
>  src/qemu/qemu_command.c | 14 --
>  1 file changed, 14 deletions(-)
> 

I suppose since it seems the only legitimate error you'll hit is ENOMEM
and if you ignore it here, some shortly to be run code is sure to run
into it, then no big deal...

Although, one could argue the callers should check/return on error, but
they'd probably lose that argument.

ACK for what's here

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 1/2] util: Add function to check if string contains some chars

[Sławek Kapłoński]

This new function can be used to check if e.g. name of XML node
don't contains forbidden chars like "/" or new-line.
---
 src/conf/network_conf.c  | 2 +-
 src/libvirt_private.syms | 1 +
 src/util/virstring.c | 9 +
 src/util/virstring.h | 1 +
 4 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index aa39776..949d138 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -2117,7 +2117,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
 goto error;
 }
 
-if (strchr(def->name, '/')) {
+if (virStringHasChars(def->name, "/")) {
 virReportError(VIR_ERR_XML_ERROR,
_("name %s cannot contain '/'"), def->name);
 goto error;
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 55b6a24..6f41234 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2435,6 +2435,7 @@ virStringEncodeBase64;
 virStringFreeList;
 virStringFreeListCount;
 virStringGetFirstWithPrefix;
+virStringHasChars;
 virStringHasControlChars;
 virStringIsEmpty;
 virStringIsPrintable;
diff --git a/src/util/virstring.c b/src/util/virstring.c
index 4a70620..7799d87 100644
--- a/src/util/virstring.c
+++ b/src/util/virstring.c
@@ -975,6 +975,15 @@ virStringStripIPv6Brackets(char *str)
 }
 
 
+bool
+virStringHasChars(const char *str, const char *chars)
+{
+if (strpbrk(str, chars))
+return true;
+return false;
+}
+
+
 static const char control_chars[] =
 "\x01\x02\x03\x04\x05\x06\x07"
 "\x08" /* \t \n */ "\x0B\x0C" /* \r */ "\x0E\x0F"
diff --git a/src/util/virstring.h b/src/util/virstring.h
index 8854d5f..7f2c96d 100644
--- a/src/util/virstring.h
+++ b/src/util/virstring.h
@@ -272,6 +272,7 @@ char *virStringReplace(const char *haystack,
 ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
 
 void virStringStripIPv6Brackets(char *str);
+bool virStringHasChars(const char *str, const char *chars);
 bool virStringHasControlChars(const char *str);
 void virStringStripControlChars(char *str);
 
-- 
2.10.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 2/2] Forbid new-line char in name of new networks

[Sławek Kapłoński]

New line character in name of network is now forbidden because it
mess virsh output and can be confusing for users.
Validation of name is done in network driver, after parsing XML to avoid
problems with dissappeared network which was already created with
new-line char in name.

Closes-Bug: https://bugzilla.redhat.com/show_bug.cgi?id=818064
---
 src/network/bridge_driver.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index b2af482..df85884 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -2973,6 +2973,12 @@ networkValidate(virNetworkDriverStatePtr driver,
 bool bandwidthAllowed = true;
 bool usesInterface = false, usesAddress = false;
 
+if (virStringHasChars(def->name, "\n")) {
+virReportError(VIR_ERR_XML_ERROR,
+   _("name %s cannot contain '\\n'"), def->name);
+return -1;
+}
+
 /* Only the three L3 network types that are configured by libvirt
  * need to have a bridge device name / mac address provided
  */
-- 
2.10.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/4] qemu: command: Fix up coding style of smbios commandine formatters

[John Ferlan]

On 10/10/2016 11:51 AM, Peter Krempa wrote:
> ---
>  src/qemu/qemu_command.c | 13 +
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 

ACK, trivial

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v3 0/2] Forbid new-line char in name of networks

[Sławek Kapłoński]

v2: http://www.redhat.com/archives/libvir-list/2016-October/msg00451.html

Differences in v3:
* function to check string moved from src/util/virxml to src/util/virstring
* validation if name of network contains \n char moved from parsing XML to
  functions responsible for create/define new networks

Sławek Kapłoński (2):
  util: Add function to check if string contains some chars
  Forbid new-line char in name of new networks

 src/conf/network_conf.c | 2 +-
 src/libvirt_private.syms| 1 +
 src/network/bridge_driver.c | 6 ++
 src/util/virstring.c| 9 +
 src/util/virstring.h| 1 +
 5 files changed, 18 insertions(+), 1 deletion(-)

-- 
2.10.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3 18/18] [RFC] qemu: try to put ich9 sound device at 00:1B.0

[Laine Stump]

On 10/13/2016 02:07 PM, Andrea Bolognani wrote:

On Tue, 2016-09-20 at 15:14 -0400, Laine Stump wrote:

Real Q35 hardware has an ICH9 chip that includes several integrated
devices at particular addresses (see the file docs/q35-chipset.cfg in
the qemu source). libvirt already attempts to put the first two sets
of ich9 USB2 controllers it finds at 00:1D.* and 00:1A.* to match the
real hardware. This patch does the same for the ich9 "HD audio"
device.
  
The reason I made this patch is that currently the *only* device in a

reasonable "workstation" type virtual machine config that requires a
legacy PCI slot is the audio device, Without this patch, the standard
Q35 machine created by virt-manager will have a dmi-to-pci-bridge and
a pci-bridge just for the sound device; with the patch (and if you
change the sound device model from the default "ich6" to "ich9"), the
machine definition constructed by virt-manager has absolutely no
legacy PCI controllers - any legacy PCI devices (e.g. video and sound)
are on pcie-root as integrated devices.

Everything past this point, while valuable for the discussion,
should IMHO be left out of the commit message.


Yep. I just had it in the commit message because that's when I was 
thinking about it, and I didn't want to forget later (or even worse - 
have to *re*-type it because something went wrong and I decided to abort 
the send-email and start over).





As cool as it is to have virt-manager making a legacy-PCI-free config
so easily, I'm undecided whether or not this is a worthwhile patch. On
one hand, it's following an existing convention of trying to place
devices that are known to be integrated chipset devices on Q35
hardware at the same address they appear in real life (but doesn't
insist on this address, and doesn't add any new device if one isn't
already present in the config). On the other hand it creates yet
another exception to "follow the same formula for everything", while
it's probably better for us to be trying to *get away* from that.

I'm for merging this: it's straighforward enough, and if we
ever decide to start moving stuff off pcie.0 we can just get
rid of the code you're adding without affecting existing
guests at all.


Okay, that's 2.5 votes in favor and 0 against, so I guess I'll go for it.


Two alternate solutions:
  
1) convince virt-manager to use "ich9" as the default sound for Q35,

 and explicitly place it at 00:1B.0 in the definition it sends to
 libvirt.

After this has been merged, virt-manager will still want to
change its default to ich9 (based on information retrieved
from libosinfo, I assume),


I'm not sure if virt-manager is getting soundcard info from libosinfo or 
just creating it from thin air, but in the case of Q35, it probably 
doesn't apply, because I think libosinfo doesn't know about Q35; most 
likely it's using all the same info as it does for 440fx (and I don't 
think we want it to default to the ich9 audio device on 440fx, since the 
ich9 chipset was used first in the era of Q35, ie it was never put into 
the same motherboard as a 440fx chipset.


But yeah, virt-manager needs to change its default. I had asked Cole 
about it on IRC one day, but maybe I should do that in a more formal manner.



  but it will not need to hardcode
this kind of knowledge, which sounds perfectly fine to me.


Yeah, I agree that (1) is a bad idea. I just put it there to be thorough :-)




2) convince qemu to add a PCI Express sound device (I'm not sure which
 one would be most appropriate).

That would probably be nice to have, but I'd rather have
generic PCI/PCIe controllers than a PCIe sound card, if any
QEMU developer is reading this ;)


Yeah, I think I agree with that.




---
   src/qemu/qemu_domain_address.c |  25 +
   .../qemuxml2argv-q35-virt-manager-basic.args   |  56 ++
   .../qemuxml2argv-q35-virt-manager-basic.xml|  76 ++
   tests/qemuxml2argvtest.c   |  31 ++
   .../qemuxml2xmlout-q35-virt-manager-basic.xml  | 116 
+
   tests/qemuxml2xmltest.c|  23 
   6 files changed, 327 insertions(+)
   create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-q35-virt-manager-basic.args
   create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-q35-virt-manager-basic.xml
   create mode 100644 
tests/qemuxml2xmloutdata/qemuxml2xmlout-q35-virt-manager-basic.xml
  
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c

index a9c4c32..6cfd710 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -1218,6 +1218,31 @@ qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def,
   goto cleanup;
   }
   }
+
+memset(_addr, 0, sizeof(tmp_addr));
+tmp_addr.slot = 0x1B;
+if (!virDomainPCIAddressSlotInUse(addrs, _addr)) {
+/* Since real Q35 hardware has an ICH9 chip that has an
+ * integrated HD audio 

Re: [libvirt] [PATCH] support auth for qemu SCSI hotplug

[John Ferlan]

On 10/09/2016 11:51 AM, Gema Gomez wrote:
> Hi all,
> 
> commit fceeeda2 added support for adding key objects on hotplug based on a 
> disk's secinfo for normal drives, but missed out SCSI drives. This patch adds 
> the same support for SCSI drives, so that it's possible to hotplug SCSI 
> drives requiring authentication (e.g. rbd-backed drives).
> 
> Cheers,
> Gema
> 

Need to teach your mailer to truncate long lines...


So could you provide a bit more information about the configuration.
Are you indicating that you have an RBD pool with a volume that's being
used as a SCSI device on the guest?

Reason I ask - not modifying qemuDomainAttachSCSIDisk was by choice
mainly because it's generally used with the iSCSI pool which at this
point in time cannot support this new secret model.

Tks,

John

> 
> Gema Gomez (1):
>   qemu: hotplug: support auth for scsi hotplug
> 
>  src/qemu/qemu_hotplug.c | 21 +
>  1 file changed, 21 insertions(+)
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] libvirt API / balloon last-update

[John Ferlan]

On 10/11/2016 03:15 PM, Marko Myllynen wrote:
> Hi,
> 
> I see that recently the last-update field has been added to the balloon
> statistics, this is how it looks like from Python:
> 
> [(, {'balloon.rss': 960048L,
> 'balloon.swap_in': 0L, 'balloon.usable': 1676424L, 'balloon.unused':
> 1376188L, 'balloon.major_fault': 816L, 'balloon.swap_out': 0L,
> 'balloon.current': 2097152L, 'balloon.maximum': 2097152L,
> 'balloon.available': 2048364L, 'balloon.minor_fault': 460160L,
> 'balloon.last-update': 1476213030L})]
> 
> As you can see, there's an inconsistency as all the other members use
> underscore (_), not dash (-). This actually matters with the PCP plugin
> since PCP metric names can contain _ but not -.

Looks like it was actually added as an output in 2.1.0

http://libvirt.org/git/?p=libvirt.git;a=commit;h=200a40f94ec9427eb7187d9d5396ad3a3f2925c8

There were continued adjustments:

commit id: f57fbd6c4a5dfc8e725f036791e6333d1ab6f04e
commit id: 438c204763f7d8eed79554075f5633545a4a5df1

Since this is output only and we're not setting it for input, it's
almost too bad we cannot treat this as a spelling error and change it. I
tried looking through history for other cross version instances of
changing output, but didn't find any. Closest I could come was the
blkiotune parameters which can take both _ and - for their names, but
display _ in the name.

> 
> Now that last-update is part of the API in 2.3 I guess it's too late to
> change this but it would nice if in the future the member names could be
> kept consistent and underscore used for new members, as it has been so far.
> 

Hard to be always vigilant... even harder when input parameters prefer
the dash model (see blkdeviotune for example). Sometimes things slip
through the cracks though.

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] util: Fix build on s390

[Eric Blake]

On 10/13/2016 01:50 PM, Eric Blake wrote:
> On 10/13/2016 01:23 PM, Paul Eggert wrote:
>> On 10/13/2016 06:36 AM, Eric Blake wrote:
>>> That's a bug in s390's system headers.  Gnulib should be taught to work
>>> around it.
>>
>> Although this bug was reported fixed in glibc 2.20; see
>>
>> https://sourceware.org/bugzilla/show_bug.cgi?id=16712
>>
>> the Gnulib manual says the bug is still in glibc 2.24; see
>> gnulib/doc/posix-headers/stdint.texi. Is the Gnulib manual correct? If
>> so, why didn't the earlier glibc patch fix the bug?
>>
>> Anyway, I installed into Gnulib the attached patch, which I hope works
>> around the bug. I can't easily test this, so please give it a try. And
>> if you can think of a way to test whether SIZE_MAX has the correct type
>> on pre-C11 compilers that lack __typeof__, please let me know.
> 
> With gcc, -Werror=format flags a mismatch on printf("%zu",SIZE_MAX).
> 
> With C++, you can check whether a correct overloaded function is called.
> 
> But I don't have any off-hand way of testing it without using compiler
> extensions or a different language.

That said, I think a configure test based on -Werror=format would be
sufficient - after all, our objective is only to fix the problem if it
can be observed.  If a platform is using pre-C11 gcc and the problem is
observable, we want it fixed; if they are using some other compiler,
then chances are -Wformat does not work, therefore the problem cannot be
portably observed so there is nothing to work around, whether or not the
problem is present.  That's the beauty of testing features rather than
compiler versions :)

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] util: Fix build on s390

[Eric Blake]

On 10/13/2016 01:23 PM, Paul Eggert wrote:
> On 10/13/2016 06:36 AM, Eric Blake wrote:
>> That's a bug in s390's system headers.  Gnulib should be taught to work
>> around it.
> 
> Although this bug was reported fixed in glibc 2.20; see
> 
> https://sourceware.org/bugzilla/show_bug.cgi?id=16712
> 
> the Gnulib manual says the bug is still in glibc 2.24; see
> gnulib/doc/posix-headers/stdint.texi. Is the Gnulib manual correct? If
> so, why didn't the earlier glibc patch fix the bug?
> 
> Anyway, I installed into Gnulib the attached patch, which I hope works
> around the bug. I can't easily test this, so please give it a try. And
> if you can think of a way to test whether SIZE_MAX has the correct type
> on pre-C11 compilers that lack __typeof__, please let me know.

With gcc, -Werror=format flags a mismatch on printf("%zu",SIZE_MAX).

With C++, you can check whether a correct overloaded function is called.

But I don't have any off-hand way of testing it without using compiler
extensions or a different language.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3 15/18] qemu: auto-add pcie-root-port/dmi-to-pci-bridge controllers as needed

[Laine Stump]

On 10/13/2016 01:43 PM, Laine Stump wrote:

On 10/11/2016 05:34 AM, Andrea Bolognani wrote:

On Mon, 2016-10-10 at 15:43 -0400, Laine Stump wrote:

3) Although it is strongly discouraged, it is legal for a pci-bridge
   to be directly plugged into pcie-root, and we don't want to
   auto-add a dmi-to-pci-bridge if there is already a pci-bridge
   that's been forced directly into pcie-root. Finally, 
although I
   fail to see the utility of it, it is legal to have 
something like

   this in the xml:
 
 
   and that will lead to an automatically added 
dmi-to-pci-bridge at
   index=1 (to give the unaddressed pci-bridge a proper place 
to plug

   in):
 index='1'/>

   (for example, see the existing test case
   "usb-controller-default-q35"). This is handled in
   qemuDomainPCIAddressSetCreate() when it's adding in 
controllers to

   fill holes in the indexes.

  I wonder how this "feature" came to be... It seems to be the
reason for quite a bit of convoluted code down below, which
we could avoid if this had never worked at all. As is so
often the case, too late for that :(

  Maybe not. The only place I ever saw that was in the above test case,
and another named "usb-controller-explicit-q35", and the author of both
of those tests was (wait for it!), no, not Albert Einstein. Andrea
Bolognani!

Oh, *that* guy! It's always *that* guy, isn't it? :P


The only reason it worked in the past was because we always
automatically added the dmi-to-pci-bridge very early in the post-parse
processing. This implies that any saved config anywhere will already
have the necessary dmi-to-pci-bridge at index='1', so we only need to
preserve the behavior for new domain definitions that have a pci-bridge
at index='2' but nothing at index='1'.
  Since you're the only person documented to have ever created a config
like that, and it would only be problematic if someone tried to create
another new config, maybe we should just stop accidentally 
supporting it

and count it as a bug that's being fixed. What's your opinion?

Given the evidence you're presenting, I'm all for getting
rid of it, especially since it will make the code below
much simpler and hence more maintainable.

Considering how critical that part of libvirt is, anything
we can do to make it leaner and meaner is going to be a huge
win in the long run.


I'm looking back through the code and wondering how to simplify it - 
it may be that the alternate method I had initially used (which failed 
that one test) is just as complicated as what I have now; 
unfortunately I didn't save it.


Okay, now I've reminded myself of what I did. Basically everything 
necessary for that is the changes to qemuDomainPCIAddressSetCreate() 
dealing with "lowest*Bridge" (see the patch excerpt below). I would 
still need to patch this function even if we didn't support "auto-adding 
dmi-to-pci-bridge when a pci-bridge is present in the config", but it 
would be slightly simpler.


I'll split it into a separate patch so that it's more apparent, and we 
can decide based on that.



On 09/20/2016 03:14 PM, Laine Stump wrote:

diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index 1ff80e3..a9c4c32 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -797,6 +797,11 @@ qemuDomainPCIAddressSetCreate(virDomainDefPtr def,
  {
  virDomainPCIAddressSetPtr addrs;
  size_t i;
+bool hasPCIeRoot = false;
+int lowestDMIToPCIBridge = nbuses;
+int lowestUnaddressedPCIBridge = nbuses;
+int lowestAddressedPCIBridge = nbuses;
+virDomainControllerModelPCI defaultModel;
  
  if ((addrs = virDomainPCIAddressSetAlloc(nbuses)) == NULL)

  return NULL;
@@ -804,38 +809,84 @@ qemuDomainPCIAddressSetCreate(virDomainDefPtr def,
  addrs->nbuses = nbuses;
  addrs->dryRun = dryRun;
  
-/* As a safety measure, set default model='pci-root' for first pci

- * controller and 'pci-bridge' for all subsequent. After setting
- * those defaults, then scan the config and set the actual model
- * for all addrs[idx]->bus that already have a corresponding
- * controller in the config.
- *
- */
-if (nbuses > 0)
-virDomainPCIAddressBusSetModel(>buses[0],
-   VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT);
-for (i = 1; i < nbuses; i++) {
-virDomainPCIAddressBusSetModel(>buses[i],
-   VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE);
-}
-
  for (i = 0; i < def->ncontrollers; i++) {
-size_t idx = def->controllers[i]->idx;
+virDomainControllerDefPtr cont = def->controllers[i];
+size_t idx = cont->idx;
  
-if (def->controllers[i]->type != VIR_DOMAIN_CONTROLLER_TYPE_PCI)

+if (cont->type != VIR_DOMAIN_CONTROLLER_TYPE_PCI)
  continue;
  
  if (idx >= addrs->nbuses) {

  

Re: [libvirt] [PATCH] util: Fix build on s390

[Paul Eggert]

On 10/13/2016 06:36 AM, Eric Blake wrote:

That's a bug in s390's system headers.  Gnulib should be taught to work
around it.


Although this bug was reported fixed in glibc 2.20; see

https://sourceware.org/bugzilla/show_bug.cgi?id=16712

the Gnulib manual says the bug is still in glibc 2.24; see 
gnulib/doc/posix-headers/stdint.texi. Is the Gnulib manual correct? If 
so, why didn't the earlier glibc patch fix the bug?


Anyway, I installed into Gnulib the attached patch, which I hope works 
around the bug. I can't easily test this, so please give it a try. And 
if you can think of a way to test whether SIZE_MAX has the correct type 
on pre-C11 compilers that lack __typeof__, please let me know.


From df544bd79a83880cc3287d43c5be1719e1ca2ccf Mon Sep 17 00:00:00 2001
From: Paul Eggert 
Date: Thu, 13 Oct 2016 11:16:40 -0700
Subject: [PATCH] stdint: port SIZE_MAX to glibc s390

Problem reported by Eric Blake in:
http://lists.gnu.org/archive/html/bug-gnulib/2016-10/msg00031.html
* doc/posix-headers/stdint.texi (stdint.h): Document the fix.
* m4/stdint.m4 (gl_STDINT_H): Check that SIZE_MAX has the
correct type, if possible.
---
 ChangeLog |  9 +
 doc/posix-headers/stdint.texi |  8 
 m4/stdint.m4  | 11 ++-
 3 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 070e498..89ba80f 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,12 @@
+2016-10-13  Paul Eggert  
+
+	stdint: port SIZE_MAX to glibc s390
+	Problem reported by Eric Blake in:
+	http://lists.gnu.org/archive/html/bug-gnulib/2016-10/msg00031.html
+	* doc/posix-headers/stdint.texi (stdint.h): Document the fix.
+	* m4/stdint.m4 (gl_STDINT_H): Check that SIZE_MAX has the
+	correct type, if possible.
+
 2016-10-13  Daniel Richard G.  
 
 	getprogname: port to IBM z/OS
diff --git a/doc/posix-headers/stdint.texi b/doc/posix-headers/stdint.texi
index 2cc083c..d5ca3c2 100644
--- a/doc/posix-headers/stdint.texi
+++ b/doc/posix-headers/stdint.texi
@@ -34,6 +34,10 @@ constant macros such as @code{INTMAX_C}, and one must define
 @code{__STDC_LIMIT_MACROS} to make visible the definitions of limit
 macros such as @code{INTMAX_MAX}.
 @item
+The macro @code{SIZE_MAX} has the wrong type,
+albeit with the correct value:
+32-bit glibc 2.24 (on s390 architecture), Mac OS X 10.7.
+@item
 Macros like @code{INTMAX_WIDTH} are not defined on some platforms:
 glibc 2.24, many others.
 @end itemize
@@ -48,10 +52,6 @@ so public header files should avoid these types.
 @item
 Macros are used instead of typedefs.
 @item
-The macro @code{SIZE_MAX} has the wrong type,
-albeit with the correct value:
-32-bit glibc 2.24 (on s390 architecture), Mac OS X 10.7.
-@item
 Some C preprocessors mishandle constants that do not fit in @code{long int}.
 For example, as of 2007, Sun C mishandles @code{#if LLONG_MIN < 0} on
 a platform with 32-bit @code{long int} and 64-bit @code{long long int}.
diff --git a/m4/stdint.m4 b/m4/stdint.m4
index fa6f103..05b6ab7 100644
--- a/m4/stdint.m4
+++ b/m4/stdint.m4
@@ -1,4 +1,4 @@
-# stdint.m4 serial 47
+# stdint.m4 serial 48
 dnl Copyright (C) 2001-2016 Free Software Foundation, Inc.
 dnl This file is free software; the Free Software Foundation
 dnl gives unlimited permission to copy and/or distribute it,
@@ -154,6 +154,15 @@ uintptr_t h = UINTPTR_MAX;
 intmax_t i = INTMAX_MAX;
 uintmax_t j = UINTMAX_MAX;
 
+/* Check that SIZE_MAX has the correct type, if possible.  */
+#if 201112 <= __STDC_VERSION__
+int k = _Generic (SIZE_MAX, size_t: 0);
+#elif (2 <= __GNUC__ || defined __IBM__TYPEOF__ \
+   || (0x5110 <= __SUNPRO_C && !__STDC__))
+extern size_t k;
+extern __typeof__ (SIZE_MAX) k;
+#endif
+
 #include  /* for CHAR_BIT */
 #define TYPE_MINIMUM(t) \
   ((t) ((t) 0 < (t) -1 ? (t) 0 : ~ TYPE_MAXIMUM (t)))
-- 
2.7.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3 18/18] [RFC] qemu: try to put ich9 sound device at 00:1B.0

[Andrea Bolognani]

On Tue, 2016-09-20 at 15:14 -0400, Laine Stump wrote:
> Real Q35 hardware has an ICH9 chip that includes several integrated
> devices at particular addresses (see the file docs/q35-chipset.cfg in
> the qemu source). libvirt already attempts to put the first two sets
> of ich9 USB2 controllers it finds at 00:1D.* and 00:1A.* to match the
> real hardware. This patch does the same for the ich9 "HD audio"
> device.
> 
> The reason I made this patch is that currently the *only* device in a
> reasonable "workstation" type virtual machine config that requires a
> legacy PCI slot is the audio device, Without this patch, the standard
> Q35 machine created by virt-manager will have a dmi-to-pci-bridge and
> a pci-bridge just for the sound device; with the patch (and if you
> change the sound device model from the default "ich6" to "ich9"), the
> machine definition constructed by virt-manager has absolutely no
> legacy PCI controllers - any legacy PCI devices (e.g. video and sound)
> are on pcie-root as integrated devices.

Everything past this point, while valuable for the discussion,
should IMHO be left out of the commit message.

> As cool as it is to have virt-manager making a legacy-PCI-free config
> so easily, I'm undecided whether or not this is a worthwhile patch. On
> one hand, it's following an existing convention of trying to place
> devices that are known to be integrated chipset devices on Q35
> hardware at the same address they appear in real life (but doesn't
> insist on this address, and doesn't add any new device if one isn't
> already present in the config). On the other hand it creates yet
> another exception to "follow the same formula for everything", while
> it's probably better for us to be trying to *get away* from that.

I'm for merging this: it's straighforward enough, and if we
ever decide to start moving stuff off pcie.0 we can just get
rid of the code you're adding without affecting existing
guests at all.

> Two alternate solutions:
> 
> 1) convince virt-manager to use "ich9" as the default sound for Q35,
>and explicitly place it at 00:1B.0 in the definition it sends to
>libvirt.

After this has been merged, virt-manager will still want to
change its default to ich9 (based on information retrieved
from libosinfo, I assume), but it will not need to hardcode
this kind of knowledge, which sounds perfectly fine to me.

> 2) convince qemu to add a PCI Express sound device (I'm not sure which
>one would be most appropriate).

That would probably be nice to have, but I'd rather have
generic PCI/PCIe controllers than a PCIe sound card, if any
QEMU developer is reading this ;)

> ---
>  src/qemu/qemu_domain_address.c |  25 +
>  .../qemuxml2argv-q35-virt-manager-basic.args   |  56 ++
>  .../qemuxml2argv-q35-virt-manager-basic.xml|  76 ++
>  tests/qemuxml2argvtest.c   |  31 ++
>  .../qemuxml2xmlout-q35-virt-manager-basic.xml  | 116 
>+
>  tests/qemuxml2xmltest.c|  23 
>  6 files changed, 327 insertions(+)
>  create mode 100644 
>tests/qemuxml2argvdata/qemuxml2argv-q35-virt-manager-basic.args
>  create mode 100644 
>tests/qemuxml2argvdata/qemuxml2argv-q35-virt-manager-basic.xml
>  create mode 100644 
>tests/qemuxml2xmloutdata/qemuxml2xmlout-q35-virt-manager-basic.xml
> 
> diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
> index a9c4c32..6cfd710 100644
> --- a/src/qemu/qemu_domain_address.c
> +++ b/src/qemu/qemu_domain_address.c
> @@ -1218,6 +1218,31 @@ qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr 
> def,
>  goto cleanup;
>  }
>  }
> +
> +memset(_addr, 0, sizeof(tmp_addr));
> +tmp_addr.slot = 0x1B;
> +if (!virDomainPCIAddressSlotInUse(addrs, _addr)) {
> +/* Since real Q35 hardware has an ICH9 chip that has an
> + * integrated HD audio device at :00:1B.0 put any
> + * unaddressed ICH9 audio device at that address if it's not
> + * already taken. If there's something already there, let the
> + * normal device addressing assign something later.
> + */
> +for (i = 0; i < def->nsounds; i++) {
> +virDomainSoundDefPtr sound = def->sounds[i];
> +
> +if (sound->model != VIR_DOMAIN_SOUND_MODEL_ICH9)
> +continue;
> +if (!virDeviceInfoPCIAddressWanted(>info))
> +break;

Mh, why break instead of continue here?

I'm assuming people won't have more than one ich9 sound card
per guest, but if they did and one of them already had a PCI
address assigned (which is not :00:1B.0 because of the
outer check) why wouldn't we want to try assigning the next
one to :00:1B.0?

> +if (virDomainPCIAddressReserveSlot(addrs, _addr, flags) < 0)
> +goto cleanup;

Add an empty line here, please.

> +sound->info.type = 

Re: [libvirt] [PATCH v3 15/18] qemu: auto-add pcie-root-port/dmi-to-pci-bridge controllers as needed

[Laine Stump]

On 10/10/2016 02:14 PM, Andrea Bolognani wrote:

+} else if (flags & VIR_PCI_CONNECT_TYPE_PCI_BRIDGE) {
>+model = VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE;

I'm not so clear on this bit: if we're growing the address
set to plug in a pci-bridge, can we just go ahead and assume
a dmi-to-pci-bridge is what we need? What about eg. pseries
machine types, where dmi-to-pci-bridge is not usable?



What can you plug a pci-bridge into on a pseries machine? For all 
machinetypes, if there is an unaddressed pci-bridge in the config and 
there is a slot available that accepts a pci-bridge (i.e. pci-root, 
pci-bridge, or dmi-to-pci-bridge) then we won't be calling 
virDomainPCIAddressSetGrow() in the first place (since it's only called 
if no empty slot with correct flags can be found). And I don't know 
about pseries, but for 440fx, if there isn't a matching empty slot that 
accepts a pci-bridge at that point, then it's not possible to add one 
(you already have the one and only pci-root, and you're already trying 
to add a pci-bridge --> if you have to add a pci-bridge in order to add 
a pci-bridge, then you're in an infinite recursion).



So a dmi-to-pci-bridge is the only thing that could possibly help, and 
in the cases where there is no pcie-root, it would just mean that you 
would get an error later when you're told there's no place to plug in 
the dmi-to-pci-bridge. So it makes sense for me to add a check here to 
see if the model of addrs->buses[0] is PCIE_ROOT, and only try adding 
the dmi-to-pci-bridge in that case.


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] dpdk/vpp and cross-version migration for vhost

[Michael S. Tsirkin]

Hi!
So it looks like we face a problem with cross-version
migration when using vhost. It's not new but became more
acute with the advent of vhost user.

For users to be able to migrate between different versions
of the hypervisor the interface exposed to guests
by hypervisor must stay unchanged.

The problem is that a qemu device is connected
to a backend in another process, so the interface
exposed to guests depends on the capabilities of that
process.

Specifically, for vhost user interface based on virtio, this includes
the "host features" bitmap that defines the interface, as well as more
host values such as the max ring size.  Adding new features/changing
values to this interface is required to make progress, but on the other
hand we need ability to get the old host features to be compatible.

To solve this problem within qemu, qemu has a versioning system based on
a machine type concept which fundamentally is a version string, by
specifying that string one can get hardware compatible with a previous
qemu version. QEMU also reports the latest version and list of versions
supported so libvirt records the version at VM creation and then is
careful to use this machine version whenever it migrates a VM.

One might wonder how is this solved with a kernel vhost backend. The
answer is that it mostly isn't - instead an assumption is made, that
qemu versions are deployed together with the kernel - this is generally
true for downstreams.  Thus whenever qemu gains a new feature, it is
already supported by the kernel as well.  However, if one attempts
migration with a new qemu from a system with a new to old kernel, one
would get a failure.

In the world where we have multiple userspace backends, with some of
these supplied by ISVs, this seems non-realistic.

IMO we need to support vhost backend versioning, ideally
in a way that will also work for vhost kernel backends.

So I'd like to get some input from both backend and management
developers on what a good solution would look like.

If we want to emulate the qemu solution, this involves adding the
concept of interface versions to dpdk.  For example, dpdk could supply a
file (or utility printing?) with list of versions: latest and versions
supported. libvirt could read that and
- store latest version at vm creation
- pass it around with the vm
- pass it to qemu

>From here, qemu could pass this over the vhost-user channel,
thus making sure it's initialized with the correct
compatible interface.

As version here is an opaque string for libvirt and qemu,
anything can be used - but I suggest either a list
of values defining the interface, e.g.
any_layout=on,max_ring=256
or a version including the name and vendor of the backend,
e.g. "org.dpdk.v4.5.6".

Note that typically the list of supported versions can only be
extended, not shrunk. Also, if the host/guest interface
does not change, don't change the current version as
this just creates work for everyone.

Thoughts? Would this work well for management? dpdk? vpp?

Thanks!

-- 
MST

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3 15/18] qemu: auto-add pcie-root-port/dmi-to-pci-bridge controllers as needed

[Laine Stump]

On 10/11/2016 05:34 AM, Andrea Bolognani wrote:

On Mon, 2016-10-10 at 15:43 -0400, Laine Stump wrote:

3) Although it is strongly discouraged, it is legal for a pci-bridge
   to be directly plugged into pcie-root, and we don't want to
   auto-add a dmi-to-pci-bridge if there is already a pci-bridge
   that's been forced directly into pcie-root. Finally, although I
   fail to see the utility of it, it is legal to have something like
   this in the xml:

 

 

   and that will lead to an automatically added dmi-to-pci-bridge at

   index=1 (to give the unaddressed pci-bridge a proper place to plug
   in):

 

   (for example, see the existing test case

   "usb-controller-default-q35"). This is handled in
   qemuDomainPCIAddressSetCreate() when it's adding in controllers to
   fill holes in the indexes.
  
I wonder how this "feature" came to be... It seems to be the

reason for quite a bit of convoluted code down below, which
we could avoid if this had never worked at all. As is so
often the case, too late for that :(
  
Maybe not. The only place I ever saw that was in the above test case,

and another named "usb-controller-explicit-q35", and the author of both
of those tests was (wait for it!), no, not Albert Einstein.  Andrea
Bolognani!

Oh, *that* guy! It's always *that* guy, isn't it? :P


The only reason it worked in the past was because we always
automatically added the dmi-to-pci-bridge very early in the post-parse
processing. This implies that any saved config anywhere will already
have the necessary dmi-to-pci-bridge at index='1', so we only need to
preserve the behavior for new domain definitions that have a pci-bridge
at index='2' but nothing at index='1'.
  
Since you're the only person documented to have ever created a config

like that, and it would only be problematic if someone tried to create
another new config, maybe we should just stop accidentally supporting it
and count it as a bug that's being fixed. What's your opinion?

Given the evidence you're presenting, I'm all for getting
rid of it, especially since it will make the code below
much simpler and hence more maintainable.

Considering how critical that part of libvirt is, anything
we can do to make it leaner and meaner is going to be a huge
win in the long run.


I'm looking back through the code and wondering how to simplify it - it 
may be that the alternate method I had initially used (which failed that 
one test) is just as complicated as what I have now; unfortunately I 
didn't save it.





@@ -82,6 +82,30 @@ 
virDomainPCIControllerModelToConnectType(virDomainControllerModelPCI model)
 return 0;
 }
 
+

+static int

s/int/virDomainControllerModelPCI/
  
But then we can't return -1 when there isn't a perfect match (that's why

I made it int)

Right. Disregard my comments then :)


Alternatively you could turn it into
  
 if ((flags & VIR_PCI_CONNECT_PCIE_DEVICE) ||

 (flags & VIR_PCI_CONNECT_PCIE_SWITCH_UPSTREAM_PORT))
  
which is more obviously correct and also nicer to look at :)
  
but takes two operations instead of one.

I hardly think this would turn out to be a bottleneck, but
feel free to stick to the original implementation - after
fixing it, of course :)


+if (lowestDMIToPCIBridge > idx)
+lowestDMIToPCIBridge = idx;
+} else if (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE) {
+if (virDeviceInfoPCIAddressWanted(>info)) {
+if (lowestUnaddressedPCIBridge > idx)
+lowestUnaddressedPCIBridge = idx;
+} else {
+if (lowestAddressedPCIBridge > idx)
+lowestAddressedPCIBridge = idx;
+}
 }
+}
+
+if (nbuses > 0 && !addrs->buses[0].model) {
+if (virDomainPCIAddressBusSetModel(>buses[0],
+   VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) 
< 0)
+goto error;
+}
  
Shouldn't we use either PCI_ROOT or PCIE_ROOT based on the

machine type here? And set hasPCIeRoot *after* doing so?
  
Sorry for the questions, I guess this is the point in the

patch where I got a bit lost :(

I'm afraid you missed this question :)



Sorry about the omission.  I've tried to limit the code that decides 
whether or not there should be a pci-root or a pcie-root to the one 
place when default controllers are being added (I forget the name of the 
function right now), and after that only decide based on whether a 
pci-root or pcie-root really is there in the config. My subconscious 
reasoning for this is that the extra calisthenics around supporting 
aarch64/virt with PCI(e) vs with mmio has made me wonder if there might 
be machinetypes in the future that could support one type of root bus or 
another (or none) according to what is in the config, rather than having 
a root bus just builtin to the machine. I don't 

Re: [libvirt] [PATCH v3 16/18] qemu: don't force-add a dmi-to-pci-bridge just on principle

[Laine Stump]

On 10/13/2016 11:50 AM, Andrea Bolognani wrote:

On Tue, 2016-09-20 at 15:14 -0400, Laine Stump wrote:

Now the a dmi-to-pci-bridge is automatically added just as it's needed
(when a pci-bridge is being added), we no longer have any need to
force-add one to every single Q35 domain.
---
   src/qemu/qemu_domain.c | 12 
   tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args |  3 +-
   .../qemuxml2argv-q35-pcie-autoadd.args | 55 +++
   .../qemuxml2xmlout-pcie-root.xml   |  4 --
   .../qemuxml2xmlout-q35-pcie-autoadd.xml| 82 
++
   5 files changed, 67 insertions(+), 89 deletions(-)

You didn't CC: me on this patch for some reason. Weird.


I should learn to use --cc in git send-email rather than pasting Cc: 
lines into the individual messages with --annotate.



ACK


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3 17/18] qemu: add a USB3 controller to Q35 domains by default

[Andrea Bolognani]

On Tue, 2016-09-20 at 15:14 -0400, Laine Stump wrote:
> Previously we added a set of EHCI+UHCI controllers to Q35 machines to
> mimic real hardware as closely as possible, but recent discussions
> have pointed out that the nec-usb-xhci (USB3) controller is much more
> virtualization-friendly (uses less CPU), so this patch switches the
> default for Q35 machinetypes to add an XHCI instead (if it's
> supported, which it of course *will* be).
> 
> Since none of the existing test cases left out USB controllers in the
> input XML, a new Q35 test case was added which has *no* devices, so
> ends up with only the defaults always put in by qemu, plus those added
> by libvirt.
> ---
>  src/qemu/qemu_domain.c | 10 --
>  .../qemuxml2argv-q35-default-devices-only.args | 22 
>  .../qemuxml2argv-q35-default-devices-only.xml  | 18 ++
>  tests/qemuxml2argvtest.c   | 22 
>  .../qemuxml2xmlout-q35-default-devices-only.xml| 40 
> ++
>  tests/qemuxml2xmltest.c| 22 
>  6 files changed, 131 insertions(+), 3 deletions(-)
>  create mode 100644 
> tests/qemuxml2argvdata/qemuxml2argv-q35-default-devices-only.args
>  create mode 100644 
> tests/qemuxml2argvdata/qemuxml2argv-q35-default-devices-only.xml
>  create mode 100644 
> tests/qemuxml2xmloutdata/qemuxml2xmlout-q35-default-devices-only.xml

ACK

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] Cpu Modeling

[Jiri Denemark]

On Wed, Oct 12, 2016 at 13:55:49 -0400, Jason J. Herne wrote:
> In order to properly obtain the host cpu model data for virsh
> capabilities on s390 we will need to run a Qemu process. There is no
> precedent for doing this in a cpu connection driver today.

There are two reasons for this:

1) it was not required so far
2) it doesn't belong to the CPU driver

> 1. The s390 cpu connection driver (src/cpu/cpu_s390.c) can just
> privately make use of the default Qemu binary and the appropriate qmp
> calls can be made to get the model name and possibly features.

No, the CPU driver is supposed to do generic stuff independently on any
hypervisor.

> 2. We can extend the existing interface somehow so all archs could
> make use of a hypervisor instance for nodeData() and decode()
> operations. Perhaps a new set of callbacks? nodeDataWithHypervisor(),
> decodeWithHypervisor() ?

Why? The hypervisor code that calls CPU driver APIs can just decide to
do something else for certain architectures.

> I'm trying to get the host cpu model for virCapsPtr caps, as it is
> filled in via virQEMUCapsInit --> virQEMUCapsInitCPU. And subsequently
> referenced for the output of virsh capabilities and copied into
> qemuCaps for reference by virsh domcapabilities.

Anyway, host CPU capabilities in virCapsPtr is not supposed to contain
the CPU description queried from QEMU. If we have no generic code to
detect host CPU ourselves, we should just keep host CPU model empty.

The host CPU model queried from QEMU should be placed in virQEMUCapsPtr.
When probing QEMU, we cache data we get from QEMU and the host CPU model
is supposed to be computed virQEMUCapsInitHostCPUModel from the cached
data. (Currently, the function copies the model from virCapsPtr, but
that's only because we don't query QEMU for the right stuff yet.)

Jirka

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [libvirt-perl][PATCH] Add VIR_DOMAIN_VCPU_HOTPLUGGABLE constant

[John Ferlan]

On 10/13/2016 11:49 AM, Michal Privoznik wrote:
> Signed-off-by: Michal Privoznik 
> ---
>  Changes| 2 +-
>  Virt.xs| 1 +
>  lib/Sys/Virt/Domain.pm | 4 
>  3 files changed, 6 insertions(+), 1 deletion(-)
> 

ACK (I have it queued up too, but the 2nd round iotune changes haven't
been reviewed yet, so I was waiting).

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3 16/18] qemu: don't force-add a dmi-to-pci-bridge just on principle

[Andrea Bolognani]

On Tue, 2016-09-20 at 15:14 -0400, Laine Stump wrote:
> Now the a dmi-to-pci-bridge is automatically added just as it's needed
> (when a pci-bridge is being added), we no longer have any need to
> force-add one to every single Q35 domain.
> ---
>  src/qemu/qemu_domain.c | 12 
>  tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args |  3 +-
>  .../qemuxml2argv-q35-pcie-autoadd.args | 55 +++
>  .../qemuxml2xmlout-pcie-root.xml   |  4 --
>  .../qemuxml2xmlout-q35-pcie-autoadd.xml| 82 
>++
>  5 files changed, 67 insertions(+), 89 deletions(-)

You didn't CC: me on this patch for some reason. Weird.

ACK

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [libvirt-perl][PATCH] Add VIR_DOMAIN_VCPU_HOTPLUGGABLE constant

[Michal Privoznik]

Signed-off-by: Michal Privoznik 
---
 Changes| 2 +-
 Virt.xs| 1 +
 lib/Sys/Virt/Domain.pm | 4 
 3 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/Changes b/Changes
index 4985100..5831939 100644
--- a/Changes
+++ b/Changes
@@ -2,7 +2,7 @@ Revision history for perl module Sys::Virt
 
 2.4.0 2016-00-00
 
- - XXX
+ - Add VIR_DOMAIN_VCPU_HOTPLUGGABLE constant
 
 2.3.0 2016-10-06
 
diff --git a/Virt.xs b/Virt.xs
index b4ca049..c1d923c 100644
--- a/Virt.xs
+++ b/Virt.xs
@@ -8309,6 +8309,7 @@ BOOT:
   REGISTER_CONSTANT(VIR_DOMAIN_VCPU_CONFIG, VCPU_CONFIG);
   REGISTER_CONSTANT(VIR_DOMAIN_VCPU_MAXIMUM, VCPU_MAXIMUM);
   REGISTER_CONSTANT(VIR_DOMAIN_VCPU_GUEST, VCPU_GUEST);
+  REGISTER_CONSTANT(VIR_DOMAIN_VCPU_HOTPLUGGABLE, VCPU_HOTPLUGGABLE);
 
 
   REGISTER_CONSTANT(VIR_DOMAIN_SHUTDOWN_DEFAULT, SHUTDOWN_DEFAULT);
diff --git a/lib/Sys/Virt/Domain.pm b/lib/Sys/Virt/Domain.pm
index e37b078..f45fc9e 100644
--- a/lib/Sys/Virt/Domain.pm
+++ b/lib/Sys/Virt/Domain.pm
@@ -2768,6 +2768,10 @@ Flag to request adjustment of the maximum vCPU value
 
 Flag to request the guest VCPU mask
 
+=item Sys::Virt::Domain::VCPU_HOTPLUGGABLE
+
+Flag to make vcpus added hot(un)pluggable
+
 =back
 
 =head2 STATE CHANGE EVENTS
-- 
2.8.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH] conf: Explain some code in more detail

[Andrea Bolognani]

The code is entirely correct, but it still managed to trip me
up when I first ran into it because I did not realize right away
that VIR_PCI_CONNECT_TYPES_ENDPOINT was not a single flag, but
rather a mask including both VIR_PCI_CONNECT_TYPE_PCI_DEVICE and
VIR_PCI_CONNECT_TYPE_PCIE_DEVICE.

In order to save the next distracted traveler in PCI Address Land
some time, document this fact with a comment. Add a test case for
the behavior as well.
---
 src/conf/domain_addr.c |  4 +-
 .../qemuxml2argv-q35-pci-force-address.args| 25 +++
 .../qemuxml2argv-q35-pci-force-address.xml | 32 +++
 tests/qemuxml2argvtest.c   |  7 
 .../qemuxml2xmlout-q35-pci-force-address.xml   | 48 ++
 tests/qemuxml2xmltest.c|  7 
 6 files changed, 122 insertions(+), 1 deletion(-)
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-q35-pci-force-address.args
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-q35-pci-force-address.xml
 create mode 100644 
tests/qemuxml2xmloutdata/qemuxml2xmlout-q35-pci-force-address.xml

diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
index 0406b50..bbeb611 100644
--- a/src/conf/domain_addr.c
+++ b/src/conf/domain_addr.c
@@ -100,7 +100,9 @@ virDomainPCIAddressFlagsCompatible(virPCIDeviceAddressPtr 
addr,
 if (fromConfig) {
 /* If the requested connection was manually specified in
  * config, allow a PCI device to connect to a PCIe slot, or
- * vice versa.
+ * vice versa. In order to do so, we add *both* the PCI_DEVICE
+ * and the PCIE_DEVICE flags to the bus if it already has either
+ * of them, using the ENDPOINT mask.
  */
 if (busFlags & VIR_PCI_CONNECT_TYPES_ENDPOINT)
 busFlags |= VIR_PCI_CONNECT_TYPES_ENDPOINT;
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35-pci-force-address.args 
b/tests/qemuxml2argvdata/qemuxml2argv-q35-pci-force-address.args
new file mode 100644
index 000..9ca342b
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-q35-pci-force-address.args
@@ -0,0 +1,25 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=none \
+/usr/libexec/qemu-kvm \
+-name q35-test \
+-S \
+-M q35 \
+-m 2048 \
+-smp 2,sockets=2,cores=1,threads=1 \
+-uuid 11dbdcdd-4c3b-482b-8903-9bdb8c0a2774 \
+-nographic \
+-nodefaults \
+-monitor unix:/tmp/lib/domain--1-q35-test/monitor.sock,server,nowait \
+-no-acpi \
+-boot c \
+-device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x1e \
+-device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x0 \
+-device ioh3420,port=0x10,chassis=3,id=pci.3,bus=pcie.0,addr=0x2 \
+-device e1000e,vlan=0,id=net0,mac=52:54:00:aa:bf:ef,bus=pci.2,addr=0x1 \
+-net user,vlan=0,name=hostnet0 \
+-device intel-hda,id=sound0,bus=pci.3,addr=0x0 \
+-device hda-duplex,id=sound0-codec0,bus=sound0.0,cad=0
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35-pci-force-address.xml 
b/tests/qemuxml2argvdata/qemuxml2argv-q35-pci-force-address.xml
new file mode 100644
index 000..34ec7c7
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-q35-pci-force-address.xml
@@ -0,0 +1,32 @@
+
+  q35-test
+  11dbdcdd-4c3b-482b-8903-9bdb8c0a2774
+  2097152
+  2097152
+  2
+  
+hvm
+
+  
+  
+  destroy
+  restart
+  destroy
+  
+/usr/libexec/qemu-kvm
+
+
+
+
+
+  
+
+
+  
+  
+  
+
+
+
+  
+
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index b996182..fc18317 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -1749,6 +1749,13 @@ mymain(void)
 QEMU_CAPS_ICH9_AHCI,
 QEMU_CAPS_PCI_MULTIFUNCTION, QEMU_CAPS_ICH9_USB_EHCI1,
 QEMU_CAPS_NEC_USB_XHCI);
+/* Make sure the user can always override libvirt's default device
+ * placement policy by providing an explicit PCI address */
+DO_TEST("q35-pci-force-address",
+QEMU_CAPS_DEVICE_PCI_BRIDGE,
+QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE,
+QEMU_CAPS_DEVICE_IOH3420,
+QEMU_CAPS_HDA_DUPLEX);
 
 DO_TEST_PARSE_ERROR("q35-wrong-root",
 QEMU_CAPS_DEVICE_PCI_BRIDGE,
diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35-pci-force-address.xml 
b/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35-pci-force-address.xml
new file mode 100644
index 000..c0332d4
--- /dev/null
+++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35-pci-force-address.xml
@@ -0,0 +1,48 @@
+
+  q35-test
+  11dbdcdd-4c3b-482b-8903-9bdb8c0a2774
+  2097152
+  2097152
+  2
+  
+hvm
+
+  
+  
+  destroy
+  restart
+  destroy
+  
+/usr/libexec/qemu-kvm
+
+
+  
+  
+
+
+  
+  
+  
+
+
+  
+  
+  
+
+
+
+  
+
+
+  
+  
+  
+
+
+
+
+  
+
+
+  
+
diff --git a/tests/qemuxml2xmltest.c 

Re: [libvirt] [PATCH 3/4] vbox: change API (un)initialization logic.

[Dawid Zamirski]

On Thu, 2016-10-13 at 12:04 +0200, Martin Kletzander wrote:
> On Wed, Oct 12, 2016 at 04:00:52PM -0400, Dawid Zamirski wrote:
> > 
> > So after my initial attempts at handling this as suggested (create
> > vboxDriver struct and initialize in VIR_ONCE_GLOBAL_INIT), I've
> > stumbled upon virStateDriver structs used in other drivers that
> > apparently has .stateInitialize, .stateCleanup that to me look like
> > perfect places to call vbox's pfnComInitialize pfnComUnintialize
> > pairs.
> > Would this be a good idea to utilize the virStateDriver for this or
> > should I stick with VIR_ONCE_GLOBAL_INIT way?
> > 
> 
> 
> I just now realized you said the issue we are talking about crashes
> the
> libvirt daemon, right?  And it does not use virStateDriver?  That's
> weird.  You are connecting to vbox:///session, but the crash happens
> in
> the libvirt daemon.  That means we switched the vbox driver to the
> daemo-side driver, so I'd expect it to be stateful driver.  However
> it
> doesn't use virStateDriver.  I'm Cc'ing Michal to let him look at it
> as
> he's more handy in the whole vbox area.
> 
> > PS. Thanks a lot for reviewing my code and guiding me through it
> > all.

Yes, it's crashing libvirtd daemon, however the local one that is
started on behalf of vbox:///session owned by the user that started it
(not the system-wide one running as root). One thing I realized is that
putting pfnComInitialize into the .stateInitialize will likely make
VBoxSVC start together with libvirtd which IMO is not desired. I might
still utilize the state driver to at least initialize vboxDriver object
and obtain pFuncs reference and then defer pfnComInitialize to the
first connection that comes in, similarly as my original patch does.
That way, I might be able to make vbox driver follow the initialization
pattern used by other drivers and hopefully get rid of the
g_pVBoxGlobalData.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 03/14] qemuBuildInterfaceCommandLine: Move hostdev handling a bit further

[John Ferlan]

On 10/06/2016 09:38 AM, Michal Privoznik wrote:
> The idea is to have function that does some checking of the
> arguments at its beginning and then have one big switch for all
> the interface types it supports. Each one of them generating the
> corresponding part of the command line.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_command.c| 13 
>  .../qemuxml2argv-net-hostdev-fail.xml  | 39 
> ++
>  tests/qemuxml2argvtest.c   |  4 +++
>  3 files changed, 49 insertions(+), 7 deletions(-)
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-hostdev-fail.xml
> 

Is it safe to assume that previously if the XML had ",  , and/or "" that it would be ignored, but now there will be an error
at command line build time?

Hopefully we get no complaints ;-)

ACK

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 12/14] qemuBuildVhostuserCommandLine: Unify -netdev creation

[John Ferlan]

On 10/06/2016 09:38 AM, Michal Privoznik wrote:
> Currently, what we do for vhost-user network is generate the
> following part of command line:
> 
> -netdev type=vhost-user,id=hostnet0,chardev=charnet0
> 
> There's no need for 'type=' it is the default. Drop it.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_command.c   | 2 +-
>  tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser-multiq.args | 6 +++---
>  tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser.args| 4 ++--
>  3 files changed, 6 insertions(+), 6 deletions(-)
> 

Still an ACK,

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 09/14] qemuBuildChrChardevStr: Introduce @nowait argument

[John Ferlan]

On 10/06/2016 09:38 AM, Michal Privoznik wrote:
> This alone makes not much sense. But the aim is to reuse this
> function in qemuBuildVhostuserCommandLine() where 'nowait' is not
> supported for vhost-user devices.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_command.c | 35 +++
>  1 file changed, 19 insertions(+), 16 deletions(-)
> 

ACK

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 10/14] qemuBuildVhostuserCommandLine: Reuse qemuBuildChrChardevStr

[John Ferlan]

On 10/06/2016 09:38 AM, Michal Privoznik wrote:
> There's no need to reinvent the wheel here. We already have a
> function to format virDomainChrSourceDefPtr. It's called
> qemuBuildChrChardevStr(). Use that instead of some dummy
> virBufferAsprintf().
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_command.c | 16 +---
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 

If someone bisected to here, they'd have issues because patch 11 needs
to be merged here...

ACK if merged with patch 11

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 14/14] qemu_hotplug: Support interface type of vhost-user hotplug

[John Ferlan]

On 10/06/2016 09:39 AM, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1366108
> 
> There are couple of things that needs to be done in order to

s/needs/need/

> allow vhost-user hotplug. Firstly, vhost-user requires a chardev
> which is connected to vhost-user bridge and through which qemu
> communicates with the bridge (no acutal guest traffic is sent

s/acutal/actual/

> through there, just some metadata). In order to generate proper
> chardev alias, we must assign device alias way sooner.
> 
> Then, because we are plugging the chardev first, we need to do
> the proper undo if something fails - that is remove netdev too.
> We don't want anything to be left over in case attach fails at
> some point.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_hotplug.c | 71 
> +
>  1 file changed, 60 insertions(+), 11 deletions(-)
> 
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 1b2a075..14af4e1 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -932,6 +932,10 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
>  virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>  virDomainCCWAddressSetPtr ccwaddrs = NULL;
>  size_t i;
> +char *charDevAlias = NULL;
> +bool charDevPlugged = false;
> +bool netdevPlugged = false;
> +bool hostPlugged = false;
>  
>  /* preallocate new slot for device */
>  if (VIR_REALLOC_N(vm->def->nets, vm->def->nnets + 1) < 0)
> @@ -970,6 +974,9 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
>  return -1;
>  }
>  
> +if (qemuAssignDeviceNetAlias(vm->def, net, -1) < 0)
> +goto cleanup;
> +
>  switch (actualType) {
>  case VIR_DOMAIN_NET_TYPE_BRIDGE:
>  case VIR_DOMAIN_NET_TYPE_NETWORK:
> @@ -1044,8 +1051,18 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
>  goto cleanup;
>  break;
>  
> -case VIR_DOMAIN_NET_TYPE_USER:
>  case VIR_DOMAIN_NET_TYPE_VHOSTUSER:
> +if (!qemuDomainSupportsNetdev(vm->def, priv->qemuCaps, net)) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   "%s", _("Netdev support unavailable"));
> +goto cleanup;
> +}
> +
> +if (virAsprintf(, "char%s", net->info.alias) < 0)
> +goto cleanup;
> +break;
> +
> +case VIR_DOMAIN_NET_TYPE_USER:
>  case VIR_DOMAIN_NET_TYPE_SERVER:
>  case VIR_DOMAIN_NET_TYPE_CLIENT:
>  case VIR_DOMAIN_NET_TYPE_MCAST:
> @@ -1081,9 +1098,6 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
>  goto cleanup;
>  }
>  
> -if (qemuAssignDeviceNetAlias(vm->def, net, -1) < 0)
> -goto cleanup;
> -
>  if (qemuDomainMachineIsS390CCW(vm->def) &&
>  virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VIRTIO_CCW)) {
>  net->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW;
> @@ -1143,23 +1157,36 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
>  }
>  
>  qemuDomainObjEnterMonitor(driver, vm);
> +
> +if (actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER) {
> +if (qemuMonitorAttachCharDev(priv->mon, charDevAlias, 
> net->data.vhostuser) < 0) {
> +ignore_value(qemuDomainObjExitMonitor(driver, vm));
> +virDomainAuditNet(vm, NULL, net, "attach", false);
> +goto cleanup;
> +}
> +charDevPlugged = true;
> +}
> +
>  if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NETDEV)) {
>  if (qemuMonitorAddNetdev(priv->mon, netstr,
>   tapfd, tapfdName, tapfdSize,
>   vhostfd, vhostfdName, vhostfdSize) < 0) {
>  ignore_value(qemuDomainObjExitMonitor(driver, vm));
>  virDomainAuditNet(vm, NULL, net, "attach", false);
> -goto cleanup;
> +goto try_remove;
>  }
> +netdevPlugged = true;
>  } else {
>  if (qemuMonitorAddHostNetwork(priv->mon, netstr,
>tapfd, tapfdName, tapfdSize,
>vhostfd, vhostfdName, vhostfdSize) < 
> 0) {
>  ignore_value(qemuDomainObjExitMonitor(driver, vm));
>  virDomainAuditNet(vm, NULL, net, "attach", false);
> -goto cleanup;
> +goto try_remove;
>  }
> +hostPlugged = true;
>  }
> +
>  if (qemuDomainObjExitMonitor(driver, vm) < 0)
>  goto cleanup;
>  
> @@ -1262,6 +1289,7 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
>  }
>  VIR_FREE(vhostfd);
>  VIR_FREE(vhostfdName);
> +VIR_FREE(charDevAlias);
>  virObjectUnref(cfg);
>  virDomainCCWAddressSetFree(ccwaddrs);
>  
> @@ -1277,7 +1305,11 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
>  if (virAsprintf(_name, "host%s", net->info.alias) < 0)
>  goto cleanup;
>  

Re: [libvirt] [PATCH v2 04/14] qemuBuildInterfaceCommandLine: Move vhostuser handling a bit further

[John Ferlan]

On 10/06/2016 09:38 AM, Michal Privoznik wrote:
> The idea is to have function that does some checking of the
> arguments at its beginning and then have one big switch for all
> the interface types it supports. Each one of them generating the
> corresponding part of the command line.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_command.c|  9 +++---
>  .../qemuxml2argv-net-vhostuser-fail.xml| 36 
> ++
>  tests/qemuxml2argvtest.c   |  3 ++
>  3 files changed, 44 insertions(+), 4 deletions(-)
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser-fail.xml
> 

Similarly to patch 3, if a vhost-user entry contains 'filterref' or
'backend tap', then bulding will fail.  Although queues is Ok.

BTW: I just realized cfg isn't even used in this function other than the
GetConfig call and Unref call.

ACK,

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 05/14] qemuBuildInterfaceCommandLine: Move from if-else forest to switch

[John Ferlan]

On 10/06/2016 09:38 AM, Michal Privoznik wrote:
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_command.c | 34 +++---
>  1 file changed, 27 insertions(+), 7 deletions(-)
> 

ACK

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 07/14] qemuDomainAttachNetDevice: Explicitly list allowed types for hotplug

[John Ferlan]

On 10/06/2016 09:38 AM, Michal Privoznik wrote:
> Instead of blindly claim support for hot-plugging of every
> interface type out there we should copy approach we have for
> device types: white listing supported types and explicitly error
> out on unsupported ones.
> For instance, trying to hotplug vhostuser interface results in
> nothing usable from guest currently. vhostuser typed interfaces
> require additional work on our side.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_hotplug.c | 33 +++--
>  1 file changed, 27 insertions(+), 6 deletions(-)
> 

Should have just kept quiet in the v1 review about a switch ;-)

FWIW: If someone tries to add a vhostuser it will fail before the
unsupported message because the "if (net->driver.virtio.queues > 0 &&
..." doesn't have VHOSTUSER in the list like it does for command line.

I'm OK with adding that in here now or as a separate patch...

ACK for what's here.

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 13/14] qemuBuildHostNetStr: Support VIR_DOMAIN_NET_TYPE_VHOSTUSER

[John Ferlan]

On 10/06/2016 09:38 AM, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1366505
> 
> So far, this function lacked support for
> VIR_DOMAIN_NET_TYPE_VHOSTUSER leaving callers to hack around the
> problem by constructing the command line on their own. This is
> not ideal as it blocks hot plug support.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_command.c| 38 
> +-
>  .../qemuxml2argv-net-vhostuser-multiq.args |  6 ++--
>  .../qemuxml2argv-net-vhostuser.args|  4 +--
>  3 files changed, 28 insertions(+), 20 deletions(-)
> 

ACK still applies,

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 11/14] qemuBuildInterfaceCommandLine: Pass proper args

[John Ferlan]

On 10/06/2016 09:38 AM, Michal Privoznik wrote:
> Instead of various NULL arguments, we can pass proper qemu driver
> config, log manager, and virCommand.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_command.c | 31 ++-
>  1 file changed, 18 insertions(+), 13 deletions(-)
> 

This needs to be merged with patch 10

see note below

ACK w/ the merged patches - let's avoid a round of ATTRIBUTE_NONNULL

John

> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 246ffe0..895bb23 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -7822,12 +7822,15 @@ qemuBuildGraphicsCommandLine(virQEMUDriverConfigPtr 
> cfg,
>  }
>  
>  static int
> -qemuBuildVhostuserCommandLine(virCommandPtr cmd,
> +qemuBuildVhostuserCommandLine(virQEMUDriverPtr driver,
> +  virLogManagerPtr logManager,
> +  virCommandPtr cmd,
>virDomainDefPtr def,
>virDomainNetDefPtr net,
>virQEMUCapsPtr qemuCaps,
>unsigned int bootindex)
>  {
> +virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>  char *chardev = NULL;
>  virBuffer netdev_buf = VIR_BUFFER_INITIALIZER;
>  unsigned int queues = net->driver.virtio.queues;
> @@ -7841,7 +7844,7 @@ qemuBuildVhostuserCommandLine(virCommandPtr cmd,
>  
>  switch ((virDomainChrType) net->data.vhostuser->type) {
>  case VIR_DOMAIN_CHR_TYPE_UNIX:
> -if (!(chardev = qemuBuildChrChardevStr(NULL, NULL, NULL, def,
> +if (!(chardev = qemuBuildChrChardevStr(logManager, cmd, cfg, def,
> net->data.vhostuser,
> net->info.alias, qemuCaps, 
> false)))
>  goto error;
> @@ -7895,10 +7898,12 @@ qemuBuildVhostuserCommandLine(virCommandPtr cmd,
>  
>  virCommandAddArgList(cmd, "-device", nic, NULL);
>  VIR_FREE(nic);
> +virObjectUnref(cfg);
>  
>  return 0;
>  
>   error:
> +virObjectUnref(cfg);
>  VIR_FREE(chardev);
>  virBufferFreeAndReset(_buf);
>  VIR_FREE(nic);
> @@ -7907,8 +7912,9 @@ qemuBuildVhostuserCommandLine(virCommandPtr cmd,
>  }
>  
>  static int
> -qemuBuildInterfaceCommandLine(virCommandPtr cmd,
> -  virQEMUDriverPtr driver,
> +qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver,
> +  virLogManagerPtr logManager,
> +  virCommandPtr cmd,
>virDomainDefPtr def,
>virDomainNetDefPtr net,
>virQEMUCapsPtr qemuCaps,
> @@ -7928,7 +7934,6 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd,
>  char **tapfdName = NULL;
>  char **vhostfdName = NULL;
>  virDomainNetType actualType = virDomainNetGetActualType(net);
> -virQEMUDriverConfigPtr cfg = NULL;

NOTE: Ironically I had suggested removing cfg in patch 4.  You can
either do it then or now, it doesn't matter!

>  virNetDevBandwidthPtr actualBandwidth;
>  size_t i;
>  
> @@ -7971,8 +7976,6 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd,
>  return -1;
>  }
>  
> -cfg = virQEMUDriverGetConfig(driver);
> -
>  switch (actualType) {
>  case VIR_DOMAIN_NET_TYPE_NETWORK:
>  case VIR_DOMAIN_NET_TYPE_BRIDGE:
> @@ -8032,7 +8035,8 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd,
>  break;
>  
>  case VIR_DOMAIN_NET_TYPE_VHOSTUSER:
> -ret = qemuBuildVhostuserCommandLine(cmd, def, net, qemuCaps, 
> bootindex);
> +ret = qemuBuildVhostuserCommandLine(driver, logManager, cmd, def,
> +net, qemuCaps, bootindex);
>  goto cleanup;
>  break;
>  
> @@ -8205,7 +8209,6 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd,
>  VIR_FREE(host);
>  VIR_FREE(tapfdName);
>  VIR_FREE(vhostfdName);
> -virObjectUnref(cfg);
>  return ret;
>  }
>  
> @@ -8215,8 +8218,9 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd,
>   *   API domainSetSecurityTapFDLabel that doesn't use the const format.
>   */
>  static int
> -qemuBuildNetCommandLine(virCommandPtr cmd,
> -virQEMUDriverPtr driver,
> +qemuBuildNetCommandLine(virQEMUDriverPtr driver,
> +virLogManagerPtr logManager,
> +virCommandPtr cmd,
>  virDomainDefPtr def,
>  virQEMUCapsPtr qemuCaps,
>  virNetDevVPortProfileOp vmop,
> @@ -8254,7 +8258,7 @@ qemuBuildNetCommandLine(virCommandPtr cmd,
>  else
>  vlan = i;
>  
> -if (qemuBuildInterfaceCommandLine(cmd, driver, def, net,
> +if (qemuBuildInterfaceCommandLine(driver, logManager, cmd, def, 
> net,
>  

Re: [libvirt] [PATCH v2 08/14] qemuBuildHostNetStr: Explicitly enumerate net types

[John Ferlan]

On 10/06/2016 09:38 AM, Michal Privoznik wrote:
> We tend to prevent using 'default' in switches. And it is for a
> good reason - control may end up in paths we wouldn't want for
> new values. In this specific case, if qemuBuildHostNetStr is
> called over VIR_DOMAIN_NET_TYPE_VHOSTUSER it would produce
> meaningless output. Fortunately, there no such call yet.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_command.c | 14 +-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 

Doesn't seem anything changed from the original patch (which is fine).

Yet another function that doesn't use/need the 'cfg' variable...

ACK to what's here, I'd support removing cfg as well


John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 06/14] qemuDomainAttachNetDevice: Move hostdev handling a bit further

[John Ferlan]

On 10/06/2016 09:38 AM, Michal Privoznik wrote:
> The idea is to have function that does some checking at its
> beginning and then have one big switch for all the interface
> types it supports.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_hotplug.c | 26 --
>  1 file changed, 12 insertions(+), 14 deletions(-)
> 

Similar to patch 3, we'll now fail if filterref and queues exist in the
to be attached device's XML, but it doesn't check the backend tap. Not a
problem with this patch per se, but perhaps something that could be put
on the virtual todo list (make the checks consistent between building
command line and hotplug and of course do so by adding some helper
function rather than duplicating the checks which are prone to issues)...

ACK for what's here though

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 02/14] virDomainNetGetActualType: Return type is virDomainNetType

[John Ferlan]

On 10/06/2016 09:38 AM, Michal Privoznik wrote:
> This function for some weird reason returns integer instead of
> virDomainNetType type. It is important to return the correct type
> so that we know what values we can expect.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/bhyve/bhyve_command.c |  2 +-
>  src/bhyve/bhyve_process.c |  2 +-
>  src/conf/domain_conf.c|  8 
>  src/conf/domain_conf.h|  2 +-
>  src/libxl/libxl_domain.c  |  2 +-
>  src/libxl/libxl_driver.c  |  2 +-
>  src/lxc/lxc_driver.c  |  9 +++--
>  src/qemu/qemu_command.c   |  2 +-
>  src/qemu/qemu_hotplug.c   |  4 ++--
>  src/qemu/qemu_process.c   | 13 -
>  10 files changed, 31 insertions(+), 15 deletions(-)
> 
> diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c
> index 9ad3f9b..55ad950 100644
> --- a/src/bhyve/bhyve_command.c
> +++ b/src/bhyve/bhyve_command.c
> @@ -52,7 +52,7 @@ bhyveBuildNetArgStr(const virDomainDef *def,
>  char macaddr[VIR_MAC_STRING_BUFLEN];
>  char *realifname = NULL;
>  char *brname = NULL;
> -int actualType = virDomainNetGetActualType(net);
> +virDomainNetType actualType = virDomainNetGetActualType(net);
>  
>  if (actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) {
>  if (VIR_STRDUP(brname, virDomainNetGetActualBridgeName(net)) < 0)
> diff --git a/src/bhyve/bhyve_process.c b/src/bhyve/bhyve_process.c
> index 6db070f..9d80976 100644
> --- a/src/bhyve/bhyve_process.c
> +++ b/src/bhyve/bhyve_process.c
> @@ -77,7 +77,7 @@ bhyveNetCleanup(virDomainObjPtr vm)
>  
>  for (i = 0; i < vm->def->nnets; i++) {
>  virDomainNetDefPtr net = vm->def->nets[i];
> -int actualType = virDomainNetGetActualType(net);
> +virDomainNetType actualType = virDomainNetGetActualType(net);
>  
>  if (actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) {
>  if (net->ifname) {
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 8f04e6f..9343770 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -20804,7 +20804,7 @@ virDomainActualNetDefContentsFormat(virBufferPtr buf,
>  bool inSubelement,
>  unsigned int flags)
>  {
> -int actualType = virDomainNetGetActualType(def);
> +virDomainNetType actualType = virDomainNetGetActualType(def);
>  
>  if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) {
>  if (virDomainHostdevDefFormatSubsys(buf, 
> virDomainNetGetActualHostdev(def),
> @@ -20884,7 +20884,7 @@ virDomainActualNetDefFormat(virBufferPtr buf,
>  virDomainNetDefPtr def,
>  unsigned int flags)
>  {
> -unsigned int type;
> +virDomainNetType type;
>  const char *typeStr;
>  
>  if (!def)
> @@ -21039,7 +21039,7 @@ virDomainNetDefFormat(virBufferPtr buf,
>char *prefix,
>unsigned int flags)
>  {
> -unsigned int actualType = virDomainNetGetActualType(def);
> +virDomainNetType actualType = virDomainNetGetActualType(def);
>  bool publicActual = false;
>  int sourceLines = 0;
>  const char *typeStr;
> @@ -24781,7 +24781,7 @@ virDomainStateReasonFromString(virDomainState state, 
> const char *reason)
>   * otherwise return the value from the NetDef.
>   */
>  
> -int
> +virDomainNetType
>  virDomainNetGetActualType(virDomainNetDefPtr iface)
>  {
>  if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK)
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index a70bc21..95bcf4f 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -2814,7 +2814,7 @@ int 
> virDomainGraphicsListenAppendSocket(virDomainGraphicsDefPtr def,
>  const char *socket)
>  ATTRIBUTE_NONNULL(1);
>  
> -int virDomainNetGetActualType(virDomainNetDefPtr iface);
> +virDomainNetType virDomainNetGetActualType(virDomainNetDefPtr iface);
>  const char *virDomainNetGetActualBridgeName(virDomainNetDefPtr iface);
>  int virDomainNetGetActualBridgeMACTableManager(virDomainNetDefPtr iface);
>  const char *virDomainNetGetActualDirectDev(virDomainNetDefPtr iface);
> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
> index db2c1dc..3df04cf 100644
> --- a/src/libxl/libxl_domain.c
> +++ b/src/libxl/libxl_domain.c
> @@ -937,7 +937,7 @@ libxlNetworkPrepareDevices(virDomainDefPtr def)
>  
>  for (i = 0; i < def->nnets; i++) {
>  virDomainNetDefPtr net = def->nets[i];
> -int actualType;
> +virDomainNetType actualType;
>  
>  /* If appropriate, grab a physical device from the configured
>   * network's pool of devices, or resolve bridge device name
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index b66cb1f..f87f549 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -3328,7 +3328,7 @@ 

Re: [libvirt] [PATCH v2 01/14] virDomainNetDefParseXML: Realign

[John Ferlan]

On 10/06/2016 09:38 AM, Michal Privoznik wrote:
> There are couple of formatting issues. No functional change
> though.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/conf/domain_conf.c | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 

ACK

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] util: Fix build on s390

[Eric Blake]

On 10/07/2016 05:43 AM, Jiri Denemark wrote:
> GCC on s390 complains
> 
> util/virconf.c: In function 'virConfGetValueSizeT':
> util/virconf.c:1220:9: error: format '%zu' expects argument of type
> 'size_t', but argument 9 has type 'unsigned int' [-Werror=format=]
> 

That's a bug in s390's system headers.  Gnulib should be taught to work
around it.

> Signed-off-by: Jiri Denemark 
> ---
>  src/util/virconf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/util/virconf.c b/src/util/virconf.c
> index 3e49f41..1372389 100644
> --- a/src/util/virconf.c
> +++ b/src/util/virconf.c
> @@ -1219,7 +1219,7 @@ int virConfGetValueSizeT(virConfPtr conf,
>  if (((unsigned long long)cval->l) > SIZE_MAX) {
>  virReportError(VIR_ERR_INTERNAL_ERROR,
> _("%s: value for '%s' parameter must be in range 
> 0:%zu"),
> -   conf->filename, setting, SIZE_MAX);
> +   conf->filename, setting, (size_t) SIZE_MAX);
>  return -1;
>  }
>  #endif
> 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] util: Fix build on s390

[Jiri Denemark]

Hi.

On Fri, Oct 07, 2016 at 13:36:21 +0200, Christian Borntraeger wrote:
> On 10/07/2016 12:43 PM, Jiri Denemark wrote:
> > GCC on s390 complains
> > 
> > util/virconf.c: In function 'virConfGetValueSizeT':
> > util/virconf.c:1220:9: error: format '%zu' expects argument of type
> > 'size_t', but argument 9 has type 'unsigned int' [-Werror=format=]
> 
> Interesting, we have never seen this. is this 31bit (s390) or 64bit(s390x)?
> What gcc?

I finally got the time to get the additional info for you (which was not
exactly easy as it happened on a builder system):

$ gcc --version
gcc (GCC) 4.8.5 20150623 (Red Hat 4.8.5-11)
Copyright (C) 2015 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

$ arch
s390

However, the host kernel is 2.6.32-573.22.1.el6.s390x. The build is done
on an s390x RHEL-6 host, but in a mocked s390 RHEL-7 environment :-)

> Can you maybe provide the virconf.i file to see how the SIZE_MAX define
> was resolved?

# 1199 "util/virconf.c"
int virConfGetValueSizeT(virConfPtr conf,
 const char *setting,
 size_t *value)
{
virConfValuePtr cval = virConfGetValue(conf, setting);
virLogMessage(, VIR_LOG_DEBUG, "util/virconf.c", 1206,
__func__, ((void *)0), "Get value size_t %p %d", cval,
cval ? cval->type : VIR_CONF_NONE);
if (!cval)
return 0;
if (cval->type != VIR_CONF_ULLONG) {
virReportErrorHelper(VIR_FROM_CONF, VIR_ERR_INTERNAL_ERROR,
 "util/virconf.c"
# 1212 "util/virconf.c"
, __FUNCTION__,
 1214
# 1212 "util/virconf.c"
, dcgettext ("libvirt", "%s: expected an unsigned integer for
  '%s' parameter", 5), conf->filename, setting)
   ;
return -1;
}
if (((unsigned long long)cval->l) > (4294967295U)) {
virReportErrorHelper(VIR_FROM_CONF, VIR_ERR_INTERNAL_ERROR,
 "util/virconf.c"
# 1220 "util/virconf.c"
, __FUNCTION__,
 1222
# 1220 "util/virconf.c"
, dcgettext ("libvirt", "%s: value for '%s' parameter must be
  in range 0:%zu", 5), conf->filename, setting,
  (4294967295U))
 ;
return -1;
}
*value = (size_t)cval->l;
return 1;
}

Which means SIZE_MAX is 4294967295U.

Complete virconf.i can be seen at
http://people.redhat.com/~jdenemar/libvirt/virconf.i

Jirka

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 3/4] qemu: add vcpu.n.halted to vcpu domain stats

[Viktor Mihajlovski]

Extended qemuDomainGetStatsVcpu to include the per vcpu halted
indicator if reported by QEMU. The key for new boolean value
has the format "vcpu..halted".

Signed-off-by: Viktor Mihajlovski 
Reviewed-by: Bjoern Walk 
---
 src/qemu/qemu_driver.c | 34 ++
 1 file changed, 30 insertions(+), 4 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 8789c9d..b269112 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1454,7 +1454,8 @@ qemuDomainHelperGetVcpus(virDomainObjPtr vm,
  unsigned long long *cpuwait,
  int maxinfo,
  unsigned char *cpumaps,
- int maplen)
+ int maplen,
+ bool *cpuhalted)
 {
 size_t ncpuinfo = 0;
 size_t i;
@@ -1474,6 +1475,9 @@ qemuDomainHelperGetVcpus(virDomainObjPtr vm,
 if (cpumaps)
 memset(cpumaps, 0, sizeof(*cpumaps) * maxinfo);
 
+if (cpuhalted)
+memset(cpuhalted, 0, sizeof(*cpuhalted) * maxinfo);
+
 for (i = 0; i < virDomainDefGetVcpusMax(vm->def) && ncpuinfo < maxinfo; 
i++) {
 virDomainVcpuDefPtr vcpu = virDomainDefGetVcpu(vm->def, i);
 pid_t vcpupid = qemuDomainGetVcpuPid(vm, i);
@@ -1511,6 +1515,9 @@ qemuDomainHelperGetVcpus(virDomainObjPtr vm,
 return -1;
 }
 
+if (cpuhalted)
+cpuhalted[ncpuinfo] = qemuDomainGetVcpuHalted(vm, ncpuinfo);
+
 ncpuinfo++;
 }
 
@@ -5455,7 +5462,8 @@ qemuDomainGetVcpus(virDomainPtr dom,
 goto cleanup;
 }
 
-ret = qemuDomainHelperGetVcpus(vm, info, NULL, maxinfo, cpumaps, maplen);
+ret = qemuDomainHelperGetVcpus(vm, info, NULL, maxinfo, cpumaps, maplen,
+   NULL);
 
  cleanup:
 virDomainObjEndAPI();
@@ -18922,7 +18930,7 @@ qemuDomainGetStatsBalloon(virQEMUDriverPtr driver,
 
 
 static int
-qemuDomainGetStatsVcpu(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
+qemuDomainGetStatsVcpu(virQEMUDriverPtr driver,
virDomainObjPtr dom,
virDomainStatsRecordPtr record,
int *maxparams,
@@ -18933,6 +18941,7 @@ qemuDomainGetStatsVcpu(virQEMUDriverPtr driver 
ATTRIBUTE_UNUSED,
 char param_name[VIR_TYPED_PARAM_FIELD_LENGTH];
 virVcpuInfoPtr cpuinfo = NULL;
 unsigned long long *cpuwait = NULL;
+bool *cpuhalted = NULL;
 
 if (virTypedParamsAddUInt(>params,
   >nparams,
@@ -18952,9 +18961,14 @@ qemuDomainGetStatsVcpu(virQEMUDriverPtr driver 
ATTRIBUTE_UNUSED,
 VIR_ALLOC_N(cpuwait, virDomainDefGetVcpus(dom->def)) < 0)
 goto cleanup;
 
+if (qemuDomainRefreshVcpuHalted(driver, dom,
+QEMU_ASYNC_JOB_NONE) == 0 &&
+VIR_ALLOC_N(cpuhalted, virDomainDefGetVcpus(dom->def)) < 0)
+goto cleanup;
+
 if (qemuDomainHelperGetVcpus(dom, cpuinfo, cpuwait,
  virDomainDefGetVcpus(dom->def),
- NULL, 0) < 0) {
+ NULL, 0, cpuhalted) < 0) {
 virResetLastError();
 ret = 0; /* it's ok to be silent and go ahead */
 goto cleanup;
@@ -18990,6 +19004,17 @@ qemuDomainGetStatsVcpu(virQEMUDriverPtr driver 
ATTRIBUTE_UNUSED,
 param_name,
 cpuwait[i]) < 0)
 goto cleanup;
+
+if (cpuhalted) {
+snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
+ "vcpu.%u.halted", cpuinfo[i].number);
+if (virTypedParamsAddBoolean(>params,
+ >nparams,
+ maxparams,
+ param_name,
+ cpuhalted[i]) < 0)
+goto cleanup;
+}
 }
 
 ret = 0;
@@ -18997,6 +19022,7 @@ qemuDomainGetStatsVcpu(virQEMUDriverPtr driver 
ATTRIBUTE_UNUSED,
  cleanup:
 VIR_FREE(cpuinfo);
 VIR_FREE(cpuwait);
+VIR_FREE(cpuhalted);
 return ret;
 }
 
-- 
1.9.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 0/4] Add VCPU halted to domain statistics

[Viktor Mihajlovski]

As a result of the discussions on
https://www.redhat.com/archives/libvir-list/2016-October/msg00531.html:

In order to be able to report the per VCPU halted condition as returned
by the QEMU monitor this series extends the VCPU-specific domain
statistics with a vcpu.n.halted value.

This is done by
- extending the monitor to pass back the halted condition for the vcpus
- adding a new field to the private domain vcpu object reflecting the
  halted condition for the vcpu
- modifying the driver code to report the new VCPU domain statistics value

The halted condition is however not recorded in the internal XML format, since
the state can change asynchronously (without notification).

Patches 1/4 and 2/4 are taken without modifications from the original
posting. In 4/4 I've only updated virsh.pod as I didn't want to
touch libvirt-domain.c in this context just for documentation purposes.

Viktor Mihajlovski (4):
  qemu: Add monitor support for CPU halted state
  qemu: Add domain support for VCPU halted state
  qemu: add vcpu.n.halted to vcpu domain stats
  doc: update virsh domstats documentation for vcpu statistics

 src/qemu/qemu_domain.c   | 66 
 src/qemu/qemu_domain.h   |  5 
 src/qemu/qemu_driver.c   | 34 ---
 src/qemu/qemu_monitor.c  |  6 +++-
 src/qemu/qemu_monitor.h  |  3 ++
 src/qemu/qemu_monitor_json.c |  3 ++
 src/qemu/qemu_monitor_text.c |  8 +-
 tests/qemumonitorjsontest.c  |  8 +++---
 tools/virsh.pod  |  7 +
 9 files changed, 130 insertions(+), 10 deletions(-)

-- 
1.9.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 4/4] doc: update virsh domstats documentation for vcpu statistics

[Viktor Mihajlovski]

Added description for new vcpu..halted statistics value.
While there, also added a description for vcpu..wait and
clarified the units displayed for time and wait.

Signed-off-by: Viktor Mihajlovski 
Reviewed-by: Bjoern Walk 
---
 tools/virsh.pod | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/tools/virsh.pod b/tools/virsh.pod
index 1fe4269..9e3a248 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -887,6 +887,9 @@ default all supported statistics groups are returned. 
Supported
 statistics groups flags are: I<--state>, I<--cpu-total>, I<--balloon>,
 I<--vcpu>, I<--interface>, I<--block>, I<--perf>.
 
+Note that - depending on the hypervisor type and version or the domain state
+- not all of the following statistics may be returned.
+
 When selecting the I<--state> group the following fields are returned:
 "state.state" - state of the VM, returned as number from virDomainState enum,
 "state.reason" - reason for entering given state, returned as int from
@@ -917,6 +920,10 @@ I<--vcpu> returns:
 "vcpu..state" - state of the virtual CPU , as number
 from virVcpuState enum,
 "vcpu..time" - virtual cpu time spent by virtual CPU 
+ (in microseconds),
+"vcpu..wait" - virtual cpu time spent by virtual CPU 
+waiting on I/O (in microseconds),
+"vcpu..halted" - virtual CPU  is halted: yes or no
 
 I<--interface> returns:
 "net.count" - number of network interfaces on this domain,
-- 
1.9.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 1/4] qemu: Add monitor support for CPU halted state

[Viktor Mihajlovski]

Extended the qemuMonitorCPUInfo with a halted flag. Extract the halted
flag for both text and JSON monitor.

Signed-off-by: Viktor Mihajlovski 
Signed-off-by: Boris Fiuczynski 
---
 src/qemu/qemu_monitor.c  | 6 +-
 src/qemu/qemu_monitor.h  | 3 +++
 src/qemu/qemu_monitor_json.c | 3 +++
 src/qemu/qemu_monitor_text.c | 8 +++-
 tests/qemumonitorjsontest.c  | 8 
 5 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 5175f4e..d9f66a2 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -1677,6 +1677,7 @@ qemuMonitorCPUInfoClear(qemuMonitorCPUInfoPtr cpus,
 cpus[i].thread_id = -1;
 cpus[i].vcpus = 0;
 cpus[i].tid = 0;
+cpus[i].halted = false;
 
 VIR_FREE(cpus[i].qom_path);
 VIR_FREE(cpus[i].alias);
@@ -1725,8 +1726,10 @@ qemuMonitorGetCPUInfoLegacy(struct 
qemuMonitorQueryCpusEntry *cpuentries,
 size_t i;
 
 for (i = 0; i < maxvcpus; i++) {
-if (i < ncpuentries)
+if (i < ncpuentries) {
 vcpus[i].tid = cpuentries[i].tid;
+vcpus[i].halted = cpuentries[i].halted;
+}
 
 /* for legacy hotplug to work we need to fake the vcpu count added by
  * enabling a given vcpu */
@@ -1864,6 +1867,7 @@ qemuMonitorGetCPUInfoHotplug(struct 
qemuMonitorQueryHotpluggableCpusEntry *hotpl
 }
 
 vcpus[anyvcpu].tid = cpuentries[j].tid;
+vcpus[anyvcpu].halted = cpuentries[j].halted;
 }
 
 return 0;
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index 7d78e5b..cb27412 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -394,6 +394,7 @@ int qemuMonitorSystemPowerdown(qemuMonitorPtr mon);
 struct qemuMonitorQueryCpusEntry {
 pid_t tid;
 char *qom_path;
+bool halted;
 };
 void qemuMonitorQueryCpusFree(struct qemuMonitorQueryCpusEntry *entries,
   size_t nentries);
@@ -441,6 +442,8 @@ struct _qemuMonitorCPUInfo {
 
 /* internal for use in the matching code */
 char *qom_path;
+
+bool halted;
 };
 typedef struct _qemuMonitorCPUInfo qemuMonitorCPUInfo;
 typedef qemuMonitorCPUInfo *qemuMonitorCPUInfoPtr;
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index b93220b..dff6d42 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -1349,6 +1349,7 @@ qemuMonitorJSONExtractCPUInfo(virJSONValuePtr data,
 for (i = 0; i < ncpus; i++) {
 virJSONValuePtr entry = virJSONValueArrayGet(data, i);
 int thread = 0;
+bool halted = false;
 const char *qom_path;
 if (!entry) {
 ret = -2;
@@ -1358,9 +1359,11 @@ qemuMonitorJSONExtractCPUInfo(virJSONValuePtr data,
 /* Some older qemu versions don't report the thread_id so treat this as
  * non-fatal, simply returning no data */
 ignore_value(virJSONValueObjectGetNumberInt(entry, "thread_id", 
));
+ignore_value(virJSONValueObjectGetBoolean(entry, "halted", ));
 qom_path = virJSONValueObjectGetString(entry, "qom_path");
 
 cpus[i].tid = thread;
+cpus[i].halted = halted;
 if (VIR_STRDUP(cpus[i].qom_path, qom_path) < 0)
 goto cleanup;
 }
diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c
index ff7cc79..f975347 100644
--- a/src/qemu/qemu_monitor_text.c
+++ b/src/qemu/qemu_monitor_text.c
@@ -521,7 +521,7 @@ qemuMonitorTextQueryCPUs(qemuMonitorPtr mon,
  * (qemu) info cpus
  * * CPU #0: pc=0x000f0c4a thread_id=30019
  *   CPU #1: pc=0xfff0 thread_id=30020
- *   CPU #2: pc=0xfff0 thread_id=30021
+ *   CPU #2: pc=0xfff0 (halted) thread_id=30021
  *
  */
 line = qemucpus;
@@ -541,6 +541,12 @@ qemuMonitorTextQueryCPUs(qemuMonitorPtr mon,
 
 cpu.tid = tid;
 
+/* Extract halted indicator */
+if ((offset = strstr(line, "(halted)")) != NULL)
+cpu.halted = true;
+else
+cpu.halted = false;
+
 if (VIR_APPEND_ELEMENT_COPY(cpus, ncpus, cpu) < 0) {
 ret = -1;
 goto cleanup;
diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c
index 993a373..5e72a0f 100644
--- a/tests/qemumonitorjsontest.c
+++ b/tests/qemumonitorjsontest.c
@@ -1332,10 +1332,10 @@ testQemuMonitorJSONqemuMonitorJSONQueryCPUs(const void 
*data)
 int ret = -1;
 struct qemuMonitorQueryCpusEntry *cpudata = NULL;
 struct qemuMonitorQueryCpusEntry expect[] = {
-{17622, (char *) "/machine/unattached/device[0]"},
-{17624, (char *) "/machine/unattached/device[1]"},
-{17626, (char *) "/machine/unattached/device[2]"},
-{17628, NULL},
+{17622, (char *) "/machine/unattached/device[0]", true},
+{17624, (char *) "/machine/unattached/device[1]", true},
+   

[libvirt] [PATCH 2/4] qemu: Add domain support for VCPU halted state

[Viktor Mihajlovski]

Adding a field to the domain's private vcpu object to hold the halted
state information.
Adding two functions in support of the halted state:
- qemuDomainGetVcpuHalted: retrieve the halted state from a
  private vcpu object
- qemuDomainRefreshVcpuHalted: obtain the per-vcpu halted states
  via qemu monitor and store the results in the private vcpu objects

Signed-off-by: Viktor Mihajlovski 
Reviewed-by: Bjoern Walk 
Reviewed-by: Hao QingFeng 
Signed-off-by: Boris Fiuczynski 
---
 src/qemu/qemu_domain.c | 66 ++
 src/qemu/qemu_domain.h |  5 
 2 files changed, 71 insertions(+)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 0c9416f..beb2d20 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -6045,6 +6045,72 @@ qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver,
 return ret;
 }
 
+/**
+ * qemuDomainGetVcpuHalted:
+ * @vm: domain object
+ * @vcpu: cpu id
+ *
+ * Returns the vCPU halted state.
+  */
+bool
+qemuDomainGetVcpuHalted(virDomainObjPtr vm,
+unsigned int vcpuid)
+{
+virDomainVcpuDefPtr vcpu = virDomainDefGetVcpu(vm->def, vcpuid);
+return QEMU_DOMAIN_VCPU_PRIVATE(vcpu)->halted;
+}
+
+/**
+ * qemuDomainRefreshVcpuHalted:
+ * @driver: qemu driver data
+ * @vm: domain object
+ * @asyncJob: current asynchronous job type
+ *
+ * Updates vCPU halted state in the private data of @vm.
+ *
+ * Returns 0 on success and -1 on error
+ */
+int
+qemuDomainRefreshVcpuHalted(virQEMUDriverPtr driver,
+virDomainObjPtr vm,
+int asyncJob)
+{
+virDomainVcpuDefPtr vcpu;
+qemuMonitorCPUInfoPtr info = NULL;
+size_t maxvcpus = virDomainDefGetVcpusMax(vm->def);
+size_t i;
+bool hotplug;
+int rc;
+int ret = -1;
+
+/* Not supported currently for TCG, see qemuDomainRefreshVcpuInfo */
+if (vm->def->virtType == VIR_DOMAIN_VIRT_QEMU)
+return 0;
+
+hotplug = qemuDomainSupportsNewVcpuHotplug(vm);
+
+if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
+return -1;
+
+rc = qemuMonitorGetCPUInfo(qemuDomainGetMonitor(vm), , maxvcpus, 
hotplug);
+
+if (qemuDomainObjExitMonitor(driver, vm) < 0)
+goto cleanup;
+
+if (rc < 0)
+goto cleanup;
+
+for (i = 0; i < maxvcpus; i++) {
+vcpu = virDomainDefGetVcpu(vm->def, i);
+QEMU_DOMAIN_VCPU_PRIVATE(vcpu)->halted = info[i].halted;
+}
+
+ret = 0;
+
+ cleanup:
+qemuMonitorCPUInfoFree(info, maxvcpus);
+return ret;
+}
 
 bool
 qemuDomainSupportsNicdev(virDomainDefPtr def,
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index dee5d93..ec20796 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -320,6 +320,7 @@ struct _qemuDomainVcpuPrivate {
 pid_t tid; /* vcpu thread id */
 int enable_id; /* order in which the vcpus were enabled in qemu */
 char *alias;
+bool halted;
 
 /* information for hotpluggable cpus */
 char *type;
@@ -665,6 +666,10 @@ int qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver,
   virDomainObjPtr vm,
   int asyncJob,
   bool state);
+bool qemuDomainGetVcpuHalted(virDomainObjPtr vm, unsigned int vcpu);
+int qemuDomainRefreshVcpuHalted(virQEMUDriverPtr driver,
+virDomainObjPtr vm,
+int asyncJob);
 
 bool qemuDomainSupportsNicdev(virDomainDefPtr def,
   virDomainNetDefPtr net);
-- 
1.9.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH V3 0/4] qemu: report actual vcpu state in virVcpuInfo

[Viktor Mihajlovski]

On 13.10.2016 00:46, John Ferlan wrote:
[...]

 Despite fixing user strings in this version to show the word
 'running' the enum value [1] is still changed for the new one.
 Since the actual value is used by programs and not humans that may
 break and I'm not comfortable allowing such change in semantics.
> Hm, really? From an API perspective up to now it was perfectly OK to
> accept any of "offline", "running" and "blocked".
> I maintain that it is valid to extend the state enumeration and virsh
> shows how to handle previously unknown states.
> 
>> I think the objection is for the vcpuinfo command which shouldn't need
>> to be CPU aware, but perhaps similar to how aware the hotplug code had
>> to be made for the differences between arch's, maybe the info code needs
>> that as well (as ugly as that could be). I'm just trying to throw some
>> suggestions out there to see if anything sticks.
> 
>> It's painful that the "halted" bit has multiple meanings, but I think
>> it's handle-able - we just have to find the right way to do it...
> 
I think it's not libvirt's task to interpret the meaning of halted as
reported by QEMU. That's why I added the new state in the first place
instead of reusing e.g. offline, which would have introduced
architecture depend code which I'd like to avoid where possible.
>> Another "thought" that strikes me is whether there's a "valid" scenario
>> where someone can halt a CPU from the guest and how that gets propagated
>> back - even on x86. OK it's a slight divergence...
> 
>> While I did suggest some sort of running (halted) output, does seeing
>> that on for a non x86 guest make sense for the state the CPU is in? In a
>> former job, I think we called it "running (idle)" mainly because
>> "halted" has a somewhat negative connotation.
> 
>> So another option might be to allow a vcpuinfo user to pass a new flag
>> that would allow that return of this more detailed state. Only if the
>> flag was set would we check/return the new state. I think that's what
>> we've done in the past for similar things where changing the "default"
>> return/display value could cause "issues" for those programs that rely
>> on the string being "running" without " (halted)" or " (idle)".  Does
>> this make sense?
>
In essence that would mean to implement a new API, which seems to be too
heavy a hammer for this small nail.
>> Then for the other (stats) output, returning the value of the "halted"
>> field could be an "additional" set of output/data to be added rather
>> than overloading the existing field. Does this make sense?
> 
> 
That's what I'll do, expect a new series soon...
[...]
> And, if you find value in the virsh.pod update in 1/4, you can either
> pick the parts you need or let me know and I can provide an abridged
> version.
> [...]
> 
>> I do like the adjustments made to virsh.pod.
> 
There you go...
>> John
> 
>>
>> --
>> libvir-list mailing list
>> libvir-list@redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list
>>
> 

-- 

Mit freundlichen Grüßen/Kind Regards
   Viktor Mihajlovski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martina Köderitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH] doc: Describe the VCPU states returned by virsh vcpuinfo

[Viktor Mihajlovski]

Added a brief description of the VCPU states.

Signed-off-by: Viktor Mihajlovski 
---
 tools/virsh.pod | 49 +
 1 file changed, 49 insertions(+)

diff --git a/tools/virsh.pod b/tools/virsh.pod
index 9e3a248..6d4fd07 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -2536,6 +2536,55 @@ vCPUs, the running time, the affinity to physical 
processors.
 
 With I<--pretty>, cpu affinities are shown as ranges.
 
+An example output is
+
+ $ virsh vcpuinfo fedora
+ VCPU:   0
+ CPU:0
+ State:  running
+ CPU time:   7,0s
+ CPU Affinity:   
+
+ VCPU:   1
+ CPU:1
+ State:  running
+ CPU time:   0,7s
+ CPU Affinity:   
+
+B
+
+The State field displays the current operating state of a virtual CPU
+
+=over 4
+
+=item B
+
+The virtual CPU is offline and not usable by the domain.
+This state is not supported by all hypervisors.
+
+=item B
+
+The virtual CPU is available to the domain and is operating.
+
+=item B
+
+The virtual CPU is available to the domain but is waiting for a resource.
+This state is not supported by all hypervisors, in which case I
+may be reported instead.
+
+=item B
+
+The virtual CPU state could not be determined. This could happen if
+the hypervisor is newer than virsh.
+
+=item B
+
+There's no information about the virtual CPU state available. This can
+be the case if the domain is not running or the hypervisor does
+not report the virtual CPU state.
+
+=back
+
 =item B I [I] [I] [[I<--live>]
 [I<--config>] | [I<--current>]]
 
-- 
1.9.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] virlog: Enclose part of virLogDefineOutputs in HAVE_SYSLOG_H block

[John Ferlan]

On 10/10/2016 08:04 AM, Erik Skultety wrote:
> Commit 640b58ab broke the mingw build because it referenced 'current_ident'
> as well as 'openlog' symbols both of which are declared conditionally within
> HAVE_SYSLOG_H directive. This patch fixes the broken build. Additional changes
> like moving all variable declarations in virLogDefineOutputs into the
> conditional block were necessary to avoid 'unused variable' errors from mingw.
> 
> Signed-off-by: Erik Skultety 
> ---
> I could've pushed it under build breaker rule but I'd rather be safe than 
> sorry
> that I missed anything else.
> 
> Erik
> 
> 
>  src/util/virlog.c | 28 ++--
>  1 file changed, 14 insertions(+), 14 deletions(-)
> 

Can the syslog specific hunk just be moved into a #if HAVE_SYSLOG
specific section of code as a virLogFindSyslogOutputs(outputs,
noutputs), then called inside an #if HAVE_SYSLOG_H  (similar to
virLogNewOutputToSyslog in virLogParseOutput)

While the following works - it intersperses new variable definitions
inside the #if's and doesn't seem "as clean"

John
> diff --git a/src/util/virlog.c b/src/util/virlog.c
> index 14ee701..f52d9d8 100644
> --- a/src/util/virlog.c
> +++ b/src/util/virlog.c
> @@ -1362,16 +1362,15 @@ virLogFindOutput(virLogOutputPtr *outputs, size_t 
> noutputs,
>  int
>  virLogDefineOutputs(virLogOutputPtr *outputs, size_t noutputs)
>  {
> -int ret = -1;
> -int id;
> +if (virLogInitialize() < 0)
> +return -1;
> +
> +virLogLock();
> +virLogResetOutputs();
> +
> +#if HAVE_SYSLOG_H
> +int id = -1;
>  char *tmp = NULL;
> -
> -if (virLogInitialize() < 0)
> -return -1;
> -
> -virLogLock();
> -virLogResetOutputs();
> -
>  /* syslog needs to be special-cased, since it keeps the fd in private */
>  if ((id = virLogFindOutput(outputs, noutputs, VIR_LOG_TO_SYSLOG,
> current_ident)) != -1) {
> @@ -1379,20 +1378,21 @@ virLogDefineOutputs(virLogOutputPtr *outputs, size_t 
> noutputs)
>   * holding the lock so it's safe to call openlog and change the 
> message
>   * tag
>   */
> -if (VIR_STRDUP_QUIET(tmp, outputs[id]->name) < 0)
> -goto cleanup;
> +if (VIR_STRDUP_QUIET(tmp, outputs[id]->name) < 0) {
> +virLogUnlock();
> +return -1;
> +}
>  VIR_FREE(current_ident);
>  current_ident = tmp;
>  openlog(current_ident, 0, 0);
>  }
> +#endif
>  
>  virLogOutputs = outputs;
>  virLogNbOutputs = noutputs;
>  
> -ret = 0;
> - cleanup:
>  virLogUnlock();
> -return ret;
> +return 0;
>  }
>  
>  
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 3/4] vbox: change API (un)initialization logic.

[Martin Kletzander]

On Wed, Oct 12, 2016 at 04:00:52PM -0400, Dawid Zamirski wrote:

On Tue, 2016-10-11 at 10:58 -0400, Dawid Zamirski wrote:

On Tue, 2016-10-11 at 16:22 +0200, Martin Kletzander wrote:
> On Wed, Sep 28, 2016 at 01:41:35PM -0400, Dawid Zamirski wrote:
>
> The allocation is not guarded by any lock, so there's still a
> race.  I
> think there should be a global struct that has only some lock in it
> and
> whatever global data you need, the struct will be initialized on
> the
> first call to any function (check out VIR_ONCE_GLOBAL_INIT) and
> then
> the
> connection (or global data or how it's called) would be reference
> counted (just like you have).  It's just that having the reference
> count
> in the object you will be reallocating over and over again for each
> connection is not really good.
>


Thanks, I see, I'll address this in v2



So after my initial attempts at handling this as suggested (create
vboxDriver struct and initialize in VIR_ONCE_GLOBAL_INIT), I've
stumbled upon virStateDriver structs used in other drivers that
apparently has .stateInitialize, .stateCleanup that to me look like
perfect places to call vbox's pfnComInitialize pfnComUnintialize pairs.
Would this be a good idea to utilize the virStateDriver for this or
should I stick with VIR_ONCE_GLOBAL_INIT way?



I just now realized you said the issue we are talking about crashes the
libvirt daemon, right?  And it does not use virStateDriver?  That's
weird.  You are connecting to vbox:///session, but the crash happens in
the libvirt daemon.  That means we switched the vbox driver to the
daemo-side driver, so I'd expect it to be stateful driver.  However it
doesn't use virStateDriver.  I'm Cc'ing Michal to let him look at it as
he's more handy in the whole vbox area.


PS. Thanks a lot for reviewing my code and guiding me through it all.


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/1] qemu: host NUMA hugepage policy without guest NUMA

[Martin Kletzander]

On Thu, Oct 13, 2016 at 11:34:16AM +1100, Sam Bobroff wrote:

On Wed, Oct 12, 2016 at 10:27:50AM +0200, Martin Kletzander wrote:

On Wed, Oct 12, 2016 at 03:04:53PM +1100, Sam Bobroff wrote:
>At the moment, guests that are backed by hugepages in the host are
>only able to use policy to control the placement of those hugepages
>on a per-(guest-)CPU basis. Policy applied globally is ignored.
>
>Such guests would use  and
>a  block with  but no .../> elements.
>
>This patch corrects this by, in this specific case, changing the QEMU
>command line from "-mem-prealloc -mem-path=..." (which cannot
>specify NUMA policy) to "-object memory-backend-file ..." (which can).
>
>Note: This is not visible to the guest and does not appear to create
>a migration incompatibility.
>

It could make sense, I haven't tried yet, though.  However, I still
don't see the point in using memory-backend-file.  Is it that when you
don't have cpuset cgroup the allocation doesn't work well?  Because it
certainly does work for me.


Thanks for taking a look at this :-)

The point of using a memory-backend-file is that with it, the NUMA policy can
be specified to QEMU, but with -mem-path it can't. It seems to be a way to tell
QEMU to apply NUMA policy in the right place. It does seem odd to me to use
memory-backend-file without attaching the backend to a guest NUMA node, but it
seems to do the right thing in this case. (If there are guest NUMA nodes, or if
hugepages aren't being used, policy is correctly applied.)

I'll describe my test case in detail, perhaps there's something I don't 
understand
happening.

* I set up a machine with two (fake) NUMA nodes (0 and 1), with 2G of hugepages
 on node 1, and none on node 0.

* I create a 2G guest using virt-install:

virt-install --name ppc --memory=2048 --disk ~/tmp/tmp.qcow2 --cdrom 
~/tmp/ubuntu-16.04-server-ppc64el.iso --wait 0 --virt-type qemu --memorybacking 
hugepages=on --graphics vnc --arch ppc64le

* I "virsh destroy" and then "virsh edit" to add this block to the guest XML:

 

 

* "virsh start", and the machine starts (I believe it should fail due to 
insufficient memory satasfying the policy).
* "numastat -p $(pidof qemu-system-ppc64)" shows something like this:

Per-node process memory usage (in MBs) for PID 8048 (qemu-system-ppc)
  Node 0  Node 1   Total
 --- --- ---
Huge 0.00 2048.00 2048.00
Heap 8.120.008.12
Stack0.030.000.03
Private 35.806.10   41.90
  --- --- ---
Total   43.95 2054.10 2098.05

So it looks like it's allocated hugepages from node 1, isn't this violating the
policy I set via numatune?



Oh, now I get it.  We are doing our best to apply that policy to qemu
even when we don't have this option.  However, using this works even
better (which is probably* what we want).  And that's the reasoning
behind this.

* I'm saying probably because when I was adding numactl binding to be
  used together with cgroups, I was told that we couldn't change the
  binding afterwards and it's bad.  I feel like we could do something
  with that and it would help us in the future, but there needs to be a
  discussion, I guess.  Because I might be one of the few =)

So to recapitulate that, there are three options how to affect the
allocation of qemu's memory:

1) numactl (libnuma): it works as expected, but cannot be changed later

2) cgroups: so strict it has to be applied after qemu started, due to
   that it doesn't work right, especially for stuff that gets all
   pre-allocated (like hugepages).  it can be changed later, but it
   won't always mean the memory will migrate, so upon change there is
   no guarantee.  If it's unavailable, we fallback to (1) anyway

3) memory-backing-file's host-nodes=: this works as expected, but
   cannot be used with older QEMUs, cannot be changed later and in some
   cases (not your particular one) it might screw up migration if it
   wasn't used before.

Selecting the best option from these, plus making the code work with
every possibility (erroring out when you want to change the memory node
and we had to use (1) for example) is a pain.  We should really think
about that and reorganize these things for the better of the future.
Otherwise we're going to get overwhelm ourselves.  Cc'ing Peter to get
his thoughts as well as he worked on some parts of this as well.

Martin


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 0/2] Minor panic device fixes

[Peter Krempa]

On Thu, Oct 13, 2016 at 10:28:34 +0200, Martin Kletzander wrote:
> Completely new BLURB, just for €99.99, call now!

That's a bargin!

ACK series



signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 0/2] Minor panic device fixes

[Martin Kletzander]

Completely new BLURB, just for €99.99, call now!


Martin Kletzander (2):
  schema: Allow alias for panic device
  conf: Honour flags in virDomainPanicDefParseXML

 docs/schemas/domaincommon.rng |  3 +++
 src/conf/domain_conf.c| 10 +-
 2 files changed, 8 insertions(+), 5 deletions(-)

--
2.10.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 2/2] conf: Honour flags in virDomainPanicDefParseXML

[Martin Kletzander]

Without them we're keeping  even for inactive XML.

Signed-off-by: Martin Kletzander 
---
 src/conf/domain_conf.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 4fa6976ea63e..cd1384a5c48f 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -10692,7 +10692,8 @@ virDomainTPMDefParseXML(xmlNodePtr node,
 }

 static virDomainPanicDefPtr
-virDomainPanicDefParseXML(xmlNodePtr node)
+virDomainPanicDefParseXML(xmlNodePtr node,
+  unsigned int flags)
 {
 virDomainPanicDefPtr panic;
 char *model = NULL;
@@ -10700,7 +10701,7 @@ virDomainPanicDefParseXML(xmlNodePtr node)
 if (VIR_ALLOC(panic) < 0)
 return NULL;

-if (virDomainDeviceInfoParseXML(node, NULL, >info, 0) < 0)
+if (virDomainDeviceInfoParseXML(node, NULL, >info, flags) < 0)
 goto error;

 model = virXMLPropString(node, "model");
@@ -13592,7 +13593,7 @@ virDomainDeviceDefParse(const char *xmlStr,
 goto error;
 break;
 case VIR_DOMAIN_DEVICE_PANIC:
-if (!(dev->data.panic = virDomainPanicDefParseXML(node)))
+if (!(dev->data.panic = virDomainPanicDefParseXML(node, flags)))
 goto error;
 break;
 case VIR_DOMAIN_DEVICE_MEMORY:
@@ -17545,8 +17546,7 @@ virDomainDefParseXML(xmlDocPtr xml,
 if (n && VIR_ALLOC_N(def->panics, n) < 0)
 goto error;
 for (i = 0; i < n; i++) {
-virDomainPanicDefPtr panic =
-virDomainPanicDefParseXML(nodes[i]);
+virDomainPanicDefPtr panic = virDomainPanicDefParseXML(nodes[i], 
flags);
 if (!panic)
 goto error;

-- 
2.10.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 1/2] schema: Allow alias for panic device

[Martin Kletzander]

As with all other devices, it's not part of 'address'.

Signed-off-by: Martin Kletzander 
---
 docs/schemas/domaincommon.rng | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 6eeb4e9e7df0..3f96456b5c8e 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -5534,6 +5534,9 @@
 
   
   
+
+  
+  
 
   
 
-- 
2.10.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] src: Threat pid as signed

[Martin Kletzander]

On Fri, Oct 07, 2016 at 11:22:34AM +0200, Michal Privoznik wrote:

This initially started as a fix of some debug printing in
virCgroupDetect. However it turned out that other places suffer
from the similar problem. While dealing with pids, esp. in cases
where we cannot use pid_t for ABI stability reasons, we often
chose an unsigned integer type. This makes no sense as pid_t is
signed.
Also, new syntax-check rule is introduced so we won't repeat this
mistake.

Signed-off-by: Michal Privoznik 
---

Technically, this is v2 of [1], but like 99% of the patch is
different, so I'm sending this as a completely new patch.

1: https://www.redhat.com/archives/libvir-list/2016-October/msg00254.html

cfg.mk|  8 
src/locking/lock_driver_sanlock.c |  4 ++--
src/lxc/lxc_controller.c  |  4 ++--
src/lxc/lxc_domain.c  |  8 
src/lxc/lxc_driver.c  |  4 ++--
src/lxc/lxc_monitor.c |  3 +--
src/lxc/lxc_process.c |  8 
src/qemu/qemu_process.c   | 12 ++--
src/util/vircgroup.c  | 24 
src/util/viridentity.c|  2 +-
src/util/virpolkit.c  |  1 +
src/util/virprocess.c | 14 ++
src/util/virsystemd.c |  9 +
src/util/virutil.c|  4 ++--
14 files changed, 56 insertions(+), 49 deletions(-)

diff --git a/cfg.mk b/cfg.mk
index 9f5949c..b33b1e2 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -581,6 +581,11 @@ sc_prohibit_int_assign_bool:
halt='use bool type for boolean values' \
  $(_sc_search_regexp)

+sc_prohibit_unsigned_pid:
+   @prohibit='\ [^,=]+pid'  
  \


I'd also add ';' and '(' in this  ^^ list of characters, but that's your
call on those.


+   halt='use signed type for pid values'   \
+ $(_sc_search_regexp)
+
# Many of the function names below came from this filter:
# git grep -B2 '\<_('|grep -E '\.c- *[[:alpha:]_][[:alnum:]_]* ?\(.*[,;]$' \
# |sed 's/.*\.c-  *//'|perl -pe 's/ ?\(.*//'|sort -u \
@@ -1214,6 +1219,9 @@ 
exclude_file_name_regexp--sc_prohibit_include_public_headers_brackets = \
exclude_file_name_regexp--sc_prohibit_int_ijk = \
  
^(src/remote_protocol-structs|src/remote/remote_protocol\.x|cfg\.mk|include/libvirt/libvirt.+|src/admin_protocol-structs|src/admin/admin_protocol\.x)$$

+exclude_file_name_regexp--sc_prohibit_unsigned_pid = \
+  
^(include/libvirt/libvirt-qemu.h|src/(driver-hypervisor.h|libvirt-qemu.c|locking/lock_protocol.x|lxc/lxc_monitor_protocol.x|qemu/qemu_driver.c|remote/qemu_protocol.x|util/virpolkit.c|util/virsystemd.c)|tests/virpolkittest.c|tools/virsh-domain.c)$$
+


It's regexp, so dots need to be escaped.  Also if this needs such
exceptions, I don't see the point in the syntax-check.  But anyway, I
think all protocols and all public APIs should have this exception, so
it can be made shorter.  I came up with this:

^(include/libvirt/.*\.h|src/(qemu/qemu_driver\.c|driver-hypervisor\.h|libvirt(-[a-z]*)?\.c|.*\.x|util/vir(polkit|systemd)\.c)|tests/virpolkittest\.c|tools/virsh-domain\.c)$$


diff --git a/src/util/virpolkit.c b/src/util/virpolkit.c
index e7e46b8..d8c35f1 100644
--- a/src/util/virpolkit.c
+++ b/src/util/virpolkit.c
@@ -82,6 +82,7 @@ int virPolkitCheckAuth(const char *actionid,
VIR_INFO("Checking PID %lld running as %d",
 (long long) pid, uid);

+/* Yes, PolicyKit really takes pid ad uint. */
if (virDBusCallMethod(sysbus,
  ,
  NULL,


You can safely drop this ^^ hunk.


diff --git a/src/util/virprocess.c b/src/util/virprocess.c
index 8b9f9c5..718c4a2 100644
--- a/src/util/virprocess.c
+++ b/src/util/virprocess.c
@@ -608,8 +608,7 @@ int virProcessGetPids(pid_t pid, size_t *npids, pid_t 
**pids)
*npids = 0;
*pids = NULL;

-if (virAsprintf(, "/proc/%llu/task",
-(unsigned long long)pid) < 0)
+if (virAsprintf(, "/proc/%llu/task", (long long) pid) < 0)
goto cleanup;

if (virDirOpen(, taskPath) < 0)


I see you changed lot of '(type)val' casts to '(type) val', and even
though I used to hate this, I'm kinda more in favour of the latter
nowadays.  And guess what, I looked up how it looks in the code and
after this patch, the ratio of non-spaced to spaced type casts changed
from 751:3394 to 727:3417 (very roughly), so Yay!

ACK with nits fixed.

Martin


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] leasetime support per and

[Michal Privoznik]

On 13.10.2016 00:03, Alberto Ruiz wrote:
> Support for custom dhcp wide and per host leasetime.
> 
> It is specified as a child tag for :
> 
>   24h
>   ...
> 
> 
> And as an attribute for :
> 
>   
> 
> 
> These are the different notations:
> 
> -1   (infinite/unlimited lease)
> 120  (seconds are the default unit, 120 seconds is the minimum, if less is
> specified it will use 120)
> 300s (seconds)
> 5m   (minutes)
> 24h  (hours)
> 7d   (days)
> ---

I know I'm stepping on a moving train (sorry for that), but I have two
points to raise:

1) use git send-email, this patch is mangled by your MTA and does not apply.
2) 120 seconds is minimum because of dnsmasq? If so, I think we should
error out instead of silently changing this to a different value behind
user's back.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list