Re: [libvirt] [PATCH 04/20] libxl: Use virDomainObjListFindBy{UUID|ID}Ref

[Jim Fehlig]

On 03/11/2018 08:14 PM, Jim Fehlig wrote:

On 03/09/2018 09:48 AM, John Ferlan wrote:

For libxlDomainLookupByID and libxlDomainLookupByUUID let's
return a locked and referenced @vm object so that callers can
then use the common and more consistent virDomainObjEndAPI in
order to handle cleanup rather than needing to know that the
returned object is locked and calling virObjectUnlock.

The LookupByName already returns the ref counted and locked object,
so this will make things more consistent.

Signed-off-by: John Ferlan 
---
  src/libxl/libxl_driver.c | 10 --
  1 file changed, 4 insertions(+), 6 deletions(-)


Reviewed-by: Jim Fehlig 


I pushed this one as well.

Regards,
Jim

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

Re: [libvirt] [PATCH 03/20] libxl: Properly cleanup after libxlDomObjFromDomain

[Jim Fehlig]

On 03/11/2018 08:13 PM, Jim Fehlig wrote:

On 03/09/2018 09:47 AM, John Ferlan wrote:

Commit id '9ac945078' altered libxlDomObjFromDomain to return
a locked *and* ref counted object for some specific purposes;
however, it neglected to alter all the consumers of the helper
to use virDomainObjEndAPI thus leaving many objects with extra
ref counts.

The two consumers for libxlDomainMigrationConfirm would also
originally use the libxlDomObjFromDomain API (either from
libxlDomainMigrateConfirm3Params or libxlDoMigrateP2P via
libxlDomainMigrationPerformP2P and libxlDomainMigratePerform3Params.

Signed-off-by: John Ferlan 
---
  src/libxl/libxl_driver.c    | 86 -
  src/libxl/libxl_migration.c |  3 +-
  2 files changed, 31 insertions(+), 58 deletions(-)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index b5101626e..9aa4a293c 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -1322,8 +1322,7 @@ libxlDomainShutdownFlags(virDomainPtr dom, unsigned int 
flags)

  }
   cleanup:
-    if (vm)
-    virObjectUnlock(vm);
+    virDomainObjEndAPI();
  virObjectUnref(cfg);
  return ret;
  }
@@ -1373,8 +1372,7 @@ libxlDomainReboot(virDomainPtr dom, unsigned int flags)
  }
   cleanup:
-    if (vm)
-    virObjectUnlock(vm);
+    virDomainObjEndAPI();
  virObjectUnref(cfg);
  return ret;
  }
@@ -1459,8 +1457,7 @@ libxlDomainGetOSType(virDomainPtr dom)
  goto cleanup;
   cleanup:
-    if (vm)
-    virObjectUnlock(vm);
+    virDomainObjEndAPI();
  return type;
  }
@@ -1479,8 +1476,7 @@ libxlDomainGetMaxMemory(virDomainPtr dom)
  ret = virDomainDefGetMemoryTotal(vm->def);
   cleanup:
-    if (vm)
-    virObjectUnlock(vm);
+    virDomainObjEndAPI();
  return ret;
  }
@@ -1658,8 +1654,7 @@ libxlDomainGetInfo(virDomainPtr dom, virDomainInfoPtr 
info)
  ret = 0;
   cleanup:
-    if (vm)
-    virObjectUnlock(vm);
+    virDomainObjEndAPI();
  virObjectUnref(cfg);
  return ret;
  }
@@ -1685,8 +1680,7 @@ libxlDomainGetState(virDomainPtr dom,
  ret = 0;
   cleanup:
-    if (vm)
-    virObjectUnlock(vm);
+    virDomainObjEndAPI();
  return ret;
  }
@@ -2110,8 +2104,7 @@ libxlDomainHasManagedSaveImage(virDomainPtr dom, 
unsigned int flags)

  ret = vm->hasManagedSave;
   cleanup:
-    if (vm)
-    virObjectUnlock(vm);
+    virDomainObjEndAPI();
  return ret;
  }
@@ -2140,8 +2133,7 @@ libxlDomainManagedSaveRemove(virDomainPtr dom, unsigned 
int flags)

   cleanup:
  VIR_FREE(name);
-    if (vm)
-    virObjectUnlock(vm);
+    virDomainObjEndAPI();
  return ret;
  }
@@ -2352,8 +2344,7 @@ libxlDomainGetVcpusFlags(virDomainPtr dom, unsigned int 
flags)

  ret = virDomainDefGetVcpus(def);
   cleanup:
-    if (vm)
-    virObjectUnlock(vm);
+    virDomainObjEndAPI();
  return ret;
  }
@@ -2484,8 +2475,7 @@ libxlDomainGetVcpuPinInfo(virDomainPtr dom, int ncpumaps,
 libxl_get_max_cpus(cfg->ctx), 
NULL);
   cleanup:
-    if (vm)
-    virObjectUnlock(vm);
+    virDomainObjEndAPI();
  virObjectUnref(cfg);
  return ret;
  }
@@ -2548,8 +2538,7 @@ libxlDomainGetVcpus(virDomainPtr dom, virVcpuInfoPtr 
info, int maxinfo,

  ret = maxinfo;
   cleanup:
-    if (vm)
-    virObjectUnlock(vm);
+    virDomainObjEndAPI();
  virObjectUnref(cfg);
  return ret;
  }
@@ -2580,8 +2569,7 @@ libxlDomainGetXMLDesc(virDomainPtr dom, unsigned int 
flags)
   virDomainDefFormatConvertXMLFlags(flags));
   cleanup:
-    if (vm)
-    virObjectUnlock(vm);
+    virDomainObjEndAPI();
  virObjectUnref(cfg);
  return ret;
  }
@@ -2901,8 +2889,7 @@ libxlDomainUndefineFlags(virDomainPtr dom,
   cleanup:
  VIR_FREE(name);
-    if (vm)
-    virObjectUnlock(vm);
+    virDomainObjEndAPI();
  if (event)
  libxlDomainEventQueue(driver, event);
  virObjectUnref(cfg);
@@ -4261,8 +4248,7 @@ libxlDomainUpdateDeviceFlags(virDomainPtr dom, const 
char *xml,

   cleanup:
  virDomainDefFree(vmdef);
  virDomainDeviceDefFree(dev);
-    if (vm)
-    virObjectUnlock(vm);
+    virDomainObjEndAPI();
  virObjectUnref(cfg);
  return ret;
  }
@@ -4394,8 +4380,7 @@ libxlDomainGetAutostart(virDomainPtr dom, int *autostart)
  ret = 0;
   cleanup:
-    if (vm)
-    virObjectUnlock(vm);
+    virDomainObjEndAPI();
  return ret;
  }
@@ -4521,8 +4506,7 @@ libxlDomainGetSchedulerType(virDomainPtr dom, int 
*nparams)
  ignore_value(VIR_STRDUP(ret, name));
   cleanup:
-    if (vm)
-    virObjectUnlock(vm);
+    virDomainObjEndAPI();
  virObjectUnref(cfg);
  return ret;
  }
@@ -4587,8 +4571,7 @@ libxlDomainGetSchedulerParametersFlags(virDomainPtr dom,
  ret = 0;
   cleanup:
-    if (vm)
-    virObjectUnlock(vm);
+    virDomainObjEndAPI();
  virObjectUnref(cfg);
  return ret;
  }
@@ -4750,8 +4733,7 

Re: [libvirt] [PATCH V2] libxl: MigratePrepare: use standard begin and end API pattern

[John Ferlan]

On 03/16/2018 03:46 PM, Jim Fehlig wrote:
> libxlDomainMigrationPrepare adds the incoming domain def to the list
> of domains via virDomainObjListAdd, but never adds its own ref to the
> returned virDomainObj as other callers of virDomainObjListAdd do.
> libxlDomainMigrationPrepareTunnel3 suffers the same discrepancy.
> 
> Change both to add a ref to the virDomainObj after a successful
> virDomainObjListAdd, similar to other callers. This ensures a consistent
> pattern throughout the drivers and allows using the virDomainObjEndAPI
> function for cleanup.
> 
> Signed-off-by: Jim Fehlig 
> ---
> 
> V2 of
> 
> https://www.redhat.com/archives/libvir-list/2018-March/msg00674.html
> 
> Changes in V2:
> Add ref after call to virDomainObjListAdd so that EndAPI can be used
> in the usual pattern.
> 
>  src/libxl/libxl_migration.c | 13 ++---
>  1 file changed, 6 insertions(+), 7 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 v5 3/9] libxl: error out on not supported CPU mode, instead of silently ignoring

[Marek Marczykowski-Górecki]

On Fri, Mar 16, 2018 at 05:39:35PM +, Daniel P. Berrangé wrote:
> On Wed, Mar 14, 2018 at 03:26:10AM +0100, Marek Marczykowski-Górecki wrote:
> > This change make libvirt XML with plain  element invalid for libxl,
> > which affect not only upcoming CPUID support, but also NUMA. In fact,
> > default mode 'custom' does not match what the driver actually does, so
> > it was a bug. Adjust xenconfig driver accordingly.
> > To not break existing configuration, add PostParse hook to update
> > (previously ignored) default mode 'custom' to 'host-passthrough'.
> 
> Maybe I'm missing something, but by doing this silent conversion
> from custom -> host-passthrough, don't you make it such that the
> error about unsupported CPU mode is largely unreachable ? IOW seems
> to end up with the same functional result as the original code,
> except for a error when 'host-model' is used.

Mostly - yes.

> I don't have a particular better idea though if we have alot of
> pre-existing usage with mode=custom ? 

Previously mode was mostly ignored (besides using it for enabling nested
HVM, which is now controlled additionally by global switch).

I'm not specifically attached to the PostParse, but I think it may be
nicer for users, to not throw an error on previously "valid"
configuration. If libvirt would have XML versioning, it could execute
such conversion only on upgrade...

Alternatively, instead of rejecting mode=custom, the whole cpuid
handling could be made conditional to mode=host-passthrough (and ignored
with mode=custom). I have an impression this would better fit into the
(libxl?) driver - I find it often that various settings are ignored
instead of throwing VIR_ERR_CONFIG_UNSUPPORTED... Admittedly, this
makes it easier to maintain a configuration compatible with wide range
of libvirt versions.

> Has this been widely
> done, or can we justify breaking it ?
>
> > Signed-off-by: Marek Marczykowski-Górecki 
> > ---
> > Changes since v4:
> >  - add PostParse hook to automatically set host-passthrough mode, if
> >was the default one before (custom)
> > Changes since v3:
> >  - fix vnuma tests (nested HVM implicitly enabled there)
> > Changes since v2:
> >  - change separated from 'libxl: add support for CPUID features policy'

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?


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

Re: [libvirt] [PATCH v5 7/9] tests: check CPU features handling in libxl driver

[Marek Marczykowski-Górecki]

On Fri, Mar 16, 2018 at 05:44:28PM +, Daniel P. Berrangé wrote:
> Since Xen lets you specify raw  "cpuid" register values here, surely
> this is flexible enough to allow us to support the mode=custom CPU
> models ?
> 
> We would just need to make sure every bit poisition used either
> 0 or 1, and not 'x', so that we are fully overriding whatever
> defaults are presented by the hypervisor "host" CPU model. Or is
> life more complicated than that ?

This is what v1 of this series had... See discussion there, especially
message from Jiri:
https://www.redhat.com/archives/libvir-list/2017-June/msg01304.html

And this, from... you:
https://www.redhat.com/archives/libvir-list/2017-June/msg01308.html

This is not only about 'x'. But also about setting '1' where hardware
does not really support given feature. This will also result in "broken"
CPU.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?


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

Re: [libvirt] [PATCH 02/20] libxl: Fix possible deadlock in libxlDomainMigrateBegin3Params

[Jim Fehlig]

On 03/09/2018 03:33 PM, Jim Fehlig wrote:

On 03/09/2018 09:47 AM, John Ferlan wrote:

Commit id '45697fe5' added a check for "Domain-0" to generate
an error during libxlDomainMigrateBegin3Params; however, by
returning NULL, the @vm was left locked since libxlDomObjFromDomain
returns a locked @vm.

Signed-off-by: John Ferlan 
---
  src/libxl/libxl_driver.c | 7 ---
  1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index c3616a86d..b5101626e 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -5895,9 +5895,10 @@ libxlDomainMigrateBegin3Params(virDomainPtr domain,
  return NULL;
  if (STREQ_NULLABLE(vm->def->name, "Domain-0")) {
-    virReportError(VIR_ERR_OPERATION_INVALID, "%s",
-   _("Domain-0 cannot be migrated"));
-    return NULL;
+    virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+   _("Domain-0 cannot be migrated"));
+    virObjectUnlock(vm);
+    return NULL;
  }
  if (virDomainMigrateBegin3ParamsEnsureACL(domain->conn, vm->def) < 0) {



Reviewed-by: Jim Fehlig 


No longer needed since this fix was folded into commit 64370c4b.

Regards,
Jim

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

[libvirt] [PATCH V2] libxl: MigratePrepare: use standard begin and end API pattern

[Jim Fehlig]

libxlDomainMigrationPrepare adds the incoming domain def to the list
of domains via virDomainObjListAdd, but never adds its own ref to the
returned virDomainObj as other callers of virDomainObjListAdd do.
libxlDomainMigrationPrepareTunnel3 suffers the same discrepancy.

Change both to add a ref to the virDomainObj after a successful
virDomainObjListAdd, similar to other callers. This ensures a consistent
pattern throughout the drivers and allows using the virDomainObjEndAPI
function for cleanup.

Signed-off-by: Jim Fehlig 
---

V2 of

https://www.redhat.com/archives/libvir-list/2018-March/msg00674.html

Changes in V2:
Add ref after call to virDomainObjListAdd so that EndAPI can be used
in the usual pattern.

 src/libxl/libxl_migration.c | 13 ++---
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
index 42a84bd35..7dc39ae02 100644
--- a/src/libxl/libxl_migration.c
+++ b/src/libxl/libxl_migration.c
@@ -583,6 +583,7 @@ libxlDomainMigrationPrepareTunnel3(virConnectPtr dconn,
NULL)))
 goto error;
 
+virObjectRef(vm);
 *def = NULL;
 priv = vm->privateData;
 
@@ -635,13 +636,11 @@ libxlDomainMigrationPrepareTunnel3(virConnectPtr dconn,
 /* Remove virDomainObj from domain list */
 if (vm) {
 virDomainObjListRemove(driver->domains, vm);
-vm = NULL;
+virObjectLock(vm);
 }
 
  done:
-if (vm)
-virObjectUnlock(vm);
-
+virDomainObjEndAPI();
 return ret;
 }
 
@@ -683,6 +682,7 @@ libxlDomainMigrationPrepare(virConnectPtr dconn,
NULL)))
 goto error;
 
+virObjectRef(vm);
 *def = NULL;
 priv = vm->privateData;
 
@@ -810,7 +810,7 @@ libxlDomainMigrationPrepare(virConnectPtr dconn,
 /* Remove virDomainObj from domain list */
 if (vm) {
 virDomainObjListRemove(driver->domains, vm);
-vm = NULL;
+virObjectLock(vm);
 }
 
  done:
@@ -820,8 +820,7 @@ libxlDomainMigrationPrepare(virConnectPtr dconn,
 VIR_FREE(hostname);
 else
 virURIFree(uri);
-if (vm)
-virObjectUnlock(vm);
+virDomainObjEndAPI();
 virObjectUnref(cfg);
 return ret;
 }
-- 
2.16.2

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

Re: [libvirt] [PATCH v5 2/9] libxl: pass driver config to libxlMakeDomBuildInfo

[Marek Marczykowski-Górecki]

On Fri, Mar 16, 2018 at 05:36:44PM +, Daniel P. Berrangé wrote:
> On Wed, Mar 14, 2018 at 03:26:09AM +0100, Marek Marczykowski-Górecki wrote:
> > @@ -48,6 +49,19 @@ VIR_MOCK_IMPL_RET_ARGS(xc_interface_open,
> >  }
> >  
> >  
> > +VIR_MOCK_IMPL_RET_ARGS(libxl_get_version_info,
> > +   const libxl_version_info*,
> > +   libxl_ctx *, ctx)
> > +{
> > +static libxl_version_info info;
> > +
> > +memset(, 0, sizeof(info));
> > +
> > +return 
> > +/* silence gcc warning */
> > +return real_libxl_get_version_info(ctx);
> 
> Why was gcc warning about that requires the second return
> statement ?  I would have though this would /cause/ a
> warning by creating unreachable code ?

Because or static real_##name in (unused otherwise):

# define VIR_MOCK_IMPL_RET_ARGS(name, rettype, ...) \
rettype name(VIR_MOCK_ARGTYPENAMES(__VA_ARGS__)); \
static rettype (*real_##name)(VIR_MOCK_ARGTYPES(__VA_ARGS__)); \
rettype name(VIR_MOCK_ARGTYPENAMES_UNUSED(__VA_ARGS__))

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?


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

Re: [libvirt] [PATCH v1 4/7] qemu: Use correct bus type for input devices

[Farhan Ali]

On 03/16/2018 09:39 AM, John Ferlan wrote:



On 03/08/2018 11:07 AM, Farhan Ali wrote:

commit 7210cef452db 'qemu: build command line for virtio input devices'
introduced an error, by checking if input bus type is
VIR_DOMAIN_DISK_BUS_VIRTIO.

Fix it by using the correct bus type for input devices.

Signed-off-by: Farhan Ali 
Reviewed-by: Boris Fiuczynski 
---
  src/qemu/qemu_domain_address.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)



Nice catch!  Been around awhile too.  Doesn't look like there was a test
to catch it either.


Hopefully my virtio ccw input tests should be enough to cover this?



Reviewed-by: John Ferlan 

John

(I'll push this along with patch 1 since it's separable)




Thanks so much for all your review, I really appreciate all your 
feeedback :)


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

Re: [libvirt] [PATCH v1 7/7] news: Update for virtio-gpu-ccw and virtio input ccw devices

[Farhan Ali]

On 03/16/2018 09:40 AM, John Ferlan wrote:



On 03/08/2018 11:07 AM, Farhan Ali wrote:

Document support for the virtio-gpu-ccw and
virtio-{keyboard, mouse, tablet}-ccw devices.

Signed-off-by: Farhan Ali 
Reviewed-by: Boris Fiuczynski 
---
  docs/news.xml | 10 ++
  1 file changed, 10 insertions(+)



Yay, Andrea will be happy someone who remembered news.xml ;-)




I try my best not to forget it ;)


diff --git a/docs/news.xml b/docs/news.xml
index a51ca97..7eb71e3 100644
--- a/docs/news.xml
+++ b/docs/news.xml
@@ -35,6 +35,16 @@
  

  
+  
+
+  qemu: Provide virtio-gpu-ccw and virtio input ccw support


How about:

  "Provide ccw address support for graphics and input devices"

John



Seems reasonable. Will update




+
+
+  Support the virtio-gpu-ccw device as a video device and
+  virtio-{keyboard, mouse, tablet}-ccw devices as input devices
+  on S390.
+
+  
  
  






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

Re: [libvirt] [PATCH v1 3/7] tests: Add test case for virtio-gpu-ccw

[Farhan Ali]

On 03/16/2018 09:39 AM, John Ferlan wrote:



On 03/08/2018 11:07 AM, Farhan Ali wrote:

A test case to test the virtio-gpu-ccw device.

Signed-off-by: Farhan Ali 
Signed-off-by: Boris Fiuczynski 
---
  .../qemuxml2argvdata/video-virtio-gpu-ccw-auto.xml | 18 ++
  tests/qemuxml2argvdata/video-virtio-gpu-ccw.args   | 25 ++
  tests/qemuxml2argvdata/video-virtio-gpu-ccw.xml| 35 
  tests/qemuxml2argvtest.c   |  7 
  .../video-virtio-gpu-ccw-auto.xml  | 34 +++
  tests/qemuxml2xmloutdata/video-virtio-gpu-ccw.xml  | 38 ++
  tests/qemuxml2xmltest.c| 14 
  7 files changed, 171 insertions(+)
  create mode 100644 tests/qemuxml2argvdata/video-virtio-gpu-ccw-auto.xml
  create mode 100644 tests/qemuxml2argvdata/video-virtio-gpu-ccw.args
  create mode 100644 tests/qemuxml2argvdata/video-virtio-gpu-ccw.xml
  create mode 100644 tests/qemuxml2xmloutdata/video-virtio-gpu-ccw-auto.xml
  create mode 100644 tests/qemuxml2xmloutdata/video-virtio-gpu-ccw.xml



The tests look OK - could have been merged with the "functionality"
patch, but it's OK that they're separate - as long as they exist it's a
good thing.

I assume the xml2xml output would be "valid" after the "capability"
patch - if so you could extract it into it's own patch if you felt
really compelled to do so.

