Re: [libvirt] [PATCH v2 00/11] hostdev: handle usb detach/attach on node

[Daniel Henrique Barboza]

Hi,

While reviewing these I got one question that I think it's better
asked here since it's not related to a single patch.

I understand the use case for udev machinery to handle the device
removal event - in fact, I wonder if this should be done to all hostdevs,
not just USB - but I'm not sure about the handling of device add. Let's
say you have a server running lots of guests and an administrator
physically disconnect a USB device that might have been in used
as hostdev by any of them. It makes sense to remove the device
from the domain in this scenario because, well, the device isn't there
anymore.

But when the admin connects the same USB device back, is he/she
really expecting the device to be automatically assigned to the same
guest, without direct action? Isn't there a chance of this admin reconnect
back the USB device to the server for any other use, then see Libvirt
automatically re-assign the device back to the guest that was using it
before, and get not so pleased about it (i.e. furiously opening a new
Libvirt bug)?



Thanks,


DHB




On 9/9/19 8:33 AM, Nikolay Shirokovskiy wrote:

*Notes*

Deleting usb device from qemu is synchronous operation (although it
is not stated in qemu API). I did not used this knowledge in the
series.

The last patch is remnant of previus version of the series yet it is useful.

Diff to previous[1] version:
- don't use dummy device while host usb device is unplugged

[1] https://www.redhat.com/archives/libvir-list/2019-August/msg01413.html

Nikolay Shirokovskiy (11):
   qemu: track hostdev delete intention
   qemu: support host usb device unplug
   qemu: support usb hostdev plugging back
   qemu: handle host usb device add/del udev events
   qemu: handle libvirtd restart after host usb device unplug
   qemu: handle race on device deletion and usb host device plugging
   qemu: hotplug: update device list on device deleted event
   qemu: handle host usb device plug/unplug when libvirtd is down
   qemu: don't mess with non mandatory hostdevs on reattaching
   qemu: handle detaching of unplugged hostdev
   conf: parse hostdev missing flag

  src/conf/domain_conf.c   |  32 +++
  src/conf/domain_conf.h   |  15 ++
  src/qemu/Makefile.inc.am |   2 +
  src/qemu/qemu_conf.h |   3 +
  src/qemu/qemu_domain.c   |   2 +
  src/qemu/qemu_domain.h   |   2 +
  src/qemu/qemu_driver.c   | 431 ++-
  src/qemu/qemu_hotplug.c  | 104 --
  src/qemu/qemu_hotplug.h  |   3 +-
  src/qemu/qemu_process.c  |  59 ++
  src/util/virhostdev.c|   2 +
  tests/qemuhotplugtest.c  |   2 +-
  12 files changed, 637 insertions(+), 20 deletions(-)



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

Re: [libvirt] [PATCH v2 11/11] conf: parse hostdev missing flag

[Daniel Henrique Barboza]

On 9/9/19 8:33 AM, Nikolay Shirokovskiy wrote:

We want to keep this flag across libvirtd restarts.

Signed-off-by: Nikolay Shirokovskiy 
---


Reviewed-by: Daniel Henrique Barboza 


  src/conf/domain_conf.c | 6 ++
  1 file changed, 6 insertions(+)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index c200af050c..862ca4bd3a 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -7541,6 +7541,7 @@ virDomainHostdevSubsysUSBDefParseXML(xmlNodePtr node,
  VIR_AUTOFREE(char *) startupPolicy = NULL;
  VIR_AUTOFREE(char *) autoAddress = NULL;
  VIR_AUTOFREE(char *) deleteAction = NULL;
+VIR_AUTOFREE(char *) missing = NULL;
  
  if ((startupPolicy = virXMLPropString(node, "startupPolicy"))) {

  def->startupPolicy =
@@ -7570,6 +7571,11 @@ virDomainHostdevSubsysUSBDefParseXML(xmlNodePtr node,
  }
  }
  
+if ((missing = virXMLPropString(node, "missing"))) {

+if (STREQ(missing, "yes"))
+def->missing = true;
+}
+
  /* Product can validly be 0, so we need some extra help to determine
   * if it is uninitialized*/
  got_product = false;


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

Re: [libvirt] [PATCH v2 08/11] qemu: handle host usb device plug/unplug when libvirtd is down

[Daniel Henrique Barboza]

On 9/9/19 8:33 AM, Nikolay Shirokovskiy wrote:

Somebody can easily unplug usb device from host while libvirtd is being
stopped. Also usb device can be plugged or unplugged/plugged back and so
forth. Let's handle such cases.

Signed-off-by: Nikolay Shirokovskiy 
---


Reviewed-by: Daniel Henrique Barboza 


  src/qemu/qemu_process.c | 55 +
  1 file changed, 55 insertions(+)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index c9921646e9..8bec36fe2c 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3749,6 +3749,58 @@ qemuProcessUpdateDevices(virQEMUDriverPtr driver,
  return ret;
  }
  
+

+static int
+qemuProcessReattachUSBDevices(virQEMUDriverPtr driver,
+  virDomainObjPtr vm)
+{
+size_t i;
+
+for (i = 0; i < vm->def->nhostdevs; i++) {
+virDomainHostdevDefPtr hostdev = vm->def->hostdevs[i];
+virDomainHostdevSubsysUSBPtr usbsrc = >source.subsys.u.usb;
+
+if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB)
+continue;
+
+/* don't mess with devices that don't use stable host addressing
+ * with respect to unplug/plug to host
+ */
+if (!usbsrc->vendor || !usbsrc->product)
+continue;
+
+if (!usbsrc->bus && !usbsrc->device) {
+int num;
+
+if ((num = virUSBDeviceFindByVendor(usbsrc->vendor, 
usbsrc->product,
+NULL, false, NULL)) < 0)
+return -1;
+
+if (num > 0 &&
+qemuDomainAttachHostDevice(driver, vm, hostdev) < 0)
+return -1;
+} else {
+virUSBDevicePtr usb;
+
+if (virUSBDeviceFindByBus(usbsrc->bus, usbsrc->device,
+  NULL, false, ) < 0)
+return -1;
+
+if (!usb) {
+virDomainDeviceDef dev = { .type = VIR_DOMAIN_DEVICE_HOSTDEV };
+
+dev.data.hostdev = hostdev;
+if (qemuDomainDetachDeviceLive(vm, , driver, true, true) < 
0)
+return -1;
+}
+virUSBDeviceFree(usb);
+}
+}
+
+return 0;
+}
+
+
  static int
  qemuDomainPerfRestart(virDomainObjPtr vm)
  {
@@ -8206,6 +8258,9 @@ qemuProcessReconnect(void *opaque)
  if (qemuProcessUpdateDevices(driver, obj) < 0)
  goto error;
  
+if (qemuProcessReattachUSBDevices(driver, obj) < 0)

+goto error;
+
  if (qemuRefreshPRManagerState(driver, obj) < 0)
  goto error;
  


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

Re: [libvirt] [PATCH v2 09/11] qemu: don't mess with non mandatory hostdevs on reattaching

[Daniel Henrique Barboza]

On 9/9/19 8:33 AM, Nikolay Shirokovskiy wrote:

First I don't want to add code to handle dummy device that is used when
host usb device is not present at the moment of starting/migrating etc.
Second supporting non mandatory policies would require to handle races
when host usb device is plugged to host and libvirtd starts adding
device but if in the meanwhile host usb device it unplugged back then
current code will use dummy device which is not desired in this case.

Signed-off-by: Nikolay Shirokovskiy 


Reviewed-by: Daniel Henrique Barboza 


---
  src/qemu/qemu_driver.c  | 8 
  src/qemu/qemu_process.c | 4 
  2 files changed, 12 insertions(+)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index b646642c99..fe5fd94ac5 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5337,6 +5337,10 @@ processUSBAddedEvent(virQEMUDriverPtr driver,
  if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB)
  continue;
  
+if (hostdev->startupPolicy == VIR_DOMAIN_STARTUP_POLICY_OPTIONAL ||

+hostdev->startupPolicy == VIR_DOMAIN_STARTUP_POLICY_REQUISITE)
+continue;
+
  usbsrc = >source.subsys.u.usb;
  
  if (usbsrc->vendor == data->vendor && usbsrc->product == data->product)

@@ -5392,6 +5396,10 @@ processUSBRemovedEvent(virQEMUDriverPtr driver,
  if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB)
  continue;
  
+if (hostdev->startupPolicy == VIR_DOMAIN_STARTUP_POLICY_OPTIONAL ||

+hostdev->startupPolicy == VIR_DOMAIN_STARTUP_POLICY_REQUISITE)
+continue;
+
  usbsrc = >source.subsys.u.usb;
  
  /* don't mess with devices that don't use stable host addressing

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 8bec36fe2c..d87fb637ac 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3769,6 +3769,10 @@ qemuProcessReattachUSBDevices(virQEMUDriverPtr driver,
  if (!usbsrc->vendor || !usbsrc->product)
  continue;
  
+if (hostdev->startupPolicy == VIR_DOMAIN_STARTUP_POLICY_OPTIONAL ||

+hostdev->startupPolicy == VIR_DOMAIN_STARTUP_POLICY_REQUISITE)
+continue;
+
  if (!usbsrc->bus && !usbsrc->device) {
  int num;
  


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

Re: [libvirt] [PATCH v2 06/11] qemu: handle race on device deletion and usb host device plugging

[Daniel Henrique Barboza]

On 9/9/19 8:33 AM, Nikolay Shirokovskiy wrote:

Imagine host usb device is unplugged from host and as a result we send
command to qemu to delete appropriate device. Then before qemu device is
deleted host usb device is plugged back. Currenly code supposes there is


s/Currenly/Currently



no remnant device in qemu and will try to add new device and the attempt
will fail.

Instead let's check the device is not yet deleted and postpone adding
qemu device to device_deleted event handler.

Signed-off-by: Nikolay Shirokovskiy 
---


Honestly, I tried to give a NACK to this patch, not because of coding
issues, but because I find it quite convoluted what you're doing here.

qemuCheckHostdevPlugged() is calling qemuDomainAttachHostDevice()
if the USB device is found. Problem is that you're calling this check 
function

it right after qemuDomainRemoveDevice(), inside a function that is supposed
to handle delete events. The result is a flow like this:

- inside processDeviceDeletedEvent:

    virDomainDefFindDevice(vm->def, devAlias, , true)

    qemuDomainRemoveDevice(driver, vm, )

    qemuCheckHostdevPlugged(driver, vm, devAlias);

- inside qemuCheckHostdevPlugged:

  virDomainDefFindDevice(vm->def, devAlias, , false) < same 
find you just did


(  other code that verifies if the USB device exists in the host ...)

  qemuDomainAttachHostDevice(driver, vm, hostdev)


So in short, you are executing a find(), then a remove(), then the same
find() again, some code to assert that the USB is plugged in the host,
then attach().

Problem is that I didn't come up with a cleaner solution for the problem 
you're

solving here, at least considering the changes from the previous patches
and the current code base. That said:


Reviewed-by: Daniel Henrique Barboza 









  src/qemu/qemu_driver.c | 49 ++
  1 file changed, 49 insertions(+)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index f1802b5d44..21640e49c7 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4671,6 +4671,44 @@ processGuestPanicEvent(virQEMUDriverPtr driver,
  }
  
  
+static int

+qemuCheckHostdevPlugged(virQEMUDriverPtr driver,
+virDomainObjPtr vm,
+const char *devAlias)
+{
+virDomainHostdevDefPtr hostdev;
+virDomainHostdevSubsysUSBPtr usbsrc;
+virDomainDeviceDef dev;
+int num;
+
+if (virDomainDefFindDevice(vm->def, devAlias, , false) < 0)
+return 0;
+
+if (dev.type != VIR_DOMAIN_DEVICE_HOSTDEV)
+return 0;
+
+hostdev = dev.data.hostdev;
+if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB)
+return 0;
+
+usbsrc = >source.subsys.u.usb;
+if (!usbsrc->vendor || !usbsrc->product)
+return 0;
+
+if ((num = virUSBDeviceFindByVendor(usbsrc->vendor, usbsrc->product,
+NULL, false, NULL)) < 0)
+return -1;
+
+if (num == 0)
+return 0;
+
+if (qemuDomainAttachHostDevice(driver, vm, hostdev) < 0)
+return -1;
+
+return 0;
+}
+
+
  static void
  processDeviceDeletedEvent(virQEMUDriverPtr driver,
virDomainObjPtr vm,
@@ -4698,6 +4736,11 @@ processDeviceDeletedEvent(virQEMUDriverPtr driver,
  
  if (qemuDomainRemoveDevice(driver, vm, ) < 0)

  goto endjob;
+
+/* Fall thru and save status file even on error condition because
+ * device is removed successfully and changed configuration need
+ * to be saved in status file. */
+qemuCheckHostdevPlugged(driver, vm, devAlias);
  }
  
  if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, driver->caps) < 0)

