[libvirt] [PATCHv2 2/3] vmx: convert to typesafe virConf accessors

[Fabiano Fidêncio]

From: Fabiano Fidêncio 

Signed-off-by: Fabiano Fidêncio 
---
 src/vmx/vmx.c | 196 ++
 1 file changed, 73 insertions(+), 123 deletions(-)

diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
index df6a58a474..54542c29a6 100644
--- a/src/vmx/vmx.c
+++ b/src/vmx/vmx.c
@@ -720,41 +720,36 @@ virVMXConvertToUTF8(const char *encoding, const char 
*string)
 }
 
 
-
 static int
-virVMXGetConfigString(virConfPtr conf, const char *name, char **string,
-  bool optional)
+virVMXGetConfigStringHelper(virConfPtr conf, const char *name, char **string,
+bool optional)
 {
-virConfValuePtr value;
-
+int result;
 *string = NULL;
-value = virConfGetValue(conf, name);
 
-if (value == NULL) {
-if (optional)
-return 0;
+result = virConfGetValueString(conf, name, string);
+if (result == 1 && *string != NULL)
+return 1;
 
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("Missing essential config entry '%s'"), name);
-return -1;
-}
+if (optional)
+return 0;
 
-if (value->type != VIR_CONF_STRING) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("Config entry '%s' must be a string"), name);
-return -1;
-}
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Missing essential config entry '%s'"), name);
+return -1;
+}
 
-if (value->str == NULL) {
-if (optional)
-return 0;
 
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("Missing essential config entry '%s'"), name);
+static int
+virVMXGetConfigString(virConfPtr conf, const char *name, char **string,
+  bool optional)
+{
+*string = NULL;
+
+if (virVMXGetConfigStringHelper(conf, name, string, optional) < 0)
 return -1;
-}
 
-return VIR_STRDUP(*string, value->str);
+return 0;
 }
 
 
@@ -763,43 +758,26 @@ static int
 virVMXGetConfigUUID(virConfPtr conf, const char *name, unsigned char *uuid,
 bool optional)
 {
-virConfValuePtr value;
+char *string = NULL;
+int result;
 
-value = virConfGetValue(conf, name);
+result = virVMXGetConfigStringHelper(conf, name, , optional);
+if (result <= 0)
+return result;
 
-if (value == NULL) {
-if (optional) {
-return 0;
-} else {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("Missing essential config entry '%s'"), name);
-return -1;
-}
-}
-
-if (value->type != VIR_CONF_STRING) {
+if (virUUIDParse(string, uuid) < 0) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("Config entry '%s' must be a string"), name);
-return -1;
+   _("Could not parse UUID from string '%s'"), string);
+result = -1;
+goto cleanup;
 }
 
-if (value->str == NULL) {
-if (optional) {
-return 0;
-} else {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("Missing essential config entry '%s'"), name);
-return -1;
-}
-}
+result = 0;
 
-if (virUUIDParse(value->str, uuid) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("Could not parse UUID from string '%s'"), value->str);
-return -1;
-}
+  cleanup:
+VIR_FREE(string);
 
-return 0;
+return result;
 }
 
 
@@ -808,47 +786,35 @@ static int
 virVMXGetConfigLong(virConfPtr conf, const char *name, long long *number,
 long long default_, bool optional)
 {
-virConfValuePtr value;
+char *string = NULL;
+int result;
 
 *number = default_;
-value = virConfGetValue(conf, name);
 
-if (value == NULL) {
-if (optional) {
-return 0;
-} else {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("Missing essential config entry '%s'"), name);
-return -1;
-}
-}
+result = virVMXGetConfigStringHelper(conf, name, , optional);
+if (result <= 0)
+return result;
 
-if (value->type == VIR_CONF_STRING) {
-if (value->str == NULL) {
-if (optional) {
-return 0;
-} else {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("Missing essential config entry '%s'"), name);
-return -1;
-}
-}
+if (STRCASEEQ(string, "unlimited")) {
+*number = -1;
+result = 0;
+goto cleanup;
+}
 
-if (STRCASEEQ(value->str, "unlimited")) {
-*number = -1;
-} else if (virStrToLong_ll(value->str, NULL, 10, number) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("Config 

[libvirt] [PATCHv2 3/3] xen_common: convert to typesafe virConf accessors

[Fabiano Fidêncio]

From: Fabiano Fidêncio 

There are still some places using virConfGetValue() and then checking
the specific type of the pointers and so on.

Those place are not going to be changed as:
- Directly using virConfGetValue*() would trigger virReportError() on
their current code
- Expanding virConfGetValue*() to support strings as other types (as
boolean or long) doesn't seem to be the safest path to take.

Signed-off-by: Fabiano Fidêncio 
---
 src/xenconfig/xen_common.c | 637 ++---
 1 file changed, 307 insertions(+), 330 deletions(-)

diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c
index a2b0708ee3..2e8e95f720 100644
--- a/src/xenconfig/xen_common.c
+++ b/src/xenconfig/xen_common.c
@@ -145,31 +145,18 @@ xenConfigCopyStringInternal(virConfPtr conf,
 char **value,
 int allowMissing)
 {
-virConfValuePtr val;
+int result;
 
 *value = NULL;
-if (!(val = virConfGetValue(conf, name))) {
-if (allowMissing)
-return 0;
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("config value %s was missing"), name);
-return -1;
-}
 
-if (val->type != VIR_CONF_STRING) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("config value %s was not a string"), name);
-return -1;
-}
-if (!val->str) {
-if (allowMissing)
-return 0;
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("config value %s was missing"), name);
-return -1;
-}
+result = virConfGetValueString(conf, name, value);
+if (result == 1 && *value != NULL)
+return 0;
+
+if (allowMissing)
+return 0;
 
