Re: [libvirt] [PATCH 4/4] qemu: Escape commas for qemuBuildGrapicsSPICECommandLine

[John Ferlan]

On 06/18/2018 01:57 PM, Anya Harter wrote:
> Add comma escaping for cfg->spiceTLSx509certdir and
> graphics->data.spice.rendernode.
> 
> Signed-off-by: Anya Harter 
> ---
>  src/qemu/qemu_command.c | 11 ---
>  tests/qemuxml2argvdata/name-escape.args |  5 +++--
>  tests/qemuxml2argvdata/name-escape.xml  |  1 +
>  tests/qemuxml2argvtest.c|  5 +
>  4 files changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index a9a5e200e9..36dccea9b2 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -7974,8 +7974,11 @@ 
> qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg,
>  !cfg->spicePassword)
>  virBufferAddLit(, "disable-ticketing,");
>  
> -if (hasSecure)
> -virBufferAsprintf(, "x509-dir=%s,", cfg->spiceTLSx509certdir);
> +if (hasSecure) {
> +virBufferAddLit(, "x509-dir=");
> +virQEMUBuildBufferEscapeComma(, cfg->spiceTLSx509certdir);
> +virBufferAsprintf(, ",");

make syntax-check would have told you:

src/qemu/qemu_command.c:7980:virBufferAsprintf(, ",");
src/qemu/qemu_command.c:8090:virBufferAsprintf(, ",");

So here and below, I changed to virBufferAddLit before pushing.

> +}
>  
>  switch (graphics->data.spice.defaultMode) {
>  case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_SECURE:
> @@ -8082,7 +8085,9 @@ 
> qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg,
>  goto error;
>  }
>  
> -virBufferAsprintf(, "rendernode=%s,", 
> graphics->data.spice.rendernode);
> +virBufferAddLit(, "rendernode=");
> +virQEMUBuildBufferEscapeComma(, 
> graphics->data.spice.rendernode);
> +virBufferAsprintf(, ",");
>  }
>  }

Reviewed-by: John Ferlan 

John

>From the last time I passed through this when Sukrit posted patches,
still to do are qemuBuildHostNetStr, qemuBuildDiskThrottling (for group
name), and various qemuBuildNetworkDriveURI, qemuBuildNetworkDriveStr,
and qemuGetDriveSourceString.

[...]

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

Re: [libvirt] [PATCH 1/4] qemu: Escape commas for qemuBuildChrChardevStr

[John Ferlan]

On 06/18/2018 01:57 PM, Anya Harter wrote:
> Add comma escaping for dev->data.file.path in cases
> VIR_DOMAIN_CHR_TYPE_DEV and VIR_DOMAIN_CHR_TYPE_PIPE.
> 
> Signed-off-by: Anya Harter 
> ---
>  src/qemu/qemu_command.c | 9 +
>  tests/qemuxml2argvdata/name-escape.args | 4 
>  tests/qemuxml2argvdata/name-escape.xml  | 7 +++
>  tests/qemuxml2argvtest.c| 3 ++-
>  4 files changed, 18 insertions(+), 5 deletions(-)
> 

Having tests is awesome! Thanks!

Not sure why the bite size tasks omitted VIR_DOMAIN_CHR_TYPE_FILE too.
If you want to investigate the FILE case that'd be good - just to make
sure we aren't missing any!  I'll still push this as is since it's fine,
but if the FILE needs something it can be added afterwards.

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH 2/4] qemu: Escape commas for qemuBuildChrChardevFileStr

[John Ferlan]

On 06/18/2018 01:57 PM, Anya Harter wrote:
> Add comma escaping for fileval.
> 
> Signed-off-by: Anya Harter 
> ---
>  src/qemu/qemu_command.c | 3 ++-
>  tests/qemuxml2argvdata/name-escape.args | 2 ++
>  tests/qemuxml2argvdata/name-escape.xml  | 4 
>  tests/qemuxml2argvtest.c| 3 ++-
>  4 files changed, 10 insertions(+), 2 deletions(-)
> 

Reviewed-by: John Ferlan 

John



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

Re: [libvirt] [PATCH] tests: add test file for smartcard database

[John Ferlan]

On 06/15/2018 10:45 AM, Anya Harter wrote:
> Add test case explicitly defining a smartcard host certificates
> database via the following xml:
> 
> 
>   /tmp/foo
> 
> 
> This case is not currently covered in the test suite.
> 
> Signed-off-by: Anya Harter 
> ---
>  .../smartcard-host-certificates-database.args | 28 +++
>  .../smartcard-host-certificates-database.xml  | 21 +++
>  tests/qemuxml2argvtest.c  |  2 ++
>  .../smartcard-host-certificates-database.xml  | 35 +++
>  tests/qemuxml2xmltest.c   |  1 +
>  5 files changed, 87 insertions(+)
>  create mode 100644 
> tests/qemuxml2argvdata/smartcard-host-certificates-database.args
>  create mode 100644 
> tests/qemuxml2argvdata/smartcard-host-certificates-database.xml
>  create mode 100644 
> tests/qemuxml2xmloutdata/smartcard-host-certificates-database.xml
> 

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH 3/4] qemu: Escape commas for qemuBuildSmartcardCommandLine

[John Ferlan]

On 06/18/2018 01:57 PM, Anya Harter wrote:
> Add comma escaping for smartcard->data.cert.file[i] and
> smartcard->data.cert.database.
> 
> Signed-off-by: Anya Harter 
> ---
>  src/qemu/qemu_command.c | 21 -
>  tests/qemuxml2argvdata/name-escape.args |  3 +++
>  tests/qemuxml2argvdata/name-escape.xml  |  6 ++
>  tests/qemuxml2argvtest.c|  3 ++-
>  4 files changed, 15 insertions(+), 18 deletions(-)
> 

Reviewed-by: John Ferlan 

John

(and pushed)

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

Re: [libvirt] [PATCH v2 5/5] virsh: Introduce --nowait to domstats

[John Ferlan]

On 06/15/2018 04:18 AM, Michal Privoznik wrote:
> This new switch can be used to set
> VIR_CONNECT_GET_ALL_DOMAINS_STATS_NOWAIT flag for stats
> fetching API.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  tools/virsh-domain-monitor.c |  7 +++
>  tools/virsh.pod  | 16 +++-
>  2 files changed, 18 insertions(+), 5 deletions(-)
> 

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH v2 3/5] qemu_domain: Introduce qemuDomainObjBeginJobNowait

[John Ferlan]

On 06/15/2018 04:18 AM, Michal Privoznik wrote:
> The aim of this API is to allow the caller do best effort. Some

s/do/to do/

> functions can work even when acquiring the job fails (e.g.
> qemuConnectGetAllDomainStats()). But what they can't bear is
> delay if they have to wait up to 30 seconds for each domain that
> is processing some other job.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_domain.c | 43 ++-
>  src/qemu/qemu_domain.h |  4 
>  2 files changed, 42 insertions(+), 5 deletions(-)
> 

Couple of nits noted, one above, one below...

Reviewed-by: John Ferlan 

John


> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 21d54938b6..a01067049e 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -6359,11 +6359,15 @@ qemuDomainJobAllowed(qemuDomainObjPrivatePtr priv, 
> qemuDomainJob job)
>   * @obj: domain object
>   * @job: qemuDomainJob to start
>   * @asyncJob: qemuDomainAsyncJob to start
> + * @nowait: don't wait trying to acquire @job
>   *
>   * Acquires job for a domain object which must be locked before
>   * calling. If there's already a job running waits up to
>   * QEMU_JOB_WAIT_TIME after which the functions fails reporting
> - * an error.
> + * an error unless @nowait is set.

Paragraph break have a empty line for readability.

> + * If @nowait is true this function tries to acquire job and if
> + * it fails, then it returns immediately without waiting. No
> + * error is reported in this case.
>   *
>   * Returns: 0 on success,
>   * -2 if unable to start job because of timeout or

[...]

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

Re: [libvirt] [PATCH v2 4/5] Introduce VIR_CONNECT_GET_ALL_DOMAINS_STATS_NOWAIT

[John Ferlan]

On 06/15/2018 04:18 AM, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1552092
> 
> If there's a long running job it might cause us to wait 30
> seconds before we give up acquiring the job. This is problematic
> to interactive applications that fetch stats repeatedly every few
> seconds.
> 
> The solution is to introduce
> VIR_CONNECT_GET_ALL_DOMAINS_STATS_NOWAIT flag which tries to
> acquire job but does not wait if acquiring failed.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  include/libvirt/libvirt-domain.h |  2 ++
>  src/libvirt-domain.c | 12 
>  src/qemu/qemu_driver.c   | 15 ---
>  3 files changed, 26 insertions(+), 3 deletions(-)
> 

Reviewed-by: John Ferlan 

John

Is the 30 seconds qemu specific?  Could drop it from the commit message
- if you feel so compelled.

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

Re: [libvirt] [PATCH v3 20/20] nwfilter: convert virt drivers to use public API for nwfilter bindings

[John Ferlan]

On 06/14/2018 08:33 AM, Daniel P. Berrangé wrote:
> Remove the callbacks that the nwfilter driver registers with the domain
> object config layer. Instead make the current helper methods call into
> the public API for creating/deleting nwfilter bindings.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  src/conf/domain_nwfilter.c | 124 +
>  src/conf/domain_nwfilter.h |  13 ---
>  src/libvirt_private.syms   |   1 -
>  src/nwfilter/nwfilter_driver.c |  84 +++--
>  src/nwfilter/nwfilter_gentech_driver.c |  42 -
>  src/nwfilter/nwfilter_gentech_driver.h |   4 -
>  6 files changed, 120 insertions(+), 148 deletions(-)
> 