@@ -5300,6 +5343,12 @@ processUSBAddedEvent(virQEMUDriverPtr driver,
  if (i == vm->def->nhostdevs)
  goto cleanup;
  
+/* if device is not yet even deleted from qemu then handle plugging later.

+ * Or we failed handling host usb device unplugging, then another attempt 
of
+ * unplug/plug could help. */
+if (usbsrc->bus || usbsrc->device)
+goto cleanup;
+
  if (qemuDomainAttachHostDevice(driver, vm, hostdev) < 0)
  goto cleanup;
  


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

Re: [libvirt] [PATCH v2 05/11] qemu: handle libvirtd restart after host usb device unplug

[Daniel Henrique Barboza]

On 9/9/19 8:33 AM, Nikolay Shirokovskiy wrote:

It is possible for libvirtd to go down before DEVICE_DELETED event is
delivered upon usb hostdev unplug and to receive the event after the
libvirtd is up. In order to handle this case we need to save
usb hostdev deleteAction is status file.


nit: s/is/in ?


Reviewed-by: Daniel Henrique Barboza 





Signed-off-by: Nikolay Shirokovskiy 
---
  src/conf/domain_conf.c | 26 ++
  src/conf/domain_conf.h |  1 +
  src/qemu/qemu_driver.c | 20 ++--
  3 files changed, 45 insertions(+), 2 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index b7a342bb91..c200af050c 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1236,6 +1236,13 @@ VIR_ENUM_IMPL(virDomainShmemModel,
"ivshmem-doorbell",
  );
  
+VIR_ENUM_IMPL(virDomainHostdevDeleteAction,

+  VIR_DOMAIN_HOSTDEV_DELETE_ACTION_LAST,
+  "none",
+  "delete",
+  "unplug"
+);
+
  VIR_ENUM_IMPL(virDomainLaunchSecurity,
VIR_DOMAIN_LAUNCH_SECURITY_LAST,
"",
@@ -7533,6 +7540,7 @@ virDomainHostdevSubsysUSBDefParseXML(xmlNodePtr node,
  virDomainHostdevSubsysUSBPtr usbsrc = >source.subsys.u.usb;
  VIR_AUTOFREE(char *) startupPolicy = NULL;
  VIR_AUTOFREE(char *) autoAddress = NULL;
+VIR_AUTOFREE(char *) deleteAction = NULL;
  
  if ((startupPolicy = virXMLPropString(node, "startupPolicy"))) {

  def->startupPolicy =
@@ -7550,6 +7558,18 @@ virDomainHostdevSubsysUSBDefParseXML(xmlNodePtr node,
  usbsrc->autoAddress = true;
  }
  
+if ((deleteAction = virXMLPropString(node, "deleteAction"))) {

+def->deleteAction =
+virDomainHostdevDeleteActionTypeFromString(deleteAction);
+
+if (def->deleteAction <= 0) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("Unknown deleteAction '%s'"),
+   deleteAction);
+goto out;
+}
+}
+
  /* Product can validly be 0, so we need some extra help to determine
   * if it is uninitialized*/
  got_product = false;
@@ -24905,6 +24925,12 @@ virDomainHostdevDefFormatSubsys(virBufferPtr buf,
  
  if (def->missing && !(flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE))

  virBufferAddLit(buf, " missing='yes'");
+
+if (def->deleteAction && (flags & VIR_DOMAIN_DEF_FORMAT_STATUS)) {
+const char *deleteAction;
+deleteAction = 
virDomainHostdevDeleteActionTypeToString(def->deleteAction);
+virBufferAsprintf(buf, " deleteAction='%s'", deleteAction);
+}
  }
  
  if (def->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI &&

diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 19a5b21462..df88790ac0 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -338,6 +338,7 @@ typedef enum {
  
  VIR_DOMAIN_HOSTDEV_DELETE_ACTION_LAST

  } virDomainHostdevDeleteActionType;
+VIR_ENUM_DECL(virDomainHostdevDeleteAction);
  
  
  /* basic device for direct passthrough */

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index ce41b0a8d9..f1802b5d44 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5272,10 +5272,13 @@ processUSBAddedEvent(virQEMUDriverPtr driver,
  {
  virDomainHostdevDefPtr hostdev;
  virDomainHostdevSubsysUSBPtr usbsrc;
+virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
  size_t i;
  
-if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)

+if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) {
+virObjectUnref(cfg);
  return;
+}
  
  if (!virDomainObjIsActive(vm)) {

  VIR_DEBUG("Domain is not running");
@@ -5300,8 +5303,13 @@ processUSBAddedEvent(virQEMUDriverPtr driver,
  if (qemuDomainAttachHostDevice(driver, vm, hostdev) < 0)
  goto cleanup;
  
+if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, driver->caps) < 0)

+VIR_WARN("unable to save domain status after plugging device %s",
+ hostdev->info->alias);
+
   cleanup:
  qemuDomainObjEndJob(driver, vm);
+virObjectUnref(cfg);
  }
  
  
@@ -5313,9 +5321,12 @@ processUSBRemovedEvent(virQEMUDriverPtr driver,

  size_t i;
  virDomainHostdevDefPtr hostdev;
  virDomainDeviceDef dev = { .type = VIR_DOMAIN_DEVICE_HOSTDEV };
+virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
  
-if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)

+if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) {
+virObjectUnref(cfg);
  return;
+}
  
  if (!virDomainObjIsActive(vm)) {

  VIR_DEBUG("Domain is not running");
@@ -5348,8 +5359,13 @@ processUSBRemovedEvent(virQEMUDriverPtr driver,
  if (qemuDomainDetachDeviceLive(vm, , driver, true, true) < 0)
  goto cleanup;
  
+if 

Re: [libvirt] [PATCH v2 04/11] qemu: handle host usb device add/del udev events

[Daniel Henrique Barboza]

On 9/9/19 8:33 AM, Nikolay Shirokovskiy wrote:

Now when code handling attaching/detaching usb hostdev is appropriately
changed use it to handle host usb device udev add/del events.


Assuming we're ok with adding an extra dependency (libudev),
LGTM.

Reviewed-by: Daniel Henrique Barboza 



Small detail below:





Signed-off-by: Nikolay Shirokovskiy 
---
  src/qemu/Makefile.inc.am |   2 +
  src/qemu/qemu_conf.h |   3 +
  src/qemu/qemu_domain.c   |   2 +
  src/qemu/qemu_domain.h   |   2 +
  src/qemu/qemu_driver.c   | 351 ++-
  5 files changed, 359 insertions(+), 1 deletion(-)

diff --git a/src/qemu/Makefile.inc.am b/src/qemu/Makefile.inc.am
index d16b315ebc..8be0dee396 100644
--- a/src/qemu/Makefile.inc.am
+++ b/src/qemu/Makefile.inc.am
@@ -85,6 +85,7 @@ libvirt_driver_qemu_impl_la_CFLAGS = \
-I$(srcdir)/conf \
-I$(srcdir)/secret \
$(AM_CFLAGS) \
+   $(UDEV_CFLAGS) \
$(NULL)
  libvirt_driver_qemu_impl_la_LDFLAGS = $(AM_LDFLAGS)
  libvirt_driver_qemu_impl_la_LIBADD = \
@@ -93,6 +94,7 @@ libvirt_driver_qemu_impl_la_LIBADD = \
$(LIBNL_LIBS) \
$(SELINUX_LIBS) \
$(LIBXML_LIBS) \
+   $(UDEV_LIBS) \
$(NULL)
  libvirt_driver_qemu_impl_la_SOURCES = $(QEMU_DRIVER_SOURCES)
  
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h

index 0cbddd7a9c..2e50bb0950 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -294,6 +294,9 @@ struct _virQEMUDriver {
  
  /* Immutable pointer, self-locking APIs */

  virHashAtomicPtr migrationErrors;
+
+struct udev_monitor *udev_monitor;
+int udev_watch;
  };
  
  virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged);

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 657f3ecfe4..4784804d1e 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -15034,6 +15034,8 @@ qemuProcessEventFree(struct qemuProcessEvent *event)
  case QEMU_PROCESS_EVENT_SERIAL_CHANGED:
  case QEMU_PROCESS_EVENT_BLOCK_JOB:
  case QEMU_PROCESS_EVENT_MONITOR_EOF:
+case QEMU_PROCESS_EVENT_USB_REMOVED:
+case QEMU_PROCESS_EVENT_USB_ADDED:
  VIR_FREE(event->data);
  break;
  case QEMU_PROCESS_EVENT_JOB_STATUS_CHANGE:
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index d097f23342..94aea62693 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -521,6 +521,8 @@ typedef enum {
  QEMU_PROCESS_EVENT_MONITOR_EOF,
  QEMU_PROCESS_EVENT_PR_DISCONNECT,
  QEMU_PROCESS_EVENT_RDMA_GID_STATUS_CHANGED,
+QEMU_PROCESS_EVENT_USB_REMOVED,
+QEMU_PROCESS_EVENT_USB_ADDED,
  
  QEMU_PROCESS_EVENT_LAST

  } qemuProcessEventType;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 2378a2e7d0..ce41b0a8d9 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -34,6 +34,7 @@
  #include 
  #include 
  #include 
+#include 
  
  
  #include "qemu_driver.h"

@@ -719,6 +720,254 @@ qemuDomainFindMaxID(virDomainObjPtr vm,
  }
  
  
+struct qemuUdevUSBRemoveData {

+unsigned int bus;
+unsigned int device;
+};
+
+struct qemuUdevUSBAddData {
+unsigned int vendor;
+unsigned int product;
+};
+
+struct qemuUdevUSBEventData {
+union {
+struct qemuUdevUSBRemoveData remove;
+struct qemuUdevUSBAddData add;
+} data;
+bool found;
+bool remove;
+};
+
+static int
+qemuUdevUSBHandleEvent(virDomainObjPtr vm, void *opaque)
+{
+struct qemuUdevUSBEventData *data = opaque;
+struct qemuProcessEvent *event = NULL;
+size_t i;
+
+if (data->found)
+return 0;
+
+virObjectLock(vm);
+
+if (!virDomainObjIsActive(vm))
+goto cleanup;
+
+for (i = 0; i < vm->def->nhostdevs; i++) {
+virDomainHostdevDefPtr hostdev = vm->def->hostdevs[i];
+virDomainHostdevSubsysUSBPtr usbsrc = >source.subsys.u.usb;
+
+if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB)
+continue;
+
+if (data->remove) {
+if (usbsrc->bus != data->data.remove.bus ||
+usbsrc->device != data->data.remove.device)
+continue;
+} else {
+if (usbsrc->vendor != data->data.add.vendor ||
+usbsrc->product != data->data.add.product)
+continue;
+}
+
+data->found = true;
+
+if (VIR_ALLOC(event) < 0)
+goto cleanup;
+
+if (data->remove) {
+struct qemuUdevUSBRemoveData *rm_data;
+
+
+if (VIR_ALLOC(rm_data) < 0)
+goto cleanup;
+
+*rm_data = data->data.remove;
+event->data = rm_data;
+event->eventType = QEMU_PROCESS_EVENT_USB_REMOVED;
+} else {
+struct qemuUdevUSBAddData *add_data;
+
+if (VIR_ALLOC(add_data) < 0)
+goto cleanup;
+
+*add_data = data->data.add;
+event->data = add_data;
+

Re: [libvirt] [PATCH v2 03/11] qemu: support usb hostdev plugging back

[Daniel Henrique Barboza]

On 9/9/19 8:33 AM, Nikolay Shirokovskiy wrote:

We are going to use qemuDomainAttachHostUSBDevice when
host usb device is plugged back to node.

Signed-off-by: Nikolay Shirokovskiy 
---
  src/qemu/qemu_hotplug.c | 29 -
  1 file changed, 24 insertions(+), 5 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index b045735022..ea82cb54ef 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -2437,8 +2437,18 @@ qemuDomainAttachHostUSBDevice(virQEMUDriverPtr driver,
  bool teardownlabel = false;
  bool teardowndevice = false;
  int ret = -1;
+bool replug;
+size_t i;
+
+for (i = 0; i < vm->def->nhostdevs; i++) {
+if (vm->def->hostdevs[i] == hostdev) {
+replug = true;
+break;
+}
+}
  
-if (virDomainUSBAddressEnsure(priv->usbaddrs, hostdev->info) < 0)

+if (!replug &&
+virDomainUSBAddressEnsure(priv->usbaddrs, hostdev->info) < 0)


Compilation didn't complain about it, but for clarity, you can initialize
'replug' up there to 'false' to avoid using an uninitialized value here
(since replug will not necessarily be set to 'true' in the for loop).


Looks good otherwise.

Reviewed-by: Daniel Henrique Barboza 



  return -1;
  
  if (qemuHostdevPrepareUSBDevices(driver, vm->def->name, , 1, 0) < 0)

@@ -2463,7 +2473,7 @@ qemuDomainAttachHostUSBDevice(virQEMUDriverPtr driver,
  if (!(devstr = qemuBuildUSBHostdevDevStr(vm->def, hostdev, 
priv->qemuCaps)))
  goto cleanup;
  
-if (VIR_REALLOC_N(vm->def->hostdevs, vm->def->nhostdevs+1) < 0)

+if (!replug && VIR_REALLOC_N(vm->def->hostdevs, vm->def->nhostdevs+1) < 0)
  goto cleanup;
  
  qemuDomainObjEnterMonitor(driver, vm);

@@ -2476,7 +2486,8 @@ qemuDomainAttachHostUSBDevice(virQEMUDriverPtr driver,
  if (ret < 0)
  goto cleanup;
  
-vm->def->hostdevs[vm->def->nhostdevs++] = hostdev;

+if (!replug)
+vm->def->hostdevs[vm->def->nhostdevs++] = hostdev;
  
  ret = 0;

   cleanup:
@@ -2489,9 +2500,17 @@ qemuDomainAttachHostUSBDevice(virQEMUDriverPtr driver,
  if (teardowndevice &&
  qemuDomainNamespaceTeardownHostdev(vm, hostdev) < 0)
  VIR_WARN("Unable to remove host device from /dev");
-if (added)
+if (added) {
  qemuHostdevReAttachUSBDevices(driver, vm->def->name, , 1);
-virDomainUSBAddressRelease(priv->usbaddrs, hostdev->info);
+
+if (replug) {
+virDomainHostdevSubsysUSBPtr usbsrc = 
>source.subsys.u.usb;
+usbsrc->bus = 0;
+usbsrc->device = 0;
+}
+}
+if (!replug)
+virDomainUSBAddressRelease(priv->usbaddrs, hostdev->info);
  }
  VIR_FREE(devstr);
  return ret;


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

Re: [libvirt] [PATCH v2 02/11] qemu: support host usb device unplug

[Daniel Henrique Barboza]

On 9/9/19 8:33 AM, Nikolay Shirokovskiy wrote:

Handle host usb device unplug in DEVICE_DELETED handle execution
path.

Signed-off-by: Nikolay Shirokovskiy 
---


Reviewed-by: Daniel Henrique Barboza 



  src/qemu/qemu_hotplug.c | 38 +++---
  1 file changed, 31 insertions(+), 7 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 559254b234..b045735022 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -4366,7 +4366,8 @@ qemuDomainRemoveUSBHostDevice(virQEMUDriverPtr driver,
virDomainHostdevDefPtr hostdev)
  {
  qemuHostdevReAttachUSBDevices(driver, vm->def->name, , 1);
-qemuDomainReleaseDeviceAddress(vm, hostdev->info);
+if (hostdev->deleteAction != VIR_DOMAIN_HOSTDEV_DELETE_ACTION_UNPLUG)
+qemuDomainReleaseDeviceAddress(vm, hostdev->info);
  }
  
  static void

@@ -4408,6 +4409,7 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver,
  char *drivealias = NULL;
  char *objAlias = NULL;
  bool is_vfio = false;
+bool unplug = hostdev->deleteAction == 
VIR_DOMAIN_HOSTDEV_DELETE_ACTION_UNPLUG;
  
  VIR_DEBUG("Removing host device %s from domain %p %s",

hostdev->info->alias, vm, vm->def->name);
@@ -4454,16 +4456,24 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver,
  }
  }
  
-for (i = 0; i < vm->def->nhostdevs; i++) {

-if (vm->def->hostdevs[i] == hostdev) {
-virDomainHostdevRemove(vm->def, i);
-break;
+if (!unplug) {
+for (i = 0; i < vm->def->nhostdevs; i++) {
+if (vm->def->hostdevs[i] == hostdev) {
+virDomainHostdevRemove(vm->def, i);
+break;
+}
  }
  }
  
  virDomainAuditHostdev(vm, hostdev, "detach", true);
  
-if (!is_vfio &&

+/*
+ * In case of unplug the attempt to restore label will fail. But we don't
+ * need to restore the label! In case of separate mount namespace for the
+ * domain we remove device file later in this function. In case of global
+ * mount namespace the device file is deleted or being deleted by systemd.
+ */
+if (!is_vfio && !unplug &&
  qemuSecurityRestoreHostdevLabel(driver, vm, hostdev) < 0)
  VIR_WARN("Failed to restore host device labelling");
  
@@ -4497,7 +4507,13 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver,

  break;
  }
  
-virDomainHostdevDefFree(hostdev);

+if (unplug) {
+virDomainHostdevSubsysUSBPtr usbsrc = >source.subsys.u.usb;
+usbsrc->bus = 0;
+usbsrc->device = 0;
+} else {
+virDomainHostdevDefFree(hostdev);
+}
  
  if (net) {

  if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) {
@@ -4512,6 +4528,8 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver,
  virDomainNetDefFree(net);
  }
  
+hostdev->deleteAction = VIR_DOMAIN_HOSTDEV_DELETE_ACTION_NONE;

+
  ret = 0;
  
   cleanup:

@@ -4981,6 +4999,7 @@ qemuDomainRemoveDevice(virQEMUDriverPtr driver,
  virDomainDeviceInfoPtr info;
  virObjectEventPtr event;
  VIR_AUTOFREE(char *) alias = NULL;
+bool unplug;
  
  /*

   * save the alias to use when sending a DEVICE_REMOVED event after
@@ -5021,8 +5040,13 @@ qemuDomainRemoveDevice(virQEMUDriverPtr driver,
  return -1;
  break;
  case VIR_DOMAIN_DEVICE_HOSTDEV:
+unplug = dev->data.hostdev->deleteAction == 
VIR_DOMAIN_HOSTDEV_DELETE_ACTION_UNPLUG;
+
  if (qemuDomainRemoveHostDevice(driver, vm, dev->data.hostdev) < 0)
  return -1;
+
+if (unplug)
+return 0;
  break;
  case VIR_DOMAIN_DEVICE_RNG:
  if (qemuDomainRemoveRNGDevice(driver, vm, dev->data.rng) < 0)


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

[libvirt] [PATCH] maint: Use flake8 to check python code

[Shi Lei]

Replace 'sc_prohibit_semicolon_at_eol_in_python' with generic 'sc_flake8' rule
to check python code style.

Now 'sc_flake8' just check the error E703: 'statement ends with a semicolon'.
In future, we could use '--select' to introduce more rules.

Signed-off-by: Shi Lei 
---
 cfg.mk   | 8 +++-
 configure.ac | 4 
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/cfg.mk b/cfg.mk
index 42e1abf0..8acc45ac 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -812,11 +812,9 @@ sc_require_enum_last_marker:
exit 1; } || :
 
 # In Python files we don't want to end lines with a semicolon like in C
-sc_prohibit_semicolon_at_eol_in_python:
-   @prohibit='^[^#].*\;$$' \
-   in_vc_files='\.py$$' \
-   halt='python does not require to end lines with a semicolon' \
- $(_sc_search_regexp)
+sc_flake8:
+   @$(VC_LIST_EXCEPT) | $(GREP) '\.py$$' | xargs $(FLAKE8) --select E703 \
+   | $(GREP) . && { exit 1; } || :
 
 # mymain() in test files should use return, not exit, for nicer output
 sc_prohibit_exit_in_tests:
diff --git a/configure.ac b/configure.ac
index a8f8b051..93212ca7 100644
--- a/configure.ac
+++ b/configure.ac
@@ -704,6 +704,10 @@ AC_PATH_PROGS([PYTHON], [python3 python2 python])
 if test -z "$PYTHON"; then
 AC_MSG_ERROR(['python3', 'python2' or 'python' binary is required to build 
libvirt])
 fi
+AC_PATH_PROG([FLAKE8], [flake8])
+if test -z "$FLAKE8"; then
+AC_MSG_ERROR(['flake8' binary is required to check python code style])
+fi
 AC_PATH_PROG([PERL], [perl])
 if test -z "$PERL"; then
  AC_MSG_ERROR(['perl' binary is required to build libvirt])
-- 
2.17.1


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

Re: [libvirt] [PATCH] maint: Use pep8 to implement sc_prohibit_semicolon_at_eol_in_python

[shi_...@massclouds.com]

>On Thu, Sep 12, 2019 at 10:05:34AM -0400, Cole Robinson wrote:
>> On 9/12/19 9:18 AM, Andrea Bolognani wrote:
>> > On Thu, 2019-09-12 at 12:00 +0100, Daniel P. Berrangé wrote:
>> >> On Thu, Sep 12, 2019 at 12:56:04PM +0200, Andrea Bolognani wrote:
>> >>> FWIW, libvirt-dbus is using flake8 to achieve what I believe is
>> >>> basically the same result, whereas virt-manager I think uses pylint
>> >>> and pycodestlye.
>> >>>
>> >>> I am not familiar enough with the Python ecosystem to be able to
>> >>> compare the various linters, but it would IMHO make sense to at
>> >>> least try to standardize on one or more of them and use them across
>> >>> libvirt-related projects.
>> >>
>> >> pep8 validates code style against published PEP style guidelines.
>> >>
>> >> pyflakes does static analysis to detect code errors
>> >>
>> >> flake8 is a wrapper that runs pep8 and pyflakes and does some
>> >> other stuff.
>> >>
>> >> For just doing this semicolon check then pep8 is sufficient,
>> >> but for a more general approach, then flake8 makes more
>> >> sense. In that case we'd delete sc_prohibit_semicolon_at_eol_in_python
>> >> entirely, and simply have a generic 'sc_flake8' check that runs a
>> >> configured list of checks against all py code.
>> >
>> > Yeah, the more general approach makes sense to me, so I would go
>> > that route directly instead of introducing pep8 first and only then
>> > moving to flake8.
>> >
>>
>> I don't have much experience with flake8 but I know it's commonly used,
>> so it sounds fine to me. FWIW though the pep8 tool naming is outdated,
>> the project was renamed to pycodestyle in early 2016, which does affect
>> config file naming and format
>>
>> https://github.com/PyCQA/pycodestyle
>
>flake8 seems to know to use pycodestyle, so I'd say we should go
>straight to flake8 and thus avoid worrying bout the pep/pycodestyle
>rename.
>
>Regards,
>Daniel
>--

Ok. I try to provide another patch to remove 
sc_prohibit_semicolon_at_eol_in_python
and add a new 'sc_flake8' check.

Thanks,
Shi Lei

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

Re: [libvirt] [PATCH] maint: Use pep8 to implement sc_prohibit_semicolon_at_eol_in_python

[Daniel P . Berrangé]

On Thu, Sep 12, 2019 at 10:05:34AM -0400, Cole Robinson wrote:
> On 9/12/19 9:18 AM, Andrea Bolognani wrote:
> > On Thu, 2019-09-12 at 12:00 +0100, Daniel P. Berrangé wrote:
> >> On Thu, Sep 12, 2019 at 12:56:04PM +0200, Andrea Bolognani wrote:
> >>> FWIW, libvirt-dbus is using flake8 to achieve what I believe is
> >>> basically the same result, whereas virt-manager I think uses pylint
> >>> and pycodestlye.
> >>>
> >>> I am not familiar enough with the Python ecosystem to be able to
> >>> compare the various linters, but it would IMHO make sense to at
> >>> least try to standardize on one or more of them and use them across
> >>> libvirt-related projects.
> >>
> >> pep8 validates code style against published PEP style guidelines.
> >>
> >> pyflakes does static analysis to detect code errors
> >>
> >> flake8 is a wrapper that runs pep8 and pyflakes and does some
> >> other stuff.
> >>
> >> For just doing this semicolon check then pep8 is sufficient,
> >> but for a more general approach, then flake8 makes more
> >> sense. In that case we'd delete sc_prohibit_semicolon_at_eol_in_python
> >> entirely, and simply have a generic 'sc_flake8' check that runs a
> >> configured list of checks against all py code.
> > 
> > Yeah, the more general approach makes sense to me, so I would go
> > that route directly instead of introducing pep8 first and only then
> > moving to flake8.
> > 
> 
> I don't have much experience with flake8 but I know it's commonly used,
> so it sounds fine to me. FWIW though the pep8 tool naming is outdated,
> the project was renamed to pycodestyle in early 2016, which does affect
> config file naming and format
> 
> https://github.com/PyCQA/pycodestyle

flake8 seems to know to use pycodestyle, so I'd say we should go
straight to flake8 and thus avoid worrying bout the pep/pycodestyle
rename. 

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

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

Re: [libvirt] [PATCH] conf: correctly convert 'managed' attribute from network port

[Michal Privoznik]

On 9/12/19 5:05 PM, Daniel P. Berrangé wrote:

The virNetworkPortDef config stores the 'managed' attribute
as the virTristate type.

The virDomainDef config stores the 'managed' attribute as
the bool type.

Signed-off-by: Daniel P. Berrangé 
---
  src/conf/domain_conf.c | 15 +--
  1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 3dc638f0de..ae196cac52 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -30688,7 +30688,15 @@ 
virDomainNetDefActualFromNetworkPort(virDomainNetDefPtr iface,
  actual->data.hostdev.def.parentnet = iface;
  actual->data.hostdev.def.info = >info;
  actual->data.hostdev.def.mode = VIR_DOMAIN_HOSTDEV_MODE_SUBSYS;
-actual->data.hostdev.def.managed = port->plug.hostdevpci.managed;
+switch (port->plug.hostdevpci.managed) {
+case VIR_TRISTATE_BOOL_YES:
+actual->data.hostdev.def.managed = true;
+break;
+case VIR_TRISTATE_BOOL_ABSENT:
+case VIR_TRISTATE_BOOL_NO:
+actual->data.hostdev.def.managed = false;
+break;
+}
  actual->data.hostdev.def.source.subsys.type = 
VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI;
  actual->data.hostdev.def.source.subsys.u.pci.addr = 
port->plug.hostdevpci.addr;
  switch 
((virNetworkForwardDriverNameType)port->plug.hostdevpci.driver) {
@@ -30820,7 +30828,10 @@ virDomainNetDefActualToNetworkPort(virDomainDefPtr dom,
 iface->ifname);
  goto error;
  }
-port->plug.hostdevpci.managed = actual->data.hostdev.def.managed;
+if (actual->data.hostdev.def.managed)
+port->plug.hostdevpci.managed = VIR_TRISTATE_BOOL_YES;
+else
+port->plug.hostdevpci.managed = VIR_TRISTATE_BOOL_NO;


Or just use virTristateBoolFromBool(). Unfortunately, we don't have a 
counterpart to use in the first hunk.


Reviewed-by: Michal Privoznik 

Michal

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

[libvirt] [PATCH python] sanitytest: whitelist 'network' method as having no C impl

[Daniel P . Berrangé]

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

Pushed as a CI build fix for the previous patch.

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

diff --git a/sanitytest.py b/sanitytest.py
index e87b57d..0b415fd 100644
--- a/sanitytest.py
+++ b/sanitytest.py
@@ -357,7 +357,7 @@ for klass in gotfunctions:
 for func in sorted(gotfunctions[klass]):
 # These are pure python methods with no C APi
 if func in ["connect", "getConnect", "domain", "getDomain",
-"virEventInvokeFreeCallback",
+"virEventInvokeFreeCallback", "network",
 "sparseRecvAll", "sparseSendAll"]:
 continue
 
-- 
2.21.0

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

[libvirt] [PATCH] conf: correctly convert 'managed' attribute from network port

[Daniel P . Berrangé]

The virNetworkPortDef config stores the 'managed' attribute
as the virTristate type.

The virDomainDef config stores the 'managed' attribute as
the bool type.

Signed-off-by: Daniel P. Berrangé 
---
 src/conf/domain_conf.c | 15 +--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 3dc638f0de..ae196cac52 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -30688,7 +30688,15 @@ 
virDomainNetDefActualFromNetworkPort(virDomainNetDefPtr iface,
 actual->data.hostdev.def.parentnet = iface;
 actual->data.hostdev.def.info = >info;
 actual->data.hostdev.def.mode = VIR_DOMAIN_HOSTDEV_MODE_SUBSYS;
-actual->data.hostdev.def.managed = port->plug.hostdevpci.managed;
+switch (port->plug.hostdevpci.managed) {
+case VIR_TRISTATE_BOOL_YES:
+actual->data.hostdev.def.managed = true;
+break;
+case VIR_TRISTATE_BOOL_ABSENT:
+case VIR_TRISTATE_BOOL_NO:
+actual->data.hostdev.def.managed = false;
+break;
+}
 actual->data.hostdev.def.source.subsys.type = 
VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI;
 actual->data.hostdev.def.source.subsys.u.pci.addr = 
port->plug.hostdevpci.addr;
 switch ((virNetworkForwardDriverNameType)port->plug.hostdevpci.driver) 
{
@@ -30820,7 +30828,10 @@ virDomainNetDefActualToNetworkPort(virDomainDefPtr dom,
iface->ifname);
 goto error;
 }
-port->plug.hostdevpci.managed = actual->data.hostdev.def.managed;
+if (actual->data.hostdev.def.managed)
+port->plug.hostdevpci.managed = VIR_TRISTATE_BOOL_YES;
+else
+port->plug.hostdevpci.managed = VIR_TRISTATE_BOOL_NO;
 port->plug.hostdevpci.addr = 
actual->data.hostdev.def.source.subsys.u.pci.addr;
 switch 
((virDomainHostdevSubsysPCIBackendType)actual->data.hostdev.def.source.subsys.u.pci.backend)
 {
 case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_DEFAULT:
-- 
2.21.0

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

Re: [libvirt] [PATCH v2 1/4] util: purge all code for testing OOM handling

[Michal Privoznik]

On 9/12/19 1:31 PM, Daniel P. Berrangé wrote:

The OOM handling requires special build time options which we never
enable in our CI. Even once enabled the tests are incredibly slow and
typically require manual inspection of the results to weed out false
positives.

Since there was previous agreement to switch to abort on OOM in libvirt
code, there's no point continuing to keep the unused OOM testing code.

Reviewed-by: Ján Tomko 
Signed-off-by: Daniel P. Berrangé 
---
  configure.ac  |  17 ---
  docs/docs.html.in |   3 -
  docs/internals/oomtesting.html.in | 213 --
  src/libvirt_private.syms  |   4 -
  src/util/viralloc.c   | 111 
  src/util/viralloc.h   |   5 -
  tests/Makefile.am |   1 -
  tests/oomtrace.pl |  41 --
  tests/qemuxml2argvtest.c  |  12 +-
  tests/testutils.c | 189 +-
  tests/testutils.h |   2 -
  tests/virfirewalltest.c   |  12 --
  12 files changed, 6 insertions(+), 604 deletions(-)
  delete mode 100644 docs/internals/oomtesting.html.in
  delete mode 100755 tests/oomtrace.pl

diff --git a/configure.ac b/configure.ac
index bf9e7681bc..8cb7de9c19 100644
--- a/configure.ac
+++ b/configure.ac
@@ -764,22 +764,6 @@ if test "$enable_test_coverage" = yes; then
WARN_CFLAGS=$save_WARN_CFLAGS
  fi
  
-LIBVIRT_ARG_ENABLE([TEST_OOM], [memory allocation failure checking], [no])

-case "$enable_test_oom" in
-  yes|no) ;;
-  *) AC_MSG_ERROR([bad value ${enable_test_oom} for test-oom option]) ;;
-esac
-
-if test "$enable_test_oom" = yes; then
-  have_trace=yes
-  AC_CHECK_HEADER([execinfo.h],[],[have_trace=no])
-  AC_CHECK_FUNC([backtrace],[],[have_trace=no])
-  if test "$have_trace" = "yes"; then
-AC_DEFINE([TEST_OOM_TRACE], 1, [Whether backtrace() is available])
-  fi
-  AC_DEFINE([TEST_OOM], 1, [Whether malloc OOM checking is enabled])
-fi
-
  LIBVIRT_ARG_ENABLE([TEST_LOCKING], [thread locking tests using CIL], [no])
  case "$enable_test_locking" in
yes|no) ;;
@@ -1048,7 +1032,6 @@ AC_MSG_NOTICE([])
  AC_MSG_NOTICE([Test suite])
  AC_MSG_NOTICE([])
  AC_MSG_NOTICE([ Coverage: $enable_test_coverage])
-AC_MSG_NOTICE([Alloc OOM: $enable_test_oom])


I've just created a merge conflict here, sorry. But it's trivial to resolve.

Reviewed-by: Michal Privoznik 

Michal

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

Re: [libvirt] [PATCH v2 2/4] util: make allocation functions abort on OOM

[Michal Privoznik]

On 9/12/19 1:31 PM, Daniel P. Berrangé wrote:

The functions are left returning an "int" to avoid an immediate
big-bang cleanup. They'll simply never return anything other
than 0, except for virInsertN which can still return an error
if the requested insertion index is out of range. Interestingly
in that case, the _QUIET function would none the less report
an error.

Reviewed-by: Ján Tomko 
Signed-off-by: Daniel P. Berrangé 
---
  src/util/viralloc.c | 203 +++-
  src/util/viralloc.h | 133 ++---
  2 files changed, 93 insertions(+), 243 deletions(-)



Reviewed-by: Michal Privoznik 

Michal

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

Re: [libvirt] [PATCH v2 3/4] util: remove several unused _QUIET allocation macro variants

[Michal Privoznik]

On 9/12/19 1:31 PM, Daniel P. Berrangé wrote:

Only a few of the _QUIET allocation macros are used. Since we're no
longer reporting OOM as errors, we want to eliminate all the _QUIET
variants. This starts with the easy, unused, cases.

Reviewed-by: Ján Tomko 
Signed-off-by: Daniel P. Berrangé 
---
  src/util/viralloc.h | 74 -
  1 file changed, 74 deletions(-)


Reviewed-by: Michal Privoznik 

Michal

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

Re: [libvirt] [PATCH v2 4/4] util: make string functions abort on OOM

[Michal Privoznik]

On 9/12/19 1:31 PM, Daniel P. Berrangé wrote:

The functions are left returning an "int" to avoid an immediate
big-bang cleanup. They'll simply never return anything other
than 0.

Reviewed-by: Ján Tomko 
Signed-off-by: Daniel P. Berrangé 
---
  src/util/virstring.c | 93 +++-
  src/util/virstring.h | 73 --
  2 files changed, 47 insertions(+), 119 deletions(-)


Reviewed-by: Michal Privoznik 

Michal

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

Re: [libvirt] [PATCH] fix memleak in virNodeDeviceGetPCISRIOVCaps

[Michal Privoznik]

On 9/12/19 10:05 AM, Yi Wang wrote:

From: Jiang Kun 

it always alloc new memory when get dumpxml of pf device,but never free it.
Now free the old first,then alloc new memory.

Signed-off-by: Jiang kun 
---
  src/conf/node_device_conf.c | 1 +
  1 file changed, 1 insertion(+)


Reviewed-by: Michal Privoznik  and pushed.
Congratulations Jiang on your first libvirt contribution.

Michal

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

Re: [libvirt] [PATCH] maint: Use pep8 to implement sc_prohibit_semicolon_at_eol_in_python

[Cole Robinson]

On 9/12/19 9:18 AM, Andrea Bolognani wrote:
> On Thu, 2019-09-12 at 12:00 +0100, Daniel P. Berrangé wrote:
>> On Thu, Sep 12, 2019 at 12:56:04PM +0200, Andrea Bolognani wrote:
>>> FWIW, libvirt-dbus is using flake8 to achieve what I believe is
>>> basically the same result, whereas virt-manager I think uses pylint
>>> and pycodestlye.
>>>
>>> I am not familiar enough with the Python ecosystem to be able to
>>> compare the various linters, but it would IMHO make sense to at
>>> least try to standardize on one or more of them and use them across
>>> libvirt-related projects.
>>
>> pep8 validates code style against published PEP style guidelines.
>>
>> pyflakes does static analysis to detect code errors
>>
>> flake8 is a wrapper that runs pep8 and pyflakes and does some
>> other stuff.
>>
>> For just doing this semicolon check then pep8 is sufficient,
>> but for a more general approach, then flake8 makes more
>> sense. In that case we'd delete sc_prohibit_semicolon_at_eol_in_python
>> entirely, and simply have a generic 'sc_flake8' check that runs a
>> configured list of checks against all py code.
> 
> Yeah, the more general approach makes sense to me, so I would go
> that route directly instead of introducing pep8 first and only then
> moving to flake8.
> 

I don't have much experience with flake8 but I know it's commonly used,
so it sounds fine to me. FWIW though the pep8 tool naming is outdated,
the project was renamed to pycodestyle in early 2016, which does affect
config file naming and format

https://github.com/PyCQA/pycodestyle

- Cole

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

Re: [libvirt] [python PATCH] generator: fix constructor for virNetworkPort

[Michal Privoznik]

On 9/12/19 2:53 PM, Daniel P. Berrangé wrote:

The virNetworkPort class is passed both the virNetwork parent
python class and the virNetworkPort C object. This needs special
handling in the generator, similar to how virDomainSnapshots are
dealt with.

Signed-off-by: Daniel P. Berrangé 
---
  generator.py | 13 +
  1 file changed, 13 insertions(+)


Reviewed-by: Michal Privoznik 

Michal

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

Re: [libvirt] [PATCH] tools: fix XML validator detection of network port XML schema

[Michal Privoznik]

On 9/12/19 3:12 PM, Daniel P. Berrangé wrote:

Signed-off-by: Daniel P. Berrangé 
---
  tools/virt-xml-validate.in | 3 +++
  1 file changed, 3 insertions(+)


Reviewed-by: Michal Privoznik 

Michal

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

Re: [libvirt] [PATCH] tools: add virsh docs for network port commands

[Michal Privoznik]

On 9/12/19 3:07 PM, Daniel P. Berrangé wrote:

Signed-off-by: Daniel P. Berrangé 
---
  tools/virsh.pod | 36 
  1 file changed, 36 insertions(+)



Reviewed-by: Michal Privoznik 

Michal

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

Re: [libvirt] [PATCH] conf: avoid looking up network port that doesn't exist

[Michal Privoznik]

On 9/12/19 3:24 PM, Daniel P. Berrangé wrote:

If the hypervisor driver has not yet created the network port, the
portid field will be "----".

If a failure occurs during early VM startup, the hypervisor driver may
none the less try to release the network port, resulting in an
undesirable warning:

2019-09-12 13:17:42.349+: 16544: error :
virNetworkObjLookupPort:1679 : network port not found: Network port with
UUID ---- does not exist

By checking if the portid UUID is valid, we can avoid polluting the logs
in this way.

Signed-off-by: Daniel P. Berrangé 
---
  src/conf/domain_conf.c | 6 ++
  1 file changed, 6 insertions(+)



Reviewed-by: Michal Privoznik 

Michal

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

[libvirt] [PATCH] qemu_capabilities: Put only unique FW images into domcaps

[Michal Privoznik]

In the domain capabilities XML there are FW image paths printed.
There are two sources for the image paths (in order of
preference):

  1) firmware descriptor files - as returned by
  qemuFirmwareGetSupported()

  2) a compile time list of FW:NRAM pairs which can be overridden
  in qemu.conf

If either of those contains a duplicate FW image path (which is
a valid use case) it is printed twice in the capabilities XML.
While it's technically not a bug, it doesn't look good.

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

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 9b19930964..489a6872c4 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -5136,12 +5136,22 @@ virQEMUCapsFillDomainLoaderCaps(virDomainCapsLoaderPtr 
capsLoader,
 
 for (i = 0; i < nfirmwares; i++) {
 const char *filename = firmwares[i]->name;
+size_t j;
 
 if (!virFileExists(filename)) {
 VIR_DEBUG("loader filename=%s does not exist", filename);
 continue;
 }
 
+/* Put only unique FW images onto the list */
+for (j = 0; j < capsLoader->values.nvalues; j++) {
+if (STREQ(filename, capsLoader->values.values[j]))
+break;
+}
+
+if (j != capsLoader->values.nvalues)
+continue;
+
 if (VIR_STRDUP(capsLoader->values.values[capsLoader->values.nvalues],
filename) < 0)
 return -1;
-- 
2.21.0

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

[libvirt] [PATCH] conf: avoid looking up network port that doesn't exist

[Daniel P . Berrangé]

If the hypervisor driver has not yet created the network port, the
portid field will be "----".

If a failure occurs during early VM startup, the hypervisor driver may
none the less try to release the network port, resulting in an
undesirable warning:

2019-09-12 13:17:42.349+: 16544: error :
virNetworkObjLookupPort:1679 : network port not found: Network port with
UUID ---- does not exist

By checking if the portid UUID is valid, we can avoid polluting the logs
in this way.

Signed-off-by: Daniel P. Berrangé 
---
 src/conf/domain_conf.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 3dc638f0de..b1f8a13319 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -30994,6 +30994,12 @@ virDomainNetReleaseActualDevice(virConnectPtr conn,
 virNetworkPortPtr port = NULL;
 int ret = -1;
 
+/* Port might not exist if a failure occurred during VM startup */
+if (!virUUIDIsValid(iface->data.network.portid)) {
+ret = 0;
+goto cleanup;
+}
+
 if (!(net = virNetworkLookupByName(conn, iface->data.network.name)))
 goto cleanup;
 
-- 
2.21.0

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

Re: [libvirt] [PATCH] maint: Use pep8 to implement sc_prohibit_semicolon_at_eol_in_python

[Andrea Bolognani]

On Thu, 2019-09-12 at 12:00 +0100, Daniel P. Berrangé wrote:
> On Thu, Sep 12, 2019 at 12:56:04PM +0200, Andrea Bolognani wrote:
> > FWIW, libvirt-dbus is using flake8 to achieve what I believe is
> > basically the same result, whereas virt-manager I think uses pylint
> > and pycodestlye.
> > 
> > I am not familiar enough with the Python ecosystem to be able to
> > compare the various linters, but it would IMHO make sense to at
> > least try to standardize on one or more of them and use them across
> > libvirt-related projects.
> 
> pep8 validates code style against published PEP style guidelines.
> 
> pyflakes does static analysis to detect code errors
> 
> flake8 is a wrapper that runs pep8 and pyflakes and does some
> other stuff.
> 
> For just doing this semicolon check then pep8 is sufficient,
> but for a more general approach, then flake8 makes more
> sense. In that case we'd delete sc_prohibit_semicolon_at_eol_in_python
> entirely, and simply have a generic 'sc_flake8' check that runs a
> configured list of checks against all py code.

Yeah, the more general approach makes sense to me, so I would go
that route directly instead of introducing pep8 first and only then
moving to flake8.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

[libvirt] [PATCH] tools: fix XML validator detection of network port XML schema

[Daniel P . Berrangé]

Signed-off-by: Daniel P. Berrangé 
---
 tools/virt-xml-validate.in | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/virt-xml-validate.in b/tools/virt-xml-validate.in
index 5cb7dcd276..249bcf7eef 100644
--- a/tools/virt-xml-validate.in
+++ b/tools/virt-xml-validate.in
@@ -80,6 +80,9 @@ if [ -z "$TYPE" ]; then
  *domain*)
 TYPE="domain"
 ;;
+ *networkport*)
+TYPE="networkport"
+;;
  *network*)
 TYPE="network"
 ;;
-- 
2.21.0

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

[libvirt] [PATCH] tools: add virsh docs for network port commands

[Daniel P . Berrangé]

Signed-off-by: Daniel P. Berrangé 
---
 tools/virsh.pod | 36 
 1 file changed, 36 insertions(+)

diff --git a/tools/virsh.pod b/tools/virsh.pod
index 59fb4bfc2e..cf2798e71a 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -3780,6 +3780,42 @@ specified.
 
 =back
 
+=head1 NETWORK PORT COMMANDS
+
+The following commands manipulate network ports. Libvirt virtual networks
+have ports created when a virtual machine has a virtual network interface
+added. In general there should be no need to use any of the commands
+here, since the hypervisor drivers run these commands are the right
+point in a virtual machine's lifecycle. They can be useful for debugging
+problems and / or recovering from bugs / stale state.
+
+=over 4
+
+=item B { [I<--table>] | I<--uuid> }
+   I
+
+List all network ports recorded against the network.
+
+If I<--uuid> is specified network ports' UUID's are printed
+instead of a table. Flag I<--table> specifies that the legacy
+table-formatted output should be used. This is the default.
+All of these are mutually exclusive.
+
+=item B I I
+
+Allocate a new network port reserving resources based on the
+port description.
+
+=item B I I
+
+Output the network port information as an XML dump to stdout.
+
+=item B I I
+
+Delete record of the network port and release its resources
+
+=back
+
 =head1 INTERFACE COMMANDS
 
 The following commands manipulate host interfaces.  Often, these host
-- 
2.21.0

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

[libvirt] [python PATCH] generator: fix constructor for virNetworkPort

[Daniel P . Berrangé]

The virNetworkPort class is passed both the virNetwork parent
python class and the virNetworkPort C object. This needs special
handling in the generator, similar to how virDomainSnapshots are
dealt with.

Signed-off-by: Daniel P. Berrangé 
---
 generator.py | 13 +
 1 file changed, 13 insertions(+)

diff --git a/generator.py b/generator.py
index 0b0a5a7..433c1f2 100755
--- a/generator.py
+++ b/generator.py
@@ -1083,6 +1083,10 @@ class_domain_impl = {
 "virDomainSnapshot": True,
 }
 
+class_network_impl = {
+"virNetworkPort": True,
+}
+
 functions_noexcept = {
 'virDomainGetID': True,
 'virDomainGetName': True,
@@ -1551,6 +1555,8 @@ def buildWrappers(module):
 classes.write("def __init__(self, conn, _obj=None):\n")
 elif classname in [ "virDomainCheckpoint", "virDomainSnapshot" ]:
 classes.write("def __init__(self, dom, _obj=None):\n")
+elif classname in [ "virNetworkPort" ]:
+classes.write("def __init__(self, net, _obj=None):\n")
 else:
 classes.write("def __init__(self, _obj=None):\n")
 if classname in [ "virDomain", "virNetwork", "virInterface",
@@ -1564,6 +1570,9 @@ def buildWrappers(module):
 elif classname in [ "virDomainCheckpoint", "virDomainSnapshot" ]:
 classes.write("self._dom = dom\n")
 classes.write("self._conn = dom.connect()\n")
+elif classname in [ "virNetworkPort" ]:
+classes.write("self._net = net\n")
+classes.write("self._conn = net.connect()\n")
 classes.write("if type(_obj).__name__ not in 
[\"PyCapsule\", \"PyCObject\"]:\n")
 classes.write("raise Exception(\"Expected a wrapped C 
Object but got %s\" % type(_obj))\n")
 classes.write("self._o = _obj\n\n")
@@ -1585,6 +1594,10 @@ def buildWrappers(module):
 classes.write("def domain(self):\n")
 classes.write("return self._dom\n\n")
 
+if classname in class_network_impl:
+classes.write("def network(self):\n")
+classes.write("return self._net\n\n")
+
 classes.write("def c_pointer(self):\n")
 classes.write("\"\"\"Get C pointer to underlying 
object\"\"\"\n")
 classes.write("return libvirtmod.%s_pointer(self._o)\n\n" %
-- 
2.21.0

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

Re: [libvirt] [PATCH v2 01/11] qemu: track hostdev delete intention

[Daniel Henrique Barboza]

I had to amend the patch to apply it to the current master (commit
e9d51a221c). git am was putting the 'if (!unplug)' block in the
wrong function and the code didn't compile. Here's what I did:

---

$ git diff
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 96b9ff998d..08ad8ef8cc 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -5458,20 +5458,6 @@ qemuDomainDetachPrepController(virDomainObjPtr vm,
 return -1;
 }

-    /*
- * Why having additional check in second branch? Suppose client
- * asks for device detaching and we delete device from qemu
- * but don't get DEVICE_DELETED event yet. Next USB is unplugged
- * from host and we have this function called again. If we reset
- * delete action to 'unplug' then device will be left in
- * libvirt config after handling DEVICE_DELETED event while
- * it should not as client asked to detach the device before.
- */
- */
-    if (!unplug)
-    hostdev->deleteAction = VIR_DOMAIN_HOSTDEV_DELETE_ACTION_DELETE;
-    else if (hostdev->deleteAction != 
VIR_DOMAIN_HOSTDEV_DELETE_ACTION_DELETE)

-    hostdev->deleteAction = VIR_DOMAIN_HOSTDEV_DELETE_ACTION_UNPLUG;
-
 return 0;
 }

@@ -5553,6 +5539,20 @@ qemuDomainDetachPrepHostdev(virDomainObjPtr vm,
 return -1;
 }

+    /*
+ * Why having additional check in second branch? Suppose client
+ * asks for device detaching and we delete device from qemu
+ * but don't get DEVICE_DELETED event yet. Next USB is unplugged
+ * from host and we have this function called again. If we reset
+ * delete action to 'unplug' then device will be left in
+ * libvirt config after handling DEVICE_DELETED event while
+ * it should not as client asked to detach the device before.
+ */
+    if (!unplug)
+    hostdev->deleteAction = VIR_DOMAIN_HOSTDEV_DELETE_ACTION_DELETE;
+    else if (hostdev->deleteAction != 
VIR_DOMAIN_HOSTDEV_DELETE_ACTION_DELETE)

+    hostdev->deleteAction = VIR_DOMAIN_HOSTDEV_DELETE_ACTION_UNPLUG;
+
 return 0;
 }

diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c
index 3c177c6622..4e7851aa62 100644
--- a/tests/qemuhotplugtest.c
+++ b/tests/qemuhotplugtest.c
@@ -158,7 +158,7 @@ testQemuHotplugDetach(virDomainObjPtr vm,
 case VIR_DOMAIN_DEVICE_SHMEM:
 case VIR_DOMAIN_DEVICE_WATCHDOG:
 case VIR_DOMAIN_DEVICE_HOSTDEV:
-    ret = qemuDomainDetachDeviceLive(vm, dev, , async);
+    ret = qemuDomainDetachDeviceLive(vm, dev, , async, false);
 break;
 default:
 VIR_TEST_VERBOSE("device type '%s' cannot be detached",

---


A few more nits below:


On 9/9/19 8:33 AM, Nikolay Shirokovskiy wrote:

We are going to call qemuDomainDetachDeviceLive when usb device
is unplugged from host. But later when DEVICE_DELETED event is delivered
we need to keep device in libvirt config. For this purpuse let's save



s/purpuse/purpose


delete intention in device config.


There's an explanation of why are you making this change in
the comment block you put in qemuDomainDetachPrepHostdev
(and across the other commit messages of the series). It would be good
to put more a bit more info about the motivations in this first commit
message as well (like you did in patch 06), since this patch is the backbone
of the design change you're making.




Signed-off-by: Nikolay Shirokovskiy 
---
  src/conf/domain_conf.h  | 14 ++
  src/qemu/qemu_driver.c  |  4 ++--
  src/qemu/qemu_hotplug.c | 23 ---
  src/qemu/qemu_hotplug.h |  3 ++-
  tests/qemuhotplugtest.c |  2 +-
  5 files changed, 39 insertions(+), 7 deletions(-)

diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 33cef5b75c..19a5b21462 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -326,6 +326,19 @@ struct _virDomainHostdevCaps {
  } u;
  };
  
+typedef enum {

+VIR_DOMAIN_HOSTDEV_DELETE_ACTION_NONE = 0,
+/* delete associated device from libvirt config
+ * as intended by client API call */
+VIR_DOMAIN_HOSTDEV_DELETE_ACTION_DELETE,
+/* keep associated device in libvirt config as
+ * qemu device is deleted as a result of unplugging
+ * device from host */
+VIR_DOMAIN_HOSTDEV_DELETE_ACTION_UNPLUG,
+
+VIR_DOMAIN_HOSTDEV_DELETE_ACTION_LAST
+} virDomainHostdevDeleteActionType;
+
  
  /* basic device for direct passthrough */

  struct _virDomainHostdevDef {
@@ -343,6 +356,7 @@ struct _virDomainHostdevDef {
  bool missing;
  bool readonly;
  bool shareable;
+virDomainHostdevDeleteActionType deleteAction;
  union {
  virDomainHostdevSubsys subsys;
  virDomainHostdevCaps caps;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 78f5471b79..2378a2e7d0 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -9123,7 +9123,7 @@ qemuDomainDetachDeviceLiveAndConfig(virQEMUDriverPtr 
driver,
  if (flags & VIR_DOMAIN_AFFECT_LIVE) {
 

[libvirt] [PATCH v2 1/4] util: purge all code for testing OOM handling

[Daniel P . Berrangé]

The OOM handling requires special build time options which we never
enable in our CI. Even once enabled the tests are incredibly slow and
typically require manual inspection of the results to weed out false
positives.

Since there was previous agreement to switch to abort on OOM in libvirt
code, there's no point continuing to keep the unused OOM testing code.

Reviewed-by: Ján Tomko 
Signed-off-by: Daniel P. Berrangé 
---
 configure.ac  |  17 ---
 docs/docs.html.in |   3 -
 docs/internals/oomtesting.html.in | 213 --
 src/libvirt_private.syms  |   4 -
 src/util/viralloc.c   | 111 
 src/util/viralloc.h   |   5 -
 tests/Makefile.am |   1 -
 tests/oomtrace.pl |  41 --
 tests/qemuxml2argvtest.c  |  12 +-
 tests/testutils.c | 189 +-
 tests/testutils.h |   2 -
 tests/virfirewalltest.c   |  12 --
 12 files changed, 6 insertions(+), 604 deletions(-)
 delete mode 100644 docs/internals/oomtesting.html.in
 delete mode 100755 tests/oomtrace.pl

diff --git a/configure.ac b/configure.ac
index bf9e7681bc..8cb7de9c19 100644
--- a/configure.ac
+++ b/configure.ac
@@ -764,22 +764,6 @@ if test "$enable_test_coverage" = yes; then
   WARN_CFLAGS=$save_WARN_CFLAGS
 fi
 
-LIBVIRT_ARG_ENABLE([TEST_OOM], [memory allocation failure checking], [no])
-case "$enable_test_oom" in
-  yes|no) ;;
-  *) AC_MSG_ERROR([bad value ${enable_test_oom} for test-oom option]) ;;
-esac
-
-if test "$enable_test_oom" = yes; then
-  have_trace=yes
-  AC_CHECK_HEADER([execinfo.h],[],[have_trace=no])
-  AC_CHECK_FUNC([backtrace],[],[have_trace=no])
-  if test "$have_trace" = "yes"; then
-AC_DEFINE([TEST_OOM_TRACE], 1, [Whether backtrace() is available])
-  fi
-  AC_DEFINE([TEST_OOM], 1, [Whether malloc OOM checking is enabled])
-fi
-
 LIBVIRT_ARG_ENABLE([TEST_LOCKING], [thread locking tests using CIL], [no])
 case "$enable_test_locking" in
   yes|no) ;;
@@ -1048,7 +1032,6 @@ AC_MSG_NOTICE([])
 AC_MSG_NOTICE([Test suite])
 AC_MSG_NOTICE([])
 AC_MSG_NOTICE([ Coverage: $enable_test_coverage])
-AC_MSG_NOTICE([Alloc OOM: $enable_test_oom])
 AC_MSG_NOTICE([])
 AC_MSG_NOTICE([Miscellaneous])
 AC_MSG_NOTICE([])
diff --git a/docs/docs.html.in b/docs/docs.html.in
index ba9514279a..c1741c89b4 100644
--- a/docs/docs.html.in
+++ b/docs/docs.html.in
@@ -155,9 +155,6 @@
 Lock managers
 Use lock managers to protect disk content
 
-Out of memory testing
-Simulating OOM conditions in the test suite
-
 Functional testing
 Testing libvirt with TCK test suite and
  Libvirt-test-API
diff --git a/docs/internals/oomtesting.html.in 
b/docs/internals/oomtesting.html.in
deleted file mode 100644
index 72d0f2c6ff..00
--- a/docs/internals/oomtesting.html.in
+++ /dev/null
@@ -1,213 +0,0 @@
-
-
-http://www.w3.org/1999/xhtml;>
-  
-Out of memory testing
-
-
-
-
-
-  This page describes how to use the test suite todo out of memory
-  testing.
-
-
-Building with OOM testing
-
-
-  Since OOM testing requires hooking into the malloc APIs, it is
-  not enabled by default. The flag --enable-test-oom
-  must be given to configure. When this is done the
-  libvirt allocation APIs will have some hooks enabled.
-
-
-
-$ ./configure --enable-test-oom
-
-
-
-Basic OOM testing support
-
-
-  The first step in validating OOM usage is to run a test suite
-  with full OOM testing enabled. This is done by setting the
-  VIR_TEST_OOM=1 environment variable. The way this
-  works is that it runs the test once normally to "prime" any
-  static memory allocations. Then it runs it once more counting
-  the total number of memory allocations. Then it runs it in a
-  loop failing a different memory allocation each time. For every
-  memory allocation failure triggered, it expects the test case
-  to return an error. OOM testing is quite slow requiring each
-  test case to be executed O(n) times, where 'n' is the total
-  number of memory allocations. This results in a total number
-  of memory allocations of '(n * (n + 1) ) / 2'
-
-
-
-$ VIR_TEST_OOM=1 ./qemuxml2argvtest
- 1) QEMU XML-2-ARGV minimal   ... OK
-Test OOM for nalloc=42 .. OK
- 2) QEMU XML-2-ARGV minimal-s390  ... OK
-Test OOM for nalloc=28  OK
- 3) QEMU XML-2-ARGV machine-aliases1  ... OK
-Test OOM for nalloc=38 .. OK
- 4) QEMU XML-2-ARGV machine-aliases2  ... OK
-Test OOM for nalloc=38 .. OK
- 5) QEMU XML-2-ARGV machine-core-on   

[libvirt] [PATCH v2 3/4] util: remove several unused _QUIET allocation macro variants

[Daniel P . Berrangé]

Only a few of the _QUIET allocation macros are used. Since we're no
longer reporting OOM as errors, we want to eliminate all the _QUIET
variants. This starts with the easy, unused, cases.

Reviewed-by: Ján Tomko 
Signed-off-by: Daniel P. Berrangé 
---
 src/util/viralloc.h | 74 -
 1 file changed, 74 deletions(-)

diff --git a/src/util/viralloc.h b/src/util/viralloc.h
index 78f72a6c6a..3e72e40bc9 100644
--- a/src/util/viralloc.h
+++ b/src/util/viralloc.h
@@ -177,23 +177,6 @@ void virDisposeString(char **strptr)
  */
 #define VIR_EXPAND_N(ptr, count, add) virExpandN(&(ptr), sizeof(*(ptr)), 
&(count), add)
 
-/**
- * VIR_EXPAND_N_QUIET:
- * @ptr: pointer to hold address of allocated memory
- * @count: variable tracking number of elements currently allocated
- * @add: number of elements to add
- *
- * Re-allocate an array of 'count' elements, each sizeof(*ptr)
- * bytes long, to be 'count' + 'add' elements long, then store the
- * address of allocated memory in 'ptr' and the new size in 'count'.
- * The new elements are filled with zero.
- *
- * This macro is safe to use on arguments with side effects.
- *
- * Returns 0 on success, aborts on OOM
- */
-#define VIR_EXPAND_N_QUIET(ptr, count, add) VIR_EXPAND_N(ptr, count, add)
-
 /**
  * VIR_RESIZE_N:
  * @ptr: pointer to hold address of allocated memory
@@ -219,30 +202,6 @@ void virDisposeString(char **strptr)
 #define VIR_RESIZE_N(ptr, alloc, count, add) \
 virResizeN(&(ptr), sizeof(*(ptr)), &(alloc), count, add)
 
-/**
- * VIR_RESIZE_N_QUIET:
- * @ptr: pointer to hold address of allocated memory
- * @alloc: variable tracking number of elements currently allocated
- * @count: number of elements currently in use
- * @add: minimum number of elements to additionally support
- *
- * Blindly using VIR_EXPAND_N(array, alloc, 1) in a loop scales
- * quadratically, because every iteration must copy contents from
- * all prior iterations.  But amortized linear scaling can be achieved
- * by tracking allocation size separately from the number of used
- * elements, and growing geometrically only as needed.
- *
- * If 'count' + 'add' is larger than 'alloc', then geometrically reallocate
- * the array of 'alloc' elements, each sizeof(*ptr) bytes long, and store
- * the address of allocated memory in 'ptr' and the new size in 'alloc'.
- * The new elements are filled with zero.
- *
- * This macro is safe to use on arguments with side effects.
- *
- * Returns 0 on success, aborts on OOM
- */
-#define VIR_RESIZE_N_QUIET(ptr, alloc, count, add) VIR_RESIZE_N(ptr, alloc, 
count, add)
-
 /**
  * VIR_SHRINK_N:
  * @ptr: pointer to hold address of allocated memory
@@ -349,16 +308,6 @@ void virDisposeString(char **strptr)
 virInsertElementsN(&(ptr), sizeof(*(ptr)), at, &(count), \
VIR_TYPEMATCH(ptr, &(newelem)), &(newelem), false, true)
 
-/* Quiet version of macros above */
-#define VIR_INSERT_ELEMENT_QUIET(ptr, at, count, newelem) \
-VIR_INSERT_ELEMENT(ptr, at, count, newelem)
-#define VIR_INSERT_ELEMENT_COPY_QUIET(ptr, at, count, newelem) \
-VIR_INSERT_ELEMENT_COPY(ptr, at, count, newelem)
-#define VIR_INSERT_ELEMENT_INPLACE_QUIET(ptr, at, count, newelem) \
-VIR_INSERT_ELEMENT_INPLACE(ptr, at, count, newelem)
-#define VIR_INSERT_ELEMENT_COPY_INPLACE_QUIET(ptr, at, count, newelem) \
-VIR_INSERT_ELEMENT_COPY_INPLACE(ptr, at, count, newelem)
-
 /**
  * VIR_APPEND_ELEMENT:
  * @ptr: pointer to array of objects (*not* ptr to ptr)
@@ -412,8 +361,6 @@ void virDisposeString(char **strptr)
 /* Quiet version of macros above */
 #define VIR_APPEND_ELEMENT_QUIET(ptr, count, newelem) \
 VIR_APPEND_ELEMENT(ptr, count, newelem)
-#define VIR_APPEND_ELEMENT_COPY_QUIET(ptr, count, newelem) \
-VIR_APPEND_ELEMENT_COPY(ptr, count, newelem)
 
 /**
  * VIR_DELETE_ELEMENT:
@@ -472,27 +419,6 @@ void virDisposeString(char **strptr)
 #define VIR_ALLOC_VAR(ptr, type, count) \
 virAllocVar(&(ptr), sizeof(*(ptr)), sizeof(type), (count))
 
-/**
- * VIR_ALLOC_VAR_QUIET:
- * @ptr: pointer to hold address of allocated memory
- * @type: element type of trailing array
- * @count: number of array elements to allocate
- *
- * Allocate sizeof(*ptr) bytes plus an array of 'count' elements, each
- * sizeof('type').  This sort of allocation is useful for receiving
- * the data of certain ioctls and other APIs which return a struct in
- * which the last element is an array of undefined length.  The caller
- * of this type of API is expected to know the length of the array
- * that will be returned and allocate a suitable buffer to contain the
- * returned data.  C99 refers to these variable length objects as
- * structs containing flexible array members.
- *
- * This macro is safe to use on arguments with side effects.
- *
- * Returns -1 on failure, 0 on success
- */
-#define VIR_ALLOC_VAR_QUIET(ptr, type, count) VIR_ALLOC_VAR(ptr, type, count)
-
 /**
  * VIR_FREE:
  * @ptr: pointer holding address to be freed
-- 

[libvirt] [PATCH v2 2/4] util: make allocation functions abort on OOM

[Daniel P . Berrangé]

The functions are left returning an "int" to avoid an immediate
big-bang cleanup. They'll simply never return anything other
than 0, except for virInsertN which can still return an error
if the requested insertion index is out of range. Interestingly
in that case, the _QUIET function would none the less report
an error.

Reviewed-by: Ján Tomko 
Signed-off-by: Daniel P. Berrangé 
---
 src/util/viralloc.c | 203 +++-
 src/util/viralloc.h | 133 ++---
 2 files changed, 93 insertions(+), 243 deletions(-)

diff --git a/src/util/viralloc.c b/src/util/viralloc.c
index 5a0adcc706..10a8d0fb73 100644
--- a/src/util/viralloc.c
+++ b/src/util/viralloc.c
@@ -35,33 +35,20 @@ VIR_LOG_INIT("util.alloc");
  * virAlloc:
  * @ptrptr: pointer to pointer for address of allocated memory
  * @size: number of bytes to allocate
- * @report: whether to report OOM error, if there is one
- * @domcode: error domain code
- * @filename: caller's filename
- * @funcname: caller's funcname
- * @linenr: caller's line number
  *
  * Allocate  'size' bytes of memory. Return the address of the
  * allocated memory in 'ptrptr'. The newly allocated memory is
- * filled with zeros. If @report is true, OOM errors are
- * reported automatically.
+ * filled with zeros.
  *
- * Returns -1 on failure to allocate, zero on success
+ * Returns zero on success, aborts on OOM
  */
 int virAlloc(void *ptrptr,
- size_t size,
- bool report,
- int domcode,
- const char *filename,
- const char *funcname,
- size_t linenr)
+ size_t size)
 {
 *(void **)ptrptr = calloc(1, size);
-if (*(void **)ptrptr == NULL) {
-if (report)
-virReportOOMErrorFull(domcode, filename, funcname, linenr);
-return -1;
-}
+if (*(void **)ptrptr == NULL)
+abort();
+
 return 0;
 }
 
@@ -70,35 +57,22 @@ int virAlloc(void *ptrptr,
  * @ptrptr: pointer to pointer for address of allocated memory
  * @size: number of bytes to allocate
  * @count: number of elements to allocate
- * @report: whether to report OOM error, if there is one
- * @domcode: error domain code
- * @filename: caller's filename
- * @funcname: caller's funcname
- * @linenr: caller's line number
  *
  * Allocate an array of memory 'count' elements long,
  * each with 'size' bytes. Return the address of the
  * allocated memory in 'ptrptr'.  The newly allocated
- * memory is filled with zeros. If @report is true,
- * OOM errors are reported automatically.
+ * memory is filled with zeros.
  *
- * Returns -1 on failure to allocate, zero on success
+ * Returns zero on success, aborts on OOM
  */
 int virAllocN(void *ptrptr,
   size_t size,
-  size_t count,
-  bool report,
-  int domcode,
-  const char *filename,
-  const char *funcname,
-  size_t linenr)
+  size_t count)
 {
 *(void**)ptrptr = calloc(count, size);
-if (*(void**)ptrptr == NULL) {
-if (report)
-virReportOOMErrorFull(domcode, filename, funcname, linenr);
-return -1;
-}
+if (*(void**)ptrptr == NULL)
+abort();
+
 return 0;
 }
 
@@ -107,44 +81,28 @@ int virAllocN(void *ptrptr,
  * @ptrptr: pointer to pointer for address of allocated memory
  * @size: number of bytes to allocate
  * @count: number of elements in array
- * @report: whether to report OOM error, if there is one
- * @domcode: error domain code
- * @filename: caller's filename
- * @funcname: caller's funcname
- * @linenr: caller's line number
  *
  * Resize the block of memory in 'ptrptr' to be an array of
  * 'count' elements, each 'size' bytes in length. Update 'ptrptr'
  * with the address of the newly allocated memory. On failure,
  * 'ptrptr' is not changed and still points to the original memory
  * block. Any newly allocated memory in 'ptrptr' is uninitialized.
- * If @report is true, OOM errors are reported automatically.
  *
- * Returns -1 on failure to allocate, zero on success
+ * Returns zero on success, aborts on OOM
  */
 int virReallocN(void *ptrptr,
 size_t size,
-size_t count,
-bool report,
-int domcode,
-const char *filename,
-const char *funcname,
-size_t linenr)
+size_t count)
 {
 void *tmp;
 
-if (xalloc_oversized(count, size)) {
-if (report)
-virReportOOMErrorFull(domcode, filename, funcname, linenr);
-errno = ENOMEM;
-return -1;
-}
+if (xalloc_oversized(count, size))
+abort();
+
 tmp = realloc(*(void**)ptrptr, size * count);
-if (!tmp && ((size * count) != 0)) {
-if (report)
-virReportOOMErrorFull(domcode, filename, funcname, linenr);
-return -1;
-}
+if (!tmp && ((size * count) != 0))
+abort();
+
 

[libvirt] [PATCH v2 4/4] util: make string functions abort on OOM

[Daniel P . Berrangé]

The functions are left returning an "int" to avoid an immediate
big-bang cleanup. They'll simply never return anything other
than 0.

Reviewed-by: Ján Tomko 
Signed-off-by: Daniel P. Berrangé 
---
 src/util/virstring.c | 93 +++-
 src/util/virstring.h | 73 --
 2 files changed, 47 insertions(+), 119 deletions(-)

diff --git a/src/util/virstring.c b/src/util/virstring.c
index bd269e98fe..2064944b0b 100644
--- a/src/util/virstring.c
+++ b/src/util/virstring.c
@@ -726,40 +726,27 @@ virDoubleToStr(char **strp, double number)
 
 
 int
-virVasprintfInternal(bool report,
- int domcode,
- const char *filename,
- const char *funcname,
- size_t linenr,
- char **strp,
+virVasprintfInternal(char **strp,
  const char *fmt,
  va_list list)
 {
 int ret;
 
-if ((ret = vasprintf(strp, fmt, list)) == -1) {
-if (report)
-virReportOOMErrorFull(domcode, filename, funcname, linenr);
-*strp = NULL;
-}
+if ((ret = vasprintf(strp, fmt, list)) == -1)
+abort();
+
 return ret;
 }
 
 int
-virAsprintfInternal(bool report,
-int domcode,
-const char *filename,
-const char *funcname,
-size_t linenr,
-char **strp,
+virAsprintfInternal(char **strp,
 const char *fmt, ...)
 {
 va_list ap;
 int ret;
 
 va_start(ap, fmt);
-ret = virVasprintfInternal(report, domcode, filename,
-   funcname, linenr, strp, fmt, ap);
+ret = virVasprintfInternal(strp, fmt, ap);
 va_end(ap);
 return ret;
 }
@@ -937,37 +924,20 @@ virStringIsEmpty(const char *str)
  * virStrdup:
  * @dest: where to store duplicated string
  * @src: the source string to duplicate
- * @report: whether to report OOM error, if there is one
- * @domcode: error domain code
- * @filename: caller's filename
- * @funcname: caller's funcname
- * @linenr: caller's line number
- *
- * Wrapper over strdup, which reports OOM error if told so,
- * in which case callers wants to pass @domcode, @filename,
- * @funcname and @linenr which should represent location in
- * caller's body where virStrdup is called from. Consider
- * using VIR_STRDUP which sets these automatically.
- *
- * Returns: 0 for NULL src, 1 on successful copy, -1 otherwise.
+ *
+ * Wrapper over strdup, which aborts on OOM error.
+ *
+ * Returns: 0 for NULL src, 1 on successful copy, aborts on OOM
  */
 int
 virStrdup(char **dest,
-  const char *src,
-  bool report,
-  int domcode,
-  const char *filename,
-  const char *funcname,
-  size_t linenr)
+  const char *src)
 {
 *dest = NULL;
 if (!src)
 return 0;
-if (!(*dest = strdup(src))) {
-if (report)
-virReportOOMErrorFull(domcode, filename, funcname, linenr);
-return -1;
-}
+if (!(*dest = strdup(src)))
+abort();
 
 return 1;
 }
@@ -977,45 +947,28 @@ virStrdup(char **dest,
  * @dest: where to store duplicated string
  * @src: the source string to duplicate
  * @n: how many bytes to copy
- * @report: whether to report OOM error, if there is one
- * @domcode: error domain code
- * @filename: caller's filename
- * @funcname: caller's funcname
- * @linenr: caller's line number
- *
- * Wrapper over strndup, which reports OOM error if told so,
- * in which case callers wants to pass @domcode, @filename,
- * @funcname and @linenr which should represent location in
- * caller's body where virStrndup is called from. Consider
- * using VIR_STRNDUP which sets these automatically.
+ *
+ * Wrapper over strndup, which aborts on OOM error.
  *
  * In case @n is smaller than zero, the whole @src string is
  * copied.
  *
- * Returns: 0 for NULL src, 1 on successful copy, -1 otherwise.
+ * Returns: 0 for NULL src, 1 on successful copy, aborts on OOM
  */
 int
 virStrndup(char **dest,
const char *src,
-   ssize_t n,
-   bool report,
-   int domcode,
-   const char *filename,
-   const char *funcname,
-   size_t linenr)
+   ssize_t n)
 {
 *dest = NULL;
 if (!src)
 return 0;
 if (n < 0)
 n = strlen(src);
-if (!(*dest = strndup(src, n))) {
-if (report)
-virReportOOMErrorFull(domcode, filename, funcname, linenr);
-return -1;
-}
+if (!(*dest = strndup(src, n)))
+abort();
 
-   return 1;
+return 1;
 }
 
 
@@ -1483,10 +1436,8 @@ virStringEncodeBase64(const uint8_t *buf, size_t buflen)
 char *ret;
 
 base64_encode_alloc((const char *) buf, buflen, );
-if (!ret) {
-virReportOOMError();
-return NULL;
-}
+if (!ret)
+abort();
 
 return ret;
 }
diff --git 

[libvirt] [PATCH v2 0/4] util: abort when out of memory

[Daniel P . Berrangé]

This is the first part of a previously posted series:

  https://www.redhat.com/archives/libvir-list/2019-August/msg01374.html

I've temporarily split the addition of glib into a separate branch
until we get clarity on guarantees around g_malloc() and mallc()
using the same allocator. I published a docs update

 https://gitlab.gnome.org/GNOME/glib/merge_requests/1099/

to attempt to get confirmation from glib maintainers on this
matter.

Changed in v2:

 - Keep ATTRIBUTE_RETURN_CHECK annotations. We're aborting on OOM,
   but don't want to update all callers to drop return value
   checks yet
 - Update docs for virAsprintfQuiet too

Daniel P. Berrangé (4):
  util: purge all code for testing OOM handling
  util: make allocation functions abort on OOM
  util: remove several unused _QUIET allocation macro variants
  util: make string functions abort on OOM

 configure.ac  |  17 --
 docs/docs.html.in |   3 -
 docs/internals/oomtesting.html.in | 213 
 src/libvirt_private.syms  |   4 -
 src/util/viralloc.c   | 314 +-
 src/util/viralloc.h   | 192 --
 src/util/virstring.c  |  93 +++--
 src/util/virstring.h  |  73 +++
 tests/Makefile.am |   1 -
 tests/oomtrace.pl |  41 
 tests/qemuxml2argvtest.c  |  12 +-
 tests/testutils.c | 189 +-
 tests/testutils.h |   2 -
 tests/virfirewalltest.c   |  12 --
 14 files changed, 136 insertions(+), 1030 deletions(-)
 delete mode 100644 docs/internals/oomtesting.html.in
 delete mode 100755 tests/oomtrace.pl

-- 
2.21.0

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

Re: [libvirt] [PATCH 4/4] virt-result.m4: Colourize summary printings

[Daniel P . Berrangé]

On Mon, Sep 09, 2019 at 09:49:42AM +0200, Michal Privoznik wrote:
> The LIBVIRT_RESULT function takes two or three arguments. The
> first one is the name of the result (aka CHECK_NAME). It is
> printed before the colon character. The rest of the arguments is
> printed after the character. To produce colourized output a
> couple of changes needs to be made.
> 
> Firstly, we need to print the CHECK_NAME using "echo -n" so that
> the new line is not appended at the end of the message. To
> achieve this, AS_MESSAGE_N function is introduced. It's a
> verbatim copy of AS_MESSAGE (which is just another alias to
> AC_MSG_NOTICE) except it doesn't put '\n' at the EOL.
> 
> The alias is defined at /usr/share/autoconf-*/autoconf/general.m4
> and the AS_MESSAGE is then defined at
> /usr/share/autoconf-2.69/m4sugar/m4sh.m4.
> 
> Secondly, the rest of the arguments are printed colourized and to
> achieve that and also keep printing them into the log file the
> _AS_ECHO and COLORIZE_RESULT functions need to be called.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  m4/virt-result.m4 | 21 ++---
>  1 file changed, 18 insertions(+), 3 deletions(-)

Reviewed-by: Daniel P. Berrangé 


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

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

Re: [libvirt] [PATCH 3/4] configure: Colorize output

[Daniel P . Berrangé]

On Mon, Sep 09, 2019 at 09:49:41AM +0200, Michal Privoznik wrote:
> If we're running from a TTY we can put some colors around 'yes',
> 'no' and other messages.
> 
> Shamelessly copied from Ruby source code and modified a bit to
> comply with syntax-check.
> 
> https://github.com/ruby/ruby/commit/e4879592873abd4cd8aeed56f4cbaa360a3d3736

Ruby is under the Ruby license terms, which gives a choice of
using the 2-clause BSD license instead, which in turn lets
you relicense to the GPL, so ok.

> 
> Signed-off-by: Michal Privoznik 
> ---
>  m4/virt-colours.m4 | 62 ++
>  1 file changed, 62 insertions(+)
>  create mode 100644 m4/virt-colours.m4
> 
> diff --git a/m4/virt-colours.m4 b/m4/virt-colours.m4
> new file mode 100644
> index 00..251778f2ff
> --- /dev/null
> +++ b/m4/virt-colours.m4
> @@ -0,0 +1,62 @@
> +dnl Colourful configure
> +dnl
> +dnl Copyright (C) 2015 Nobuyoshi Nakada
> +dnl
> +dnl This library is free software; you can redistribute it and/or
> +dnl modify it under the terms of the GNU Lesser General Public
> +dnl License as published by the Free Software Foundation; either
> +dnl version 2.1 of the License, or (at your option) any later version.
> +dnl
> +dnl This library is distributed in the hope that it will be useful,
> +dnl but WITHOUT ANY WARRANTY; without even the implied warranty of
> +dnl MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +dnl Lesser General Public License for more details.
> +dnl
> +dnl You should have received a copy of the GNU Lesser General Public
> +dnl License along with this library.  If not, see
> +dnl .
> +
> +AC_DEFUN([_COLORIZE_RESULT_PREPARE], [
> +msg_checking= msg_result_yes= msg_result_no= msg_result_other= msg_reset=
> +AS_IF([test -t 1], [
> + msg_begin="`tput smso 2>/dev/null`"
> + AS_CASE(["$msg_begin"], ['@<:@'*m],
> + [msg_begin="`echo "$msg_begin" | sed ['s/[0-9]*m$//']`"
> + msg_checking="${msg_begin}33m"
> + AS_IF([test ${TEST_COLORS:+set}], [
> + msg_result_yes=[`expr ":$TEST_COLORS:" : ".*:pass=\([^:]*\):"`]
> + msg_result_no=[`expr ":$TEST_COLORS:" : ".*:fail=\([^:]*\):"`]
> + msg_result_other=[`expr ":$TEST_COLORS:" : 
> ".*:skip=\([^:]*\):"`]
> + ])
> + msg_result_yes="${msg_begin}${msg_result_yes:-32;1}m"
> + msg_result_no="${msg_begin}${msg_result_no:-31;1}m"
> + msg_result_other="${msg_begin}${msg_result_other:-33;1}m"
> + msg_reset="${msg_begin}m"
> + ])
> + AS_UNSET(msg_begin)
> + ])
> +AS_REQUIRE_SHELL_FN([colorize_result],
> + [AS_FUNCTION_DESCRIBE([colorize_result], [MSG], [Colorize result])],
> +[AS_CASE(["$[]1"],
> +[yes], [AS_ECHO(["${msg_result_yes}$[]1${msg_reset}]")],
> +[no], [AS_ECHO(["${msg_result_no}$[]1${msg_reset}]")],
> +[AS_ECHO(["${msg_result_other}$[]1${msg_reset}]")])])
> +])
> +
> +AC_DEFUN([COLORIZE_RESULT], [AC_REQUIRE([_COLORIZE_RESULT_PREPARE])dnl
> +AS_LITERAL_IF([$1],
> + [m4_case([$1],
> + [yes], [AS_ECHO(["${msg_result_yes}$1${msg_reset}"])],
> + [no], [AS_ECHO(["${msg_result_no}$1${msg_reset}"])],
> + [AS_ECHO(["${msg_result_other}$1${msg_reset}"])])],
> + [colorize_result "$1"]) dnl
> +])
> +
> +AC_DEFUN([AC_CHECKING],[dnl
> +AC_REQUIRE([_COLORIZE_RESULT_PREPARE])dnl
> +AS_MESSAGE([checking ${msg_checking}$1${msg_reset}...])])
> +
> +AC_DEFUN([AC_MSG_RESULT], [dnl
> +{ _AS_ECHO_LOG([result: $1])
> +COLORIZE_RESULT([$1]); dnl
> +}])

I can't claim I attempted to understand that. I'm just assuming
that since it works for Ruby, its bug-free :-)  Clever how they
redefine existing autoconf macros AC_CHECKING and AC_MSG_RESULT
to make them colourful.

Reviewed-by: Daniel P. Berrangé 


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

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

Re: [libvirt] [PATCH 2/4] virt-result.m4: Align string more generously

[Daniel P . Berrangé]

On Thu, Sep 12, 2019 at 12:18:10PM +0200, Michal Privoznik wrote:
> On 9/12/19 12:30 AM, Cole Robinson wrote:
> > On 9/9/19 3:49 AM, Michal Privoznik wrote:
> > > The times, when we had small CRTs are long gone. Now, in the era
> > > of wide screens we can be more generous when it comes to aligning
> > > the output of configure. The longest string before the colon is
> > > 'wireshark_dissector' which counts 19 characters.  Therefore,
> > > align the strings at 20.
> > > 
> > > At the same time, drop the useless result alignment. It behaves
> > > oddly - it puts a space at the end of each "no" because of the
> > > %-3s format we use.
> > > 
> > > Signed-off-by: Michal Privoznik 
> > > ---
> > >   m4/virt-result.m4 | 4 ++--
> > >   1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/m4/virt-result.m4 b/m4/virt-result.m4
> > > index cc622fe35b..36973ba0b5 100644
> > > --- a/m4/virt-result.m4
> > > +++ b/m4/virt-result.m4
> > > @@ -33,9 +33,9 @@ dnl  LIBVIRT_RESULT([yajl], [yes], [-I/opt/yajl/include 
> > > -lyajl])
> > >   dnl
> > >   AC_DEFUN([LIBVIRT_RESULT], [
> > > if test "$2" = "no" || test -z "$3" ; then
> > > -STR=`printf "%10s: %-3s" "$1" "$2"`
> > > +STR=`printf "%20s: %s" "$1" "$2"`
> > > else
> > > -STR=`printf "%10s: %-3s (%s)" "$1" "$2" "$3"`
> > > +STR=`printf "%20s: %s (%s)" "$1" "$2" "$3"`
> > > fi
> > > AC_MSG_NOTICE([$STR])
> > > 
> > 
> > For the first 2:
> > 
> > Reviewed-by: Cole Robinson 
> > 
> > I like the look of the colors and I agree it speeds up visually scanning
> > the configure output. But I'm neutral on whether adding more m4 to the
> > build system to facilitate it is worth it. So I'll abstain from giving
> > ack or nack on those.
> 
> Fair enough. When we switch to meson we'll get colours for free.

FWIW, I've no objection to people continuing to make autotools
usage better. In it unreasonable to block patches people send,
until the meson patches are actually complete & published for
review. I merely caution that any investment has a time limited
window for payback, but since you've already done the work that's
not a problem.

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

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

Re: [libvirt] [PATCH 2/4] virt-result.m4: Align string more generously

[Daniel P . Berrangé]

On Mon, Sep 09, 2019 at 09:49:40AM +0200, Michal Privoznik wrote:
> The times, when we had small CRTs are long gone. Now, in the era
> of wide screens we can be more generous when it comes to aligning
> the output of configure. The longest string before the colon is
> 'wireshark_dissector' which counts 19 characters.  Therefore,
> align the strings at 20.
> 
> At the same time, drop the useless result alignment. It behaves
> oddly - it puts a space at the end of each "no" because of the
> %-3s format we use.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  m4/virt-result.m4 | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Daniel P. Berrangé 


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

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

Re: [libvirt] [PATCH 1/4] configure: Prefer LIBVIRT_RESULT over AC_MSG_NOTICE

[Daniel P . Berrangé]

On Mon, Sep 09, 2019 at 09:49:39AM +0200, Michal Privoznik wrote:
> One of the advantages is that LIBVIRT_RESULT aligns the resulting
> message for us. The other is that in near future we will colour
> some parts of the message and thus it helps if we get arguments
> split in two.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  configure.ac | 8 
>  m4/virt-chrdev-lock-files.m4 | 2 +-
>  m4/virt-debug.m4 | 2 +-
>  m4/virt-default-editor.m4| 2 +-
>  m4/virt-dtrace.m4| 2 +-
>  m4/virt-host-validate.m4 | 2 +-
>  m4/virt-init-script.m4   | 2 +-
>  m4/virt-loader-nvram.m4  | 2 +-
>  m4/virt-login-shell.m4   | 2 +-
>  m4/virt-numad.m4 | 2 +-
>  10 files changed, 13 insertions(+), 13 deletions(-)

Reviewed-by: Daniel P. Berrangé 


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

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

Re: [libvirt] [PATCH 2/4] virt-result.m4: Align string more generously

[Michal Privoznik]

On 9/12/19 12:33 PM, Daniel P. Berrangé wrote:

On Thu, Sep 12, 2019 at 12:18:10PM +0200, Michal Privoznik wrote:

On 9/12/19 12:30 AM, Cole Robinson wrote:

On 9/9/19 3:49 AM, Michal Privoznik wrote:

The times, when we had small CRTs are long gone. Now, in the era
of wide screens we can be more generous when it comes to aligning
the output of configure. The longest string before the colon is
'wireshark_dissector' which counts 19 characters.  Therefore,
align the strings at 20.

At the same time, drop the useless result alignment. It behaves
oddly - it puts a space at the end of each "no" because of the
%-3s format we use.

Signed-off-by: Michal Privoznik 
---
   m4/virt-result.m4 | 4 ++--
   1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/m4/virt-result.m4 b/m4/virt-result.m4
index cc622fe35b..36973ba0b5 100644
--- a/m4/virt-result.m4
+++ b/m4/virt-result.m4
@@ -33,9 +33,9 @@ dnl  LIBVIRT_RESULT([yajl], [yes], [-I/opt/yajl/include 
-lyajl])
   dnl
   AC_DEFUN([LIBVIRT_RESULT], [
 if test "$2" = "no" || test -z "$3" ; then
-STR=`printf "%10s: %-3s" "$1" "$2"`
+STR=`printf "%20s: %s" "$1" "$2"`
 else
-STR=`printf "%10s: %-3s (%s)" "$1" "$2" "$3"`
+STR=`printf "%20s: %s (%s)" "$1" "$2" "$3"`
 fi
 AC_MSG_NOTICE([$STR])



For the first 2:

Reviewed-by: Cole Robinson 

I like the look of the colors and I agree it speeds up visually scanning
the configure output. But I'm neutral on whether adding more m4 to the
build system to facilitate it is worth it. So I'll abstain from giving
ack or nack on those.


Fair enough. When we switch to meson we'll get colours for free.


I've not checked how meson handles it, but in Travis with this series
applied, the raw logs are now full of escape codes:

   https://api.travis-ci.org/v3/job/584045926/log.txt

   checking for library containing forkpty... -lutil
   checking whether strerror(0) succeeds... yes
   checking for strerror_r with POSIX signature... no
   checking whether __xpg_strerror_r works... yes
   checking whether strerror_r is declared... yes
   checking for external symbol _system_configuration... no
   checking for pthread_t... yes
   checking for pthread_spinlock_t... yes
   checking for PTHREAD_CREATE_DETACHED... yes
   checking for PTHREAD_MUTEX_RECURSIVE... yes

which makes the raw log much less readable.

This is also the case already for the automake 'make check' output
in fact >
PASS: test-fnmatch-h
PASS: test-fnmatch
PASS: test-fpurge
PASS: test-fputc
PASS: test-fread
PASS: test-freading
PASS: test-fseek2.sh


On the flip side, the default log view honours the colors which is
nice:

   https://travis-ci.org/berrange/libvirt/jobs/584045926



This is a travis bug. I've noticed this when doing my own builds and the 
problem is that somehow, travis reads TTY output instead of doing 
something like ./configure | tee. Travis runs ./configure from a TTY and 
thus my code (and obviously 'make check' too) sees FD 1 opened and being 
a TTY (`test -t 1') and thus colours are enabled. Fortunately, the 
default log view is not affected.


Michal

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

Re: [libvirt] [PATCH] maint: Use pep8 to implement sc_prohibit_semicolon_at_eol_in_python

[Daniel P . Berrangé]

On Thu, Sep 12, 2019 at 12:56:04PM +0200, Andrea Bolognani wrote:
> On Thu, 2019-09-12 at 11:13 +0100, Daniel P. Berrangé wrote:
> > On Thu, Sep 12, 2019 at 05:55:38PM +0800, Shi Lei wrote:
> > > +AC_PATH_PROG([PEP8], [pep8])
> > > +if test -z "$PEP8"; then
> > > +AC_MSG_ERROR(['pep8' binary is required to check python code style])
> > > +fi
> > 
> > Using pep8 is an interesting idea. Especially with my series to
> > standardize on using python for all build scripts, it will be
> > valuable to have much more advanced python style checks.
> > 
> > The only thing I wonder about is whether its reasonable to make
> > it a mandatory requirement or not, since it is a separate package
> > from python itself we can't assume it is present I think. It is
> > on the various Linux we care about and FreeBSD too, but I'm not
> > seeing it for macOS via homebrew.
> > 
> > Also on my host 'pep8' is a python2 impl, you need 'pep8-3' for
> > the python3 impl. Except that when I run it, it warns that
> > it is renamed to pycodestyle upstream and 'pep8' will be dropped
> > in future.
> > 
> > IOW, I think we'll need to check for existence of the first available
> > bniary from the list in this order:
> > 
> >   pycodestyle-3 pycodestyle pycodestyle-2 pep8-3 pep8 pep8-2
> 
> FWIW, libvirt-dbus is using flake8 to achieve what I believe is
> basically the same result, whereas virt-manager I think uses pylint
> and pycodestlye.
> 
> I am not familiar enough with the Python ecosystem to be able to
> compare the various linters, but it would IMHO make sense to at
> least try to standardize on one or more of them and use them across
> libvirt-related projects.

pep8 validates code style against published PEP style guidelines.

pyflakes does static analysis to detect code errors

flake8 is a wrapper that runs pep8 and pyflakes and does some
other stuff.

For just doing this semicolon check then pep8 is sufficient,
but for a more general approach, then flake8 makes more
sense. In that case we'd delete sc_prohibit_semicolon_at_eol_in_python
entirely, and simply have a generic 'sc_flake8' check that runs a
configured list of checks against all py code.

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

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

Re: [libvirt] [PATCH] maint: Use pep8 to implement sc_prohibit_semicolon_at_eol_in_python

[Andrea Bolognani]

On Thu, 2019-09-12 at 11:13 +0100, Daniel P. Berrangé wrote:
> On Thu, Sep 12, 2019 at 05:55:38PM +0800, Shi Lei wrote:
> > +AC_PATH_PROG([PEP8], [pep8])
> > +if test -z "$PEP8"; then
> > +AC_MSG_ERROR(['pep8' binary is required to check python code style])
> > +fi
> 
> Using pep8 is an interesting idea. Especially with my series to
> standardize on using python for all build scripts, it will be
> valuable to have much more advanced python style checks.
> 
> The only thing I wonder about is whether its reasonable to make
> it a mandatory requirement or not, since it is a separate package
> from python itself we can't assume it is present I think. It is
> on the various Linux we care about and FreeBSD too, but I'm not
> seeing it for macOS via homebrew.
> 
> Also on my host 'pep8' is a python2 impl, you need 'pep8-3' for
> the python3 impl. Except that when I run it, it warns that
> it is renamed to pycodestyle upstream and 'pep8' will be dropped
> in future.
> 
> IOW, I think we'll need to check for existence of the first available
> bniary from the list in this order:
> 
>   pycodestyle-3 pycodestyle pycodestyle-2 pep8-3 pep8 pep8-2

FWIW, libvirt-dbus is using flake8 to achieve what I believe is
basically the same result, whereas virt-manager I think uses pylint
and pycodestlye.

I am not familiar enough with the Python ecosystem to be able to
compare the various linters, but it would IMHO make sense to at
least try to standardize on one or more of them and use them across
libvirt-related projects.

CC'ing Cole and Pavel, who as maintainers of the biggest chunk of
Python code in the entire stack can probably offer some useful
insights.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH 2/7] util: make allocation functions abort on OOM

[Daniel P . Berrangé]

On Thu, Sep 05, 2019 at 06:03:57PM -0400, John Ferlan wrote:
> 
> 
> On 8/29/19 2:02 PM, Daniel P. Berrangé wrote:
> > The functions are left returning an "int" to avoid an immediate
> > big-bang cleanup. They'll simply never return anything other
> > than 0, except for virInsertN which can still return an error
> > if the requested insertion index is out of range. Interestingly
> > in that case, the _QUIET function would none the less report
> > an error.
> > 
> > Signed-off-by: Daniel P. Berrangé 
> > ---
> >  src/util/viralloc.c | 201 +++-
> >  src/util/viralloc.h | 145 +++-
> >  2 files changed, 97 insertions(+), 249 deletions(-)
> > 
> 
> 
> FWIW: I applied the series to my Coverity branch to see what (if
> anything) gets shaken there.  Good news is - not much. There were a
> couple of things I already reported on that were fixed for 5.7.0 (commit
> ed7e342b0 and be9d259eb).
> 
> There's one more false positive in qemuDomainGetNumaParameters that
> probably would be fixed by AUTOFREE stuff (nodeset gets set to NULL
> after virTypedParameterAssign).
> 
> Beyond that the only thing is two CHECKED_RETURN's (e.g. N callers check
> the return value, but these 2 don't).  I consider both false positives;
> however, it could also be argued that unless any of the functions where
> either 0 or abort occurs, then make them just void, but that's a sh*load
> more work...
> 
> [...]
> 
> >  /**
> > @@ -204,50 +143,35 @@ int virExpandN(void *ptrptr,
> >   * @allocptr: pointer to number of elements allocated in array
> >   * @count: number of elements currently used in array
> >   * @add: minimum number of additional elements to support in array
> > - * @report: whether to report OOM error, if there is one
> > - * @domcode: error domain code
> > - * @filename: caller's filename
> > - * @funcname: caller's funcname
> > - * @linenr: caller's line number
> >   *
> >   * If 'count' + 'add' is larger than '*allocptr', then resize the
> >   * block of memory in 'ptrptr' to be an array of at least 'count' +
> >   * 'add' elements, each 'size' bytes in length. Update 'ptrptr' and
> >   * 'allocptr' with the details of the newly allocated memory. On
> >   * failure, 'ptrptr' and 'allocptr' are not changed. Any newly
> > - * allocated memory in 'ptrptr' is zero-filled. If @report is true,
> > - * OOM errors are reported automatically.
> > - *
> > + * allocated memory in 'ptrptr' is zero-filled.
> >   *
> > - * Returns -1 on failure to allocate, zero on success
> > + * Returns zero on success, aborts on OOM
> >   */
> >  int virResizeN(void *ptrptr,
> > size_t size,
> > size_t *allocptr,
> > size_t count,
> > -   size_t add,
> > -   bool report,
> > -   int domcode,
> > -   const char *filename,
> > -   const char *funcname,
> > -   size_t linenr)
> > +   size_t add)
> >  {
> >  size_t delta;
> >  
> > -if (count + add < count) {
> > -if (report)
> > -virReportOOMErrorFull(domcode, filename, funcname, linenr);
> > -errno = ENOMEM;
> > -return -1;
> > -}
> > +if (count + add < count)
> > +abort();
> > +
> >  if (count + add <= *allocptr)
> >  return 0;
> >  
> >  delta = count + add - *allocptr;
> >  if (delta < *allocptr / 2)
> >  delta = *allocptr / 2;
> > -return virExpandN(ptrptr, size, allocptr, delta, report,
> > -  domcode, filename, funcname, linenr);
> > +virExpandN(ptrptr, size, allocptr, delta);
> > +return 0;
> 
> Could just return virExpandN here...
> 
> >  }
> 
> [...]
> 
> > @@ -312,12 +229,7 @@ int
> >  virInsertElementsN(void *ptrptr, size_t size, size_t at,
> > size_t *countptr,
> > size_t add, void *newelems,
> > -   bool clearOriginal, bool inPlace,
> > -   bool report,
> > -   int domcode,
> > -   const char *filename,
> > -   const char *funcname,
> > -   size_t linenr)
> > +   bool clearOriginal, bool inPlace)
> >  {
> >  if (at == -1) {
> >  at = *countptr;
> > @@ -330,9 +242,8 @@ virInsertElementsN(void *ptrptr, size_t size, size_t at,
> >  
> >  if (inPlace) {
> >  *countptr += add;
> > -} else if (virExpandN(ptrptr, size, countptr, add, report,
> > -  domcode, filename, funcname, linenr) < 0) {
> > -return -1;
> > +} else {
> > +virExpandN(ptrptr, size, countptr, add);
> >  }
> >  
> 
> This one's more painful, with using ignore_value() wrapper to just
> pacify Coverity.
> 
> [...]
> 
> I'm not saying anything has to be done, but I figured it could be a
> useful data point for you -

In this patch I removed the ATTRIBUTE_RETURN_CHECK annotation, but I'm
going to 

Re: [libvirt] [PATCH 2/4] virt-result.m4: Align string more generously

[Daniel P . Berrangé]

On Thu, Sep 12, 2019 at 12:18:10PM +0200, Michal Privoznik wrote:
> On 9/12/19 12:30 AM, Cole Robinson wrote:
> > On 9/9/19 3:49 AM, Michal Privoznik wrote:
> > > The times, when we had small CRTs are long gone. Now, in the era
> > > of wide screens we can be more generous when it comes to aligning
> > > the output of configure. The longest string before the colon is
> > > 'wireshark_dissector' which counts 19 characters.  Therefore,
> > > align the strings at 20.
> > > 
> > > At the same time, drop the useless result alignment. It behaves
> > > oddly - it puts a space at the end of each "no" because of the
> > > %-3s format we use.
> > > 
> > > Signed-off-by: Michal Privoznik 
> > > ---
> > >   m4/virt-result.m4 | 4 ++--
> > >   1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/m4/virt-result.m4 b/m4/virt-result.m4
> > > index cc622fe35b..36973ba0b5 100644
> > > --- a/m4/virt-result.m4
> > > +++ b/m4/virt-result.m4
> > > @@ -33,9 +33,9 @@ dnl  LIBVIRT_RESULT([yajl], [yes], [-I/opt/yajl/include 
> > > -lyajl])
> > >   dnl
> > >   AC_DEFUN([LIBVIRT_RESULT], [
> > > if test "$2" = "no" || test -z "$3" ; then
> > > -STR=`printf "%10s: %-3s" "$1" "$2"`
> > > +STR=`printf "%20s: %s" "$1" "$2"`
> > > else
> > > -STR=`printf "%10s: %-3s (%s)" "$1" "$2" "$3"`
> > > +STR=`printf "%20s: %s (%s)" "$1" "$2" "$3"`
> > > fi
> > > AC_MSG_NOTICE([$STR])
> > > 
> > 
> > For the first 2:
> > 
> > Reviewed-by: Cole Robinson 
> > 
> > I like the look of the colors and I agree it speeds up visually scanning
> > the configure output. But I'm neutral on whether adding more m4 to the
> > build system to facilitate it is worth it. So I'll abstain from giving
> > ack or nack on those.
> 
> Fair enough. When we switch to meson we'll get colours for free.

I've not checked how meson handles it, but in Travis with this series
applied, the raw logs are now full of escape codes:

  https://api.travis-ci.org/v3/job/584045926/log.txt

  checking for library containing forkpty... -lutil
  checking whether strerror(0) succeeds... yes
  checking for strerror_r with POSIX signature... no
  checking whether __xpg_strerror_r works... yes
  checking whether strerror_r is declared... yes
  checking for external symbol _system_configuration... no
  checking for pthread_t... yes
  checking for pthread_spinlock_t... yes
  checking for PTHREAD_CREATE_DETACHED... yes
  checking for PTHREAD_MUTEX_RECURSIVE... yes

which makes the raw log much less readable.

This is also the case already for the automake 'make check' output
in fact

PASS: test-fnmatch-h
PASS: test-fnmatch
PASS: test-fpurge
PASS: test-fputc
PASS: test-fread
PASS: test-freading
PASS: test-fseek2.sh


On the flip side, the default log view honours the colors which is
nice:

  https://travis-ci.org/berrange/libvirt/jobs/584045926


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

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

Re: [libvirt] [PATCH 2/4] virt-result.m4: Align string more generously

[Michal Privoznik]

On 9/12/19 12:30 AM, Cole Robinson wrote:

On 9/9/19 3:49 AM, Michal Privoznik wrote:

The times, when we had small CRTs are long gone. Now, in the era
of wide screens we can be more generous when it comes to aligning
the output of configure. The longest string before the colon is
'wireshark_dissector' which counts 19 characters.  Therefore,
align the strings at 20.

At the same time, drop the useless result alignment. It behaves
oddly - it puts a space at the end of each "no" because of the
%-3s format we use.

Signed-off-by: Michal Privoznik 
---
  m4/virt-result.m4 | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/m4/virt-result.m4 b/m4/virt-result.m4
index cc622fe35b..36973ba0b5 100644
--- a/m4/virt-result.m4
+++ b/m4/virt-result.m4
@@ -33,9 +33,9 @@ dnl  LIBVIRT_RESULT([yajl], [yes], [-I/opt/yajl/include 
-lyajl])
  dnl
  AC_DEFUN([LIBVIRT_RESULT], [
if test "$2" = "no" || test -z "$3" ; then
-STR=`printf "%10s: %-3s" "$1" "$2"`
+STR=`printf "%20s: %s" "$1" "$2"`
else
-STR=`printf "%10s: %-3s (%s)" "$1" "$2" "$3"`
+STR=`printf "%20s: %s (%s)" "$1" "$2" "$3"`
fi
  
AC_MSG_NOTICE([$STR])




For the first 2:

Reviewed-by: Cole Robinson 

I like the look of the colors and I agree it speeds up visually scanning
the configure output. But I'm neutral on whether adding more m4 to the
build system to facilitate it is worth it. So I'll abstain from giving
ack or nack on those.


Fair enough. When we switch to meson we'll get colours for free.



If you push the first two independently you may want to strip mention of
colour from their commit messages


Yep, that was my plan. The first two patches make sense even without the 
rest. I'll push them shortly, thanks.


Michal

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

Re: [libvirt] [PATCH] maint: Use pep8 to implement sc_prohibit_semicolon_at_eol_in_python

[Daniel P . Berrangé]

On Thu, Sep 12, 2019 at 05:55:38PM +0800, Shi Lei wrote:
> Now sc_prohibit_semicolon_at_eol_in_python can't handle semicolon
> within multiline strings(comments) properly.
> 
> I suggest that we could use pep8 to check python code style error, such
> as 'statement ends with a semicolon'. In future, we could use '--select'
> to introduce other rules.
> 
> Signed-off-by: Shi Lei 
> ---
>  cfg.mk   | 6 ++
>  configure.ac | 4 
>  2 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/cfg.mk b/cfg.mk
> index 42e1abf0..c8eaf74e 100644
> --- a/cfg.mk
> +++ b/cfg.mk
> @@ -813,10 +813,8 @@ sc_require_enum_last_marker:
>  
>  # In Python files we don't want to end lines with a semicolon like in C
>  sc_prohibit_semicolon_at_eol_in_python:
> - @prohibit='^[^#].*\;$$' \
> - in_vc_files='\.py$$' \
> - halt='python does not require to end lines with a semicolon' \
> -   $(_sc_search_regexp)
> + @$(VC_LIST_EXCEPT) | $(GREP) '\.py$$' | xargs $(PEP8) --select E703 \
> + | $(GREP) . && { exit 1; } || :
>  
>  # mymain() in test files should use return, not exit, for nicer output
>  sc_prohibit_exit_in_tests:
> diff --git a/configure.ac b/configure.ac
> index bf9e7681..37fa9924 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -704,6 +704,10 @@ AC_PATH_PROGS([PYTHON], [python3 python2 python])
>  if test -z "$PYTHON"; then
>  AC_MSG_ERROR(['python3', 'python2' or 'python' binary is required to 
> build libvirt])
>  fi
> +AC_PATH_PROG([PEP8], [pep8])
> +if test -z "$PEP8"; then
> +AC_MSG_ERROR(['pep8' binary is required to check python code style])
> +fi
>  AC_PATH_PROG([PERL], [perl])
>  if test -z "$PERL"; then
>   AC_MSG_ERROR(['perl' binary is required to build libvirt])

Using pep8 is an interesting idea. Especially with my series to
standardize on using python for all build scripts, it will be
valuable to have much more advanced python style checks.

The only thing I wonder about is whether its reasonable to make
it a mandatory requirement or not, since it is a separate package
from python itself we can't assume it is present I think. It is
on the various Linux we care about and FreeBSD too, but I'm not
seeing it for macOS via homebrew.

Also on my host 'pep8' is a python2 impl, you need 'pep8-3' for
the python3 impl. Except that when I run it, it warns that
it is renamed to pycodestyle upstream and 'pep8' will be dropped
in future.

IOW, I think we'll need to check for existence of the first available
bniary from the list in this order:

  pycodestyle-3 pycodestyle pycodestyle-2 pep8-3 pep8 pep8-2


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

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

Re: [libvirt] [dockerfiles PATCH] Add 'trigger' and 'monitor' scripts

[Andrea Bolognani]

On Wed, 2019-09-11 at 18:38 +0200, Fabiano Fidêncio wrote:
> On Wed, Sep 11, 2019 at 3:12 PM Andrea Bolognani  wrote:
> > These are extremely crude scripts that are nonetheless useful when
> > it's time to rebuild all images. Usage is along these lines:
> > 
> >   $ ls libvirt-debian-10* >in
> >   $ ./trigger in out
> >   $ ./monitor out 3
> > 
> > Error handling is almost non-existent, but realistically speaking
> > at most three people will ever run these scripts anyway :)
> > 
> > Quay has limits on both the number of jobs that can be running at
> > the same time and the rate of job submission: rebuilding containers
> > in batches, eg. all Debian 10 containers, followed by all Debian 9
> > containers, and so on, allows us to remain within those limits.
> > 
> > Signed-off-by: Andrea Bolognani 
> 
> Reviewed-by: Fabiano Fidêncio 

Thanks, pushed now.

I actually got thinking whether these scripts, as well as the
existing 'refresh' script, really belong to this repo rather than
to the libvirt-jenkins-ci repo... But figuring that out, and if
necessary acting on it, is a task for another day :)

-- 
Andrea Bolognani / Red Hat / Virtualization

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

[libvirt] [PATCH] maint: Use pep8 to implement sc_prohibit_semicolon_at_eol_in_python

[Shi Lei]

Now sc_prohibit_semicolon_at_eol_in_python can't handle semicolon
within multiline strings(comments) properly.

I suggest that we could use pep8 to check python code style error, such
as 'statement ends with a semicolon'. In future, we could use '--select'
to introduce other rules.

Signed-off-by: Shi Lei 
---
 cfg.mk   | 6 ++
 configure.ac | 4 
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/cfg.mk b/cfg.mk
index 42e1abf0..c8eaf74e 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -813,10 +813,8 @@ sc_require_enum_last_marker:
 
 # In Python files we don't want to end lines with a semicolon like in C
 sc_prohibit_semicolon_at_eol_in_python:
-   @prohibit='^[^#].*\;$$' \
-   in_vc_files='\.py$$' \
-   halt='python does not require to end lines with a semicolon' \
- $(_sc_search_regexp)
+   @$(VC_LIST_EXCEPT) | $(GREP) '\.py$$' | xargs $(PEP8) --select E703 \
+   | $(GREP) . && { exit 1; } || :
 
 # mymain() in test files should use return, not exit, for nicer output
 sc_prohibit_exit_in_tests:
diff --git a/configure.ac b/configure.ac
index bf9e7681..37fa9924 100644
--- a/configure.ac
+++ b/configure.ac
@@ -704,6 +704,10 @@ AC_PATH_PROGS([PYTHON], [python3 python2 python])
 if test -z "$PYTHON"; then
 AC_MSG_ERROR(['python3', 'python2' or 'python' binary is required to build 
libvirt])
 fi
+AC_PATH_PROG([PEP8], [pep8])
+if test -z "$PEP8"; then
+AC_MSG_ERROR(['pep8' binary is required to check python code style])
+fi
 AC_PATH_PROG([PERL], [perl])
 if test -z "$PERL"; then
  AC_MSG_ERROR(['perl' binary is required to build libvirt])
-- 
2.17.1


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

Re: [libvirt] [PATCH v2 03/24] build: force a UTF-8 locale for python

[Daniel P . Berrangé]

On Wed, Sep 11, 2019 at 12:27:45PM -0500, Eric Blake wrote:
> On 9/11/19 11:23 AM, Daniel P. Berrangé wrote:
> > Python3 versions less than 3.7 have very unhelpful handling
> > of the C locale where they assume data is 7-bit only. This
> > violates POSIX which requires the C locale to be 8-bit clean.
> > Python3 >= 3.7 now assumes that the C locale is always UTF-8.
> 
> Being UTF-8 vs. being 8-bit clean are not necessarily synonymous, but
> the difference shouldn't matter when we only use UTF-8 encoding in our
> script source code and inputs.
> 
> > 
> > Set env variables to force LC_CTYPE to en_US.UTF-8 so that
> > we get UTF-8 handling on all python versions. Note we do
> > not use C.UTF-8 since not all C libraries support that.
> > 
> > Signed-off-by: Daniel P. Berrangé 
> > ---
> >  Makefile.am| 2 +-
> >  configure.ac   | 8 
> >  docs/Makefile.am   | 3 ++-
> >  src/esx/Makefile.inc.am| 2 +-
> >  src/hyperv/Makefile.inc.am | 2 +-
> >  src/util/Makefile.inc.am   | 8 
> >  6 files changed, 17 insertions(+), 8 deletions(-)
> 
> Reviewed-by: Eric Blake 
> 
> 
> > +++ b/src/esx/Makefile.inc.am
> > @@ -63,7 +63,7 @@ $(ESX_DRIVER_GENERATED): $(ESX_GENERATED_STAMP)
> >  
> >  $(ESX_GENERATED_STAMP): $(srcdir)/esx/esx_vi_generator.input \
> >   $(srcdir)/esx/esx_vi_generator.py
> > -   $(AM_V_GEN)srcdir=$(srcdir) $(PYTHON) $(srcdir)/esx/esx_vi_generator.py 
> > \
> > +   $(AM_V_GEN)srcdir=$(srcdir) $(RUNUTF8) $(PYTHON) 
> > $(srcdir)/esx/esx_vi_generator.py \
> >   && touch $@
> 
> Worth rewrapping long lines any differently, since your addition pushes
> this beyond 80 columns?

Ohh, bug-tastic. make syntax-check validates long lines, but we filter
for Makefile.am, so we've been silently missing out Makefile.in.am
files.


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

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

Re: [libvirt] [PATCH v1] docs: Expand the "BIOS bootloader" documentation for domainCaps

[Kashyap Chamarthy]

On Wed, Sep 11, 2019 at 05:40:02PM +0200, Michal Privoznik wrote:
> On 9/11/19 4:34 PM, Kashyap Chamarthy wrote:

[...]

> > diff --git a/docs/formatdomaincaps.html.in b/docs/formatdomaincaps.html.in
> > index 
> > bc99d378567a553afe682bc522e7a753b2d805fc..a8d970934df2c0ce8c41eb4958c94fbdf96ef8e0
> >  100644
> > --- a/docs/formatdomaincaps.html.in
> > +++ b/docs/formatdomaincaps.html.in
> > @@ -127,7 +127,7 @@
> > value/usr/share/OVMF/OVMF_CODE.fd/value
> > enum name='type'
> >   valuerom/value
> > -valuepflash/value
> > +valuepflapsh/value
> 
> This looks like a unintended change.

Oops, indeed; sorry.

[...]

> > +  firmware attribute of the os element in
> > +  the domain XML. The presence of this enum means libvirt is capable
> > +  of the so-called firmware auto-selection feature. And the listed
> > +  firmware values represent the accepted input in the domain
> > +  XML. Note that the firmware enum reports only those
> > +  values for which a firmware "descriptor file" exists on the host
> > +  -- a small JSON document that describes details about a given UEFI
> > +  binary on the host, e.g. the fimware binary path, its
> 
> FW descriptors can describe a BIOS image too.

Yes; missed to be careful here.

> > +  architecture, supported machine type, NVRAM template, etc. This
> > +  ensures that the reported values won't cause a failure on guest
> > +  boot.
> 
> 
> >  (The firmware "descriptor files" are typically shipped
> > +  Linux distribution as part of the firmware package,
> > +  e.g. EDK2/OVMF.)
> 
> This is not exactly true. FW descriptors are shipped by qemu actaully. 

Yes and no.

QEMU upstream ships them in the pc-bios/descriptors.

However, most major distributions Debian, Ubuntu and Fedora[*] ship them
as part of edk2/edk2-ovmf packages.  Because (quoting from the commit
message from here[*]):

[quote]

  - Distributions providing their own EDK2 packages would not include
the descriptors from upstream QEMU, even if they otherwise package
QEMU.  That's beause the descriptor files in QEMU match the firmware
bundled with QEMU -- but the firmware images in the distros' own
EDK2 packages are different.  So, if a distro provides an EDK2
package, then the same EDK2 package should offer matching
descriptors.  QEMU offers descriptors (soon) because QEMU
technically distributes edk2 firmware binaries (soon).  [Where
"soon" == QEMU 4.1] 

  - In Fedora, we need to ship them [the "descriptor files"] as part of
the EDK2 package, because Fedora throws away all the firmware files
that QEMU bundles, because we're [Fedora] required to rebuild
everything from pristine source.

[/quote]


[*] https://src.fedoraproject.org/rpms/edk2/c/674b3c8a27a8

> But also, I don't think users need to bother - their distro will
> install it when updating qemu package.

Yeah.  I just wanted to add a hint for those wondering "where do I find
these files".

> ACK to the rest and pushed. Thanks for taking care of this.

Thanks for the quick review and merge.

-- 
/kashyap

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

[libvirt] [PATCH] fix memleak in virNodeDeviceGetPCISRIOVCaps

[Yi Wang]

From: Jiang Kun 

it always alloc new memory when get dumpxml of pf device,but never free it.
Now free the old first,then alloc new memory.

Signed-off-by: Jiang kun 
---
 src/conf/node_device_conf.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
index e51371d..618ce8e 100644
--- a/src/conf/node_device_conf.c
+++ b/src/conf/node_device_conf.c
@@ -2509,6 +2509,7 @@ virNodeDeviceGetPCISRIOVCaps(const char *sysfsPath,
 for (i = 0; i < pci_dev->num_virtual_functions; i++)
VIR_FREE(pci_dev->virtual_functions[i]);
 VIR_FREE(pci_dev->virtual_functions);
+VIR_FREE(pci_dev->physical_function);
 pci_dev->num_virtual_functions = 0;
 pci_dev->max_virtual_functions = 0;
 pci_dev->flags &= ~VIR_NODE_DEV_CAP_FLAG_PCI_VIRTUAL_FUNCTION;
-- 
1.8.3.1

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