John



I just feel it's easier to understand to have them all in one patch :)

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

Re: [libvirt] [PATCH v1 2/7] qemu: Add support for virtio-gpu-ccw device

[Farhan Ali]

Hi Jon,

On 03/16/2018 09:38 AM, John Ferlan wrote:



On 03/08/2018 11:07 AM, Farhan Ali wrote:

QEMU on S390 (since v2.11) can support the virtio-gpu-ccw device,
so on S390 assign CCW address for a video device.

Also introduce a new capability for the virtio-gpu-ccw
device.

Signed-off-by: Farhan Ali 
Signed-off-by: Boris Fiuczynski 
---
  docs/formatdomain.html.in  |  3 +
  src/qemu/qemu_capabilities.c   |  5 ++
  src/qemu/qemu_capabilities.h   |  1 +
  src/qemu/qemu_command.c| 18 -
  src/qemu/qemu_domain.c |  2 +-
  src/qemu/qemu_domain_address.c |  8 +++
  src/qemu/qemu_process.c|  5 +-
  .../qemucapabilitiesdata/caps_2.11.0.s390x.replies | 83 +++---
  tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml   |  3 +-
  9 files changed, 114 insertions(+), 14 deletions(-)



First off - other upstream changes has caused this to not be able to
apply w/ git am -3 - so once patch1 is pushed, you'll need to rework and
repost the series (hopefully this time you won't have the strange split
with a CC address having "--dry-run" appended to an email ;-)


Ah yes, it was silly of me! I didn't realize till it was too late :)



Since I cannot compile this one - I'll just be able to go through
visually and note a few things...  Primarily though - this particular
patch should be split up a bit more.  Separate out the capabilities into
their own separate "capabilities" patch and the second patch becomes a
"functionality" patch.



Sure, I will move capabilities into a different patch.


diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 6fd2189..0908709 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -6484,6 +6484,9 @@ qemu-kvm -net nic,model=? /dev/null

  The optional address sub-element can be used to
  tie the video device to a particular PCI slot.
+On S390, address can be used to provide the
+CCW address for the video device (
+since 4.2.0).

  
driver



... The above would go with the "functionality" patch and the next few
with a "capability" patch...


diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index b5eb8cf..9db4c31 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -459,6 +459,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
"pl011",
"machine.pseries.max-cpu-compat",
"dump-completed",
+  "virtio-gpu-ccw",
  );
  
  
@@ -1694,6 +1695,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = {

  { "sclplmconsole", QEMU_CAPS_DEVICE_SCLPLMCONSOLE },
  { "isa-serial", QEMU_CAPS_DEVICE_ISA_SERIAL },
  { "pl011", QEMU_CAPS_DEVICE_PL011 },
+{ "virtio-gpu-ccw", QEMU_CAPS_DEVICE_VIRTIO_GPU_CCW },
  };
  
  static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsVirtioBalloon[] = {

@@ -1932,6 +1934,9 @@ static struct virQEMUCapsObjectTypeProps 
virQEMUCapsObjectProps[] = {
  { "spapr-pci-host-bridge", virQEMUCapsObjectPropsSpaprPCIHostBridge,
ARRAY_CARDINALITY(virQEMUCapsObjectPropsSpaprPCIHostBridge),
QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE },
+{ "virtio-gpu-ccw", virQEMUCapsObjectPropsVirtioGpu,
+  ARRAY_CARDINALITY(virQEMUCapsObjectPropsVirtioGpu),
+  QEMU_CAPS_DEVICE_VIRTIO_GPU_CCW },
  };
  
  struct virQEMUCapsPropTypeObjects {

diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index c2ec2be..b4852e5 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -444,6 +444,7 @@ typedef enum {
  QEMU_CAPS_DEVICE_PL011, /* -device pl011 (not user-instantiable) */
  QEMU_CAPS_MACHINE_PSERIES_MAX_CPU_COMPAT, /* -machine 
pseries,max-cpu-compat= */
  QEMU_CAPS_DUMP_COMPLETED, /* DUMP_COMPLETED event */
+QEMU_CAPS_DEVICE_VIRTIO_GPU_CCW, /* -device virtio-gpu-ccw */
  
  QEMU_CAPS_LAST /* this must always be the last item */

  } virQEMUCapsFlags;


... Above here with the "capability" patch and below with the
"functionality" patch...


diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index fa0aa5d..ba63670 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -132,7 +132,7 @@ VIR_ENUM_IMPL(qemuDeviceVideoSecondary, 
VIR_DOMAIN_VIDEO_TYPE_LAST,
"", /* don't support vbox */
"qxl",
"", /* don't support parallels */
-  "virtio-gpu-pci",
+  "virtio-gpu",
"" /* don't support gop */);
  
  VIR_ENUM_DECL(qemuSoundCodec)

@@ -4262,7 +4262,21 @@ qemuBuildDeviceVideoStr(const virDomainDef *def,
  goto error;
  }
  
-virBufferAsprintf(, "%s,id=%s", model, video->info.alias);

+if (STREQ(model, "virtio-gpu")) {

Re: [libvirt] [PATCH 1/2] apibuild: Fix errors on python3

[Daniel P . Berrangé]

On Fri, Mar 16, 2018 at 02:05:11PM -0400, Cole Robinson wrote:
> Module 'string' function lower doesn't exist in python3. The canonical
> way is to call .lower() on a str instance. Do that, and make the
> exception handling more specific, which would have made this issue
> obvious.
> 
> Signed-off-by: Cole Robinson 
> ---
>  docs/apibuild.py | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Daniel P. Berrangé 


> 
> diff --git a/docs/apibuild.py b/docs/apibuild.py
> index 67b7eed1e..e81980e3c 100755
> --- a/docs/apibuild.py
> +++ b/docs/apibuild.py
> @@ -2326,10 +2326,10 @@ class docBuilder:
>  for data in ('Summary', 'Description', 'Author'):
>  try:
>  output.write(" <%s>%s\n" % (
> - string.lower(data),
> + data.lower(),
>   escape(dict.info[data]),
> - string.lower(data)))
> -except:
> + data.lower()))
> +except KeyError:
>  self.warning("Header %s lacks a %s description" % 
> (module, data))
>  if 'Description' in dict.info:
>  desc = dict.info['Description']
> -- 
> 2.14.3
> 
> --
> 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] [PATCH 2/2] apibuild: Fix -refs.xml building

[Daniel P . Berrangé]

On Fri, Mar 16, 2018 at 02:05:12PM -0400, Cole Robinson wrote:
> Another usage of deprecated 'string' functions. We are just trying to
> match ascii letters here, so use a simple regex. And again drop the
> aggressive exception handling, it doesn't seem to trigger for anything
> in libvirt code.
> 
> Signed-off-by: Cole Robinson 
> ---
>  docs/apibuild.py | 28 
>  1 file changed, 12 insertions(+), 16 deletions(-)

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 2/2] apibuild: Fix -refs.xml building

[Cole Robinson]

Another usage of deprecated 'string' functions. We are just trying to
match ascii letters here, so use a simple regex. And again drop the
aggressive exception handling, it doesn't seem to trigger for anything
in libvirt code.

Signed-off-by: Cole Robinson 
---
 docs/apibuild.py | 28 
 1 file changed, 12 insertions(+), 16 deletions(-)

diff --git a/docs/apibuild.py b/docs/apibuild.py
index e81980e3c..51abf8383 100755
--- a/docs/apibuild.py
+++ b/docs/apibuild.py
@@ -11,7 +11,6 @@
 from __future__ import print_function
 
 import os, sys
-import string
 import glob
 import re
 
@@ -2092,23 +2091,20 @@ class docBuilder:
 str = str.replace(';', ' ')
 tokens = str.split()
 for token in tokens:
-try:
-c = token[0]
-if string.letters.find(c) < 0:
-pass
-elif len(token) < 3:
+c = token[0]
+if not re.match(r"[a-zA-Z]", c):
+pass
+elif len(token) < 3:
+pass
+else:
+lower = token.lower()
+# TODO: generalize this a bit
+if lower == 'and' or lower == 'the':
 pass
+elif token in self.xref:
+self.xref[token].append(id)
 else:
-lower = string.lower(token)
-# TODO: generalize this a bit
-if lower == 'and' or lower == 'the':
-pass
-elif token in self.xref:
-self.xref[token].append(id)
-else:
-self.xref[token] = [id]
-except:
-pass
+self.xref[token] = [id]
 
 def analyze(self):
 if not quiet:
-- 
2.14.3

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

[libvirt] [PATCH 0/2] apibuild: fix with python3

[Cole Robinson]

These patches fix apibuild.py to work with python3 and generate
identical output to python2, at least in my testing. Someone
else should confirm though

Thanks,
Cole

Cole Robinson (2):
  apibuild: Fix errors on python3
  apibuild: Fix -refs.xml building

 docs/apibuild.py | 34 +++---
 1 file changed, 15 insertions(+), 19 deletions(-)

-- 
2.14.3

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

[libvirt] [PATCH 1/2] apibuild: Fix errors on python3

[Cole Robinson]

Module 'string' function lower doesn't exist in python3. The canonical
way is to call .lower() on a str instance. Do that, and make the
exception handling more specific, which would have made this issue
obvious.

Signed-off-by: Cole Robinson 
---
 docs/apibuild.py | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/docs/apibuild.py b/docs/apibuild.py
index 67b7eed1e..e81980e3c 100755
--- a/docs/apibuild.py
+++ b/docs/apibuild.py
@@ -2326,10 +2326,10 @@ class docBuilder:
 for data in ('Summary', 'Description', 'Author'):
 try:
 output.write(" <%s>%s\n" % (
- string.lower(data),
+ data.lower(),
  escape(dict.info[data]),
- string.lower(data)))
-except:
+ data.lower()))
+except KeyError:
 self.warning("Header %s lacks a %s description" % (module, 
data))
 if 'Description' in dict.info:
 desc = dict.info['Description']
-- 
2.14.3

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

Re: [libvirt] [PATCH 3/4] libxl: MigratePrepare: properly cleanup after virDomainObjListAdd

[Jim Fehlig]

On 03/14/2018 07:19 AM, John Ferlan wrote:



On 03/13/2018 01:26 PM, Jim Fehlig wrote:

libxlDomainMigrationPrepare adds the incoming domain def to the list of
domains via virDomainObjListAdd, which returns a locked and ref counted
virDomainObj. On exit the object is unlocked but not unref'ed. The same
is true for libxlDomainMigrationPrepareTunnel3. Convert both to use the
virDomainObjEndAPI function for cleanup.

Signed-off-by: Jim Fehlig 
---
  src/libxl/libxl_migration.c | 7 ++-
  1 file changed, 2 insertions(+), 5 deletions(-)



These two leave me concerned - mainly because as I described in my
review of 2/4 the ListAdd function will return 2 refs and 1 lock on the
object unlike the libxlDomObjFromDomain where there are at least 3 refs
and 1 lock. Since neither of these code paths adds a Ref(vm) after the
ListAdd call (like CreateXML and RestoreFlags do) that means calling
EndAPI will remove 1 ref and the lock leaving only 1 ref on the object.


I'll send a V2 of this patch that adds a ref after ListAdd so the pattern is 
consistent throughout the driver.



Later on when ListRemove is called it would enter with 2 refs and 1 lock
and leave with @vm being destroyed. Not having a clear picture of all
the paths a @vm could have in libxl makes me concerned because there are
a few places where ListRemove is called *and* @vm is referenced
afterwards such as libxlDomainDestroyFlags

I think once ListAdd returns with the right number of refs, then yes,
this is the proper adjustment, but for now unless there's an extra Ref
done after the ListAdd, then we should leave things as is.

Thoughts? And does this make sense?


Yes, it makes sense. Thanks again for the details!

Regards,
Jim

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

Re: [libvirt] [PATCH 2/4] libxl: MigrateConfirm: Dont unlock virDomainObj in helper function

[Jim Fehlig]

On 03/14/2018 06:53 AM, John Ferlan wrote:



On 03/13/2018 01:26 PM, Jim Fehlig wrote:

The libxlDomainMigrateConfirm3Params API locks and ref counts the associated
virDomainObj but relies on the helper function libxlDomainMigrationConfirm
to unlock the object. Unref'ing the object is not done in either function.
libxlDomainMigrationConfirm is also used by libxlDomainMigratePerform3Params
for p2p migration, but in that case the lock/ref and unref/unlock are
properly handled in the API entry point.

Remove the unlock from libxlDomainMigrationConfirm and adjust
libxlDomainMigrateConfirm3Params to properly unref/unlock the virDomainObj
on success and error paths.

