[libvirt] [libvirt-tck] [PATCH] Add new option for virt-builder

[Dan Zheng]

>From 0cbe381e782a18cfa9730e8791a80ff01497d8fc Mon Sep 17 00:00:00 2001
From: Dan Zheng 
Date: Tue, 16 Jan 2018 13:39:50 +0800
Subject: [PATCH] Add new option for virt-builder

Using --selinux-relabel is required by Fedora and RHEL guests to ensure
SELinux labels correctly in the disk image.

Signed-off-by: Dan Zheng 
---
 lib/Sys/Virt/TCK.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/Sys/Virt/TCK.pm b/lib/Sys/Virt/TCK.pm
index e9da8d2..1621342 100644
--- a/lib/Sys/Virt/TCK.pm
+++ b/lib/Sys/Virt/TCK.pm
@@ -405,7 +405,7 @@ sub create_virt_builder_disk {
 }
 
 print "# running virt-builder $osname\n";
-system "virt-builder", "--install", "dsniff", "--root-password", 
"password:$password", "--output", $target, $osname;
+system "virt-builder", "--install", "dsniff", "--root-password", 
"password:$password", "--output", $target, $osname, "--selinux-relabel";
 
 die "cannot run virt-builder: $?" if $? != 0;
 
-- 
1.8.3.1


-- 
Best Regards,
Dan Zheng(郑丹)
Red Hat Software (Beijing) Co.
9/F, North Tower C, Raycom Infotech Park
No.2 Ke Xueyuan Nanlu, Haidian District Beijing 100190

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

[libvirt] [PATCH] check/add for mdev_types capability

[Dan Zheng]

>From be62ea5394d52fa168079edc3aa7e558363a3026 Mon Sep 17 00:00:00 2001
From: Dan Zheng 
Date: Tue, 16 Jan 2018 12:21:21 +0800
Subject: [PATCH] nodedev: check/add for mdev_types capability

This is similar to commit f44ec9c1.
Commit id '500cbc06' introduced a new nested capability element of type
'mdev_types' and the ability to use the flag as a way to search for a
specific subset of a 'pci' device - namely a 'mdev_types'.
The code modified the virNodeDeviceCapMatch whichs allows for
searching using the 'virsh nodedev-list [cap]' via
virConnectListAllNodeDevices.

However, the original patches did not account for other searches for
the same capability key from virNodeDeviceNumOfCaps and
virNodeDeviceListCaps using nodeDeviceNumOfCaps and nodeDeviceListCaps.

This patch adds the check for the 'mdev_types' bits within a 'pci'
device and allows the following python code to find the capabilities
for the device:

import libvirt
conn = libvirt.openReadOnly('qemu:///system')
devs = conn.listAllDevices()
for dev in devs:
if 'mdev_types' in dev.listCaps():
print dev.name(),dev.numOfCaps(),dev.listCaps()

Signed-off-by: Dan Zheng 
---
 src/node_device/node_device_driver.c | 15 +++
 1 file changed, 15 insertions(+)

diff --git a/src/node_device/node_device_driver.c 
b/src/node_device/node_device_driver.c
index facfeb6..6216a69 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -382,6 +382,12 @@ nodeDeviceNumOfCaps(virNodeDevicePtr device)
 VIR_NODE_DEV_CAP_FLAG_HBA_VPORT_OPS)
 ncaps++;
 }
+if (caps->data.type == VIR_NODE_DEV_CAP_PCI_DEV) {
+if (caps->data.pci_dev.flags &
+VIR_NODE_DEV_CAP_FLAG_PCI_MDEV)
+ncaps++;
+}
+
 }
 
 ret = ncaps;
@@ -432,6 +438,15 @@ nodeDeviceListCaps(virNodeDevicePtr device,
 goto cleanup;
 }
 }
+if (caps->data.type == VIR_NODE_DEV_CAP_PCI_DEV) {
+if (ncaps < maxnames &&
+caps->data.pci_dev.flags &
+VIR_NODE_DEV_CAP_FLAG_PCI_MDEV) {
+if (VIR_STRDUP(names[ncaps++],
+   
virNodeDevCapTypeToString(VIR_NODE_DEV_CAP_MDEV_TYPES)) < 0)
+goto cleanup;
+}
+}
 }
 ret = ncaps;
 
-- 
1.8.3.1

-- 
Best Regards,
Dan Zheng(郑丹)
Red Hat Software (Beijing) Co.
9/F, North Tower C, Raycom Infotech Park
No.2 Ke Xueyuan Nanlu, Haidian District Beijing 100190

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

[libvirt] [PATCH v3 2/2] tests: Adding test case for virsh 'domrename' command.

[Julio Faracco]

This commit introduce the virsh-rename test script to test the 'domrename'
command. The test contains one succedeed script to rename and another
failed test.

Signed-off-by: Julio Faracco 
---
 tests/Makefile.am  |  1 +
 tests/virsh-rename | 43 +++
 2 files changed, 44 insertions(+)
 create mode 100755 tests/virsh-rename

diff --git a/tests/Makefile.am b/tests/Makefile.am
index 3441dab..0ff90fb 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -410,6 +410,7 @@ libvirtd_test_scripts = \
virt-admin-self-test \
virsh-start \
virsh-undefine \
+   virsh-rename \
virsh-uriprecedence \
virsh-vcpupin \
$(NULL)
diff --git a/tests/virsh-rename b/tests/virsh-rename
new file mode 100755
index 000..4d976bb
--- /dev/null
+++ b/tests/virsh-rename
@@ -0,0 +1,43 @@
+#!/bin/sh
+# exercise virsh's "domrename" command
+
+# Copyright (C) 2008-2009, 2017 Red Hat, Inc.
+
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 2 of the License, or
+# (at your option) any later version.
+
+# This program is distributed in the hope that it will be useful,
+# but WITHEXP ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see
+# .
+
+. "$(dirname $0)/test-lib.sh"
+
+if test "$VERBOSE" = yes; then
+  set -x
+  $abs_top_builddir/tools/virsh --version
+fi
+
+fail=0
+
+# Succeed, now: first shut down, then rename the domain.
+$abs_top_builddir/tools/virsh -q -c test:///default \
+'shutdown test; domrename test anothertest' > out 2>&1
+test $? = 1 && fail=1
+
+# Failed, now: rename the domain without shutting down.
+$abs_top_builddir/tools/virsh -q -c test:///default \
+'domrename test anothertest' > out 2>&1
+test $? = 1 || fail=1
+cat <<\EOF > expout || fail=1
+error: Requested operation is not valid: cannot rename active domain
+EOF
+compare expout out || fail=1
+
+(exit $fail); exit $fail
-- 
2.7.4

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

[libvirt] [PATCH v3 1/2] test: Implementing testDomainRename().

[Julio Faracco]

There is no method to rename inactive domains for test driver.
After this patch, we can rename the domains using 'domrename'.

virsh# domrename test anothertest
Domain successfully renamed

Signed-off-by: Julio Faracco 
---
 src/test/test_driver.c | 84 ++
 1 file changed, 84 insertions(+)

diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index dc743b4..1461efb 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -2618,6 +2618,89 @@ testDomainGetVcpuPinInfo(virDomainPtr dom,
 return ret;
 }
 
