Re: [libvirt] [PATCH v4 10/25] [ACKED] conf: make virDomainPCIAddressGetNextSlot() a local static function

[Laine Stump]

On 10/18/2016 10:46 AM, Andrea Bolognani wrote:

On Fri, 2016-10-14 at 15:54 -0400, Laine Stump wrote:

This function is no longer needed outside of domain_addr.c.
---
   src/conf/domain_addr.c   | 2 +-
   src/conf/domain_addr.h   | 5 -
   src/libvirt_private.syms | 1 -
   3 files changed, 1 insertion(+), 7 deletions(-)
  
diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c

index 1710220..3a9e474 100644
--- a/src/conf/domain_addr.c
+++ b/src/conf/domain_addr.c
@@ -591,7 +591,7 @@ virDomainPCIAddressSetFree(virDomainPCIAddressSetPtr addrs)
   }
   
   
-int

+static int
   virDomainPCIAddressGetNextSlot(virDomainPCIAddressSetPtr addrs,
  virPCIDeviceAddressPtr next_addr,
  virDomainPCIConnectFlags flags)
diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h
index 904d060..4d6d12a 100644
--- a/src/conf/domain_addr.h
+++ b/src/conf/domain_addr.h
@@ -155,11 +155,6 @@ int 
virDomainPCIAddressReleaseSlot(virDomainPCIAddressSetPtr addrs,
  virPCIDeviceAddressPtr addr)
   ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
   
-int virDomainPCIAddressGetNextSlot(virDomainPCIAddressSetPtr addrs,

-   virPCIDeviceAddressPtr next_addr,
-   virDomainPCIConnectFlags flags)
-ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);