-return VIR_STRDUP(*value, val->str);
+return -1;
 }
 
 
@@ -193,7 +180,8 @@ xenConfigCopyStringOpt(virConfPtr conf, const char *name, 
char **value)
 static int
 xenConfigGetUUID(virConfPtr conf, const char *name, unsigned char *uuid)
 {
-virConfValuePtr val;
+char *string = NULL;
+int result;
 
 if (!uuid || !name || !conf) {
 virReportError(VIR_ERR_INVALID_ARG, "%s",
@@ -201,35 +189,35 @@ xenConfigGetUUID(virConfPtr conf, const char *name, 
unsigned char *uuid)
 return -1;
 }
 
-if (!(val = virConfGetValue(conf, name))) {
-if (virUUIDGenerate(uuid) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   "%s", _("Failed to generate UUID"));
-return -1;
-} else {
-return 0;
-}
-}
-
-if (val->type != VIR_CONF_STRING) {
-virReportError(VIR_ERR_CONF_SYNTAX,
-   _("config value %s not a string"), name);
+if (virConfGetValueString(conf, name, ) < 0)
 return -1;
-}
 
-if (!val->str) {
+if (!string) {
 virReportError(VIR_ERR_CONF_SYNTAX,
_("%s can't be empty"), name);
 return -1;
 }
 
-if (virUUIDParse(val->str, uuid) < 0) {
+if (virUUIDGenerate(uuid) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   "%s", _("Failed to generate UUID"));
+result = -1;
+goto cleanup;
+}
+
+if (virUUIDParse(string, uuid) < 0) {
 virReportError(VIR_ERR_CONF_SYNTAX,
-   _("%s not parseable"), val->str);
-return -1;
+   _("%s not parseable"), string);
+result = -1;
+goto cleanup;
 }
 
-return 0;
+result = 1;
+
+ cleanup:
+VIR_FREE(string);
+
+return result;
 }
 
 
@@ -242,23 +230,15 @@ xenConfigGetString(virConfPtr conf,
const char **value,
const char *def)
 {
-virConfValuePtr val;
+char *string = NULL;
 
-*value = NULL;
-if (!(val = virConfGetValue(conf, name))) {
+if (virConfGetValueString(conf, name, ) < 0) {
 *value = def;
-return 0;
-}
-
-if (val->type != VIR_CONF_STRING) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("config value %s was malformed"), name);
 return -1;
 }
-if (!val->str)
-*value = def;
-else
-*value = val->str;
+
+*value = string != NULL ? string : def;
+
 return 0;
 }
 