I ran the more "aggressive" Avocado tests based on an old bz
(https://bugzilla.redhat.com/show_bug.cgi?id=1034807) and got the
following in my debug libvirtd output:

2018-06-15 15:46:18.683+: 18817: error :
virNWFilterBindingLookupByPortDev:589 : portdev in
virNWFilterBindingLookupByPortDev must not be NULL
2018-06-15 15:46:18.684+: 18817: error :
virNWFilterBindingLookupByPortDev:589 : portdev in
virNWFilterBindingLookupByPortDev must not be NULL

The first thing the test does is remove the defined interface:

   
  
  
  
  


and then replaces/adds with a new interface:


  
  
  
  
  


for the test domain via device attach.

Then 2 threads are started - 1 that continually starts/destroys the
domain and 1 that continually removes/adds allow-arp. The actual logic
in the test is busted because starting up a domain without a defined
filter will fail and the thread doing the start/stop has no retry
(try/except) logic. When I added the try/except logic and toned down the
retry logic a bit I could get the test to pass with a few adjustments to
the libvirt code here.  Ironically, when the test goes too fast it
caused my CPU's to get hot and generate a false positive failure because
there were dmesg events related to core temperature above threshold.

Anyway, I've noted a couple of places I think adjustments could/should
be made - hopefully they make sense as I started looking "backwards" to
see where things go introduced.


> diff --git a/src/conf/domain_nwfilter.c b/src/conf/domain_nwfilter.c
> index 7570e0ae83..ed45394918 100644
> --- a/src/conf/domain_nwfilter.c
> +++ b/src/conf/domain_nwfilter.c

[...]

>  
> +
>  int
>  virDomainConfNWFilterInstantiate(const char *vmname,
>   const unsigned char *vmuuid,
>   virDomainNetDefPtr net)

Revisiting my comment from patch 13 - it is possible to enter here with
net->filter being NULL. Is it worth returning 0 in that case under the
assumption that there is no filter so that the callers then don't have
to make that check? If portdev/ifname is NULL, not much is going to be
found as well.

>  {
> -if (nwfilterDriver != NULL)
> -return nwfilterDriver->instantiateFilter(vmname, vmuuid, net);
> +virConnectPtr conn = virGetConnectNWFilter();
> +virNWFilterBindingDefPtr def = NULL;
> +virNWFilterBindingPtr binding = NULL;
> +char *xml;
> +int ret = -1;
> +
> +VIR_DEBUG("vmname=%s portdev=%s filter=%s",
> +  vmname, NULLSTR(net->ifname), NULLSTR(net->filter));

Both being NULL probably is not a good thing - what filter for what portdev?

> +
> +if (!conn)
> +goto cleanup;
> +

Based on patch 16/19 comments and the thought above:

 if (!net->ifname ||
 (binding = virNWFilterBindingLookupByPortDev(conn, net->ifname))) {
 ret = 0;
 goto cleanup;
 }

where the !net->ifname may avoids the patch 13 comment issue and the ret
= 0 when finding the binding handles the filter already loaded issue.
The filter would be loaded for a running guest (after at least the
second libvirtd restart) by virNWFilterBindingObjListLoadAllConfigs.

> +if (!(def = virNWFilterBindingDefForNet(vmname, vmuuid, net)))
> +goto cleanup;
> +
> +if (!(xml = virNWFilterBindingDefFormat(def)))
> +goto cleanup;
> +
> +if (!(binding = virNWFilterBindingCreateXML(conn, xml, 0)))
> +goto cleanup;
> +
> +ret = 0;
> +
> + cleanup:
> +VIR_FREE(xml);
> +virNWFilterBindingDefFree(def);
> +virObjectUnref(binding);
> +virObjectUnref(conn);
> +return ret;
> +}
> +
> +
> +static void
> +virDomainConfNWFilterTeardownImpl(virConnectPtr conn,
> +  virDomainNetDefPtr net)
> +{
> +virNWFilterBindingPtr binding;
>  
> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> -   _("No network filter driver available"));
> -return -1;
> +binding = virNWFilterBindingLookupByPortDev(conn, net->ifname);
> +if (!binding)

virNWFilterBindingLookupByPortDev will generate an error when there's no
filter defined for @net (if you're running libvirtd in a debugger you
see it).

Shouldn't the call be guarded by a "if (!net->filter)"?

if (!net->filter ||

Re: [libvirt] [PATCH v3 19/20] nwfilter: wire up new APIs for creating and deleting nwfilter bindings

[John Ferlan]

On 06/14/2018 08:33 AM, Daniel P. Berrangé wrote:
> This allows the virsh commands nwfilter-binding-create and
> nwfilter-binding-delete to be used.
> 
> Note using these commands lets you delete filters that were
> previously created automatically by the virt drivers, or add
> filters for VM nics that were not there before. Generally it
> is expected these new APIs will only be used by virt drivers.
> It is the admin's responsibility to not shoot themselves in
> the foot.

Can't wait to see QE get ahold of this ;-)

> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  src/nwfilter/nwfilter_driver.c | 79 ++
>  1 file changed, 79 insertions(+)
> 

I think with a couple of extra comments as described below, then this
looks fine.  Not sure how the other point regarding calling CreateXML
from the ConfNWFilterInstantiate path (and the reload of load all
configs) will be handled, but I'm sure it'll be something handled in
patch 16 and 20, so the comment below is just the "tracing" I did while
reviewing...

Reviewed-by: John Ferlan 

John

> diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c
> index 6bfb584b09..2b6856a36c 100644
> --- a/src/nwfilter/nwfilter_driver.c
> +++ b/src/nwfilter/nwfilter_driver.c
> @@ -788,6 +788,83 @@ nwfilterBindingGetXMLDesc(virNWFilterBindingPtr binding,
>  }
>  
>  

Because "not everyone" reads the commit message that added this, I think
adding a few comments here and for BindingDelete to essentially indicate
the same as the commit message would be good.

> +static virNWFilterBindingPtr
> +nwfilterBindingCreateXML(virConnectPtr conn,
> + const char *xml,
> + unsigned int flags)
> +{
> +virNWFilterBindingObjPtr obj;
> +virNWFilterBindingDefPtr def;
> +virNWFilterBindingPtr ret = NULL;
> +
> +virCheckFlags(0, NULL);
> +
> +def = virNWFilterBindingDefParseString(xml);
> +if (!def)
> +return NULL;
> +
> +if (virNWFilterBindingCreateXMLEnsureACL(conn, def) < 0)
> +goto cleanup;
> +
> +obj = virNWFilterBindingObjListFindByPortDev(driver->bindings, 
> def->portdevname);
> +if (obj) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("Filter already present for NIC %s"), 
> def->portdevname);

Recall my point from patch 16 regarding the existence of the filter.
>From certain paths it's OK if it exists but once this becomes functional
for the subsequent patch via virDomainConfNWFilterInstantiate, then the
issue from patch 16 moves to patch 20 (e.g. guest not restarting).

> +goto cleanup;
> +}
> +
> +obj = virNWFilterBindingObjListAdd(driver->bindings,
> +   def);
> +if (!obj)
> +goto cleanup;
> +
> +if (!(ret = virGetNWFilterBinding(conn, def->portdevname, def->filter)))
> +goto cleanup;
> +
> +if (virNWFilterInstantiateFilter(driver, def) < 0) {
> +virNWFilterBindingObjListRemove(driver->bindings, obj);
> +virObjectUnref(ret);
> +ret = NULL;
> +goto cleanup;
> +}
> +virNWFilterBindingObjSave(obj, driver->bindingDir);
> +
> + cleanup:
> +if (!obj)
> +virNWFilterBindingDefFree(def);
> +virNWFilterBindingObjEndAPI();
> +
> +return ret;
> +}
> +
> +
> +static int
> +nwfilterBindingDelete(virNWFilterBindingPtr binding)
> +{
> +virNWFilterBindingObjPtr obj;
> +virNWFilterBindingDefPtr def;
> +int ret = -1;
> +

[...]

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

Re: [libvirt] [PATCH v3 17/20] nwfilter: remove virt driver callback layer for rebuilding filters

[John Ferlan]

On 06/14/2018 08:33 AM, Daniel P. Berrangé wrote:
> Now that the nwfilter driver keeps a list of bindings that it has
> created, there is no need for the complex virt driver callbacks. It is
> possible to simply iterate of the list of recorded filter bindings.
> 
> This means that rebuilding filters no longer has to acquire any locks on
> the virDomainObj objects, as they're never touched.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  src/conf/nwfilter_conf.c   | 134 +++-
>  src/conf/nwfilter_conf.h   |  51 +---
>  src/conf/virnwfilterobj.c  |   4 +-
>  src/libvirt_private.syms   |   7 +-
>  src/lxc/lxc_driver.c   |  28 -
>  src/nwfilter/nwfilter_driver.c |  21 ++--
>  src/nwfilter/nwfilter_gentech_driver.c | 167 -
>  src/nwfilter/nwfilter_gentech_driver.h |   4 +-
>  src/qemu/qemu_driver.c |  25 
>  src/uml/uml_driver.c   |  29 -
>  10 files changed, 141 insertions(+), 329 deletions(-)
> 

A diffstat that Jano usually applauds!  Miracles do happen when you
close your eyes and say 3 times "I wish this code would just go away"
;-).  Still this is some of the most "entertaining code" - that now used
to exist!  It seems I can dig up my nwfilter obj/hash code once this is
in...

There's a couple nits below...

Reviewed-by: John Ferlan 

John


> diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c
> index de26a6d034..5bb8a0c2e7 100644
> --- a/src/conf/nwfilter_conf.c
> +++ b/src/conf/nwfilter_conf.c

[...]

> +
> +
> +int
> +virNWFilterTriggerRebuild(void)
> +{
> +return rebuildCallback(rebuildOpaque);

In the better safe than sorry - should we gate on "if
(rebuildCallback)"?  I don't think there's a way into here with it being
NULL, but those are always "famous last words".

>  }
>  
>  

[...]

> diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c
> index 2388e925fc..3b111e3dc7 100644
> --- a/src/nwfilter/nwfilter_driver.c
> +++ b/src/nwfilter/nwfilter_driver.c
> @@ -163,6 +163,14 @@ nwfilterDriverInstallDBusMatches(DBusConnection *sysbus 
> ATTRIBUTE_UNUSED)
>  
>  #endif /* HAVE_FIREWALLD */
>  
> +static int virNWFilterTriggerRebuildImpl(void *opaque)

NIT:

static int
virNWFilterTriggerRebuildImpl(void *opaque)

> +{
> +virNWFilterDriverStatePtr nwdriver = opaque;
> +
> +return virNWFilterBuildAll(nwdriver, true);
> +}
> +
> +

[...]

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

Re: [libvirt] [PATCH v3 16/20] nwfilter: keep track of active filter bindings

[John Ferlan]

On 06/14/2018 08:33 AM, Daniel P. Berrangé wrote:
> Currently the nwfilter driver does not keep any record of what filter
> bindings it has active. This means that when it needs to recreate
> filters, it has to rely on triggering callbacks provided by the virt
> drivers. This introduces a hash table recording the virNWFilterBinding
> objects so the driver has a record of all active filters.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  src/conf/virnwfilterobj.h  |  4 ++
>  src/nwfilter/nwfilter_driver.c | 86 --
>  2 files changed, 65 insertions(+), 25 deletions(-)
> 

[...]

> diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c
> index 7202691646..2388e925fc 100644
> --- a/src/nwfilter/nwfilter_driver.c
> +++ b/src/nwfilter/nwfilter_driver.c
> @@ -38,7 +38,6 @@
>  #include "domain_conf.h"
>  #include "domain_nwfilter.h"
>  #include "nwfilter_driver.h"
> -#include "virnwfilterbindingdef.h"
>  #include "nwfilter_gentech_driver.h"
>  #include "configmake.h"
>  #include "virfile.h"
> @@ -174,7 +173,6 @@ nwfilterStateInitialize(bool privileged,
>  virStateInhibitCallback callback ATTRIBUTE_UNUSED,
>  void *opaque ATTRIBUTE_UNUSED)
>  {

[...]

>  
>  if (virNWFilterObjListLoadAllConfigs(driver->nwfilters, 
> driver->configDir) < 0)
>  goto error;
>  
> +if (virNWFilterBindingObjListLoadAllConfigs(driver->bindings, 
> driver->bindingDir) < 0)
> +goto error;
> +

Because of this... [1]

>  nwfilterDriverUnlock();
>  
>  return 0;
>  

[...]

> @@ -647,13 +656,37 @@ nwfilterInstantiateFilter(const char *vmname,
>const unsigned char *vmuuid,
>virDomainNetDefPtr net)
>  {
> -virNWFilterBindingDefPtr binding;
> +virNWFilterBindingObjPtr obj;
> +virNWFilterBindingDefPtr def;
>  int ret;
>  
> -if (!(binding = virNWFilterBindingDefForNet(vmname, vmuuid, net)))
> +obj = virNWFilterBindingObjListFindByPortDev(driver->bindings, 
> net->ifname);
> +if (obj) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("Filter already present for NIC %s"), net->ifname);

[1]
If I stop at this patch, start a domain with a filter, then restart
libvirtd, then that will cause a guest running with a filter to exit and
not just disappear - as it can be restarted. The error is:

2018-06-18 16:49:07.405+: 3978: error :
nwfilterInstantiateFilter:666 : internal error: Filter already present
for NIC vnet1

Because once I have this patch built/running the
/var/run/libvirt/nwfilter-binding/vnet1.xml exists and get's loaded when
virNWFilterBindingObjListLoadAllConfigs is called at StateInitialize.

I only saw this because I found later in patch 20 the failure comes from
nwfilterBindingCreateXML instead when virDomainConfNWFilterInstantiate
is called. I worked my way back to this point.

Not sure which would be the "best" solution at this point. Should we
wait to do [1] until patch 20 or perhaps just not have an error here.

NB: If the guest was running at a point up through patch 15 then it
won't exit on the first libvirtd restart (since the obj dir doesn't
exist), but the issue shows up on the 2nd restart.

In general the code is fine to me, but just need to handle this one
issue in some way.

John

> +virNWFilterBindingObjEndAPI();
> +return -1;
> +}
> +
> +if (!(def = virNWFilterBindingDefForNet(vmname, vmuuid, net)))
>  return -1;
> -ret = virNWFilterInstantiateFilter(driver, binding);
> -virNWFilterBindingDefFree(binding);

[...]

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

[libvirt] [PATCH 4/4] qemu: Escape commas for qemuBuildGrapicsSPICECommandLine

[Anya Harter]

Add comma escaping for cfg->spiceTLSx509certdir and
graphics->data.spice.rendernode.

Signed-off-by: Anya Harter 
---
 src/qemu/qemu_command.c | 11 ---
 tests/qemuxml2argvdata/name-escape.args |  5 +++--
 tests/qemuxml2argvdata/name-escape.xml  |  1 +
 tests/qemuxml2argvtest.c|  5 +
 4 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index a9a5e200e9..36dccea9b2 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7974,8 +7974,11 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr 
cfg,
 !cfg->spicePassword)
 virBufferAddLit(, "disable-ticketing,");
 
-if (hasSecure)
-virBufferAsprintf(, "x509-dir=%s,", cfg->spiceTLSx509certdir);
+if (hasSecure) {
+virBufferAddLit(, "x509-dir=");
+virQEMUBuildBufferEscapeComma(, cfg->spiceTLSx509certdir);
+virBufferAsprintf(, ",");
+}
 
 switch (graphics->data.spice.defaultMode) {
 case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_SECURE:
@@ -8082,7 +8085,9 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr 
cfg,
 goto error;
 }
 
-virBufferAsprintf(, "rendernode=%s,", 
graphics->data.spice.rendernode);
+virBufferAddLit(, "rendernode=");
+virQEMUBuildBufferEscapeComma(, 
graphics->data.spice.rendernode);
+virBufferAsprintf(, ",");
 }
 }
 
diff --git a/tests/qemuxml2argvdata/name-escape.args 
b/tests/qemuxml2argvdata/name-escape.args
index d3b908a7e6..72ed2e8410 100644
--- a/tests/qemuxml2argvdata/name-escape.args
+++ b/tests/qemuxml2argvdata/name-escape.args
@@ -33,6 +33,7 @@ cert3=cert3,db=/etc/pki/nssdb,,foo,id=smartcard0,bus=ccid0.0 \
 -chardev pipe,id=charchannel0,path=/tmp/guestfwd,,foo \
 -netdev user,guestfwd=tcp:10.0.2.1:4600-chardev:charchannel0,id=user-channel0 \
 -vnc unix:/tmp/lib/domain--1-foo=1,,bar=2/vnc.sock \
--spice unix,addr=/tmp/lib/domain--1-foo=1,,bar=2/spice.sock \
--vga cirrus \
+-spice unix,addr=/tmp/lib/domain--1-foo=1,,bar=2/spice.sock,gl=on,\
+rendernode=/dev/dri/foo,,bar \
+-device cirrus-vga,id=video0,bus=pci.0,addr=0x2 \
 -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
diff --git a/tests/qemuxml2argvdata/name-escape.xml 
b/tests/qemuxml2argvdata/name-escape.xml
index 9ca7be5968..0580de1813 100644
--- a/tests/qemuxml2argvdata/name-escape.xml
+++ b/tests/qemuxml2argvdata/name-escape.xml
@@ -19,6 +19,7 @@
 
 
   
+  
 
 
   
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 7468537c68..ade21f5a10 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -2761,6 +2761,11 @@ mymain(void)
 QEMU_CAPS_DEVICE_CIRRUS_VGA,
 QEMU_CAPS_SPICE,
 QEMU_CAPS_SPICE_UNIX,
+QEMU_CAPS_DEVICE_VIRTIO_GPU,
+QEMU_CAPS_VIRTIO_GPU_VIRGL,
+QEMU_CAPS_SPICE_GL,
+QEMU_CAPS_SPICE_RENDERNODE,
+QEMU_CAPS_DEVICE_VIDEO_PRIMARY,
 QEMU_CAPS_DEVICE_ISA_SERIAL,
 QEMU_CAPS_CHARDEV_FILE_APPEND,
 QEMU_CAPS_CCID_EMULATED);