As noted while reviewing v3, you're losing both
ATTRIBUTE_NONNULL() with this commit. I was probably not
clear enough last time around, sorry about that :(


Nah, I just assumed you were merely pointing it out, not that you 
actually wanted me to maintain it in the static function.


I actually dislike ATTRIBUTE_NONNULL() because it doesn't guarantee any 
behavior of the functions callers, and even worse - it ends up 
optimizing out any code in the function that actually *checks* for those 
arguments being NULL. So it ends up being 0 help in catching any 
accidental NULL references (*maybe* it can be used by a static analyzer, 
but I remember at least one occasion where it actually covered up / 
created a bug by removing the check for NULL when one of the callers of 
a function actually was sending a NULL).


Still, I'll move the ATTRIBUTE_NONNULL over to the static function, and 
leave the debate of whether or not it really should be there to another day.


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

Re: [libvirt] [PATCH v4 02/25] [ACKED but has additions] qemu: replace a lot of "def->controllers[i]" with equivalent "cont"

[Laine Stump]

On 10/18/2016 07:22 AM, Andrea Bolognani wrote:

On Fri, 2016-10-14 at 15:53 -0400, Laine Stump wrote:

There's no functional change here. This pointer was just used so many
times that the extra long lines became annoying.
---
  
Change: added more instances of the same change.
  
   src/qemu/qemu_domain_address.c | 208 ++---

   1 file changed, 111 insertions(+), 97 deletions(-)
  
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c

index dc67d51..e6abadf 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -220,18 +220,22 @@ qemuDomainAssignSpaprVIOAddresses(virDomainDefPtr def,
   }
   
   for (i = 0; i < def->ncontrollers; i++) {

-model = def->controllers[i]->model;
-if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) {
+virDomainControllerDefPtr cont = def->controllers[i];
+
+model = cont->model;
+if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) {
   if (qemuDomainSetSCSIControllerModel(def, qemuCaps, ) < 0)

Definitely not something that should be touched by this patch,
but shouldn't we pass >model here?

I mean, if the value stored in model will be different than
the one that was already in cont->model, it means that the
default controller model was not set properly earlier...

On the other hand, the default model should really have been
set in PostParse() or something like that.


The function qemuDomainSetSCSIControllerModel() seems to be a confused 
mess. If model is set (non-0) then it checks the qemuCaps to makes sure 
the model that's set is allowed. Otherwise, it sets a default model. 
*All* calls to this function are on a temporary variable rather than the 
item in the controller object, so any non-zero model set will never be 
stored in the config. Why it is that way, I have no idea.


I choose to not touch it, for fear that something offbeat would break. 
(Maybe it's done this way because some old version of libvirt 6 or 7 
years ago didn't support setting a scsi controller model or something? 
Who knows...)


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

[libvirt] [PATCH 4/4] conf: Add iterators for RNG and Redirdev

[John Ferlan]

Add the virDomainRNGDefForeach and +virDomainRedirdevDefForeach iterators
to traverse all the RNG and Redirdevs just like the Smartcard's and Chr's

Signed-off-by: John Ferlan 
---
 src/conf/domain_conf.c   | 48 
 src/conf/domain_conf.h   | 18 ++
 src/libvirt_private.syms |  2 ++
 3 files changed, 68 insertions(+)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index de7df5c..30f2d6d 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -24559,6 +24559,54 @@ virDomainSmartcardDefForeach(virDomainDefPtr def,
 
 
 int
+virDomainRNGDefForeach(virDomainDefPtr def,
+   bool abortOnError,
+   virDomainRNGDefIterator iter,
+   void *opaque)
+{
+size_t i;
+int rc = 0;
+
+for (i = 0; i < def->nrngs; i++) {
+if ((iter)(def,
+   def->rngs[i],
+   opaque) < 0)
+rc = -1;
+
+if (abortOnError && rc != 0)
+goto done;
+}
+
+ done:
+return rc;
+}
+
+
+int
+virDomainRedirdevDefForeach(virDomainDefPtr def,
+bool abortOnError,
+virDomainRedirdevDefIterator iter,
+void *opaque)
+{
+size_t i;
+int rc = 0;
+
+for (i = 0; i < def->nredirdevs; i++) {
+if ((iter)(def,
+   def->redirdevs[i],
+   opaque) < 0)
+rc = -1;
+
+if (abortOnError && rc != 0)
+goto done;
+}
+
+ done:
+return rc;
+}
+
+
+int
 virDomainUSBDeviceDefForeach(virDomainDefPtr def,
  virDomainUSBDeviceDefIterator iter,
  void *opaque,
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index df216e8..df962d7 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2948,6 +2948,24 @@ int virDomainChrDefForeach(virDomainDefPtr def,
virDomainChrDefIterator iter,
void *opaque);
 
+typedef int (*virDomainRNGDefIterator)(virDomainDefPtr def,
+   virDomainRNGDefPtr dev,
+   void *opaque);
+
+int virDomainRNGDefForeach(virDomainDefPtr def,
+   bool abortOnError,
+   virDomainRNGDefIterator iter,
+   void *opaque);
+
+typedef int (*virDomainRedirdevDefIterator)(virDomainDefPtr def,
+virDomainRedirdevDefPtr dev,
+void *opaque);
+
+int virDomainRedirdevDefForeach(virDomainDefPtr def,
+bool abortOnError,
+virDomainRedirdevDefIterator iter,
+void *opaque);
+
 typedef int (*virDomainDiskDefPathIterator)(virDomainDiskDefPtr disk,
 const char *path,
 size_t depth,
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index bf503a5..c38 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -453,9 +453,11 @@ virDomainPMSuspendedReasonTypeToString;
 virDomainRedirdevBusTypeFromString;
 virDomainRedirdevBusTypeToString;
 virDomainRedirdevDefFind;
+virDomainRedirdevDefForeach;
 virDomainRedirdevDefFree;
 virDomainRedirdevDefRemove;
 virDomainRNGBackendTypeToString;
+virDomainRNGDefForeach;
 virDomainRNGDefFree;
 virDomainRNGFind;
 virDomainRNGModelTypeToString;
-- 
2.7.4

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

[libvirt] [PATCH 2/4] conf: Use virDomainChrSourceDefPtr for _virDomainRedirdevDef 'source.chr'

[John Ferlan]

Use a pointer and the virDomainChrSourceDefNew() function in order to
allocate the structure for _virDomainRedirdevDef.

Signed-off-by: John Ferlan 
---
 src/conf/domain_conf.c  | 36 
 src/conf/domain_conf.h  |  2 +-
 src/qemu/qemu_command.c |  2 +-
 src/qemu/qemu_hotplug.c |  2 +-
 4 files changed, 23 insertions(+), 19 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 7e2cc0c..bf55f01 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2363,7 +2363,7 @@ void virDomainRedirdevDefFree(virDomainRedirdevDefPtr def)
 if (!def)
 return;
 
-virDomainChrSourceDefClear(>source.chr);
+virDomainChrSourceDefFree(def->source.chr);
 virDomainDeviceInfoClear(>info);
 
 VIR_FREE(def);
@@ -12972,7 +12972,8 @@ virDomainHostdevDefParseXML(virDomainXMLOptionPtr 
xmlopt,
 
 
 static virDomainRedirdevDefPtr
-virDomainRedirdevDefParseXML(xmlNodePtr node,
+virDomainRedirdevDefParseXML(virDomainXMLOptionPtr xmlopt,
+ xmlNodePtr node,
  virHashTablePtr bootHash,
  unsigned int flags)
 {
@@ -12984,6 +12985,9 @@ virDomainRedirdevDefParseXML(xmlNodePtr node,
 if (VIR_ALLOC(def) < 0)
 return NULL;
 
+if (!(def->source.chr = virDomainChrSourceDefNew(xmlopt)))
+goto error;
+
 bus = virXMLPropString(node, "bus");
 if (bus) {
 if ((def->bus = virDomainRedirdevBusTypeFromString(bus)) < 0) {
@@ -12997,7 +13001,7 @@ virDomainRedirdevDefParseXML(xmlNodePtr node,
 
 type = virXMLPropString(node, "type");
 if (type) {
-if ((def->source.chr.type = virDomainChrTypeFromString(type)) < 0) {
+if ((def->source.chr->type = virDomainChrTypeFromString(type)) < 0) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("unknown redirdev character device type '%s'"), 
type);
 goto error;
@@ -13012,13 +13016,13 @@ virDomainRedirdevDefParseXML(xmlNodePtr node,
 /* boot gets parsed in virDomainDeviceInfoParseXML
  * source gets parsed in virDomainChrSourceDefParseXML
  * we don't know any of the elements that might remain */
-remaining = virDomainChrSourceDefParseXML(>source.chr, cur, flags,
+remaining = virDomainChrSourceDefParseXML(def->source.chr, cur, flags,
   NULL, NULL, NULL, 0);
 if (remaining < 0)
 goto error;
 
-if (def->source.chr.type == VIR_DOMAIN_CHR_TYPE_SPICEVMC)
-def->source.chr.data.spicevmc = VIR_DOMAIN_CHR_SPICEVMC_USBREDIR;
+if (def->source.chr->type == VIR_DOMAIN_CHR_TYPE_SPICEVMC)
+def->source.chr->data.spicevmc = VIR_DOMAIN_CHR_SPICEVMC_USBREDIR;
 
 if (virDomainDeviceInfoParseXML(node, bootHash, >info,
 flags | VIR_DOMAIN_DEF_PARSE_ALLOW_BOOT) < 
0)
@@ -13583,7 +13587,8 @@ virDomainDeviceDefParse(const char *xmlStr,
 goto error;
 break;
 case VIR_DOMAIN_DEVICE_REDIRDEV:
-if (!(dev->data.redirdev = virDomainRedirdevDefParseXML(node, NULL, 
flags)))
+if (!(dev->data.redirdev = virDomainRedirdevDefParseXML(xmlopt, node,
+NULL, flags)))
 goto error;
 break;
 case VIR_DOMAIN_DEVICE_RNG:
@@ -14846,8 +14851,8 @@ virDomainRedirdevDefFind(virDomainDefPtr def,
 if (redirdev->bus != tmp->bus)
 continue;
 
-if (!virDomainChrSourceDefIsEqual(>source.chr,
-  >source.chr))
+if (!virDomainChrSourceDefIsEqual(redirdev->source.chr,
+  tmp->source.chr))
 continue;
 
 if (redirdev->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE &&
@@ -17546,9 +17551,8 @@ virDomainDefParseXML(xmlDocPtr xml,
 if (n && VIR_ALLOC_N(def->redirdevs, n) < 0)
 goto error;
 for (i = 0; i < n; i++) {
-virDomainRedirdevDefPtr redirdev = 
virDomainRedirdevDefParseXML(nodes[i],
-
bootHash,
-flags);
+virDomainRedirdevDefPtr redirdev =
+virDomainRedirdevDefParseXML(xmlopt, nodes[i], bootHash, flags);
 if (!redirdev)
 goto error;
 
@@ -18665,12 +18669,12 @@ 
virDomainRedirdevDefCheckABIStability(virDomainRedirdevDefPtr src,
 
 switch ((virDomainRedirdevBus) src->bus) {
 case VIR_DOMAIN_REDIRDEV_BUS_USB:
-if (src->source.chr.type != dst->source.chr.type) {
+if (src->source.chr->type != dst->source.chr->type) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("Target redirected device source type %s does "
  "not match source device source type %s"),
-   

[libvirt] [PATCH 0/4] Complete virDomainChrSourceDefPtr adjustments

[John Ferlan]

Similar to _virDomainChrDef, alter _virDomainSmartcardDef and
_virDomainRedirdevDef in order to use a virDomainChrSourceDefPtr. This
includes modifying the relevant allocations for the source. Next,
modify the virDomainRNGDef allocation to use virDomainChrSourceNew
rather than a straight VIR_ALLOC.

Finally, add the "missing" iterators for RNG and Redirdev.

All this should set up what's necessary in order to handle adding
the TLS chardev TCP for all chr, smartcard, rng, and redirdev devices.

John Ferlan (4):
  conf: Use virDomainChrSourceDefPtr for _virDomainSmartcardDef
'passthru'
  conf: Use virDomainChrSourceDefPtr for _virDomainRedirdevDef
'source.chr'
  conf: Use virDomainChrSourceDefNew for virDomainRNGDef allocation
  conf: Add iterators for RNG and Redirdev

 src/conf/domain_audit.c |   2 +-
 src/conf/domain_conf.c  | 122 ++--
 src/conf/domain_conf.h  |  22 +++-
 src/libvirt_private.syms|   2 +
 src/qemu/qemu_command.c |   4 +-
 src/qemu/qemu_hotplug.c |   2 +-
 src/security/security_selinux.c |   4 +-
 7 files changed, 119 insertions(+), 39 deletions(-)

-- 
2.7.4

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

[libvirt] [PATCH 1/4] conf: Use virDomainChrSourceDefPtr for _virDomainSmartcardDef 'passthru'

[John Ferlan]

Use a pointer and the virDomainChrSourceDefNew() function in order to
allocate the structure for _virDomainSmartcardDef.

Signed-off-by: John Ferlan 
---
 src/conf/domain_audit.c |  2 +-
 src/conf/domain_conf.c  | 25 -
 src/conf/domain_conf.h  |  2 +-
 src/qemu/qemu_command.c |  2 +-
 src/security/security_selinux.c |  4 ++--
 5 files changed, 21 insertions(+), 14 deletions(-)

diff --git a/src/conf/domain_audit.c b/src/conf/domain_audit.c
index 314dee7..2decf02 100644
--- a/src/conf/domain_audit.c
+++ b/src/conf/domain_audit.c
@@ -218,7 +218,7 @@ virDomainAuditSmartcard(virDomainObjPtr vm,
 
 case VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH:
 virDomainAuditGenericDev(vm, "smartcard", NULL,
- 
virDomainAuditChardevPath(>data.passthru),
+ 
virDomainAuditChardevPath(def->data.passthru),
  reason, success);
 break;
 
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 748ffd5..7e2cc0c 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2146,7 +2146,7 @@ void virDomainSmartcardDefFree(virDomainSmartcardDefPtr 
def)
 break;
 
 case VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH:
-virDomainChrSourceDefClear(>data.passthru);
+virDomainChrSourceDefFree(def->data.passthru);
 break;
 
 default:
@@ -10489,7 +10489,8 @@ virDomainChrDefParseXML(virDomainXMLOptionPtr xmlopt,
 }
 
 static virDomainSmartcardDefPtr
-virDomainSmartcardDefParseXML(xmlNodePtr node,
+virDomainSmartcardDefParseXML(virDomainXMLOptionPtr xmlopt,
+  xmlNodePtr node,
   unsigned int flags)
 {
 xmlNodePtr cur;
@@ -10569,7 +10570,11 @@ virDomainSmartcardDefParseXML(xmlNodePtr node,
  "device type attribute"));
 goto error;
 }
-if ((def->data.passthru.type = virDomainChrTypeFromString(type)) < 0) {
+
+if (!(def->data.passthru = virDomainChrSourceDefNew(xmlopt)))
+goto error;
+
+if ((def->data.passthru->type = virDomainChrTypeFromString(type)) < 0) 
{
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("unknown type presented to host for "
  "character device: %s"), type);
@@ -10577,12 +10582,12 @@ virDomainSmartcardDefParseXML(xmlNodePtr node,
 }
 
 cur = node->children;
-if (virDomainChrSourceDefParseXML(>data.passthru, cur, flags,
+if (virDomainChrSourceDefParseXML(def->data.passthru, cur, flags,
   NULL, NULL, NULL, 0) < 0)
 goto error;
 
-if (def->data.passthru.type == VIR_DOMAIN_CHR_TYPE_SPICEVMC) {
-def->data.passthru.data.spicevmc
+if (def->data.passthru->type == VIR_DOMAIN_CHR_TYPE_SPICEVMC) {
+def->data.passthru->data.spicevmc
 = VIR_DOMAIN_CHR_SPICEVMC_SMARTCARD;
 }
 
@@ -13595,7 +13600,8 @@ virDomainDeviceDefParse(const char *xmlStr,
 goto error;
 break;
 case VIR_DOMAIN_DEVICE_SMARTCARD:
-if (!(dev->data.smartcard = virDomainSmartcardDefParseXML(node, 
flags)))
+if (!(dev->data.smartcard = virDomainSmartcardDefParseXML(xmlopt, node,
+  flags)))
 goto error;
 break;
 case VIR_DOMAIN_DEVICE_MEMBALLOON:
@@ -17189,7 +17195,8 @@ virDomainDefParseXML(xmlDocPtr xml,
 goto error;
 
 for (i = 0; i < n; i++) {
-virDomainSmartcardDefPtr card = virDomainSmartcardDefParseXML(nodes[i],
+virDomainSmartcardDefPtr card = virDomainSmartcardDefParseXML(xmlopt,
+  nodes[i],
   flags);
 if (!card)
 goto error;
@@ -21661,7 +21668,7 @@ virDomainSmartcardDefFormat(virBufferPtr buf,
 break;
 
 case VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH:
-if (virDomainChrSourceDefFormat(buf, NULL, >data.passthru, false,
+if (virDomainChrSourceDefFormat(buf, NULL, def->data.passthru, false,
 flags) < 0)
 return -1;
 break;
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 8738c80..c6654c2 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1159,7 +1159,7 @@ struct _virDomainSmartcardDef {
 char *file[VIR_DOMAIN_SMARTCARD_NUM_CERTIFICATES];
 char *database;
 } cert; /* 'host-certificates' */
-virDomainChrSourceDef passthru; /* 'passthrough' */
+virDomainChrSourceDefPtr passthru; /* 'passthrough' */
 } data;
 
 virDomainDeviceInfo info;
diff --git a/src/qemu/qemu_command.c 

[libvirt] [PATCH 3/4] conf: Use virDomainChrSourceDefNew for virDomainRNGDef allocation

[John Ferlan]

Rather than VIR_ALLOC() the data, use virDomainChrSourceDefNew in order
to get the private data if necessary.

Signed-off-by: John Ferlan 
---
 src/conf/domain_conf.c | 13 +++--
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index bf55f01..de7df5c 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -12099,7 +12099,8 @@ virDomainWatchdogDefParseXML(xmlNodePtr node,
 
 
 static virDomainRNGDefPtr
-virDomainRNGDefParseXML(xmlNodePtr node,
+virDomainRNGDefParseXML(virDomainXMLOptionPtr xmlopt,
+xmlNodePtr node,
 xmlXPathContextPtr ctxt,
 unsigned int flags)
 {
@@ -12172,7 +12173,7 @@ virDomainRNGDefParseXML(xmlNodePtr node,
 goto error;
 }
 
-if (VIR_ALLOC(def->source.chardev) < 0)
+if (!(def->source.chardev = virDomainChrSourceDefNew(xmlopt)))
 goto error;
 
 def->source.chardev->type = virDomainChrTypeFromString(type);
@@ -13592,7 +13593,8 @@ virDomainDeviceDefParse(const char *xmlStr,
 goto error;
 break;
 case VIR_DOMAIN_DEVICE_RNG:
-if (!(dev->data.rng = virDomainRNGDefParseXML(node, ctxt, flags)))
+if (!(dev->data.rng = virDomainRNGDefParseXML(xmlopt, node,
+  ctxt, flags)))
 goto error;
 break;
 case VIR_DOMAIN_DEVICE_CHR:
@@ -17481,9 +17483,8 @@ virDomainDefParseXML(xmlDocPtr xml,
 if (n && VIR_ALLOC_N(def->rngs, n) < 0)
 goto error;
 for (i = 0; i < n; i++) {
-virDomainRNGDefPtr rng = virDomainRNGDefParseXML(nodes[i],
- ctxt,
- flags);
+virDomainRNGDefPtr rng = virDomainRNGDefParseXML(xmlopt, nodes[i],
+ ctxt, flags);
 if (!rng)
 goto error;
 
-- 
2.7.4

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

Re: [libvirt] [REPOST] regarding cgroup v2 support in libvirt

[Tejun Heo]

Hello, Daniel.

On Fri, Oct 21, 2016 at 11:19:02AM +0100, Daniel P. Berrange wrote:
> The big question I have around cgroup v2 is state of support for all
> controllers that libvirt uses (cpu, cpuacct, cpuset, memory, devices,
> freezer, blkio).  IIUC, not all of these have been ported to cgroup
> v2 setup and the cpu port in particular was rejected by Linux maintainers.
> Libvirt has a general policy that we won't support features that only
> exist in out of tree patches (applies to kernel and any other software
> we build against or use).

I see and that's understandable.  However, I think supporting resource
control through systemd can be a good way of navigating the situation.
The back and forward compatibility issues are handled by systemd
allowing libvirt users to make use of what's available on the system
without burdening libvirt with complications.

> IIRC from earlier discussions, the model for dealing with processes in
> cgroup v2 was quite different. In libvirt we rely on the ability to
> assign different threads within a process to different cgroups, because
> we need to control CPU schedular parameters on different threads in
> QEMU. eg we have vCPU threads, I/O threads and general emulator threads
> each of which get different policies.

How thread granularity will be handled in cgroup v2 is still
contentious but I believe that we'll eventually have something.  I
have always been curious about the QEMU thread control tho.  What
prevents it from using the usual nice level adjustments?  Does it
actually require hierarchical resource distribution?

> When I spoke with Lennart about cgroup v2, way back in Jan, he indicated
> that while systemd can technically work with a system where some
> controllers are mounted as v1, while others are mounted as v2, this
> would not be an officially supported solution. Thus systemd in  Fedora
> was not likely to switch to v2 until all required controllers could use
> v2. I'm not sure if this still corresponds to Lennarts current views, so
> CC'ing him to confirm/deny.

The hybrid mode implemented in systemd uses cgroup v2 for process
management (the "name=systemd" hierarchy) but keeps using v1
hierarchies for all resource control.  For "Delegate=" users, I don't
think it'd matter all that much.  Such users either see all v1
hierarchies for all resource controllers as before or the v2
hierarchy.

> I think from Libvirt POV it would greatly simplify life if we could
> likewise restrict ourselves to dealing with hosts which are exclusively
> v1 or exclusively v2, and not a mixture. ie we can completely isolate
> our codebases for v1 vs v2 management, making it easier to reason about
> and test their correctness, reducing QA testing burden.

I think that's gonna be the case.  People *may* try to mix v1 and v2
hierarchies for resource control manually but supporting the mixture
in any major software project would require a lot of complications
which are difficult to justify.

> I recall that systemd policy for v2 was inteded to be that no app
> should write to cgroup sysfs except for systemd, unless there was
> a sub-tree created with Delegate=yes set on the scope. So this clearly
> means when using v2 we'll have to use the systemd DBus APIs for managing
> cgroups v2 on such hosts.

Hmmm... maybe I'm mistaken but it's also kinda broken without
"Delegate=" on v1 too and we got bitten by that already.  An internal
software assumed that it can branch down from the cgroups that the
target process is in at the time of startup and ended up building
sub-hierarchies at different positions in different hierarchies.
Later somebody launched a systemd service which requested some
resource accounting and systemd ended up relocating processes from
those sub-hierarchies.

On systemd systems, I don't think it makes sense to try to do
sub-hierarchy management directly without telling systemd about it.

The flip side is the same too.  With "Delegate=" set, cgroup v2
doesn't pose any more problems than v1 does.

> > It is true that, as libvirt can be used without systemd, libvirt will
> > probably want its own direct implementation down the line, but I think
> > there are benefits to going through systemd for resource settings in
> > general given that hierarchy setup is already done through systemd
> > when available.
> 
> While it is certainly nice that the vast majority of OS distros have
> switched over to using systemd for init, there's still enough users
> out there that I think we'll need to continue to have libvirt support
> for using sysfs for v2 on non-systemd hosts.

Definitely.

> Any way in summary, we'd like to see v2 support of course, since that
> is clearly the future. The big question is what we do about situation
> wrt not all controllers being supported in v2 - the lack of complete
> conversion is what has stopped me from doing any work in this area
> upto now.

What I'm suggesting now is, if available, to use systemd to set up
resource control up to delegation 

Re: [libvirt] [PATCH 1/3] vmx: Use the allocator virDomainChrDefNew

[Pavel Hrdina]

On Fri, Oct 21, 2016 at 01:58:42PM -0400, John Ferlan wrote:
> 
> 
> On 10/21/2016 12:01 PM, Pavel Hrdina wrote:
> > On Fri, Oct 21, 2016 at 09:01:42AM -0400, John Ferlan wrote:
> >> Rather than VIR_ALLOC of the virDomainChrDefPtr
> >>
> >> Signed-off-by: John Ferlan 
> >> ---
> >>  src/vmx/vmx.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
> >> index fc4347f..f61c4d6 100644
> >> --- a/src/vmx/vmx.c
> >> +++ b/src/vmx/vmx.c
> >> @@ -2758,7 +2758,7 @@ virVMXParseSerial(virVMXContext *ctx, virConfPtr 
> >> conf, int port,
> >>  return -1;
> >>  }
> >>  
> >> -if (VIR_ALLOC(*def) < 0)
> >> +if (!(*def = virDomainChrDefNew(NULL)))
> >>  return -1;
> >>  
> >>  (*def)->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL;
> >> @@ -2946,7 +2946,7 @@ virVMXParseParallel(virVMXContext *ctx, virConfPtr 
> >> conf, int port,
> >>  return -1;
> >>  }
> >>  
> >> -if (VIR_ALLOC(*def) < 0)
> >> +if (!(*def = virDomainChrDefNew(NULL)))
> >>  return -1;
> >>  
> >>  (*def)->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL;
> >> -- 
> > 
> > You've missed those two occurrences:
> > src/conf/domain_conf.c:3876if (VIR_ALLOC(def->consoles[0]) 
> > < 0)
> > src/conf/domain_conf.c:3891if (VIR_ALLOC(chr) < 0)
> > 
> 
> Thanks - I added them... I tried scouring all virDomainChrDef[Ptr] - I
> probably just had the overwhelming power of assumption that
> domain_conf.c would have used virAllocChrDefNew properly. The *-aa-*
> were certainly one of those environments in the 5% from my cover...
> 
> Those will have to be (NULL) calls though which I would think be correct
> since they're going to be implicitly added and they wouldn't be needing
> a TLS backend setup.

Oh, that's right, because this is just a copy of the first serial char device
and it is not actually used to create a command line.

> Not clear if there was an implicit ACK here or not, so I'll wait to push
> since it's not ultra critical to get this in.

So yes, it's an ACK.

Pavel


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/3] vmx: Use the allocator virDomainChrDefNew

[John Ferlan]

On 10/21/2016 12:01 PM, Pavel Hrdina wrote:
> On Fri, Oct 21, 2016 at 09:01:42AM -0400, John Ferlan wrote:
>> Rather than VIR_ALLOC of the virDomainChrDefPtr
>>
>> Signed-off-by: John Ferlan 
>> ---
>>  src/vmx/vmx.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
>> index fc4347f..f61c4d6 100644
>> --- a/src/vmx/vmx.c
>> +++ b/src/vmx/vmx.c
>> @@ -2758,7 +2758,7 @@ virVMXParseSerial(virVMXContext *ctx, virConfPtr conf, 
>> int port,
>>  return -1;
>>  }
>>  
>> -if (VIR_ALLOC(*def) < 0)
>> +if (!(*def = virDomainChrDefNew(NULL)))
>>  return -1;
>>  
>>  (*def)->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL;
>> @@ -2946,7 +2946,7 @@ virVMXParseParallel(virVMXContext *ctx, virConfPtr 
>> conf, int port,
>>  return -1;
>>  }
>>  
>> -if (VIR_ALLOC(*def) < 0)
>> +if (!(*def = virDomainChrDefNew(NULL)))
>>  return -1;
>>  
>>  (*def)->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL;
>> -- 
> 
> You've missed those two occurrences:
> src/conf/domain_conf.c:3876if (VIR_ALLOC(def->consoles[0]) < 
> 0)
> src/conf/domain_conf.c:3891if (VIR_ALLOC(chr) < 0)
> 

Thanks - I added them... I tried scouring all virDomainChrDef[Ptr] - I
probably just had the overwhelming power of assumption that
domain_conf.c would have used virAllocChrDefNew properly. The *-aa-*
were certainly one of those environments in the 5% from my cover...

Those will have to be (NULL) calls though which I would think be correct
since they're going to be implicitly added and they wouldn't be needing
a TLS backend setup.

Not clear if there was an implicit ACK here or not, so I'll wait to push
since it's not ultra critical to get this in.

John


> I've found those because virt-aa-helper-test was crashing after applying the
> next patch.  Because this is a generic code for all drivers we need to pass
> xmlopt to allocate the privateData.
> 
> Pavel
> 

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

[libvirt] [RFC] qemu: Use virtio-pci by default for mach-virt guests

[Andrea Bolognani]

virtio-pci is the way forward for aarch64 guests: it's faster
and less alien to people coming from other architectures.
Now that guest support is finally getting there, we'd like to
start using it by default instead of virtio-mmio.

Users and applications can already opt-in by explicitly using

  

inside the relevant elements, but that's kinda cumbersome and
requires all users and management applications to adapt, which
we'd really like to avoid.

What we can do instead is use virtio-mmio only if the guest
already has at least one virtio-mmio device, and use virtio-pci
in all other situations.

That means existing virtio-mmio guests will keep using the old
addressing scheme, and new guests will automatically be created
using virtio-pci instead. Users can still override the default
in either direction.
---
Sending this as an RFC for the time being because it clearly
needs some more polish, but I wanted to get the idea out
there sooner rather than later.

It needs to be applied on top of Laine's PCI series[1].


[1] https://www.redhat.com/archives/libvir-list/2016-October/msg00699.html

 src/qemu/qemu_domain_address.c | 128 -
 ...l2argv-aarch64-virt-2.6-virtio-pci-default.args |  14 ++-
 .../qemuxml2argv-aarch64-virtio-pci-default.args   |  14 ++-
 .../qemuxml2xmlout-aarch64-virtio-pci-default.xml  |  24 +++-
 4 files changed, 162 insertions(+), 18 deletions(-)

diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index f27d1e3..7f07764 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -265,6 +265,118 @@ qemuDomainAssignSpaprVIOAddresses(virDomainDefPtr def,
 }
 
 
+static bool
+qemuDomainAnyDeviceHasAddressOfType(virDomainDefPtr def,
+virDomainDeviceAddressType type)
+{
+size_t i;
+
+for (i = 0; i < def->ndisks; i++) {
+if (def->disks[i]->info.type == type)
+return true;
+}
+
+for (i = 0; i < def->ncontrollers; i++) {
+if (def->controllers[i]->info.type == type)
+return true;
+}
+
+for (i = 0; i < def->nfss; i++) {
+if (def->fss[i]->info.type == type)
+return true;
+}
+
+for (i = 0; i < def->nnets; i++) {
+if (def->nets[i]->info.type == type)
+return true;
+}
+
+for (i = 0; i < def->ninputs; i++) {
+if (def->inputs[i]->info.type == type)
+return true;
+}
+
+for (i = 0; i < def->nsounds; i++) {
+if (def->sounds[i]->info.type == type)
+return true;
+}
+
+for (i = 0; i < def->nvideos; i++) {
+if (def->videos[i]->info.type == type)
+return true;
+}
+
+for (i = 0; i < def->nhostdevs; i++) {
+if (def->hostdevs[i]->info->type == type)
+return true;
+}
+
+for (i = 0; i < def->nredirdevs; i++) {
+if (def->redirdevs[i]->info.type == type)
+return true;
+}
+
+for (i = 0; i < def->nsmartcards; i++) {
+if (def->smartcards[i]->info.type == type)
+return true;
+}
+
+for (i = 0; i < def->nserials; i++) {
+if (def->serials[i]->info.type == type)
+return true;
+}
+
+for (i = 0; i < def->nparallels; i++) {
+if (def->parallels[i]->info.type == type)
+return true;
+}
+
+for (i = 0; i < def->nchannels; i++) {
+if (def->channels[i]->info.type == type)
+return true;
+}
+
+for (i = 0; i < def->nconsoles; i++) {
+if (def->consoles[i]->info.type == type)
+return true;
+}
+
+for (i = 0; i < def->nhubs; i++) {
+if (def->hubs[i]->info.type == type)
+return true;
+}
+
+for (i = 0; i < def->nrngs; i++) {
+if (def->rngs[i]->info.type == type)
+return true;
+}
+
+for (i = 0; i < def->nshmems; i++) {
+if (def->shmems[i]->info.type == type)
+return true;
+}
+
+for (i = 0; i < def->nmems; i++) {
+if (def->mems[i]->info.type == type)
+return true;
+}
+
+if (def->memballoon && def->memballoon->info.type == type)
+return true;
+
+if (def->watchdog && def->watchdog->info.type == type)
+return true;
+
+if (def->nvram && def->nvram->info.type == type)
+return true;
+
+if (def->tpm && def->tpm->info.type == type)
+return true;
+
+return false;
+}
+
+
 static void
 qemuDomainPrimeVirtioDeviceAddresses(virDomainDefPtr def,
  virDomainDeviceAddressType type)
@@ -390,6 +502,8 @@ static void
 qemuDomainAssignARMVirtioMMIOAddresses(virDomainDefPtr def,
virQEMUCapsPtr qemuCaps)
 {
+bool usesMMIO;
+
 if (def->os.arch != VIR_ARCH_ARMV7L &&
 def->os.arch != VIR_ARCH_AARCH64)
 return;
@@ -398,9 +512,17 @@ qemuDomainAssignARMVirtioMMIOAddresses(virDomainDefPtr def,
   

Re: [libvirt] [PATCH 1/3] vmx: Use the allocator virDomainChrDefNew

[Pavel Hrdina]

On Fri, Oct 21, 2016 at 09:01:42AM -0400, John Ferlan wrote:
> Rather than VIR_ALLOC of the virDomainChrDefPtr
> 
> Signed-off-by: John Ferlan 
> ---
>  src/vmx/vmx.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
> index fc4347f..f61c4d6 100644
> --- a/src/vmx/vmx.c
> +++ b/src/vmx/vmx.c
> @@ -2758,7 +2758,7 @@ virVMXParseSerial(virVMXContext *ctx, virConfPtr conf, 
> int port,
>  return -1;
>  }
>  
> -if (VIR_ALLOC(*def) < 0)
> +if (!(*def = virDomainChrDefNew(NULL)))
>  return -1;
>  
>  (*def)->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL;
> @@ -2946,7 +2946,7 @@ virVMXParseParallel(virVMXContext *ctx, virConfPtr 
> conf, int port,
>  return -1;
>  }
>  
> -if (VIR_ALLOC(*def) < 0)
> +if (!(*def = virDomainChrDefNew(NULL)))
>  return -1;
>  
>  (*def)->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL;
> -- 

You've missed those two occurrences:
src/conf/domain_conf.c:3876if (VIR_ALLOC(def->consoles[0]) < 0)
src/conf/domain_conf.c:3891if (VIR_ALLOC(chr) < 0)

I've found those because virt-aa-helper-test was crashing after applying the
next patch.  Because this is a generic code for all drivers we need to pass
xmlopt to allocate the privateData.

Pavel


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/3] Introduce virDomainChrSourceDefNew for virDomainChrDefPtr

[Pavel Hrdina]

On Fri, Oct 21, 2016 at 09:01:43AM -0400, John Ferlan wrote:
> Change the virDomainChrDef to use a pointer to 'source' and allocate
> that pointer during virDomainChrDefNew.
> 
> This has tremendous "fallout" in the rest of the code which mainly
> has to change source.$field to source->$field.
> 
> Signed-off-by: John Ferlan 
> ---

ACK

Pavel


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

Re: [libvirt] [PATCH 3/3] conf: Move the privateData from virDomainChrDef to virDomainChrSourceDef

[Pavel Hrdina]

On Fri, Oct 21, 2016 at 09:01:44AM -0400, John Ferlan wrote:
> Commit id '5f2a132786' should have placed the data in the host source
> def structure since that's also used by smartcard, redirdev, and rng in
> order to provide a backend tcp channel.  The data in the private structure
> will be necessary in order to provide the secret properly
> 
> Signed-off-by: John Ferlan 
> ---
>  src/conf/domain_conf.c | 17 +++--
>  src/conf/domain_conf.h |  2 +-
>  2 files changed, 8 insertions(+), 11 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 2b89ea2..a684c48 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -2024,6 +2024,7 @@ void virDomainChrSourceDefFree(virDomainChrSourceDefPtr 
> def)
>  return;
>  
>  virDomainChrSourceDefClear(def);
> +virObjectUnref(def->privateData);
>  
>  VIR_FREE(def);
>  }
> @@ -2125,8 +2126,6 @@ void virDomainChrDefFree(virDomainChrDefPtr def)
>  VIR_FREE(def->seclabels);
>  }
>  
> -virObjectUnref(def->privateData);
> -
>  VIR_FREE(def);
>  }
>  
> @@ -10318,13 +10317,17 @@ 
> virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
>  
>  
>  static virDomainChrSourceDefPtr
> -virDomainChrSourceDefNew(void)
> +virDomainChrSourceDefNew(virDomainXMLOptionPtr xmlopt)
>  {
>  virDomainChrSourceDefPtr def = NULL;
>  
>  if (VIR_ALLOC(def) < 0)
>  return NULL;
>  
> +if (xmlopt && xmlopt->privateData.chardevNew &&
> +!(def->privateData = xmlopt->privateData.chardevNew()))

Perhaps rename it to chardevSourceNew or charSourceNew.

ACK

Pavel

> +VIR_FREE(def);
> +
>  return def;
>  }
>  
> @@ -10342,15 +10345,9 @@ virDomainChrDefNew(virDomainXMLOptionPtr xmlopt)
>  
>  def->target.port = -1;
>  
> -if (!(def->source = virDomainChrSourceDefNew()))
> +if (!(def->source = virDomainChrSourceDefNew(xmlopt)))
>  VIR_FREE(def);
>  
> -if (xmlopt && xmlopt->privateData.chardevNew &&
> -!(def->privateData = xmlopt->privateData.chardevNew())) {
> -virDomainChrSourceDefFree(def->source);
> -VIR_FREE(def);
> -}
> -
>  return def;
>  }
>  
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 5247d34..97ffcf4 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1078,6 +1078,7 @@ typedef enum {
>  /* The host side information for a character device.  */
>  struct _virDomainChrSourceDef {
>  int type; /* virDomainChrType */
> +virObjectPtr privateData;
>  union {
>  /* no  for null, vc, stdio */
>  struct {
> @@ -1117,7 +1118,6 @@ struct _virDomainChrSourceDef {
>  /* A complete character device, both host and domain views.  */
>  struct _virDomainChrDef {
>  int deviceType; /* enum virDomainChrDeviceType */
> -virObjectPtr privateData;
>  
>  bool targetTypeAttr;
>  int targetType; /* enum virDomainChrConsoleTargetType ||
> -- 
> 2.7.4
> 
> --
> 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 11/15] qemu: capabilities: Detect support for gluster debug setting

[Eric Blake]

On 10/21/2016 08:22 AM, Peter Krempa wrote:

>>>  static struct virQEMUCapsStringFlags virQEMUCapsQMPSchemaQueries[] = {
>>> -{ "bogus/path/to/satisfy/compiler", 0 },
>>> +{ "blockdev-add/arg-type/options/+gluster/debug-level", 
>>> QEMU_CAPS_GLUSTER_DEBUG_LEVEL},
>>
>> Okay, I see how you plan to use this.
>>
>> BIG WARNING FLAG - in qemu 2.7, blockdev-add is unstable (it should have
>> been named x-blockdev-add; but we goofed); but you can use the existence
>> of x-blockdev-del as a witness that blockdev-add is incomplete.
>>
>> In qemu 2.8, we are taking advantage of the fact that 2.7 wasn't
>> complete in order to get rid of the 'options' nesting layer; the plan is
>> to also rename x-blockdev-add at the same time.  Which means this probe,
>> as written, will only succeed for 2.7.
> 
> So. Should I add both? One with the "options" indirection and one
> without? Since there is no other way to access the structure than via
> blockdev-add and I really don't want to add a version check I see it as
> the only option.

Qemu 2.8 isn't released yet.  We tend to only commit into libvirt
support for released qemu. So I think I'm okay with using this patch
as-is (it will detect support for 2.7); then a followup patch once 2.8
is actually released and we've tested that we can still detect the
capability with the new-and-improved blockdev-add and corresponding
second query addition at that time.

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



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

Re: [libvirt] [PATCH 1/3] vz: support type=bridge network interface type correctly

[Maxim Nestratov]

07-Sep-16 14:00, Nikolay Shirokovskiy пишет:



On 05.09.2016 19:39, Maxim Nestratov wrote:

Recently, libprlsdk got a separate flag PNA_BRIDGE corresponding to
type=bridge libvirt network interfaces. Let's use it and get rid of
all workarounds previously added to  support it.

Signed-off-by: Maxim Nestratov 
---
  src/vz/vz_sdk.c | 100 
  1 file changed, 14 insertions(+), 86 deletions(-)

diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c
index f2a5c96..933b222 100644
--- a/src/vz/vz_sdk.c
+++ b/src/vz/vz_sdk.c
@@ -1004,27 +1004,19 @@ prlsdkGetNetInfo(PRL_HANDLE netAdapter, 
virDomainNetDefPtr net, bool isCt)
 PARALLELS_DOMAIN_ROUTED_NETWORK_NAME) < 0)
  goto cleanup;
  } else {
-char *netid = NULL;
-
-if (!(netid =
+char *netid =
prlsdkGetStringParamVar(PrlVmDevNet_GetVirtualNetworkId,
-  netAdapter)))
-goto cleanup;
+  netAdapter);
  
-/*

- * We use VIR_DOMAIN_NET_TYPE_NETWORK for all network adapters
- * except those whose Virtual Network Id differ from Parallels
- * predefined ones such as PARALLELS_DOMAIN_BRIDGED_NETWORK_NAME
- * and PARALLELS_DONAIN_ROUTED_NETWORK_NAME
- */
-if (STRNEQ(netid, PARALLELS_DOMAIN_BRIDGED_NETWORK_NAME)) {
+if (emulatedType == PNA_BRIDGE) {
  net->type = VIR_DOMAIN_NET_TYPE_BRIDGE;
-net->data.network.name = netid;
+if (netid)
+net->data.bridge.brname = netid;
  } else {
  net->type = VIR_DOMAIN_NET_TYPE_NETWORK;
-net->data.bridge.brname = netid;
+if (netid)
+net->data.network.name = netid;

not sure why check for not NULL here and above,


Assuming that I fix this and below, I have your ACK on the first two patches, 
right?

Maxim



1.) now i see my mistake introduced in 3a82c04c, setting net->data need to be 
swapped)


  }
-

here is my usual pedantic comment


  }
  
  if (!isCt) {

[snip]


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

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

[John Ferlan]

On 10/13/2016 07:08 AM, Viktor Mihajlovski wrote:
> Added a brief description of the VCPU states.
> 
> Signed-off-by: Viktor Mihajlovski 
> ---
>  tools/virsh.pod | 49 +
>  1 file changed, 49 insertions(+)
> 

ACK and pushed,

John

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

Re: [libvirt] [PATCH] xenFormatXLDisk: Fix commas for arguments

[Ján Tomko]

On Thu, Oct 20, 2016 at 11:05:36PM +0800, Michal Privoznik wrote:

On 20.10.2016 22:27, Ján Tomko wrote:

On Thu, Oct 20, 2016 at 10:16:05PM +0800, Michal Privoznik wrote:

instead of:

virBufferAdd(buf, "arg1,");
virBufferAdd(buf, "arg2");

lets have:

virBufferAdd(buf, "arg1");
virBufferAdd(buf, ",arg2");



Why?


Because it's better. Consider we want to add conditionally arg3. With my
change, it's simple:

if (cond)
 virBufferAdd(buf, ",arg3");

with current code there might be a comma hanging at EOL.



ACK with the explanation included in the commit message.

Please substitute the 'Fix' in the summary for something
that does not imply a functional change.

Jan


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

[libvirt] [PATCH 1/4] qemu: Introduce qemuDomainGetChardevTLSObjects for hotplug

[John Ferlan]

As it turns out more than one place will need these objects, so rather
than cut-copy-paste in each, make a helper

Signed-off-by: John Ferlan 
---
 src/qemu/qemu_hotplug.c | 41 +
 1 file changed, 29 insertions(+), 12 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 2cb2267..8a7c7cb 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1474,6 +1474,32 @@ qemuDomainAttachHostPCIDevice(virQEMUDriverPtr driver,
 }
 
 
+static int
+qemuDomainGetChardevTLSObjects(virQEMUDriverConfigPtr cfg,
+   qemuDomainObjPrivatePtr priv,
+   virDomainChrSourceDefPtr dev,
+   char *charAlias,
+   virJSONValuePtr *tlsProps,
+   char **tlsAlias)
+{
+if (!cfg->chardevTLS)
+return 0;
+
+if (qemuBuildTLSx509BackendProps(cfg->chardevTLSx509certdir,
+ dev->data.tcp.listen,
+ cfg->chardevTLSx509verify,
+ priv->qemuCaps,
+ tlsProps) < 0)
+return -1;
+
+if (!(*tlsAlias = qemuAliasTLSObjFromChardevAlias(charAlias)))
+return -1;
+dev->data.tcp.tlscreds = true;
+
+return 0;
+}
+
+
 int qemuDomainAttachRedirdevDevice(virQEMUDriverPtr driver,
virDomainObjPtr vm,
virDomainRedirdevDefPtr redirdev)
@@ -1730,18 +1756,9 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver,
 goto cleanup;
 
 if (dev->type == VIR_DOMAIN_CHR_TYPE_TCP &&
-cfg->chardevTLS) {
-if (qemuBuildTLSx509BackendProps(cfg->chardevTLSx509certdir,
- dev->data.tcp.listen,
- cfg->chardevTLSx509verify,
- priv->qemuCaps,
- ) < 0)
-goto cleanup;
-
-if (!(tlsAlias = qemuAliasTLSObjFromChardevAlias(charAlias)))
-goto cleanup;
-dev->data.tcp.tlscreds = true;
-}
+qemuDomainGetChardevTLSObjects(cfg, priv, dev, charAlias,
+   , ) < 0)
+goto cleanup;
 
 qemuDomainObjEnterMonitor(driver, vm);
 if (tlsAlias) {
-- 
2.7.4

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

[libvirt] [PATCH 4/4] qemu: Add TLS hotplug for qemuDomainAttachRNGDevice

[John Ferlan]

Commit id '2c322378' missed the nuance that the rng backend could be
using a TCP chardev and if TLS is enabled on the host, thus will need
to have the TLS object added.

Signed-off-by: John Ferlan 
---
 src/qemu/qemu_hotplug.c | 31 +++
 1 file changed, 27 insertions(+), 4 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 4b2a24c..aac1338 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1851,26 +1851,30 @@ qemuDomainAttachRNGDevice(virQEMUDriverPtr driver,
   virDomainObjPtr vm,
   virDomainRNGDefPtr rng)
 {
+virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
 qemuDomainObjPrivatePtr priv = vm->privateData;
 virErrorPtr orig_err;
 char *devstr = NULL;
 char *charAlias = NULL;
 char *objAlias = NULL;
+char *tlsAlias = NULL;
 bool releaseaddr = false;
 bool chardevAdded = false;
 bool objAdded = false;
+bool tlsobjAdded = false;
 virJSONValuePtr props = NULL;
+virJSONValuePtr tlsProps = NULL;
 virDomainCCWAddressSetPtr ccwaddrs = NULL;
 const char *type;
 int ret = -1;
 int rv;
 
 if (qemuAssignDeviceRNGAlias(vm->def, rng) < 0)
-return -1;
+goto cleanup;
 
 /* preallocate space for the device definition */
 if (VIR_REALLOC_N(vm->def->rngs, vm->def->nrngs + 1) < 0)
-return -1;
+goto cleanup;
 
 if (rng->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
 if (qemuDomainMachineIsS390CCW(vm->def) &&
@@ -1882,14 +1886,14 @@ qemuDomainAttachRNGDevice(virQEMUDriverPtr driver,
 } else {
 if (!qemuCheckCCWS390AddressSupport(vm->def, rng->info, priv->qemuCaps,
 rng->source.file))
-return -1;
+goto cleanup;
 }
 releaseaddr = true;
 
 if (rng->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE ||
 rng->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) {
 if (virDomainPCIAddressEnsureAddr(priv->pciaddrs, >info) < 0)
-return -1;
+goto cleanup;
 } else if (rng->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) {
 if (!(ccwaddrs = qemuDomainCCWAddrSetCreateFromDomain(vm->def)))
 goto cleanup;
@@ -1911,8 +1915,22 @@ qemuDomainAttachRNGDevice(virQEMUDriverPtr driver,
 if (!(charAlias = qemuAliasChardevFromDevAlias(rng->info.alias)))
 goto cleanup;
 
+if (rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD &&
+qemuDomainGetChardevTLSObjects(cfg, priv, rng->source.chardev,
+   charAlias, , ) < 0)
+goto cleanup;
+
 qemuDomainObjEnterMonitor(driver, vm);
 
+if (tlsAlias) {
+rv = qemuMonitorAddObject(priv->mon, "tls-creds-x509",
+  tlsAlias, tlsProps);
+tlsProps = NULL; /* qemuMonitorAddObject consumes */
+if (rv < 0)
+goto exit_monitor;
+tlsobjAdded = true;
+}
+
 if (rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD &&
 qemuMonitorAttachCharDev(priv->mon, charAlias,
  rng->source.chardev) < 0)
@@ -1940,17 +1958,22 @@ qemuDomainAttachRNGDevice(virQEMUDriverPtr driver,
  audit:
 virDomainAuditRNG(vm, NULL, rng, "attach", ret == 0);
  cleanup:
+virJSONValueFree(tlsProps);
 virJSONValueFree(props);
 if (ret < 0 && releaseaddr)
 qemuDomainReleaseDeviceAddress(vm, >info, NULL);
+VIR_FREE(tlsAlias);
 VIR_FREE(charAlias);
 VIR_FREE(objAlias);
 VIR_FREE(devstr);
 virDomainCCWAddressSetFree(ccwaddrs);
+virObjectUnref(cfg);
 return ret;
 
  exit_monitor:
 orig_err = virSaveLastError();
+if (tlsobjAdded)
+ignore_value(qemuMonitorDelObject(priv->mon, tlsAlias));
 if (objAdded)
 ignore_value(qemuMonitorDelObject(priv->mon, objAlias));
 if (rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD && chardevAdded)
-- 
2.7.4

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

[libvirt] [PATCH 3/4] qemu: Add TLS hotplug for qemuDomainAttachRedirdevDevice

[John Ferlan]

Commit id '2c322378' missed the nuance that the redirdev backend could
be using a TCP chardev and if TLS is enabled on the host, thus will need
to have the TLS object added.

Signed-off-by: John Ferlan 
---
 src/qemu/qemu_hotplug.c | 24 
 1 file changed, 24 insertions(+)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index cdd9222..4b2a24c 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1505,11 +1505,16 @@ int qemuDomainAttachRedirdevDevice(virQEMUDriverPtr 
driver,
virDomainRedirdevDefPtr redirdev)
 {
 int ret = -1;
+int rc;
+virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
 qemuDomainObjPrivatePtr priv = vm->privateData;
 virDomainDefPtr def = vm->def;
 char *charAlias = NULL;
 char *devstr = NULL;
 bool chardevAdded = false;
+bool tlsobjAdded = false;
+virJSONValuePtr tlsProps = NULL;
+char *tlsAlias = NULL;
 virErrorPtr orig_err;
 
 if (qemuAssignDeviceRedirdevAlias(def, redirdev, -1) < 0)
@@ -1524,7 +1529,21 @@ int qemuDomainAttachRedirdevDevice(virQEMUDriverPtr 
driver,
 if (VIR_REALLOC_N(def->redirdevs, def->nredirdevs+1) < 0)
 goto cleanup;
 
+if (redirdev->source.chr.type == VIR_DOMAIN_CHR_TYPE_TCP &&
+qemuDomainGetChardevTLSObjects(cfg, priv, &(redirdev->source.chr),
+   charAlias, , ) < 0)
+goto cleanup;
+
 qemuDomainObjEnterMonitor(driver, vm);
+if (tlsAlias) {
+rc = qemuMonitorAddObject(priv->mon, "tls-creds-x509",
+  tlsAlias, tlsProps);
+tlsProps = NULL; /* qemuMonitorAddObject consumes */
+if (rc < 0)
+goto exit_monitor;
+tlsobjAdded = true;
+}
+
 if (qemuMonitorAttachCharDev(priv->mon,
  charAlias,
  &(redirdev->source.chr)) < 0)
@@ -1542,12 +1561,17 @@ int qemuDomainAttachRedirdevDevice(virQEMUDriverPtr 
driver,
  audit:
 virDomainAuditRedirdev(vm, redirdev, "attach", ret == 0);
  cleanup:
+VIR_FREE(tlsAlias);
+virJSONValueFree(tlsProps);
 VIR_FREE(charAlias);
 VIR_FREE(devstr);
+virObjectUnref(cfg);
 return ret;
 
  exit_monitor:
 orig_err = virSaveLastError();
+if (tlsobjAdded)
+ignore_value(qemuMonitorDelObject(priv->mon, tlsAlias));
 /* detach associated chardev on error */
 if (chardevAdded)
 ignore_value(qemuMonitorDetachCharDev(priv->mon, charAlias));
-- 
2.7.4

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

[libvirt] [PATCH 0/4] Add TLS for TCP chardev redirdev and rng backends

[John Ferlan]

This "would" conflict with other TLS/TCP changes on list depending on
order of commit, but it does work against the current top. The conflict
would be limited to the new helper qemuDomainGetChardevTLSObjects and
changing the 'if (!cfg->chardevTLS)' to the corresponding 'if
dev->source.data.tcp.haveTLS != VIR_TRISTATE_BOOL_YES)' once Pavel's
changes are in and of course removing the need to fetch 'cfg'. Then
if my other changes related to making 'source' a pointer in the
_virDomainChrDef is accepted, the 'source.' here would change to
'source->'.

Essentially during the review process of adding a secret to access
the TLS environment, it was determined that the backends for redirdev
and rng weren't adding the TLS object on hotplug, even though the
command line processing would add it. 

NB: Smartcard would be in the same category, but it doesn't support
hotplug, so we're off the hook for that.

John Ferlan (4):
  qemu: Introduce qemuDomainGetChardevTLSObjects for hotplug
  qemu: Clean up error path in qemuDomainAttachRedirdevDevice
  qemu: Add TLS hotplug for qemuDomainAttachRedirdevDevice
  qemu: Add TLS hotplug for qemuDomainAttachRNGDevice

 src/qemu/qemu_hotplug.c | 125 ++--
 1 file changed, 99 insertions(+), 26 deletions(-)

-- 
2.7.4

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

[libvirt] [PATCH 2/4] qemu: Clean up error path in qemuDomainAttachRedirdevDevice

[John Ferlan]

It's about to get more complicated - let's alter the logic to handle
various failures. Adds saving of the error as well.

Signed-off-by: John Ferlan 
---
 src/qemu/qemu_hotplug.c | 29 +++--
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 8a7c7cb..cdd9222 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1509,6 +1509,8 @@ int qemuDomainAttachRedirdevDevice(virQEMUDriverPtr 
driver,
 virDomainDefPtr def = vm->def;
 char *charAlias = NULL;
 char *devstr = NULL;
+bool chardevAdded = false;
+virErrorPtr orig_err;
 
 if (qemuAssignDeviceRedirdevAlias(def, redirdev, -1) < 0)
 goto cleanup;
@@ -1525,17 +1527,12 @@ int qemuDomainAttachRedirdevDevice(virQEMUDriverPtr 
driver,
 qemuDomainObjEnterMonitor(driver, vm);
 if (qemuMonitorAttachCharDev(priv->mon,
  charAlias,
- &(redirdev->source.chr)) < 0) {
-ignore_value(qemuDomainObjExitMonitor(driver, vm));
-goto audit;
-}
+ &(redirdev->source.chr)) < 0)
+goto exit_monitor;
+chardevAdded = true;
 
-if (qemuMonitorAddDevice(priv->mon, devstr) < 0) {
-/* detach associated chardev on error */
-qemuMonitorDetachCharDev(priv->mon, charAlias);
-ignore_value(qemuDomainObjExitMonitor(driver, vm));
-goto audit;
-}
+if (qemuMonitorAddDevice(priv->mon, devstr) < 0)
+goto exit_monitor;
 
 if (qemuDomainObjExitMonitor(driver, vm) < 0)
 goto audit;
@@ -1548,6 +1545,18 @@ int qemuDomainAttachRedirdevDevice(virQEMUDriverPtr 
driver,
 VIR_FREE(charAlias);
 VIR_FREE(devstr);
 return ret;
+
+ exit_monitor:
+orig_err = virSaveLastError();
+/* detach associated chardev on error */
+if (chardevAdded)
+ignore_value(qemuMonitorDetachCharDev(priv->mon, charAlias));
+if (orig_err) {
+virSetError(orig_err);
+virFreeError(orig_err);
+}
+ignore_value(qemuDomainObjExitMonitor(driver, vm));
+goto audit;
 }
 
 static int
-- 
2.7.4

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

[libvirt] [PATCH 3/6] Return directly from qemuDomainAttachChrDeviceAssignAddr

[Ján Tomko]

This function should never need a cleanup section.
---
 src/qemu/qemu_hotplug.c | 28 ++--
 1 file changed, 10 insertions(+), 18 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index cbdcd81..407ae73 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1640,47 +1640,39 @@ qemuDomainAttachChrDeviceAssignAddr(virDomainDefPtr def,
 qemuDomainObjPrivatePtr priv,
 virDomainChrDefPtr chr)
 {
-int ret = -1;
-
 if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE &&
 chr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO) {
 if (virDomainVirtioSerialAddrAutoAssign(def, >info, true) < 0)
-goto cleanup;
-ret = 1;
+return -1;
+return 1;
 
 } else if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL &&
chr->targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_PCI) {
 if (virDomainPCIAddressEnsureAddr(priv->pciaddrs, >info) < 0)
-goto cleanup;
-ret = 1;
+return -1;
+return 1;
 
 } else if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL &&
chr->targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_USB) {
 if (virDomainUSBAddressEnsure(priv->usbaddrs, >info) < 0)
-goto cleanup;
-ret = 1;
+return -1;
+return 1;
 
 } else if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL &&
chr->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO) {
 if (virDomainVirtioSerialAddrAutoAssign(def, >info, false) < 0)
-goto cleanup;
-ret = 1;
+return -1;
+return 1;
 }
 
-if (ret == 1)
-goto cleanup;
-
 if (chr->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL ||
 chr->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("Unsupported address type for character device"));
-goto cleanup;
+return -1;
 }
 
-ret = 0;
-
- cleanup:
-return ret;
+return 0;
 }
 
 int qemuDomainAttachChrDevice(virQEMUDriverPtr driver,
-- 
2.7.3

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

[libvirt] [PATCH v2 01/15] util: json: add helper to iterate and steal members of json array

[Peter Krempa]

Simplifies cases where JSON array members need to be transferred to a
different structure.
---

Notes:
v2:
- iterate from beginning
- add option to keep array member in the array without breaking iteration
- documented callback return codes

 src/libvirt_private.syms |  1 +
 src/util/virjson.c   | 54 
 src/util/virjson.h   |  6 ++
 3 files changed, 61 insertions(+)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index bf503a5..e21be67 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1762,6 +1762,7 @@ virISCSIScanTargets;

 # util/virjson.h
 virJSONValueArrayAppend;
+virJSONValueArrayForeachSteal;
 virJSONValueArrayGet;
 virJSONValueArraySize;
 virJSONValueArraySteal;
diff --git a/src/util/virjson.c b/src/util/virjson.c
index 1d8e6d5..d55600a 100644
--- a/src/util/virjson.c
+++ b/src/util/virjson.c
@@ -949,6 +949,60 @@ virJSONValueArraySteal(virJSONValuePtr array,
 }


+/**
+ * virJSONValueArrayForeachSteal:
+ * @array: array to iterate
+ * @cb: callback called on every member of the array
+ * @opaque: custom data for the callback
+ *
+ * Iterates members of the array and calls the callback on every single member.
+ * The return codes of the callback are interpreted as follows:
+ * 0: callback claims ownership of the array element and is responsible for
+ *freeing it
+ * 1: callback does not claim the ownership, but the iteration continues
+ * -1: callback doesn't claim ownership and iteration does not continue
+ *
+ * Returns 0 if all members were iterated and/or stolen by the callback; -1
+ * on callback failure or if the JSON value object is not an array.
+ * The rest of the members stay in possession of the array and it's condensed.
+ */
+int
+virJSONValueArrayForeachSteal(virJSONValuePtr array,
+  virJSONArrayIteratorFunc cb,
+  void *opaque)
+{
+size_t i;
+size_t j = 0;
+int ret = 0;
+int rc;
+
+if (array->type != VIR_JSON_TYPE_ARRAY)
+return -1;
+
+for (i = 0; i < array->data.array.nvalues; i++) {
+if ((rc = cb(i, array->data.array.values[i], opaque)) < 0) {
+ret = -1;
+break;
+}
+
+if (rc == 0)
+array->data.array.values[i] = NULL;
+}
+
+/* condense the remaining entries at the beginning */
+for (i = 0; i < array->data.array.nvalues; i++) {
+if (!array->data.array.values[i])
+continue;
+
+array->data.array.values[j++] = array->data.array.values[i];
+}
+
+array->data.array.nvalues = j;
+
+return ret;
+}
+
+
 const char *
 virJSONValueGetString(virJSONValuePtr string)
 {
diff --git a/src/util/virjson.h b/src/util/virjson.h
index 8b62d65..5b4f172 100644
--- a/src/util/virjson.h
+++ b/src/util/virjson.h
@@ -117,6 +117,12 @@ bool virJSONValueIsArray(virJSONValuePtr array);
 ssize_t virJSONValueArraySize(const virJSONValue *array);
 virJSONValuePtr virJSONValueArrayGet(virJSONValuePtr object, unsigned int 
element);
 virJSONValuePtr virJSONValueArraySteal(virJSONValuePtr object, unsigned int 
element);
+typedef int (*virJSONArrayIteratorFunc)(size_t pos,
+virJSONValuePtr item,
+void *opaque);
+int virJSONValueArrayForeachSteal(virJSONValuePtr array,
+  virJSONArrayIteratorFunc cb,
+  void *opaque);

 int virJSONValueObjectKeysNumber(virJSONValuePtr object);
 const char *virJSONValueObjectGetKey(virJSONValuePtr object, unsigned int n);
-- 
2.10.0

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

[libvirt] [PATCH 4/6] Also create the USB address cache for domains with all the USB addresses

[Ján Tomko]

When starting a new domain, we allocate the USB addresses and keep
an address cache in the domain object's private data.

However this data is lost on libvirtd restart.

Also generate the address cache if all the addresses have been
specified, so that devices hotplugged after libvirtd restart
also get theirs assigned.

https://bugzilla.redhat.com/show_bug.cgi?id=1387666
---
 src/conf/domain_addr.c | 12 
 src/conf/domain_addr.h |  4 
 src/libvirt_private.syms   |  1 +
 src/qemu/qemu_domain_address.c | 16 ++--
 4 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
index 92a5516..34fa01b 100644
--- a/src/conf/domain_addr.c
+++ b/src/conf/domain_addr.c
@@ -1797,6 +1797,18 @@ virDomainUSBAddressAssign(virDomainUSBAddressSetPtr 
addrs,
 
 
 int
+virDomainUSBAddressPresent(virDomainDeviceInfoPtr info,
+   void *data ATTRIBUTE_UNUSED)
+{
+if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB &&
+virDomainUSBAddressPortIsValid(info->addr.usb.port))
+return 0;
+
+return -1;
+}
+
+
+int
 virDomainUSBAddressReserve(virDomainDeviceInfoPtr info,
void *data)
 {
diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h
index 141f83b..ee8dc04 100644
--- a/src/conf/domain_addr.h
+++ b/src/conf/domain_addr.h
@@ -299,6 +299,10 @@ virDomainUSBAddressCountAllPorts(virDomainDefPtr def);
 void virDomainUSBAddressSetFree(virDomainUSBAddressSetPtr addrs);
 
 int
+virDomainUSBAddressPresent(virDomainDeviceInfoPtr info,
+   void *data)
+ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
+int
 virDomainUSBAddressReserve(virDomainDeviceInfoPtr info,
void *data)
 ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index ce9c4c4..cbc97f7 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -116,6 +116,7 @@ virDomainUSBAddressEnsure;
 virDomainUSBAddressPortFormat;
 virDomainUSBAddressPortFormatBuf;
 virDomainUSBAddressPortIsValid;
+virDomainUSBAddressPresent;
 virDomainUSBAddressRelease;
 virDomainUSBAddressReserve;
 virDomainUSBAddressSetAddControllers;
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index 7d92e0b..a6001ad 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -1710,12 +1710,24 @@ qemuDomainUSBAddressAddHubs(virDomainDefPtr def)
 
 static int
 qemuDomainAssignUSBAddresses(virDomainDefPtr def,
- virDomainObjPtr obj)
+ virDomainObjPtr obj,
+ bool newDomain)
 {
 int ret = -1;
 virDomainUSBAddressSetPtr addrs = NULL;
 qemuDomainObjPrivatePtr priv = NULL;
 
+if (!newDomain) {
+/* only create the address cache for:
+ *  new domains
+ *  domains that already have all the addresses specified
+ * otherwise libvirt's attempt to recreate the USB topology via
+ * QEMU command line might fail */
+if (virDomainUSBDeviceDefForeach(def, virDomainUSBAddressPresent, NULL,
+ false) < 0)
+return 0;
+}
+
 if (!(addrs = virDomainUSBAddressSetCreate()))
 goto cleanup;
 
@@ -1772,7 +1784,7 @@ qemuDomainAssignAddresses(virDomainDefPtr def,
 if (qemuDomainAssignPCIAddresses(def, qemuCaps, obj) < 0)
 return -1;
 
-if (newDomain && qemuDomainAssignUSBAddresses(def, obj) < 0)
+if (qemuDomainAssignUSBAddresses(def, obj, newDomain) < 0)
 return -1;
 
 return 0;
-- 
2.7.3

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

[libvirt] [PATCH 5/6] Fix crash on usb-serial hotplug

[Ján Tomko]

For domains with no USB address cache, we should not attempt
to generate a USB address.

https://bugzilla.redhat.com/show_bug.cgi?id=1387665
---
 src/qemu/qemu_hotplug.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 407ae73..9e9073d 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1652,7 +1652,8 @@ qemuDomainAttachChrDeviceAssignAddr(virDomainDefPtr def,
 return -1;
 return 1;
 
-} else if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL &&
+} else if (priv->usbaddrs &&
+   chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL &&
chr->targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_USB) {
 if (virDomainUSBAddressEnsure(priv->usbaddrs, >info) < 0)
 return -1;
-- 
2.7.3

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

[libvirt] [PATCH v2 10/15] qemu: capabilities: Add support for QMP schema introspection

[Peter Krempa]

Allow detecting capabilities according to the qemu QMP schema. This is
necessary as sometimes the availability of certain options depends on
the presence of a field in the schema.

This patch adds support for loading the QMP schema when detecting qemu
capabilities and adds a very simple query language to allow traversing
the schema and selecting a certain element from it.

The infrastructure in this patch uses a query path to set a specific
capability flag according to the availability of the given element in
the schema.
---

Notes:
v2:
- fixed typos
- added docs for one of the helper functions and changed param order
- added hint for query string format docs

 src/qemu/qemu_capabilities.c  | 191 ++
 src/qemu/qemu_capabilities.h  |   1 +
 tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml  |   1 +
 tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml |   1 +
 tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml  |   1 +
 tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml  |   1 +
 6 files changed, 196 insertions(+)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 9132469..c7ceb54 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -346,6 +346,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
   "virtio-net.rx_queue_size", /* 235 */
   "machine-iommu",
   "virtio-vga",
+  "query-qmp-schema",
 );


@@ -1487,6 +1488,7 @@ struct virQEMUCapsStringFlags virQEMUCapsCommands[] = {
 { "rtc-reset-reinjection", QEMU_CAPS_RTC_RESET_REINJECTION },
 { "migrate-incoming", QEMU_CAPS_INCOMING_DEFER },
 { "query-hotpluggable-cpus", QEMU_CAPS_QUERY_HOTPLUGGABLE_CPUS },
+{ "query-qmp-schema", QEMU_CAPS_QUERY_QMP_SCHEMA }
 };

 struct virQEMUCapsStringFlags virQEMUCapsMigration[] = {
@@ -1691,6 +1693,11 @@ static struct virQEMUCapsStringFlags 
virQEMUCapsObjectPropsUSBNECXHCI[] = {
 { "p3", QEMU_CAPS_NEC_USB_XHCI_PORTS },
 };

+/* see documentation for virQEMUCapsQMPSchemaGetByPath for the query format */
+static struct virQEMUCapsStringFlags virQEMUCapsQMPSchemaQueries[] = {
+{ "bogus/path/to/satisfy/compiler", 0 },
+};
+
 struct virQEMUCapsObjectTypeProps {
 const char *type;
 struct virQEMUCapsStringFlags *props;
@@ -3691,6 +3698,187 @@ virQEMUCapsInitArchQMPBasic(virQEMUCapsPtr qemuCaps,
 return ret;
 }

+
+/**
+ * virQEMUCapsQMPSchemaObjectGetType:
+ * @field: name of the object containing the requested type
+ * @name: name of the requested type
+ * @namefield: name of the object property holding @name
+ *
+ * Helper that selects the type of a QMP schema object or it's variant. Returns
+ * the  type string on success or NULL on error.
+ */
+static const char *
+virQEMUCapsQMPSchemaObjectGetType(const char *field,
+  const char *name,
+  const char *namefield,
+  virJSONValuePtr elem)
+{
+virJSONValuePtr arr;
+virJSONValuePtr cur;
+const char *curname;
+const char *type;
+size_t i;
+
+if (!(arr = virJSONValueObjectGetArray(elem, field)))
+return NULL;
+
+for (i = 0; i < virJSONValueArraySize(arr); i++) {
+if (!(cur = virJSONValueArrayGet(arr, i)) ||
+!(curname = virJSONValueObjectGetString(cur, namefield)) ||
+!(type = virJSONValueObjectGetString(cur, "type")))
+continue;
+
+if (STREQ(name, curname))
+return type;
+}
+
+return NULL;
+}
+
+
+static virJSONValuePtr
+virQEMUCapsQMPSchemaTraverse(const char *basename,
+ char **query,
+ virHashTablePtr schema)
+{
+virJSONValuePtr base;
+const char *metatype;
+
+do {
+if (!(base = virHashLookup(schema, basename)))
+return NULL;
+
+if (!*query)
+return base;
+
+if (!(metatype = virJSONValueObjectGetString(base, "meta-type")))
+return NULL;
+
+/* flatten arrays by default */
+if (STREQ(metatype, "array")) {
+if (!(basename = virJSONValueObjectGetString(base, 
"element-type")))
+return NULL;
+
+continue;
+} else if (STREQ(metatype, "object")) {
+if (**query == '+')
+basename = virQEMUCapsQMPSchemaObjectGetType("variants",
+ *query + 1,
+ "case", base);
+else
+basename = virQEMUCapsQMPSchemaObjectGetType("members",
+ *query,
+ "name", base);
+
+if (!basename)
+return NULL;
+} else if (STREQ(metatype, "command") ||
+   STREQ(metatype, "event")) {

[libvirt] [PATCH 1/6] Add 'FromCache' to virDomainVirtioSerialAddrAutoAssign

[Ján Tomko]

We dropped the cache from QEMU's private domain object.
Assume the callers do not have the cache by default and use
a longer name for the internal ones that do.

This makes the shorter 'virDomainVirtioSerialAddrAutoAssign'
name availabe for a function that will not require the cache.
---
 src/conf/domain_addr.c | 8 
 src/conf/domain_addr.h | 8 
 src/libvirt_private.syms   | 2 +-
 src/qemu/qemu_domain_address.c | 6 --
 src/qemu/qemu_hotplug.c| 8 
 5 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
index 1e96fe9..065baa7 100644
--- a/src/conf/domain_addr.c
+++ b/src/conf/domain_addr.c
@@ -1154,10 +1154,10 @@ 
virDomainVirtioSerialAddrNextFromController(virDomainVirtioSerialAddrSetPtr addr
  * or assign a virtio serial address to the device
  */
 int
-virDomainVirtioSerialAddrAutoAssign(virDomainDefPtr def,
-virDomainVirtioSerialAddrSetPtr addrs,
-virDomainDeviceInfoPtr info,
-bool allowZero)
+virDomainVirtioSerialAddrAutoAssignFromCache(virDomainDefPtr def,
+ virDomainVirtioSerialAddrSetPtr 
addrs,
+ virDomainDeviceInfoPtr info,
+ bool allowZero)
 {
 bool portOnly = info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL;
 if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL &&
diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h
index 0072a08..b0fadc1 100644
--- a/src/conf/domain_addr.h
+++ b/src/conf/domain_addr.h
@@ -222,10 +222,10 @@ 
virDomainVirtioSerialAddrSetCreateFromDomain(virDomainDefPtr def)
 bool
 virDomainVirtioSerialAddrIsComplete(virDomainDeviceInfoPtr info);
 int
-virDomainVirtioSerialAddrAutoAssign(virDomainDefPtr def,
-virDomainVirtioSerialAddrSetPtr addrs,
-virDomainDeviceInfoPtr info,
-bool allowZero)
+virDomainVirtioSerialAddrAutoAssignFromCache(virDomainDefPtr def,
+ virDomainVirtioSerialAddrSetPtr 
addrs,
+ virDomainDeviceInfoPtr info,
+ bool allowZero)
 ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
 
 int
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index bf503a5..8bc2584 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -123,7 +123,7 @@ virDomainUSBAddressSetAddHub;
 virDomainUSBAddressSetCreate;
 virDomainUSBAddressSetFree;
 virDomainVirtioSerialAddrAssign;
-virDomainVirtioSerialAddrAutoAssign;
+virDomainVirtioSerialAddrAutoAssignFromCache;
 virDomainVirtioSerialAddrIsComplete;
 virDomainVirtioSerialAddrRelease;
 virDomainVirtioSerialAddrReserve;
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index dc67d51..7d92e0b 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -122,7 +122,8 @@ qemuDomainAssignVirtioSerialAddresses(virDomainDefPtr def)
 if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE &&
 chr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO &&
 !virDomainVirtioSerialAddrIsComplete(>info) &&
-virDomainVirtioSerialAddrAutoAssign(def, addrs, >info, true) 
< 0)
+virDomainVirtioSerialAddrAutoAssignFromCache(def, addrs,
+ >info, true) < 0)
 goto cleanup;
 }
 
@@ -131,7 +132,8 @@ qemuDomainAssignVirtioSerialAddresses(virDomainDefPtr def)
 if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL &&
 chr->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO &&
 !virDomainVirtioSerialAddrIsComplete(>info) &&
-virDomainVirtioSerialAddrAutoAssign(def, addrs, >info, false) 
< 0)
+virDomainVirtioSerialAddrAutoAssignFromCache(def, addrs,
+ >info, false) < 
0)
 goto cleanup;
 }
 
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 2cb2267..7bd38ab 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1648,8 +1648,8 @@ qemuDomainAttachChrDeviceAssignAddr(virDomainDefPtr def,
 
 if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE &&
 chr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO) {
-if (virDomainVirtioSerialAddrAutoAssign(NULL, vioaddrs,
->info, true) < 0)
+if (virDomainVirtioSerialAddrAutoAssignFromCache(NULL, vioaddrs,
+ >info, true) < 0)
 goto cleanup;
 ret = 1;
 
@@ -1667,8 +1667,8 @@ 

[libvirt] [PATCH 6/6] Do not try to release virtio serial addresses

[Ján Tomko]

Return 0 instead of 1, so that qemuDomainAttachChrDevice does not
assume the address neeeds to be released on error.

No functional change, since qemuDomainReleaseDeviceAddress has been a noop
for virtio serial addresses since the address cache was removed.
---
 src/qemu/qemu_hotplug.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 9e9073d..d432616 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1635,6 +1635,10 @@ qemuDomainChrRemove(virDomainDefPtr vmdef,
 return ret;
 }
 
+/* Returns  1 if the address will need to be released later,
+ * -1 on error
+ *  0 otherwise
+ */
 static int
 qemuDomainAttachChrDeviceAssignAddr(virDomainDefPtr def,
 qemuDomainObjPrivatePtr priv,
@@ -1644,7 +1648,7 @@ qemuDomainAttachChrDeviceAssignAddr(virDomainDefPtr def,
 chr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO) {
 if (virDomainVirtioSerialAddrAutoAssign(def, >info, true) < 0)
 return -1;
-return 1;
+return 0;
 
 } else if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL &&
chr->targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_PCI) {
@@ -1663,7 +1667,7 @@ qemuDomainAttachChrDeviceAssignAddr(virDomainDefPtr def,
chr->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO) {
 if (virDomainVirtioSerialAddrAutoAssign(def, >info, false) < 0)
 return -1;
-return 1;
+return 0;
 }
 
 if (chr->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL ||
-- 
2.7.3

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

[libvirt] [PATCH 0/6] Address assignment fixes

[Ján Tomko]

Fixes a crash on usb-serial hotplug and missing addresses
after libvirtd restart, and makes some code more readable.

Ján Tomko (6):
  Add 'FromCache' to  virDomainVirtioSerialAddrAutoAssign
  Introduce virDomainVirtioSerialAddrAutoAssign again
  Return directly from qemuDomainAttachChrDeviceAssignAddr
  Also create the USB address cache for domains with all the USB
addresses
  Fix crash on usb-serial hotplug
  Do not try to release virtio serial addresses

 src/conf/domain_addr.c | 41 +
 src/conf/domain_addr.h | 14 +++--
 src/libvirt_private.syms   |  2 ++
 src/qemu/qemu_domain_address.c | 22 
 src/qemu/qemu_hotplug.c| 46 +-
 5 files changed, 87 insertions(+), 38 deletions(-)

-- 
2.7.3

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

[libvirt] [PATCH 2/6] Introduce virDomainVirtioSerialAddrAutoAssign again

[Ján Tomko]

This time do not require an address cache as a parameter.

Simplify qemuDomainAttachChrDeviceAssignAddr to not generate
the virtio serial address cache for devices of other types.
---
 src/conf/domain_addr.c   | 21 +
 src/conf/domain_addr.h   |  6 ++
 src/libvirt_private.syms |  1 +
 src/qemu/qemu_hotplug.c  | 11 ++-
 4 files changed, 30 insertions(+), 9 deletions(-)

diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
index 065baa7..92a5516 100644
--- a/src/conf/domain_addr.c
+++ b/src/conf/domain_addr.c
@@ -1167,6 +1167,27 @@ 
virDomainVirtioSerialAddrAutoAssignFromCache(virDomainDefPtr def,
 return virDomainVirtioSerialAddrAssign(def, addrs, info, allowZero, 
portOnly);
 }
 
+int
+virDomainVirtioSerialAddrAutoAssign(virDomainDefPtr def,
+virDomainDeviceInfoPtr info,
+bool allowZero)
+{
+virDomainVirtioSerialAddrSetPtr addrs = NULL;
+int ret = -1;
+
+if (!(addrs = virDomainVirtioSerialAddrSetCreateFromDomain(def)))
+goto cleanup;
+
+if (virDomainVirtioSerialAddrAutoAssignFromCache(def, addrs, info, 
allowZero) < 0)
+goto cleanup;
+
+ret = 0;
+
+ cleanup:
+virDomainVirtioSerialAddrSetFree(addrs);
+return ret;
+}
+
 
 int
 virDomainVirtioSerialAddrAssign(virDomainDefPtr def,
diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h
index b0fadc1..141f83b 100644
--- a/src/conf/domain_addr.h
+++ b/src/conf/domain_addr.h
@@ -229,6 +229,12 @@ 
virDomainVirtioSerialAddrAutoAssignFromCache(virDomainDefPtr def,
 ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
 
 int
+virDomainVirtioSerialAddrAutoAssign(virDomainDefPtr def,
+virDomainDeviceInfoPtr info,
+bool allowZero)
+ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
+
+int
 virDomainVirtioSerialAddrAssign(virDomainDefPtr def,
 virDomainVirtioSerialAddrSetPtr addrs,
 virDomainDeviceInfoPtr info,
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 8bc2584..ce9c4c4 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -123,6 +123,7 @@ virDomainUSBAddressSetAddHub;
 virDomainUSBAddressSetCreate;
 virDomainUSBAddressSetFree;
 virDomainVirtioSerialAddrAssign;
+virDomainVirtioSerialAddrAutoAssign;
 virDomainVirtioSerialAddrAutoAssignFromCache;
 virDomainVirtioSerialAddrIsComplete;
 virDomainVirtioSerialAddrRelease;
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 7bd38ab..cbdcd81 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1641,15 +1641,10 @@ qemuDomainAttachChrDeviceAssignAddr(virDomainDefPtr def,
 virDomainChrDefPtr chr)
 {
 int ret = -1;
-virDomainVirtioSerialAddrSetPtr vioaddrs = NULL;
-
-if (!(vioaddrs = virDomainVirtioSerialAddrSetCreateFromDomain(def)))
-goto cleanup;
 
 if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE &&
 chr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO) {
-if (virDomainVirtioSerialAddrAutoAssignFromCache(NULL, vioaddrs,
- >info, true) < 0)
+if (virDomainVirtioSerialAddrAutoAssign(def, >info, true) < 0)
 goto cleanup;
 ret = 1;
 
@@ -1667,8 +1662,7 @@ qemuDomainAttachChrDeviceAssignAddr(virDomainDefPtr def,
 
 } else if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL &&
chr->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO) {
-if (virDomainVirtioSerialAddrAutoAssignFromCache(NULL, vioaddrs,
- >info, false) < 
0)
+if (virDomainVirtioSerialAddrAutoAssign(def, >info, false) < 0)
 goto cleanup;
 ret = 1;
 }
@@ -1686,7 +1680,6 @@ qemuDomainAttachChrDeviceAssignAddr(virDomainDefPtr def,
 ret = 0;
 
  cleanup:
-virDomainVirtioSerialAddrSetFree(vioaddrs);
 return ret;
 }
 
-- 
2.7.3

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

Re: [libvirt] [PATCH 10/15] qemu: capabilities: Add support for QMP schema introspection

[Peter Krempa]

On Thu, Oct 20, 2016 at 15:05:21 -0500, Eric Blake wrote:
> On 10/20/2016 10:25 AM, Peter Krempa wrote:

[...]

> > +continue;
> > +} else if (STREQ(metatype, "object")) {
> > +if (**query == '+')
> > +basename = virQEMUCapsQMPSchemaObjectGetType(*query + 1,
> > + "variants",
> > + "case", base);
> > +else
> > +basename = virQEMUCapsQMPSchemaObjectGetType(*query,
> > + "members",
> > + "name",
> > + base);
> 
> I'm a bit worried here.  Qemu promises that a name should not disappear
> from a QMP command, but warns that between releases, locating that name
> may migrate from the 'names' array (always present) to the 'variants'
> array (present only under some circumstances), based on what else was
> added to the command in the meantime.  I guess what that means is that
> when qemu re-does a type layout, it may mean that we need two separate
> query paths (one for the old layout, one for the new) to properly detect
> the existence of that capability under all versions of qemu.  I don't
> think it is a show-stopper, but is something to be aware of.

This is very unpleasant to code against. Looking into all variants would
certainly be wrong in some cases.i

I think we can possibly add a '*' operator for the variant, which could
iterate all the variants and find the first match. I've prepared the
function for recursion as I wanted to implement the 'alternate's this
way.


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

Re: [libvirt] [PATCH 0/2] vz: add serial number to disk devices

[Maxim Nestratov]

22-Sep-16 17:55, Nikolay Shirokovskiy пишет:


Only the first patch is really on the subject. The second one is
bugfix that is included mainly because it touches the same place
(and nice to have to test first patch too...)

Nikolay Shirokovskiy (2):
   vz: add serial number to disk devices
   vz: set something in disk driver name

  src/vz/vz_sdk.c   | 16 
  src/vz/vz_utils.c |  6 +++---
  2 files changed, 19 insertions(+), 3 deletions(-)



After offline discussion we decided to stick to this patch series.
ACKed and pushed now.

Maxim

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

Re: [libvirt] [PATCH v2 0/3] qemu: util: vz: support setting disk serial number

[Maxim Nestratov]

21-Oct-16 12:50, Maxim Nestratov пишет:


Actually this is a combination of the two previous series
https://www.redhat.com/archives/libvir-list/2016-October/msg00890.html
and
https://www.redhat.com/archives/libvir-list/2016-September/msg01024.html

Maxim Nestratov (1):
   util: qemu: make qemuSafeSerialParamValue function usable by other
 drivers

Nikolay Shirokovskiy (2):
   vz: set something in disk driver name
   vz: add serial number to disk devices

  src/libvirt_private.syms |  1 +
  src/qemu/qemu_command.c  | 19 +--
  src/util/virutil.c   | 16 
  src/util/virutil.h   |  2 ++
  src/vz/vz_sdk.c  | 24 
  src/vz/vz_utils.c|  6 +++---
  6 files changed, 47 insertions(+), 21 deletions(-)



Disregard this please. I pushed the original series:
https://www.redhat.com/archives/libvir-list/2016-September/msg01024.html

Maxim


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

Re: [libvirt] [PATCH 11/15] qemu: capabilities: Detect support for gluster debug setting

[Peter Krempa]

On Thu, Oct 20, 2016 at 15:17:50 -0500, Eric Blake wrote:
> On 10/20/2016 10:25 AM, Peter Krempa wrote:
> > From: Prasanna Kumar Kalever 
> > 
> > Teach qemu driver to detect whether qemu supports specifying debug level
> > for gluster volumes.
> > 
> > Signed-off-by: Prasanna Kumar Kalever 
> > Signed-off-by: Peter Krempa 
> > ---
> >  src/qemu/qemu_capabilities.c | 3 ++-
> >  src/qemu/qemu_capabilities.h | 1 +
> >  tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml | 1 +
> >  3 files changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> > index 97e29db..7f0a47b 100644
> > --- a/src/qemu/qemu_capabilities.c
> > +++ b/src/qemu/qemu_capabilities.c
> > @@ -347,6 +347,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
> >"machine-iommu",
> >"virtio-vga",
> >"query-qmp-schema",
> > +  "gluster.debug_level",
> >  );
> > 
> > 
> > @@ -1694,7 +1695,7 @@ static struct virQEMUCapsStringFlags 
> > virQEMUCapsObjectPropsUSBNECXHCI[] = {
> >  };
> > 
> >  static struct virQEMUCapsStringFlags virQEMUCapsQMPSchemaQueries[] = {
> > -{ "bogus/path/to/satisfy/compiler", 0 },
> > +{ "blockdev-add/arg-type/options/+gluster/debug-level", 
> > QEMU_CAPS_GLUSTER_DEBUG_LEVEL},
> 
> Okay, I see how you plan to use this.
> 
> BIG WARNING FLAG - in qemu 2.7, blockdev-add is unstable (it should have
> been named x-blockdev-add; but we goofed); but you can use the existence
> of x-blockdev-del as a witness that blockdev-add is incomplete.
> 
> In qemu 2.8, we are taking advantage of the fact that 2.7 wasn't
> complete in order to get rid of the 'options' nesting layer; the plan is
> to also rename x-blockdev-add at the same time.  Which means this probe,
> as written, will only succeed for 2.7.

So. Should I add both? One with the "options" indirection and one
without? Since there is no other way to access the structure than via
blockdev-add and I really don't want to add a version check I see it as
the only option.



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

[libvirt] [PATCH 3/3] conf: Move the privateData from virDomainChrDef to virDomainChrSourceDef

[John Ferlan]

Commit id '5f2a132786' should have placed the data in the host source
def structure since that's also used by smartcard, redirdev, and rng in
order to provide a backend tcp channel.  The data in the private structure
will be necessary in order to provide the secret properly

Signed-off-by: John Ferlan 
---
 src/conf/domain_conf.c | 17 +++--
 src/conf/domain_conf.h |  2 +-
 2 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 2b89ea2..a684c48 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2024,6 +2024,7 @@ void virDomainChrSourceDefFree(virDomainChrSourceDefPtr 
def)
 return;
 
 virDomainChrSourceDefClear(def);
+virObjectUnref(def->privateData);
 
 VIR_FREE(def);
 }
@@ -2125,8 +2126,6 @@ void virDomainChrDefFree(virDomainChrDefPtr def)
 VIR_FREE(def->seclabels);
 }
 
-virObjectUnref(def->privateData);
-
 VIR_FREE(def);
 }
 
@@ -10318,13 +10317,17 @@ 
virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
 
 
 static virDomainChrSourceDefPtr
-virDomainChrSourceDefNew(void)
+virDomainChrSourceDefNew(virDomainXMLOptionPtr xmlopt)
 {
 virDomainChrSourceDefPtr def = NULL;
 
 if (VIR_ALLOC(def) < 0)
 return NULL;
 
+if (xmlopt && xmlopt->privateData.chardevNew &&
+!(def->privateData = xmlopt->privateData.chardevNew()))
+VIR_FREE(def);
+
 return def;
 }
 
@@ -10342,15 +10345,9 @@ virDomainChrDefNew(virDomainXMLOptionPtr xmlopt)
 
 def->target.port = -1;
 
-if (!(def->source = virDomainChrSourceDefNew()))
+if (!(def->source = virDomainChrSourceDefNew(xmlopt)))
 VIR_FREE(def);
 
-if (xmlopt && xmlopt->privateData.chardevNew &&
-!(def->privateData = xmlopt->privateData.chardevNew())) {
-virDomainChrSourceDefFree(def->source);
-VIR_FREE(def);
-}
-
 return def;
 }
 
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 5247d34..97ffcf4 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1078,6 +1078,7 @@ typedef enum {
 /* The host side information for a character device.  */
 struct _virDomainChrSourceDef {
 int type; /* virDomainChrType */
+virObjectPtr privateData;
 union {
 /* no  for null, vc, stdio */
 struct {
@@ -1117,7 +1118,6 @@ struct _virDomainChrSourceDef {
 /* A complete character device, both host and domain views.  */
 struct _virDomainChrDef {
 int deviceType; /* enum virDomainChrDeviceType */
-virObjectPtr privateData;
 
 bool targetTypeAttr;
 int targetType; /* enum virDomainChrConsoleTargetType ||
-- 
2.7.4

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

[libvirt] [PATCH 2/3] Introduce virDomainChrSourceDefNew for virDomainChrDefPtr

[John Ferlan]

Change the virDomainChrDef to use a pointer to 'source' and allocate
that pointer during virDomainChrDefNew.

This has tremendous "fallout" in the rest of the code which mainly
has to change source.$field to source->$field.

Signed-off-by: John Ferlan 
---
 src/bhyve/bhyve_command.c|  8 ++---
 src/bhyve/bhyve_driver.c |  2 +-
 src/bhyve/bhyve_parse_command.c  | 20 +--
 src/conf/domain_audit.c  |  4 +--
 src/conf/domain_conf.c   | 50 +-
 src/conf/domain_conf.h   |  2 +-
 src/libxl/libxl_conf.c   | 42 +++---
 src/libxl/libxl_domain.c | 22 ++--
 src/libxl/libxl_driver.c |  4 +--
 src/lxc/lxc_driver.c |  4 +--
 src/lxc/lxc_native.c |  2 +-
 src/lxc/lxc_process.c|  6 ++--
 src/qemu/qemu_cgroup.c   |  2 +-
 src/qemu/qemu_command.c  | 28 +++
 src/qemu/qemu_domain.c   | 16 -
 src/qemu/qemu_driver.c   |  8 ++---
 src/qemu/qemu_hotplug.c  |  6 ++--
 src/qemu/qemu_parse_command.c|  4 +--
 src/qemu/qemu_process.c  | 28 +++
 src/security/security_dac.c  |  4 +--
 src/security/security_selinux.c  |  4 +--
 src/security/virt-aa-helper.c| 70 ++--
 src/uml/uml_conf.c   | 14 
 src/uml/uml_driver.c | 12 +++
 src/vbox/vbox_common.c   | 46 
 src/vmx/vmx.c| 74 +++---
 src/vz/vz_sdk.c  | 78 
 src/xen/xen_driver.c |  4 +--
 src/xenconfig/xen_sxpr.c | 76 +++
 src/xenconfig/xen_xl.c   | 14 
 tests/securityselinuxlabeltest.c | 14 
 31 files changed, 343 insertions(+), 325 deletions(-)

diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c
index 55ad950..022b03b 100644
--- a/src/bhyve/bhyve_command.c
+++ b/src/bhyve/bhyve_command.c
@@ -130,7 +130,7 @@ bhyveBuildConsoleArgStr(const virDomainDef *def, 
virCommandPtr cmd)
 
 chr = def->serials[0];
 
-if (chr->source.type != VIR_DOMAIN_CHR_TYPE_NMDM) {
+if (chr->source->type != VIR_DOMAIN_CHR_TYPE_NMDM) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("only nmdm console types are supported"));
 return -1;
@@ -146,7 +146,7 @@ bhyveBuildConsoleArgStr(const virDomainDef *def, 
virCommandPtr cmd)
 virCommandAddArgList(cmd, "-s", "1,lpc", NULL);
 virCommandAddArg(cmd, "-l");
 virCommandAddArgFormat(cmd, "com%d,%s",
-   chr->target.port + 1, chr->source.data.file.path);
+   chr->target.port + 1, chr->source->data.file.path);
 
 return 0;
 }
@@ -505,14 +505,14 @@ virBhyveProcessBuildGrubbhyveCmd(virDomainDefPtr def,
 
 chr = def->serials[0];
 
-if (chr->source.type != VIR_DOMAIN_CHR_TYPE_NMDM) {
+if (chr->source->type != VIR_DOMAIN_CHR_TYPE_NMDM) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("only nmdm console types are supported"));
 return NULL;
 }
 
 virCommandAddArg(cmd, "--cons-dev");
-virCommandAddArg(cmd, chr->source.data.file.path);
+virCommandAddArg(cmd, chr->source->data.file.path);
 }
 
 /* VM name */
diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c
index 04be78b..38fb9f0 100644
--- a/src/bhyve/bhyve_driver.c
+++ b/src/bhyve/bhyve_driver.c
@@ -1080,7 +1080,7 @@ bhyveDomainOpenConsole(virDomainPtr dom,
 
 chr = vm->def->serials[0];
 
-if (virFDStreamOpenPTY(st, chr->source.data.nmdm.slave,
+if (virFDStreamOpenPTY(st, chr->source->data.nmdm.slave,
0, 0, O_RDWR) < 0)
 goto cleanup;
 
diff --git a/src/bhyve/bhyve_parse_command.c b/src/bhyve/bhyve_parse_command.c
index 1a7de3d..0ae7a83 100644
--- a/src/bhyve/bhyve_parse_command.c
+++ b/src/bhyve/bhyve_parse_command.c
@@ -290,9 +290,9 @@ bhyveParseBhyveLPCArg(virDomainDefPtr def,
 if (!(chr = virDomainChrDefNew(NULL)))
 goto error;
 
-chr->source.type = VIR_DOMAIN_CHR_TYPE_NMDM;
-chr->source.data.nmdm.master = NULL;
-chr->source.data.nmdm.slave = NULL;
+chr->source->type = VIR_DOMAIN_CHR_TYPE_NMDM;
+chr->source->data.nmdm.master = NULL;
+chr->source->data.nmdm.slave = NULL;
 chr->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL;
 
 if (!STRPREFIX(param, "/dev/nmdm")) {
@@ -302,12 +302,12 @@ bhyveParseBhyveLPCArg(virDomainDefPtr def,
 goto error;
 }
 
-if (VIR_STRDUP(chr->source.data.nmdm.master, param) < 0) {
+if (VIR_STRDUP(chr->source->data.nmdm.master, param) < 0) {
 virDomainChrDefFree(chr);
 goto error;
 }
 
-if 

[libvirt] [PATCH 1/3] vmx: Use the allocator virDomainChrDefNew

[John Ferlan]

Rather than VIR_ALLOC of the virDomainChrDefPtr

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

diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
index fc4347f..f61c4d6 100644
--- a/src/vmx/vmx.c
+++ b/src/vmx/vmx.c
@@ -2758,7 +2758,7 @@ virVMXParseSerial(virVMXContext *ctx, virConfPtr conf, 
int port,
 return -1;
 }
 
-if (VIR_ALLOC(*def) < 0)
+if (!(*def = virDomainChrDefNew(NULL)))
 return -1;
 
 (*def)->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL;
@@ -2946,7 +2946,7 @@ virVMXParseParallel(virVMXContext *ctx, virConfPtr conf, 
int port,
 return -1;
 }
 
-if (VIR_ALLOC(*def) < 0)
+if (!(*def = virDomainChrDefNew(NULL)))
 return -1;
 
 (*def)->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL;
-- 
2.7.4

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

[libvirt] [PATCH 0/3] Alter 'source' to be virDomainChrSourceDefPtr in _virDomainChrDef

[John Ferlan]

Yes, it's absolutely terrifying; however, in order to "fix" problems noted
during the review of the TLS chardev TCP 'properly' making the 'source' be
a pointer and then moving the 'privateData' from _virDomainChrDef to
_virDomainChrSourceDef will allow for setting of the 'secinfo' for the
smartcard, rng, and redirdev backends that use a TCP channel.

95% of the changes obviously fell out of build issues from a 'make check
syntax-check'. The remaining 5% of the changes are from sources I don't
normally build; however, those were found by using cscope on the sources
and finding references of "source\.{type|data|logfile|logappend} (IOW:
virDomainChrSourceDef members).

John Ferlan (3):
  vmx: Use the allocator virDomainChrDefNew
  Introduce virDomainChrSourceDefNew for virDomainChrDefPtr
  conf: Move the privateData from virDomainChrDef to
virDomainChrSourceDef

 src/bhyve/bhyve_command.c|  8 ++---
 src/bhyve/bhyve_driver.c |  2 +-
 src/bhyve/bhyve_parse_command.c  | 20 +--
 src/conf/domain_audit.c  |  4 +--
 src/conf/domain_conf.c   | 53 +--
 src/conf/domain_conf.h   |  4 +--
 src/libxl/libxl_conf.c   | 42 +++---
 src/libxl/libxl_domain.c | 22 ++--
 src/libxl/libxl_driver.c |  4 +--
 src/lxc/lxc_driver.c |  4 +--
 src/lxc/lxc_native.c |  2 +-
 src/lxc/lxc_process.c|  6 ++--
 src/qemu/qemu_cgroup.c   |  2 +-
 src/qemu/qemu_command.c  | 28 +++
 src/qemu/qemu_domain.c   | 16 -
 src/qemu/qemu_driver.c   |  8 ++---
 src/qemu/qemu_hotplug.c  |  6 ++--
 src/qemu/qemu_parse_command.c|  4 +--
 src/qemu/qemu_process.c  | 28 +++
 src/security/security_dac.c  |  4 +--
 src/security/security_selinux.c  |  4 +--
 src/security/virt-aa-helper.c| 70 ++--
 src/uml/uml_conf.c   | 14 
 src/uml/uml_driver.c | 12 +++
 src/vbox/vbox_common.c   | 46 
 src/vmx/vmx.c| 78 
 src/vz/vz_sdk.c  | 78 
 src/xen/xen_driver.c |  4 +--
 src/xenconfig/xen_sxpr.c | 76 +++
 src/xenconfig/xen_xl.c   | 14 
 tests/securityselinuxlabeltest.c | 14 
 31 files changed, 346 insertions(+), 331 deletions(-)

-- 
2.7.4

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

[libvirt] [PATCH] bhyve: Need to add parameter to virDomainChrDefNew

[John Ferlan]

Commit id '5f2a13278' missed this one.

Signed-off-by: John Ferlan 
---

 Pushed as build breaker
 
https://ci.centos.org/view/libvirt-project/job/libvirt-master-build/150/systems=libvirt-freebsd/

 src/bhyve/bhyve_parse_command.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/bhyve/bhyve_parse_command.c b/src/bhyve/bhyve_parse_command.c
index fd9384d..1a7de3d 100644
--- a/src/bhyve/bhyve_parse_command.c
+++ b/src/bhyve/bhyve_parse_command.c
@@ -287,7 +287,7 @@ bhyveParseBhyveLPCArg(virDomainDefPtr def,
 
 /* Only support com%d */
 if (STRPREFIX(type, "com") && type[4] == 0) {
-if (!(chr = virDomainChrDefNew()))
+if (!(chr = virDomainChrDefNew(NULL)))
 goto error;
 
 chr->source.type = VIR_DOMAIN_CHR_TYPE_NMDM;
-- 
2.7.4

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

Re: [libvirt] [PATCH 4/4] qemuBuildHostNetStr: remove dead code

[Pavel Hrdina]

On Fri, Oct 14, 2016 at 04:32:18PM +0200, Ján Tomko wrote:
> This function is never called for VIR_DOMAIN_NET_TYPE_HOSTDEV,
> and the dead code comment agrees.
> 
> Introduced by commit 1dcbef8a.
> ---

ACK

Pavel


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

Re: [libvirt] [PATCH 3/4] qemuBuildHostNetStr: move stray VIR_DOMAIN_NET_TYPE_INTERNAL

[Pavel Hrdina]

On Fri, Oct 14, 2016 at 04:32:17PM +0200, Ján Tomko wrote:
> This network type is only used by the vbox driver and it does not
> make sense to group it with VIR_DOMAIN_NET_TYPE_USER.
> 
> Introduced by commit 1dcbef8.
> ---
>  src/qemu/qemu_command.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 80ebe51..19ee652 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -3719,7 +3719,6 @@ qemuBuildHostNetStr(virDomainNetDefPtr net,
>  break;
>  
>  case VIR_DOMAIN_NET_TYPE_USER:
> -case VIR_DOMAIN_NET_TYPE_INTERNAL:
>  virBufferAsprintf(, "user%c", type_sep);
>  break;
>  
> @@ -3737,6 +3736,8 @@ qemuBuildHostNetStr(virDomainNetDefPtr net,
>net->driver.virtio.queues);
>  break;
>  
> +case VIR_DOMAIN_NET_TYPE_INTERNAL:
> +/* Not supported by QEMU driver */
>  case VIR_DOMAIN_NET_TYPE_LAST:
>  break;
>  }

This one is tricky.  The net type *internal* was introduced for vbox driver,
but there was no check in other drivers to refuse this type.  Before commit
1dcbef8 this code handled all not listed cases the same way as net type *user*
and that goes back before the *internal* type was introduced.  So basically for
qemu driver we've handled *internal* as *user* since it was introduced in
libvirt.

Personaly I don't like that fact and IMO we can add a code to
${driver}DomainDefValidate() to refuse that type except vbox driver.
The argument to do it is that the net type *internal* is not documented
so there is a chance that it's not used at all for other drivers except vbox.

This patch resolves in this error while starting a guest:

error: internal error: process exited while connecting to monitor:
2016-10-21T12:28:46.841029Z qemu-system-x86_64: -netdev id=hostnet1: Parameter
'type' is missing

Pavel


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

Re: [libvirt] [PATCH v10 2/4] domain: Add optional 'tls' attribute for TCP chardev

[John Ferlan]

On 10/21/2016 07:57 AM, Pavel Hrdina wrote:
> On Fri, Oct 21, 2016 at 07:15:55AM -0400, John Ferlan wrote:
>>
>>
>> On 10/21/2016 02:29 AM, Pavel Hrdina wrote:
>>> On Thu, Oct 20, 2016 at 03:48:30PM -0400, John Ferlan wrote:
 [...]

>>
>> Since I assume you have these changes in your local branch - let's go
>> with the two patches and move on.
>>
>> It would be nice while it's still fresh to update the documentation, but
>> that's a separate patch.
>>
>> So "officially" it's an ACK of your changes on top of and in addition to
>> my changes.
>>
>> John
> 
> We should probably figure out also rng, smartcard and redirdevs before 
> pushing.
> They also use the source as you've pointed out and currently they can be
> configured to use TLS with the chardev_tls config option.  I'll send a new
> version of patch series to cover all users of TCP source type.
> 

The rng, smartcard, and redirdevs for hotplug is a different issue
though.  They come more into play when adding the passphrase.

As painful as it is (or will be to review) - I'm trying to move the
privateData from virDomainChrDef to virDomainChrSourceDef. That will
then magically do the right thing for rng, smartcard, and redirdevs.
Right now for command line processing they work fine since adding the
tls-creds is based on virDomainChrSourceDef


John

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

Re: [libvirt] [REPOST] regarding cgroup v2 support in libvirt

[Lennart Poettering]

On Fri, 21.10.16 11:19, Daniel P. Berrange (berra...@redhat.com) wrote:

> On Thu, Oct 20, 2016 at 02:59:45PM -0400, Tejun Heo wrote:
> > (reposting w/ libvir-list cc'd, sorry about the delay in reposting,
> >  was traveling and then on vacation)
> > 
> > Hello, Daniel.  How have you been?
> > 
> > We (facebook) are deploying cgroup v2 and internally use libvirt to
> > manage virtual machines, so I'm trying to add cgroup v2 support to
> > libvirt.
> > 
> > Because cgroup v2's resource configurations differ from v1 in varying
> > degrees depending on the specific resource type, it unfortunately
> > introduces new configurations (some completely new configs, others
> > just a different range / format).  This means that adding cgroup v2
> > support to libvirt requires adding new config options to it and maybe
> > implementing some form of translation mechanism between overlapping
> > configs.
> > 
> > The upcoming systemd release includes all that's necessary to support
> > v1/v2 compatibility so that users setting resource configs through
> > systemd don't have to worry about whether v1 or v2 is in use.  I'm
> > wondering whether it would make sense to make libvirt use dbus calls
> > to systemd to set resource configs when systemd is in use, so that it
> > can piggyback on systemd's v1/v2 compatibility.
> 
> The big question I have around cgroup v2 is state of support for all
> controllers that libvirt uses (cpu, cpuacct, cpuset, memory, devices,
> freezer, blkio).  IIUC, not all of these have been ported to cgroup
> v2 setup and the cpu port in particular was rejected by Linux maintainers.
> Libvirt has a general policy that we won't support features that only
> exist in out of tree patches (applies to kernel and any other software
> we build against or use).
> 
> IIRC from earlier discussions, the model for dealing with processes in
> cgroup v2 was quite different. In libvirt we rely on the ability to
> assign different threads within a process to different cgroups, because
> we need to control CPU schedular parameters on different threads in
> QEMU. eg we have vCPU threads, I/O threads and general emulator threads
> each of which get different policies.
> 
> When I spoke with Lennart about cgroup v2, way back in Jan, he indicated
> that while systemd can technically work with a system where some
> controllers are mounted as v1, while others are mounted as v2, this
> would not be an officially supported solution. Thus systemd in  Fedora
> was not likely to switch to v2 until all required controllers could use
> v2. I'm not sure if this still corresponds to Lennarts current views, so
> CC'ing him to confirm/deny.

So, the "hybrid" mode is probably nothing RHEL or so would want to
support. However, I think it might be a good step for Fedora at
least. But yes, supporting this mode means additional porting effort
for the various daemons that access cgroupfs...

> I recall that systemd policy for v2 was inteded to be that no app
> should write to cgroup sysfs except for systemd, unless there was
> a sub-tree created with Delegate=yes set on the scope. So this clearly
> means when using v2 we'll have to use the systemd DBus APIs for managing
> cgroups v2 on such hosts.

Yes, this is our policy: the cgroup tree is private property of
systemd (at least regarding write access), except when your have a
service or scope unit where Delegate=yes is set, in which case you can
manage your own subtree of that freely.

Lennart

-- 
Lennart Poettering, Red Hat

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

Re: [libvirt] [PATCH 2/4] qemuBuildHostNetStr: do not start options with a comma

[Pavel Hrdina]

On Fri, Oct 14, 2016 at 04:32:16PM +0200, Ján Tomko wrote:
> Put the comma at the end and trim it later for consistency.
> ---

ACK

Pavel


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/4] qemuBuildHostNetStr: use type_sep earlier

[Pavel Hrdina]

On Fri, Oct 14, 2016 at 04:32:15PM +0200, Ján Tomko wrote:
> When hotplugging networks with ancient QEMUs not supporting
> QEMU_CAPS_NETDEV, we use space instead of a comma as the separator
> between the network type and other options.
> 
> Except for "user", all the network types pass other options
> and use up the first separator by the time we get to the section
> that adds the alias (or vlan for QEMUs without CAPS_NETDEV).
> 
> Since the alias/vlan is mandatory, convert all preceding code to add
> the separator at the end, removing the need to rewrite type_sep for
> all types but NET_TYPE_USER.
> ---

ACK

Pavel


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

Re: [libvirt] [PATCH v10 2/4] domain: Add optional 'tls' attribute for TCP chardev

[Pavel Hrdina]

On Fri, Oct 21, 2016 at 07:15:55AM -0400, John Ferlan wrote:
> 
> 
> On 10/21/2016 02:29 AM, Pavel Hrdina wrote:
> > On Thu, Oct 20, 2016 at 03:48:30PM -0400, John Ferlan wrote:
> >> [...]
> >>
> 
> Since I assume you have these changes in your local branch - let's go
> with the two patches and move on.
> 
> It would be nice while it's still fresh to update the documentation, but
> that's a separate patch.
> 
> So "officially" it's an ACK of your changes on top of and in addition to
> my changes.
> 
> John

We should probably figure out also rng, smartcard and redirdevs before pushing.
They also use the source as you've pointed out and currently they can be
configured to use TLS with the chardev_tls config option.  I'll send a new
version of patch series to cover all users of TCP source type.

Pavel

> 
> [...]
> 
> > Now I see why we are not able to agree on this change.  The modification 
> > done
> > in qemuProcessPrepareDomain() are only to live definition, the config
> > definition remains untouched, which means the *tls* attribute (if it's 
> > based on
> > qemu.conf) will appear only in live XML.  The config XML will remain the 
> > same
> > after the guest is stopped.
> > 
> >> I think it should be strictly optional and that's where we differ. I see
> >> no reason to change the domain xml unless as a consumer that's what you
> >> want to do - be able to control which domains will have the setting.
> >> What else would be the purpose of a host wide setting to go with a
> >> domain optional setting?
> >>
> >> Finally, if your idea is accepted, that means for any configuration with
> >> chardev_tls=0 (either because it's commented or set that way), every
> >> domain that starts will be updated to have this new attribute
> >> "tls='no'". Then one day, I read up on this wonderful new feature and
> > 
> > Not the domain, only the live XML which is not saved as config XML ...
> > 
> >> modify my qemu.conf file to set chardev_tls=1 and set up the TLS
> >> environment properly. I go to start my domain, but wait it's not using
> > 
> > And after you start the domain there will be "tls='yes'" because the config 
> > XML
> > doesn't contain any *tls* attribute.
> > 
> > I've tested all of those cases before proposing this patch:
> > 
> > prerequisite: prepare certificate files to be used for chardev devices
> > 
> > for running domain:
> > live XML - virsh dumpxml $domain
> > config XML - virsh dumpxml $domain --config
> > migratable XML - virsh dumpxml $domain --migratable
> > 
> > 1. set chardev_tls = 1
> > a) start domain where there is no *tls* attribute in config XML
> > - the domain is started and TLS is properly configured
> > - in the live XML there is "tls='yes'"
> > - in the config XML there is no *tls* attribute
> > - in the migratable XML there is no *tls* attribure
> > 
> > b) start domain where there is "tls='no'" in config XML
> > - the domain is started and TLS is not configured
> > - in the live XML there is "tls='no'"
> > - in the config XML there is "tls='no'"
> > - in the migratable XML there is "tls='no'"
> > 
> > c) start domain where there is "tls='yes'" in config XML
> > - the domain is started and TLS is properly configured
> > - in the live XML there is "tls='yes'"
> > - in the config XML there is "tls='yes'"
> > - in the migratable XML there is "tls='yes'"
> > 
> > 2. set chardev_tls = 0
> > a) start domain where there is no *tls* attribute in config XML
> > - the domain is started and TLS is not configured
> > - in the live XML there is "tls='no'"
> > - in the config XML there is no *tls* attribute
> > - in the migratable XML there is no *tls* attribure
> > 
> > b) start domain where there is "tls='no'" in config XML
> > - the domain is started and TLS is not configured
> > - in the live XML there is "tls='no'"
> > - in the config XML there is "tls='no'"
> > - in the migratable XML there is "tls='no'"
> > 
> > c) start domain where there is "tls='yes'" in config XML
> > - the domain is started and TLS is properly configured
> > - in the live XML there is "tls='yes'"
> > - in the config XML there is "tls='yes'"
> > - in the migratable XML there is "tls='yes'"
> > 
> > Pavel
> > 
> >> it. Closer inspection finds, someone put "tls='no'" into my domain... To
> >> me that's not right.  And I won't necessarily know unless I know to look
> >> at the cmdline of the started domain to find that 'tls-creds' or I in
> >> some way "track" when TLS is being used.
> >>
> >>
> >> John
> >>
> >> --
> >> 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


signature.asc
Description: Digital signature
--

Re: [libvirt] [PATCH v10 2/4] domain: Add optional 'tls' attribute for TCP chardev

[John Ferlan]

On 10/21/2016 02:29 AM, Pavel Hrdina wrote:
> On Thu, Oct 20, 2016 at 03:48:30PM -0400, John Ferlan wrote:
>> [...]
>>

Since I assume you have these changes in your local branch - let's go
with the two patches and move on.

It would be nice while it's still fresh to update the documentation, but
that's a separate patch.

So "officially" it's an ACK of your changes on top of and in addition to
my changes.

John

[...]

> Now I see why we are not able to agree on this change.  The modification done
> in qemuProcessPrepareDomain() are only to live definition, the config
> definition remains untouched, which means the *tls* attribute (if it's based 
> on
> qemu.conf) will appear only in live XML.  The config XML will remain the same
> after the guest is stopped.
> 
>> I think it should be strictly optional and that's where we differ. I see
>> no reason to change the domain xml unless as a consumer that's what you
>> want to do - be able to control which domains will have the setting.
>> What else would be the purpose of a host wide setting to go with a
>> domain optional setting?
>>
>> Finally, if your idea is accepted, that means for any configuration with
>> chardev_tls=0 (either because it's commented or set that way), every
>> domain that starts will be updated to have this new attribute
>> "tls='no'". Then one day, I read up on this wonderful new feature and
> 
> Not the domain, only the live XML which is not saved as config XML ...
> 
>> modify my qemu.conf file to set chardev_tls=1 and set up the TLS
>> environment properly. I go to start my domain, but wait it's not using
> 
> And after you start the domain there will be "tls='yes'" because the config 
> XML
> doesn't contain any *tls* attribute.
> 
> I've tested all of those cases before proposing this patch:
> 
> prerequisite: prepare certificate files to be used for chardev devices
> 
> for running domain:
> live XML - virsh dumpxml $domain
> config XML - virsh dumpxml $domain --config
> migratable XML - virsh dumpxml $domain --migratable
> 
> 1. set chardev_tls = 1
> a) start domain where there is no *tls* attribute in config XML
> - the domain is started and TLS is properly configured
> - in the live XML there is "tls='yes'"
> - in the config XML there is no *tls* attribute
> - in the migratable XML there is no *tls* attribure
> 
> b) start domain where there is "tls='no'" in config XML
> - the domain is started and TLS is not configured
> - in the live XML there is "tls='no'"
> - in the config XML there is "tls='no'"
> - in the migratable XML there is "tls='no'"
> 
> c) start domain where there is "tls='yes'" in config XML
> - the domain is started and TLS is properly configured
> - in the live XML there is "tls='yes'"
> - in the config XML there is "tls='yes'"
> - in the migratable XML there is "tls='yes'"
> 
> 2. set chardev_tls = 0
> a) start domain where there is no *tls* attribute in config XML
> - the domain is started and TLS is not configured
> - in the live XML there is "tls='no'"
> - in the config XML there is no *tls* attribute
> - in the migratable XML there is no *tls* attribure
> 
> b) start domain where there is "tls='no'" in config XML
> - the domain is started and TLS is not configured
> - in the live XML there is "tls='no'"
> - in the config XML there is "tls='no'"
> - in the migratable XML there is "tls='no'"
> 
> c) start domain where there is "tls='yes'" in config XML
> - the domain is started and TLS is properly configured
> - in the live XML there is "tls='yes'"
> - in the config XML there is "tls='yes'"
> - in the migratable XML there is "tls='yes'"
> 
> Pavel
> 
>> it. Closer inspection finds, someone put "tls='no'" into my domain... To
>> me that's not right.  And I won't necessarily know unless I know to look
>> at the cmdline of the started domain to find that 'tls-creds' or I in
>> some way "track" when TLS is being used.
>>
>>
>> John
>>
>> --
>> 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] libxl: fix leaking of allocated migration ports

[Pavel Hrdina]

On Fri, Oct 14, 2016 at 12:19:19PM -0600, Jim Fehlig wrote:
> Although the migration port is immediately released in the
> finish phase of migration, it was never set in the domain
> private object when allocated in the prepare phase. So
> libxlDomainMigrationFinish() always released a 0-initialized
> migrationPort, leaking any allocated port. After enough
> migrations to exhaust the migration port pool, migration would
> fail with
> 
> error: internal error: Unable to find an unused port in range
>'migration' (49152-49216)
> 
> Fix it by setting libxlDomainObjPrivate->migrationPort to the
> port allocated in the prepare phase. While at it, also fix
> leaking an allocated port if the prepare phase fails.
> 
> Signed-off-by: Jim Fehlig 
> ---

ACK

Pavel


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

Re: [libvirt] [RFC] Toward a better NEWS file

[Daniel P. Berrange]

On Fri, Oct 21, 2016 at 12:33:44PM +0200, Sławek Kapłoński wrote:
> Hello,
> 
> I'm quite new in libvirt community so I don't know all requirements
> which community have but did You know about reno project from Openstack:
> http://docs.openstack.org/developer/reno/ ? It's used for managing
> releae notes which are added in git as separate files. Maybe it can be
> some solution for You also :)

reno is totally over-engineered IMHO - we don't need to add another
build pre-requisite just to manage what is ultimately a simple
text file.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|

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

Re: [libvirt] [RFC] Toward a better NEWS file

[Sławek Kapłoński]

Hello,

I'm quite new in libvirt community so I don't know all requirements
which community have but did You know about reno project from Openstack:
http://docs.openstack.org/developer/reno/ ? It's used for managing
releae notes which are added in git as separate files. Maybe it can be
some solution for You also :)

-- 
Best regards / Pozdrawiam
Sławek Kapłoński
sla...@kaplonski.pl

On Fri, 21 Oct 2016, Michal Privoznik wrote:

> On 19.10.2016 19:58, Daniel P. Berrange wrote:
> > On Wed, Oct 19, 2016 at 01:53:41PM +0200, Andrea Bolognani wrote:
> >> Hi,
> >>
> >> 
> > 
> > Why don't we simply have a NEWS file in GIT, and require that
> > non-trivial commits or patch series include an update to NEWS,
> > so the NEWS file gets populated at time the feature/bug fix
> > gets merged.
> 
> I'm up for this one. While introducing a new feature (which touches a
> lot of code anyway) we will just update one file more. I see no problem
> with that. Having to provide news per developer later, trying to recall
> what have I done, might be harder. Therefore NEWS (or news.html.in)
> should be updated among with a major change in the code.
> 
> But let me also provide another reason why this is important (thank you
> Andrea for starting the thread). I spoke to people who test our software
> and sometimes they are just lost with current form of NEWS. It is very
> hard for them to even understand new features in the release so that
> they cannot really update their test cases (or they do so partially). If
> we make NEWS format more readable it will be easier for them to
> implement new tests and subsequently it will increase quality of our
> project.
> 
> Michal
> 
> --
> 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] [REPOST] regarding cgroup v2 support in libvirt

[Daniel P. Berrange]

On Thu, Oct 20, 2016 at 02:59:45PM -0400, Tejun Heo wrote:
> (reposting w/ libvir-list cc'd, sorry about the delay in reposting,
>  was traveling and then on vacation)
> 
> Hello, Daniel.  How have you been?
> 
> We (facebook) are deploying cgroup v2 and internally use libvirt to
> manage virtual machines, so I'm trying to add cgroup v2 support to
> libvirt.
> 
> Because cgroup v2's resource configurations differ from v1 in varying
> degrees depending on the specific resource type, it unfortunately
> introduces new configurations (some completely new configs, others
> just a different range / format).  This means that adding cgroup v2
> support to libvirt requires adding new config options to it and maybe
> implementing some form of translation mechanism between overlapping
> configs.
> 
> The upcoming systemd release includes all that's necessary to support
> v1/v2 compatibility so that users setting resource configs through
> systemd don't have to worry about whether v1 or v2 is in use.  I'm
> wondering whether it would make sense to make libvirt use dbus calls
> to systemd to set resource configs when systemd is in use, so that it
> can piggyback on systemd's v1/v2 compatibility.

The big question I have around cgroup v2 is state of support for all
controllers that libvirt uses (cpu, cpuacct, cpuset, memory, devices,
freezer, blkio).  IIUC, not all of these have been ported to cgroup
v2 setup and the cpu port in particular was rejected by Linux maintainers.
Libvirt has a general policy that we won't support features that only
exist in out of tree patches (applies to kernel and any other software
we build against or use).

IIRC from earlier discussions, the model for dealing with processes in
cgroup v2 was quite different. In libvirt we rely on the ability to
assign different threads within a process to different cgroups, because
we need to control CPU schedular parameters on different threads in
QEMU. eg we have vCPU threads, I/O threads and general emulator threads
each of which get different policies.

When I spoke with Lennart about cgroup v2, way back in Jan, he indicated
that while systemd can technically work with a system where some
controllers are mounted as v1, while others are mounted as v2, this
would not be an officially supported solution. Thus systemd in  Fedora
was not likely to switch to v2 until all required controllers could use
v2. I'm not sure if this still corresponds to Lennarts current views, so
CC'ing him to confirm/deny.

I think from Libvirt POV it would greatly simplify life if we could
likewise restrict ourselves to dealing with hosts which are exclusively
v1 or exclusively v2, and not a mixture. ie we can completely isolate
our codebases for v1 vs v2 management, making it easier to reason about
and test their correctness, reducing QA testing burden.

I recall that systemd policy for v2 was inteded to be that no app
should write to cgroup sysfs except for systemd, unless there was
a sub-tree created with Delegate=yes set on the scope. So this clearly
means when using v2 we'll have to use the systemd DBus APIs for managing
cgroups v2 on such hosts.

> It is true that, as libvirt can be used without systemd, libvirt will
> probably want its own direct implementation down the line, but I think
> there are benefits to going through systemd for resource settings in
> general given that hierarchy setup is already done through systemd
> when available.

While it is certainly nice that the vast majority of OS distros have
switched over to using systemd for init, there's still enough users
out there that I think we'll need to continue to have libvirt support
for using sysfs for v2 on non-systemd hosts.


Any way in summary, we'd like to see v2 support of course, since that
is clearly the future. The big question is what we do about situation
wrt not all controllers being supported in v2 - the lack of complete
conversion is what has stopped me from doing any work in this area
upto now.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|

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

[libvirt] [PATCH v2 3/3] vz: add serial number to disk devices

[Maxim Nestratov]

From: Nikolay Shirokovskiy 

vz sdk supports setting serial number only for disk devices.
Getting serial upon cdrom(for example) is error however
setting is just ignored. Let's check for disk device
explicitly for clarity in both cases.

Setting serial number for other devices is ignored
with an info note just as before.

We need usual conversion from "" to NULL in direction
vz sdk -> libvirt, because "" is not valid for libvirt
and "" means unspecifiend in vz sdk which is NULL for libvirt.

Signed-off-by: Nikolay Shirokovskiy 
Signed-off-by: Maxim Nestratov 
---
 src/vz/vz_sdk.c   | 21 +
 src/vz/vz_utils.c |  6 +++---
 2 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c
index 8178d3b..e14caa5 100644
--- a/src/vz/vz_sdk.c
+++ b/src/vz/vz_sdk.c
@@ -590,6 +590,7 @@ prlsdkGetDiskInfo(vzDriverPtr driver,
   bool isCt)
 {
 char *buf = NULL;
+char *serial = NULL;
 PRL_RESULT pret;
 PRL_UINT32 emulatedType;
 virDomainDeviceDriveAddressPtr address;
@@ -626,6 +627,20 @@ prlsdkGetDiskInfo(vzDriverPtr driver,
 if (*buf != '\0' && virDomainDiskSetSource(disk, buf) < 0)
 goto cleanup;
 
+if (!isCdrom) {
+serial = prlsdkGetStringParamVar(PrlVmDevHd_GetSerialNumber, prldisk);
+if (serial) {
+if (virSafeSerialParamValue(serial) < 0)
+goto cleanup;
+
+if (*serial == '\0')
+VIR_FREE(serial);
+else
+disk->serial = serial;
+serial = NULL;
+}
+}
+
 if (prlsdkGetDiskId(prldisk, >bus, >dst) < 0)
 goto cleanup;
 
@@ -646,6 +661,7 @@ prlsdkGetDiskInfo(vzDriverPtr driver,
 
  cleanup:
 VIR_FREE(buf);
+VIR_FREE(serial);
 return ret;
 }
 
@@ -3495,6 +3511,11 @@ static int prlsdkConfigureDisk(vzDriverPtr driver,
 pret = PrlVmDev_SetStackIndex(sdkdisk, idx);
 prlsdkCheckRetGoto(pret, cleanup);
 
+if (devType == PDE_HARD_DISK) {
+pret = PrlVmDevHd_SetSerialNumber(sdkdisk, disk->serial);
+prlsdkCheckRetGoto(pret, cleanup);
+}
+
 return 0;
  cleanup:
 PrlHandle_Free(sdkdisk);
diff --git a/src/vz/vz_utils.c b/src/vz/vz_utils.c
index eaf09f2..81429d2 100644
--- a/src/vz/vz_utils.c
+++ b/src/vz/vz_utils.c
@@ -323,9 +323,9 @@ vzCheckDiskUnsupportedParams(virDomainDiskDefPtr disk)
 return -1;
 }
 
-if (disk->serial) {
-VIR_INFO("%s", _("Setting disk serial number is not "
- "supported by vz driver."));
+if (disk->serial && disk->device != VIR_DOMAIN_DISK_DEVICE_DISK) {
+VIR_INFO("%s", _("Setting disk serial number is "
+ "supported only for disk devices."));
 }
 
 if (disk->wwn) {
-- 
2.4.11

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

[libvirt] [PATCH v2 1/3] vz: set something in disk driver name

[Maxim Nestratov]

From: Nikolay Shirokovskiy 

Absent driver name attribute is invalid xml. Which in turn makes
unusable 'virsh edit' for example. The value does not make
much sense and ignored on input so nobody will hurt.
---
 src/vz/vz_sdk.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c
index f36064d..8178d3b 100644
--- a/src/vz/vz_sdk.c
+++ b/src/vz/vz_sdk.c
@@ -632,6 +632,9 @@ prlsdkGetDiskInfo(vzDriverPtr driver,
 if (virDiskNameToBusDeviceIndex(disk, , ) < 0)
 goto cleanup;
 
+if (virDomainDiskSetDriver(disk, "vz") < 0)
+goto cleanup;
+
 address = >info.addr.drive;
 address->bus = busIdx;
 address->target = 0;
-- 
2.4.11

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

[libvirt] [PATCH v2 0/3] qemu: util: vz: support setting disk serial number

[Maxim Nestratov]

Actually this is a combination of the two previous series
https://www.redhat.com/archives/libvir-list/2016-October/msg00890.html
and
https://www.redhat.com/archives/libvir-list/2016-September/msg01024.html

Maxim Nestratov (1):
  util: qemu: make qemuSafeSerialParamValue function usable by other
drivers

Nikolay Shirokovskiy (2):
  vz: set something in disk driver name
  vz: add serial number to disk devices

 src/libvirt_private.syms |  1 +
 src/qemu/qemu_command.c  | 19 +--
 src/util/virutil.c   | 16 
 src/util/virutil.h   |  2 ++
 src/vz/vz_sdk.c  | 24 
 src/vz/vz_utils.c|  6 +++---
 6 files changed, 47 insertions(+), 21 deletions(-)

-- 
2.4.11

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

[libvirt] [PATCH v2 2/3] util: qemu: make qemuSafeSerialParamValue function usable by other drivers

[Maxim Nestratov]

Rename qemuSafeSerialParamValue to virSafeSerialParamValue and move it to utils

Signed-off-by: Maxim Nestratov 
---
 src/libvirt_private.syms |  1 +
 src/qemu/qemu_command.c  | 19 +--
 src/util/virutil.c   | 16 
 src/util/virutil.h   |  2 ++
 4 files changed, 20 insertions(+), 18 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index bf503a5..170195b 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2649,6 +2649,7 @@ virParseVersionString;
 virPipeReadUntilEOF;
 virReadFCHost;
 virReadSCSIUniqueId;
+virSafeSerialParamValue;
 virScaleInteger;
 virSetBlocking;
 virSetCloseExec;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 8282162..0be2ffa 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -427,23 +427,6 @@ qemuBuildIoEventFdStr(virBufferPtr buf,
 return 0;
 }
 
-#define QEMU_SERIAL_PARAM_ACCEPTED_CHARS \
-  "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-_ "
-
-static int
-qemuSafeSerialParamValue(const char *value)
-{
-if (strspn(value, QEMU_SERIAL_PARAM_ACCEPTED_CHARS) != strlen(value)) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("driver serial '%s' contains unsafe characters"),
-   value);
-return -1;
-}
-
-return 0;
-}
-
-
 static int
 qemuNetworkDriveGetPort(int protocol,
 const char *port)
@@ -1600,7 +1583,7 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk,
 
 if (disk->serial &&
 virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_SERIAL)) {
-if (qemuSafeSerialParamValue(disk->serial) < 0)
+if (virSafeSerialParamValue(disk->serial) < 0)
 goto error;
 if (disk->bus == VIR_DOMAIN_DISK_BUS_SCSI &&
 disk->device == VIR_DOMAIN_DISK_DEVICE_LUN) {
diff --git a/src/util/virutil.c b/src/util/virutil.c
index 844c947..58ace3f 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -2684,3 +2684,19 @@ virMemoryMaxValue(bool capped)
 else
 return LLONG_MAX;
 }
+
+#define VIR_SERIAL_PARAM_ACCEPTED_CHARS \
+  "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-_ "
+
+int
+virSafeSerialParamValue(const char *value)
+{
+if (strspn(value, VIR_SERIAL_PARAM_ACCEPTED_CHARS) != strlen(value)) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("driver serial '%s' contains unsafe characters"),
+   value);
+return -1;
+}
+
+return 0;
+}
diff --git a/src/util/virutil.h b/src/util/virutil.h
index 8c0d83c..f9b4831 100644
--- a/src/util/virutil.h
+++ b/src/util/virutil.h
@@ -252,6 +252,8 @@ unsigned long long virMemoryLimitTruncate(unsigned long 
long value);
 bool virMemoryLimitIsSet(unsigned long long value);
 unsigned long long virMemoryMaxValue(bool ulong);
 
+int virSafeSerialParamValue(const char *value);
+
 /**
  * VIR_ASSIGN_IS_OVERFLOW:
  * @rvalue: value that is checked (evaluated twice)
-- 
2.4.11

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

Re: [libvirt] [RFC] Toward a better NEWS file

[Daniel P. Berrange]

On Fri, Oct 21, 2016 at 10:10:50AM +0800, Michal Privoznik wrote:
> On 19.10.2016 19:58, Daniel P. Berrange wrote:
> > On Wed, Oct 19, 2016 at 01:53:41PM +0200, Andrea Bolognani wrote:
> >> Hi,
> >>
> >> 
> > 
> > Why don't we simply have a NEWS file in GIT, and require that
> > non-trivial commits or patch series include an update to NEWS,
> > so the NEWS file gets populated at time the feature/bug fix
> > gets merged.
> 
> I'm up for this one. While introducing a new feature (which touches a
> lot of code anyway) we will just update one file more. I see no problem
> with that. Having to provide news per developer later, trying to recall
> what have I done, might be harder. Therefore NEWS (or news.html.in)
> should be updated among with a major change in the code.
> 
> But let me also provide another reason why this is important (thank you
> Andrea for starting the thread). I spoke to people who test our software
> and sometimes they are just lost with current form of NEWS. It is very
> hard for them to even understand new features in the release so that
> they cannot really update their test cases (or they do so partially). If
> we make NEWS format more readable it will be easier for them to
> implement new tests and subsequently it will increase quality of our
> project.

Take a look at how we structure NEWS file for gtk-vnc:

  https://git.gnome.org/browse/gtk-vnc/tree/NEWS

We try to give users concise information, ordered by importance

That is the kind of format I think we should aim for.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|

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

Re: [libvirt] [PATCH] vz: set localhost as vnc address

[Maxim Nestratov]

19-Oct-16 12:40, Nikolay Shirokovskiy пишет:



On 18.10.2016 19:19, Mikhail Feoktistov wrote:

We should set localhost as vnc address in case of empty string.
Because Virtuozzo sets 0.0.0.0 as default vnc address.
---
  src/vz/vz_sdk.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c
index f2a5c96..7235172 100644
--- a/src/vz/vz_sdk.c
+++ b/src/vz/vz_sdk.c
@@ -2967,7 +2967,7 @@ static int prlsdkApplyGraphicsParams(PRL_HANDLE sdkdom,
  
  glisten = virDomainGraphicsGetListen(gr, 0);

  pret = PrlVmCfg_SetVNCHostName(sdkdom, glisten && glisten->address ?
-   glisten->address : "");
+   glisten->address : "127.0.0.1");
  prlsdkCheckRetGoto(pret, cleanup);
  
  ret = 0;



ACK


Pushed now. Thank you.

Maxim

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

Re: [libvirt] [RFC] Toward a better NEWS file

[Andrea Bolognani]

On Wed, 2016-10-19 at 18:22 -0400, John Ferlan wrote:
> I really think we should do something - especially to be able to
> describe what's new, what was added when, etc. beyond what DV culls out
> of the existing commits.
> 
> Whether the mechanism is some news.html.in or features.html.in or
> something else is fine. I don't think we wait until the end of the
> release to update. Manually going through patches and matching up to
> cover letters by one person for some releases will be easy, while for
> other releases it will be an arduous task. That becomes a bottleneck on
> one person and could be a time consuming task.

I think all those taking part in the discussion now agree
that it would be better to simply make updating news.html.in
part of the code submission process. That way the entry is
going to be reviewed along with the code, and the work is
spread much more evenly during the release cycle.

Some minor editorial work would probably still be necessary,
or at least welcome, during the freeze to massage the end
result, but the worst case scenario becomes that we ship a
NEWS entry that's not quite as polished as the previous ones.
I think we can live with that :)

I'm confident that, while at first the output might be
sub-par, over the course of a few release we will develop
a "feel" for what category of changes are NEWS-worthy, what
kind of wording is better used to describe a new feature, and
so on... As a result, the quality will improve substantially.

> I was thinking after KVM Forum it would be nice if we had some way to
> make it more "obvious" when new features were added and when along with
> git commit id link and/or a link to the archives discussing the change.
> Makes it easier to find when something was added and get a sense for any
> discussion. Something that could be linked off the front page somehow
> and updated as the new features are added. Something PROMINENT that
> people won't MISS. I wasn't sure about the best way to do that and well
> didn't want to take on the task of going through history either. Say
> nothing about the task of chasing, editing, etc. possible responses to a
> call for NEWS. A task which thankfully you will take on - that's free
> beer at every KVM Forum for you from those that don't want to be "it"!
> 
> Adding in non trivial bug fixes is nice, especially when we could link
> them back to a feature that was added so that someone downstream or up
> the stack could use that information to help determine why something
> isn't working the way they thought it should be. Using entries that
> provide bugzilla pointers would be good too. Still what criteria do we
> use for trivial-ness?

What you're describing, while definitely an interesting
concept, is probably out of scope for the NEWS file due to
the sheer amount of metadata involved.

At the end of the day, the source of truth for all changes
to libvirt (and any other project) is going to be the git
log, and the NEWS file shouldn't aim to replace that; instead,
it should provide a high-level and non-comprehensive summary
of the changes made during a release cycle.

Think of it this way: the NEWS file should be an interesting
read for basically 100% of people who install libvirt. Those
who are looking for very specific information, such as a
potentially obscure bug being finally fixed, are probably
best served by the full git log anyway.

One last thing: my proposal only applies to changes made
*going forward*. Nobody's going through 10+ years of libvirt
history and come up with NEWS entries for them ;)

> Reviewers will have to remember to ping/remind on
> needing to describe/summarize things (that could be in cover letters
> found in the archive) if we decide to take the path of having each
> commit provide the news update.
> 
> Having everyone update the news as changes are being made is another
> possibility, but when things get busy (especially at release end) -
> dealing with the merge conflicts of the news file will always be a pain.
> Plus what about those with push capabilities that don't update (or
> refuse to) the news...

Merge conflicts might end up being annoying, but at least
resolving them is going to be trivial.

Catching changes that are NEWS-worthy but don't update
news.html.in is a job for the reviewer, and we're just going
to get used to it in due time :)

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH 1/2] vz: add serial number to disk devices

[Maxim Nestratov]

22-Sep-16 17:55, Nikolay Shirokovskiy пишет:


vz sdk supports setting serial number only for disk devices.
Getting serial upon cdrom(for example) is error however
setting is just ignored. Let's check for disk device
explicitly for clarity in both cases.

Setting serial number for other devices is ignored
with an info note just as before.

We need usual conversion from "" to NULL in direction
vz sdk -> libvirt, because "" is not valid for libvirt
and "" means unspecifiend in vz sdk which is NULL for libvirt.
---
  src/vz/vz_sdk.c   | 13 +
  src/vz/vz_utils.c |  6 +++---
  2 files changed, 16 insertions(+), 3 deletions(-)


Hmm. I missed this series and sent mine on the matter yesterday. I see that 
your patch has some
useful changes regarding CDROM absent in my patch. So I'll merge these two and 
resend a
new version shortly.

Maxim


diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c
index f2a5c96..a38f2af 100644
--- a/src/vz/vz_sdk.c
+++ b/src/vz/vz_sdk.c
@@ -639,6 +639,14 @@ prlsdkGetDiskInfo(vzDriverPtr driver,
  
  disk->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE;
  
+if (!isCdrom) {

+if (!(disk->serial = 
prlsdkGetStringParamVar(PrlVmDevHd_GetSerialNumber, prldisk)))
+goto cleanup;
+
+if (*disk->serial == '\0')
+VIR_FREE(disk->serial);
+}
+
  ret = 0;
  
   cleanup:

@@ -3492,6 +3500,11 @@ static int prlsdkConfigureDisk(vzDriverPtr driver,
  pret = PrlVmDev_SetStackIndex(sdkdisk, idx);
  prlsdkCheckRetGoto(pret, cleanup);
  
+if (devType == PDE_HARD_DISK) {

+pret = PrlVmDevHd_SetSerialNumber(sdkdisk, disk->serial);
+prlsdkCheckRetGoto(pret, cleanup);
+}
+
  return 0;
   cleanup:
  PrlHandle_Free(sdkdisk);
diff --git a/src/vz/vz_utils.c b/src/vz/vz_utils.c
index eaf09f2..81429d2 100644
--- a/src/vz/vz_utils.c
+++ b/src/vz/vz_utils.c
@@ -323,9 +323,9 @@ vzCheckDiskUnsupportedParams(virDomainDiskDefPtr disk)
  return -1;
  }
  
-if (disk->serial) {

-VIR_INFO("%s", _("Setting disk serial number is not "
- "supported by vz driver."));
+if (disk->serial && disk->device != VIR_DOMAIN_DISK_DEVICE_DISK) {
+VIR_INFO("%s", _("Setting disk serial number is "
+ "supported only for disk devices."));
  }
  
  if (disk->wwn) {


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

Re: [libvirt] [PATCH 2/2] vz: set something in disk driver name

[Maxim Nestratov]

22-Sep-16 17:55, Nikolay Shirokovskiy пишет:


Absent driver name attribute is invalid xml. Which in turn makes
unusable 'virsh edit' for example. The value does not make
much sense and ignored on input so nobody will hurt.
---
  src/vz/vz_sdk.c | 3 +++
  1 file changed, 3 insertions(+)

diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c
index a38f2af..0af450b 100644
--- a/src/vz/vz_sdk.c
+++ b/src/vz/vz_sdk.c
@@ -647,6 +647,9 @@ prlsdkGetDiskInfo(vzDriverPtr driver,
  VIR_FREE(disk->serial);
  }
  
+if (virDomainDiskSetDriver(disk, "vz") < 0)

+goto cleanup;
+
  ret = 0;
  
   cleanup:


ACK

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

Re: [libvirt] [PATCH] virt-wireshark.m4: Defer $(prefix) substitution

[Andrea Bolognani]

On Fri, 2016-10-21 at 10:47 +0800, Michal Privoznik wrote:
> > If the plugindir for wireshark is eg.
> > /usr/lib64/wireshark/plugins, should we still install under
> > an arbitrary $(prefix)? Will wireshark pick up the plugin
> > then?
> 
> Well, depends how you installed wireshark. If you provided the same
> prefix during its installation, the plugin is loaded. If you didn't,
> then it isn't loaded. But that's what you want, isn't it?

Okay, you convinced me :)

Thanks for looking into this issue by the way!

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH v10 2/4] domain: Add optional 'tls' attribute for TCP chardev

[Pavel Hrdina]

On Thu, Oct 20, 2016 at 03:48:30PM -0400, John Ferlan wrote:
> [...]
> 
>  +
>  +  Since 2.4.0, the optional attribute
>  +  tls can be used to control whether a serial chardev
>  +  TCP communication channel would utilize a hypervisor configured
>  +  TLS X.509 certificate environment in order to encrypt the data
>  +  channel. For the QEMU hypervisor, usage of a TLS envronment can
>  +  be controlled on the host by the chardev_tls and
>  +  chardev_tls_x509_cert_dir or
>  +  default_tls_x509_cert_dir settings in the file
>  +  /etc/libvirt/qemu.conf. If chardev_tls is enabled,
>  +  then unless the tls attribute is set to "no", libvirt
>  +  will use the host configured TLS environment.
>  +  If chardev_tls is disabled, but the tls
>  +  attribute is set to "yes", then libvirt will attempt to use the
>  +  host TLS environment if either the 
>  chardev_tls_x509_cert_dir
>  +  or default_tls_x509_cert_dir TLS directory structure 
>  exists.
>  +
> >>>
> >>> Nice, this is a good description how to use the *tls* attribute.
> >>>
> >>
> >> BTW (regarding your followup reply):
> >>
> >> The 4 "consumers" of virDomainChrSourceDefParseXML (where this would be
> >> parsed) refer to this as a "serial chardev"
> > 
> > This is a generic function that parses source for a lot of different device
> > types/
> > 
> 
> Shortcutting in my mind to  service='%s'/> which is the VIR_DOMAIN_DEVICE_CHR, smartcard, rng, and
> redirdev. And yes, VIR_DOMAIN_DEVICE_CHR has paths for parallels,
> serials, consoles, and channels that are defined using <%s type='tcp'>.
> 
> >> The 'virDomainChrDefParseXML' comments have a list of " >> types. The location of the above description is describing a  >> type="tcp"> definition.
> > 
> > Well, the comment have that list but is used to parse all character devices,
> > not only serial char device.  TLS encryption can be used also for those 
> > types:
> > parallel, channel, and console.
> > 
> 
> My continual battle against under documentation.  The code is not self
> documenting in all cases...

I agree, we should be better in documenting things.

> >> The 'smartcard' discussion for a 'passthrough' device that would use
> >> this code says "Rather than having the hypervisor directly communicate
> >> with the host, it is possible to tunnel all requests through a secondary
> >> character device to a third-party provider (which may in turn be talking
> >> to a smartcard or using three certificate files). In this mode of
> >> operation, an additional attribute type is required, matching one of the
> >> supported serial device types, to describe the host side of the tunnel;..."
> > 
> > This comment is wrong, it should be "supported character device types".  
> > This
> > attribute tells what interface is presented to the host.  Check this part of
> > documentation for all character devices:
> > 
> >   
> > 
> > there is this sentence:
> > 
> >   "The interface presented to the host is given in the type attribute of the
> >   top-level element. The host interface is configured by the source 
> > element."
> > 
> > So it refers to all host interfaces:
> > 
> >   
> > 
> >> The 'rng' discussion for backend that would use this code says "This
> >> backend connects to a source using the EGD protocol. The source is
> >> specified as a character device. Refer to character device host
> >> interface for more information. ..."
> > 
> > This is correct, there is no reference to serial character device.
> > 
> >> Redevdir says "An additional attribute type is required, matching one of
> >> the supported serial device types, to describe the host side of the
> >> tunnel; type='tcp' or type='spicevmc' ..."
> > 
> > This is the same case as smartcard device, this is wrong.
> > 
> >> So the long and short of it is, IMO it's a serial chardev device.
> >> Semantically it could be claimed otherwise, but the parsing proves
> >> otherwise as does the existing documentation of "Host interface"
> >> character devices.
> >>
> >> I prefer to keep it described as is. It's only ever used, parsed, etc.
> >> when ... ...  >>
> >> If anything, the description should become more restrictive to indicate
> >> that the option shouldn't be used for smartcards, rngs, and redirdevs,
> >> but I'll save that discussion for patch 3.
> > 
> > Based on the documentation it may appear that it should be a serial chardev,
> > but that's misleading and it should refer only to the "host interfaces of
> > character devices".
> > 
> 
> I cannot begin to describe how many times I've scrolled up and down
> through that discussion and thought how does anyone get this stuff
> correct... Trial and error I suppose.
> 
> In any case, it seems of the 3 the rng is the most correct and the