@@ -392,93 +372,92 @@ xenParseEventsActions(virConfPtr conf, virDomainDefPtr 
def)
 static int
 xenParsePCI(virConfPtr conf, virDomainDefPtr def)
 {
-virConfValuePtr list = virConfGetValue(conf, "pci");
 virDomainHostdevDefPtr hostdev = NULL;
+char **pcis = NULL, **entries;
 
-if (list && list->type == VIR_CONF_LIST) {
-list = list->list;
-while (list) {
-char domain[5];
-char bus[3];
-char slot[3];
-char func[2];
-char *key, *nextkey;
-int domainID;
-int busID;
-int slotID;
-int funcID;
-
-   

[libvirt] [PATCHv2 0/3] Finish the conversion to virConfGetValue* functions

[Fabiano Fidêncio]

This patchset finishes the conversion to virConfGetValue* functions,
started by Daniel Berrange a few months ago.

Please, mind that although we could make virConfGetValue* functions more
generic in order to support numbers and booleans as strings, that
doesn't seem the safest path to take. The side-effect of this is that we
will have to live with some specific code doing that as part of vmx and
xen_common.

Once this patchset gets merged,
https://wiki.libvirt.org/page/BiteSizedTasks#Finish_conversion_to_virConfGetValue.2A_functions
can be removed.

- Changes since v1:
  All the "values" from virConfGetValueString() are freed

Fabiano Fidêncio (3):
  xen_vm: convert to typesafe virConf accessors
  vmx: convert to typesafe virConf accessors
  xen_common: convert to typesafe virConf accessors

 src/vmx/vmx.c  | 196 ++
 src/xenconfig/xen_common.c | 637 ++---
 src/xenconfig/xen_xm.c | 268 ++-
 3 files changed, 512 insertions(+), 589 deletions(-)

-- 
2.14.3

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

[libvirt] [PATCHv2 1/3] xen_vm: convert to typesafe virConf accessors

[Fabiano Fidêncio]

From: Fabiano Fidêncio 

Signed-off-by: Fabiano Fidêncio 
---
 src/xenconfig/xen_xm.c | 268 -
 1 file changed, 132 insertions(+), 136 deletions(-)

diff --git a/src/xenconfig/xen_xm.c b/src/xenconfig/xen_xm.c
index 4becb40b4c..fc88ac8238 100644
--- a/src/xenconfig/xen_xm.c
+++ b/src/xenconfig/xen_xm.c
@@ -112,172 +112,168 @@ xenParseXMDisk(virConfPtr conf, virDomainDefPtr def)
 {
 virDomainDiskDefPtr disk = NULL;
 int hvm = def->os.type == VIR_DOMAIN_OSTYPE_HVM;
-virConfValuePtr list = virConfGetValue(conf, "disk");
+char **disks = NULL, **entries;
 
-if (list && list->type == VIR_CONF_LIST) {
-list = list->list;
-while (list) {
-char *head;
-char *offset;
-char *tmp;
-const char *src;
+if (virConfGetValueStringList(conf, "disk", false, ) < 0)
+goto cleanup;
 
-if ((list->type != VIR_CONF_STRING) || (list->str == NULL))
-goto skipdisk;
+for (entries = disks; *entries; entries++) {
+char *head = *entries;
+char *offset;
+char *tmp;
+const char *src;
 
-head = list->str;
-if (!(disk = virDomainDiskDefNew(NULL)))
-return -1;
+if (!(disk = virDomainDiskDefNew(NULL)))
+return -1;
+
+/*
+ * Disks have 3 components, SOURCE,DEST-DEVICE,MODE
+ * eg, phy:/dev/HostVG/XenGuest1,xvda,w
+ * The SOURCE is usually prefixed with a driver type,
+ * and optionally driver sub-type
+ * The DEST-DEVICE is optionally post-fixed with disk type
+ */
+
+/* Extract the source file path*/
+if (!(offset = strchr(head, ',')))
+goto skipdisk;
+
+if (offset == head) {
+/* No source file given, eg CDROM with no media */
+ignore_value(virDomainDiskSetSource(disk, NULL));
+} else {
+if (VIR_STRNDUP(tmp, head, offset - head) < 0)
+goto cleanup;
+
+if (virDomainDiskSetSource(disk, tmp) < 0) {
+VIR_FREE(tmp);
+goto cleanup;
+}
+VIR_FREE(tmp);
+}
 
-/*
- * Disks have 3 components, SOURCE,DEST-DEVICE,MODE
- * eg, phy:/dev/HostVG/XenGuest1,xvda,w
- * The SOURCE is usually prefixed with a driver type,
- * and optionally driver sub-type
- * The DEST-DEVICE is optionally post-fixed with disk type
- */
-
-/* Extract the source file path*/
-if (!(offset = strchr(head, ',')))
-goto skipdisk;
-
-if (offset == head) {
-/* No source file given, eg CDROM with no media */
-ignore_value(virDomainDiskSetSource(disk, NULL));
-} else {
-if (VIR_STRNDUP(tmp, head, offset - head) < 0)
+head = offset + 1;
+/* Remove legacy ioemu: junk */
+if (STRPREFIX(head, "ioemu:"))
+head = head + 6;
+
+/* Extract the dest device name */
+if (!(offset = strchr(head, ',')))
+goto skipdisk;
+
+if (VIR_ALLOC_N(disk->dst, (offset - head) + 1) < 0)
+goto cleanup;
+
+if (virStrncpy(disk->dst, head, offset - head,
+   (offset - head) + 1) == NULL) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Dest file %s too big for destination"), head);
+goto cleanup;
+}
+
+head = offset + 1;
+/* Extract source driver type */
+src = virDomainDiskGetSource(disk);
+if (src) {
+size_t len;
+/* The main type  phy:, file:, tap: ... */
+if ((tmp = strchr(src, ':')) != NULL) {
+len = tmp - src;
+if (VIR_STRNDUP(tmp, src, len) < 0)
 goto cleanup;
 
-if (virDomainDiskSetSource(disk, tmp) < 0) {
+if (virDomainDiskSetDriver(disk, tmp) < 0) {
 VIR_FREE(tmp);
 goto cleanup;
 }
 VIR_FREE(tmp);
-}
-
-head = offset + 1;
-/* Remove legacy ioemu: junk */
-if (STRPREFIX(head, "ioemu:"))
-head = head + 6;
-
-/* Extract the dest device name */
-if (!(offset = strchr(head, ',')))
-goto skipdisk;
 
-if (VIR_ALLOC_N(disk->dst, (offset - head) + 1) < 0)
-goto cleanup;
+/* Strip the prefix we found off the source file name */
+if (virDomainDiskSetSource(disk, src + len + 1) < 0)
+goto cleanup;
 
-if (virStrncpy(disk->dst, head, offset - head,
-   (offset - head) + 1) == NULL) {
-

Re: [libvirt] [PATCH v2 12/12] news: Document new API introduction

[Ján Tomko]

On Thu, May 24, 2018 at 01:13:39PM +0200, Michal Privoznik wrote:

Signed-off-by: Michal Privoznik 
---
docs/news.xml | 8 
1 file changed, 8 insertions(+)



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH v2 11/12] qemu: Implement virDomainDetachDeviceAlias

[Ján Tomko]

On Thu, May 24, 2018 at 01:13:38PM +0200, Michal Privoznik wrote:

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

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 81a9833b39..6f0a3d0cda 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -8750,6 +8750,77 @@ qemuDomainDetachDeviceLiveAndConfig(virQEMUDriverPtr 
driver,
return ret;
}

+
+static int
+qemuDomainDetachDeviceAliasLiveAndConfig(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ const char *alias,
+ unsigned int flags)
+{
+virCapsPtr caps = NULL;
+virQEMUDriverConfigPtr cfg = NULL;
+virDomainDefPtr def = NULL;
+virDomainDefPtr persistentDef = NULL;
+virDomainDefPtr vmdef = NULL;
+unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE;


Why on earth would we need parse_flags to detach a device?


+int ret = -1;
+
+virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
+  VIR_DOMAIN_AFFECT_CONFIG, -1);
+
+if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
+goto cleanup;
+
+cfg = virQEMUDriverGetConfig(driver);
+
+if ((flags & VIR_DOMAIN_AFFECT_CONFIG) &&
+!(flags & VIR_DOMAIN_AFFECT_LIVE))
+parse_flags |= VIR_DOMAIN_DEF_PARSE_INACTIVE;
+
+if (virDomainObjGetDefs(vm, flags, , ) < 0)
+goto cleanup;
+
+if (persistentDef) {
+virDomainDeviceDef dev;
+
+vmdef = virDomainObjCopyPersistentDef(vm, caps, driver->xmlopt);
+if (!vmdef)
+goto cleanup;
+
+if (virDomainDefFindDevice(persistentDef, alias, , true) < 0)
+goto cleanup;
+
+if (qemuDomainDetachDeviceConfig(persistentDef, , caps,
+ parse_flags, driver->xmlopt) < 0)


Oh, somebody thought it would be a good idea to call postParse even
though no parsing has happened

Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH v2 10/12] qemu_hotplug: Allow asynchronous detach

[Ján Tomko]

On Thu, May 24, 2018 at 01:13:37PM +0200, Michal Privoznik wrote:

The virDomainDetachDeviceAlias API is designed so that it only
sends detach request to qemu. It's user's responsibility to wait


s/user/the user/


for DEVICE_DELETED event, not libvirt's. Add @async flag to
qemuDomainDetach*Device() functions so that caller can chose if
detach is semi-synchronous (old virDomainDetachDeviceFlags()) or
fully asynchronous (new virDomainDetachDeviceFlags()).

Signed-off-by: Michal Privoznik 
---
src/qemu/qemu_driver.c  |  32 +++
src/qemu/qemu_hotplug.c | 231 
src/qemu/qemu_hotplug.h |  33 ---
tests/qemuhotplugtest.c |  13 +--
4 files changed, 203 insertions(+), 106 deletions(-)

@@ -4769,18 +4771,24 @@ qemuDomainDetachVirtioDiskDevice(virQEMUDriverPtr 
driver,
if (qemuDomainObjExitMonitor(driver, vm) < 0)
goto cleanup;

-if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1)
-ret = qemuDomainRemoveDiskDevice(driver, vm, detach);
+if (async) {
+ret = 0;
+} else {
+if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1)
+ret = qemuDomainRemoveDiskDevice(driver, vm, detach);
+}


I'm sure the ret assignments could be reduced here, but that's out of
scope of this series...

Also, most of the code is executed if (!async). It might be nicer to
come up with a variable that can be used in a positive condition,
but it would be less precise, because only the new async API is
reasonably defined - even if (wait) does not seem to outweigh
the loss of clarity.



 cleanup:
-qemuDomainResetDeviceRemoval(vm);
+if (!async)
+qemuDomainResetDeviceRemoval(vm);
return ret;
}

static int
qemuDomainDetachDiskDevice(virQEMUDriverPtr driver,
   virDomainObjPtr vm,
-   virDomainDiskDefPtr detach)
+   virDomainDiskDefPtr detach,
+   bool async)
{
int ret = -1;
qemuDomainObjPrivatePtr priv = vm->privateData;
@@ -5336,7 +5375,8 @@ qemuDomainDetachWatchdog(virQEMUDriverPtr driver,
return -1;
}

-qemuDomainMarkDeviceForRemoval(vm, >info);
+if (!async)
+qemuDomainMarkDeviceForRemoval(vm, >info);

qemuDomainObjEnterMonitor(driver, vm);
if (qemuMonitorDelDevice(priv->mon, watchdog->info.alias) < 0) {


Missing condition around qemuDomainWaitForDeviceRemoval here in
qemuDomainDetachWatchdog. It should only be called if (!async).

With that fixed:

Reviewed-by: Ján Tomko 

Jano


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

[libvirt] [PATCH 2/1] nwfilter: Fix IP address learning

[John Ferlan]

In a previous commit:

   commit d4bf8f415074759baf051644559e04fe7f8b
   Author: Daniel P. Berrangé 
   Date:   Wed Feb 14 09:43:59 2018 +

 nwfilter: handle missing switch enum cases

 Ensure all enum cases are listed in switch statements, or cast away
 enum type in places where we don't wish to cover all cases.

 Reviewed-by: John Ferlan 
 Signed-off-by: Daniel P. Berrangé 

we changed a switch in the nwfilter learning thread so that it had
explict cases for all enum entries. Unfortunately the parameters in the
method had been declared with incorrect type. The "howDetect" parameter
does *not* accept "enum howDetect" values, rather it accepts a bitmask
of "enum howDetect" values, so it should have been an "int" type.

The caller always passes DETECT_STATIC|DETECT_DHCP, so essentially the
IP addressing learning was completely broken by the above change, as it
never matched any switch case, hitting the default leading to EINVAL.

Rather than treat the howDetect as a numerical enum, let's treat it
as a bitmask/flag since that's the way it's used. Thus, the switch
changes to a simple if bit is set keeping the original intention that
if @howDetect == DETECT_DHCP, then use applyDHCPOnlyRules; otherwise,
use applyBasicRules to determine the IP Address.

Proposed-by: Daniel P. Berrangé 
Signed-off-by: John Ferlan 
---

 Here's to hoping I've used the correct magic incantation to get this
 to reply to Daniel's poll series as this should be pushed in
 conjunction with that (and thus would need review). This should be
 done before the 4.4.0 release.

 BTW: This also fixes a strange phenomena I was seeing in my testing
  environment where domain startups would periodically fail with:

  error: Failed to start domain f23
  error: Machine 'qemu-6-f23' already exists

  but could be started with enough retry attempts. What causes me
  to believe there's a relationship with this series is that if
  running libvirtd debug, I see the following as output:


  Detaching after fork from child process 22442.
  2018-05-25 20:37:24.966+: 25923: error :
  virSystemdCreateMachine:361 : Machine 'qemu-6-f23' already exists
  2018-05-25 20:37:24.988+: 22440: error :
  learnIPAddressThread:668   : encountered an error on interface
  vnet0 index 150: Invalid argument
  [Thread 0x7fffacdb7700 (LWP 22440) exited]

  With these two patches in place, no more errors.

 src/nwfilter/nwfilter_learnipaddr.c | 27 +--
 src/nwfilter/nwfilter_learnipaddr.h | 10 +-
 2 files changed, 18 insertions(+), 19 deletions(-)

diff --git a/src/nwfilter/nwfilter_learnipaddr.c 
b/src/nwfilter/nwfilter_learnipaddr.c
index 506b98fd07..ab2697274c 100644
--- a/src/nwfilter/nwfilter_learnipaddr.c
+++ b/src/nwfilter/nwfilter_learnipaddr.c
@@ -145,7 +145,7 @@ struct _virNWFilterIPAddrLearnReq {
 char *filtername;
 virHashTablePtr filterparams;
 virNWFilterDriverStatePtr driver;
-enum howDetect howDetect;
+virNWFilterLearnIPHowDetectFlags howDetect;
 
 int status;
 volatile bool terminate;
@@ -344,7 +344,7 @@ virNWFilterDeregisterLearnReq(int ifindex)
 static void
 procDHCPOpts(struct dhcp *dhcp, int dhcp_opts_len,
  uint32_t *vmaddr, uint32_t *bcastaddr,
- enum howDetect *howDetected)
+ virNWFilterLearnIPHowDetectFlags *howDetected)
 {
 struct dhcp_option *dhcpopt = >options[0];
 
@@ -413,7 +413,7 @@ learnIPAddressThread(void *arg)
 char *filter = NULL;
 uint16_t etherType;
 bool showError = true;
-enum howDetect howDetected = 0;
+virNWFilterLearnIPHowDetectFlags howDetected = 0;
 virNWFilterTechDriverPtr techdriver = req->techdriver;
 struct pollfd fds[1];
 
@@ -442,28 +442,27 @@ learnIPAddressThread(void *arg)
 
 virMacAddrFormat(>macaddr, macaddr);
 
-switch (req->howDetect) {
-case DETECT_DHCP:
+if (req->howDetect == DETECT_DHCP) {
 if (techdriver->applyDHCPOnlyRules(req->ifname,
>macaddr,
NULL, false) < 0) {
+VIR_DEBUG("Unable to apply DHCP only rules");
 req->status = EINVAL;
 goto done;
 }
 virBufferAddLit(, "src port 67 and dst port 68");
-break;
-case DETECT_STATIC:
+} else {
+/* howDetect == DETECT_DHCP ||
+ * howDetect == (DETECT_DHCP | DETECT_STATIC)
+ */
 if (techdriver->applyBasicRules(req->ifname,
 >macaddr) < 0) {
+VIR_DEBUG("Unable to apply basic rules");
 req->status = EINVAL;
 goto done;
 }
 virBufferAsprintf(, "ether host %s or ether dst ff:ff:ff:ff:ff:ff",
   

Re: [libvirt] [PATCH] nwfilter: fix IP address learning

[John Ferlan]

On 05/18/2018 07:59 AM, Daniel P. Berrangé wrote:
> In a previous commit:
> 
>   commit d4bf8f415074759baf051644559e04fe7f8b
>   Author: Daniel P. Berrangé 
>   Date:   Wed Feb 14 09:43:59 2018 +
> 
> nwfilter: handle missing switch enum cases
> 
> Ensure all enum cases are listed in switch statements, or cast away
> enum type in places where we don't wish to cover all cases.
> 
> Reviewed-by: John Ferlan 
> Signed-off-by: Daniel P. Berrangé 
> 
> we changed a switch in the nwfilter learning thread so that it had
> explict cases for all enum entries. Unfortunately the parameters in the
> method had been declared with incorrect type. The "howDetect" parameter
> does *not* accept "enum howDetect" values, rather it accepts a bitmask
> of "enum howDetect" values, so it should have been an "int" type.
> 
> The caller always passes DETECT_STATIC|DETECT_DHCP, so essentially the
> IP addressing learning was completely broken by the above change, as it
> never matched any switch case, hitting the default leading to EINVAL.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  src/nwfilter/nwfilter_learnipaddr.c | 13 ++---
>  src/nwfilter/nwfilter_learnipaddr.h |  2 +-
>  2 files changed, 7 insertions(+), 8 deletions(-)
> 

This is a companion patch to:

https://www.redhat.com/archives/libvir-list/2018-May/msg01484.html

Still, perhaps to avoid future issues the howDetect related code should
adopt a bitmask model...

Since  Daniel is away and it'd be good to get this into this release
(before he returns) I'll propose an alternative...

John


> diff --git a/src/nwfilter/nwfilter_learnipaddr.c 
> b/src/nwfilter/nwfilter_learnipaddr.c
> index cc3bfd971c..061b39d72b 100644
> --- a/src/nwfilter/nwfilter_learnipaddr.c
> +++ b/src/nwfilter/nwfilter_learnipaddr.c
> @@ -144,7 +144,7 @@ struct _virNWFilterIPAddrLearnReq {
>  char *filtername;
>  virHashTablePtr filterparams;
>  virNWFilterDriverStatePtr driver;
> -enum howDetect howDetect;
> +int howDetect; /* bitmask of enum howDetect */
>  
>  int status;
>  volatile bool terminate;
> @@ -442,23 +442,22 @@ learnIPAddressThread(void *arg)
>  if (techdriver->applyDHCPOnlyRules(req->ifname,
> >macaddr,
> NULL, false) < 0) {
> +VIR_DEBUG("Unable to apply DHCP only rules");
>  req->status = EINVAL;
>  goto done;
>  }
>  virBufferAddLit(, "src port 67 and dst port 68");
>  break;
> -case DETECT_STATIC:
> +default:
>  if (techdriver->applyBasicRules(req->ifname,
>  >macaddr) < 0) {
> +VIR_DEBUG("Unable to apply basic rules");
>  req->status = EINVAL;
>  goto done;
>  }
>  virBufferAsprintf(, "ether host %s or ether dst 
> ff:ff:ff:ff:ff:ff",
>macaddr);
>  break;
> -default:
> -req->status = EINVAL;
> -goto done;
>  }
>  
>  if (virBufferError()) {
> @@ -693,7 +692,7 @@ learnIPAddressThread(void *arg)
>   *   once its IP address has been detected
>   * @driver : the network filter driver
>   * @howDetect : the method on how the thread is supposed to detect the
> - *  IP address; must choose any of the available flags
> + *  IP address; bitmask of "enum howDetect" flags.
>   *
>   * Instruct to learn the IP address being used on a given interface (ifname).
>   * Unless there already is a thread attempting to learn the IP address
> @@ -711,7 +710,7 @@ virNWFilterLearnIPAddress(virNWFilterTechDriverPtr 
> techdriver,
>const char *filtername,
>virHashTablePtr filterparams,
>virNWFilterDriverStatePtr driver,
> -  enum howDetect howDetect)
> +  int howDetect)
>  {
>  int rc;
>  virThread thread;
> diff --git a/src/nwfilter/nwfilter_learnipaddr.h 
> b/src/nwfilter/nwfilter_learnipaddr.h
> index 06fea5bff8..753aabc594 100644
> --- a/src/nwfilter/nwfilter_learnipaddr.h
> +++ b/src/nwfilter/nwfilter_learnipaddr.h
> @@ -43,7 +43,7 @@ int virNWFilterLearnIPAddress(virNWFilterTechDriverPtr 
> techdriver,
>const char *filtername,
>virHashTablePtr filterparams,
>virNWFilterDriverStatePtr driver,
> -  enum howDetect howDetect);
> +  int howDetect);
>  
>  bool virNWFilterHasLearnReq(int ifindex);
>  int virNWFilterTerminateLearnReq(const char *ifname);
> 

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

Re: [libvirt] [PATCH] nwfilter: directly use poll to wait for packets instead of pcap_next

[John Ferlan]

On 05/21/2018 08:15 AM, Daniel P. Berrangé wrote:
> When a QEMU VM shuts down its TAP device gets deleted while nwfilter
> IP address learning thread is still capturing packets. It is seen that
> with TPACKET_V3 support in libcap, the pcap_next() call will not always
> exit its poll() when the NIC is removed. This prevents the learning
> thread from exiting which blocks the rest of libvirtd waiting on mutex
> acquisition. By switching to do poll() in libvirt code, we can ensure
> that we always exit the poll() at a time that is right for libvirt.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  src/nwfilter/nwfilter_learnipaddr.c | 37 +++--
>  1 file changed, 35 insertions(+), 2 deletions(-)
> 

FWIW: This patch is related to:

https://www.redhat.com/archives/libvir-list/2018-May/msg01440.html

Reviewed-by: John Ferlan 

I think this needs to be pushed before the release! Since Daniel is
away, I can take care of that.  I will wait for Monday though to ensure
there's no objection...

There's also a companion patch:

https://www.redhat.com/archives/libvir-list/2018-May/msg01429.html

but I have some issues with that and will post an alternative that would
also need to be pushed before the release.

John

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

Re: [libvirt] [PATCH v2 09/12] qemuDomainDetachDeviceLiveAndConfig: Avoid overwriting @ret

[Ján Tomko]

On Thu, May 24, 2018 at 01:13:36PM +0200, Michal Privoznik wrote:

The fact that we are overwriting @ret multiple times is very
confusing to see what is actually happening here.


-EPARSE
s/is very confusing/makes it difficult to see/


Follow our
traditional pattern where @ret is initialized to -1, and set to 0
only in case we know we succeeded.

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



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH v2 08/12] qemuDomainDetachDeviceLiveAndConfig: Don't use driver->caps directly

[Ján Tomko]

On Thu, May 24, 2018 at 01:13:35PM +0200, Michal Privoznik wrote:

Funny, we obtain driver caps at the beginning of the function,
but then for unknown reason access driver->caps directly.

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



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH v2 07/12] qemu_hotplug: Use more gotos in qemuDomainDetach*Device

[Ján Tomko]

On Thu, May 24, 2018 at 01:13:34PM +0200, Michal Privoznik wrote:

We are overwriting @ret a lot. It makes hard to see what is
actually going on. Use more gotos. Two functions are fixed here:
qemuDomainDetachShmemDevice() and qemuDomainDetachWatchdog().

Signed-off-by: Michal Privoznik 
---
src/qemu/qemu_hotplug.c | 42 ++
1 file changed, 22 insertions(+), 20 deletions(-)



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH v2 06/12] qemuDomainDetachWatchdog: Don't release watchdog address twice

[Ján Tomko]

On Thu, May 24, 2018 at 01:13:33PM +0200, Michal Privoznik wrote:

On watchdog unplug, when qemu doesn't support DEVICE_DELETED event
(or couple of other reasons) we do two things:

1) release watchdog device address,
2) call qemuDomainRemoveWatchdog() which does 1) again.

This is potentially dangerous.

Signed-off-by: Michal Privoznik 
---
src/qemu/qemu_hotplug.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH v2 05/12] qemuDomainDetachShmemDevice: Don't release shmem address twice

[Ján Tomko]

On Thu, May 24, 2018 at 01:13:32PM +0200, Michal Privoznik wrote:

On shmem unplug, when qemu doesn't support DEVICE_DELETED event
(or couple of other reasons) we do two things:

1) release shmem device address,
2) call qemuDomainRemoveShmemDevice() which does 1) again.

This is potentially dangerous.

Signed-off-by: Michal Privoznik 
---
src/qemu/qemu_hotplug.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH v2 04/12] qemuDomainRemoveChrDevice: Release device address

[Ján Tomko]

On Thu, May 24, 2018 at 01:13:31PM +0200, Michal Privoznik wrote:

Instead of releasing address only sometimes in
qemuDomainDetachChrDevice() let's release it whenever the device
is actually removed from the domain definition.

Signed-off-by: Michal Privoznik 
---
src/qemu/qemu_hotplug.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH v2 03/12] virsh: Expose virDomainDetachDeviceAlias