-- 
2.17.1

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

[libvirt] [PATCH 1/4] qemu: Escape commas for qemuBuildChrChardevStr

[Anya Harter]

Add comma escaping for dev->data.file.path in cases
VIR_DOMAIN_CHR_TYPE_DEV and VIR_DOMAIN_CHR_TYPE_PIPE.

Signed-off-by: Anya Harter 
---
 src/qemu/qemu_command.c | 9 +
 tests/qemuxml2argvdata/name-escape.args | 4 
 tests/qemuxml2argvdata/name-escape.xml  | 7 +++
 tests/qemuxml2argvtest.c| 3 ++-
 4 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index bb956a77f4..b764008949 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4975,9 +4975,10 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager,
 break;
 
 case VIR_DOMAIN_CHR_TYPE_DEV:
-virBufferAsprintf(, "%s,id=%s,path=%s",
+virBufferAsprintf(, "%s,id=%s,path=",
   STRPREFIX(alias, "parallel") ? "parport" : "tty",
-  charAlias, dev->data.file.path);
+  charAlias);
+virQEMUBuildBufferEscapeComma(, dev->data.file.path);
 break;
 
 case VIR_DOMAIN_CHR_TYPE_FILE:
@@ -4997,8 +4998,8 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager,
 break;
 
 case VIR_DOMAIN_CHR_TYPE_PIPE:
-virBufferAsprintf(, "pipe,id=%s,path=%s", charAlias,
-  dev->data.file.path);
+virBufferAsprintf(, "pipe,id=%s,path=", charAlias);
+virQEMUBuildBufferEscapeComma(, dev->data.file.path);
 break;
 
 case VIR_DOMAIN_CHR_TYPE_STDIO:
diff --git a/tests/qemuxml2argvdata/name-escape.args 
b/tests/qemuxml2argvdata/name-escape.args
index 5ff8c03db8..4b03068f95 100644
--- a/tests/qemuxml2argvdata/name-escape.args
+++ b/tests/qemuxml2argvdata/name-escape.args
@@ -23,6 +23,10 @@ bar=2/monitor.sock,server,nowait \
 -no-acpi \
 -boot c \
 -usb \
+-chardev tty,id=charserial0,path=/dev/ttyS2,,foo \
+-device isa-serial,chardev=charserial0,id=serial0 \
+-chardev pipe,id=charchannel0,path=/tmp/guestfwd,,foo \
+-netdev user,guestfwd=tcp:10.0.2.1:4600-chardev:charchannel0,id=user-channel0 \
 -vnc unix:/tmp/lib/domain--1-foo=1,,bar=2/vnc.sock \
 -spice unix,addr=/tmp/lib/domain--1-foo=1,,bar=2/spice.sock \
 -vga cirrus \
diff --git a/tests/qemuxml2argvdata/name-escape.xml 
b/tests/qemuxml2argvdata/name-escape.xml
index 6b93d71798..3f5e1c9829 100644
--- a/tests/qemuxml2argvdata/name-escape.xml
+++ b/tests/qemuxml2argvdata/name-escape.xml
@@ -20,5 +20,12 @@
 
   
 
+
+  
+
+
+  
+  
+
   
 
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index f44cac9fef..c194ff59c9 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -2760,7 +2760,8 @@ mymain(void)
 QEMU_CAPS_NAME_GUEST,
 QEMU_CAPS_DEVICE_CIRRUS_VGA,
 QEMU_CAPS_SPICE,
-QEMU_CAPS_SPICE_UNIX);
+QEMU_CAPS_SPICE_UNIX,
+QEMU_CAPS_DEVICE_ISA_SERIAL);
 DO_TEST("debug-threads", QEMU_CAPS_NAME_DEBUG_THREADS);
 
 DO_TEST("master-key", QEMU_CAPS_OBJECT_SECRET);
-- 
2.17.1

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

[libvirt] [PATCH 2/4] qemu: Escape commas for qemuBuildChrChardevFileStr

[Anya Harter]

Add comma escaping for fileval.

Signed-off-by: Anya Harter 
---
 src/qemu/qemu_command.c | 3 ++-
 tests/qemuxml2argvdata/name-escape.args | 2 ++
 tests/qemuxml2argvdata/name-escape.xml  | 4 
 tests/qemuxml2argvtest.c| 3 ++-
 4 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index b764008949..40e8f8fccf 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4867,7 +4867,8 @@ qemuBuildChrChardevFileStr(virLogManagerPtr logManager,
 virBufferAsprintf(buf, ",%s=%s,%s=on", filearg, fdpath, appendarg);
 VIR_FREE(fdpath);
 } else {
-virBufferAsprintf(buf, ",%s=%s", filearg, fileval);
+virBufferAsprintf(buf, ",%s=", filearg);
+virQEMUBuildBufferEscapeComma(buf, fileval);
 if (appendval != VIR_TRISTATE_SWITCH_ABSENT) {
 virBufferAsprintf(buf, ",%s=%s", appendarg,
   virTristateSwitchTypeToString(appendval));
diff --git a/tests/qemuxml2argvdata/name-escape.args 
b/tests/qemuxml2argvdata/name-escape.args
index 4b03068f95..35a13b2533 100644
--- a/tests/qemuxml2argvdata/name-escape.args
+++ b/tests/qemuxml2argvdata/name-escape.args
@@ -25,6 +25,8 @@ bar=2/monitor.sock,server,nowait \
 -usb \
 -chardev tty,id=charserial0,path=/dev/ttyS2,,foo \
 -device isa-serial,chardev=charserial0,id=serial0 \
+-chardev file,id=charserial1,path=/tmp/serial.log,,foo,append=on \
+-device isa-serial,chardev=charserial1,id=serial1 \
 -chardev pipe,id=charchannel0,path=/tmp/guestfwd,,foo \
 -netdev user,guestfwd=tcp:10.0.2.1:4600-chardev:charchannel0,id=user-channel0 \
 -vnc unix:/tmp/lib/domain--1-foo=1,,bar=2/vnc.sock \
diff --git a/tests/qemuxml2argvdata/name-escape.xml 
b/tests/qemuxml2argvdata/name-escape.xml
index 3f5e1c9829..79c1b34458 100644
--- a/tests/qemuxml2argvdata/name-escape.xml
+++ b/tests/qemuxml2argvdata/name-escape.xml
@@ -23,6 +23,10 @@
 
   
 
+
+  
+  
+
 
   
   
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index c194ff59c9..3e02fa576c 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -2761,7 +2761,8 @@ mymain(void)
 QEMU_CAPS_DEVICE_CIRRUS_VGA,
 QEMU_CAPS_SPICE,
 QEMU_CAPS_SPICE_UNIX,
-QEMU_CAPS_DEVICE_ISA_SERIAL);
+QEMU_CAPS_DEVICE_ISA_SERIAL,
+QEMU_CAPS_CHARDEV_FILE_APPEND);
 DO_TEST("debug-threads", QEMU_CAPS_NAME_DEBUG_THREADS);
 
 DO_TEST("master-key", QEMU_CAPS_OBJECT_SECRET);
-- 
2.17.1

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

[libvirt] [PATCH 0/4] qemu: add comma escaping in qemu_command.c

[Anya Harter]

The function virQEMUBuildBufferEscapeComma is used to escape commas in
user provided fields for qemu command line processing in the four
functions listed below.

A corresponding test for each comma escaping instance has been added to
tests/qemuxml2argvdata/name-escape.xml.

This should complete the BiteSizedTask entry at
https://wiki.libvirt.org/page/BiteSizedTasks#qemu:_Use_comma_escaping_for_more_command_line_values.

Anya Harter (4):
  qemu: Escape commas for qemuBuildChrChardevStr
  qemu: Escape commas for qemuBuildChrChardevFileStr
  qemu: Escape commas for qemuBuildSmartcardCommandLine
  qemu: Escape commas for qemuBuildGrapicsSPICECommandLine

 src/qemu/qemu_command.c | 44 +++--
 tests/qemuxml2argvdata/name-escape.args | 14 ++--
 tests/qemuxml2argvdata/name-escape.xml  | 18 ++
 tests/qemuxml2argvtest.c| 10 +-
 4 files changed, 58 insertions(+), 28 deletions(-)

-- 
2.17.1

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

[libvirt] [PATCH 3/4] qemu: Escape commas for qemuBuildSmartcardCommandLine

[Anya Harter]

Add comma escaping for smartcard->data.cert.file[i] and
smartcard->data.cert.database.

Signed-off-by: Anya Harter 
---
 src/qemu/qemu_command.c | 21 -
 tests/qemuxml2argvdata/name-escape.args |  3 +++
 tests/qemuxml2argvdata/name-escape.xml  |  6 ++
 tests/qemuxml2argvtest.c|  3 ++-
 4 files changed, 15 insertions(+), 18 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 40e8f8fccf..a9a5e200e9 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -8716,29 +8716,16 @@ qemuBuildSmartcardCommandLine(virLogManagerPtr 
logManager,
 
 virBufferAddLit(, "ccid-card-emulated,backend=certificates");
 for (i = 0; i < VIR_DOMAIN_SMARTCARD_NUM_CERTIFICATES; i++) {
-if (strchr(smartcard->data.cert.file[i], ',')) {
-virBufferFreeAndReset();
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("invalid certificate name: %s"),
-   smartcard->data.cert.file[i]);
-return -1;
-}
-virBufferAsprintf(, ",cert%zu=%s", i + 1,
-  smartcard->data.cert.file[i]);
+virBufferAsprintf(, ",cert%zu=", i + 1);
+virQEMUBuildBufferEscapeComma(, smartcard->data.cert.file[i]);
 }
 if (smartcard->data.cert.database) {
-if (strchr(smartcard->data.cert.database, ',')) {
-virBufferFreeAndReset();
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("invalid database name: %s"),
-   smartcard->data.cert.database);
-return -1;
-}
 database = smartcard->data.cert.database;
 } else {
 database = VIR_DOMAIN_SMARTCARD_DEFAULT_DATABASE;
 }
-virBufferAsprintf(, ",db=%s", database);
+virBufferAddLit(, ",db=");
+virQEMUBuildBufferEscapeComma(, database);
 break;
 
 case VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH:
diff --git a/tests/qemuxml2argvdata/name-escape.args 
b/tests/qemuxml2argvdata/name-escape.args
index 35a13b2533..d3b908a7e6 100644
--- a/tests/qemuxml2argvdata/name-escape.args
+++ b/tests/qemuxml2argvdata/name-escape.args
@@ -22,7 +22,10 @@ bar=2/monitor.sock,server,nowait \
 -no-shutdown \
 -no-acpi \
 -boot c \
+-device usb-ccid,id=ccid0,bus=usb.0,port=1 \
 -usb \
+-device ccid-card-emulated,backend=certificates,cert1=cert1,,foo,cert2=cert2,\
+cert3=cert3,db=/etc/pki/nssdb,,foo,id=smartcard0,bus=ccid0.0 \
 -chardev tty,id=charserial0,path=/dev/ttyS2,,foo \
 -device isa-serial,chardev=charserial0,id=serial0 \
 -chardev file,id=charserial1,path=/tmp/serial.log,,foo,append=on \
diff --git a/tests/qemuxml2argvdata/name-escape.xml 
b/tests/qemuxml2argvdata/name-escape.xml
index 79c1b34458..9ca7be5968 100644
--- a/tests/qemuxml2argvdata/name-escape.xml
+++ b/tests/qemuxml2argvdata/name-escape.xml
@@ -31,5 +31,11 @@
   
   
 
+
+  cert1,foo
+  cert2
+  cert3
+  /etc/pki/nssdb,foo
+
   
 
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 3e02fa576c..7468537c68 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -2762,7 +2762,8 @@ mymain(void)
 QEMU_CAPS_SPICE,
 QEMU_CAPS_SPICE_UNIX,
 QEMU_CAPS_DEVICE_ISA_SERIAL,
-QEMU_CAPS_CHARDEV_FILE_APPEND);
+QEMU_CAPS_CHARDEV_FILE_APPEND,
+QEMU_CAPS_CCID_EMULATED);
 DO_TEST("debug-threads", QEMU_CAPS_NAME_DEBUG_THREADS);
 
 DO_TEST("master-key", QEMU_CAPS_OBJECT_SECRET);
-- 
2.17.1

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

Re: [libvirt] [Qemu-devel] [PATCH RFC] hw/pc: set q35 as the default x86 machine

[Michael S. Tsirkin]

On Mon, Jun 18, 2018 at 02:14:31PM -0300, Eduardo Habkost wrote:
> > Sure if someone does that, we'll have no choice, but as long as 'pc' is
> > shipped we shouldn't gratuitously break apps by changing the default.
> 
> Right.  I just want to make sure "omitting the machine-type may
> stop working in the future" is documented somehow.

I still think we should just add links to the qemu binary and
use ARGV to detect the machine type.

qemu-pc-i386
qemu-q35-x86_64

etc.

> -- 
> Eduardo

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

Re: [libvirt] [Qemu-devel] [PATCH RFC] hw/pc: set q35 as the default x86 machine

[Eduardo Habkost]