Signed-off-by: Jim Fehlig 
---
  src/libxl/libxl_driver.c| 13 -
  src/libxl/libxl_migration.c |  2 --
  2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index eff13e5aa..67a638da0 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -6161,6 +6161,7 @@ libxlDomainMigrateConfirm3Params(virDomainPtr domain,
  {
  libxlDriverPrivatePtr driver = domain->conn->privateData;
  virDomainObjPtr vm = NULL;
+int ret = -1;
  
  #ifdef LIBXL_HAVE_NO_SUSPEND_RESUME

  virReportUnsupportedError();
@@ -6174,12 +6175,14 @@ libxlDomainMigrateConfirm3Params(virDomainPtr domain,
  if (!(vm = libxlDomObjFromDomain(domain)))
  return -1;
  
-if (virDomainMigrateConfirm3ParamsEnsureACL(domain->conn, vm->def) < 0) {

-virObjectUnlock(vm);
-return -1;
-}
+if (virDomainMigrateConfirm3ParamsEnsureACL(domain->conn, vm->def) < 0)
+goto cleanup;
  
-return libxlDomainMigrationConfirm(driver, vm, flags, cancelled);

+ret = libxlDomainMigrationConfirm(driver, vm, flags, cancelled);
+
+ cleanup:
+virDomainObjEndAPI();
+return ret;
  }
  
  static int libxlNodeGetSecurityModel(virConnectPtr conn,

diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
index 4b848c920..fef1c998b 100644
--- a/src/libxl/libxl_migration.c
+++ b/src/libxl/libxl_migration.c
@@ -1400,8 +1400,6 @@ libxlDomainMigrationConfirm(libxlDriverPrivatePtr driver,


Above here there is a possible call to virDomainObjListRemove which
would return @vm w/ at least 1 ref and unlocked. The reason it's "at
least 1 ref" is because some call paths in libxl will call
virDomainObjListAdd *and* then perform a virObjectRef(vm) while others
don't do that ref. Return from ListAdd will have 2 refs and 1 lock on
@vm (although it should be 3, hence my other series). It's not
necessarily crystal clear which ListAdd was used in this path...

In any case, "for now" I think we're mostly OK here because at least @vm
was obtained via libxlDomObjFromDomain which will return @vm w/ 1 extra
ref and locked (e.g. 3 refs and locked). So calling
virDomainObjListRemove will remove 2 refs and the lock.

All it means is the EndAPI call done will call Unlock even though it
doesn't have the lock. Of course there's no error propagation, so it
doesn't hurt. On the upside, the changes will at least allow @vm to be
free'd once the EndAPI is called (when @vm refcnt == 0) as opposed to
the changes before here where refcnt was never lowered and @vm was
probably leaked once/if it was removed from the list.

I think what could be done is - after calling ListRemove, you could call
virObjectLock(vm) and remove the vm = NULL.  That way you know you
"leave" this function with at least 1 ref and @vm being locked
regardless of what happens. That way the caller using EndAPI will do the
right thing too.


Thanks for the details. I agree with your suggestion and made the changes before 
pushing, along with adding a comment. E.g.


virDomainObjListRemove(driver->domains, vm);
/* Caller passed a locked vm and expects the same on return */
virObjectLock(vm);


Hopefully this makes sense, I have various stages of this ListAdd
problem percolating inside my head...


Yes it does, and has helped me get my head around locking and ref counting the 
virDomainObj. It's also encouraged me to audit the libxl code for other locking 
and ref counting bugs.


Regards,
Jim

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

Re: [libvirt] [PATCH v5 7/9] tests: check CPU features handling in libxl driver

[Daniel P . Berrangé]

On Wed, Mar 14, 2018 at 03:26:14AM +0100, Marek Marczykowski-Górecki wrote:
> Test enabling/disabling individual CPU features and also setting
> nested HVM support, which is also controlled by CPU features node.
> 
> Signed-off-by: Marek Marczykowski-Górecki 
> Reviewed-by: Jim Fehlig 
> ---
> Changes since v3:
>  - adjust for modified nested HVM handling
> Changes since v1:
>  - rewritten to Jim's test suite for libxl_domain_config generator
> ---
>  tests/libxlxml2domconfigdata/fullvirt-cpuid.json | 64 +-
>  tests/libxlxml2domconfigdata/fullvirt-cpuid.xml  | 37 ++-
>  tests/libxlxml2domconfigtest.c   |  1 +-
>  3 files changed, 102 insertions(+)
>  create mode 100644 tests/libxlxml2domconfigdata/fullvirt-cpuid.json
>  create mode 100644 tests/libxlxml2domconfigdata/fullvirt-cpuid.xml
> 
> diff --git a/tests/libxlxml2domconfigdata/fullvirt-cpuid.json 
> b/tests/libxlxml2domconfigdata/fullvirt-cpuid.json
> new file mode 100644
> index 000..28037be
> --- /dev/null
> +++ b/tests/libxlxml2domconfigdata/fullvirt-cpuid.json
> @@ -0,0 +1,64 @@
> +{
> +"c_info": {
> +"type": "hvm",
> +"name": "XenGuest2",
> +"uuid": "c7a5fdb2-cdaf-9455-926a-d65c16db1809"
> +},
> +"b_info": {
> +"max_vcpus": 1,
> +"avail_vcpus": [
> +0
> +],
> +"max_memkb": 592896,
> +"target_memkb": 403456,
> +"video_memkb": 8192,
> +"shadow_memkb": 5656,
> +"cpuid": [
> +{
> +"leaf": 1,
> +"ecx": "xxx0",
> +"edx": "xxx1"
> +}


Since Xen lets you specify raw  "cpuid" register values here, surely
this is flexible enough to allow us to support the mode=custom CPU
models ?

We would just need to make sure every bit poisition used either
0 or 1, and not 'x', so that we are fully overriding whatever
defaults are presented by the hypervisor "host" CPU model. Or is
life more complicated than that ?


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] [PATCH v5 6/9] libxl: add support for CPUID features policy

[Daniel P . Berrangé]

On Wed, Mar 14, 2018 at 03:26:13AM +0100, Marek Marczykowski-Górecki wrote:
> Convert CPU features policy into libxl cpuid policy settings. Use new
> ("libxl") syntax, which allow to enable/disable specific bits, using
> host CPU as a base. For this reason, only "host-passthrough" mode is
> accepted.
> Libxl do not have distinction between "force" and "required" policy
> (there is only "force") and also between "forbid" and "disable" (there
> is only "disable"). So, merge them appropriately. If anything, "require"
> and "forbid" should be enforced outside of specific driver.
> Nested HVM (vmx and svm features) is handled separately, so exclude it
> from translation.
> 
> Signed-off-by: Marek Marczykowski-Górecki 
> ---
> Changes since v4:
>  - added spec-ctrl/ibrsb
> Changes since v2:
>  - drop spurious changes
>  - move libxlTranslateCPUFeature function to xen_xl.c, to be reused by
>  xenconfig driver
> Changes since v1:
>  - use new ("libxl") syntax to set only bits explicitly mentioned in
>  domain XML
> ---
>  src/libxl/libxl_conf.c | 36 +---
>  src/xenconfig/xen_xl.c | 35 +++
>  src/xenconfig/xen_xl.h |  2 ++
>  3 files changed, 70 insertions(+), 3 deletions(-)

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

Re: [libvirt] [PATCH v5 4/9] libxl: do not enable nested HVM unless global nested_hvm option enabled

[Daniel P . Berrangé]

On Wed, Mar 14, 2018 at 03:26:11AM +0100, Marek Marczykowski-Górecki wrote:
> Introduce global libxl option for enabling nested HVM feature, similar
> to kvm module parameter. This will prevent enabling experimental feature
> by mere presence of  element in domain
> config, unless explicitly enabled.  element
> may be used to configure other features, like NUMA, or CPUID.
> 
> Signed-off-by: Marek Marczykowski-Górecki 
> ---
> Changes since v4:
>  - add nested_hvm option to test_libvirtd_libxl.aug.in and libvirtd_libxl.aug
>  - make it possible to override nested_hvm=0 with explicit policy='require' name='vmx'/>
>  - split xenconfig changes into separate commits
> Changes since v3:
>  - use config option nested_hvm, instead of requiring explicit ...> entries
>  - title changed from "libxl: do not enable nested HVM by mere presence
>of  element"
>  - xenconfig: don't add  since it is
>implied by presence of  element
>  - xenconfig: produce  element even when converting on host not
>supporting vmx/svm, to not lose setting value
> Changes since v2:
>  - new patch
> ---
>  src/libxl/libvirtd_libxl.aug |  2 ++
>  src/libxl/libxl.conf |  8 
>  src/libxl/libxl_conf.c   | 12 +++-
>  src/libxl/libxl_conf.h   |  2 ++
>  src/libxl/test_libvirtd_libxl.aug.in |  1 +
>  tests/libxlxml2domconfigtest.c   |  3 +++
>  6 files changed, 27 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

Re: [libvirt] [PATCH v5 3/9] libxl: error out on not supported CPU mode, instead of silently ignoring

[Daniel P . Berrangé]

On Wed, Mar 14, 2018 at 03:26:10AM +0100, Marek Marczykowski-Górecki wrote:
> This change make libvirt XML with plain  element invalid for libxl,
> which affect not only upcoming CPUID support, but also NUMA. In fact,
> default mode 'custom' does not match what the driver actually does, so
> it was a bug. Adjust xenconfig driver accordingly.
> To not break existing configuration, add PostParse hook to update
> (previously ignored) default mode 'custom' to 'host-passthrough'.

Maybe I'm missing something, but by doing this silent conversion
from custom -> host-passthrough, don't you make it such that the
error about unsupported CPU mode is largely unreachable ? IOW seems
to end up with the same functional result as the original code,
except for a error when 'host-model' is used.

I don't have a particular better idea though if we have alot of
pre-existing usage with mode=custom ?  Has this been widely
done, or can we justify breaking it ?

> 
> Signed-off-by: Marek Marczykowski-Górecki 
> ---
> Changes since v4:
>  - add PostParse hook to automatically set host-passthrough mode, if
>was the default one before (custom)
> Changes since v3:
>  - fix vnuma tests (nested HVM implicitly enabled there)
> Changes since v2:
>  - change separated from 'libxl: add support for CPUID features policy'
> ---
>  src/libxl/libxl_conf.c  | 10 --
>  src/libxl/libxl_domain.c|  5 +-
>  src/xenconfig/xen_xl.c  |  1 +-
>  tests/xlconfigdata/test-fullvirt-vnuma-autocomplete.cfg |  1 +-
>  tests/xlconfigdata/test-fullvirt-vnuma-autocomplete.xml |  2 +-
>  tests/xlconfigdata/test-fullvirt-vnuma-nodistances.cfg  |  1 +-
>  tests/xlconfigdata/test-fullvirt-vnuma-nodistances.xml  |  2 +-
>  tests/xlconfigdata/test-fullvirt-vnuma-partialdist.cfg  |  1 +-
>  tests/xlconfigdata/test-fullvirt-vnuma-partialdist.xml  |  2 +-
>  tests/xlconfigdata/test-fullvirt-vnuma.cfg  |  1 +-
>  tests/xlconfigdata/test-fullvirt-vnuma.xml  |  2 +-
>  11 files changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index e7727a1..9301731 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -355,11 +355,17 @@ libxlMakeDomBuildInfo(virDomainDefPtr def,
>def->features[VIR_DOMAIN_FEATURE_ACPI] ==
>VIR_TRISTATE_SWITCH_ON);
>  
> -if (caps &&
> -def->cpu && def->cpu->mode == (VIR_CPU_MODE_HOST_PASSTHROUGH)) {
> +if (caps && def->cpu) {
>  bool hasHwVirt = false;
>  bool svm = false, vmx = false;
>  
> +if (def->cpu->mode != VIR_CPU_MODE_HOST_PASSTHROUGH) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +   _("unsupported cpu mode '%s'"),
> +   virCPUModeTypeToString(def->cpu->mode));
> +return -1;
> +}
> +
>  if (ARCH_IS_X86(def->os.arch)) {
>  vmx = virCPUCheckFeature(caps->host.arch, caps->host.cpu, 
> "vmx");
>  svm = virCPUCheckFeature(caps->host.arch, caps->host.cpu, 
> "svm");
> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
> index 8879481..98da68d 100644
> --- a/src/libxl/libxl_domain.c
> +++ b/src/libxl/libxl_domain.c
> @@ -414,6 +414,11 @@ libxlDomainDefPostParse(virDomainDefPtr def,
>  def->features[VIR_DOMAIN_FEATURE_ACPI] = VIR_TRISTATE_SWITCH_ON;
>  }
>  
> +/* set default CPU mode to host-passthrough, as the only one supported by
> + * the driver */
> +if (def->cpu && def->cpu->mode == VIR_CPU_MODE_CUSTOM)
> +def->cpu->mode = VIR_CPU_MODE_HOST_PASSTHROUGH;
> +
>  return 0;
>  }
>  
> diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c
> index 2ef80eb..6cda305 100644
> --- a/src/xenconfig/xen_xl.c
> +++ b/src/xenconfig/xen_xl.c
> @@ -494,6 +494,7 @@ xenParseXLVnuma(virConfPtr conf,
>  goto cleanup;
>  }
>  
> +cpu->mode = VIR_CPU_MODE_HOST_PASSTHROUGH;
>  cpu->type = VIR_CPU_TYPE_GUEST;
>  def->cpu = cpu;
>  
> diff --git a/tests/xlconfigdata/test-fullvirt-vnuma-autocomplete.cfg 
> b/tests/xlconfigdata/test-fullvirt-vnuma-autocomplete.cfg
> index edba69a..2c9de44 100644
> --- a/tests/xlconfigdata/test-fullvirt-vnuma-autocomplete.cfg
> +++ b/tests/xlconfigdata/test-fullvirt-vnuma-autocomplete.cfg
> @@ -22,5 +22,6 @@ parallel = "none"
>  serial = "none"
>  builder = "hvm"
>  boot = "d"
> +nestedhvm = 1
>  vnuma = [ [ "pnode=0", "size=2048", "vcpus=0,11", 
> "vdistances=10,21,31,41,51,61" ], [ "pnode=1", "size=2048", "vcpus=1,10", 
> "vdistances=21,10,21,31,41,51" ], [ "pnode=2", "size=2048", "vcpus=2,9", 
> "vdistances=31,21,10,21,31,41" ], [ "pnode=3", "size=2048", "vcpus=3,8", 
> "vdistances=41,31,21,10,21,31" ], [ "pnode=4", "size=2048", "vcpus=4,7", 
> 

Re: [libvirt] [PATCH v5 5/9] xenconfig: do not override def->cpu if already set elsewhere

[Daniel P . Berrangé]

On Wed, Mar 14, 2018 at 03:26:12AM +0100, Marek Marczykowski-Górecki wrote:
> This will help with adding cpuid support.
> 
> Signed-off-by: Marek Marczykowski-Górecki 
> ---
> Changes since v4:
>  - patch separated from "libxl: do not enable nested HVM unless global
>nested_hvm option enabled"
> ---
>  src/xenconfig/xen_xl.c | 37 -
>  1 file changed, 12 insertions(+), 25 deletions(-)

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

Re: [libvirt] [PATCH v5 2/9] libxl: pass driver config to libxlMakeDomBuildInfo

[Daniel P . Berrangé]

On Wed, Mar 14, 2018 at 03:26:09AM +0100, Marek Marczykowski-Górecki wrote:
> Preparation for global nestedhvm configuration - libxlMakeDomBuildInfo
> needs access to libxlDriverConfig.
> No functional change.
> 
> Adjusting tests require slightly more mockup functions, because of
> libxlDriverConfigNew() call.
> 
> Signed-off-by: Marek Marczykowski-Górecki 
> ---
> Changes since v4:
>  - drop now unneeded parameters
> Changes since v3:
>  - new patch, preparation
> ---
>  src/libxl/libxl_conf.c | 13 +++--
>  src/libxl/libxl_conf.h |  4 +---
>  src/libxl/libxl_domain.c   |  2 +-
>  tests/libxlxml2domconfigtest.c | 23 ---
>  tests/virmocklibxl.c   | 25 +
>  5 files changed, 50 insertions(+), 17 deletions(-)

Reviewed-by: Daniel P. Berrangé 

Though one question...

> @@ -48,6 +49,19 @@ VIR_MOCK_IMPL_RET_ARGS(xc_interface_open,
>  }
>  
>  
> +VIR_MOCK_IMPL_RET_ARGS(libxl_get_version_info,
> +   const libxl_version_info*,
> +   libxl_ctx *, ctx)
> +{
> +static libxl_version_info info;
> +
> +memset(, 0, sizeof(info));
> +
> +return 
> +/* silence gcc warning */
> +return real_libxl_get_version_info(ctx);

Why was gcc warning about that requires the second return
statement ?  I would have though this would /cause/ a
warning by creating unreachable code ?


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] [PATCH 1/4] libxl: MigrateBegin: Dont call EndAPI in helper function

[Jim Fehlig]

On 03/14/2018 06:43 AM, John Ferlan wrote:



On 03/13/2018 01:26 PM, Jim Fehlig wrote:

The libxlDomainMigrateBegin3Params API locks and ref counts the associated
virDomainObj but relies on the helper function libxlDomainMigrationBegin
to unref/unlock the object. libxlDomainMigrationBegin is also used by
libxlDomainMigratePerform3Params for p2p migration, but in that case the
lock/ref and unref/unlock are properly handled in the API entry point. So
p2p migrations suffer a double unref/unlock in the Perform API.

Remove the unref/unlock (virDomainObjEndAPI) from libxlDomainMigrationBegin
and adjust libxlDomainMigrateBegin3Params to properly unref/unlock
the virDomainObj on success and error paths.

Signed-off-by: Jim Fehlig 
---
  src/libxl/libxl_driver.c| 24 +---
  src/libxl/libxl_migration.c |  1 -
  2 files changed, 13 insertions(+), 12 deletions(-)



Reviewed-by: John Ferlan 


Thanks for reviewing the series!


BTW: Outside the scope of this series; however, danpb went through the
painstaking task of modifying names of qemu_migration API's such that
it's easier to determine if the API was run on qemuMigrationSrc or
qemuMigrationDst - made it so much easier to remember which was which
w/r/t the various stages - you may want to consider doing that too for
libxl!


Good point. I'm familiar with the code and still find myself thinking about 
which phase pertains to the src and which to the dest.


Regards,
Jim

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

Re: [libvirt] [PATCH v5 1/9] libxl: fix libxlDriverConfigDispose for partially constructed object

[Daniel P . Berrangé]

On Wed, Mar 14, 2018 at 03:26:08AM +0100, Marek Marczykowski-Górecki wrote:
> libxlDriverConfigNew() use libxlDriverConfigDispose() for cleanup in
> case of errors. Do not call libxlLoggerFree() on not allocated logger
> (NULL).
> 
> Signed-off-by: Marek Marczykowski-Górecki 
> Reviewed-by: Jim Fehlig 
> ---
> Changes since v3:
>  - new patch, mostly unrelated, but found while adjusting tests
> ---
>  src/libxl/libxl_conf.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé 


> 
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index 970cff2..2d2a707 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -80,7 +80,8 @@ libxlDriverConfigDispose(void *obj)
>  
>  virObjectUnref(cfg->caps);
>  libxl_ctx_free(cfg->ctx);
> -libxlLoggerFree(cfg->logger);
> +if (cfg->logger)
> +libxlLoggerFree(cfg->logger);
>  
>  VIR_FREE(cfg->configDir);
>  VIR_FREE(cfg->autostartDir);
> -- 
> git-series 0.9.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

[libvirt] [PATCH] gnulib: switch to use https:// instead of git:// protocol

[Daniel P . Berrangé]

Some contributors are behind obnoxious firewalls that block everything
except http(s) traffic, preventing checkout of modules using the git://
protocol. Since git.savannah.gnu.org is using the modern, fast HTTP
transport, there's no real downside to using that by default.

Signed-off-by: Daniel P. Berrangé 
---

AFAICT, this change has no effect for anyone with an existing checkout
of gnulib. They'll continue using the git protocol, just new checkouts
will start using http. This is fine so no effort is made to "fix"
existing checkouts.

 .gitmodules | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/.gitmodules b/.gitmodules
index cfee24d7b8..0fda887528 100644
--- a/.gitmodules
+++ b/.gitmodules
@@ -1,6 +1,6 @@
 [submodule "gnulib"]
path = .gnulib
-   url = git://git.sv.gnu.org/gnulib.git
+   url = https://git.savannah.gnu.org/git/gnulib.git/
 [submodule "keycodemapdb"]
path = src/keycodemapdb
url = https://gitlab.com/keycodemap/keycodemapdb.git
-- 
2.14.3

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

Re: [libvirt] [PATCH v2 2/2] qemu: hostdev: Fix the error on VM start with an mdev when IOMMU is off

[Peter Krempa]

On Fri, Mar 16, 2018 at 13:37:54 +0100, Erik Skultety wrote:
> Commit b4c2ac8d56 made a false assumption that IOMMU support necessary
> for an mdev device to be assigned to a VM. Unlike direct PCI assignment,
> IOMMU support is not needed for mediated devices, as the physical parent
> device provides the IOMMU isolation, therefore, simply checking for VFIO

I'd drop the word IOMMU here.

> presence is enough to successfully start a VM.
> Luckily, this issue is not serious, since as of yet, libvirt mandates
> mdevs to be pre-created prior to a domain's launch - if it is,
> everything does work smoothly, because the parent device will ensure the
> iommu groups we try to access exist. However, if the mdev didn't exist,
> one would see the following error:

That is true only if the mdev does not exist (since it creates an
iommu/isolation group) and also no other iommu groups are present on the
host.

If there are any other iommu group then qemuHostdevHostSupportsPassthroughVFIO
will return true and you'll still get the proper error that you report
below.

> "unsupported configuration: Mediated host device assignment requires VFIO
> support"

In that case this error would be wrong.

> 
> The error msg above is simply wrong and doesn't even reflect the IOMMU
> reality, so after applying this patch one would rather hit the following
> error:
> 
> "device not found: mediated device '' not found"

This is okay as long as you point out that the above case would happen
only if no other iommu groups were present on the host.

> 
> Signed-off-by: Erik Skultety 
> ---
>  src/qemu/qemu_hostdev.c | 7 ++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
> index 73d26f4c6..afe445d4e 100644
> --- a/src/qemu/qemu_hostdev.c
> +++ b/src/qemu/qemu_hostdev.c
> @@ -330,9 +330,14 @@ qemuHostdevPrepareMediatedDevices(virQEMUDriverPtr 
> driver,
>int nhostdevs)
>  {
>  virHostdevManagerPtr hostdev_mgr = driver->hostdevMgr;
> -bool supportsVFIO = qemuHostdevHostSupportsPassthroughVFIO();
> +bool supportsVFIO;
>  size_t i;
> 
> +/* Checking for VFIO only is fine with mdev, as IOMMU isolation is 
> achieved
> + * by the physical parent device.
> + */
> +supportsVFIO = virFileExists("/dev/vfio/vfio");
> +
>  for (i = 0; i < nhostdevs; i++) {
>  if (hostdevs[i]->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
>  hostdevs[i]->source.subsys.type == 
> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV) {

ACK with the reworded commit message

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


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

[libvirt] [PATCH] docs: mention viewing security notices on front page

[Daniel P . Berrangé]

Signed-off-by: Daniel P. Berrangé 
---

Pushed as a trivial docs change after suggestion by Roman in other review

 docs/index.html.in | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/index.html.in b/docs/index.html.in
index c0c55cb148..1b3a7a3db6 100644
--- a/docs/index.html.in
+++ b/docs/index.html.in
@@ -52,7 +52,7 @@
 Get involved in the libvirt community  student outreach 
programs
 
 Security vulnerabilities
-Report vulnerabilities to the libvirt security response team
+View security notices and report vulnerabilities to the libvirt 
security response team
 
 Bug reporting
 View and report bugs in libvirt packages
-- 
2.14.3

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

[libvirt] [PATCH] qemu: add virQEMUBuildBufferEscapeComma in qemu_command.c

[Sukrit Bhatnagar]

This patch adds virQEMUBuildBufferEscapeComma wherever applicable in 
src/qemu/qemu_command.c
Based on: 
https://wiki.libvirt.org/page/BiteSizedTasks#qemu:_Use_comma_escaping_for_more_command_line_values

Signed-off-by: Sukrit Bhatnagar 
---


This patch is submitted towards my proposal for GSoC'18.

Changes made:
- info->romfile in qemuBuildRomStr
- disk->vendor, disk->product in qemuBuildDriveDevStr
- fs->src->path in qemuBuildFSStr
- fs->dst in qemuBuildFSDevStr
- connect= in qemuBuildHostNetStr
- fileval handling in qemuBuildChrChardevStr
- TYPE_DEV, TYPE_PIPE handling in qemuBuildChrChardevStr
- cfg->vncTLSx509certdir in qemuBuildGraphicsVNCCommandLine
- cfg->spiceTLSx509certdir in qemuBuildGraphicsSPICECommandLine
- loader->path, loader->nvram usage qemuBuildDomainLoaderCommandLine

Places where no changes were made:
- src->hosts->socket in qemuBuildNetworkDriveURI uses virAsprintf not 
virBufferAsprintf; src->path, src->configFile are not used
- qemuBuildChrArgStr function does not exist
- not applicable on data.nix.path in qemuBuildVhostuserCommandLine
- converting places that use strchr in qemuBuildSmartcardCommandLine to use 
virBufferEscape

I have run `make check VIR_TEST_EXPENSIVE=1`, `make syntax-check` and `make -C 
tests valgrind`.
Some tests fail on my system, even for an unmodified clone of the repo.
But, all the tests which were passed by the unmodified clone were passed after 
I made these changes.

As always, your feedback is welcome!


src/qemu/qemu_command.c | 73 +
 1 file changed, 44 insertions(+), 29 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index fa0aa5d5c..06f4f72fc 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -474,8 +474,10 @@ qemuBuildRomStr(virBufferPtr buf,
 default:
 break;
 }
-if (info->romfile)
-   virBufferAsprintf(buf, ",romfile=%s", info->romfile);
+if (info->romfile) {
+virBufferAddLit(buf, ",romfile=");
+virQEMUBuildBufferEscapeComma(buf, info->romfile);
+}
 }
 return 0;
 }
@@ -2177,11 +2179,15 @@ qemuBuildDriveDevStr(const virDomainDef *def,
 virBufferAsprintf(, ",wwn=0x%s", disk->wwn);
 }
 
-if (disk->vendor)
-virBufferAsprintf(, ",vendor=%s", disk->vendor);
+if (disk->vendor) {
+virBufferAddLit(, ",vendor=");
+virQEMUBuildBufferEscapeComma(, disk->vendor);
+}
 
-if (disk->product)
-virBufferAsprintf(, ",product=%s", disk->product);
+if (disk->product) {
+virBufferAddLit(, ",product=");
+virQEMUBuildBufferEscapeComma(, disk->product);
+}
 
 if (disk->bus == VIR_DOMAIN_DISK_BUS_USB) {
 if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_USB_STORAGE_REMOVABLE)) {
@@ -2418,7 +2424,8 @@ qemuBuildFSStr(virDomainFSDefPtr fs,
 }
 
 virBufferAsprintf(, ",id=%s%s", QEMU_FSDEV_HOST_PREFIX, 
fs->info.alias);
-virBufferAsprintf(, ",path=%s", fs->src->path);
+virBufferAddLit(, ",path=");
+virQEMUBuildBufferEscapeComma(, fs->src->path);
 
 if (fs->readonly) {
 if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_FSDEV_READONLY)) {
@@ -2463,7 +2470,8 @@ qemuBuildFSDevStr(const virDomainDef *def,
 virBufferAsprintf(, ",id=%s", fs->info.alias);
 virBufferAsprintf(, ",fsdev=%s%s",
   QEMU_FSDEV_HOST_PREFIX, fs->info.alias);
-virBufferAsprintf(, ",mount_tag=%s", fs->dst);
+virBufferAddLit(, ",mount_tag=");
+virQEMUBuildBufferEscapeComma(, fs->dst);
 
 if (qemuBuildVirtioOptionsStr(, fs->virtio, qemuCaps) < 0)
 goto error;
@@ -3603,10 +3611,9 @@ qemuBuildHostNetStr(virDomainNetDefPtr net,
 break;
 
 case VIR_DOMAIN_NET_TYPE_CLIENT:
-virBufferAsprintf(, "socket%cconnect=%s:%d,",
-  type_sep,
-  net->data.socket.address,
-  net->data.socket.port);
+virBufferAsprintf(, "socket%cconnect=", type_sep);
+virQEMUBuildBufferEscapeComma(, net->data.socket.address);
+virBufferAsprintf(, ":%d,", net->data.socket.port);
 break;
 
 case VIR_DOMAIN_NET_TYPE_SERVER:
@@ -4858,7 +4865,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));
@@ -4916,9 +4924,10 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager,
 break;
 
 case VIR_DOMAIN_CHR_TYPE_DEV:
-virBufferAsprintf(, "%s,id=%s,path=%s",
+

Re: [libvirt] [PATCH] qemu: Build usb controller command line more wisely

[Ján Tomko]

On Fri, Mar 16, 2018 at 04:38:14PM +0100, Michal Privoznik wrote:

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

When building command line for USB controllers we have to do more
than just put controller's alias onto the command line. QEMU has
concept of these joined USB controllers. For instance ehci and
uhci controllers need to create the same USB bus. To achieve that
the slave controller needs to refer the master controller. This
worked until we've introduced user aliases because both master
and slave had the same alias. With user aliases slave can have
different alias than master. Therefore, when generating command
line for slave we need to look up the master's alias.

Signed-off-by: Michal Privoznik 
---
src/qemu/qemu_command.c  | 44 ++--
tests/qemuxml2argvdata/user-aliases-usb.args | 38 ++
tests/qemuxml2argvdata/user-aliases-usb.xml  | 78 
tests/qemuxml2argvtest.c |  3 ++
4 files changed, 158 insertions(+), 5 deletions(-)
create mode 100644 tests/qemuxml2argvdata/user-aliases-usb.args
create mode 100644 tests/qemuxml2argvdata/user-aliases-usb.xml



ACK

Jan


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

[libvirt] [PATCH] qemu: Build usb controller command line more wisely

[Michal Privoznik]

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

When building command line for USB controllers we have to do more
than just put controller's alias onto the command line. QEMU has
concept of these joined USB controllers. For instance ehci and
uhci controllers need to create the same USB bus. To achieve that
the slave controller needs to refer the master controller. This
worked until we've introduced user aliases because both master
and slave had the same alias. With user aliases slave can have
different alias than master. Therefore, when generating command
line for slave we need to look up the master's alias.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_command.c  | 44 ++--
 tests/qemuxml2argvdata/user-aliases-usb.args | 38 ++
 tests/qemuxml2argvdata/user-aliases-usb.xml  | 78 
 tests/qemuxml2argvtest.c |  3 ++
 4 files changed, 158 insertions(+), 5 deletions(-)
 create mode 100644 tests/qemuxml2argvdata/user-aliases-usb.args
 create mode 100644 tests/qemuxml2argvdata/user-aliases-usb.xml

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index fa0aa5d5c3..a8afbd14fa 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -2545,8 +2545,34 @@ qemuControllerModelUSBToCaps(int model)
 }
 
 
+static const char *
+qemuBuildUSBControllerFindMasterAlias(const virDomainDef *domainDef,
+  const virDomainControllerDef *def)
+{
+size_t i;
+
+for (i = 0; i < domainDef->ncontrollers; i++) {
+const virDomainControllerDef *tmp = domainDef->controllers[i];
+
+if (tmp->type != VIR_DOMAIN_CONTROLLER_TYPE_USB)
+continue;
+
+if (tmp->idx != def->idx)
+continue;
+
+if (tmp->info.mastertype == VIR_DOMAIN_CONTROLLER_MASTER_USB)
+continue;
+
+return tmp->info.alias;
+}
+
+return NULL;
+}
+
+
 static int
-qemuBuildUSBControllerDevStr(virDomainControllerDefPtr def,
+qemuBuildUSBControllerDevStr(const virDomainDef *domainDef,
+ virDomainControllerDefPtr def,
  virQEMUCapsPtr qemuCaps,
  virBuffer *buf)
 {
@@ -2586,11 +2612,19 @@ qemuBuildUSBControllerDevStr(virDomainControllerDefPtr 
def,
   def->opts.usbopts.ports, def->opts.usbopts.ports);
 }
 
-if (def->info.mastertype == VIR_DOMAIN_CONTROLLER_MASTER_USB)
+if (def->info.mastertype == VIR_DOMAIN_CONTROLLER_MASTER_USB) {
+const char *masterbus;
+
+if (!(masterbus = qemuBuildUSBControllerFindMasterAlias(domainDef, 
def))) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("masterbus not found"));
+return -1;
+}
 virBufferAsprintf(buf, ",masterbus=%s.0,firstport=%d",
-  def->info.alias, def->info.master.usb.startport);
-else
+  masterbus, def->info.master.usb.startport);
+} else {
 virBufferAsprintf(buf, ",id=%s", def->info.alias);
+}
 
 return 0;
 }
@@ -2722,7 +2756,7 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef,
 break;
 
 case VIR_DOMAIN_CONTROLLER_TYPE_USB:
-if (qemuBuildUSBControllerDevStr(def, qemuCaps, ) == -1)
+if (qemuBuildUSBControllerDevStr(domainDef, def, qemuCaps, ) == -1)
 goto error;
 
 if (nusbcontroller)
diff --git a/tests/qemuxml2argvdata/user-aliases-usb.args 
b/tests/qemuxml2argvdata/user-aliases-usb.args
new file mode 100644
index 00..3dfaadc33b
--- /dev/null
+++ b/tests/qemuxml2argvdata/user-aliases-usb.args
@@ -0,0 +1,38 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu-system-x86_64 \
+-name gentoo \
+-S \
+-M pc-i440fx-1.4 \
+-m 4096 \
+-smp 4,sockets=4,cores=1,threads=1 \
+-uuid a75aca4b-a02f-2bcb-4a91-c93cd848c34b \
+-nographic \
+-nodefaults \
+-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-gentoo/monitor.sock,\
+server,nowait \
+-mon chardev=charmonitor,id=monitor,mode=readline \
+-global PIIX4_PM.disable_s3=0 \
+-global PIIX4_PM.disable_s4=0 \
+-boot cd \
+-device ich9-usb-ehci1,id=ua-myUSB1,bus=pci.0,addr=0x4.0x7 \
+-device ich9-usb-uhci1,masterbus=ua-myUSB1.0,firstport=0,bus=pci.0,\
+multifunction=on,addr=0x4 \
+-device ich9-usb-uhci2,masterbus=ua-myUSB1.0,firstport=2,bus=pci.0,\
+addr=0x4.0x1 \
+-device ich9-usb-uhci3,masterbus=ua-myUSB1.0,firstport=4,bus=pci.0,\
+addr=0x4.0x2 \
+-device ich9-usb-ehci1,id=ua-myUSB5,bus=pci.0,addr=0x5.0x7 \
+-device ich9-usb-uhci1,masterbus=ua-myUSB5.0,firstport=0,bus=pci.0,\
+multifunction=on,addr=0x5 \
+-device ich9-usb-uhci2,masterbus=ua-myUSB5.0,firstport=2,bus=pci.0,\
+addr=0x5.0x1 \
+-device ich9-usb-uhci3,masterbus=ua-myUSB5.0,firstport=4,bus=pci.0,\
+addr=0x5.0x2 \
+-device 

Re: [libvirt] [PATCH 7/8] qemu: Implement the HTM pSeries feature

[John Ferlan]

On 03/09/2018 10:07 AM, Andrea Bolognani wrote:
> This is the first in a list of pSeries-specific optional features
> that have recently been introduced in QEMU. Along with the feature
> proper, some generic code that will make it easier to implement
> subsequent features is introduced as well.
> 
> Signed-off-by: Andrea Bolognani 
> ---
>  docs/formatdomain.html.in  |   7 +
>  docs/schemas/domaincommon.rng  |   5 +
>  src/conf/domain_conf.c |  19 +++
>  src/conf/domain_conf.h |   1 +
>  src/qemu/qemu_command.c| 149 
> +
>  src/qemu/qemu_domain.c |  13 ++
>  .../pseries-features-invalid-machine.xml   |   1 +
>  tests/qemuxml2argvdata/pseries-features.args   |   2 +-
>  tests/qemuxml2argvdata/pseries-features.xml|   1 +
>  tests/qemuxml2argvtest.c   |   1 +
>  tests/qemuxml2xmltest.c|   1 +
>  11 files changed, 199 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 6fd2189cd2..51400afa49 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -2057,6 +2057,13 @@
>the attribute is not defined, the hypervisor default will be used.
>Since 3.10.0 (QEMU/KVM only)
>
> +  htm
> +  Configure HTM (Hardware Transational Memory) availability for
> +  pSeries guests. Possible values for the state 
> attribute
> +  are on and off. If the attribute is not
> +  defined, the hypervisor default will be used.
> +  Since 4.2.0 (QEMU/KVM only)
> +  
>vmcoreinfo
>Enable QEMU vmcoreinfo device to let the guest kernel save debug
>details. Since 3.10.0 (QEMU only)
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 8165e699d6..b4143f5bc3 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -4797,6 +4797,11 @@
>
>  
>
> +  
> +
> +  
> +
> +  
>
>  
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 70b19311b4..98897d3a63 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -152,6 +152,7 @@ VIR_ENUM_IMPL(virDomainFeature, VIR_DOMAIN_FEATURE_LAST,
>"ioapic",
>"hpt",
>"vmcoreinfo",
> +  "htm",
>  );
>  
>  VIR_ENUM_IMPL(virDomainCapabilitiesPolicy, 
> VIR_DOMAIN_CAPABILITIES_POLICY_LAST,
> @@ -19297,6 +19298,22 @@ virDomainDefParseXML(xmlDocPtr xml,
>  }
>  break;
>  
> +case VIR_DOMAIN_FEATURE_HTM:
> +if (!(tmp = virXMLPropString(nodes[i], "state"))) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +   _("missing state attribute '%s' of feature 
> '%s'"),
> +   tmp, virDomainFeatureTypeToString(val));
> +goto error;
> +}
> +if ((def->features[val] = virTristateSwitchTypeFromString(tmp)) 
> < 0) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +   _("unknown state attribute '%s' of feature 
> '%s'"),
> +   tmp, virDomainFeatureTypeToString(val));
> +goto error;
> +}
> +VIR_FREE(tmp);
> +break;
> +
>  /* coverity[dead_error_begin] */
>  case VIR_DOMAIN_FEATURE_LAST:
>  break;
> @@ -21384,6 +21401,7 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr 
> src,
>  case VIR_DOMAIN_FEATURE_VMPORT:
>  case VIR_DOMAIN_FEATURE_SMM:
>  case VIR_DOMAIN_FEATURE_VMCOREINFO:
> +case VIR_DOMAIN_FEATURE_HTM:
>  if (src->features[i] != dst->features[i]) {
>  virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> _("State of feature '%s' differs: "
> @@ -26845,6 +26863,7 @@ virDomainDefFormatInternal(virDomainDefPtr def,
>  case VIR_DOMAIN_FEATURE_PVSPINLOCK:
>  case VIR_DOMAIN_FEATURE_VMPORT:
>  case VIR_DOMAIN_FEATURE_SMM:
> +case VIR_DOMAIN_FEATURE_HTM:
>  switch ((virTristateSwitch) def->features[i]) {
>  case VIR_TRISTATE_SWITCH_LAST:
>  case VIR_TRISTATE_SWITCH_ABSENT:
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 5859c8f4b1..b87a9d9de2 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1746,6 +1746,7 @@ typedef enum {
>  VIR_DOMAIN_FEATURE_IOAPIC,
>  VIR_DOMAIN_FEATURE_HPT,
>  VIR_DOMAIN_FEATURE_VMCOREINFO,
> +VIR_DOMAIN_FEATURE_HTM,
>  
>  

Re: [libvirt] [PATCH] docs: link to security.libvirt.org website

[Roman Bogorodskiy]

  Daniel P. Berrangé wrote:

> We forgot to tell anyone that we were publishing security notices
> online at https://security.libvirt.org
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  docs/securityprocess.html.in | 20 +---
>  1 file changed, 13 insertions(+), 7 deletions(-)

Thanks!

I guess it'd be also useful to update the description of the 'Security
vulnerabilities' entry in the 'Quick Links' block on the index page.
Right now it says 'Report vulnerabilities to the libvirt security
response team'. It could be '..., and view existing ones' (with that
probably being a link to security.libvirt.org).

Roman Bogorodskiy


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

Re: [libvirt] [PATCH 6/8] qemu: Add capability for the HTM pSeries feature

[John Ferlan]

On 03/09/2018 10:07 AM, Andrea Bolognani wrote:
> This is the first capability that depends on an object property,
> so we need to introduce a couple new functions at the same time.
> 
> Signed-off-by: Andrea Bolognani 
> ---
>  src/qemu/qemu_capabilities.c   |  38 +
>  src/qemu/qemu_capabilities.h   |   1 +
>  src/qemu/qemu_monitor.c|  13 ++
>  src/qemu/qemu_monitor.h|   3 +
>  src/qemu/qemu_monitor_json.c   |  10 ++
>  src/qemu/qemu_monitor_json.h   |   5 +
>  .../caps_2.12.0-gicv2.aarch64.replies  |  24 ++-
>  .../caps_2.12.0-gicv2.aarch64.xml  |   2 +-
>  .../caps_2.12.0-gicv3.aarch64.replies  |  24 ++-
>  .../caps_2.12.0-gicv3.aarch64.xml  |   2 +-
>  .../qemucapabilitiesdata/caps_2.12.0.ppc64.replies | 170 
> -
>  tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml   |   3 +-
>  .../caps_2.12.0.x86_64.replies |  30 ++--
>  tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml  |   2 +-
>  14 files changed, 289 insertions(+), 38 deletions(-)
> 

In general - looks reasonable - although need to wait for respin once we
create the "more official" 2.12 caps...


> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 83ec8a67d5..0165de0407 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -460,6 +460,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
>"machine.pseries.max-cpu-compat",
>"dump-completed",
>"qom-list-properties",
> +  "machine.pseries.cap-htm",
>  );
>  
>  
> @@ -1950,6 +1951,21 @@ static struct virQEMUCapsObjectTypeProps 
> virQEMUCapsDeviceProps[] = {
>QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE },
>  };
>  
> +/* Object properties.
> + *
> + * The following can be probed using the qom-list-properties QMP command
> + */
> +
> +static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsSPAPRMachine[] = {
> +{ "cap-htm", QEMU_CAPS_MACHINE_PSERIES_CAP_HTM },
> +};
> +
> +static struct virQEMUCapsObjectTypeProps virQEMUCapsObjectProps[] = {
> +{ "spapr-machine", virQEMUCapsObjectPropsSPAPRMachine,
> +  ARRAY_CARDINALITY(virQEMUCapsObjectPropsSPAPRMachine),
> +  -1 },
> +};
> +
>  /* see documentation for virQEMUCapsQMPSchemaGetByPath for the query format 
> */
>  static struct virQEMUCapsStringFlags virQEMUCapsQMPSchemaQueries[] = {
>  { "blockdev-add/arg-type/options/+gluster/debug-level", 
> QEMU_CAPS_GLUSTER_DEBUG_LEVEL},
> @@ -2876,6 +2892,28 @@ virQEMUCapsProbeQMPObjects(virQEMUCapsPtr qemuCaps,
>  virStringListFreeCount(values, nvalues);
>  }
>  
> +/* If the qom-list-properties QMP command is available, then we
> + * can probe objects in addition to devices */
> +if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_QOM_LIST_PROPERTIES)) {
> +for (i = 0; i < ARRAY_CARDINALITY(virQEMUCapsObjectProps); i++) {
> +const char *type = virQEMUCapsObjectProps[i].type;
> +int cap = virQEMUCapsObjectProps[i].capsCondition;
> +
> +if (cap >= 0 && !virQEMUCapsGet(qemuCaps, cap))
> +continue;
> +
> +if ((nvalues = qemuMonitorGetObjectProps(mon,
> + type,
> + )) < 0)
> +return -1;
> +virQEMUCapsProcessStringFlags(qemuCaps,
> +  virQEMUCapsObjectProps[i].nprops,
> +  virQEMUCapsObjectProps[i].props,
> +  nvalues, values);
> +virStringListFreeCount(values, nvalues);
> +}
> +}
> +

Is it worth creating a helper that takes the "virQEMUCapsDeviceProps"
and "virQEMUCapsObjectProps" as input - since this is mostly cut-n-paste
of the same code.  Not required...

John


>  /* Prefer -chardev spicevmc (detected earlier) over -device spicevmc */
>  if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_CHARDEV_SPICEVMC))
>  virQEMUCapsClear(qemuCaps, QEMU_CAPS_DEVICE_SPICEVMC);
> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
> index ce07dfd6b1..62a1138130 100644
> --- a/src/qemu/qemu_capabilities.h
> +++ b/src/qemu/qemu_capabilities.h
> @@ -445,6 +445,7 @@ typedef enum {
>  QEMU_CAPS_MACHINE_PSERIES_MAX_CPU_COMPAT, /* -machine 
> pseries,max-cpu-compat= */
>  QEMU_CAPS_DUMP_COMPLETED, /* DUMP_COMPLETED event */
>  QEMU_CAPS_QOM_LIST_PROPERTIES, /* qom-list-properties QMP command */
> +QEMU_CAPS_MACHINE_PSERIES_CAP_HTM, /* -machine pseries,cap-htm= */
>  
>  QEMU_CAPS_LAST /* this must always be the last item */
>  } virQEMUCapsFlags;
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index 

Re: [libvirt] [PATCH 5/8] qemu: Extract generic part from qemuMonitorJSONGetDeviceProps()

[John Ferlan]

On 03/09/2018 10:07 AM, Andrea Bolognani wrote:
> Querying properties for devices and objects is identical except
> for the specific QMP command that needs to be called.
> 
> Take the existing qemuMonitorJSONGetDeviceProps(), scrub all the
> device-specific parts, thus turning it into the generic helper
> qemuMonitorJSONGetProps(), which is then used to reimplement the
> original function and will be used again once object properties
> are needed.
> 
> Signed-off-by: Andrea Bolognani 
> ---
>  src/qemu/qemu_monitor_json.c | 30 ++
>  1 file changed, 22 insertions(+), 8 deletions(-)
> 
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index eb32811cd1..141873a705 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -6058,9 +6058,11 @@ int qemuMonitorJSONSetObjectProperty(qemuMonitorPtr 
> mon,
>  #undef MAKE_SET_CMD
>  
>  
> -int qemuMonitorJSONGetDeviceProps(qemuMonitorPtr mon,
> -  const char *type,
> -  char ***props)
> +static int
> +qemuMonitorJSONGetProps(qemuMonitorPtr mon,

Is perhaps this name too generic?  How about JSONGetListProps where
currently it's "device-list-properties", but shortly there will be
"qom-list-properties".

With some sort of API name adjustment,

Reviewed-by: John Ferlan 

John


> +const char *type,
> +char ***props,
> +const char *impl)
>  {
>  int ret = -1;
>  virJSONValuePtr cmd;

[...]

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

Re: [libvirt] [PATCH 4/8] qemu: Rename virQEMUCapsObjectProps* -> virQEMUCapsDeviceProps*

[John Ferlan]

On 03/09/2018 10:07 AM, Andrea Bolognani wrote:
> In QOM, all devices are objects, which makes the existing names
> technically correct; however, not all objects are devices, and
> soon we're going to start looking for object properties in
> addition to device properties: the former need to go through a
> different code path, so we need to be able to tell them apart.
> Using more precise names is a good way to achieve that.
> 
> While renaming, hunks are also being moved around a bit: the
> new grouping, too, will make things nicer once we start adding
> support for object properties.
> 
> Signed-off-by: Andrea Bolognani 
> ---
>  src/qemu/qemu_capabilities.c | 239 
> ++-
>  src/qemu/qemu_monitor.c  |   4 +-
>  src/qemu/qemu_monitor.h  |   2 +-
>  src/qemu/qemu_monitor_json.c |   2 +-
>  src/qemu/qemu_monitor_json.h |   2 +-
>  5 files changed, 128 insertions(+), 121 deletions(-)
> 

This is unrelated to the v2.12/capabilities... although I imagine
someone will be cursing at you for the merge conflicts they'll have.

Reviewed-by: John Ferlan 

John

note the minor nit ...


> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index c70bd27f18..83ec8a67d5 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -1559,6 +1559,13 @@ struct virQEMUCapsStringFlags {
>  int flag;
>  };
>  
> +struct virQEMUCapsObjectTypeProps {
> +const char *type;
> +struct virQEMUCapsStringFlags *props;
> +size_t nprops;
> +int capsCondition;
> +};
> +
>  
>  struct virQEMUCapsStringFlags virQEMUCapsCommands[] = {
>  { "system_wakeup", QEMU_CAPS_WAKEUP },
> @@ -1698,14 +1705,21 @@ struct virQEMUCapsStringFlags 
> virQEMUCapsObjectTypes[] = {
>  { "pl011", QEMU_CAPS_DEVICE_PL011 },
>  };
>  
> -static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsVirtioBalloon[] = 
> {
> +/* Device properties.
> + *
> + * The following can be probed either using the device-list-properties
> + * QMP command or, for older QEMU versions, from the help text obtained

s/or,/or/
s/, from/

IOW: QMP command or for older QEMU versions the help text obtained

> + * through the '-device xxx,?' command line option
> + */
> +
> +static struct virQEMUCapsStringFlags virQEMUCapsDevicePropsVirtioBalloon[] = 
> {
>  { "deflate-on-oom", QEMU_CAPS_VIRTIO_BALLOON_AUTODEFLATE },
>  { "disable-legacy", QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY },
>  { "iommu_platform", QEMU_CAPS_VIRTIO_PCI_IOMMU_PLATFORM },
>  { "ats", QEMU_CAPS_VIRTIO_PCI_ATS },
>  };
>  

[...]

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

Re: [libvirt] [PATCH 3/8] qemu: Add capability for qom-list-properties

[John Ferlan]

On 03/09/2018 10:07 AM, Andrea Bolognani wrote:
> Signed-off-by: Andrea Bolognani 
> ---
>  src/qemu/qemu_capabilities.c | 4 +++-
>  src/qemu/qemu_capabilities.h | 1 +
>  tests/qemucapabilitiesdata/caps_2.12.0-gicv2.aarch64.xml | 1 +
>  tests/qemucapabilitiesdata/caps_2.12.0-gicv3.aarch64.xml | 1 +
>  tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml | 1 +
>  tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml| 1 +
>  6 files changed, 8 insertions(+), 1 deletion(-)
> 

Looks like some competition to see who gets the next qemu_capabilities
update flags - I think this is the 3rd or 4th patch I've seen in this
area on the list.

Although I haven't been able to build this series because of the various
conflicts in patch 1 and 2, this looks right...

Still it's not required until patch 6... and it seems we need to wait a
bit for patch 2 to be pushed for the:

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/8] tests: Add capabilities data for QEMU 2.12

[Daniel P . Berrangé]

On Fri, Mar 16, 2018 at 10:33:30AM -0400, John Ferlan wrote:
> 
> 
> On 03/09/2018 10:18 AM, Andrea Bolognani wrote:
> > Signed-off-by: Andrea Bolognani 
> > ---
> >  .../caps_2.12.0-gicv2.aarch64.replies  | 17145 +++
> >  .../caps_2.12.0-gicv2.aarch64.xml  |   318 +
> >  .../caps_2.12.0-gicv3.aarch64.replies  | 17145 +++
> >  .../caps_2.12.0-gicv3.aarch64.xml  |   318 +
> >  .../qemucapabilitiesdata/caps_2.12.0.ppc64.replies | 21091 
> > +++
> >  tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml   |  1093 +
> >  .../caps_2.12.0.x86_64.replies | 19113 
> > +
> >  tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml  |  1270 ++
> >  tests/qemucapabilitiestest.c   | 4 +
> >  9 files changed, 77497 insertions(+)
> >  create mode 100644 
> > tests/qemucapabilitiesdata/caps_2.12.0-gicv2.aarch64.replies
> >  create mode 100644 tests/qemucapabilitiesdata/caps_2.12.0-gicv2.aarch64.xml
> >  create mode 100644 
> > tests/qemucapabilitiesdata/caps_2.12.0-gicv3.aarch64.replies
> >  create mode 100644 tests/qemucapabilitiesdata/caps_2.12.0-gicv3.aarch64.xml
> >  create mode 100644 tests/qemucapabilitiesdata/caps_2.12.0.ppc64.replies
> >  create mode 100644 tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml
> >  create mode 100644 tests/qemucapabilitiesdata/caps_2.12.0.x86_64.replies
> >  create mode 100644 tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml
> > 
> 
> What, not adventurous enough for s390 too?! :-)
> 
> While 2.12 isn't "complete" yet, we may as well update now and then try
> to remember later on when 2.12 finalizes to do the final update. I was
> off trying to recollect the process of generating the files ;-)

We have hit feature freeze for QEMU. A few pull requests are still
pending to be merged, but if you wait until middle of next week,
the capabilities should not change again before release unless
bad bugs are found.

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] [PATCH 2/8] tests: Add capabilities data for QEMU 2.12

[John Ferlan]

On 03/09/2018 10:18 AM, Andrea Bolognani wrote:
> Signed-off-by: Andrea Bolognani 
> ---
>  .../caps_2.12.0-gicv2.aarch64.replies  | 17145 +++
>  .../caps_2.12.0-gicv2.aarch64.xml  |   318 +
>  .../caps_2.12.0-gicv3.aarch64.replies  | 17145 +++
>  .../caps_2.12.0-gicv3.aarch64.xml  |   318 +
>  .../qemucapabilitiesdata/caps_2.12.0.ppc64.replies | 21091 
> +++
>  tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml   |  1093 +
>  .../caps_2.12.0.x86_64.replies | 19113 +
>  tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml  |  1270 ++
>  tests/qemucapabilitiestest.c   | 4 +
>  9 files changed, 77497 insertions(+)
>  create mode 100644 
> tests/qemucapabilitiesdata/caps_2.12.0-gicv2.aarch64.replies
>  create mode 100644 tests/qemucapabilitiesdata/caps_2.12.0-gicv2.aarch64.xml
>  create mode 100644 
> tests/qemucapabilitiesdata/caps_2.12.0-gicv3.aarch64.replies
>  create mode 100644 tests/qemucapabilitiesdata/caps_2.12.0-gicv3.aarch64.xml
>  create mode 100644 tests/qemucapabilitiesdata/caps_2.12.0.ppc64.replies
>  create mode 100644 tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml
>  create mode 100644 tests/qemucapabilitiesdata/caps_2.12.0.x86_64.replies
>  create mode 100644 tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml
> 

What, not adventurous enough for s390 too?! :-)

While 2.12 isn't "complete" yet, we may as well update now and then try
to remember later on when 2.12 finalizes to do the final update. I was
off trying to recollect the process of generating the files ;-)

If you need to do an update w/ latest before pushing this, that's fine.

Doesn't seem like anyone else is putting up the not yet flag, so in
order to make progress...

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCHv2 0/6] Use query-cpus-fast instead of query-cpus

[Viktor Mihajlovski]

On 05.03.2018 12:44, Viktor Mihajlovski wrote:
> The QEMU monitor commmand query-cpus is deprecated starting
> with QEMU 2.12.0 because it can adversely affect the performance of
> a running virtual machine.
> 
> This series enables libvirt to use the new query-cpus-fast interface
> if supported by the local QEMU instance and is required in order
> to support QEMU once the interface has been removed.
> 
[...]
ping (as QEMU has entered soft freeze for 2.12 this week), see also
https://wiki.qemu.org/ChangeLog/2.12#Monitor

-- 
Regards,
 Viktor Mihajlovski

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

Re: [libvirt] [GSoC] Contribution towards GSoC'18 [BiteSizedTasks].

[Martin Kletzander]

On Fri, Mar 16, 2018 at 05:53:01PM +0530, Sukrit Bhatnagar wrote:

Task: Use comma escaping for more command line values in qemu

Added virQEMUBuildBufferEscapeComma wherever applicable in 
src/qemu/qemu_command.c as specified in the Task page.

Places where no changes were made:
- src->hosts->socket in qemuBuildNetworkDriveURI uses virAsprintf not 
virBufferAsprintf


It could utilize a buffer for that, I'm not sure who supplies the
socket, though.


- TYPE_DEV, TYPE_FILE, TYPE_PIPE, TYPE_UNIX not found in qemuBuildChrArgStr


I'm not sure what you mean.


- not applicable on data.nix.path in qemuBuildVhostuserCommandLine


Yeah data.nix.path is not used for the command-line, really, IIUC


- UNCLEAR: places that use strchr in qemuBuildSmartcardCommandLine, can be 
converted to use virBufferEscape



Probably.  The code is from 2011 so we might've just disallowed that
since it won't probably ever happen.


All tests which were passed by an unmodified clone were passed after I made 
these changes.
Once `make syntax-check` gave a false error related to matching of braces in 
if-else.



Hi, cool that you ran the checks, but you missed some things in the
Coding Guidelines, I guess.  Some of the things (like the way the email
is sent -- missing PATCH) would be solved by setting up the git
send-email command to which Laine already very helpfully replied.  So
that could help you.

I believe the syntax-check error is wrong only due to the exact wording,
but if you check how braces are supposed to be used there really is a
syntax error.

Also check how commit messages (and e-mails with patches) are worded.
What is in the email becomes part of the commit message and if you want
to mention anything else, you can do so under ...


Your feedback is welcome :)