[Ján Tomko]

On Thu, May 24, 2018 at 01:13:30PM +0200, Michal Privoznik wrote:

Signed-off-by: Michal Privoznik 
---
tools/virsh-domain.c | 72 
tools/virsh.pod  | 12 +
2 files changed, 84 insertions(+)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index cfbbf5a7bc..26f7983540 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -11878,6 +11878,72 @@ cmdDetachDevice(vshControl *ctl, const vshCmd *cmd)
return funcRet;
}

+
+/*
+ * "detach-device-alias" command
+ */
+static const vshCmdInfo info_detach_device_alias[] = {
+{.name = "help",
+ .data = N_("detach device from an alias")
+},
+{.name = "desc",
+ .data = N_("Detach device from domain using given alias to identify 
device")


"from a domain" feels better. Or even:
Detach device identified by the given alias from a domain


+},
+{.name = NULL}
+};
+


[...]


+static bool
+cmdDetachDeviceAlias(vshControl *ctl, const vshCmd *cmd)
+{
+virDomainPtr dom = NULL;
+const char *alias = NULL;
+bool current = vshCommandOptBool(cmd, "current");
+bool config = vshCommandOptBool(cmd, "config");
+bool live = vshCommandOptBool(cmd, "live");
+unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT;
+bool ret = false;
+
+VSH_EXCLUSIVE_OPTIONS_VAR(current, live);
+VSH_EXCLUSIVE_OPTIONS_VAR(current, config);
+
+if (config)
+flags |= VIR_DOMAIN_AFFECT_CONFIG;
+if (live)
+flags |= VIR_DOMAIN_AFFECT_LIVE;
+
+if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
+return false;
+
+if (vshCommandOptStringReq(ctl, cmd, "alias", ) < 0)
+goto cleanup;
+
+if (virDomainDetachDeviceAlias(dom, alias, flags) < 0) {
+vshError(ctl, _("Failed to detach device with alias %s"), alias);
+goto cleanup;
+}
+
+vshPrintExtra(ctl, "%s", _("Device detached successfully\n"));


Or, to match the semantics of the API more closely:
"Device detach request sent successfully\n"


+ret = true;
+
+ cleanup:
+virshDomainFree(dom);
+return ret;
+}
+
+
/*
 * "update-device" command
 */
@@ -13999,6 +14065,12 @@ const vshCmdDef domManagementCmds[] = {
 .info = info_detach_device,
 .flags = 0
},
+{.name = "detach-device-alias",
+ .handler = cmdDetachDeviceAlias,
+ .opts = opts_detach_device_alias,
+ .info = info_detach_device_alias,
+ .flags = 0
+},
{.name = "detach-disk",
 .handler = cmdDetachDisk,
 .opts = opts_detach_disk,
diff --git a/tools/virsh.pod b/tools/virsh.pod
index 929958a953..45c1a3f271 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -3112,6 +3112,18 @@ an offline domain, and like I<--live> I<--config> for a 
running domain.
Note that older versions of virsh used I<--config> as an alias for
I<--persistent>.

+=item B I I
+[[[I<--live>] [I<--config>] | [I<--current>]] | [I<--persistent>]]
+
+Detach a device with given I from the I.
+
+If I<--live> is specified, affect a running domain.
+If I<--config> is specified, affect the next startup of a persistent domain.
+If I<--current> is specified, affect the current domain state.
+Both I<--live> and I<--config> flags may be given, but I<--current> is
+exclusive.



When no flag is specified legacy API is used whose behavior depends
+on the hypervisor driver.
+


Drop this sentence, it is not relevant for the new API.

Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH v2 02/12] remote: Implement virDomainDetachDeviceAlias

[Ján Tomko]

On Thu, May 24, 2018 at 01:13:29PM +0200, Michal Privoznik wrote:

Signed-off-by: Michal Privoznik 
---
src/remote/remote_driver.c   |  1 +
src/remote/remote_protocol.x | 16 +++-
src/remote_protocol-structs  |  6 ++
3 files changed, 22 insertions(+), 1 deletion(-)



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH v2 01/12] Introduce virDomainDetachDeviceAlias API