On Fri, Jun 15, 2018 at 10:03:14AM +0100, Daniel P. Berrangé wrote:
> On Thu, Jun 14, 2018 at 11:50:56PM -0300, Eduardo Habkost wrote:
> > On Thu, Jun 14, 2018 at 09:09:48AM +0100, Daniel P. Berrangé wrote:
> > > On Wed, Jun 13, 2018 at 03:05:08PM -0300, Eduardo Habkost wrote:
> > > > Getting back to this discussion:
> > > > 
> > > > On Tue, Jun 05, 2018 at 09:43:00AM +0100, Daniel P. Berrangé wrote:
> > > > > On Tue, Jun 05, 2018 at 09:27:46AM +0200, Gerd Hoffmann wrote:
> > > > > >   Hi,
> > > > > > 
> > > > > > > >   Add to that shortcuts like -cdrom
> > > > > > > > stop working,
> > > > > > > 
> > > > > > > Maybe is fixable.
> > > > > > 
> > > > > > Already fixed for ages.
> > > > > > 
> > > > > > > I see marking Q35 as the default machine a first step.
> > > > > > 
> > > > > > Maybe the better option is to go the arm route:  Just don't define a
> > > > > > default, so users have to specify pc or q35.  That will make them 
> > > > > > notice
> > > > > > there is a world beside 'pc', and we also avoid breaking things
> > > > > > silently.
> > > > > 
> > > > > If QEMU removes the default, then libvirt will have to hardcode
> > > > > 'pc' as the default to maintain back compatibility, so I don't
> > > > > think that ends up as a net win
> > > > 
> > > > I believe there's consensus that applications blindly relying on
> > > > the default machine-type when creating a domain is a bad idea.
> > > > 
> > > > That said, can we deprecate this feature in libvirt, encourage
> > > > applications to always specify an explicit machine-type, thus
> > > > making it possible to deprecate the i440fx machine-types one day?
> > > 
> > > Well from libvirt's POV this scenario arrives if a mgmt app simply omits
> > > the relevant element/attribute from the XML config. Deprecating something
> > > implies that in future we'd drop support for it, but we're never going
> > > to make this mandatory in libvirt as that would be a regression in
> > > behaviour from libvirt's POV. So I don't think it is something we would
> > > deprecate.
> > 
> > Does libvirt really have an option, here?  I'm sure that sooner
> > or later somebody will distribute QEMU binaries without "pc".
> 
> Sure if someone does that, we'll have no choice, but as long as 'pc' is
> shipped we shouldn't gratuitously break apps by changing the default.

Right.  I just want to make sure "omitting the machine-type may
stop working in the future" is documented somehow.

-- 
Eduardo

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

Re: [libvirt] [PATCH v3 14/20] conf: introduce a virNWFilterBindingObjPtr struct

[John Ferlan]

On 06/14/2018 08:33 AM, Daniel P. Berrangé wrote:
> Introduce a new struct to act as the stateful owner of the
> virNWFilterBindingDefPtr objects.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  src/conf/Makefile.inc.am |   2 +
>  src/conf/virnwfilterbindingobj.c | 299 +++
>  src/conf/virnwfilterbindingobj.h |  69 +++
>  src/libvirt_private.syms |  14 ++
>  4 files changed, 384 insertions(+)
>  create mode 100644 src/conf/virnwfilterbindingobj.c
>  create mode 100644 src/conf/virnwfilterbindingobj.h
> 

While continuing, I tripped across this:

> +
> +static virNWFilterBindingObjPtr
> +virNWFilterBindingObjParseNode(xmlDocPtr doc,
> +   xmlNodePtr root)
> +{
> +xmlXPathContextPtr ctxt = NULL;
> +virNWFilterBindingObjPtr obj = NULL;
> +
> +if (STRNEQ((const char *)root->name, "filterbinding")) {

"filterbindingstatus"

> +virReportError(VIR_ERR_XML_ERROR, "%s",
> +   _("unknown root element for filter binding"));

Found by adding the '%s' here to print the root->name...


> +goto cleanup;
> +}
> +
> +ctxt = xmlXPathNewContext(doc);
> +if (ctxt == NULL) {
> +virReportOOMError();
> +goto cleanup;
> +}
> +
> +ctxt->node = root;
> +obj = virNWFilterBindingObjParseXML(doc, ctxt);
> +
> + cleanup:
> +xmlXPathFreeContext(ctxt);
> +return obj;
> +}
> +
> +

John

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

Re: [libvirt] [PATCH] qemu: Fix qemuMonitorCreateObjectProps

[John Ferlan]

On 06/18/2018 08:35 AM, Ján Tomko wrote:
> The commit message is one long sentence and a bit hard to read.
> 

True, haha so it is... Still better than nothing and assuming the reader
can figure it out ;-)

> On Mon, Jun 18, 2018 at 07:56:35AM -0400, John Ferlan wrote:
>> Commit id f0a23c0c3 introduced qemuMonitorCreateObjectProps
>> to manage wrapping the object name and alias; however, calling
> 
> Listing the commit that broke it can be in a separate paragraph.
> 
>> virJSONValueObjectCreateVArgs and checking a unary ! return
>> status meant when the CreateVAArgs function returned zero, then
>> rather than generating a @propsret, the jump to cleanup would
> 
> Rather than explaing the semantics of C operators, it would be helpful
> to mention that virJSONValueObjectCreateVArgs returns -1 on failure
> and 0 when we did not need to add any arguments.
> 

6 of one... < 0 is just a C semantic check too of a different style. The
unary check for an integer which can be == 0 is just one of those things
that I see a lot in code and I usually point out much to the dismay of a
few people.