Signed-off-by: Sukrit Bhatnagar 
---


... these ^^ three dashes.  Whatever is here does not become part of
the commit message.

Anyway, don't worry about it much, resending and fixing is part of the
whole experience ;)


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

Re: [libvirt] [PATCH 1/8] tests: Rename pseries-features-hpt test

[John Ferlan]

On 03/09/2018 10:07 AM, Andrea Bolognani wrote:
> We're going to use the same test case to exercise all optional
> pSeries features, so a more generic name is needed.
> 
> Signed-off-by: Andrea Bolognani 
> ---
>  .../{pseries-features-hpt.args => pseries-features.args}  | 0
>  .../{pseries-features-hpt.xml => pseries-features.xml}| 0
>  tests/qemuxml2argvtest.c  | 4 
> ++--
>  tests/qemuxml2xmloutdata/pseries-features-hpt.xml | 1 -
>  tests/qemuxml2xmloutdata/pseries-features.xml | 1 +
>  tests/qemuxml2xmltest.c   | 2 +-
>  6 files changed, 4 insertions(+), 4 deletions(-)
>  rename tests/qemuxml2argvdata/{pseries-features-hpt.args => 
> pseries-features.args} (100%)
>  rename tests/qemuxml2argvdata/{pseries-features-hpt.xml => 
> pseries-features.xml} (100%)
>  delete mode 12 tests/qemuxml2xmloutdata/pseries-features-hpt.xml
>  create mode 12 tests/qemuxml2xmloutdata/pseries-features.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 v1 6/7] tests: Add test case for virtio input ccw devices