[Ján Tomko]

On Thu, May 24, 2018 at 01:13:28PM +0200, Michal Privoznik wrote:

When detaching a device it can be uniquely identified by its
alias. Instead of misusing virDomainDetachDeviceFlags which has
the same signature introduce new function.


s/new/a new/



Signed-off-by: Michal Privoznik 
---
include/libvirt/libvirt-domain.h |  3 +++
src/driver-hypervisor.h  |  6 +
src/libvirt-domain.c | 53 
src/libvirt_public.syms  |  5 
4 files changed, 67 insertions(+)

diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index 2d86e48979..e536781e38 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -8349,6 +8349,59 @@ virDomainUpdateDeviceFlags(virDomainPtr domain,
}


+/**
+ * virDomainDetachDeviceAlias:
+ * @domain: pointer to domain object


s/domain/a domain/

or even

d/pointer to /


+ * @alias: device alias
+ * @flags: bitwise-OR of virDomainDeviceModifyFlags
+ *
+ * Detach a virtual device from a domain, using the alias to
+ * specify device. The value of @flags should be either


s/device/the device/


+ * VIR_DOMAIN_AFFECT_CURRENT, or a bitwise-or of values from
+ * VIR_DOMAIN_AFFECT_LIVE and VIR_DOMAIN_AFFECT_CURRENT, although
+ * hypervisors vary in which flags are supported.
+ *
+ * In contrast to virDomainDetachDeviceFlags() this API only send
+ * request to the hypervisor and returns immediately.


this API is asynchronous - it returns immediately after sending
the detach request to the hypervisor.


It's
+ * caller's responsibility to wait for


s/caller/the caller/


+ * VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED event to signal actual
+ * device removal.
+ *
+ * Returns 0 in case of success, -1 in case of failure.
+ */
+int
+virDomainDetachDeviceAlias(virDomainPtr domain,
+   const char *alias,
+   unsigned int flags)


Reviewed-by: Ján Tomko 

Jano


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