> 
>> result in an unexpected failure and cause virsh to issue the
>> "error: An error occurred, but the cause is unknown" error
>> message for something like "virsh iothreadadd $DOM 10" (assuming
>> that IOThread 10 didn't already exist).
> 
> Putting the example error message and virsh commands on separate lines
> would make the commit message easier to read.
> 

I'll alter to:

Fix the return value status comparison checking for call to
virJSONValueObjectCreateVArgs introduced by commit id f0a23c0c3.

If a NULL arglist is passed, then a 0 is returned which is a valid
status and we only should fail when the return is < 0.

This resolves an issue seen for "virsh iothreadadd $dom $iothread" where
a "error: An error occurred, but the cause is unknown" error was
generated when trying to hotplug an IOThread to a domain since
qemuDomainHotplugAddIOThread passes a NULL arglist.



>>
>> Signed-off-by: John Ferlan 
>> ---
>> src/qemu/qemu_monitor.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
>> index b29672d4f1..d6771c1d52 100644
>> --- a/src/qemu/qemu_monitor.c
>> +++ b/src/qemu/qemu_monitor.c
>> @@ -3051,7 +3051,7 @@ qemuMonitorCreateObjectProps(virJSONValuePtr
>> *propsret,
>>
>>     va_start(args, alias);
>>
>> -    if (!(virJSONValueObjectCreateVArgs(, args)))
>> +    if (virJSONValueObjectCreateVArgs(, args) < 0)
> 
> Looks like qemuDomainHotplugAddIOThread is the only user of
> qemuMonitorCreateObjectProps
> passing NULL for args. Can it be tested by our test suite?

Perhaps by anyone that wants to write those tests... maybe the original
author should have done that ;-)!

Thanks for the quick review.

John

> 
> With more newlines in the commit message:
> 
> Reviewed-by: Ján Tomko 
> 
> Jano

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

Re: [libvirt] [PATCH] qemu: Fix qemuMonitorCreateObjectProps

[Ján Tomko]

The commit message is one long sentence and a bit hard to read.

On Mon, Jun 18, 2018 at 07:56:35AM -0400, John Ferlan wrote:

Commit id f0a23c0c3 introduced qemuMonitorCreateObjectProps
to manage wrapping the object name and alias; however, calling


Listing the commit that broke it can be in a separate paragraph.


virJSONValueObjectCreateVArgs and checking a unary ! return
status meant when the CreateVAArgs function returned zero, then
rather than generating a @propsret, the jump to cleanup would


Rather than explaing the semantics of C operators, it would be helpful
to mention that virJSONValueObjectCreateVArgs returns -1 on failure
and 0 when we did not need to add any arguments.



result in an unexpected failure and cause virsh to issue the
"error: An error occurred, but the cause is unknown" error
message for something like "virsh iothreadadd $DOM 10" (assuming
that IOThread 10 didn't already exist).


Putting the example error message and virsh commands on separate lines
would make the commit message easier to read.



Signed-off-by: John Ferlan 
---
src/qemu/qemu_monitor.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index b29672d4f1..d6771c1d52 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -3051,7 +3051,7 @@ qemuMonitorCreateObjectProps(virJSONValuePtr *propsret,

va_start(args, alias);

-if (!(virJSONValueObjectCreateVArgs(, args)))
+if (virJSONValueObjectCreateVArgs(, args) < 0)


Looks like qemuDomainHotplugAddIOThread is the only user of 
qemuMonitorCreateObjectProps
passing NULL for args. Can it be tested by our test suite?

With more newlines in the commit message:

Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [libvirt-go-xml PATCH v2 2/2] Add support for SEV in domain capabilities XML

[Erik Skultety]

On Mon, Jun 18, 2018 at 01:15:54PM +0100, Daniel P. Berrangé wrote:
> On Mon, Jun 18, 2018 at 09:06:00AM +0200, Erik Skultety wrote:
> > Signed-off-by: Erik Skultety 
> > Reviewed-by: Daniel P. Berrangé
>
> Lost my email address here.

Oops, fixed.
Thanks a lot for your help and guidance introducing the bindings.

Erik

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

Re: [libvirt] [dockerfiles PATCH] README: Provide information about the images

[Daniel P . Berrangé]

On Fri, Jun 15, 2018 at 07:03:47PM +0200, Andrea Bolognani wrote:
> Signed-off-by: Andrea Bolognani 
> ---
> Note that this document will be displayed both for the
> repository containing the Dockerfiles
> 
>   https://github.com/libvirt/libvirt-dockerfiles
> 
> and for the images produced by said Dockerfiles, eg.
> 
>   https://hub.docker.com/r/libvirt/buildenv-centos-7/
> 
> so the language is intentionally vague at times. I believe
> I've achieved a decent balance between the needs of the two
> scenarios, but I'm of course totally open to feedback :)
> 
>  README.md | 50 ++
>  1 file changed, 50 insertions(+)

Reviewed-by: Daniel P. Berrangé 


> 
> diff --git a/README.md b/README.md
> index a84caf3..ccd7ad1 100644
> --- a/README.md
> +++ b/README.md
> @@ -1,2 +1,52 @@
>  Docker-based build environments for libvirt
>  ===
> +
> +These images come with all libvirt build dependencies, including
> +optional ones, already installed: this makes it possible to run
> +something like
> +
> +$ docker run \
> +  -v $(pwd):/libvirt \
> +  -w /libvirt \
> +  -it \
> +  buildenv-centos-7
> +
> +from a git clone and start building libvirt right away.
> +
> +Image availability is influenced by libvirt's
> +[platform support policy](https://libvirt.org/platforms.html),
> +with the obvious caveat that non-Linux operating systems can't
> +be run on top of a Linux kernel and as such are not included.
> +
> +
> +Intended use
> +
> +
> +The images are primarily intended for use on
> +[Travis CI](https://travis-ci.org/libvirt/libvirt).
> +
> +The primary CI environment for the libvirt project is hosted on
> +[CentOS CI](https://ci.centos.org/view/libvirt/); however, since
> +that environment feeds off the `master` branch of the various
> +projects, it can only detect issues after the code causing them
> +has already been merged.
> +
> +While testing on Travis CI doesn't cover as many platforms or the
> +interactions between as many components, it can be very useful as
> +a smoke test of sorts that allows developers to catch mistakes
> +before posting patches to the mailing list.
> +
> +As an alternative, images can be used locally without relying on
> +third-party services; in this scenario, the number of platforms
> +patches are tested against is only limited by image availability
> +and hardware resources.
> +
> +
> +Information about build dependencies
> +
> +
> +The list of build dependencies for libvirt (as well as many
> +other virtualization-related projects) is taken from the
> +[libvirt-jenkins-ci](https://libvirt.org/git/?p=libvirt-jenkins-ci.git)
> +repository, which also contains the tooling used to generate
> +Dockerfiles.
> -- 
> 2.17.1
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [libvirt-go-xml PATCH v2 2/2] Add support for SEV in domain capabilities XML

[Daniel P . Berrangé]

On Mon, Jun 18, 2018 at 09:06:00AM +0200, Erik Skultety wrote:
> Signed-off-by: Erik Skultety 
> Reviewed-by: Daniel P. Berrangé

Lost my email address here.

Reviewed-by: Daniel P. Berrangé 


> ---
>  domain_capabilities.go | 7 +++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/domain_capabilities.go b/domain_capabilities.go
> index 3f5a752..0faa06a 100644
> --- a/domain_capabilities.go
> +++ b/domain_capabilities.go
> @@ -106,6 +106,7 @@ type DomainCapsFeatures struct {
>   GIC*DomainCapsFeatureGIC`xml:"gic"`
>   VMCoreInfo *DomainCapsFeatureVMCoreInfo `xml:"vmcoreinfo"`
>   GenID  *DomainCapsFeatureGenID  `xml:"genid"`
> + SEV*DomainCapsFeatureSEV`xml:"sev"`
>  }
>  
>  type DomainCapsFeatureGIC struct {
> @@ -121,6 +122,12 @@ type DomainCapsFeatureGenID struct {
>   Supported string `xml:"supported,attr"`
>  }
>  
> +type DomainCapsFeatureSEV struct {
> + Supported   string `xml:"supported,attr"`
> + CBitPos uint   `xml:"cbitpos,omitempty"`
> + ReducedPhysBits uint   `xml:"reducedPhysBits,omitempty"`
> +}
> +
>  func (c *DomainCaps) Unmarshal(doc string) error {
>   return xml.Unmarshal([]byte(doc), c)
>  }
> -- 
> 2.14.4
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [libvirt-go-xml PATCH v2 1/2] Add support for domain launch security

[Daniel P . Berrangé]

On Mon, Jun 18, 2018 at 09:05:59AM +0200, Erik Skultety wrote:
> Signed-off-by: Erik Skultety 
> ---
>  domain.go | 162 
> +-
>  1 file changed, 161 insertions(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé 


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

[libvirt] [PATCH] qemu: Fix qemuMonitorCreateObjectProps

[John Ferlan]

Commit id f0a23c0c3 introduced qemuMonitorCreateObjectProps
to manage wrapping the object name and alias; however, calling
virJSONValueObjectCreateVArgs and checking a unary ! return
status meant when the CreateVAArgs function returned zero, then
rather than generating a @propsret, the jump to cleanup would
result in an unexpected failure and cause virsh to issue the
"error: An error occurred, but the cause is unknown" error
message for something like "virsh iothreadadd $DOM 10" (assuming
that IOThread 10 didn't already exist).

Signed-off-by: John Ferlan 
---
 src/qemu/qemu_monitor.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index b29672d4f1..d6771c1d52 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -3051,7 +3051,7 @@ qemuMonitorCreateObjectProps(virJSONValuePtr *propsret,
 
 va_start(args, alias);
 
-if (!(virJSONValueObjectCreateVArgs(, args)))
+if (virJSONValueObjectCreateVArgs(, args) < 0)
 goto cleanup;
 
 if (!(*propsret = qemuMonitorCreateObjectPropsWrap(type, alias, )))
-- 
2.14.4

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

Re: [libvirt] [GSoC] Code design for scalar and external types

[Andrea Bolognani]

On Mon, 2018-06-18 at 11:52 +0100, Daniel P. Berrangé wrote:
> On Mon, Jun 18, 2018 at 12:24:39PM +0200, Pavel Hrdina wrote:
> > # define VIR_DEFINE_AUTOCLEAR_FUNC(type, func) \
> > static inline void VIR_AUTOCLEAR_FUNC_NAME(type)(type *_ptr) \
> > { \
> > (func)(_ptr); \
> > }
> 
> For anything where we define the impl of (func) these two macros
> should never be used IMHO. We should just change the signature of
> the real functions so that accept a double pointer straight away,
> and thus not require the wrapper function. Yes, it this will
> require updating existing code, but such a design change is
> beneficial even without the cleanup macros, as it prevents the
> possbility of double frees. I'd suggest we just do this kind of
> change straightaway

Agreed that we want to fix our own free functions.

> > # define VIR_AUTOFREE(type) __attribute__((cleanup(virFree))) type
> 
> I don't see the point in passing "type" in here we could simplify
> this:
> 
>   #define VIR_AUTOFREE __attribute__((cleanup(virFree))
> 
>   VIR_AUTOFREE char *foo = NULL;

Passing the type was suggested earlier to make usage consistent
across all cases, eg.

  VIR_AUTOFREE(char *) str = NULL;
  VIR_AUTOPTR(virDomain) dom = NULL;

and IIRC we ended up agreeing that it was a good idea overall.

> > # define VIR_AUTOPTR(type) \
> > __attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type 
> > VIR_AUTOPTR_TYPE_NAME(type)
> > 
> >   The usage would not require our internal vir*Ptr types and would
> >   be easy to use with external types as well:
> > 
> > VIR_DEFINE_AUTOPTR_FUNC(virBitmap, virBitmapFree);
> > 
> > VIR_AUTOPTR(virBitmap) map = NULL;
> > 
> > VIR_DEFINE_AUTOPTR_FUNC(LIBSSH2_CHANNEL, libssh2_channel_free);
> 
> Contrary to my general point above about VIR_DEFINE_AUTOPTR_FUNC,
> this is a reasonable usage, because we don't control the signature
> of the libssh2_channel_free function.

This is why I suggested we might want to have two sets of
macros, one for types we defined ourselves and one for types
we bring in from external libraries.

> > VIR_AUTOPTR(LIBSSH2_CHANNEL) channel = NULL;
> 
> With my example above
> 
>#define VIR_AUTOFREE_LIBSSH_CHANNEL VIR_AUTOFREE_FUNC(LIBSSH2_CHANNEL)
> 
>VIR_AUTOFREE_LIBSSH_CHANNEL LIBSSH_CHANNEL *chan = NULL;

This makes the declaration needlessly verbose, since you're
repeating the type name twice; Pavel's approach would avoid
that.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

[libvirt] [dbus PATCH] README: Drop warning about lack of API/ABI guarantees

[Andrea Bolognani]

We have promised not to break compatibility after v1.0.0,
which means the warning is no longer accurate and should
be dropped.

Signed-off-by: Andrea Bolognani 
---
 README.md | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/README.md b/README.md
index 1ce265c..66aa6f6 100644
--- a/README.md
+++ b/README.md
@@ -18,9 +18,6 @@ The latest official releases can be found at:
 
   * [https://libvirt.org/sources/dbus/](https://libvirt.org/sources/dbus/)
 
-NB: at this time, libvirt-dbus is *NOT* considered API/ABI stable. Future
-releases may still include API/ABI incompatible changes.
-
 
 Dependencies / supported platforms
 --
-- 
2.17.1

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

Re: [libvirt] [PATCH 2/2] rpc: Fix segmentation fault when no worker pool is available

[Marc Hartmayer]

On Mon, Jun 18, 2018 at 09:47 AM +0200, Erik Skultety  
wrote:
> On Fri, Jun 15, 2018 at 03:31:34PM +0200, Marc Hartmayer wrote:
>> On Fri, Jun 15, 2018 at 01:39 PM +0200, Marc Hartmayer 
>>  wrote:
>> > If srv->workers is a NULL pointer, as it is the case if there are no
>> > workers, then don't try to dereference it.
>> >
>> > Signed-off-by: Marc Hartmayer 
>> > Reviewed-by: Boris Fiuczynski 
>> > ---
>> >  src/rpc/virnetserver.c | 22 +++---
>> >  1 file changed, 15 insertions(+), 7 deletions(-)
>> >
>> > diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
>> > index 5ae809e372..be6f610880 100644
>> > --- a/src/rpc/virnetserver.c
>> > +++ b/src/rpc/virnetserver.c
>> > @@ -933,13 +933,21 @@ virNetServerGetThreadPoolParameters(virNetServerPtr 
>> > srv,
>> >  size_t *jobQueueDepth)
>> >  {
>> >  virObjectLock(srv);
>> > -
>> > -*minWorkers = virThreadPoolGetMinWorkers(srv->workers);
>> > -*maxWorkers = virThreadPoolGetMaxWorkers(srv->workers);
>> > -*freeWorkers = virThreadPoolGetFreeWorkers(srv->workers);
>> > -*nWorkers = virThreadPoolGetCurrentWorkers(srv->workers);
>> > -*nPrioWorkers = virThreadPoolGetPriorityWorkers(srv->workers);
>> > -*jobQueueDepth = virThreadPoolGetJobQueueDepth(srv->workers);
>> > +if (srv->workers) {
>> > +*minWorkers = virThreadPoolGetMinWorkers(srv->workers);
>> > +*maxWorkers = virThreadPoolGetMaxWorkers(srv->workers);
>> > +*freeWorkers = virThreadPoolGetFreeWorkers(srv->workers);
>> > +*nWorkers = virThreadPoolGetCurrentWorkers(srv->workers);
>> > +*nPrioWorkers = virThreadPoolGetPriorityWorkers(srv->workers);
>> > +*jobQueueDepth = virThreadPoolGetJobQueueDepth(srv->workers);
>> > +} else {
>> > +*minWorkers = 0;
>> > +*maxWorkers = 0;
>> > +*freeWorkers = 0;
>> > +*nWorkers = 0;
>> > +*nPrioWorkers = 0;
>> > +*jobQueueDepth = 0;
>> > +}
>> >
>> >  virObjectUnlock(srv);
>> >  return 0;
>> > --
>> > 2.13.6
>>
>> After thinking again it probably makes more sense (and the code more
>> beautiful) to initialize the worker pool even for maxworker=0 (within
>
> I don't understand why should we do that. We don't even initialize it for
> libvirtd server - the implications are clear - you don't have workers, you
> don't get to process a job.
>
>> virNetServerNew) (=> we'll have to adapt virNetServerDispatchNewMessage
>> as well). BTW, there is also a segmentation fault in
>> virThreadPoolSetParameters… And currently it’s not possible to start
>> with maxworkers set to 0 and then increase it via
>
> Do I assume correctly that virNetServerDispatchNewMessage would allocate a new
> worker if there was a request to process but the threadpool was empty? If so, 
> I
> don't see a reason to do that, why would anyone want to run with no workers?
> They don't consume any resources, since they're waiting on a condition.
> However, any segfaults or deadlocks must be fixed, I'll have a look at the
> series as is, unless you've got a compelling reason why it's beneficial to run
> with no workers at all.

Another problem/inconsistency in the current implementation is that if
you start with maxworkers=5 and then set the value to 0 via
virThreadPoolSetParameters (e.g. 'virt-admin server-threadpool-set
--min-workers 0 --max-workers 0 --priority-workers 0 libvirtd')
srv->workers still remains a non NULL value and the “thread pool memory
struct” still remains allocated and virNetServerDispatchNewMessage will
try to request a job… :)

>
> Thanks,
> Erik
>
--
Beste Grüße / Kind regards
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
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

Re: [libvirt] [GSoC] Code design for scalar and external types

[Daniel P . Berrangé]

On Mon, Jun 18, 2018 at 12:24:39PM +0200, Pavel Hrdina wrote:
> Hi,
> 
> It took me longer to sit down and write this mail but here it goes.
> 
> There was a lot of ideas about the macro design and it's easy to
> get lost in all the designs.
> 
> So we agreed on this form:
> 
> 
> # define VIR_AUTOPTR_FUNC_NAME(type) virAutoPtr##type
> # define VIR_AUTOCLEAR_FUNC_NAME(type) virAutoClear##type
> 
> # define VIR_DEFINE_AUTOPTR_FUNC(type, func) \
> static inline void VIR_AUTOPTR_FUNC_NAME(type)(type *_ptr) \
> { \
> (func)(*_ptr); \
> }
> 
> # define VIR_DEFINE_AUTOCLEAR_FUNC(type, func) \
> static inline void VIR_AUTOCLEAR_FUNC_NAME(type)(type *_ptr) \
> { \
> (func)(_ptr); \
> }

For anything where we define the impl of (func) these two macros
should never be used IMHO. We should just change the signature of
the real functions so that accept a double pointer straight away,
and thus not require the wrapper function. Yes, it this will
require updating existing code, but such a design change is
beneficial even without the cleanup macros, as it prevents the
possbility of double frees. I'd suggest we just do this kind of
change straightaway

> # define VIR_AUTOFREE(type) __attribute__((cleanup(virFree))) type

I don't see the point in passing "type" in here we could simplify
this:

  #define VIR_AUTOFREE __attribute__((cleanup(virFree))

  VIR_AUTOFREE char *foo = NULL;

and at the same time fix the char ** problems

  #define VIR_AUTOFREE_FUNC(func) __attribute__((cleanup(func))

  VIR_AUTOFREE_FUNC(virStringListFree) char *foo = NULL;

or if we want to simplify it

  #define VIR_AUTOFREE_STRINGLIST VIR_AUTOFREE_FUNC(virStringListFree)

  VIR_AUTOFREE_STRINGLIST  char **strs = NULL;


This also works for external libraries



> # define VIR_AUTOPTR(type) \
> __attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type type
> 
> # define VIR_AUTOCLEAR(type) \
> __attribute__((cleanup(VIR_AUTOCLEAR_FUNC_NAME(type type
> 
> 
> but it turned out the it's not sufficient for several reasons:
> 
> - there is no simple way ho to free list of strings (char **)
> - this will not work for external types with their own free function
> 
> 
> In order to solve these two issues there were some ideas.
> 
> 
> 1. How to handle list of strings
> 
> We have virStringListFree() function in order to free list of strings,
> the question is how to use it with these macros:
> 
> - Create a new typedef for list of strings and use VIR_AUTOPTR:
> 
> typedef char **virStringList;
> 
> VIR_DEFINE_AUTOPTR_FUNC(virStringList, virStringListFree);
> 
> VIR_AUTOPTR(virStringList) list = NULL;

I don't think we should be creating artifical new typedefs just to
deal with the flawed design of our own autofree macros.

> - Overload VIR_AUTOFREE macro by making it variadic
> 
> # define _VIR_AUTOFREE_0(type) __attribute__((cleanup(virFree))) type
> # define _VIR_AUTOFREE_1(type, func) __attribute__((cleanup(func))) type
> 
> # define _VIR_AUTOFREE_OVERLOADER(_1, _2, NAME, ...) NAME
> # define VIR_AUTOFREE(...) _VIR_AUTOFREE_OVERLOADER(__VA_ARGS__, 
> _VIR_AUTOFREE_1, _VIR_AUTOFREE_0)(__VA_ARGS__)

This is uneccessarily black magic - just have VIR_AUTOFREE and
VIR_AUTOFREE_FUNC defined separately.

> void virStringListPtrFree(char ***stringsp)
> {
> virStringListFree(*stringsp);
> }

As above, we should just fixed virStringListFree to have the better
signature straight away.

> VIR_AUTOFREE(char **, virStringListPtrFree) list = NULL;
> 
> 
> 2. External types with free function
> 
> Libvirt uses a lot of external libraries where we use the types and
> relevant free functions.  The issue with original design is that it was
> counting on the fact that we would have vir*Ptr typedefs, but that's not
> the case for external types.
> 
> - We can modify the design to be closer to GLib design which would
>   work even with external types and would be safe in case external
>   free functions will not behave correctly.  These are to
>   modification to the original design:
> 
> # define VIR_AUTOPTR_TYPE_NAME(type) ##typeAutoPtr
> 
> # define VIR_DEFINE_AUTOPTR_FUNC(type, func) \
> typedef type *VIR_AUTOPTR_TYPE_NAME(type);
> static inline void VIR_AUTOPTR_FUNC_NAME(type)(type **_ptr) \
> { \
> if (*_ptr) { \
> (func)(*_ptr); \
> *_ptr = NULL; \
> } \
> }
> 
> # define VIR_AUTOPTR(type) \
> __attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type 
> VIR_AUTOPTR_TYPE_NAME(type)
> 
>   The usage would not require our internal vir*Ptr types and would
>   be easy to use with external types as well:
> 
> VIR_DEFINE_AUTOPTR_FUNC(virBitmap, virBitmapFree);
> 
> VIR_AUTOPTR(virBitmap) map = NULL;
> 
> VIR_DEFINE_AUTOPTR_FUNC(LIBSSH2_CHANNEL, libssh2_channel_free);

Contrary to my general point above about VIR_DEFINE_AUTOPTR_FUNC,
this is a reasonable usage, because we don't control the signature
of the libssh2_channel_free 

Re: [libvirt] [PATCH 2/2] rpc: Fix segmentation fault when no worker pool is available

[Marc Hartmayer]

On Mon, Jun 18, 2018 at 09:47 AM +0200, Erik Skultety  
wrote:
> On Fri, Jun 15, 2018 at 03:31:34PM +0200, Marc Hartmayer wrote:
>> On Fri, Jun 15, 2018 at 01:39 PM +0200, Marc Hartmayer 
>>  wrote:
>> > If srv->workers is a NULL pointer, as it is the case if there are no
>> > workers, then don't try to dereference it.
>> >
>> > Signed-off-by: Marc Hartmayer 
>> > Reviewed-by: Boris Fiuczynski 
>> > ---
>> >  src/rpc/virnetserver.c | 22 +++---
>> >  1 file changed, 15 insertions(+), 7 deletions(-)
>> >
>> > diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
>> > index 5ae809e372..be6f610880 100644
>> > --- a/src/rpc/virnetserver.c
>> > +++ b/src/rpc/virnetserver.c
>> > @@ -933,13 +933,21 @@ virNetServerGetThreadPoolParameters(virNetServerPtr 
>> > srv,
>> >  size_t *jobQueueDepth)
>> >  {
>> >  virObjectLock(srv);
>> > -
>> > -*minWorkers = virThreadPoolGetMinWorkers(srv->workers);
>> > -*maxWorkers = virThreadPoolGetMaxWorkers(srv->workers);
>> > -*freeWorkers = virThreadPoolGetFreeWorkers(srv->workers);
>> > -*nWorkers = virThreadPoolGetCurrentWorkers(srv->workers);
>> > -*nPrioWorkers = virThreadPoolGetPriorityWorkers(srv->workers);
>> > -*jobQueueDepth = virThreadPoolGetJobQueueDepth(srv->workers);
>> > +if (srv->workers) {
>> > +*minWorkers = virThreadPoolGetMinWorkers(srv->workers);
>> > +*maxWorkers = virThreadPoolGetMaxWorkers(srv->workers);
>> > +*freeWorkers = virThreadPoolGetFreeWorkers(srv->workers);
>> > +*nWorkers = virThreadPoolGetCurrentWorkers(srv->workers);
>> > +*nPrioWorkers = virThreadPoolGetPriorityWorkers(srv->workers);
>> > +*jobQueueDepth = virThreadPoolGetJobQueueDepth(srv->workers);
>> > +} else {
>> > +*minWorkers = 0;
>> > +*maxWorkers = 0;
>> > +*freeWorkers = 0;
>> > +*nWorkers = 0;
>> > +*nPrioWorkers = 0;
>> > +*jobQueueDepth = 0;
>> > +}
>> >
>> >  virObjectUnlock(srv);
>> >  return 0;
>> > --
>> > 2.13.6
>>
>> After thinking again it probably makes more sense (and the code more
>> beautiful) to initialize the worker pool even for maxworker=0 (within
>
> I don't understand why should we do that.

Because right now there are several functionalities broken. Segmentation
faults in virNetServerGet/SetThreadPoolParameters, it’s not possible to
start with maxworkers=0 and then change it at runtime via
virNetServerSetThreadPoolParameters. Sure we can fix all these problems,
but then we’ve to introduce new code paths. Why can’t we just call
virThreadPoolNew(0, 0 , …)? This will only initialize the memory
structure of the pool and _no_ threads. The only change we’ve to do then
is to change the condition 'if (srv->workers)' of
'virNetServerDispatchNewMessage' to something like 'if
(virThreadPoolGetMaxWorkers(srv->workers) > 0)'.

> We don't even initialize it for
> libvirtd server - the implications are clear - you don't have workers, you
> don't get to process a job.

Yep. I don’t want to change that behavior either.

>
>> virNetServerNew) (=> we'll have to adapt virNetServerDispatchNewMessage
>> as well). BTW, there is also a segmentation fault in
>> virThreadPoolSetParameters… And currently it’s not possible to start
>> with maxworkers set to 0 and then increase it via
>
> Do I assume correctly that virNetServerDispatchNewMessage would allocate a new
> worker if there was a request to process but the threadpool was empty?

Do you mean with my proposed changes? Or without?

> If so, I
> don't see a reason to do that, why would anyone want to run with no
> workers?

Commit dff6d809fb6c851244ea07afd07f580d7320cc7a which actually
introduces this feature says:

“Allow RPC server to run single threaded

Refactor the RPC server dispatcher code so that if 'max_workers==0' the
entire server will run single threaded. This is useful for use cases
where there will only ever be 1 client connected which serializes its
requests”.

> They don't consume any resources, since they're waiting on a condition.
> However, any segfaults or deadlocks must be fixed, I'll have a look at the
> series as is, unless you've got a compelling reason why it's beneficial to run
> with no workers at all.

See the commit message above.

>
> Thanks,
> Erik
>

Thank you for looking at it.

--
Beste Grüße / Kind regards
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
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

Re: [libvirt] [GSoC] Code design for scalar and external types

[Pavel Hrdina]

Hi,

It took me longer to sit down and write this mail but here it goes.

There was a lot of ideas about the macro design and it's easy to
get lost in all the designs.

So we agreed on this form:


# define VIR_AUTOPTR_FUNC_NAME(type) virAutoPtr##type
# define VIR_AUTOCLEAR_FUNC_NAME(type) virAutoClear##type

# define VIR_DEFINE_AUTOPTR_FUNC(type, func) \
static inline void VIR_AUTOPTR_FUNC_NAME(type)(type *_ptr) \
{ \
(func)(*_ptr); \
}

# define VIR_DEFINE_AUTOCLEAR_FUNC(type, func) \
static inline void VIR_AUTOCLEAR_FUNC_NAME(type)(type *_ptr) \
{ \
(func)(_ptr); \
}

# define VIR_AUTOFREE(type) __attribute__((cleanup(virFree))) type

# define VIR_AUTOPTR(type) \
__attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type type

# define VIR_AUTOCLEAR(type) \
__attribute__((cleanup(VIR_AUTOCLEAR_FUNC_NAME(type type


but it turned out the it's not sufficient for several reasons:

- there is no simple way ho to free list of strings (char **)
- this will not work for external types with their own free function


In order to solve these two issues there were some ideas.


1. How to handle list of strings

We have virStringListFree() function in order to free list of strings,
the question is how to use it with these macros:

- Create a new typedef for list of strings and use VIR_AUTOPTR:

typedef char **virStringList;

VIR_DEFINE_AUTOPTR_FUNC(virStringList, virStringListFree);

VIR_AUTOPTR(virStringList) list = NULL;


  The benefit of this solution is that it works with current design
  and we only need to create single typedef.

- Overload VIR_AUTOFREE macro by making it variadic

# define _VIR_AUTOFREE_0(type) __attribute__((cleanup(virFree))) type
# define _VIR_AUTOFREE_1(type, func) __attribute__((cleanup(func))) type

# define _VIR_AUTOFREE_OVERLOADER(_1, _2, NAME, ...) NAME
# define VIR_AUTOFREE(...) _VIR_AUTOFREE_OVERLOADER(__VA_ARGS__, 
_VIR_AUTOFREE_1, _VIR_AUTOFREE_0)(__VA_ARGS__)

  This would allow us to use any existing function directly with
  attribute cleanup.  However, the function must take as argument
  pointer to the specified type so we would have to create a wrapper
  functions which would be used in the VIR_AUTOFREE macro, for
  example:

void virStringListPtrFree(char ***stringsp)
{
virStringListFree(*stringsp);
}

VIR_AUTOFREE(char **, virStringListPtrFree) list = NULL;


2. External types with free function

Libvirt uses a lot of external libraries where we use the types and
relevant free functions.  The issue with original design is that it was
counting on the fact that we would have vir*Ptr typedefs, but that's not
the case for external types.

- We can modify the design to be closer to GLib design which would
  work even with external types and would be safe in case external
  free functions will not behave correctly.  These are to
  modification to the original design:

# define VIR_AUTOPTR_TYPE_NAME(type) ##typeAutoPtr

# define VIR_DEFINE_AUTOPTR_FUNC(type, func) \
typedef type *VIR_AUTOPTR_TYPE_NAME(type);
static inline void VIR_AUTOPTR_FUNC_NAME(type)(type **_ptr) \
{ \
if (*_ptr) { \
(func)(*_ptr); \
*_ptr = NULL; \
} \
}

# define VIR_AUTOPTR(type) \
__attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type 
VIR_AUTOPTR_TYPE_NAME(type)

  The usage would not require our internal vir*Ptr types and would
  be easy to use with external types as well:

VIR_DEFINE_AUTOPTR_FUNC(virBitmap, virBitmapFree);

VIR_AUTOPTR(virBitmap) map = NULL;

VIR_DEFINE_AUTOPTR_FUNC(LIBSSH2_CHANNEL, libssh2_channel_free);

VIR_AUTOPTR(LIBSSH2_CHANNEL) channel = NULL;

  There was one suggestion to make another set of macros for the
  external types or just ignore them because there are only few
  places which uses external free functions.

Pavel


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

[libvirt] [PATCH v2 3/3] qemu: Switch code to use new agent job APIs

[Michal Privoznik]

There are two sets of functions here:
1) some functions talk on both monitor and agent monitor,
2) some functions only talk on agent monitor.

For functions from set 1) we need to use
qemuDomainObjBeginJobWithAgent() and for functions from set 2) we
need to use qemuDomainObjBeginAgentJob() only.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_driver.c | 91 --
 1 file changed, 58 insertions(+), 33 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 7fe86d4d2e..5191a924a3 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1954,6 +1954,7 @@ static int qemuDomainShutdownFlags(virDomainPtr dom, 
unsigned int flags)
 bool useAgent = false, agentRequested, acpiRequested;
 bool isReboot = false;
 bool agentForced;
+bool agentJob = false;
 int agentFlag = QEMU_AGENT_SHUTDOWN_POWERDOWN;
 
 virCheckFlags(VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN |
@@ -1980,9 +1981,14 @@ static int qemuDomainShutdownFlags(virDomainPtr dom, 
unsigned int flags)
 if (virDomainShutdownFlagsEnsureACL(dom->conn, vm->def, flags) < 0)
 goto cleanup;
 
-if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
+if ((!useAgent && qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) 
||
+(useAgent && qemuDomainObjBeginJobWithAgent(driver, vm,
+QEMU_JOB_MODIFY,
+QEMU_AGENT_JOB_MODIFY) < 
0))
 goto cleanup;
 
+agentJob = useAgent;
+
 if (virDomainObjGetState(vm, NULL) != VIR_DOMAIN_RUNNING) {
 virReportError(VIR_ERR_OPERATION_INVALID,
"%s", _("domain is not running"));
@@ -2026,7 +2032,10 @@ static int qemuDomainShutdownFlags(virDomainPtr dom, 
unsigned int flags)
 }
 
  endjob:
-qemuDomainObjEndJob(driver, vm);
+if (agentJob)
+qemuDomainObjEndJobWithAgent(driver, vm);
+else
+qemuDomainObjEndJob(driver, vm);
 
  cleanup:
 virDomainObjEndAPI();
@@ -2049,6 +2058,7 @@ qemuDomainReboot(virDomainPtr dom, unsigned int flags)
 bool useAgent = false, agentRequested, acpiRequested;
 bool isReboot = true;
 bool agentForced;
+bool agentJob = false;
 int agentFlag = QEMU_AGENT_SHUTDOWN_REBOOT;
 
 virCheckFlags(VIR_DOMAIN_REBOOT_ACPI_POWER_BTN |
@@ -2075,9 +2085,14 @@ qemuDomainReboot(virDomainPtr dom, unsigned int flags)
 if (virDomainRebootEnsureACL(dom->conn, vm->def, flags) < 0)
 goto cleanup;
 
-if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
+if ((!useAgent && qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) 
||
+(useAgent && qemuDomainObjBeginJobWithAgent(driver, vm,
+QEMU_JOB_MODIFY,
+QEMU_AGENT_JOB_MODIFY) < 
0))
 goto cleanup;
 
+agentJob = useAgent;
+
 agentForced = agentRequested && !acpiRequested;
 if (!qemuDomainAgentAvailable(vm, agentForced)) {
 if (agentForced)
@@ -2115,7 +2130,10 @@ qemuDomainReboot(virDomainPtr dom, unsigned int flags)
 }
 
  endjob:
-qemuDomainObjEndJob(driver, vm);
+if (agentJob)
+qemuDomainObjEndJobWithAgent(driver, vm);
+else
+qemuDomainObjEndJob(driver, vm);
 
  cleanup:
 virDomainObjEndAPI();
@@ -4949,6 +4967,7 @@ qemuDomainSetVcpusFlags(virDomainPtr dom,
 virDomainDefPtr def;
 virDomainDefPtr persistentDef;
 bool hotpluggable = !!(flags & VIR_DOMAIN_VCPU_HOTPLUGGABLE);
+bool useAgent = !!(flags & VIR_DOMAIN_VCPU_GUEST);
 int ret = -1;
 
 virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
@@ -4963,13 +4982,14 @@ qemuDomainSetVcpusFlags(virDomainPtr dom,
 if (virDomainSetVcpusFlagsEnsureACL(dom->conn, vm->def, flags) < 0)
 goto cleanup;
 
-if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
+if ((!useAgent && qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) 
||
+(useAgent && qemuDomainObjBeginAgentJob(driver, vm, 
QEMU_AGENT_JOB_MODIFY) < 0))
 goto cleanup;
 
 if (virDomainObjGetDefs(vm, flags, , ) < 0)
 goto endjob;
 
-if (flags & VIR_DOMAIN_VCPU_GUEST)
+if (useAgent)
 ret = qemuDomainSetVcpusAgent(vm, nvcpus);
 else if (flags & VIR_DOMAIN_VCPU_MAXIMUM)
 ret = qemuDomainSetVcpusMax(driver, def, persistentDef, nvcpus);
@@ -4978,7 +4998,10 @@ qemuDomainSetVcpusFlags(virDomainPtr dom,
  nvcpus, hotpluggable);
 
  endjob:
-qemuDomainObjEndJob(driver, vm);
+if (useAgent)
+qemuDomainObjEndAgentJob(vm);
+else
+qemuDomainObjEndJob(driver, vm);
 
  cleanup:
 virDomainObjEndAPI();
@@ -5429,7 +5452,7 @@ qemuDomainGetVcpusFlags(virDomainPtr dom, unsigned int 
flags)
 goto cleanup;
 
 if (flags & VIR_DOMAIN_VCPU_GUEST) {
-if (qemuDomainObjBeginJob(driver, vm, 

[libvirt] [PATCH v2 0/3] qemu: Allow concurrent access to monitor and guest agent

[Michal Privoznik]

v2 of:

https://www.redhat.com/archives/libvir-list/2018-June/msg00700.html

diff to v1:
- extended THREADS.txt documentation
- added more comments
- merged 3/4 to 1/4 in original patch set
(I think that's everything that was pointed out in John's review)

Michal Privoznik (3):
  qemu: Introduce qemuDomainAgentJob
  qemu: Introduce APIs for manipulating qemuDomainAgentJob
  qemu: Switch code to use new agent job APIs

 src/qemu/THREADS.txt   | 112 ++---
 src/qemu/qemu_domain.c | 191 -
 src/qemu/qemu_domain.h |  30 
 src/qemu/qemu_driver.c |  91 ++-
 4 files changed, 345 insertions(+), 79 deletions(-)

-- 
2.16.4

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

[libvirt] [PATCH v2 2/3] qemu: Introduce APIs for manipulating qemuDomainAgentJob

[Michal Privoznik]

The point is to break QEMU_JOB_* into smaller pieces which
enables us to achieve higher throughput. For instance, if there
are two threads, one is trying to query something on qemu
monitor while the other is trying to query something on agent
monitor these two threads would serialize. There is not much
reason for that.

Signed-off-by: Michal Privoznik 
---
 src/qemu/THREADS.txt   | 112 +++---
 src/qemu/qemu_domain.c | 185 +++--
 src/qemu/qemu_domain.h |  12 
 3 files changed, 263 insertions(+), 46 deletions(-)

diff --git a/src/qemu/THREADS.txt b/src/qemu/THREADS.txt
index 7243161fe3..d17f3f4e0c 100644
--- a/src/qemu/THREADS.txt
+++ b/src/qemu/THREADS.txt
@@ -51,8 +51,8 @@ There are a number of locks on various objects
 Since virDomainObjPtr lock must not be held during sleeps, the job
 conditions provide additional protection for code making updates.
 
-QEMU driver uses two kinds of job conditions: asynchronous and
-normal.
+QEMU driver uses three kinds of job conditions: asynchronous, agent
+and normal.
 
 Asynchronous job condition is used for long running jobs (such as
 migration) that consist of several monitor commands and it is
@@ -69,16 +69,23 @@ There are a number of locks on various objects
 it needs to wait until the asynchronous job ends and try to acquire
 the job again.
 
+Agent job condition is then used when thread wishes to talk to qemu
+agent monitor. It is possible to acquire just agent job
+(qemuDomainObjBeginAgentJob), or only normal job
+(qemuDomainObjBeginJob) or both at the same time
+(qemuDomainObjBeginJobWithAgent). Which type of job to grab depends
+whether caller wishes to communicate only with agent socket, or only
+with qemu monitor socket or both, respectively.
+
 Immediately after acquiring the virDomainObjPtr lock, any method
-which intends to update state must acquire either asynchronous or
-normal job condition.  The virDomainObjPtr lock is released while
-blocking on these condition variables.  Once the job condition is
-acquired, a method can safely release the virDomainObjPtr lock
-whenever it hits a piece of code which may sleep/wait, and
-re-acquire it after the sleep/wait.  Whenever an asynchronous job
-wants to talk to the monitor, it needs to acquire nested job (a
-special kind of normal job) to obtain exclusive access to the
-monitor.
+which intends to update state must acquire asynchronous, normal or
+agent job . The virDomainObjPtr lock is released while blocking on
+these condition variables.  Once the job condition is acquired, a
+method can safely release the virDomainObjPtr lock whenever it hits
+a piece of code which may sleep/wait, and re-acquire it after the
+sleep/wait.  Whenever an asynchronous job wants to talk to the
+monitor, it needs to acquire nested job (a special kind of normal
+job) to obtain exclusive access to the monitor.
 
 Since the virDomainObjPtr lock was dropped while waiting for the
 job condition, it is possible that the domain is no longer active
@@ -132,6 +139,30 @@ To acquire the normal job condition
 
 
 
+To acquire the agent job condition
+
+  qemuDomainObjBeginAgentJob()
+- Waits until there is no other agent job set
+- Sets job.agentActive tp the job type
+
+  qemuDomainObjEndAgentJob()
+- Sets job.agentActive to 0
+- Signals on job.cond condition
+
+
+
+To acquire both normal and agent job condition
+
+  qemuDomainObjBeginJobWithAgent()
+- Waits until there is no normal and no agent job set
+- Sets both job.active and job.agentActive with required job types
+
+  qemuDomainObjEndJobWithAgent()
+- Sets both job.active and job.agentActive to 0
+- Signals on job.cond condition
+
+
+
 To acquire the asynchronous job condition
 
   qemuDomainObjBeginAsyncJob()
@@ -247,6 +278,65 @@ Design patterns
  virDomainObjEndAPI();
 
 
+ * Invoking an agent command on a virDomainObjPtr
+
+ virDomainObjPtr obj;
+ qemuAgentPtr agent;
+
+ obj = qemuDomObjFromDomain(dom);
+
+ qemuDomainObjBeginAgentJob(obj, QEMU_AGENT_JOB_TYPE);
+
+ ...do prep work...
+
+ if (!qemuDomainAgentAvailable(obj, true))
+ goto cleanup;
+
+ agent = qemuDomainObjEnterAgent(obj);
+ qemuAgent(agent, ..);
+ qemuDomainObjExitAgent(obj, agent);
+
+ ...do final work...
+
+ qemuDomainObjEndAgentJob(obj);
+ virDomainObjEndAPI();
+
+
+ * Invoking both monitor and agent commands on a virDomainObjPtr
+
+ virDomainObjPtr obj;
+ qemuAgentPtr agent;
+
+ obj = qemuDomObjFromDomain(dom);
+
+ qemuDomainObjBeginJobWithAgent(obj, QEMU_JOB_TYPE, QEMU_AGENT_JOB_TYPE);
+
+ if (!virDomainObjIsActive(dom))
+ goto cleanup;
+
+ ...do prep work...
+
+ if (!qemuDomainAgentAvailable(obj, true))
+ goto cleanup;
+
+ agent = 

[libvirt] [PATCH v2 1/3] qemu: Introduce qemuDomainAgentJob

[Michal Privoznik]

Introduce guest agent specific job categories to allow threads to
run agent monitor specific jobs while normal monitor jobs can
also be running.

Alter _qemuDomainJobObj in order to duplicate certain fields that
will be used for guest agent specific tasks to increase
concurrency and throughput and reduce serialization.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_domain.c |  6 ++
 src/qemu/qemu_domain.h | 18 ++
 2 files changed, 24 insertions(+)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 2119233907..a12905436e 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -93,6 +93,12 @@ VIR_ENUM_IMPL(qemuDomainJob, QEMU_JOB_LAST,
   "async nested",
 );
 
+VIR_ENUM_IMPL(qemuDomainAgentJob, QEMU_AGENT_JOB_LAST,
+  "none",
+  "query",
+  "modify",
+);
+
 VIR_ENUM_IMPL(qemuDomainAsyncJob, QEMU_ASYNC_JOB_LAST,
   "none",
   "migration out",
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index fd8d9b5305..603a8a39e3 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -82,6 +82,15 @@ typedef enum {
 } qemuDomainJob;
 VIR_ENUM_DECL(qemuDomainJob)
 
+typedef enum {
+QEMU_AGENT_JOB_NONE = 0,/* No agent job. */
+QEMU_AGENT_JOB_QUERY,   /* Does not change state of domain */
+QEMU_AGENT_JOB_MODIFY,  /* May change state of domain */
+
+QEMU_AGENT_JOB_LAST
+} qemuDomainAgentJob;
+VIR_ENUM_DECL(qemuDomainAgentJob)
+
 /* Async job consists of a series of jobs that may change state. Independent
  * jobs that do not change state (and possibly others if explicitly allowed by
  * current async job) are allowed to be run even if async job is active.
@@ -158,11 +167,20 @@ typedef struct _qemuDomainJobObj qemuDomainJobObj;
 typedef qemuDomainJobObj *qemuDomainJobObjPtr;
 struct _qemuDomainJobObj {
 virCond cond;   /* Use to coordinate jobs */
+
+/* The following members are for QEMU_JOB_* */
 qemuDomainJob active;   /* Currently running job */
 unsigned long long owner;   /* Thread id which set current job */
 const char *ownerAPI;   /* The API which owns the job */
 unsigned long long started; /* When the current job started */
 
+/* The following members are for QEMU_AGENT_JOB_* */
+qemuDomainAgentJob agentActive; /* Currently running agent job */
+unsigned long long agentOwner;  /* Thread id which set current agent 
job */
+const char *agentOwnerAPI;  /* The API which owns the agent job */
+unsigned long long agentStarted;/* When the current agent job started 
*/
+
+/* The following members are for QEMU_ASYNC_JOB_* */
 virCond asyncCond;  /* Use to coordinate with async jobs */
 qemuDomainAsyncJob asyncJob;/* Currently active async job */
 unsigned long long asyncOwner;  /* Thread which set current async job 
*/
-- 
2.16.4

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

Re: [libvirt] [PATCH 2/2] rpc: Fix segmentation fault when no worker pool is available

[Erik Skultety]

On Fri, Jun 15, 2018 at 03:31:34PM +0200, Marc Hartmayer wrote:
> On Fri, Jun 15, 2018 at 01:39 PM +0200, Marc Hartmayer 
>  wrote:
> > If srv->workers is a NULL pointer, as it is the case if there are no
> > workers, then don't try to dereference it.
> >
> > Signed-off-by: Marc Hartmayer 
> > Reviewed-by: Boris Fiuczynski 
> > ---
> >  src/rpc/virnetserver.c | 22 +++---
> >  1 file changed, 15 insertions(+), 7 deletions(-)
> >
> > diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
> > index 5ae809e372..be6f610880 100644
> > --- a/src/rpc/virnetserver.c
> > +++ b/src/rpc/virnetserver.c
> > @@ -933,13 +933,21 @@ virNetServerGetThreadPoolParameters(virNetServerPtr 
> > srv,
> >  size_t *jobQueueDepth)
> >  {
> >  virObjectLock(srv);
> > -
> > -*minWorkers = virThreadPoolGetMinWorkers(srv->workers);
> > -*maxWorkers = virThreadPoolGetMaxWorkers(srv->workers);
> > -*freeWorkers = virThreadPoolGetFreeWorkers(srv->workers);
> > -*nWorkers = virThreadPoolGetCurrentWorkers(srv->workers);
> > -*nPrioWorkers = virThreadPoolGetPriorityWorkers(srv->workers);
> > -*jobQueueDepth = virThreadPoolGetJobQueueDepth(srv->workers);
> > +if (srv->workers) {
> > +*minWorkers = virThreadPoolGetMinWorkers(srv->workers);
> > +*maxWorkers = virThreadPoolGetMaxWorkers(srv->workers);
> > +*freeWorkers = virThreadPoolGetFreeWorkers(srv->workers);
> > +*nWorkers = virThreadPoolGetCurrentWorkers(srv->workers);
> > +*nPrioWorkers = virThreadPoolGetPriorityWorkers(srv->workers);
> > +*jobQueueDepth = virThreadPoolGetJobQueueDepth(srv->workers);
> > +} else {
> > +*minWorkers = 0;
> > +*maxWorkers = 0;
> > +*freeWorkers = 0;
> > +*nWorkers = 0;
> > +*nPrioWorkers = 0;
> > +*jobQueueDepth = 0;
> > +}
> >
> >  virObjectUnlock(srv);
> >  return 0;
> > --
> > 2.13.6
>
> After thinking again it probably makes more sense (and the code more
> beautiful) to initialize the worker pool even for maxworker=0 (within

I don't understand why should we do that. We don't even initialize it for
libvirtd server - the implications are clear - you don't have workers, you
don't get to process a job.

> virNetServerNew) (=> we'll have to adapt virNetServerDispatchNewMessage
> as well). BTW, there is also a segmentation fault in
> virThreadPoolSetParameters… And currently it’s not possible to start
> with maxworkers set to 0 and then increase it via

Do I assume correctly that virNetServerDispatchNewMessage would allocate a new
worker if there was a request to process but the threadpool was empty? If so, I
don't see a reason to do that, why would anyone want to run with no workers?
They don't consume any resources, since they're waiting on a condition.
However, any segfaults or deadlocks must be fixed, I'll have a look at the
series as is, unless you've got a compelling reason why it's beneficial to run
with no workers at all.

Thanks,
Erik

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

[libvirt] [PATCH 2/2] qemu: sev: Don't jump to endjob if SEV measurement retrieval fails

[Erik Skultety]

If measurement retrieval fails we'd forget to call ExitMonitor to unlock
the monitor.

Signed-off-by: Erik Skultety 
Reported-by: Luyao Huang 
---
 src/qemu/qemu_driver.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index fd25eb1b0b..d71956988f 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -21513,10 +21513,8 @@ qemuDomainGetSEVMeasurement(virQEMUDriverPtr driver,
 
 qemuDomainObjEnterMonitor(driver, vm);
 tmp = qemuMonitorGetSEVMeasurement(QEMU_DOMAIN_PRIVATE(vm)->mon);
-if (tmp == NULL)
-goto endjob;
 
-if (qemuDomainObjExitMonitor(driver, vm) < 0)
+if (qemuDomainObjExitMonitor(driver, vm) < 0 || !tmp)
 goto endjob;
 
 if (virTypedParamsAddString(params, nparams, ,
-- 
2.14.4

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

[libvirt] [PATCH 1/2] qemu: sev: Use EnterMonitor instead of EnterMonitorAsync

[Erik Skultety]

Since it's being called with QEMU_ASYNC_JOB_NONE which is what
qemuDomainObjEnterMonitor is going to use with the internal helper,
let's use that one instead.

Signed-off-by: Erik Skultety 
---
 src/qemu/qemu_driver.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 42069ee617..fd25eb1b0b 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -21511,9 +21511,7 @@ qemuDomainGetSEVMeasurement(virQEMUDriverPtr driver,
 if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
 return -1;
 
-if (qemuDomainObjEnterMonitorAsync(driver, vm, QEMU_ASYNC_JOB_NONE) < 0)
-goto endjob;
-
+qemuDomainObjEnterMonitor(driver, vm);
 tmp = qemuMonitorGetSEVMeasurement(QEMU_DOMAIN_PRIVATE(vm)->mon);
 if (tmp == NULL)
 goto endjob;
-- 
2.14.4

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

[libvirt] [PATCH 0/2] A potential deadlock fix for SEV and a tiny cleanup

[Erik Skultety]

*** BLURB HERE ***

Erik Skultety (2):
  qemu: sev: Use EnterMonitor instead of EnterMonitorAsync
  qemu: sev: Don't jump to endjob if SEV measurement retrieval fails

 src/qemu/qemu_driver.c | 8 ++--
 1 file changed, 2 insertions(+), 6 deletions(-)

--
2.14.4

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

[libvirt] [libvirt-go-xml PATCH v2 1/2] Add support for domain launch security

[Erik Skultety]

Signed-off-by: Erik Skultety 
---
 domain.go | 162 +-
 1 file changed, 161 insertions(+), 1 deletion(-)

diff --git a/domain.go b/domain.go
index aeeb24a..c910962 100644
--- a/domain.go
+++ b/domain.go
@@ -1863,6 +1863,18 @@ type DomainFeatureCapability struct {
State string `xml:"state,attr,omitempty"`
 }
 
+type DomainLaunchSecurity struct {
+   SEV *DomainLaunchSecuritySEV `xml:"-"`
+}
+
+type DomainLaunchSecuritySEV struct {
+   CBitPos *uint  `xml:"cbitpos"`
+   ReducedPhysBits *uint  `xml:"reducedPhysBits"`
+   Policy  *uint  `xml:"policy"`
+   DHCert  string `xml:"dhCert"`
+   Session string `xml:"sesion"`
+}
+
 type DomainFeatureCapabilities struct {
Policy string   `xml:"policy,attr,omitempty"`
AuditControl   *DomainFeatureCapability `xml:"audit_control"`
@@ -2182,7 +2194,8 @@ type Domain struct {
QEMUCommandline  *DomainQEMUCommandline
LXCNamespace *DomainLXCNamespace
VMWareDataCenterPath *DomainVMWareDataCenterPath
-   KeyWrap  *DomainKeyWrap `xml:"keywrap"`
+   KeyWrap  *DomainKeyWrap`xml:"keywrap"`
+   LaunchSecurity   *DomainLaunchSecurity `xml:"launchSecurity"`
 }
 
 func (d *Domain) Unmarshal(doc string) error {
@@ -4864,3 +4877,150 @@ func (d *DomainCPU) Marshal() (string, error) {
}
return string(doc), nil
 }
+
+func (a *DomainLaunchSecuritySEV) MarshalXML (e *xml.Encoder, start 
xml.StartElement) error {
+   e.EncodeToken(start)
+   cbitpos := xml.StartElement{
+   Name: xml.Name{Local: "cbitpos"},
+   }
+   e.EncodeToken(cbitpos)
+   e.EncodeToken(xml.CharData(fmt.Sprintf("%d", *a.CBitPos)))
+   e.EncodeToken(cbitpos.End())
+
+   reducedPhysBits := xml.StartElement{
+   Name: xml.Name{Local: "reducedPhysBits"},
+   }
+   e.EncodeToken(reducedPhysBits)
+   e.EncodeToken(xml.CharData(fmt.Sprintf("%d", *a.ReducedPhysBits)))
+   e.EncodeToken(reducedPhysBits.End())
+
+   if a.Policy != nil {
+   policy := xml.StartElement{
+   Name: xml.Name{Local: "policy"},
+   }
+   e.EncodeToken(policy)
+   e.EncodeToken(xml.CharData(fmt.Sprintf("0x%04x", *a.Policy)))
+   e.EncodeToken(policy.End())
+   }
+
+   dhcert := xml.StartElement{
+   Name: xml.Name{Local: "dhCert"},
+   }
+   e.EncodeToken(dhcert)
+   e.EncodeToken(xml.CharData(fmt.Sprintf("%s", a.DHCert)))
+   e.EncodeToken(dhcert.End())
+
+   session := xml.StartElement{
+   Name: xml.Name{Local: "session"},
+   }
+   e.EncodeToken(session)
+   e.EncodeToken(xml.CharData(fmt.Sprintf("%s", a.Session)))
+   e.EncodeToken(session.End())
+
+   e.EncodeToken(start.End())
+
+   return nil
+}
+
+func (a *DomainLaunchSecuritySEV) UnmarshalXML(d *xml.Decoder, start 
xml.StartElement) error {
+   for {
+   tok, err := d.Token()
+   if err == io.EOF {
+   break
+   }
+   if err != nil {
+   return err
+   }
+
+   switch tok := tok.(type) {
+   case xml.StartElement:
+   if tok.Name.Local == "policy" {
+   data, err := d.Token()
+   if err != nil {
+   return err
+   }
+   switch data := data.(type) {
+   case xml.CharData:
+   if err := 
unmarshalUintAttr(string(data), , 16); err != nil {
+   return err
+   }
+   }
+   } else if tok.Name.Local == "cbitpos" {
+   data, err := d.Token()
+   if err != nil {
+   return err
+   }
+   switch data := data.(type) {
+   case xml.CharData:
+   if err := 
unmarshalUintAttr(string(data), , 10); err != nil {
+   return err
+   }
+   }
+   } else if tok.Name.Local == "reducedPhysBits" {
+   data, err := d.Token()
+   if err != nil {
+   return err
+   }
+   switch data := data.(type) {
+   case xml.CharData:
+   

[libvirt] [libvirt-go-xml PATCH v2 0/2] Add support for domain launch security

[Erik Skultety]

since v1:
- added a launch security subtype for SEV to avoid having a 'type' XML
attribute

Erik Skultety (2):
  Add support for domain launch security
  Add support for SEV in domain capabilities XML

 domain.go  | 162 -
 domain_capabilities.go |   7 +++
 2 files changed, 168 insertions(+), 1 deletion(-)

--
2.14.4

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

[libvirt] [libvirt-go-xml PATCH v2 2/2] Add support for SEV in domain capabilities XML

[Erik Skultety]

Signed-off-by: Erik Skultety 
Reviewed-by: Daniel P. Berrangé
---
 domain_capabilities.go | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/domain_capabilities.go b/domain_capabilities.go
index 3f5a752..0faa06a 100644
--- a/domain_capabilities.go
+++ b/domain_capabilities.go
@@ -106,6 +106,7 @@ type DomainCapsFeatures struct {
GIC*DomainCapsFeatureGIC`xml:"gic"`
VMCoreInfo *DomainCapsFeatureVMCoreInfo `xml:"vmcoreinfo"`
GenID  *DomainCapsFeatureGenID  `xml:"genid"`
+   SEV*DomainCapsFeatureSEV`xml:"sev"`
 }
 
 type DomainCapsFeatureGIC struct {
@@ -121,6 +122,12 @@ type DomainCapsFeatureGenID struct {
Supported string `xml:"supported,attr"`
 }
 
+type DomainCapsFeatureSEV struct {
+   Supported   string `xml:"supported,attr"`
+   CBitPos uint   `xml:"cbitpos,omitempty"`
+   ReducedPhysBits uint   `xml:"reducedPhysBits,omitempty"`
+}
+
 func (c *DomainCaps) Unmarshal(doc string) error {
return xml.Unmarshal([]byte(doc), c)
 }
-- 
2.14.4

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

Re: [libvirt] [PATCH] events: Remove ATTRIBUTE_NONNULL for virObjectEventStateQueue[Remote]

[Ján Tomko]

On Fri, Jun 15, 2018 at 03:49:51PM -0400, John Ferlan wrote:

Commit aad3a0b5f altered virObjectEventStateQueueRemote to move
the "if (!event) return" call added in the previous commit 031eb8f6
to virObjectEventStateQueue. Neither commit altered the function
prototype which used ATTRIBUTE_NONNULL(2).

This caused Coverity build problems. Since @event is now checked,
just remove the ATTRIBUTE_NONNULL check from both prototypes.

Signed-off-by: John Ferlan 
---
src/conf/object_event.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

Although a Coverity build breaker, that doesn't affect everyone so
I'll wait for R-By or ACK for push.



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [libvirt-go PATCH 2/2] Add support for AMD SEV platform info

[Erik Skultety]

On Thu, Jun 14, 2018 at 04:52:04PM +0100, Daniel P. Berrangé wrote:
> On Thu, Jun 14, 2018 at 04:30:01PM +0200, Erik Skultety wrote:
> > Signed-off-by: Erik Skultety 
> > ---
> >  connect.go| 59 
> > +++
> >  connect_compat.go | 12 +++
> >  connect_compat.h  |  7 +++
> >  3 files changed, 78 insertions(+)
> >
> > diff --git a/connect.go b/connect.go
> > index e3e643e..8bb5fe6 100644
> > --- a/connect.go
> > +++ b/connect.go
> > @@ -2765,3 +2765,62 @@ func (c *Connect) GetAllDomainStats(doms []*Domain, 
> > statsTypes DomainStatsTypes,
> >
> > return stats, nil
> >  }
> > +
> > +type NodeSEVParameters struct {
> > +   PdhSet bool
> > +   Pdhstring
>
> Lets s/Pdh/PDH/ since its an acronym
>
> > +   CertChainSet   bool
> > +   CertChain  string
> > +   CbitposSet bool
> > +   Cbitposuint
>
> and s/Cbitpos/CBitPos/
>
> > +   ReducedPhysBitsSet bool
> > +   ReducedPhysBitsuint
> > +}
> > +
> > +func getNodeSEVFieldInfo(params *NodeSEVParameters) 
> > map[string]typedParamsFieldInfo {
> > +   return map[string]typedParamsFieldInfo{
> > +   C.VIR_NODE_SEV_PDH: typedParamsFieldInfo{
> > +   set: ,
> > +   s:   ,
> > +   },
> > +   C.VIR_NODE_SEV_CERT_CHAIN: typedParamsFieldInfo{
> > +   set: ,
> > +   s:   ,
> > +   },
> > +   C.VIR_NODE_SEV_CBITPOS: typedParamsFieldInfo{
> > +   set: ,
> > +   ui:   ,
> > +   },
> > +   C.VIR_NODE_SEV_REDUCED_PHYS_BITS: typedParamsFieldInfo{
> > +   set: ,
> > +   ui:   ,
> > +   },
>
> Miinor nitpick on indentation  - just run gofmt over it
>
>
>
> > diff --git a/connect_compat.go b/connect_compat.go
> > index 617bc4a..544def2 100644
> > --- a/connect_compat.go
> > +++ b/connect_compat.go
> > @@ -157,5 +157,17 @@ int virConnectCompareHypervisorCPUCompat(virConnectPtr 
> > conn,
> >  #endif
> >  }
> >
> > +int virNodeGetSEVInfoCompat(virConnectPtr conn,
> > +   virTypedParameterPtr *params,
> > +   int *nparams,
> > +   unsigned int flags)
>
> Indent here too, though gofmt probably won't fix this since it is C code layer
>
> > +{
> > +#if LIBVIR_VERSION_NUMBER < 4005000
> > +assert(0); // Caller should have checked version
> > +#else
> > +return virNodeGetSEVInfo(conn, params, nparams, flags);
> > +#endif
> > +}
> > +
> >  */
> >  import "C"
>
> With the few nitpicks fixed
>
> Reviewed-by: Daniel P. Berrangé 

Fixed and pushed, thanks.

Erik

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

Re: [libvirt] [PATCH] qemuDomainDetachDeviceConfig: Don't free device from @dev

[Ján Tomko]

On Fri, Jun 15, 2018 at 04:20:41PM +0200, Michal Privoznik wrote:

https://bugzilla.redhat.com/show_bug.cgi?id=1591561

For reasons I don't understand my original patch of 75f0fd51124
freed not only the chardev from domain but also the one from
passed virDomainDeviceDefPtr. This caused no troubles until now,
because those two pointers were separate, but after I've
introduced virDomainDetachDeviceAlias() they became the same
resulting in double free on detach.

Signed-off-by: Michal Privoznik 
---
src/qemu/qemu_driver.c | 2 --
1 file changed, 2 deletions(-)



Reviewed-by: Ján Tomko 

Jano


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