+static int
+testDomainRenameCallback(virDomainObjPtr privdom,
+ const char *new_name,
+ unsigned int flags,
+ void *opaque)
+{
+testDriverPtr driver = opaque;
+virObjectEventPtr event_new = NULL;
+virObjectEventPtr event_old = NULL;
+int ret = -1;
+char *new_dom_name = NULL;
+char *old_dom_name = NULL;
+
+virCheckFlags(0, -1);
+
+if (VIR_STRDUP(new_dom_name, new_name) < 0)
+goto cleanup;
+
+event_old = virDomainEventLifecycleNewFromObj(privdom,
+VIR_DOMAIN_EVENT_UNDEFINED,
+
VIR_DOMAIN_EVENT_UNDEFINED_RENAMED);
+
+/* Switch name in domain definition. */
+old_dom_name = privdom->def->name;
+privdom->def->name = new_dom_name;
+new_dom_name = NULL;
+
+event_new = virDomainEventLifecycleNewFromObj(privdom,
+  VIR_DOMAIN_EVENT_DEFINED,
+  
VIR_DOMAIN_EVENT_DEFINED_RENAMED);
+ret = 0;
+
+ cleanup:
+VIR_FREE(old_dom_name);
+VIR_FREE(new_dom_name);
+testObjectEventQueue(driver, event_old);
+testObjectEventQueue(driver, event_new);
+return ret;
+}
+
+static int testDomainRename(virDomainPtr dom,
+const char *new_name,
+unsigned int flags)
+{
+testDriverPtr driver = dom->conn->privateData;
+virDomainObjPtr privdom = NULL;
+int ret = -1;
+
+virCheckFlags(0, -1);
+
+if (!(privdom = testDomObjFromDomain(dom)))
+goto cleanup;
+
+if (virDomainObjIsActive(privdom)) {
+virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+   _("cannot rename active domain"));
+goto cleanup;
+}
+
+if (!privdom->persistent) {
+virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+   _("cannot rename a transient domain"));
+goto cleanup;
+}
+
+if (virDomainObjGetState(privdom, NULL) != VIR_DOMAIN_SHUTOFF) {
+virReportError(VIR_ERR_OPERATION_INVALID,
+   "%s", _("domain has to be shutoff before renaming"));
+goto cleanup;
+}
+
+if (virDomainObjListRename(driver->domains, privdom, new_name, flags,
+   testDomainRenameCallback, driver) < 0)
+goto cleanup;
+
+/* Success, domain has been renamed. */
+ret = 0;
+
+ cleanup:
+virDomainObjEndAPI();
+return ret;
+}
+
 static char *testDomainGetXMLDesc(virDomainPtr domain, unsigned int flags)
 {
 testDriverPtr privconn = domain->conn->privateData;
@@ -6822,6 +6905,7 @@ static virHypervisorDriver testHypervisorDriver = {
 .connectDomainEventDeregisterAny = testConnectDomainEventDeregisterAny, /* 
0.8.0 */
 .connectIsAlive = testConnectIsAlive, /* 0.9.8 */
 .nodeGetCPUMap = testNodeGetCPUMap, /* 1.0.0 */
+.domainRename = testDomainRename, /* 4.0.0 */
 .domainScreenshot = testDomainScreenshot, /* 1.0.5 */
 .domainGetMetadata = testDomainGetMetadata, /* 1.1.3 */
 .domainSetMetadata = testDomainSetMetadata, /* 1.1.3 */
-- 
2.7.4

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

[libvirt] [PATCH v3 0/2] Introducing testDomainRename().

[Julio Faracco]

This commit introduces the testDomainRename() for test driver. It includes:
- testDomainRename() implementation.
- Testcase script to test 'domrename' command.

Julio Faracco (2):
  test: Implementing testDomainRename().
  tests: Adding test case for virsh 'domrename' command.

 src/test/test_driver.c | 84 ++
 tests/Makefile.am  |  1 +
 tests/virsh-rename | 43 ++
 3 files changed, 128 insertions(+)
 create mode 100755 tests/virsh-rename

-- 
2.7.4

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

Re: [libvirt] [PATCH perl] block_stats: Fix rd_req and wr_req compat hash keys

[Daniel P. Berrange]

On Mon, Jan 15, 2018 at 07:50:10PM +0200, Ville Skyttä wrote:
> When virDomainBlockStatsFlags with NULL params fails, the returned
> read and write requests hash keys are said to be backwards compatible
> with the key names success case. However, they were rd_reqs and
> wr_reqs (in plural) as opposed to the success case's rd_req and
> wr_req.
> 
> There is also a similar flush_reqs key, but that one does not have a
> corresponding value in the success case's hash, so it is left
> unmodified here.
> ---
>  Virt.xs | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Thanks, pushed to git.

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

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

[libvirt] [PATCH perl] block_stats: Fix rd_req and wr_req compat hash keys

[Ville Skyttä]

When virDomainBlockStatsFlags with NULL params fails, the returned
read and write requests hash keys are said to be backwards compatible
with the key names success case. However, they were rd_reqs and
wr_reqs (in plural) as opposed to the success case's rd_req and
wr_req.

There is also a similar flush_reqs key, but that one does not have a
corresponding value in the success case's hash, so it is left
unmodified here.
---
 Virt.xs | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Virt.xs b/Virt.xs
index c47b915..a6f0477 100644
--- a/Virt.xs
+++ b/Virt.xs
@@ -5611,9 +5611,9 @@ block_stats(dom, path, flags=0)
   field = NULL;
   /* For back compat with previous hash above */
   if (strcmp(params[i].field, "rd_operations") == 0)
-  field = "rd_reqs";
+  field = "rd_req";
   else if (strcmp(params[i].field, "wr_operations") == 0)
-  field = "wr_reqs";
+  field = "wr_req";
   else if (strcmp(params[i].field, "flush_operations") == 0)
   field = "flush_reqs";
   if (field) {
-- 
2.14.1

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

Re: [libvirt] [PATCH 3/3] m4: Check for rl_completion_quote_character

[Michal Privoznik]

On 01/15/2018 05:39 PM, Andrea Bolognani wrote:
> On Mon, 2018-01-15 at 16:36 +0100, Michal Privoznik wrote:
>>> So, one way to solve this once and for all would be to:
>>>
>>>   * try looking up readline through pkg-config. If that works,
>>> then we already know we're compiling against a recent
>>> readline version and everything will work;
>>
>> I just found out that this will not work - even though there is
>> readline.pc.in in the readline repo, they are lacking rule to install
>> the .pc file. So nobody ships that. For instance, on my rawhide box:
>>
>> [root@fedora ~]# rpm -q readline
>> readline-7.0-5.fc26.x86_64
>> [root@fedora ~]# rpm -ql readline | grep \.pc
>> [root@fedora ~]#
> 
> Ouch.
> 
> (Note the .pc file would be in the readline-devel package, not in
> the runtime one. Still, I've checked on my Rawhide guest and it's
> not there.)

Ah, right.

> 
> At least FreeBSD ships it, though:
> 
>   # pkg list readline | grep pc$
>   /usr/local/libdata/pkgconfig/readline.pc

is this GNU readline? I've heard that FreeBSD is ditching GNU software.

> 
> Not sure about brew. Not having access to a macOS box is really
> annoying in this kind of scenario.
> 
>>>   * if readline's pkg-config file is not available, try linking
>>> against it the old way. This will succeed on oldish versions
>>> like the one shipped with CentOS but fail because of missing
>>> functions on macOS.

1: ^^^

>>
>> I should have commented earlier too - what good it is to switch to
>> pkg-config if we're keeping the old way of detecting the library (with
>> this patch included) anyway?
> 
> We wouldn't need to include this patch: we could just assume
> a readline new enough to provide a .pc file contains all the
> functionality we need.

Well, what if isn't? We would just disable readline? We can't because
you're advocating for linking the old way [1]. However, that wouldn't
work on systems where we require more than just -lreadline (which is
current situation). Currently, the problem is that my code requires this
rl_completion_quote_character variable from readline.h. It's not a
function, it's a variable. And if not present, compilation fails. So
what should we do if pkg-config fails?

> 
>> Therefore I think we should merge this patch and switch to pkg-config
>> later (when distros have it).
> 
> Well, we're running out of time for 4.0.0 anyway, so
> 
>   Reviewed-by: Andrea Bolognani 

Agreed, pushed. Thanks.

Michal

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

Re: [libvirt] [PATCH perl] Spelling fixes

[Daniel P. Berrange]

On Mon, Jan 15, 2018 at 07:29:59PM +0200, Ville Skyttä wrote:
> ---
>  Changes |  4 ++--
>  HACKING |  4 ++--
>  lib/Sys/Virt.pm |  6 +++---
>  lib/Sys/Virt/Domain.pm  | 14 +++---
>  lib/Sys/Virt/Interface.pm   |  2 +-
>  lib/Sys/Virt/NWFilter.pm|  2 +-
>  lib/Sys/Virt/Network.pm |  4 ++--
>  lib/Sys/Virt/NodeDevice.pm  |  4 ++--
>  lib/Sys/Virt/Secret.pm  |  4 ++--
>  lib/Sys/Virt/StoragePool.pm |  4 ++--
>  lib/Sys/Virt/StorageVol.pm  |  8 
>  lib/Sys/Virt/Stream.pm  |  2 +-
>  12 files changed, 29 insertions(+), 29 deletions(-)

Thanks, I have pushed to git


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

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

[libvirt] [PATCH perl] Spelling fixes

[Ville Skyttä]

---
 Changes |  4 ++--
 HACKING |  4 ++--
 lib/Sys/Virt.pm |  6 +++---
 lib/Sys/Virt/Domain.pm  | 14 +++---
 lib/Sys/Virt/Interface.pm   |  2 +-
 lib/Sys/Virt/NWFilter.pm|  2 +-
 lib/Sys/Virt/Network.pm |  4 ++--
 lib/Sys/Virt/NodeDevice.pm  |  4 ++--
 lib/Sys/Virt/Secret.pm  |  4 ++--
 lib/Sys/Virt/StoragePool.pm |  4 ++--
 lib/Sys/Virt/StorageVol.pm  |  8 
 lib/Sys/Virt/Stream.pm  |  2 +-
 12 files changed, 29 insertions(+), 29 deletions(-)

diff --git a/Changes b/Changes
index 96b42f8..0571cf8 100644
--- a/Changes
+++ b/Changes
@@ -472,7 +472,7 @@ Revision history for perl module Sys::Virt
 0.2.6 2011-02-16
 
  - Fix build with 0.8.7 libvirt by removing reference to
-   non-existant constant only introduced in libvirt > 0.8.7
+   non-existent constant only introduced in libvirt > 0.8.7
  - Fix test script when hostname is undefined
 
 0.2.5 2011-02-04
@@ -533,7 +533,7 @@ Revision history for perl module Sys::Virt
  - Added networking APIs
  - Added APIs for host capabilities
  - Added APIs for inactive domains
- - Switch licensse to GPLv2+ or Artistic for CPAN compatability
+ - Switch licensse to GPLv2+ or Artistic for CPAN compatibility
  - Fix return values for reboot/shutdown/undefine/create APIs
  - Expanded documentation coverage
 
diff --git a/HACKING b/HACKING
index bb8b0f0..bfd1212 100644
--- a/HACKING
+++ b/HACKING
@@ -15,7 +15,7 @@ Additions to the libvirt C API will require changes to a 
minimum
 of two parts of the Sys::Virt codebase.
 
  - Virt.xs - this provides the C glue code to access the libvirt C
-   library APIs and constants from the Perl interpretor. As a general
+   library APIs and constants from the Perl interpreter. As a general
rule, every new function and header file constant/enum requires an
addition to this file.  The exceptions are functions that are only
provided for the benefit of language bindings and not intended for
@@ -93,7 +93,7 @@ effective way to update the Perl binding.
  - For each missing item reported in the test suite...
 
  - Edit Virt.xs to add the C binding
- - Edit lib/*.pm to add the POD documentation (and occassionally Perl glue 
code)
+ - Edit lib/*.pm to add the POD documentation (and occasionally Perl glue 
code)
  - Edit Changes to document the addition
  - Run the test suite (without maintainer mode) to verify POD docs
  # ../libvirt/run make test
diff --git a/lib/Sys/Virt.pm b/lib/Sys/Virt.pm
index 2ce7ed5..faaf668 100644
--- a/lib/Sys/Virt.pm
+++ b/lib/Sys/Virt.pm
@@ -135,7 +135,7 @@ types that will be supported. The credential constants in
 this module can be used as values in this list. The C
 parameter should be a subroutine reference containing the
 code necessary to gather the credentials. When invoked it
-will be supplied with a single parameter, a array reference
+will be supplied with a single parameter, an array reference
 of requested credentials. The elements of the array are
 hash references, with keys C giving the type of
 credential, C giving a user descriptive user
@@ -1164,7 +1164,7 @@ pool sources.
 
 =item my @stats = $vmm->get_all_domain_stats($stats, \@doms=undef, $flags=0);
 
-Get an list of all statistics for domains known to the hypervisor.
+Get a list of all statistics for domains known to the hypervisor.
 The C<$stats> parameter controls which data fields to return and
 should be a combination of the DOMAIN STATS FIELD CONSTANTS.
 
@@ -1469,7 +1469,7 @@ how long the node is suspended for before waking up.
 
 =item $conn->domain_event_register($callback)
 
-Register a callback to received notificaitons of domain state change
+Register a callback to received notifications of domain state change
 events. Only a single callback can be registered with each connection
 instance. The callback will be invoked with four parameters, an
 instance of C for the connection, an instance of 
C
diff --git a/lib/Sys/Virt/Domain.pm b/lib/Sys/Virt/Domain.pm
index 38b9c9b..7b90b68 100644
--- a/lib/Sys/Virt/Domain.pm
+++ b/lib/Sys/Virt/Domain.pm
@@ -45,7 +45,7 @@ sub _new {
 my $class = ref($proto) || $proto;
 my %params = @_;
 
-my $con = exists $params{connection} ? $params{connection} : die 
"connection parameter is requried";
+my $con = exists $params{connection} ? $params{connection} : die 
"connection parameter is required";
 my $self;
 if (exists $params{name}) {
$self = Sys::Virt::Domain::_lookup_by_name($con,  $params{name});
@@ -490,7 +490,7 @@ One of the CONTROL INFO constants listed later
 
 =item C
 
-Currently unsed, always 0.
+Currently unused, always 0.
 
 =item C
 
@@ -658,7 +658,7 @@ later.
 
 =item $dom->detach_device($xml[, $flags])
 
-Hotunplug a existing device whose configuration is given by C<$xml>,
+Hotunplug an existing device whose configuration is given by C<$xml>,
 from the running guest. The optional <$flags> parameter defaults
 to 0, but can accept one of 

Re: [libvirt] [RFC PATCH 04/10] qemu: Introduce virTheadPoolDrain

[Daniel P. Berrange]

On Mon, Jan 15, 2018 at 05:51:28PM +0100, Erik Skultety wrote:
> On Wed, Jan 10, 2018 at 12:23:29PM -0500, John Ferlan wrote:
> > Split up virThreadPoolFree to create a Drain function which will
> > be called from virNetServerClose in order to ensure the various
> > worker threads are removed during the close rather than waiting
> > for the dispose function.
> >
> > Signed-off-by: John Ferlan 
> > ---
> 
> I think that Dan had a bit different idea about the virThreadPoolDrain (I 
> think
> that the name should have been something like virThreadPoolJobsPurge) 
> function.
> https://www.redhat.com/archives/libvir-list/2017-December/msg00802.html

Yep, this impl in John's patch is more akin to a StopWorkers()
method, than a Drain() method. To me "Drain" implies the workers
are still available to process further jobs

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] [RFC PATCH 04/10] qemu: Introduce virTheadPoolDrain

[Erik Skultety]

On Wed, Jan 10, 2018 at 12:23:29PM -0500, John Ferlan wrote:
> Split up virThreadPoolFree to create a Drain function which will
> be called from virNetServerClose in order to ensure the various
> worker threads are removed during the close rather than waiting
> for the dispose function.
>
> Signed-off-by: John Ferlan 
> ---

I think that Dan had a bit different idea about the virThreadPoolDrain (I think
that the name should have been something like virThreadPoolJobsPurge) function.
https://www.redhat.com/archives/libvir-list/2017-December/msg00802.html

Erik

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

Re: [libvirt] [PATCH 3/3] m4: Check for rl_completion_quote_character

[Andrea Bolognani]

On Mon, 2018-01-15 at 16:36 +0100, Michal Privoznik wrote:
> > So, one way to solve this once and for all would be to:
> > 
> >   * try looking up readline through pkg-config. If that works,
> > then we already know we're compiling against a recent
> > readline version and everything will work;
> 
> I just found out that this will not work - even though there is
> readline.pc.in in the readline repo, they are lacking rule to install
> the .pc file. So nobody ships that. For instance, on my rawhide box:
> 
> [root@fedora ~]# rpm -q readline
> readline-7.0-5.fc26.x86_64
> [root@fedora ~]# rpm -ql readline | grep \.pc
> [root@fedora ~]#

Ouch.

(Note the .pc file would be in the readline-devel package, not in
the runtime one. Still, I've checked on my Rawhide guest and it's
not there.)

At least FreeBSD ships it, though:

  # pkg list readline | grep pc$
  /usr/local/libdata/pkgconfig/readline.pc

Not sure about brew. Not having access to a macOS box is really
annoying in this kind of scenario.

> >   * if readline's pkg-config file is not available, try linking
> > against it the old way. This will succeed on oldish versions
> > like the one shipped with CentOS but fail because of missing
> > functions on macOS.
> 
> I should have commented earlier too - what good it is to switch to
> pkg-config if we're keeping the old way of detecting the library (with
> this patch included) anyway?

We wouldn't need to include this patch: we could just assume
a readline new enough to provide a .pc file contains all the
functionality we need.

> Therefore I think we should merge this patch and switch to pkg-config
> later (when distros have it).

Well, we're running out of time for 4.0.0 anyway, so

  Reviewed-by: Andrea Bolognani 

to your patch and let's work on a better solution, assuming one
is even possible, later :)

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [RFC PATCH 03/10] netserver: Toggle service off during close

[Erik Skultety]

On Wed, Jan 10, 2018 at 12:23:28PM -0500, John Ferlan wrote:
> Rather than waiting until virNetServerDispose to toggle the service
> to off, let's do that when virNetServerServiceClose is called such
> as during virNetServerClose.
>
> Signed-off-by: John Ferlan 
> ---
>  src/rpc/virnetserver.c| 3 ---
>  src/rpc/virnetserverservice.c | 2 ++
>  2 files changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
> index 77a4c0b8d..7bab11efb 100644
> --- a/src/rpc/virnetserver.c
> +++ b/src/rpc/virnetserver.c
> @@ -805,9 +805,6 @@ void virNetServerDispose(void *obj)
>
>  VIR_FREE(srv->name);
>
> -for (i = 0; i < srv->nservices; i++)
> -virNetServerServiceToggle(srv->services[i], false);
> -

^This hunk would suffice.

>  virThreadPoolFree(srv->workers);
>
>  for (i = 0; i < srv->nservices; i++)
> diff --git a/src/rpc/virnetserverservice.c b/src/rpc/virnetserverservice.c
> index 4e5426ffe..636c5be4e 100644
> --- a/src/rpc/virnetserverservice.c
> +++ b/src/rpc/virnetserverservice.c
> @@ -525,4 +525,6 @@ void virNetServerServiceClose(virNetServerServicePtr svc)
>  virNetSocketClose(svc->socks[i]);
>  virObjectUnref(svc);
>  }
> +
> +virNetServerServiceToggle(svc, false);

^This is a NOP, since all the sockets have been closed already (in the loop
which precedes the call) and the IO callback handle removed with watch reset to
-1.

Reviewed-by: Erik Skultety 

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

Re: [libvirt] [RFC PATCH 02/10] libvirtd: Alter refcnt processing for server program objects

[Erik Skultety]

On Wed, Jan 10, 2018 at 12:23:27PM -0500, John Ferlan wrote:
> Once virNetServerProgramNew returns, the program objects have
> either been added to the @srv or @srvAdm list increasing the
> refcnt or there was an error. In either case, we can lower the
> refcnt from virNetServerProgramNew and set the object to NULL
> since it won't be used any more.
>
> The @srv or @srvAdm dispose function (virNetServerDispose) will
> handle the Unref of each object during the virHashFree when the
> object is removed from the hash table.
>
> Signed-off-by: John Ferlan 
> ---
Reviewed-by: Erik Skultety 

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

Re: [libvirt] [RFC PATCH 01/10] libvirtd: Alter refcnt processing for domain server objects

[Erik Skultety]

On Wed, Jan 10, 2018 at 12:23:26PM -0500, John Ferlan wrote:
> Once virNetDaemonAddServer returns, the @srv or @srvAdm have
> either been added to the @dmn list increasing the refcnt or
> there was an error. In either case, we can lower the refcnt
> from virNetServerNew, but not set to NULL. Thus "using" the
> hash table reference when adding programs later or allowing
> the immediate free of the object on error.
>
> The @dmn dispose function (virNetDaemonDispose) will handle
> the Unref of each object during the virHashFree when the
> object is removed from the hash table.
>
> Signed-off-by: John Ferlan 
> ---
Reviewed-by: Erik Skultety 

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

Re: [libvirt] [PATCH 3/3] m4: Check for rl_completion_quote_character

[Michal Privoznik]

On 01/15/2018 12:30 PM, Andrea Bolognani wrote:
> On Mon, 2018-01-15 at 10:26 +0100, Andrea Bolognani wrote:
>> macOS has brew, though. I've kicked off a Travis build with this
>> commit[1] included, let's see whether configure picks up readline
>> installed from brew instead of the obsolete one available in the
>> base system.
> 
> Nope, it still picks up the one shipped with the OS :/
> 
>> If it does, then we can omit your patch and... Document the version
>> requirement somehow? If we used pkg-config to detect readline
>> availability, that would be easy. Alas, readline only introduced
>> pkg-config support relatively recently, so we can't do that.
> 
> So, one way to solve this once and for all would be to:
> 
>   * try looking up readline through pkg-config. If that works,
> then we already know we're compiling against a recent
> readline version and everything will work;

I just found out that this will not work - even though there is
readline.pc.in in the readline repo, they are lacking rule to install
the .pc file. So nobody ships that. For instance, on my rawhide box:

[root@fedora ~]# rpm -q readline
readline-7.0-5.fc26.x86_64
[root@fedora ~]# rpm -ql readline | grep \.pc
[root@fedora ~]#

> 
>   * if readline's pkg-config file is not available, try linking
> against it the old way. This will succeed on oldish versions
> like the one shipped with CentOS but fail because of missing
> functions on macOS.

I should have commented earlier too - what good it is to switch to
pkg-config if we're keeping the old way of detecting the library (with
this patch included) anyway?

Therefore I think we should merge this patch and switch to pkg-config
later (when distros have it).

Michal

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

[libvirt] [PATCH] libvirt-guests: make async stop work with transient guest domains

[Vincent Bernat]

After being stopped, a transient guest domain doesn't appear in the
list of guests anymore. Therefore, we can't get its state anymore. The
branch handling this case displays an error message on stdout. This
error message confuses the script which expects to get a list of
"still up" guests. We just remove the message.

Also, since we can't get a name, display the UUID of the stopped
domain.
---
 tools/libvirt-guests.sh.in | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tools/libvirt-guests.sh.in b/tools/libvirt-guests.sh.in
index 8a158cca434d..e272325f7d5d 100644
--- a/tools/libvirt-guests.sh.in
+++ b/tools/libvirt-guests.sh.in
@@ -337,8 +337,6 @@ check_guests_shutdown()
 guests_up=
 for guest in $guests; do
 if ! guest_is_on "$uri" "$guest" >/dev/null 2>&1; then
-eval_gettext "Failed to determine state of guest: \$guest. Not 
tracking it anymore."
-echo
 continue
 fi
 if "$guest_running"; then
@@ -363,6 +361,7 @@ print_guests_shutdown()
 esac
 
 name=$(guest_name "$uri" "$guest")
+[ -n "$name" ] || name="$guest"
 eval_gettext "Shutdown of guest \$name complete."
 echo
 done
-- 
2.15.1

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

Re: [libvirt] [PATCH RESEND v1 1/2] i386: Add Intel Processor Trace feature support

[Eduardo Habkost]

On Mon, Jan 15, 2018 at 03:25:18PM +0100, Jiri Denemark wrote:
> On Mon, Jan 15, 2018 at 12:04:55 -0200, Eduardo Habkost wrote:
> > CCing libvirt developers.
> ...
> > This case is slightly more problematic, however: the new feature
> > is actually migratable (under very controlled circumstances)
> > because of patch 2/2, but it is not migration-safe[1].  This
> > means libvirt shouldn't include it in "host-model" expansion
> > (which uses the query-cpu-model-expansion QMP command) until we
> > make the feature migration-safe.
> > 
> > For QEMU, this means the feature shouldn't be returned by
> > "query-cpu-model-expansion type=static model=max" (but it can be
> > returned by "query-cpu-model-expansion type=full model=max").
> > 
> > Jiri, it looks like libvirt uses type=full on
> > query-cpu-model-expansion on x86.  It needs to use
> > type=static[2], or it will have no way to find out if a feature
> > is migration-safe or not.
> ...
> > [2] It looks like libvirt uses type=full because it wants to get
> > all QOM property aliases returned.  In this case, one
> > solution for libvirt is to use:
> > 
> > static_expansion = query_cpu_model_expansion(type=static, model)
> > all_props = query_cpu_model_expansion(type=full, static_expansion)
> 
> This is exactly what libvirt is doing (with model = "host") ever since
> query-cpu-model-expansion support was implemented for x86.

Oh, now I see that the x86 code uses
QEMU_MONITOR_CPU_MODEL_EXPANSION_STATIC_FULL and not just
QEMU_MONITOR_CPU_MODEL_EXPANSION_FULL.  Nice!

-- 
Eduardo

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

Re: [libvirt] [PATCH RESEND v1 1/2] i386: Add Intel Processor Trace feature support

[Jiri Denemark]

On Mon, Jan 15, 2018 at 12:04:55 -0200, Eduardo Habkost wrote:
> CCing libvirt developers.
...
> This case is slightly more problematic, however: the new feature
> is actually migratable (under very controlled circumstances)
> because of patch 2/2, but it is not migration-safe[1].  This
> means libvirt shouldn't include it in "host-model" expansion
> (which uses the query-cpu-model-expansion QMP command) until we
> make the feature migration-safe.
> 
> For QEMU, this means the feature shouldn't be returned by
> "query-cpu-model-expansion type=static model=max" (but it can be
> returned by "query-cpu-model-expansion type=full model=max").
> 
> Jiri, it looks like libvirt uses type=full on
> query-cpu-model-expansion on x86.  It needs to use
> type=static[2], or it will have no way to find out if a feature
> is migration-safe or not.
...
> [2] It looks like libvirt uses type=full because it wants to get
> all QOM property aliases returned.  In this case, one
> solution for libvirt is to use:
> 
> static_expansion = query_cpu_model_expansion(type=static, model)
> all_props = query_cpu_model_expansion(type=full, static_expansion)

This is exactly what libvirt is doing (with model = "host") ever since
query-cpu-model-expansion support was implemented for x86.

Jirka

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

Re: [libvirt] [PATCH] vshReadlineOptionsGenerator: Don't add already specified options to the list

[Erik Skultety]

On Mon, Jan 15, 2018 at 10:34:29AM +0100, Michal Privoznik wrote:
> The current state of art is as follows:
>
>  1) vshReadlineOptionsGenerator() generate all possible --options
>  for given command, and then
>  2) vshReadlineOptionsPrune() clears out already provided ones
>  from the list.
>
> Not only this brings needless memory complexity it is also not
> trivial to get right. We can switch to easier approach: just
> don't add already specified --options in the first step.
>
> Signed-off-by: Michal Privoznik 
> ---
Reviewed-by: Erik Skultety 

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

Re: [libvirt] [PATCH RESEND v1 1/2] i386: Add Intel Processor Trace feature support

[Eduardo Habkost]

CCing libvirt developers.

On Mon, Jan 15, 2018 at 10:33:35AM +0100, Paolo Bonzini wrote:
> On 15/01/2018 08:19, Kang, Luwei wrote:
> >> If you are forwarding host info directly to the guest, the feature
> >> is not migration-safe.  The new feature needs to be added to 
> >> feature_word_info[FEAT_7_0_EBX].unmigratable_flags.
> >> 
> > Hi, Thanks for you review. I want to support Intel PT live migration
> > and patch [2/2] to do this. I don't understand  why need to add this
> > feature in feature_word_info[FEAT_7_0_EBX].unmigratable_flags first
> > to disable live migration.
> 
> Hi Luwei,
> 
> the issue is that different hosts can have different CPUID flags.  You
> don't have a way to set the CPUID flags from the "-cpu" command line,
> and it's not clear what values will be there in the various Ice Lake SKUs.
> 
> I am not sure if there is a mechanism to allow live migration only for
> "-cpu host", but it cannot be supported for any other "-cpu" model.

IIRC, we don't have any mechanism to actually prevent migration
if an unmigratable flag is enabled.  We just avoid enabling them
by accident on "-cpu host".

This case is slightly more problematic, however: the new feature
is actually migratable (under very controlled circumstances)
because of patch 2/2, but it is not migration-safe[1].  This
means libvirt shouldn't include it in "host-model" expansion
(which uses the query-cpu-model-expansion QMP command) until we
make the feature migration-safe.

For QEMU, this means the feature shouldn't be returned by
"query-cpu-model-expansion type=static model=max" (but it can be
returned by "query-cpu-model-expansion type=full model=max").

Jiri, it looks like libvirt uses type=full on
query-cpu-model-expansion on x86.  It needs to use
type=static[2], or it will have no way to find out if a feature
is migration-safe or not.

This case is similar to "pmu", which is not migration-safe but
enabled by -cpu host by default.  But "pmu" is less problematic
because it is already skipped by query-cpu-model-expansion
type=static.

---

[1] "migration-safe" is defined in the documentation for CpuDefinitionInfo on
the QAPI schema:

# @migration-safe: whether a CPU definition can be safely used for
#  migration in combination with a QEMU compatibility machine
#  when migrating between different QMU versions and between
#  hosts with different sets of (hardware or software)
#  capabilities. If not provided, information is not available
#  and callers should not assume the CPU definition to be
#  migration-safe. (since 2.8)

[2] It looks like libvirt uses type=full because it wants to get
all QOM property aliases returned.  In this case, one
solution for libvirt is to use:

static_expansion = query_cpu_model_expansion(type=static, model)
all_props = query_cpu_model_expansion(type=full, static_expansion)

-- 
Eduardo

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

Re: [libvirt] [PATCH 3/3] m4: Check for rl_completion_quote_character

[Daniel P. Berrange]

On Mon, Jan 15, 2018 at 02:20:01PM +0100, Andrea Bolognani wrote:
> On Mon, 2018-01-15 at 13:31 +0100, Michal Privoznik wrote:
> > > So, one way to solve this once and for all would be to:
> > > 
> > >   * try looking up readline through pkg-config. If that works,
> > > then we already know we're compiling against a recent
> > > readline version and everything will work;
> > > 
> > >   * if readline's pkg-config file is not available, try linking
> > > against it the old way. This will succeed on oldish versions
> > > like the one shipped with CentOS but fail because of missing
> > > functions on macOS.
> > > 
> > > I could try cooking up something like the above, but I can never
> > > seem to get it right the first couple of times when m4 is involved,
> > > so in the interest of time - and not having to merge this patch you
> > > hate ;) - would you mind looking into it yourself instead?
> > 
> > The reason I hate this patch is not because the patch itself looks ugly.
> > It's because we have to deal with the situation in the first place and
> > invest our time in resolving it. And what you're suggesting might sound
> > right but we'll end up with the same situation after all.
> 
> I don't think that's the case. Right now we have to work around
> issues in macOS all the time because we're linking against the
> obsolete readline version included in the base system; if we
> implemented what I propose above, then we could just mandate that
> readline 6.0 or newer is required. macOS builds would then fail
> unless you install a recent readline using brew, but that's
> entirely acceptable, and we would have obtained a reasonable
> baseline to work against going forward. Plus it would allow us to
> get rid of some nasty hacks[1] on our CI as well :)

IMHO it is entirely reasonable to require modern readline when
building on macOS, and not try to support the one in bsae system. 

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

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

[libvirt] [jenkins-ci PATCH] guests: Enable readline compat on FreeBSD CURRENT

[Andrea Bolognani]

FreeBSD 10 is the only version *not* to need the compat hack,
so special-case it instead of the other way around.

Signed-off-by: Andrea Bolognani 
---
 guests/tasks/compat.yml | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/guests/tasks/compat.yml b/guests/tasks/compat.yml
index 001b5c6..d48834b 100644
--- a/guests/tasks/compat.yml
+++ b/guests/tasks/compat.yml
@@ -15,8 +15,8 @@
   when:
 - os_name == 'FreeBSD'
 
-# Same as above, except we only need to do it on FreeBSD 11 because
-# FreeBSD 10 shipped (an old version of) readline in the base system
+# Same as above, except we only need to do it on FreeBSD 11 and later
+# because FreeBSD 10 shipped (an old version of) readline in the base system
 - name: Create compatibility symlinks
   file:
 src: '/usr/local/{{ item }}'
@@ -28,4 +28,4 @@
 - lib/libreadline.so
   when:
 - os_name == 'FreeBSD'
-- os_version == '11'
+- os_version != '10'
-- 
2.14.3

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

Re: [libvirt] [PATCH 3/3] m4: Check for rl_completion_quote_character

[Andrea Bolognani]

On Mon, 2018-01-15 at 13:31 +0100, Michal Privoznik wrote:
> > So, one way to solve this once and for all would be to:
> > 
> >   * try looking up readline through pkg-config. If that works,
> > then we already know we're compiling against a recent
> > readline version and everything will work;
> > 
> >   * if readline's pkg-config file is not available, try linking
> > against it the old way. This will succeed on oldish versions
> > like the one shipped with CentOS but fail because of missing
> > functions on macOS.
> > 
> > I could try cooking up something like the above, but I can never
> > seem to get it right the first couple of times when m4 is involved,
> > so in the interest of time - and not having to merge this patch you
> > hate ;) - would you mind looking into it yourself instead?
> 
> The reason I hate this patch is not because the patch itself looks ugly.
> It's because we have to deal with the situation in the first place and
> invest our time in resolving it. And what you're suggesting might sound
> right but we'll end up with the same situation after all.

I don't think that's the case. Right now we have to work around
issues in macOS all the time because we're linking against the
obsolete readline version included in the base system; if we
implemented what I propose above, then we could just mandate that
readline 6.0 or newer is required. macOS builds would then fail
unless you install a recent readline using brew, but that's
entirely acceptable, and we would have obtained a reasonable
baseline to work against going forward. Plus it would allow us to
get rid of some nasty hacks[1] on our CI as well :)


[1] 
https://github.com/libvirt/libvirt-jenkins-ci/blob/635a1e22806c525a11d80fc32cc664bb672f65fa/guests/tasks/compat.yml#L18-L31
-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH 3/3] m4: Check for rl_completion_quote_character

[Michal Privoznik]

On 01/15/2018 12:30 PM, Andrea Bolognani wrote:
> On Mon, 2018-01-15 at 10:26 +0100, Andrea Bolognani wrote:
>> macOS has brew, though. I've kicked off a Travis build with this
>> commit[1] included, let's see whether configure picks up readline
>> installed from brew instead of the obsolete one available in the
>> base system.
> 
> Nope, it still picks up the one shipped with the OS :/
> 
>> If it does, then we can omit your patch and... Document the version
>> requirement somehow? If we used pkg-config to detect readline
>> availability, that would be easy. Alas, readline only introduced
>> pkg-config support relatively recently, so we can't do that.
> 
> So, one way to solve this once and for all would be to:
> 
>   * try looking up readline through pkg-config. If that works,
> then we already know we're compiling against a recent
> readline version and everything will work;
> 
>   * if readline's pkg-config file is not available, try linking
> against it the old way. This will succeed on oldish versions
> like the one shipped with CentOS but fail because of missing
> functions on macOS.
> 
> I could try cooking up something like the above, but I can never
> seem to get it right the first couple of times when m4 is involved,
> so in the interest of time - and not having to merge this patch you
> hate ;) - would you mind looking into it yourself instead?
> 

The reason I hate this patch is not because the patch itself looks ugly.
It's because we have to deal with the situation in the first place and
invest our time in resolving it. And what you're suggesting might sound
right but we'll end up with the same situation after all.

I don't know. What are your thoughts?

Michal

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

Re: [libvirt] [PATCH 3/3] m4: Check for rl_completion_quote_character

[Andrea Bolognani]

On Mon, 2018-01-15 at 10:26 +0100, Andrea Bolognani wrote:
> macOS has brew, though. I've kicked off a Travis build with this
> commit[1] included, let's see whether configure picks up readline
> installed from brew instead of the obsolete one available in the
> base system.

Nope, it still picks up the one shipped with the OS :/

> If it does, then we can omit your patch and... Document the version
> requirement somehow? If we used pkg-config to detect readline
> availability, that would be easy. Alas, readline only introduced
> pkg-config support relatively recently, so we can't do that.

So, one way to solve this once and for all would be to:

  * try looking up readline through pkg-config. If that works,
then we already know we're compiling against a recent
readline version and everything will work;

  * if readline's pkg-config file is not available, try linking
against it the old way. This will succeed on oldish versions
like the one shipped with CentOS but fail because of missing
functions on macOS.

I could try cooking up something like the above, but I can never
seem to get it right the first couple of times when m4 is involved,
so in the interest of time - and not having to merge this patch you
hate ;) - would you mind looking into it yourself instead?

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH v2] virsh: fix build without readline

[Roman Bogorodskiy]

  Erik Skultety wrote:

> On Sat, Jan 13, 2018 at 06:46:00PM +0400, Roman Bogorodskiy wrote:
> > Completion in virsh is enabled when readline is available. In order to
> > fix build when it's not available, do the following:
> >
> >  * Unconditionally add virsh-completer.[ch] and
> >virt-admin-completer.[ch] to the build, and provide stub functions
> >for when readline is not available. This way virsh builds without
> >complaining about missing symbols used for 'completer' in
> >vshCmdOptDef;
> >  * In cmdComplete(), mark unused arguments when there's no readline
> >with ATTRIBUTE_UNUSED.
> > ---
> 
> ...
> 
> >
> >
> > +#ifdef WITH_READLINE
> >  char **
> >  virshDomainNameCompleter(vshControl *ctl,
> >   const vshCmd *cmd ATTRIBUTE_UNUSED,
> > @@ -147,3 +148,25 @@ virshDomainInterfaceCompleter(vshControl *ctl,
> >  virStringListFree(ret);
> >  return NULL;
> >  }
> > +#else
> > +char **
> > +virshDomainNameCompleter(vshControl *ctl ATTRIBUTE_UNUSED,
> > + const vshCmd *cmd ATTRIBUTE_UNUSED,
> > + unsigned int flags)
> > +{
> > +virCheckFlags(-1, NULL);
> > +
> > +return NULL;
> > +}
> 
> Do you actually need to define these "no readline" function versions? I'm
> asking because the completer callbacks are transitively invoked from
> vshReadlineCompletion (and friends) which is only called from the cmdComplete
> which both your and Michal's patch handles. So, are you experiencing any
> problems on any platforms without these hunks? (I compiles fine with just the
> first and last hunk applied, obviously no readline support)
> 
> Anyhow, I went ahead and reviewed Michal's patches, so unless you really need
> the hunks I mentioned in the previous paragraph, let's go with Michal's 
> instead.
> 
> Erik

These stub versions are not needed indeed, let's go with Michal's patches.

Roman Bogorodskiy


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

Re: [libvirt] [jenkins-ci PATCH] guests: Install libtirpc when building libvirt

[Daniel P. Berrange]

On Mon, Jan 15, 2018 at 09:12:18AM +0100, Andrea Bolognani wrote:
> Recent glibc versions don't include an RPC library anymore, so
> we need to make sure an alternative implementation is available
> for libvirt to use.
> 
> Signed-off-by: Andrea Bolognani 
> ---
>  guests/vars/mappings.yml | 4 
>  guests/vars/projects/libvirt.yml | 1 +
>  2 files changed, 5 insertions(+)

Reviewed-by: Daniel P. Berrange 


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 v2] virsh: fix build without readline

[Erik Skultety]

On Sat, Jan 13, 2018 at 06:46:00PM +0400, Roman Bogorodskiy wrote:
> Completion in virsh is enabled when readline is available. In order to
> fix build when it's not available, do the following:
>
>  * Unconditionally add virsh-completer.[ch] and
>virt-admin-completer.[ch] to the build, and provide stub functions
>for when readline is not available. This way virsh builds without
>complaining about missing symbols used for 'completer' in
>vshCmdOptDef;
>  * In cmdComplete(), mark unused arguments when there's no readline
>with ATTRIBUTE_UNUSED.
> ---

...

>
>
> +#ifdef WITH_READLINE
>  char **
>  virshDomainNameCompleter(vshControl *ctl,
>   const vshCmd *cmd ATTRIBUTE_UNUSED,
> @@ -147,3 +148,25 @@ virshDomainInterfaceCompleter(vshControl *ctl,
>  virStringListFree(ret);
>  return NULL;
>  }
> +#else
> +char **
> +virshDomainNameCompleter(vshControl *ctl ATTRIBUTE_UNUSED,
> + const vshCmd *cmd ATTRIBUTE_UNUSED,
> + unsigned int flags)
> +{
> +virCheckFlags(-1, NULL);
> +
> +return NULL;
> +}

Do you actually need to define these "no readline" function versions? I'm
asking because the completer callbacks are transitively invoked from
vshReadlineCompletion (and friends) which is only called from the cmdComplete
which both your and Michal's patch handles. So, are you experiencing any
problems on any platforms without these hunks? (I compiles fine with just the
first and last hunk applied, obviously no readline support)

Anyhow, I went ahead and reviewed Michal's patches, so unless you really need
the hunks I mentioned in the previous paragraph, let's go with Michal's instead.

Erik

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

[libvirt] [PATCH] vshReadlineOptionsGenerator: Don't add already specified options to the list

[Michal Privoznik]

The current state of art is as follows:

 1) vshReadlineOptionsGenerator() generate all possible --options
 for given command, and then
 2) vshReadlineOptionsPrune() clears out already provided ones
 from the list.

Not only this brings needless memory complexity it is also not
trivial to get right. We can switch to easier approach: just
don't add already specified --options in the first step.

Signed-off-by: Michal Privoznik 
---
 tools/vsh.c | 80 +++--
 1 file changed, 19 insertions(+), 61 deletions(-)

diff --git a/tools/vsh.c b/tools/vsh.c
index 4426c08d6..a99d47bfc 100644
--- a/tools/vsh.c
+++ b/tools/vsh.c
@@ -2663,7 +2663,9 @@ vshReadlineCommandGenerator(const char *text)
 }
 
 static char **
-vshReadlineOptionsGenerator(const char *text, const vshCmdDef *cmd)
+vshReadlineOptionsGenerator(const char *text,
+const vshCmdDef *cmd,
+vshCmd *last)
 {
 size_t list_index = 0;
 size_t len = strlen(text);
@@ -2678,6 +2680,8 @@ vshReadlineOptionsGenerator(const char *text, const 
vshCmdDef *cmd)
 return NULL;
 
 while ((name = cmd->opts[list_index].name)) {
+bool exists = false;
+vshCmdOpt *opt =  last->opts;
 size_t name_len;
 
 list_index++;
@@ -2692,6 +2696,18 @@ vshReadlineOptionsGenerator(const char *text, const 
vshCmdDef *cmd)
 return NULL;
 }
 
+while (opt) {
+if (STREQ(opt->def->name, name)) {
+exists = true;
+break;
+}
+
+opt = opt->next;
+}
+
+if (exists)
+continue;
+
 if (VIR_REALLOC_N(ret, ret_size + 2) < 0) {
 virStringListFree(ret);
 return NULL;
@@ -2772,60 +2788,6 @@ vshCompleterFilter(char ***list,
 }
 
 
-static int
-vshReadlineOptionsPrune(char ***list,
-vshCmd *last)
-{
-char **newList = NULL;
-size_t newList_len = 0;
-size_t list_len;
-size_t i;
-
-if (!list || !*list)
-return -1;
-
-if (!last->opts)
-return 0;
-
-list_len = virStringListLength((const char **) *list);
-
-if (VIR_ALLOC_N(newList, list_len + 1) < 0)
-return -1;
-
-for (i = 0; i < list_len; i++) {
-const char *list_opt = STRSKIP((*list)[i], "--");
-bool exist = false;
-vshCmdOpt *opt =  last->opts;
-
-/* Should never happen (TM) */
-if (!list_opt)
-return -1;
-
-while (opt) {
-if (STREQ(opt->def->name, list_opt)) {
-exist = true;
-break;
-}
-
-opt = opt->next;
-}
-
-if (exist) {
-VIR_FREE((*list)[i]);
-continue;
-}
-
-VIR_STEAL_PTR(newList[newList_len], (*list)[i]);
-newList_len++;
-}
-
-ignore_value(VIR_REALLOC_N_QUIET(newList, newList_len + 1));
-VIR_FREE(*list);
-*list = newList;
-return 0;
-}
-
-
 static char *
 vshReadlineParse(const char *text, int state)
 {
@@ -2874,12 +2836,8 @@ vshReadlineParse(const char *text, int state)
 if (!cmd) {
 list = vshReadlineCommandGenerator(text);
 } else {
-if (!opt || (opt->type != VSH_OT_DATA && opt->type != 
VSH_OT_STRING)) {
-list = vshReadlineOptionsGenerator(text, cmd);
-
-if (vshReadlineOptionsPrune(, partial) < 0)
-goto cleanup;
-}
+if (!opt || (opt->type != VSH_OT_DATA && opt->type != 
VSH_OT_STRING))
+list = vshReadlineOptionsGenerator(text, cmd, partial);
 
 if (opt && opt->completer) {
 char **completer_list = opt->completer(autoCompleteOpaque,
-- 
2.13.6

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

Re: [libvirt] [PATCH 3/3] m4: Check for rl_completion_quote_character

[Andrea Bolognani]

On Sun, 2018-01-14 at 14:46 +0100, Michal Privoznik wrote:
> Apparently we can't assume that people run readline recent enough
> to have rl_completion_quote_character (added in readline-5.0
> released in 2011). However, we can't compile without it. So if
> not present, disable readline.
> 
> Signed-off-by: Michal Privoznik 
> ---
> 
> Frankly, I hate this patch. How far into the past do we want to go when
> introducing something new? 10 years? 15? I've only written this patch
> because travis is unhappy without it (I'm looking at you Mac OS/X).

The problem with macOS is that Apple is shipping very old releases
of a lot of GNU software. Compare that with FreeBSD, which got rid
of basically all GNU software from the base system but still makes
(modern versions of) it available through ports.

macOS has brew, though. I've kicked off a Travis build with this
commit[1] included, let's see whether configure picks up readline
installed from brew instead of the obsolete one available in the
base system.

If it does, then we can omit your patch and... Document the version
requirement somehow? If we used pkg-config to detect readline
availability, that would be easy. Alas, readline only introduced
pkg-config support relatively recently, so we can't do that.


[1] 
https://github.com/andreabolognani/libvirt/commit/f2ca5da50609b814edbbb73858ce006862d4cb2e
-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH] vshReadlineOptionsPrune: Fix possible leak

[Michal Privoznik]

On 01/12/2018 10:07 PM, John Ferlan wrote:
> 
> 
> On 01/12/2018 11:08 AM, Michal Privoznik wrote:
>> The function should prune list of --options so that options
>> already specified are not offered to user for completion again.
>> However, if the list of offered options contains a string that
>> doesn't start with double dash the function returns leaking
>> partially constructed list. There's not much benefit from trying
>> to roll back. Just free everything up - our only caller would do
>> that anyway.
>>
>> Signed-off-by: Michal Privoznik 
>> ---
>>  tools/vsh.c | 11 ++-
>>  1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/vsh.c b/tools/vsh.c
>> index 4426c08d6..7db0a16f1 100644
>> --- a/tools/vsh.c
>> +++ b/tools/vsh.c
>> @@ -2798,8 +2798,17 @@ vshReadlineOptionsPrune(char ***list,
>>  vshCmdOpt *opt =  last->opts;
>>  
>>  /* Should never happen (TM) */
>> -if (!list_opt)
>> +if (!list_opt) {
>> +/* But in case it does, we're in a tough situation
>> + * because @list[0..i-1] is possibly sparse. That
>> + * means if caller were to call virStringListFree
>> + * over it some memory is definitely going to be
>> + * leaked. The best we can do is to free from list[i]
>> + * as our only caller is just fine with it. */
>> +virStringListFree(list[i]);
> 
> Sorry for opening Pandora's box of pain and suffering.
> 
> There's something about this that is just strange... Since list is a
> VIR_ALLOC_N list with N entries filled in
> 
> Passing virStringListFree(list[i]) would be the current entry and I
> think would be fine in the "while (tmp && *tmp) loop; however, when the
> code gets to VIR_FREE(strings), wouldn't that be the "wrong place" (e.g.
> list[i] instead of list)?  Later we would then VIR_FREE(list) because of
> the -1 return, which would be correct.
> 
> So all that said, should this be something like:
> 
> while (list_len > i)
> VIR_FREE((*list)[--list_len]);
> 
> (could be gated with an i > 0 too).
> 
> then later when the caller runs virStringListFree it does the
> VIR_FREE(strings) avoiding of course the while(tmp && *tmp) loop.

Okay, I think something fishy is going on here. I mean, if I modify the
condition to:

if (!list_opt || i > 10) {

I can not only see the leak but even virsh is crashing for me. Anyway,
why don't we take a different approach - when constructing the list of
--options simply don't put already specified --options there instead of
pruning the list later. I'm gonna send a patch for that in a moment.

Michal

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

Re: [libvirt] [PATCH 2/3] tools: Always compile {virsh, virt-admin}-completer.c

[Erik Skultety]

On Sun, Jan 14, 2018 at 02:46:44PM +0100, Michal Privoznik wrote:
> The functions defined in these sources are referenced all over
> the place, however, compiler only when building with readline.
> Thus when building without it linker gets sad as it can't find
> them.
>
> Signed-off-by: Michal Privoznik 

Reviewed-by: Erik Skultety 

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

Re: [libvirt] [PATCH 1/3] vsh: Mark cmdComplete arguments as unused

[Erik Skultety]

On Sun, Jan 14, 2018 at 02:46:43PM +0100, Michal Privoznik wrote:
> When building without readline, this function does nothing but
> return false. Without touching any of its arguments. Therefore,
> we have to mark them as unused even though they might be used
> when building with readline support.
>
> Signed-off-by: Michal Privoznik 
> ---
>  tools/vsh.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/tools/vsh.c b/tools/vsh.c
> index 4426c08d6..88561ef61 100644
> --- a/tools/vsh.c
> +++ b/tools/vsh.c
> @@ -3500,7 +3500,8 @@ const vshCmdInfo info_complete[] = {
>  };
>
>  bool
> -cmdComplete(vshControl *ctl, const vshCmd *cmd)
> +cmdComplete(vshControl *ctl ATTRIBUTE_UNUSED,
> +const vshCmd *cmd ATTRIBUTE_UNUSED)
>  {
>  bool ret = false;
>  #ifdef WITH_READLINE

Hmm, I have no problem with this, although, I kinda find the usage of
ATTRIBUTE_UNUSED a bit obfuscated here, especially if compiled with readline,
since one might not see the connection at first glance - I think we might want
to go down the usual road and have a separate definition of the function in the
distinct preprocessor branches, I know, more lines, but IMHO it enhances the
readability.
Just my 2 cents.

Reviewed-by: Erik Skultety 
Erik

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

[libvirt] [jenkins-ci PATCH] guests: Install libtirpc when building libvirt

[Andrea Bolognani]

Recent glibc versions don't include an RPC library anymore, so
we need to make sure an alternative implementation is available
for libvirt to use.

Signed-off-by: Andrea Bolognani 
---
 guests/vars/mappings.yml | 4 
 guests/vars/projects/libvirt.yml | 1 +
 2 files changed, 5 insertions(+)

diff --git a/guests/vars/mappings.yml b/guests/vars/mappings.yml
index 9c194d0..e669b6c 100644
--- a/guests/vars/mappings.yml
+++ b/guests/vars/mappings.yml
@@ -279,6 +279,10 @@ mappings:
 pkg: libssh2
 rpm: libssh2-devel
 
+  libtirpc:
+deb: libtirpc-dev
+rpm: libtirpc-devel
+
   libtool:
 default: libtool
 Debian: libtool-bin
diff --git a/guests/vars/projects/libvirt.yml b/guests/vars/projects/libvirt.yml
index ba1e4c3..598dfc4 100644
--- a/guests/vars/projects/libvirt.yml
+++ b/guests/vars/projects/libvirt.yml
@@ -32,6 +32,7 @@ packages:
   - libselinux
   - libssh
   - libssh2
+  - libtirpc
   - libudev
   - libxml2
   - lvm2
-- 
2.14.3

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