[John Ferlan]

On 03/08/2018 11:07 AM, Farhan Ali wrote:
> Test for virtio-{keyboard, mouse, tablet}-ccw.
> 
> Signed-off-by: Farhan Ali 
> Signed-off-by: Boris Fiuczynski 
> ---
>  tests/qemuxml2argvdata/input-virtio-ccw.args  | 26 +++
>  tests/qemuxml2argvdata/input-virtio-ccw.xml   | 29 +
>  tests/qemuxml2argvtest.c  |  8 ++
>  tests/qemuxml2xmloutdata/input-virtio-ccw.xml | 36 
> +++
>  tests/qemuxml2xmltest.c   |  9 +++
>  5 files changed, 108 insertions(+)
>  create mode 100644 tests/qemuxml2argvdata/input-virtio-ccw.args
>  create mode 100644 tests/qemuxml2argvdata/input-virtio-ccw.xml
>  create mode 100644 tests/qemuxml2xmloutdata/input-virtio-ccw.xml
> 

Similar to 3/7 - seems reasonable.


John

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

Re: [libvirt] [PATCH v1 7/7] news: Update for virtio-gpu-ccw and virtio input ccw devices

[John Ferlan]

On 03/08/2018 11:07 AM, Farhan Ali wrote:
> Document support for the virtio-gpu-ccw and
> virtio-{keyboard, mouse, tablet}-ccw devices.
> 
> Signed-off-by: Farhan Ali 
> Reviewed-by: Boris Fiuczynski 
> ---
>  docs/news.xml | 10 ++
>  1 file changed, 10 insertions(+)
> 

Yay, Andrea will be happy someone who remembered news.xml ;-)


> diff --git a/docs/news.xml b/docs/news.xml
> index a51ca97..7eb71e3 100644
> --- a/docs/news.xml
> +++ b/docs/news.xml
> @@ -35,6 +35,16 @@
>  
>
>  
> +  
> +
> +  qemu: Provide virtio-gpu-ccw and virtio input ccw support

How about:

 "Provide ccw address support for graphics and input devices"

John

> +
> +
> +  Support the virtio-gpu-ccw device as a video device and
> +  virtio-{keyboard, mouse, tablet}-ccw devices as input devices
> +  on S390.
> +
> +  
>  
>  
>
> 

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

Re: [libvirt] [PATCH v1 5/7] qemu: Add support for virtio input ccw devices

[John Ferlan]

On 03/08/2018 11:07 AM, Farhan Ali wrote:
> QEMU on S390 (since v2.11) can support virtio input ccw devices.
> So build the qemu command line for ccw devices.
> 
> Also introduce capabilities for virtio-{keyboard, mouse, tablet}-ccw
> devices.
> 
> Signed-off-by: Farhan Ali 
> Signed-off-by: Boris Fiuczynski 
> ---
>  docs/formatdomain.html.in|  2 ++
>  src/qemu/qemu_capabilities.c |  8 
>  src/qemu/qemu_capabilities.h |  5 +
>  src/qemu/qemu_command.c  | 14 +++---
>  tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml |  3 +++
>  5 files changed, 29 insertions(+), 3 deletions(-)
> 

Similar to earlier - let's split the "capability" and "functionality"

John

> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 0908709..08dc74b 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -6048,6 +6048,8 @@ qemu-kvm -net nic,model=? /dev/null
>sub-element address which can tie the
>device to a particular PCI
>slot, documented above.
> +  On S390, address can be used to provide a CCW address for
> +  an input device (since 4.2.0).
>  
>For type passthrough, the mandatory sub-element 
> source
>must have an evdev attribute containing the absolute path 
> to the
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 9db4c31..14564e8 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -460,6 +460,11 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
>"machine.pseries.max-cpu-compat",
>"dump-completed",
>"virtio-gpu-ccw",
> +  "virtio-keyboard-ccw",
> +
> +  /* 285 */
> +  "virtio-mouse-ccw",
> +  "virtio-tablet-ccw",
>  );
>  
>  
> @@ -1696,6 +1701,9 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] 
> = {
>  { "isa-serial", QEMU_CAPS_DEVICE_ISA_SERIAL },
>  { "pl011", QEMU_CAPS_DEVICE_PL011 },
>  { "virtio-gpu-ccw", QEMU_CAPS_DEVICE_VIRTIO_GPU_CCW },
> +{ "virtio-keyboard-ccw", QEMU_CAPS_DEVICE_VIRTIO_KEYBOARD_CCW },
> +{ "virtio-mouse-ccw", QEMU_CAPS_DEVICE_VIRTIO_MOUSE_CCW },
> +{ "virtio-tablet-ccw", QEMU_CAPS_DEVICE_VIRTIO_TABLET_CCW },
>  };
>  
>  static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsVirtioBalloon[] = 
> {
> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
> index b4852e5..3f3c29f 100644
> --- a/src/qemu/qemu_capabilities.h
> +++ b/src/qemu/qemu_capabilities.h
> @@ -445,6 +445,11 @@ typedef enum {
>  QEMU_CAPS_MACHINE_PSERIES_MAX_CPU_COMPAT, /* -machine 
> pseries,max-cpu-compat= */
>  QEMU_CAPS_DUMP_COMPLETED, /* DUMP_COMPLETED event */
>  QEMU_CAPS_DEVICE_VIRTIO_GPU_CCW, /* -device virtio-gpu-ccw */
> +QEMU_CAPS_DEVICE_VIRTIO_KEYBOARD_CCW, /* -device virtio-keyboard-ccw */
> +
> +/* 285 */
> +QEMU_CAPS_DEVICE_VIRTIO_MOUSE_CCW, /* -device virtio-mouse-ccw */
> +QEMU_CAPS_DEVICE_VIRTIO_TABLET_CCW, /* -device virtio-tablet-ccw */
>  
>  QEMU_CAPS_LAST /* this must always be the last item */
>  } virQEMUCapsFlags;
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index ba63670..5477e14 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -3921,6 +3921,8 @@ qemuBuildVirtioInputDevStr(const virDomainDef *def,
>  
>  if (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) {
>  suffix = "-pci";
> +} else if (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) {
> +suffix = "-ccw";
>  } else if (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO) 
> {
>  suffix = "-device";
>  } else {
> @@ -3932,7 +3934,9 @@ qemuBuildVirtioInputDevStr(const virDomainDef *def,
>  
>  switch ((virDomainInputType) dev->type) {
>  case VIR_DOMAIN_INPUT_TYPE_MOUSE:
> -if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_MOUSE)) {
> +if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_MOUSE) ||
> +(dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW &&
> + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_MOUSE_CCW))) {
>  virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> _("virtio-mouse is not supported by this QEMU 
> binary"));
>  goto error;
> @@ -3940,7 +3944,9 @@ qemuBuildVirtioInputDevStr(const virDomainDef *def,
>  virBufferAsprintf(, "virtio-mouse%s,id=%s", suffix, 
> dev->info.alias);
>  break;
>  case VIR_DOMAIN_INPUT_TYPE_TABLET:
> -if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_TABLET)) {
> +if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_TABLET) ||
> +(dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW &&
> + !virQEMUCapsGet(qemuCaps, 

Re: [libvirt] [PATCH v1 4/7] qemu: Use correct bus type for input devices

[John Ferlan]

On 03/08/2018 11:07 AM, Farhan Ali wrote:
> commit 7210cef452db 'qemu: build command line for virtio input devices'
> introduced an error, by checking if input bus type is
> VIR_DOMAIN_DISK_BUS_VIRTIO.
> 
> Fix it by using the correct bus type for input devices.
> 
> Signed-off-by: Farhan Ali 
> Reviewed-by: Boris Fiuczynski 
> ---
>  src/qemu/qemu_domain_address.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Nice catch!  Been around awhile too.  Doesn't look like there was a test
to catch it either.

Reviewed-by: John Ferlan 

John

(I'll push this along with patch 1 since it's separable)

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

Re: [libvirt] [PATCH v1 3/7] tests: Add test case for virtio-gpu-ccw

[John Ferlan]

On 03/08/2018 11:07 AM, Farhan Ali wrote:
> A test case to test the virtio-gpu-ccw device.
> 
> Signed-off-by: Farhan Ali 
> Signed-off-by: Boris Fiuczynski 
> ---
>  .../qemuxml2argvdata/video-virtio-gpu-ccw-auto.xml | 18 ++
>  tests/qemuxml2argvdata/video-virtio-gpu-ccw.args   | 25 ++
>  tests/qemuxml2argvdata/video-virtio-gpu-ccw.xml| 35 
>  tests/qemuxml2argvtest.c   |  7 
>  .../video-virtio-gpu-ccw-auto.xml  | 34 +++
>  tests/qemuxml2xmloutdata/video-virtio-gpu-ccw.xml  | 38 
> ++
>  tests/qemuxml2xmltest.c| 14 
>  7 files changed, 171 insertions(+)
>  create mode 100644 tests/qemuxml2argvdata/video-virtio-gpu-ccw-auto.xml
>  create mode 100644 tests/qemuxml2argvdata/video-virtio-gpu-ccw.args
>  create mode 100644 tests/qemuxml2argvdata/video-virtio-gpu-ccw.xml
>  create mode 100644 tests/qemuxml2xmloutdata/video-virtio-gpu-ccw-auto.xml
>  create mode 100644 tests/qemuxml2xmloutdata/video-virtio-gpu-ccw.xml
> 

The tests look OK - could have been merged with the "functionality"
patch, but it's OK that they're separate - as long as they exist it's a
good thing.

I assume the xml2xml output would be "valid" after the "capability"
patch - if so you could extract it into it's own patch if you felt
really compelled to do so.

John

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

Re: [libvirt] [PATCH v1 2/7] qemu: Add support for virtio-gpu-ccw device

[John Ferlan]

On 03/08/2018 11:07 AM, Farhan Ali wrote:
> QEMU on S390 (since v2.11) can support the virtio-gpu-ccw device,
> so on S390 assign CCW address for a video device.
> 
> Also introduce a new capability for the virtio-gpu-ccw
> device.
> 
> Signed-off-by: Farhan Ali 
> Signed-off-by: Boris Fiuczynski 
> ---
>  docs/formatdomain.html.in  |  3 +
>  src/qemu/qemu_capabilities.c   |  5 ++
>  src/qemu/qemu_capabilities.h   |  1 +
>  src/qemu/qemu_command.c| 18 -
>  src/qemu/qemu_domain.c |  2 +-
>  src/qemu/qemu_domain_address.c |  8 +++
>  src/qemu/qemu_process.c|  5 +-
>  .../qemucapabilitiesdata/caps_2.11.0.s390x.replies | 83 
> +++---
>  tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml   |  3 +-
>  9 files changed, 114 insertions(+), 14 deletions(-)
> 

First off - other upstream changes has caused this to not be able to
apply w/ git am -3 - so once patch1 is pushed, you'll need to rework and
repost the series (hopefully this time you won't have the strange split
with a CC address having "--dry-run" appended to an email ;-)

Since I cannot compile this one - I'll just be able to go through
visually and note a few things...  Primarily though - this particular
patch should be split up a bit more.  Separate out the capabilities into
their own separate "capabilities" patch and the second patch becomes a
"functionality" patch.

> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 6fd2189..0908709 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -6484,6 +6484,9 @@ qemu-kvm -net nic,model=? /dev/null
>
>  The optional address sub-element can be used to
>  tie the video device to a particular PCI slot.
> +On S390, address can be used to provide the
> +CCW address for the video device (
> +since 4.2.0).
>
>  
>driver


... The above would go with the "functionality" patch and the next few
with a "capability" patch...

> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index b5eb8cf..9db4c31 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -459,6 +459,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
>"pl011",
>"machine.pseries.max-cpu-compat",
>"dump-completed",
> +  "virtio-gpu-ccw",
>  );
>  
>  
> @@ -1694,6 +1695,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] 
> = {
>  { "sclplmconsole", QEMU_CAPS_DEVICE_SCLPLMCONSOLE },
>  { "isa-serial", QEMU_CAPS_DEVICE_ISA_SERIAL },
>  { "pl011", QEMU_CAPS_DEVICE_PL011 },
> +{ "virtio-gpu-ccw", QEMU_CAPS_DEVICE_VIRTIO_GPU_CCW },
>  };
>  
>  static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsVirtioBalloon[] = 
> {
> @@ -1932,6 +1934,9 @@ static struct virQEMUCapsObjectTypeProps 
> virQEMUCapsObjectProps[] = {
>  { "spapr-pci-host-bridge", virQEMUCapsObjectPropsSpaprPCIHostBridge,
>ARRAY_CARDINALITY(virQEMUCapsObjectPropsSpaprPCIHostBridge),
>QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE },
> +{ "virtio-gpu-ccw", virQEMUCapsObjectPropsVirtioGpu,
> +  ARRAY_CARDINALITY(virQEMUCapsObjectPropsVirtioGpu),
> +  QEMU_CAPS_DEVICE_VIRTIO_GPU_CCW },
>  };
>  
>  struct virQEMUCapsPropTypeObjects {
> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
> index c2ec2be..b4852e5 100644
> --- a/src/qemu/qemu_capabilities.h
> +++ b/src/qemu/qemu_capabilities.h
> @@ -444,6 +444,7 @@ typedef enum {
>  QEMU_CAPS_DEVICE_PL011, /* -device pl011 (not user-instantiable) */
>  QEMU_CAPS_MACHINE_PSERIES_MAX_CPU_COMPAT, /* -machine 
> pseries,max-cpu-compat= */
>  QEMU_CAPS_DUMP_COMPLETED, /* DUMP_COMPLETED event */
> +QEMU_CAPS_DEVICE_VIRTIO_GPU_CCW, /* -device virtio-gpu-ccw */
>  
>  QEMU_CAPS_LAST /* this must always be the last item */
>  } virQEMUCapsFlags;

... Above here with the "capability" patch and below with the
"functionality" patch...

> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index fa0aa5d..ba63670 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -132,7 +132,7 @@ VIR_ENUM_IMPL(qemuDeviceVideoSecondary, 
> VIR_DOMAIN_VIDEO_TYPE_LAST,
>"", /* don't support vbox */
>"qxl",
>"", /* don't support parallels */
> -  "virtio-gpu-pci",
> +  "virtio-gpu",
>"" /* don't support gop */);
>  
>  VIR_ENUM_DECL(qemuSoundCodec)
> @@ -4262,7 +4262,21 @@ qemuBuildDeviceVideoStr(const virDomainDef *def,
>  goto error;
>  }
>  
> -virBufferAsprintf(, "%s,id=%s", model, video->info.alias);
> +if (STREQ(model, "virtio-gpu")) {

Rather than STREQ comparison perhaps:

if 

Re: [libvirt] [PATCH v1 1/7] qemu: Fix comment for 'qemuValidateDevicePCISlotsChipsets'

[John Ferlan]

On 03/08/2018 11:07 AM, Farhan Ali wrote:
> There is no function 'qemuValidateDevicePCISlotsChipsets', it
> should be qemuDomainValidateDevicePCISlotsChipsets.
> 

Commit id '177db487' renamed 'qemuValidateDevicePCISlotsChipsets' to
'qemuDomainValidateDevicePCISlotsChipsets', but didn't adjust comment.

> Signed-off-by: Farhan Ali 
> Reviewed-by: Boris Fiuczynski 
> ---
>  src/qemu/qemu_domain_address.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Reviewed-by: John Ferlan 

John

(I'll push this one shortly)

> diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
> index 3fcb36a..ae49cf2 100644
> --- a/src/qemu/qemu_domain_address.c
> +++ b/src/qemu/qemu_domain_address.c
> @@ -1815,7 +1815,7 @@ 
> qemuDomainValidateDevicePCISlotsChipsets(virDomainDefPtr def,
>   *  - Video (slot 2)
>   *
>   *  - These integrated devices were already added by
> - *qemuValidateDevicePCISlotsChipsets invoked right before this function
> + *qemuDomainValidateDevicePCISlotsChipsets invoked right before this 
> function
>   *
>   * Incrementally assign slots from 3 onwards:
>   *
> 

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

Re: [libvirt] [PATCH v2 1/2] util: mdev: Improve the error msg on non-existent mdev prior to VM start

[Peter Krempa]

On Fri, Mar 16, 2018 at 13:37:53 +0100, Erik Skultety wrote:
> What one currently gets is:
> failed to read '/sys/bus/mdev/devices//mdev_type/device_api': No
> such file or directory
> 
> This indicates that something is missing within the device's sysfs tree
> which likely might be not be the case here because the device simply
> doesn't exist yet. So, when creating our internal mdev obj, let's check
> whether the device exists first prior to trying to verify the
> user-provided model within domain XML.
> 
> Signed-off-by: Erik Skultety 
> ---
>  src/util/virmdev.c | 16 +---
>  1 file changed, 13 insertions(+), 3 deletions(-)

ACK


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

[libvirt] [PATCH v2 2/2] qemu: hostdev: Fix the error on VM start with an mdev when IOMMU is off

[Erik Skultety]

Commit b4c2ac8d56 made a false assumption that IOMMU support necessary
for an mdev device to be assigned to a VM. Unlike direct PCI assignment,
IOMMU support is not needed for mediated devices, as the physical parent
device provides the IOMMU isolation, therefore, simply checking for VFIO
presence is enough to successfully start a VM.
Luckily, this issue is not serious, since as of yet, libvirt mandates
mdevs to be pre-created prior to a domain's launch - if it is,
everything does work smoothly, because the parent device will ensure the
iommu groups we try to access exist. However, if the mdev didn't exist,
one would see the following error:

"unsupported configuration: Mediated host device assignment requires VFIO
support"

The error msg above is simply wrong and doesn't even reflect the IOMMU
reality, so after applying this patch one would rather hit the following
error:

"device not found: mediated device '' not found"

Signed-off-by: Erik Skultety 
---
 src/qemu/qemu_hostdev.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
index 73d26f4c6..afe445d4e 100644
--- a/src/qemu/qemu_hostdev.c
+++ b/src/qemu/qemu_hostdev.c
@@ -330,9 +330,14 @@ qemuHostdevPrepareMediatedDevices(virQEMUDriverPtr driver,
   int nhostdevs)
 {
 virHostdevManagerPtr hostdev_mgr = driver->hostdevMgr;
-bool supportsVFIO = qemuHostdevHostSupportsPassthroughVFIO();
+bool supportsVFIO;
 size_t i;

+/* Checking for VFIO only is fine with mdev, as IOMMU isolation is achieved
+ * by the physical parent device.
+ */
+supportsVFIO = virFileExists("/dev/vfio/vfio");
+
 for (i = 0; i < nhostdevs; i++) {
 if (hostdevs[i]->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
 hostdevs[i]->source.subsys.type == 
VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV) {
--
2.13.6

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

[libvirt] [PATCH v2 1/2] util: mdev: Improve the error msg on non-existent mdev prior to VM start

[Erik Skultety]

What one currently gets is:
failed to read '/sys/bus/mdev/devices//mdev_type/device_api': No
such file or directory

This indicates that something is missing within the device's sysfs tree
which likely might be not be the case here because the device simply
doesn't exist yet. So, when creating our internal mdev obj, let's check
whether the device exists first prior to trying to verify the
user-provided model within domain XML.

Signed-off-by: Erik Skultety 
---
 src/util/virmdev.c | 16 +---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/src/util/virmdev.c b/src/util/virmdev.c
index e4816cf20..27541cf34 100644
--- a/src/util/virmdev.c
+++ b/src/util/virmdev.c
@@ -150,13 +150,22 @@ virMediatedDeviceNew(const char *uuidstr, 
virMediatedDeviceModelType model)
 {
 virMediatedDevicePtr ret = NULL;
 virMediatedDevicePtr dev = NULL;
+char *sysfspath = NULL;
+
+if (!(sysfspath = virMediatedDeviceGetSysfsPath(uuidstr)))
+goto cleanup;
+
+if (!virFileExists(sysfspath)) {
+virReportError(VIR_ERR_DEVICE_MISSING,
+   _("mediated device '%s' not found"), uuidstr);
+goto cleanup;
+}
 
 if (VIR_ALLOC(dev) < 0)
-return NULL;
-
-if (!(dev->path = virMediatedDeviceGetSysfsPath(uuidstr)))
 goto cleanup;
 
+VIR_STEAL_PTR(dev->path, sysfspath);
+
 /* Check whether the user-provided model corresponds with the actually
  * supported mediated device's API.
  */
@@ -167,6 +176,7 @@ virMediatedDeviceNew(const char *uuidstr, 
virMediatedDeviceModelType model)
 VIR_STEAL_PTR(ret, dev);
 
  cleanup:
+VIR_FREE(sysfspath);
 virMediatedDeviceFree(dev);
 return ret;
 }
-- 
2.13.6

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

[libvirt] [PATCH v2 0/2] Fix the odd VFIO error for a non-existent mdev when IOMMU is disabled

[Erik Skultety]

Unlike GPU assignment, mdev doesn't need the IOMMU to be enabled within kernel,
since the physical parent device takes care of the isolation, i.e. there's an
IOMMU group entry for each mdev created under sysfs, thus a VM can start
successfully.

Since v1:
- adjusted the error msg in patch 1 according to reviewer
- rephrased the commit message for patch 2 to make it less confusing

Erik Skultety (2):
  util: mdev: Improve the error msg on non-existent mdev prior to VM
start
  qemu: hostdev: Fix the error on VM start with an mdev when IOMMU is
off

 src/qemu/qemu_hostdev.c |  7 ++-
 src/util/virmdev.c  | 16 +---
 2 files changed, 19 insertions(+), 4 deletions(-)

--
2.13.6

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

[libvirt] [GSoC] Contribution towards GSoC'18 [BiteSizedTasks].

[Sukrit Bhatnagar]

Task: Use comma escaping for more command line values in qemu

Added virQEMUBuildBufferEscapeComma wherever applicable in 
src/qemu/qemu_command.c as specified in the Task page.

Places where no changes were made:
- src->hosts->socket in qemuBuildNetworkDriveURI uses virAsprintf not 
virBufferAsprintf
- TYPE_DEV, TYPE_FILE, TYPE_PIPE, TYPE_UNIX not found in qemuBuildChrArgStr
- not applicable on data.nix.path in qemuBuildVhostuserCommandLine
- UNCLEAR: places that use strchr in qemuBuildSmartcardCommandLine, can be 
converted to use virBufferEscape

All tests which were passed by an unmodified clone were passed after I made 
these changes.
Once `make syntax-check` gave a false error related to matching of braces in 
if-else.

Your feedback is welcome :)

Signed-off-by: Sukrit Bhatnagar 
---
 src/qemu/qemu_command.c | 74 ++---
 1 file changed, 45 insertions(+), 29 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index fa0aa5d5c..beabf8837 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -474,8 +474,10 @@ qemuBuildRomStr(virBufferPtr buf,
 default:
 break;
 }
-if (info->romfile)
-   virBufferAsprintf(buf, ",romfile=%s", info->romfile);
+if (info->romfile) {
+virBufferAddLit(buf, ",romfile=");
+virQEMUBuildBufferEscapeComma(buf, info->romfile);
+}
 }
 return 0;
 }
@@ -2177,11 +2179,15 @@ qemuBuildDriveDevStr(const virDomainDef *def,
 virBufferAsprintf(, ",wwn=0x%s", disk->wwn);
 }
 
-if (disk->vendor)
-virBufferAsprintf(, ",vendor=%s", disk->vendor);
+if (disk->vendor) {
+virBufferAddLit(, ",vendor=");
+virQEMUBuildBufferEscapeComma(, disk->vendor);
+}
 
-if (disk->product)
-virBufferAsprintf(, ",product=%s", disk->product);
+if (disk->product) {
+virBufferAddLit(, ",product=");
+virQEMUBuildBufferEscapeComma(, disk->product);
+}
 
 if (disk->bus == VIR_DOMAIN_DISK_BUS_USB) {
 if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_USB_STORAGE_REMOVABLE)) {
@@ -2418,7 +2424,8 @@ qemuBuildFSStr(virDomainFSDefPtr fs,
 }
 
 virBufferAsprintf(, ",id=%s%s", QEMU_FSDEV_HOST_PREFIX, 
fs->info.alias);
-virBufferAsprintf(, ",path=%s", fs->src->path);
+virBufferAddLit(, ",path=");
+virQEMUBuildBufferEscapeComma(, fs->src->path);
 
 if (fs->readonly) {
 if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_FSDEV_READONLY)) {
@@ -2463,7 +2470,8 @@ qemuBuildFSDevStr(const virDomainDef *def,
 virBufferAsprintf(, ",id=%s", fs->info.alias);
 virBufferAsprintf(, ",fsdev=%s%s",
   QEMU_FSDEV_HOST_PREFIX, fs->info.alias);
-virBufferAsprintf(, ",mount_tag=%s", fs->dst);
+virBufferAddLit(, ",mount_tag=");
+virQEMUBuildBufferEscapeComma(, fs->dst);
 
 if (qemuBuildVirtioOptionsStr(, fs->virtio, qemuCaps) < 0)
 goto error;
@@ -3603,10 +3611,9 @@ qemuBuildHostNetStr(virDomainNetDefPtr net,
 break;
 
 case VIR_DOMAIN_NET_TYPE_CLIENT:
-virBufferAsprintf(, "socket%cconnect=%s:%d,",
-  type_sep,
-  net->data.socket.address,
-  net->data.socket.port);
+virBufferAsprintf(, "socket%cconnect=", type_sep);
+virQEMUBuildBufferEscapeComma(, net->data.socket.address);
+virBufferAsprintf(, ":%d,", net->data.socket.port);
 break;
 
 case VIR_DOMAIN_NET_TYPE_SERVER:
@@ -4858,7 +4865,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));
@@ -4916,9 +4924,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:
@@ -4938,8 +4947,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);
   

Re: [libvirt] [PATCH 1/2] util:mdev: Improve the error msg on non-existent mdev prior to VM start

[Peter Krempa]

On Fri, Mar 16, 2018 at 13:13:41 +0100, Erik Skultety wrote:
> On Fri, Mar 16, 2018 at 12:08:46PM +0100, Peter Krempa wrote:
> > missing space in summary
> >
> > On Fri, Mar 16, 2018 at 11:50:01 +0100, Erik Skultety wrote:
> > > What one currently gets is:
> > > failed to read '/sys/bus/mdev/devices//mdev_type/device_api': No
> > > such file or directory
> > >
> > > This indicates that something is missing within the device's sysfs tree
> > > which likely might be not be the case here because the device simply
> > > doesn't exist yet. So, when creating our internal mdev obj, let's check
> > > whether the device exists first prior to trying to verify the
> > > user-provided model within domain XML.
> > >
> > > Signed-off-by: Erik Skultety 
> > > ---
> > >  src/util/virmdev.c | 16 +---
> > >  1 file changed, 13 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/src/util/virmdev.c b/src/util/virmdev.c
> > > index e4816cf20..2e3769aa6 100644
> > > --- a/src/util/virmdev.c
> > > +++ b/src/util/virmdev.c
> > > @@ -150,13 +150,22 @@ virMediatedDeviceNew(const char *uuidstr, 
> > > virMediatedDeviceModelType model)
> > >  {
> > >  virMediatedDevicePtr ret = NULL;
> > >  virMediatedDevicePtr dev = NULL;
> > > +char *sysfspath = NULL;
> > > +
> > > +if (!(sysfspath = virMediatedDeviceGetSysfsPath(uuidstr)))
> > > +goto cleanup;
> > > +
> > > +if (!virFileExists(sysfspath)) {
> > > +virReportSystemError(errno, _("failed to read device '%s'"),
> > > + sysfspath);
> >
> > You did not try to read the device at this point. It merely does not
> > exist.
> 
> Would you like "device '%s' not found" better?

How about:

"mediated device '%s' not found", uuidstr ?

I don't think that adding the full sysfs path is any helpful since it's
generated from the uuid anyways.


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

Re: [libvirt] [PATCH 1/2] util:mdev: Improve the error msg on non-existent mdev prior to VM start

[Erik Skultety]

On Fri, Mar 16, 2018 at 12:08:46PM +0100, Peter Krempa wrote:
> missing space in summary
>
> On Fri, Mar 16, 2018 at 11:50:01 +0100, Erik Skultety wrote:
> > What one currently gets is:
> > failed to read '/sys/bus/mdev/devices//mdev_type/device_api': No
> > such file or directory
> >
> > This indicates that something is missing within the device's sysfs tree
> > which likely might be not be the case here because the device simply
> > doesn't exist yet. So, when creating our internal mdev obj, let's check
> > whether the device exists first prior to trying to verify the
> > user-provided model within domain XML.
> >
> > Signed-off-by: Erik Skultety 
> > ---
> >  src/util/virmdev.c | 16 +---
> >  1 file changed, 13 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/util/virmdev.c b/src/util/virmdev.c
> > index e4816cf20..2e3769aa6 100644
> > --- a/src/util/virmdev.c
> > +++ b/src/util/virmdev.c
> > @@ -150,13 +150,22 @@ virMediatedDeviceNew(const char *uuidstr, 
> > virMediatedDeviceModelType model)
> >  {
> >  virMediatedDevicePtr ret = NULL;
> >  virMediatedDevicePtr dev = NULL;
> > +char *sysfspath = NULL;
> > +
> > +if (!(sysfspath = virMediatedDeviceGetSysfsPath(uuidstr)))
> > +goto cleanup;
> > +
> > +if (!virFileExists(sysfspath)) {
> > +virReportSystemError(errno, _("failed to read device '%s'"),
> > + sysfspath);
>
> You did not try to read the device at this point. It merely does not
> exist.

Would you like "device '%s' not found" better?

Erik

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

Re: [libvirt] Emacs tip for easily adding Reviewed-by tags & friends

[Martin Kletzander]

On Thu, Mar 15, 2018 at 11:36:28AM +, Daniel P. Berrangé wrote:

It is nice that git has the short-hand for adding Signed-off-by, but
adding other tags during reviews is kind of tedious and long winded.
eg "ACK" is much shorter than typing "Reviewed-by: ...blah blah blah.."

Good editors have a way to setup macros though, and so I thought I'd
share the emacs approach to making life easy again...

In my $HOME/.emacs.d/abbrev_defs file I have this:

(define-abbrev-table 'global-abbrev-table
 '(
   ("8rev" "Reviewed-by: Daniel P. Berrangé " nil 1)
   ("8ack" "Acked-by: Daniel P. Berrangé " nil 1)
   ("8test" "Tested-by: Daniel P. Berrangé " nil 1)
   ("8sob" "Signed-off-by: Daniel P. Berrangé " nil 1)
  ))

Now, if I type the "8rev" [1] and then hit space-bar or enter, emacs expands
it to the full "Reviewed-by: blah blah blah..." line. This makes adding
the full tags just as quick & easy as it was to type a traditional "ACK".

Anyone have an equivalent tip for Vim ?

Perhaps we could add them to the hacking file.



Since we're sharing O:-) I had a script that extracted the mail from the 'From'
mail header, so that you can reply from any address and don't need to
change it.  It also jumps to a newline in case you are not in start of
the line.  It'd be trivial to modify for arbitrary tags, of course.

#+begin_src emacs-lisp
 (defun npx/message-get-from ()
   "Get the address from the 'From' field of the current message"
   (save-excursion
 (goto-char (point-min))
 (let ((beg (search-forward "From: "))
   (end (line-end-position)))
   (buffer-substring-no-properties beg end

 (defun npx/insert-tag-reviewed-by ()
   "Inserts 'Reviewed-by: Real Name ' in the current position in buffer"
   (interactive)
   (let ((from (npx/message-get-from)))
 (when (not (equalp (point) (line-beginning-position)))
   (move-end-of-line nil)
   (newline 2))
 (insert "Reviewed-by: " from)
 (newline)))

 (global-set-key (kbd "C-c r") 'npx/insert-tag-reviewed-by)
#+end_src

HTH,
Martin


Regards,
Daniel

[1] Why the leading "8" you might ask ?  I've no idea. I just got this
   tip from QEMU developers - guess you just want some character seq
   that you're unlikely to want to type for real, to avoid accidental
   expansions.
--
|: 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


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

Re: [libvirt] [PATCH 2/2] qemu: hostdev: Don't error out on domain with an mdev when IOMMU is off

[Erik Skultety]

On Fri, Mar 16, 2018 at 12:07:50PM +0100, Peter Krempa wrote:
> On Fri, Mar 16, 2018 at 11:50:02 +0100, Erik Skultety wrote:
> > Commit b4c2ac8d56 made a false assumption that IOMMU support necessary
> > for an mdev device to be assigned to a VM. Unlike direct PCI assignment,
> > IOMMU support is not needed for mediated devices, as the physical parent
> > device provides the IOMMU isolation, therefore, simply checking for VFIO
> > presence is enough to successfully start a VM.
> > Luckily, this issue is not very serious, since as of yet, libvirt
> > mandates the mdevs to be pre-created prior to a domain's launch, so this
> > patch will merely change the error the end user is going to see.
> >
> > Previously:
> > unsupported configuration: Mediated host device assignment requires VFIO
> > support
> >
> > Now:
> > failed to read device '/sys/bus/mdev/devices//': No such file or
> > directory
>
> I'm not sure I understood what you wanted to say in the commit message.
>
> You are not requiring the IOMMU to bepresent for the mediated device
> since isolation is inherent. This makes sense.
>
> You are then giving an example of error message what would occur if the
> iommu groups are not present on the host.

I should have been more clear about that. So, if you're referencing a
non-existent mdev within the domain XML, the error you get indicates something
that is not true, but you're right in what you deduced that you won't come
across this weird error if the device you're referencing already exists.
Therefore I mentioned in the patch that the issue isn't serious since
everything works if used correctly, only when you don't you'd experience some
odd messages, that's all, I'll rephrase the commit message.

Erik

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

[libvirt] [PATCH] virDomainDeviceDefValidateAliasesIterator: Ignore some hostdevs

[Michal Privoznik]

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

When defining a domain that has  our
parser creates two entries in virDomainDef: one for 
and one for . However, some info is shared between the
two which makes user alias validation fail because alias belongs
to the set of shared info.

Signed-off-by: Michal Privoznik 
---
 src/conf/domain_conf.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 86fc275116..c8d051fa9f 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -5584,6 +5584,13 @@ 
virDomainDeviceDefValidateAliasesIterator(virDomainDefPtr def,
 virDomainChrEquals(def->serials[0], dev->data.chr))
 return 0;
 
+if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV &&
+dev->data.hostdev->parent.type == VIR_DOMAIN_DEVICE_NET) {
+/* This hostdev is a copy of some previous interface.
+ * Aliases are duplicated. */
+return 0;
+}
+
 if (virHashLookup(data->aliases, alias)) {
 virReportError(VIR_ERR_XML_ERROR,
_("non unique alias detected: %s"),
-- 
2.16.1

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

Re: [libvirt] [PATCH 1/2] util:mdev: Improve the error msg on non-existent mdev prior to VM start

[Peter Krempa]

missing space in summary

On Fri, Mar 16, 2018 at 11:50:01 +0100, Erik Skultety wrote:
> What one currently gets is:
> failed to read '/sys/bus/mdev/devices//mdev_type/device_api': No
> such file or directory
> 
> This indicates that something is missing within the device's sysfs tree
> which likely might be not be the case here because the device simply
> doesn't exist yet. So, when creating our internal mdev obj, let's check
> whether the device exists first prior to trying to verify the
> user-provided model within domain XML.
> 
> Signed-off-by: Erik Skultety 
> ---
>  src/util/virmdev.c | 16 +---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/src/util/virmdev.c b/src/util/virmdev.c
> index e4816cf20..2e3769aa6 100644
> --- a/src/util/virmdev.c
> +++ b/src/util/virmdev.c
> @@ -150,13 +150,22 @@ virMediatedDeviceNew(const char *uuidstr, 
> virMediatedDeviceModelType model)
>  {
>  virMediatedDevicePtr ret = NULL;
>  virMediatedDevicePtr dev = NULL;
> +char *sysfspath = NULL;
> +
> +if (!(sysfspath = virMediatedDeviceGetSysfsPath(uuidstr)))
> +goto cleanup;
> +
> +if (!virFileExists(sysfspath)) {
> +virReportSystemError(errno, _("failed to read device '%s'"),
> + sysfspath);

You did not try to read the device at this point. It merely does not
exist.


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

Re: [libvirt] [PATCH 2/2] qemu: hostdev: Don't error out on domain with an mdev when IOMMU is off

[Peter Krempa]

On Fri, Mar 16, 2018 at 11:50:02 +0100, Erik Skultety wrote:
> Commit b4c2ac8d56 made a false assumption that IOMMU support necessary
> for an mdev device to be assigned to a VM. Unlike direct PCI assignment,
> IOMMU support is not needed for mediated devices, as the physical parent
> device provides the IOMMU isolation, therefore, simply checking for VFIO
> presence is enough to successfully start a VM.
> Luckily, this issue is not very serious, since as of yet, libvirt
> mandates the mdevs to be pre-created prior to a domain's launch, so this
> patch will merely change the error the end user is going to see.
> 
> Previously:
> unsupported configuration: Mediated host device assignment requires VFIO
> support
> 
> Now:
> failed to read device '/sys/bus/mdev/devices//': No such file or
> directory

I'm not sure I understood what you wanted to say in the commit message.

You are not requiring the IOMMU to bepresent for the mediated device
since isolation is inherent. This makes sense.

You are then giving an example of error message what would occur if the
iommu groups are not present on the host.

As a 'fixed' result you provide an error message if the user did not
create a MDEV prior to starting the VM. How is that relevant to this
patch? The VM should start just fine in that case, shouldn't it?

> 
> Signed-off-by: Erik Skultety 
> ---
>  src/qemu/qemu_hostdev.c | 7 ++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
> index 73d26f4c6..afe445d4e 100644
> --- a/src/qemu/qemu_hostdev.c
> +++ b/src/qemu/qemu_hostdev.c
> @@ -330,9 +330,14 @@ qemuHostdevPrepareMediatedDevices(virQEMUDriverPtr 
> driver,
>int nhostdevs)
>  {
>  virHostdevManagerPtr hostdev_mgr = driver->hostdevMgr;
> -bool supportsVFIO = qemuHostdevHostSupportsPassthroughVFIO();
> +bool supportsVFIO;
>  size_t i;
> 
> +/* Checking for VFIO only is fine with mdev, as IOMMU isolation is 
> achieved
> + * by the physical parent device.
> + */
> +supportsVFIO = virFileExists("/dev/vfio/vfio");
> +
>  for (i = 0; i < nhostdevs; i++) {
>  if (hostdevs[i]->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
>  hostdevs[i]->source.subsys.type == 
> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV) {
> --
> 2.13.6
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list


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

Re: [libvirt] [PATCH rebase v4 3/5] qemu: introduce qemuARPGetInterfaces to get IP from host's arp table

[John Ferlan]

On 03/08/2018 02:11 AM, Chen Hanxiao wrote:
> From: Chen Hanxiao 
> 
> introduce VIR_DOMAIN_INTERFACE_ADDRESSES_SRC_ARP to get ip address
> of VM from the message of netlink RTM_GETNEIGH
> 
> Signed-off-by: Chen Hanxiao 
> ---
> v4：
>   remove dummy entry
>   use VIR_APPEND_ELEMENT
> 
> v3:
>   add docs in virDomainInterfaceAddresses
>   remove error label
>   show network interface which did not match the arp table
> 
>  include/libvirt/libvirt-domain.h |  1 +
>  src/libvirt-domain.c |  7 
>  src/qemu/qemu_driver.c   | 72 
> 
>  3 files changed, 80 insertions(+)
> 

More Coverity found issues...


[...]

> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 9e715e7a0..7d77e1643 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c

[...]

> +static int
> +qemuARPGetInterfaces(virDomainObjPtr vm,
> + virDomainInterfacePtr **ifaces)
> +{
> +size_t i, j;
> +size_t ifaces_count = 0;
> +int ret = -1;
> +char macaddr[VIR_MAC_STRING_BUFLEN];
> +virDomainInterfacePtr *ifaces_ret = NULL;
> +virDomainInterfacePtr iface = NULL;
> +virArpTablePtr table;
> +
> +table = virArpTableGet();
> +if (!table)
> +goto cleanup;
> +
> +for (i = 0; i < vm->def->nnets; i++) {
> +if (vm->def->nets[i]->type != VIR_DOMAIN_NET_TYPE_NETWORK)
> +continue;
> +
> +virMacAddrFormat(&(vm->def->nets[i]->mac), macaddr);
> +virArpTableEntry entry;
> +for (j = 0; j < table->n; j++) {
> +entry = table->t[j];
> +if (STREQ(entry.mac, macaddr)) {
> +if (VIR_ALLOC(iface) < 0)
> +goto cleanup;

Prior to getting to the VIR_APPEND_ELEMENT, we can jump to cleanup and
we leak @iface and everything that's been allocated within @iface.

> +
> +iface->naddrs = 1;
> +if (VIR_STRDUP(iface->name, vm->def->nets[i]->ifname) < 0)
> +goto cleanup;
> +
> +if (VIR_STRDUP(iface->hwaddr, macaddr) < 0)
> +goto cleanup;
> +
> +if (VIR_ALLOC_N(iface->addrs, iface->naddrs) < 0)
> +goto cleanup;
> +
> +if (VIR_STRDUP(iface->addrs->addr, entry.ipaddr) < 0)
> +goto cleanup;
> +
> +if (VIR_APPEND_ELEMENT(ifaces_ret, ifaces_count, iface) < 0)
> +goto cleanup;
> +
> +}
> +}
> +}
> +
> +VIR_STEAL_PTR(*ifaces, ifaces_ret);
> +ret = ifaces_count;
> +
> + cleanup:
> +virArpTableFree(table);

As noted in patch2, we can get here if table == NULL when virArpTableGet
fails, so either we fix it here or in the API itself. The API should be
fixed rather than here...

> +
> +if (ifaces_ret) {
> +for (i = 0; i < ifaces_count; i++)
> +virDomainInterfaceFree(ifaces_ret[i]);
> +}
> +VIR_FREE(ifaces_ret);
> +
> +return ret;
> +}
> +
> +
>  static int
>  qemuDomainSetUserPassword(virDomainPtr dom,
>const char *user,
> 

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

[libvirt] [PATCH 0/2] Don't report a VFIO error for mdev when IOMMU is disabled

[Erik Skultety]

Unlike GPU assignment, mdev doesn't need the IOMMU to be enabled within kernel,
since the physical parent device takes care of the isolation, i.e. there's an
IOMMU group entry for each mdev created under sysfs, thus a VM can start
successfully

Erik Skultety (2):
  util:mdev: Improve the error msg on non-existent mdev prior to VM
start
  qemu: hostdev: Don't error out on domain with an mdev when IOMMU is
off

 src/qemu/qemu_hostdev.c |  7 ++-
 src/util/virmdev.c  | 16 +---
 2 files changed, 19 insertions(+), 4 deletions(-)

--
2.13.6

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

[libvirt] [PATCH 2/2] qemu: hostdev: Don't error out on domain with an mdev when IOMMU is off

[Erik Skultety]

Commit b4c2ac8d56 made a false assumption that IOMMU support necessary
for an mdev device to be assigned to a VM. Unlike direct PCI assignment,
IOMMU support is not needed for mediated devices, as the physical parent
device provides the IOMMU isolation, therefore, simply checking for VFIO
presence is enough to successfully start a VM.
Luckily, this issue is not very serious, since as of yet, libvirt
mandates the mdevs to be pre-created prior to a domain's launch, so this
patch will merely change the error the end user is going to see.

Previously:
unsupported configuration: Mediated host device assignment requires VFIO
support

Now:
failed to read device '/sys/bus/mdev/devices//': No such file or
directory

Signed-off-by: Erik Skultety 
---
 src/qemu/qemu_hostdev.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
index 73d26f4c6..afe445d4e 100644
--- a/src/qemu/qemu_hostdev.c
+++ b/src/qemu/qemu_hostdev.c
@@ -330,9 +330,14 @@ qemuHostdevPrepareMediatedDevices(virQEMUDriverPtr driver,
   int nhostdevs)
 {
 virHostdevManagerPtr hostdev_mgr = driver->hostdevMgr;
-bool supportsVFIO = qemuHostdevHostSupportsPassthroughVFIO();
+bool supportsVFIO;
 size_t i;

+/* Checking for VFIO only is fine with mdev, as IOMMU isolation is achieved
+ * by the physical parent device.
+ */
+supportsVFIO = virFileExists("/dev/vfio/vfio");
+
 for (i = 0; i < nhostdevs; i++) {
 if (hostdevs[i]->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
 hostdevs[i]->source.subsys.type == 
VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV) {
--
2.13.6

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

Re: [libvirt] [PATCH rebase v4 2/5] util: introduce helper to parse message from RTM_GETNEIGH query

[John Ferlan]

On 03/08/2018 02:11 AM, Chen Hanxiao wrote:
> From: Chen Hanxiao 
> 
> introduce helper to parse RTM_GETNEIGH query message and
> store it in struct virArpTable.
> 
> Signed-off-by: Chen Hanxiao 
> ---
> v4-rebase:
>   fit split Makefile.am
>   fit new virMacAddr fields
> 
> v4:
>   use netlink query instead of parsing /proc/net/arp
> 
> v3:
>   s/virGetArpTable/virArpTableGet
>   alloc virArpTable in virArpTableGet
>   return ENOSUPP on none-Linux platform
>   move helpers to virarptable.[ch]
> 
>  po/POTFILES.in   |   1 +
>  src/Makefile.am  |   1 +
>  src/libvirt_private.syms |   5 ++
>  src/util/Makefile.inc.am |   2 +
>  src/util/virarptable.c   | 181 
> +++
>  src/util/virarptable.h   |  48 +
>  6 files changed, 238 insertions(+)
>  create mode 100644 src/util/virarptable.c
>  create mode 100644 src/util/virarptable.h
> 

Couple of Coverity issues

[...]

> diff --git a/src/util/virarptable.c b/src/util/virarptable.c
> new file mode 100644
> index 0..cb56338eb
> --- /dev/null
> +++ b/src/util/virarptable.c

[...]

> +# define NDA_RTA(r) \
> +((struct rtattr*)(((char*)(r)) + NLMSG_ALIGN(sizeof(struct ndmsg
> +
> +static int
> +parse_rtattr(struct rtattr *tb[], int max, struct rtattr *rta, int len)
> +{
> +memset(tb, 0, sizeof(struct rtattr *) * (max + 1));
> +while (RTA_OK(rta, len)) {
> +if ((rta->rta_type <= max) && (!tb[rta->rta_type]))
> +tb[rta->rta_type] = rta;
> +rta = RTA_NEXT(rta, len);
> +}
> +
> +if (len)
> +VIR_WARN("malformed netlink message: Deficit %d, rta_len=%d",
> + len, rta->rta_len);
> +return 0;
> +}
> +
> +virArpTablePtr virArpTableGet(void)

As an aside - this format is non standard, should be

virArpTablePtr
virArpTableGet(void)

and there should be 2 blank lines between functions.

> +{
> +int num = 0;
> +int msglen;
> +void *nlData = NULL;
> +virArpTablePtr table = NULL;
> +char *ipstr = NULL;
> +struct nlmsghdr* nh;
> +struct rtattr * tb[NDA_MAX+1];
> +
> +msglen = virNetlinkGetNeighbor(, 0, 0);
> +if (msglen < 0)
> +return NULL;
> +
> +if (VIR_ALLOC(table) < 0)
> +return NULL;
> +
> +nh = (struct nlmsghdr*)nlData;
> +
> +while (NLMSG_OK(nh, msglen)) {
> +struct ndmsg *r = NLMSG_DATA(nh);
> +int len = nh->nlmsg_len;
> +void *addr;
> +
> +  if ((len -= NLMSG_LENGTH(sizeof(*nh))) < 0) {
> +  virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("wrong nlmsg len"));
> +  goto cleanup;
> +  }
> +
> +  if (r->ndm_family && (r->ndm_family != AF_INET))
> +  goto next_nlmsg;
> +
> +  /* catch stale and reachalbe arp entry only */
> +  if (r->ndm_state &&
> +  (!(r->ndm_state == NUD_STALE || r->ndm_state == NUD_REACHABLE))) {
> +  nh = NLMSG_NEXT(nh, msglen);
> +  continue;
> +  }
> +
> +  if (nh->nlmsg_type == NLMSG_DONE)
> +  goto end_of_netlink_messages;
> +
> +  parse_rtattr(tb, NDA_MAX, NDA_RTA(r),
> +   nh->nlmsg_len - NLMSG_LENGTH(sizeof(*r)));
> +
> +  if (tb[NDA_DST] == NULL || tb[NDA_LLADDR] == NULL)
> +  goto next_nlmsg;
> +
> +  if (tb[NDA_DST]) {
> +  virSocketAddr virAddr;
> +  if (VIR_REALLOC_N(table->t, num + 1) < 0)
> +  goto cleanup;
> +
> +  table->n = num + 1;
> +
> +  addr = RTA_DATA(tb[NDA_DST]);
> +  bzero(, sizeof(virAddr));
> +  virAddr.len = sizeof(virAddr.data.inet4);
> +  virAddr.data.inet4.sin_family = AF_INET;
> +  virAddr.data.inet4.sin_addr = *(struct in_addr *)addr;
> +  ipstr = virSocketAddrFormat();
> +
> +  if (VIR_STRDUP(table->t[num].ipaddr, ipstr) < 0)
> +  goto cleanup;
> +
> +  VIR_FREE(ipstr);
> +  }
> +
> +  if (tb[NDA_LLADDR]) {
> +  virMacAddr macaddr;
> +  char ifmac[VIR_MAC_STRING_BUFLEN];
> +
> +  addr = RTA_DATA(tb[NDA_LLADDR]);
> +  memcpy(macaddr.addr, addr, VIR_MAC_BUFLEN);
> +
> +  virMacAddrFormat(, ifmac);
> +
> +  if (VIR_STRDUP(table->t[num].mac, ifmac) < 0)
> +  goto cleanup;
> +
> +  num++;
> +  }
> +
> + next_nlmsg:
> +  nh = NLMSG_NEXT(nh, msglen);
> +}
> +
> + end_of_netlink_messages:
> +VIR_FREE(nlData);
> +return table;
> +
> + cleanup:

If we end up here, then @table (and anything that's allocated into it)
is leaked

> +VIR_FREE(ipstr);
> +VIR_FREE(nlData);
> +return NULL;
> +}
> +
> +#else
> +
> +virArpTablePtr virArpTableGet(void)

Similar comment here about the format...

> +{
> +virReportError(VIR_ERR_NO_SUPPORT, "%s",
> +   _("get arp table not implemented on this platform"));
> +return NULL;
> +}
> +
> +#endif /* __linux__ */
> +
> +void
> 

[libvirt] [PATCH 1/2] util:mdev: Improve the error msg on non-existent mdev prior to VM start

[Erik Skultety]

What one currently gets is:
failed to read '/sys/bus/mdev/devices//mdev_type/device_api': No
such file or directory

This indicates that something is missing within the device's sysfs tree
which likely might be not be the case here because the device simply
doesn't exist yet. So, when creating our internal mdev obj, let's check
whether the device exists first prior to trying to verify the
user-provided model within domain XML.

Signed-off-by: Erik Skultety 
---
 src/util/virmdev.c | 16 +---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/src/util/virmdev.c b/src/util/virmdev.c
index e4816cf20..2e3769aa6 100644
--- a/src/util/virmdev.c
+++ b/src/util/virmdev.c
@@ -150,13 +150,22 @@ virMediatedDeviceNew(const char *uuidstr, 
virMediatedDeviceModelType model)
 {
 virMediatedDevicePtr ret = NULL;
 virMediatedDevicePtr dev = NULL;
+char *sysfspath = NULL;
+
+if (!(sysfspath = virMediatedDeviceGetSysfsPath(uuidstr)))
+goto cleanup;
+
+if (!virFileExists(sysfspath)) {
+virReportSystemError(errno, _("failed to read device '%s'"),
+ sysfspath);
+goto cleanup;
+}
 
 if (VIR_ALLOC(dev) < 0)
-return NULL;
-
-if (!(dev->path = virMediatedDeviceGetSysfsPath(uuidstr)))
 goto cleanup;
 
+VIR_STEAL_PTR(dev->path, sysfspath);
+
 /* Check whether the user-provided model corresponds with the actually
  * supported mediated device's API.
  */
@@ -167,6 +176,7 @@ virMediatedDeviceNew(const char *uuidstr, 
virMediatedDeviceModelType model)
 VIR_STEAL_PTR(ret, dev);
 
  cleanup:
+VIR_FREE(sysfspath);
 virMediatedDeviceFree(dev);
 return ret;
 }
-- 
2.13.6

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

Re: [libvirt] [PATCH 03/20] virConnect(Un)RegisterCloseCallback: Throw an error in case the API is not supported

[Nikolay Shirokovskiy]

On 16.03.2018 12:39, Nikolay Shirokovskiy wrote:
> 
> 
> On 08.03.2018 15:20, Marc Hartmayer wrote:
>> Report an error in case the driver does not support
>> connect(Un)registerCloseCallback. The commit 'close callback: move it
>> to driver' (88f09b75eb99) moved the responsibility for the close
>> callback to the driver. But if the driver doesn't support the
>> connectRegisterCloseCallback API this function does nothing, even no
>> unsupported error report. The only case where an error is reported is
>> when the API is supported but the call fails. The same behavior
>> applies to virConnectUnregisterCloseCallback.
>>
>> This behavior is not intended as there are many use cases of this API
>> where the state of for example allocations depends on the result of
>> these functions.
>>
>> To keep the behavior of virsh as before it must silently ignore
>> unsupported error for virConnectRegisterCloseCallback. For the remote
>> driver this change wouldn't be needed, but for the byhve driver, for
>> example. Otherwise the user would see the error message that virsh was
>> unable to register a disconnect callback.
>>
>> Signed-off-by: Marc Hartmayer 
>> Reviewed-by: Boris Fiuczynski 
>> ---
>>  src/libvirt-host.c | 24 ++--
>>  tools/virsh.c  | 11 +--
>>  2 files changed, 23 insertions(+), 12 deletions(-)
>>
> 
> We can't change public API. As to patch 18 I suggest either to:
> 
> - don't refcount client object, this works though it is dangerous. Or
> - check if driver supports VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK and throw
>   an error if it does not (looks better to me)
> 

Still other clients (other than daemon) can have opaque leaks as they
don't check for VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK before call to 
virConnectRegisterCloseCallback. This is introduced in [1]

[1] 88f09b75e
Author: Nikolay Shirokovskiy 
Date:   Tue Mar 1 14:17:38 2016 +

close callback: move it to driver

Signed-off-by: Nikolay Shirokovskiy 

To fix this we can fallback to previous scheme (using closeCallback in
virConnectPtr) if connectRegisterCloseCallback is not defined for
the driver. Then we can refcount client object in patch 18 without
any additional checks.

Nikolay

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

Re: [libvirt] [PATCH 03/20] virConnect(Un)RegisterCloseCallback: Throw an error in case the API is not supported

[Nikolay Shirokovskiy]

On 08.03.2018 15:20, Marc Hartmayer wrote:
> Report an error in case the driver does not support
> connect(Un)registerCloseCallback. The commit 'close callback: move it
> to driver' (88f09b75eb99) moved the responsibility for the close
> callback to the driver. But if the driver doesn't support the
> connectRegisterCloseCallback API this function does nothing, even no
> unsupported error report. The only case where an error is reported is
> when the API is supported but the call fails. The same behavior
> applies to virConnectUnregisterCloseCallback.
> 
> This behavior is not intended as there are many use cases of this API
> where the state of for example allocations depends on the result of
> these functions.
> 
> To keep the behavior of virsh as before it must silently ignore
> unsupported error for virConnectRegisterCloseCallback. For the remote
> driver this change wouldn't be needed, but for the byhve driver, for
> example. Otherwise the user would see the error message that virsh was
> unable to register a disconnect callback.
> 
> Signed-off-by: Marc Hartmayer 
> Reviewed-by: Boris Fiuczynski 
> ---
>  src/libvirt-host.c | 24 ++--
>  tools/virsh.c  | 11 +--
>  2 files changed, 23 insertions(+), 12 deletions(-)
> 

We can't change public API. As to patch 18 I suggest either to:

- don't refcount client object, this works though it is dangerous. Or
- check if driver supports VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK and throw
  an error if it does not (looks better to me)

Nikolay

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

Re: [libvirt] [PATCH 18/20] remote: Use domainClientEventCallbacks for remoteReplayConnectionClosedEvent

[Nikolay Shirokovskiy]

On 15.03.2018 21:56, John Ferlan wrote:
> 
> 
> On 03/08/2018 07:20 AM, Marc Hartmayer wrote:
>> This allows us to get rid of another usage of the global variable
>> remoteProgram.
>>
>> Signed-off-by: Marc Hartmayer 
>> Reviewed-by: Boris Fiuczynski 
>> ---
>>  src/remote/remote_daemon_dispatch.c | 18 ++
>>  1 file changed, 14 insertions(+), 4 deletions(-)
>>
> 
> Reviewed-by: John Ferlan 
> 
> John
> 
> (see my note below, I imagine you agree...)
> 
>> diff --git a/src/remote/remote_daemon_dispatch.c 
>> b/src/remote/remote_daemon_dispatch.c
>> index 9580e854efbe..95940ffdeefe 100644
>> --- a/src/remote/remote_daemon_dispatch.c
>> +++ b/src/remote/remote_daemon_dispatch.c
>> @@ -1661,12 +1661,12 @@ remoteRelayDomainQemuMonitorEvent(virConnectPtr conn,
>>  static
>>  void remoteRelayConnectionClosedEvent(virConnectPtr conn ATTRIBUTE_UNUSED, 
>> int reason, void *opaque)
>>  {
>> -virNetServerClientPtr client = opaque;
>> +daemonClientEventCallbackPtr callback = opaque;
>>  
>>  VIR_DEBUG("Relaying connection closed event, reason %d", reason);
>>  
>>  remote_connect_event_connection_closed_msg msg = { reason };
>> -remoteDispatchObjectEventSend(client, remoteProgram,
>> +remoteDispatchObjectEventSend(callback->client, callback->program,
>>
>> REMOTE_PROC_CONNECT_EVENT_CONNECTION_CLOSED,
>>
>> (xdrproc_t)xdr_remote_connect_event_connection_closed_msg,
>>);
>> @@ -3814,6 +3814,7 @@ 
>> remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server 
>> ATTRIBUTE_UNUS
>> virNetMessageErrorPtr rerr)
>>  {
>>  int rv = -1;
>> +daemonClientEventCallbackPtr callback = NULL;
>>  struct daemonClientPrivate *priv =
>>  virNetServerClientGetPrivateData(client);
>>  
>> @@ -3824,9 +3825,16 @@ 
>> remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server 
>> ATTRIBUTE_UNUS
>>  goto cleanup;
>>  }
>>  
>> +if (VIR_ALLOC(callback) < 0)
>> +goto cleanup;
>> +callback->client = virObjectRef(client);
> 
> This one's scary because currently that means we currently call
> virConnectRegisterCloseCallback, but haven't been doing the
> virObjectRef(client) prior to using it as an opaque... Meaning
> remoteRelayConnectionClosedEvent could be called with client already free'd.

Refcounting was here originally but then removed in [1] as it conflicts with
virConnectRegisterCloseCallback returns 0 in case connectRegisterCloseCallback
is not implemented. This is safe though (at least nobody touch this place :).

ce35122cfe: "daemon: fixup refcounting in close callback handling"

Nikolay

> 
> 
>> +callback->program = virObjectRef(remoteProgram);
>> +/* eventID, callbackID, and legacy are not used */
>> +callback->eventID = -1;
>> +callback->callbackID = -1;
>>  if (virConnectRegisterCloseCallback(priv->conn,
>>  remoteRelayConnectionClosedEvent,
>> -client, NULL) < 0)
>> +callback, remoteEventCallbackFree) 
>> < 0)
>>  goto cleanup;
>>  
>>  priv->closeRegistered = true;
>> @@ -3834,8 +3842,10 @@ 
>> remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server 
>> ATTRIBUTE_UNUS
>>  
>>   cleanup:
>>  virMutexUnlock(>lock);
>> -if (rv < 0)
>> +if (rv < 0) {
>> +remoteEventCallbackFree(callback);
>>  virNetMessageSaveError(rerr);
>> +}
>>  return rv;
>>  }
>>  
>>
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
> 

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

Re: [libvirt] [PATCH 02/20] remote: Don't hard code the feature VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK as available

[Nikolay Shirokovskiy]

On 15.03.2018 21:52, Marc Hartmayer wrote:
> On Thu, Mar 15, 2018 at 12:02 AM +0100, John Ferlan  
> wrote:
>> On 03/14/2018 05:30 PM, John Ferlan wrote:
>>>
>>>
>>> On 03/08/2018 07:20 AM, Marc Hartmayer wrote:
 Don't assume that the feature VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK is
 available for every driver used for the connection.

 Signed-off-by: Marc Hartmayer 
 Reviewed-by: Bjoern Walk 
 Reviewed-by: Boris Fiuczynski 
 ---
  src/remote/remote_daemon_dispatch.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

>>>
>>> Something that is not clear about this one - since this was added for
>>> 'vz' driver by commit id 'f484310a', then shouldn't
>>> vzConnectSupportsFeature be updated to indicate support?
>>>
>>> If I'm right and you add the feature to the vz routine along with a
>>> reference to the commit id that forgot to in your commit message,
>>> then
> 
> You’re right.
> 
>>>
>>> Reviewed-by: John Ferlan 
>>>
>>> If I'm wrong - then help me understand!
>>>
>>> John
>>>
>>
>> Once I got to patch 5 I started questioning my (limited) understanding
>> of what's going on here.
>>
>> Still if we move the REMOTE_CLOSE_CALLBACK into the "check with the
>> connection" to see if it's supported, then how does patch 3/virsh
>> actually utilize the virConnectRegisterCloseCallback unless the vz
>> driver is enabled?
> 
> For the vz driver we’ve to add this feature to the
> 'vzConnectSupportsFeature' function (and for all other internal drivers,
> which supports the register/unregister close callback interface).
> 
>>
>> Won't the check with the connection driver for everyone else return 0?
> 
> So lets start from the beginning…
> 
> 'doRemoteOpen' checks if the server has the feature to register a close
> callback for the connection between the “remote daemon driver” and the
> used internal driver (e.g. QEMU, bhyve, etc.) (this is cached in
> priv->serverCloseCallback (bool) on the client side). In my opinion this
> depends (also) on the internal driver and therefore there is the need to
> check this by use of 'virConnectSupportsFeature'.

Hi, Marc.

I agree with the suggested change. There is no need to register/unregister
close callback in remote driver if internal driver does not implement 
appropriate feature
(now this is only vz driver). Current code works because if internal driver
does not implement connect(Un)RegisterCloseCallback then 
virConnectRegisterCloseCallback 
returns success as you mentioned below.

As to changing virConnectRegisterCloseCallback to return "not supported"
- I don't think this possible as this breaks backward compat.

Nikolay

> 
> The cover letter '[libvirt] [PATCH v5 0/10] add close callback for
> drivers with persistent connection' says to the original change:
> 
>“Currently close callback API can only inform us of closing the
>connection between remote driver and daemon. But what if a driver
>running in the daemon itself can have another persistent connection?
>In this case we want to be informed of that connection changes state
>too.”
> 
> 
> 'doRemoteOpen' does the following RPC call in remote_driver.c for the
> check:
> 
> remoteConnectSupportsFeatureUnlocked(conn, priv, 
> VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK);
> 
> Before this patch, 'remoteDispatchConnectSupportsFeature' always
> returned that the feature is supported (regardless of the capability of
> the used internal driver) => priv->serverCloseCallback is always true on
> the client side.
> 
> If now 'remoteConnectRegisterCloseCallback' is called on the client side
> (virsh calls it in virshReconnect) the client tests for
> 'priv->serverCloseCallback' and as this is always true, it will call the
> RPC 'REMOTE_PROC_CONNECT_REGISTER_CLOSE_CALLBACK' =>
> 'remoteDispatchConnectRegisterCloseCallback' is called on the server
> side.
> 
> So lets have a look at 'remoteDispatchConnectRegisterCloseCallback'. It
> registers internally that a close callback has been registered
> ('priv->closeRegistered = true' (daemon/remote.c)) and calls:
> 
> if (virConnectRegisterCloseCallback(priv->conn,
> remoteRelayConnectionClosedEvent,
> client, NULL) < 0)
> 
> priv->conn is the connection to the internal driver (e.g. QEMU, vz,
> etc.). virConnectRegisterCloseCallback is a _public_ API and it returns
> _only_ an error if the used internal driver implements the
> 'connectRegisterCloseCallback' function and an error happens during the
> 'connectRegisterCloseCallback(…)' call (src/libvirt-host.c).
> 
> Important: virConnect(Un)RegisterCloseCallback throws _no_ unsupported
> error even if the internal driver doesn’t support this API (this is
> changed in patch 3 of this series).
> 
> This works as long as you don't rely on the return value of
> 

[libvirt] [PATCH 0/2] Another round of build fixes

[Michal Privoznik]

Pushed under build-breaker rule.

Michal Privoznik (2):
  virarptable: Avoid cast align warnings
  virnetlink: Provide virNetlinkGetNeighbor non-Linux stub

 src/util/virarptable.c | 100 -
 src/util/virnetlink.c  |  11 ++
 2 files changed, 61 insertions(+), 50 deletions(-)

-- 
2.16.1

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

[libvirt] [PATCH 2/2] virnetlink: Provide virNetlinkGetNeighbor non-Linux stub

[Michal Privoznik]

This function is exported and therefore we have to have
implementation for all platforms.

Signed-off-by: Michal Privoznik 
---
 src/util/virnetlink.c | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c
index f0a92db234..0e281b06b8 100644
--- a/src/util/virnetlink.c
+++ b/src/util/virnetlink.c
@@ -1207,6 +1207,17 @@ virNetlinkDelLink(const char *ifname ATTRIBUTE_UNUSED,
 return -1;
 }
 
+
+int
+virNetlinkGetNeighbor(void **nlData ATTRIBUTE_UNUSED,
+  uint32_t src_pid ATTRIBUTE_UNUSED,
+  uint32_t dst_pid ATTRIBUTE_UNUSED)
+{
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _(unsupported));
+return -1;
+}
+
+
 /**
  * stopNetlinkEventServer: stop the monitor to receive netlink
  * messages for libvirtd
-- 
2.16.1

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

[libvirt] [PATCH 1/2] virarptable: Avoid cast align warnings

[Michal Privoznik]

We have to use VIR_WARNINGS_NO_CAST_ALIGN to avoid clang warning
about increased required alignment caused by some netlink macros.

Signed-off-by: Michal Privoznik 
---
 src/util/virarptable.c | 100 -
 1 file changed, 50 insertions(+), 50 deletions(-)

diff --git a/src/util/virarptable.c b/src/util/virarptable.c
index 8d9ab5fdc8..3819435d38 100644
--- a/src/util/virarptable.c
+++ b/src/util/virarptable.c
@@ -51,10 +51,11 @@ static int
 parse_rtattr(struct rtattr *tb[], int max, struct rtattr *rta, int len)
 {
 memset(tb, 0, sizeof(struct rtattr *) * (max + 1));
-while (RTA_OK(rta, len)) {
+VIR_WARNINGS_NO_CAST_ALIGN
+for (; RTA_OK(rta, len); rta = RTA_NEXT(rta, len)) {
+VIR_WARNINGS_RESET
 if ((rta->rta_type <= max) && (!tb[rta->rta_type]))
 tb[rta->rta_type] = rta;
-rta = RTA_NEXT(rta, len);
 }
 
 if (len)
@@ -82,73 +83,72 @@ virArpTablePtr virArpTableGet(void)
 
 nh = (struct nlmsghdr*)nlData;
 
-while (NLMSG_OK(nh, msglen)) {
+VIR_WARNINGS_NO_CAST_ALIGN
+for(; NLMSG_OK(nh, msglen); nh = NLMSG_NEXT(nh, msglen)) {
+VIR_WARNINGS_RESET
 struct ndmsg *r = NLMSG_DATA(nh);
 int len = nh->nlmsg_len;
 void *addr;
 
-  if ((len -= NLMSG_LENGTH(sizeof(*nh))) < 0) {
-  virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("wrong nlmsg len"));
-  goto cleanup;
-  }
+if ((len -= NLMSG_LENGTH(sizeof(*nh))) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("wrong nlmsg len"));
+goto cleanup;
+}
 
-  if (r->ndm_family && (r->ndm_family != AF_INET))
-  goto next_nlmsg;
+if (r->ndm_family && (r->ndm_family != AF_INET))
+continue;
 
-  /* catch stale and reachalbe arp entry only */
-  if (r->ndm_state &&
-  (!(r->ndm_state == NUD_STALE || r->ndm_state == NUD_REACHABLE))) {
-  nh = NLMSG_NEXT(nh, msglen);
-  continue;
-  }
+/* catch stale and reachalbe arp entry only */
+if (r->ndm_state &&
+(!(r->ndm_state == NUD_STALE || r->ndm_state == NUD_REACHABLE)))
+continue;
 
-  if (nh->nlmsg_type == NLMSG_DONE)
-  goto end_of_netlink_messages;
+if (nh->nlmsg_type == NLMSG_DONE)
+goto end_of_netlink_messages;
 
-  parse_rtattr(tb, NDA_MAX, NDA_RTA(r),
-   nh->nlmsg_len - NLMSG_LENGTH(sizeof(*r)));
+VIR_WARNINGS_NO_CAST_ALIGN
+parse_rtattr(tb, NDA_MAX, NDA_RTA(r),
+ nh->nlmsg_len - NLMSG_LENGTH(sizeof(*r)));
+VIR_WARNINGS_RESET
 
-  if (tb[NDA_DST] == NULL || tb[NDA_LLADDR] == NULL)
-  goto next_nlmsg;
+if (tb[NDA_DST] == NULL || tb[NDA_LLADDR] == NULL)
+continue;
 
-  if (tb[NDA_DST]) {
-  virSocketAddr virAddr;
-  if (VIR_REALLOC_N(table->t, num + 1) < 0)
-  goto cleanup;
+if (tb[NDA_DST]) {
+virSocketAddr virAddr;
+if (VIR_REALLOC_N(table->t, num + 1) < 0)
+goto cleanup;
 
-  table->n = num + 1;
+table->n = num + 1;
 
-  addr = RTA_DATA(tb[NDA_DST]);
-  bzero(, sizeof(virAddr));
-  virAddr.len = sizeof(virAddr.data.inet4);
-  virAddr.data.inet4.sin_family = AF_INET;
-  virAddr.data.inet4.sin_addr = *(struct in_addr *)addr;
-  ipstr = virSocketAddrFormat();
+addr = RTA_DATA(tb[NDA_DST]);
+bzero(, sizeof(virAddr));
+virAddr.len = sizeof(virAddr.data.inet4);
+virAddr.data.inet4.sin_family = AF_INET;
+virAddr.data.inet4.sin_addr = *(struct in_addr *)addr;
+ipstr = virSocketAddrFormat();
 
-  if (VIR_STRDUP(table->t[num].ipaddr, ipstr) < 0)
-  goto cleanup;
+if (VIR_STRDUP(table->t[num].ipaddr, ipstr) < 0)
+goto cleanup;
 
-  VIR_FREE(ipstr);
-  }
+VIR_FREE(ipstr);
+}
 
-  if (tb[NDA_LLADDR]) {
-  virMacAddr macaddr;
-  char ifmac[VIR_MAC_STRING_BUFLEN];
+if (tb[NDA_LLADDR]) {
+virMacAddr macaddr;
+char ifmac[VIR_MAC_STRING_BUFLEN];
 
-  addr = RTA_DATA(tb[NDA_LLADDR]);
-  memcpy(macaddr.addr, addr, VIR_MAC_BUFLEN);
+addr = RTA_DATA(tb[NDA_LLADDR]);
+memcpy(macaddr.addr, addr, VIR_MAC_BUFLEN);
 
-  virMacAddrFormat(, ifmac);
+virMacAddrFormat(, ifmac);
 
-  if (VIR_STRDUP(table->t[num].mac, ifmac) < 0)
-  goto cleanup;
+if (VIR_STRDUP(table->t[num].mac, ifmac) < 0)
+goto cleanup;
 
-  num++;
-  }
-
- next_nlmsg:
-  nh = NLMSG_NEXT(nh, msglen);
+num++;
+}
 }
 
  end_of_netlink_messages:
-- 
2.16.1

--
libvir-list mailing list