[libvirt] [PATCH] qemu: Prepare BIOS/UEFI when starting a domain

[Michal Privoznik]

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

Users might use a block device as UEFI VAR store. Or even have
OVMF stored there. Therefore, when starting a domain and separate
mount namespace is used, we have to create all the /dev entries
that are configured for the domain.

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

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 43bd0fff4..04af68809 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -9470,6 +9470,44 @@ qemuDomainSetupAllRNGs(virQEMUDriverConfigPtr cfg,
 }
 
 
+static int
+qemuDomainSetupLoader(virQEMUDriverConfigPtr cfg ATTRIBUTE_UNUSED,
+  virDomainObjPtr vm,
+  const struct qemuDomainCreateDeviceData *data)
+{
+virDomainLoaderDefPtr loader = vm->def->os.loader;
+int ret = -1;
+
+VIR_DEBUG("Setting up loader");
+
+if (loader) {
+switch ((virDomainLoader) loader->type) {
+case VIR_DOMAIN_LOADER_TYPE_ROM:
+if (qemuDomainCreateDevice(loader->path, data, false) < 0)
+goto cleanup;
+break;
+
+case VIR_DOMAIN_LOADER_TYPE_PFLASH:
+if (qemuDomainCreateDevice(loader->path, data, false) < 0)
+goto cleanup;
+
+if (loader->nvram &&
+qemuDomainCreateDevice(loader->nvram, data, false) < 0)
+goto cleanup;
+break;
+
+case VIR_DOMAIN_LOADER_TYPE_LAST:
+break;
+}
+}
+
+VIR_DEBUG("Setup loader");
+ret = 0;
+ cleanup:
+return ret;
+}
+
+
 int
 qemuDomainBuildNamespace(virQEMUDriverConfigPtr cfg,
  virSecurityManagerPtr mgr,
@@ -9538,6 +9576,9 @@ qemuDomainBuildNamespace(virQEMUDriverConfigPtr cfg,
 if (qemuDomainSetupAllRNGs(cfg, vm, ) < 0)
 goto cleanup;
 
+if (qemuDomainSetupLoader(cfg, vm, ) < 0)
+goto cleanup;
+
 /* Save some mount points because we want to share them with the host */
 for (i = 0; i < ndevMountsPath; i++) {
 struct stat sb;
-- 
2.13.6

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

[libvirt] [PATCH v2] qemuBuildMemPathStr: Forbid memoryBacking/access for non-numa case

[Michal Privoznik]

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

If a domain has no numa nodes, that means we don't put any
memory-backend-file onto the qemu command line. That in turn
means we can't set access='shared'. Therefore, we should produce
an error instead of ignoring the setting silently.

Signed-off-by: Michal Privoznik 
---

diff to v2:
- switched from check at cmd line generation to validation callback (as
  requested in review).

 src/qemu/qemu_domain.c  | 29 -
 tests/qemuxml2argvdata/hugepages-memaccess3.xml | 87 +
 tests/qemuxml2argvtest.c|  3 +
 3 files changed, 118 insertions(+), 1 deletion(-)
 create mode 100644 tests/qemuxml2argvdata/hugepages-memaccess3.xml

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 43bd0fff4..70fb40650 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3997,6 +3997,30 @@ qemuDomainDeviceDefValidateController(const 
virDomainControllerDef *controller,
 }
 
 
+static int
+qemuDomainDeviceDefValidateMemory(const virDomainMemoryDef *memory 
ATTRIBUTE_UNUSED,
+  const virDomainDef *def)
+{
+const long system_page_size = virGetSystemPageSizeKB();
+
+/* We can't guarantee any other mem.access
+ * if no guest NUMA nodes are defined. */
+if (def->mem.nhugepages != 0 &&
+def->mem.hugepages[0].size != system_page_size &&
+virDomainNumaGetNodeCount(def->numa) == 0 &&
+def->mem.access != VIR_DOMAIN_MEMORY_ACCESS_DEFAULT &&
+def->mem.access != VIR_DOMAIN_MEMORY_ACCESS_PRIVATE) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("memory access mode '%s' not supported "
+ "without guest numa node"),
+   virDomainMemoryAccessTypeToString(def->mem.access));
+return -1;
+}
+
+return 0;
+}
+
+
 static int
 qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev,
 const virDomainDef *def,
@@ -4052,6 +4076,10 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef 
*dev,
 qemuCaps);
 break;
 
+case VIR_DOMAIN_DEVICE_MEMORY:
+ret = qemuDomainDeviceDefValidateMemory(dev->data.memory, def);
+break;
+
 case VIR_DOMAIN_DEVICE_LEASE:
 case VIR_DOMAIN_DEVICE_FS:
 case VIR_DOMAIN_DEVICE_INPUT:
@@ -4063,7 +4091,6 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev,
 case VIR_DOMAIN_DEVICE_SHMEM:
 case VIR_DOMAIN_DEVICE_TPM:
 case VIR_DOMAIN_DEVICE_PANIC:
-case VIR_DOMAIN_DEVICE_MEMORY:
 case VIR_DOMAIN_DEVICE_IOMMU:
 case VIR_DOMAIN_DEVICE_NONE:
 case VIR_DOMAIN_DEVICE_LAST:
diff --git a/tests/qemuxml2argvdata/hugepages-memaccess3.xml 
b/tests/qemuxml2argvdata/hugepages-memaccess3.xml
new file mode 100644
index 0..8ec38d802
--- /dev/null
+++ b/tests/qemuxml2argvdata/hugepages-memaccess3.xml
@@ -0,0 +1,87 @@
+
+  fedora
+  63840878-0deb-4095-97e6-fc444d9bc9fa
+  4194304
+  4194304
+  
+
+
+  
+  4
+  
+hvm
+
+  
+  
+
+
+
+  
+  
+
+  
+  
+
+
+
+  
+  destroy
+  restart
+  destroy
+  
+
+
+  
+  
+/usr/bin/qemu-system-x86_64
+
+  
+  
+  
+  
+  
+
+
+  
+
+
+
+  
+
+
+  
+
+
+  
+
+
+  
+
+  
+
+
+  
+
+
+  
+  
+
+
+
+
+  
+
+
+  
+
+
+  
+
+  
+  
+
+
+  
+
+  
+
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 10ab8d787..fe15072dc 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -861,6 +861,9 @@ mymain(void)
 DO_TEST("hugepages-memaccess2", QEMU_CAPS_OBJECT_MEMORY_FILE,
 QEMU_CAPS_OBJECT_MEMORY_RAM, QEMU_CAPS_DEVICE_PC_DIMM,
 QEMU_CAPS_NUMA);
+DO_TEST_FAILURE("hugepages-memaccess3", QEMU_CAPS_MEM_PATH,
+QEMU_CAPS_OBJECT_MEMORY_RAM, QEMU_CAPS_OBJECT_MEMORY_FILE,
+QEMU_CAPS_VIRTIO_SCSI);
 DO_TEST("nosharepages", QEMU_CAPS_MACHINE_OPT, QEMU_CAPS_MEM_MERGE);
 DO_TEST("disk-cdrom", NONE);
 DO_TEST("disk-iscsi", NONE);
-- 
2.13.6

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

Re: [libvirt] virt-install ERROR Host does not support any virtualization options

[Michal Privoznik]

On 01/02/2018 12:57 PM, Mihamina RAKOTOMANDIMBY wrote:
> Hello,
> 
> I face this problem and I am willing to provide a patch in order to have
> a more informative message.
> This, of course with our help. I am not quite certain.
> 
> First of all: What is the problem?
> 
> On a fresh Install (ArchLinux for me), if ever forget to install Qemu
> and launch "virt-install" with "--type kvm", I get the message:
> 
> "Host does not support any virtualization options"
> 
> 
> According to me, this message should be improved: if Qemu is not found,
> this should be "Did not find Qemu, please install it".
> 
> As I searched, the message is generated by this piece of code:
> 
> https://github.com/virt-manager/virt-manager/blob/c92aade081687b19f5a60cddfe331b2fb6d09f98/virtinst/capabilities.py#L390
> 
> 
> As I write now, I cant find the pice of code actually calling the
> "quemu" binary. I think I should add a try/catch block there and Raise
> the right Exception.
> Would someone help me a bit?

virt-manager is not calling any qemu binaries at all. It just relies on
whatever capabilities libvirt presents. The reason for that is remote
access - just imagine you'd be installing a domain on a remote machine
and virt-manager (virt-install) would spawn your local qemu binary to
find out its capabilities. That would be flawed, wouldn't it.

I agree that the error message can be made better (just like 90% of
other messages of ours), but first things first. What's the output of
'virsh capabilities' (if you're giving any connection URI to
virt-install give it to virsh too).

Michal

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

Re: [libvirt] [PATCH v2] maint: Update to latest gnulib

[Michal Privoznik]

On 01/02/2018 11:23 PM, Eric Blake wrote:
> From: Michal Privoznik 
> 
> Unfortunately, since gnulib's commit of 2c5d558745 there's an
> unused parameter to stat_time_normalize() function which gnulib
> developers don't want to fix yet [1]. Therefore, we have to work
> around it by temporarily providing a downstream patch.
> 
> 1: http://lists.gnu.org/archive/html/bug-gnulib/2018-01/msg0.html
> 
> Signed-off-by: Michal Privoznik 
> [eblake: use gnulib local diff, instead of #pragma]
> Signed-off-by: Eric Blake 
> 
> ---
> 
> v2: perhaps nicer than Michal's original suggestion, but I'd still
> wait another day or two to see if my arguments on the upstream
> gnulib thread can instead give us a better submodule commit to
> update to that avoids the problem altogether.
> ---
>  .gnulib   |  2 +-
>  bootstrap |  4 ++--
>  gnulib/local/lib/stat-time.h.diff | 13 +
>  3 files changed, 16 insertions(+), 3 deletions(-)
>  create mode 100644 gnulib/local/lib/stat-time.h.diff

Looks like gnulib can be fixed after all. I mean, both Jim an Paul
agreed with your patch for _GL_UNUSED. Therefore we can just update the
submodule after you push the gnulib fix.

Awesome, thanks for your help!

Michal

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

[libvirt] [PATCH v2] maint: Update to latest gnulib

[Eric Blake]

From: Michal Privoznik 

Unfortunately, since gnulib's commit of 2c5d558745 there's an
unused parameter to stat_time_normalize() function which gnulib
developers don't want to fix yet [1]. Therefore, we have to work
around it by temporarily providing a downstream patch.

1: http://lists.gnu.org/archive/html/bug-gnulib/2018-01/msg0.html

Signed-off-by: Michal Privoznik 
[eblake: use gnulib local diff, instead of #pragma]
Signed-off-by: Eric Blake 

---

v2: perhaps nicer than Michal's original suggestion, but I'd still
wait another day or two to see if my arguments on the upstream
gnulib thread can instead give us a better submodule commit to
update to that avoids the problem altogether.
---
 .gnulib   |  2 +-
 bootstrap |  4 ++--
 gnulib/local/lib/stat-time.h.diff | 13 +
 3 files changed, 16 insertions(+), 3 deletions(-)
 create mode 100644 gnulib/local/lib/stat-time.h.diff

diff --git a/.gnulib b/.gnulib
index 5e9abf8716..c2cb55b34e 16
--- a/.gnulib
+++ b/.gnulib
@@ -1 +1 @@
-Subproject commit 5e9abf87163ad4aeaefef0b02961f8674b0a4879
+Subproject commit c2cb55b34e76546479f195c14202dfcc870c4914
diff --git a/bootstrap b/bootstrap
index 85b85c530f..25920e991c 100755
--- a/bootstrap
+++ b/bootstrap
@@ -4,7 +4,7 @@ scriptversion=2017-09-19.08; # UTC

 # Bootstrap this package from checked-out sources.

-# Copyright (C) 2003-2017 Free Software Foundation, Inc.
+# Copyright (C) 2003-2018 Free Software Foundation, 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
@@ -792,7 +792,7 @@ symlink_to_dir()
   # aren't confused into doing unnecessary builds.  Conversely, if the
   # existing symlink's timestamp is older than the source, make it afresh,
   # so that broken tools aren't confused into skipping needed builds.  See
-  # .
+  # .
   test -h "$dst" &&
   src_ls=$(ls -diL "$src" 2>/dev/null) && set $src_ls && src_i=$1 &&
   dst_ls=$(ls -diL "$dst" 2>/dev/null) && set $dst_ls && dst_i=$1 &&
diff --git a/gnulib/local/lib/stat-time.h.diff 
b/gnulib/local/lib/stat-time.h.diff
new file mode 100644
index 00..8333520d2d
--- /dev/null
+++ b/gnulib/local/lib/stat-time.h.diff
@@ -0,0 +1,13 @@
+diff --git i/lib/stat-time.h w/lib/stat-time.h
+index 5f8bf4e12..68871f567 100644
+--- i/lib/stat-time.h
 w/lib/stat-time.h
+@@ -212,7 +212,7 @@ get_stat_birthtime (struct stat const *st)
+errno to EOVERFLOW if normalization overflowed.  This function
+is intended to be private to this .h file.  */
+ _GL_STAT_TIME_INLINE int
+-stat_time_normalize (int result, struct stat *st)
++stat_time_normalize (int result, struct stat *st _GL_UNUSED)
+ {
+ #if defined __sun && defined STAT_TIMESPEC
+   if (result == 0)
-- 
2.14.3

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

Re: [libvirt] [PATCH] blockjob: Fix error checking of blockjob status

[Eric Blake]

On 12/26/2017 07:35 AM, Jie Wang wrote:
> when the blockjob return status:"BLOCK_JOB_COMPLETED" with error:
> "File descriptor in bad state", "offset" and "len" are equal to zero,
> but the blockjob event should be "VIR_DOMAIN_BLOCK_JOB_FAILED"
> ---
>  src/qemu/qemu_monitor_json.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index e45868b..943360a 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -886,7 +886,7 @@ qemuMonitorJSONHandleBlockJobImpl(qemuMonitorPtr mon,
>  case VIR_DOMAIN_BLOCK_JOB_COMPLETED:
>  error = virJSONValueObjectGetString(data, "error");
>  /* Make sure the whole device has been processed */
> -if (offset != len)
> +if (offset != len || offset == 0)

Unfortunately, offset can also be 0 on success if you perform a block
job on a zero-length disk (why anyone would use a 0-length disk in
practice is beyond me, but who knows).  Isn't a better patch to make our
decision based on whether 'error' is non-NULL?

>  event = VIR_DOMAIN_BLOCK_JOB_FAILED;
>  break;
>  case VIR_DOMAIN_BLOCK_JOB_CANCELED:
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



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

Re: [libvirt] [PATCH] maint: Update to latest gnulib

[Eric Blake]

On 01/02/2018 07:09 AM, John Ferlan wrote:
> 
> 
> On 01/02/2018 04:28 AM, Michal Privoznik wrote:
>> Unfortunately, since gnulib's commit of 2c5d558745 there's an
>> unused parameter to stat_time_normalize() function which gnulib
>> developers don't want to fix [1]. Therefore, we have to work
>> around it by temporarily suspending -Wunused-parameter.
>>
>> 1: http://lists.gnu.org/archive/html/bug-gnulib/2018-01/msg0.html
>>
>> Signed-off-by: Michal Privoznik 
>> ---
>>
>> While we have 'gnulib update free' push rule, this one is not trivial at
>> all and thus I have not pushed it. It's ugly and I don't like it. So any
>> ideas are welcome.
>>
>>  .gnulib| 2 +-
>>  bootstrap  | 4 ++--
>>  src/storage/storage_util.c | 3 +++
>>  3 files changed, 6 insertions(+), 3 deletions(-)
>>
> 
> 
> No bright ideas on this other than perhaps only including changes just
> prior to the particular one that breaks things or somehow revert just
> that one in our local copy.

We can provide a downstream-only patch against the gnulib file that adds
the attribute unused marker in our builds, since upstream didn't like
the patch. I'll work up something like that in a moment...

> 
> Other thoughts require additional local changes just to "work around"
> the broken definition.  Such as grabbing the "old" stat-time.h, rename
> it, save it in libvirt sources, then use that new name instead of the
> new .gnulib file. That's probably worse than what you've done though.
> 
> At this point, I think your solution to minimize the impact to one
> include file is perhaps the easiest/best solution albeit not perfect.

Yes, the #pragma usage is more concise than carrying a downstream gnulib
patch, but the two approaches are not that much different in
maintenance, so it will be a matter of taste which variant of the patch
to use.

> 
> It's interesting that there is no desire to fix this problem in .gnulib
> especially since there are already 2 patches proposed and in reality the
> change fundamentally breaks on every platform other than __sun when
> STAT_TIMESPEC is defined just because it's possible (or more desirable
> in the reviewers feedback) to compile with ignore unused-parameter.

I'm also going to reply in the upstream gnulib thread.  When the warning
is in a .c file, they are justified in not caring.  But when it is in a
.h file, it really does seem like something worth cleaning up in gnulib.

> Wonder what would happen if someone posted a patch to .gnulib to revert
> the change for the reason that it breaks other platforms that don't
> desire to configure in that manner. Perhaps Eric Blake would have some
> thoughts or additional muscle with the .gnulib maintainers.
> 


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



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

Re: [libvirt] [PATCH 4/9] util: Remove now-unneeded resctrl functions

[John Ferlan]

On 12/13/2017 10:39 AM, Martin Kletzander wrote:
> Signed-off-by: Martin Kletzander 
> ---
>  src/libvirt_private.syms |   2 -
>  src/util/virresctrl.c| 132 
> ---
>  src/util/virresctrl.h|  11 
>  3 files changed, 145 deletions(-)
> 

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH 3/9] conf: Use virResctrlInfo in capabilities

[John Ferlan]

On 12/13/2017 10:39 AM, Martin Kletzander wrote:
> Signed-off-by: Martin Kletzander 
> ---
>  src/conf/capabilities.c | 53 
> -
>  src/conf/capabilities.h |  2 ++
>  2 files changed, 28 insertions(+), 27 deletions(-)
> 

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH 2/9] util: Add virResctrlInfo

[John Ferlan]

On 12/13/2017 10:39 AM, Martin Kletzander wrote:
> This will make the current functions obsolete and it will provide more
> information to the virresctrl module so that it can be used later.
> 
> Signed-off-by: Martin Kletzander 
> ---
>  po/POTFILES.in   |   1 +
>  src/libvirt_private.syms |   3 +
>  src/util/virresctrl.c| 301 
> +++
>  src/util/virresctrl.h|  19 +++
>  4 files changed, 324 insertions(+)
> 
> diff --git a/po/POTFILES.in b/po/POTFILES.in
> index c1fa23427eff..8382ee633621 100644
> --- a/po/POTFILES.in
> +++ b/po/POTFILES.in
> @@ -252,6 +252,7 @@ src/util/virportallocator.c
>  src/util/virprocess.c
>  src/util/virqemu.c
>  src/util/virrandom.c
> +src/util/virresctrl.c
>  src/util/virrotatingfile.c
>  src/util/virscsi.c
>  src/util/virscsihost.c
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index de4ec4d442c9..75be612a2f13 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2550,6 +2550,9 @@ virCacheTypeFromString;
>  virCacheTypeToString;
>  virResctrlGetCacheControlType;
>  virResctrlGetCacheInfo;
> +virResctrlGetInfo;
> +virResctrlInfoGetCache;
> +virResctrlInfoNew;
>  
>  
>  # util/virrotatingfile.h
> diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
> index 050a08178e24..6fd1ceb587cf 100644
> --- a/src/util/virresctrl.c
> +++ b/src/util/virresctrl.c
> @@ -25,12 +25,15 @@
>  #include "viralloc.h"
>  #include "virfile.h"
>  #include "virlog.h"
> +#include "virobject.h"
>  #include "virstring.h"
>  
>  #define VIR_FROM_THIS VIR_FROM_RESCTRL
>  
>  VIR_LOG_INIT("util.virresctrl")
>  
> +
> +/* Common definitions */
>  #define SYSFS_RESCTRL_PATH "/sys/fs/resctrl"
>  
>  /* Resctrl is short for Resource Control.  It might be implemented for 
> various
> @@ -55,6 +58,304 @@ VIR_ENUM_IMPL(virResctrl, VIR_CACHE_TYPE_LAST,
>"CODE",
>"DATA")
>  
> +
> +/* Info-related definitions and InfoClass-related functions */
> +typedef struct _virResctrlInfoPerType virResctrlInfoPerType;
> +typedef virResctrlInfoPerType *virResctrlInfoPerTypePtr;
> +struct _virResctrlInfoPerType {
> +/* Kernel-provided information */
> +char *cbm_mask;
> +unsigned int min_cbm_bits;
> +
> +/* Our computed information from the above */
> +unsigned int bits;
> +unsigned int max_cache_id;
> +
> +/* In order to be self-sufficient we need size information per cache.
> + * Funnily enough, one of the outcomes of the resctrlfs design is that it
> + * does not account for different sizes per cache on the same level.  So
> + * for the sake of easiness, let's copy that, for now. */
> +unsigned long long size;
> +
> +/* Information that we will return upon request (this is public struct) 
> as
> + * until now all the above is internal to this module */
> +virResctrlInfoPerCache control;
> +};
> +
> +typedef struct _virResctrlInfoPerLevel virResctrlInfoPerLevel;
> +typedef virResctrlInfoPerLevel *virResctrlInfoPerLevelPtr;
> +struct _virResctrlInfoPerLevel {
> +virResctrlInfoPerTypePtr *types;
> +};
> +
> +struct _virResctrlInfo {
> +virObject parent;
> +
> +virResctrlInfoPerLevelPtr *levels;
> +size_t nlevels;
> +};
> +
> +static virClassPtr virResctrlInfoClass;
> +
> +static void
> +virResctrlInfoDispose(void *obj)
> +{
> +size_t i = 0;
> +size_t j = 0;
> +
> +virResctrlInfoPtr resctrl = obj;
> +
> +for (i = 0; i < resctrl->nlevels; i++) {
> +virResctrlInfoPerLevelPtr level = resctrl->levels[i];
> +
> +if (!level)
> +continue;
> +
> +if (level->types) {
> +for (j = 0; j < VIR_CACHE_TYPE_LAST; j++) {
> +if (level->types[j])
> +VIR_FREE(level->types[j]->cbm_mask);
> +VIR_FREE(level->types[j]);
> +}
> +}
> +VIR_FREE(level->types);
> +VIR_FREE(level);
> +}
> +
> +VIR_FREE(resctrl->levels);
> +}
> +
> +
> +static int virResctrlInfoOnceInit(void)

static int
virRes

> +{
> +if (!(virResctrlInfoClass = virClassNew(virClassForObject(),
> +"virResctrlInfo",
> +sizeof(virResctrlInfo),
> +virResctrlInfoDispose)))
> +return -1;
> +
> +return 0;
> +}
> +
> +
> +VIR_ONCE_GLOBAL_INIT(virResctrlInfo)
> +
> +
> +virResctrlInfoPtr
> +virResctrlInfoNew(void)
> +{
> +if (virResctrlInfoInitialize() < 0)
> +return NULL;
> +
> +return virObjectNew(virResctrlInfoClass);
> +}
> +
> +
> +/* Info-related functions */
> +bool
> +virResctrlInfoIsEmpty(virResctrlInfoPtr resctrl)
> +{
> +size_t i = 0;
> +size_t j = 0;
> +
> +if (!resctrl)
> +return true;
> +
> +for (i = 0; i < resctrl->nlevels; i++) {
> +virResctrlInfoPerLevelPtr i_level = resctrl->levels[i];
> +
> + 

Re: [libvirt] [PATCH 1/9] Rename virResctrlInfo to virResctrlInfoPerCache

[John Ferlan]

On 12/13/2017 10:39 AM, Martin Kletzander wrote:
> Just to ease the review of following patches.
> 
> Signed-off-by: Martin Kletzander 
> ---
>  src/conf/capabilities.c | 2 +-
>  src/conf/capabilities.h | 2 +-
>  src/util/virresctrl.c   | 4 ++--
>  src/util/virresctrl.h   | 8 
>  4 files changed, 8 insertions(+), 8 deletions(-)
> 

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH] storage: Fixing missing 'backingStore' tag from volume XML dumps.

[Julio Faracco]

Hi guys,

Any possibility to include a test case for this scenario?

2018-01-02 16:52 GMT-02:00 Julio Faracco :
> After commit a693fdb 'vol-dumpxml' missed the ability to show backingStore
> information. This commit adds a volume type for files that fixes this
> problem.
>
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1529663
>
> Signed-off-by: Julio Faracco 
> ---
>  src/storage/storage_util.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c
> index 899a557..9e1b63a 100644
> --- a/src/storage/storage_util.c
> +++ b/src/storage/storage_util.c
> @@ -1957,6 +1957,8 @@ 
> virStorageBackendUpdateVolTargetInfoFD(virStorageSourcePtr target,
>  target->timestamps->ctime = get_stat_ctime(sb);
>  target->timestamps->mtime = get_stat_mtime(sb);
>
> +target->type = VIR_STORAGE_TYPE_FILE;
> +
>  VIR_FREE(target->perms->label);
>
>  #if WITH_SELINUX
> --
> 2.7.4
>

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

[libvirt] [PATCH] storage: Fixing missing 'backingStore' tag from volume XML dumps.

[Julio Faracco]

After commit a693fdb 'vol-dumpxml' missed the ability to show backingStore
information. This commit adds a volume type for files that fixes this
problem.

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

Signed-off-by: Julio Faracco 
---
 src/storage/storage_util.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c
index 899a557..9e1b63a 100644
--- a/src/storage/storage_util.c
+++ b/src/storage/storage_util.c
@@ -1957,6 +1957,8 @@ 
virStorageBackendUpdateVolTargetInfoFD(virStorageSourcePtr target,
 target->timestamps->ctime = get_stat_ctime(sb);
 target->timestamps->mtime = get_stat_mtime(sb);
 
+target->type = VIR_STORAGE_TYPE_FILE;
+
 VIR_FREE(target->perms->label);
 
 #if WITH_SELINUX
-- 
2.7.4

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

Re: [libvirt] [PATCH 4/5] qemu: implement the new virDomainQemuReconnect method

[John Ferlan]

On 12/20/2017 11:47 AM, Daniel P. Berrange wrote:
> Signed-off-by: Daniel P. Berrange 
> ---
>  src/qemu/qemu_driver.c  | 57 
> +
>  src/qemu/qemu_process.c | 31 ++-
>  src/qemu/qemu_process.h |  1 +
>  3 files changed, 79 insertions(+), 10 deletions(-)
> 

It feels like there's two separate patches here as the qemu_process
stuff seems separable.

Of course it dawns on me now... Should we go with a QemuConnect type
naming scheme? Usage of Reconnect would seem to assume we lost
connection at some point because libvirtd was stopped or crashed. Of
course Attach would have been "optimal", but that's already used for
something else.

Perhaps the "proof" that things can work is to be able to have the
process reconnect logic and this logic intersect. Theoretically they are
doing the same thing in the long run at least with respect to being able
to connect to an existing qemu process.

> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 97b194b057..fea1f24250 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -16300,6 +16300,62 @@ static virDomainPtr 
> qemuDomainQemuAttach(virConnectPtr conn,
>  }
>  
>  
> +static virDomainPtr qemuDomainQemuReconnect(virConnectPtr conn,
> +const char *name,
> +unsigned int flags)

static virDomainPtr
qemuDomainQemuReconnect(...)

> +{
> +virQEMUDriverPtr driver = conn->privateData;
> +virDomainObjPtr vm = NULL;
> +virDomainPtr dom = NULL;
> +virCapsPtr caps = NULL;
> +virQEMUDriverConfigPtr cfg;
> +
> +virCheckFlags(0, NULL);
> +
> +cfg = virQEMUDriverGetConfig(driver);
> +
> +if (strchr(name, '/')) {
> +virReportError(VIR_ERR_XML_ERROR,
> +   _("name %s cannot contain '/'"), name);
> +goto cleanup;
> +}
> +
> +vm = virDomainObjListFindByName(driver->domains, name);
> +if (vm) {
> +virReportError(VIR_ERR_OPERATION_INVALID,
> +   _("Domain '%s' already exists"), name);
> +goto cleanup;
> +}
> +
> +if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
> +goto cleanup;
> +
> +if (!(vm = virDomainObjListLoadStatus(driver->domains,
> +  cfg->stateDir,
> +  name,
> +  caps,
> +  driver->xmlopt,
> +  NULL, NULL))) {
> +goto cleanup;
> +}

This would assume that stateDir has a file @name.xml, right? IOW: shim
processing creates it. I only mention it as a "one extra sanity" type
check that we may want to make; otherwise, we could get some sort of
less obvious error...  I didn't do much digging to prove/disprove.

> +
> +if (virDomainQemuReconnectEnsureACL(conn, vm->def) < 0) {
> +qemuDomainRemoveInactive(driver, vm);
> +goto cleanup;
> +}
> +
> +dom = virGetDomain(conn, vm->def->name, vm->def->uuid, vm->def->id);
> +
> +qemuDomainObjEndJob(driver, vm);

When did we Begin the job?   I assume that this is borrowing logic from
qemuProcessReconnect and this may just be "leftover".

> +
> + cleanup:
> +virDomainObjEndAPI();
> +virObjectUnref(caps);
> +virObjectUnref(cfg);
> +return dom;
> +}
> +
> +
>  static int
>  qemuDomainOpenConsole(virDomainPtr dom,
>const char *dev_name,
> @@ -21262,6 +21318,7 @@ static virHypervisorDriver qemuHypervisorDriver = {
>  .domainSnapshotDelete = qemuDomainSnapshotDelete, /* 0.8.0 */
>  .domainQemuMonitorCommand = qemuDomainQemuMonitorCommand, /* 0.8.3 */
>  .domainQemuAttach = qemuDomainQemuAttach, /* 0.9.4 */
> +.domainQemuReconnect = qemuDomainQemuReconnect, /* 4.1.0 */
>  .domainQemuAgentCommand = qemuDomainQemuAgentCommand, /* 0.10.0 */
>  .connectDomainQemuMonitorEventRegister = 
> qemuConnectDomainQemuMonitorEventRegister, /* 1.2.3 */
>  .connectDomainQemuMonitorEventDeregister = 
> qemuConnectDomainQemuMonitorEventDeregister, /* 1.2.3 */
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index a0f430f89f..ea924cf9b6 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -7088,14 +7088,10 @@ struct qemuProcessReconnectData {
>   * We can't do normal MonitorEnter & MonitorExit because these two lock the
>   * monitor lock, which does not exists in this early phase.
>   */
> -static void
> -qemuProcessReconnect(void *opaque)
> +int
> +qemuProcessReconnect(virQEMUDriverPtr driver, virDomainObjPtr obj, 
> virConnectPtr conn)

One argument per line...

>  {
> -struct qemuProcessReconnectData *data = opaque;
> -virQEMUDriverPtr driver = data->driver;
> -virDomainObjPtr obj = data->obj;
>  qemuDomainObjPrivatePtr priv;
> -virConnectPtr 

[libvirt] [PATCH 14/18] vsh: Introduce complete command

[Michal Privoznik]

This command is going to be called from bash completion script in
the following form:

  virsh complete -- start --domain

Its only purpose is to return list of possible strings for
completion. Note that this is a 'hidden', unlisted command and
therefore there's no documentation to it.

Signed-off-by: Michal Privoznik 
---
 tools/virsh.c  |  1 +
 tools/virt-admin.c |  1 +
 tools/vsh.c| 80 ++
 tools/vsh.h| 14 ++
 4 files changed, 96 insertions(+)

diff --git a/tools/virsh.c b/tools/virsh.c
index 933d6b4c9..5f8352e86 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -833,6 +833,7 @@ static const vshCmdDef virshCmds[] = {
 VSH_CMD_PWD,
 VSH_CMD_QUIT,
 VSH_CMD_SELF_TEST,
+VSH_CMD_COMPLETE,
 {.name = "connect",
  .handler = cmdConnect,
  .opts = opts_connect,
diff --git a/tools/virt-admin.c b/tools/virt-admin.c
index b8b33af19..ef5bada63 100644
--- a/tools/virt-admin.c
+++ b/tools/virt-admin.c
@@ -1351,6 +1351,7 @@ static const vshCmdDef vshAdmCmds[] = {
 VSH_CMD_PWD,
 VSH_CMD_QUIT,
 VSH_CMD_SELF_TEST,
+VSH_CMD_COMPLETE,
 {.name = "uri",
  .handler = cmdURI,
  .opts = NULL,
diff --git a/tools/vsh.c b/tools/vsh.c
index f061783e0..9f4805b3d 100644
--- a/tools/vsh.c
+++ b/tools/vsh.c
@@ -3472,3 +3472,83 @@ cmdSelfTest(vshControl *ctl ATTRIBUTE_UNUSED,
 
 return true;
 }
+
+/* --
+ * Autocompletion command
+ * -- */
+
+const vshCmdOptDef opts_complete[] = {
+{.name = "string",
+ .type = VSH_OT_ARGV,
+ .flags = VSH_OFLAG_EMPTY_OK,
+ .help = N_("partial string to autocomplete")
+},
+{.name = NULL}
+};
+
+const vshCmdInfo info_complete[] = {
+{.name = "help",
+ .data = N_("internal command for autocompletion")
+},
+{.name = "desc",
+ .data = N_("internal use only")
+},
+{.name = NULL}
+};
+
+bool
+cmdComplete(vshControl *ctl, const vshCmd *cmd)
+{
+bool ret = false;
+#ifdef WITH_READLINE
+const vshClientHooks *hooks = ctl->hooks;
+int stdin_fileno = STDIN_FILENO;
+const char *arg = "";
+const vshCmdOpt *opt = NULL;
+char **matches = NULL, **iter;
+virBuffer buf = VIR_BUFFER_INITIALIZER;
+
+if (vshCommandOptStringQuiet(ctl, cmd, "string", ) <= 0)
+goto cleanup;
+
+/* This command is flagged VSH_CMD_FLAG_NOCONNECT because we
+ * need to prevent auth hooks reading any input. Therefore, we
+ * have to close stdin and then connect ourselves. */
+VIR_FORCE_CLOSE(stdin_fileno);
+
+if (!(hooks && hooks->connHandler && hooks->connHandler(ctl)))
+goto cleanup;
+
+while ((opt = vshCommandOptArgv(ctl, cmd, opt))) {
+if (virBufferUse() != 0)
+virBufferAddChar(, ' ');
+virBufferAddStr(, opt->data);
+arg = opt->data;
+}
+
+if (virBufferCheckError() < 0)
+goto cleanup;
+
+vshReadlineInit(ctl);
+
+if (!(rl_line_buffer = virBufferContentAndReset()) &&
+VIR_STRDUP(rl_line_buffer, "") < 0)
+goto cleanup;
+
+/* rl_point is current cursor position in rl_line_buffer.
+ * In our case it's at the end of the whole line. */
+rl_point = strlen(rl_line_buffer);
+
+if (!(matches = vshReadlineCompletion(arg, 0, 0)))
+goto cleanup;
+
+for (iter = matches; *iter; iter++)
+printf("%s\n", *iter);
+
+ret = true;
+ cleanup:
+virBufferFreeAndReset();
+virStringListFree(matches);
+#endif /* WITH_READLINE */
+return ret;
+}
diff --git a/tools/vsh.h b/tools/vsh.h
index ae40fb4e8..6894700d9 100644
--- a/tools/vsh.h
+++ b/tools/vsh.h
@@ -382,6 +382,8 @@ extern const vshCmdInfo info_echo[];
 extern const vshCmdInfo info_pwd[];
 extern const vshCmdInfo info_quit[];
 extern const vshCmdInfo info_selftest[];
+extern const vshCmdOptDef opts_complete[];
+extern const vshCmdInfo info_complete[];
 
 bool cmdHelp(vshControl *ctl, const vshCmd *cmd);
 bool cmdCd(vshControl *ctl, const vshCmd *cmd);
@@ -389,6 +391,7 @@ bool cmdEcho(vshControl *ctl, const vshCmd *cmd);
 bool cmdPwd(vshControl *ctl, const vshCmd *cmd);
 bool cmdQuit(vshControl *ctl, const vshCmd *cmd);
 bool cmdSelfTest(vshControl *ctl, const vshCmd *cmd);
+bool cmdComplete(vshControl *ctl, const vshCmd *cmd);
 
 # define VSH_CMD_CD \
 { \
@@ -454,6 +457,17 @@ bool cmdSelfTest(vshControl *ctl, const vshCmd *cmd);
 .alias = "self-test" \
 }
 
+# define VSH_CMD_COMPLETE \
+{ \
+.name = "complete", \
+.handler = cmdComplete, \
+.opts = opts_complete, \
+.info = info_complete, \
+.flags = VSH_CMD_FLAG_NOCONNECT | VSH_CMD_FLAG_ALIAS, \
+.alias = "complete" \
+}
+
+
 
 /* readline */
 char * vshReadline(vshControl *ctl, const char *prompt);
-- 
2.13.6

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

[libvirt] [PATCH 16/18] virsh: Introduce virshDomainNameCompleter

[Michal Privoznik]

Now that we have everything prepared let the fun begin. This
completer is very simple and returns domain names. Moreover,
depending on the command it can return just a subset of domains
(e.g. only running/paused/transient/.. ones).

Signed-off-by: Michal Privoznik 
---
 tools/Makefile.am|   9 +++
 tools/virsh-completer.c  |  90 +
 tools/virsh-completer.h  |  33 
 tools/virsh-domain-monitor.c |  30 +++
 tools/virsh-domain.c | 182 ++-
 tools/virsh-snapshot.c   |  24 +++---
 tools/virsh.h|   7 +-
 7 files changed, 256 insertions(+), 119 deletions(-)
 create mode 100644 tools/virsh-completer.c
 create mode 100644 tools/virsh-completer.h

diff --git a/tools/Makefile.am b/tools/Makefile.am
index 1df2a601f..7466d8282 100644
--- a/tools/Makefile.am
+++ b/tools/Makefile.am
@@ -231,6 +231,15 @@ virsh_SOURCES = \
virsh-volume.c virsh-volume.h \
$(NULL)
 
+VIRSH_COMPLETER = \
+   virsh-completer.c virsh-completer.h
+
+if WITH_READLINE
+virsh_SOURCES += $(VIRSH_COMPLETER)
+else ! WITH_READLINE
+EXTRA_DIST += $(VIRSH_COMPLETER)
+endif ! WITH_READLINE
+
 virsh_LDFLAGS = \
$(AM_LDFLAGS) \
$(PIE_LDFLAGS) \
diff --git a/tools/virsh-completer.c b/tools/virsh-completer.c
new file mode 100644
index 0..4e32b882b
--- /dev/null
+++ b/tools/virsh-completer.c
@@ -0,0 +1,90 @@
+/*
+ * virsh-completer.c: virsh completer callbacks
+ *
+ * Copyright (C) 2017 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * .
+ *
+ *  Michal Privoznik 
+ *
+ */
+
+#include 
+
+#include "virsh-completer.h"
+#include "virsh.h"
+#include "virsh-util.h"
+#include "internal.h"
+#include "viralloc.h"
+#include "virstring.h"
+
+
+char **
+virshDomainNameCompleter(vshControl *ctl,
+ const vshCmd *cmd ATTRIBUTE_UNUSED,
+ unsigned int flags)
+{
+virshControlPtr priv = ctl->privData;
+virDomainPtr *domains = NULL;
+int ndomains = 0;
+size_t i = 0;
+char **ret = NULL;
+
+virCheckFlags(VIR_CONNECT_LIST_DOMAINS_ACTIVE |
+  VIR_CONNECT_LIST_DOMAINS_INACTIVE |
+  VIR_CONNECT_LIST_DOMAINS_PERSISTENT |
+  VIR_CONNECT_LIST_DOMAINS_TRANSIENT |
+  VIR_CONNECT_LIST_DOMAINS_RUNNING |
+  VIR_CONNECT_LIST_DOMAINS_PAUSED |
+  VIR_CONNECT_LIST_DOMAINS_SHUTOFF |
+  VIR_CONNECT_LIST_DOMAINS_OTHER |
+  VIR_CONNECT_LIST_DOMAINS_MANAGEDSAVE |
+  VIR_CONNECT_LIST_DOMAINS_NO_MANAGEDSAVE |
+  VIR_CONNECT_LIST_DOMAINS_AUTOSTART |
+  VIR_CONNECT_LIST_DOMAINS_NO_AUTOSTART |
+  VIR_CONNECT_LIST_DOMAINS_HAS_SNAPSHOT |
+  VIR_CONNECT_LIST_DOMAINS_NO_SNAPSHOT,
+  NULL);
+
+if (!priv->conn || virConnectIsAlive(priv->conn) <= 0)
+return NULL;
+
+if ((ndomains = virConnectListAllDomains(priv->conn, , flags)) < 0)
+return NULL;
+
+if (VIR_ALLOC_N(ret, ndomains + 1) < 0)
+goto error;
+
+for (i = 0; i < ndomains; i++) {
+const char *name = virDomainGetName(domains[i]);
+
+if (VIR_STRDUP(ret[i], name) < 0)
+goto error;
+
+virshDomainFree(domains[i]);
+}
+VIR_FREE(domains);
+
+return ret;
+ error:
+
+for (; i < ndomains; i++)
+virshDomainFree(domains[i]);
+VIR_FREE(domains);
+for (i = 0; i < ndomains; i++)
+VIR_FREE(ret[i]);
+VIR_FREE(ret);
+return NULL;
+}
diff --git a/tools/virsh-completer.h b/tools/virsh-completer.h
new file mode 100644
index 0..288e17909
--- /dev/null
+++ b/tools/virsh-completer.h
@@ -0,0 +1,33 @@
+/*
+ * virsh-completer.h: virsh completer callbacks
+ *
+ * Copyright (C) 2017 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied 

[libvirt] [PATCH 15/18] tools: Provide bash autompletion file

[Michal Privoznik]

The only purpose of this file is to be sourced. After that one
can use completion even for their bash:

  # virsh list --
  --all   --inactive ...

Signed-off-by: Michal Privoznik 
---
 configure.ac   |  3 ++
 libvirt.spec.in|  3 ++
 m4/virt-bash-completion.m4 | 74 ++
 tools/Makefile.am  | 22 --
 tools/bash-completion/vsh  | 67 +
 5 files changed, 167 insertions(+), 2 deletions(-)
 create mode 100644 m4/virt-bash-completion.m4
 create mode 100644 tools/bash-completion/vsh

diff --git a/configure.ac b/configure.ac
index fba4e4bf5..06236fad8 100644
--- a/configure.ac
+++ b/configure.ac
@@ -242,6 +242,7 @@ LIBVIRT_ARG_APPARMOR
 LIBVIRT_ARG_ATTR
 LIBVIRT_ARG_AUDIT
 LIBVIRT_ARG_AVAHI
+LIBVIRT_ARG_BASH_COMPLETION
 LIBVIRT_ARG_BLKID
 LIBVIRT_ARG_CAPNG
 LIBVIRT_ARG_CURL
@@ -278,6 +279,7 @@ LIBVIRT_CHECK_ATOMIC
 LIBVIRT_CHECK_ATTR
 LIBVIRT_CHECK_AUDIT
 LIBVIRT_CHECK_AVAHI
+LIBVIRT_CHECK_BASH_COMPLETION
 LIBVIRT_CHECK_BLKID
 LIBVIRT_CHECK_CAPNG
 LIBVIRT_CHECK_CURL
@@ -976,6 +978,7 @@ LIBVIRT_RESULT_APPARMOR
 LIBVIRT_RESULT_ATTR
 LIBVIRT_RESULT_AUDIT
 LIBVIRT_RESULT_AVAHI
+LIBVIRT_RESULT_BASH_COMPLETION
 LIBVIRT_RESULT_BLKID
 LIBVIRT_RESULT_CAPNG
 LIBVIRT_RESULT_CURL
diff --git a/libvirt.spec.in b/libvirt.spec.in
index 7e1b6a27d..d4ef116b2 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -306,6 +306,7 @@ BuildRequires: xen-devel
 BuildRequires: libxml2-devel
 BuildRequires: libxslt
 BuildRequires: readline-devel
+BuildRequires: bash-completion >= 2.0
 BuildRequires: ncurses-devel
 BuildRequires: gettext
 BuildRequires: libtasn1-devel
@@ -2047,6 +2048,8 @@ exit 0
 %{_datadir}/systemtap/tapset/libvirt_qemu_probes*.stp
 %{_datadir}/systemtap/tapset/libvirt_functions.stp
 
+%{_datadir}/bash-completion/completions/vsh
+
 
 %if %{with_systemd}
 %{_unitdir}/libvirt-guests.service
diff --git a/m4/virt-bash-completion.m4 b/m4/virt-bash-completion.m4
new file mode 100644
index 0..e1ef58740
--- /dev/null
+++ b/m4/virt-bash-completion.m4
@@ -0,0 +1,74 @@
+dnl Bash completion support
+dnl
+dnl Copyright (C) 2017 Red Hat, Inc.
+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 .
+dnl
+dnl Inspired by libguestfs code.
+dnl
+
+AC_DEFUN([LIBVIRT_ARG_BASH_COMPLETION],[
+  LIBVIRT_ARG_WITH_FEATURE([BASH_COMPLETION], [bash-completion], [check], 
[2.0])
+  LIBVIRT_ARG_WITH([BASH_COMPLETIONS_DIR],
+   [directory containing bash completions scripts],
+   [check])
+])
+
+AC_DEFUN([LIBVIRT_CHECK_BASH_COMPLETION], [
+  AC_REQUIRE([LIBVIRT_CHECK_READLINE])
+
+  if test "x$with_readline" != "xyes" ; then
+if test "x$with_bash_completion" != "xyes" ; then
+  with_bash_completion=no
+else
+  AC_MSG_ERROR([readline is required for bash completion support])
+fi
+  else
+if test "x$with_bash_completion" = "xcheck" ; then
+  with_bash_completion=yes
+fi
+  fi
+
+  LIBVIRT_CHECK_PKG([BASH_COMPLETION], [bash-completion], [2.0])
+
+  if test "x$with_bash_completion" = "xyes" ; then
+if test "x$with_bash_completions_dir" = "xcheck"; then
+  AC_MSG_CHECKING([for bash-completions directory])
+  BASH_COMPLETIONS_DIR="$($PKG_CONFIG --variable=completionsdir 
bash-completion)"
+  AC_MSG_RESULT([$BASH_COMPLETIONS_DIR])
+
+  dnl Replace bash completions's exec_prefix with our own.
+  dnl Note that ${exec_prefix} is kept verbatim at this point in time,
+  dnl and will only be expanded later, when make is called: this makes
+  dnl it possible to override such prefix at compilation or installation
+  dnl time
+  bash_completions_prefix="$($PKG_CONFIG --variable=prefix 
bash-completion)"
+  if test "x$bash_completions_prefix" = "x" ; then
+bash_completions_prefix="/usr"
+  fi
+
+  
BASH_COMPLETIONS_DIR='${exec_prefix}'"${BASH_COMPLETIONS_DIR#$bash_completions_prefix}"
+elif test "x$with_bash_completions_dir" = "xno" || test 
"x$with_bash_completions_dir" = "xyes"; then
+  AC_MSG_ERROR([bash-completions-dir must be used only with valid path])
+else
+  BASH_COMPLETIONS_DIR=$with_bash_completions_dir
+fi
+AC_SUBST([BASH_COMPLETIONS_DIR])
+  fi
+])
+
+AC_DEFUN([LIBVIRT_RESULT_BASH_COMPLETION],[
+  

[libvirt] [PATCH 17/18] virsh: Introduce virshDomainInterfaceCompleter

[Michal Privoznik]

For given domain fetch list of defined interfaces. This can be
used for commands like domif-getlink and others. If available,
the interface name is returned (e.g. "vnet0", usually available
only for running domains), if not the MAC address is returned.
Moreover, the detach-interface command requires only MAC address
and therefore we have new flag that forces the completer to
return just the MAC address.

Signed-off-by: Michal Privoznik 
---
 tools/virsh-completer.c  | 60 
 tools/virsh-completer.h  |  8 ++
 tools/virsh-domain-monitor.c |  3 +++
 tools/virsh-domain.c |  4 +++
 4 files changed, 75 insertions(+)

diff --git a/tools/virsh-completer.c b/tools/virsh-completer.c
index 4e32b882b..25c3aaa0d 100644
--- a/tools/virsh-completer.c
+++ b/tools/virsh-completer.c
@@ -29,6 +29,7 @@
 #include "internal.h"
 #include "viralloc.h"
 #include "virstring.h"
+#include "virxml.h"
 
 
 char **
@@ -88,3 +89,62 @@ virshDomainNameCompleter(vshControl *ctl,
 VIR_FREE(ret);
 return NULL;
 }
+
+
+char **
+virshDomainInterfaceCompleter(vshControl *ctl,
+  const vshCmd *cmd,
+  unsigned int flags)
+{
+virshControlPtr priv = ctl->privData;
+xmlDocPtr xmldoc = NULL;
+xmlXPathContextPtr ctxt = NULL;
+int ninterfaces;
+xmlNodePtr *interfaces = NULL;
+size_t i;
+unsigned int domainXMLFlags = 0;
+char **ret = NULL;
+
+virCheckFlags(VIRSH_DOMAIN_INTERFACE_COMPLETER_MAC, NULL);
+
+if (!priv->conn || virConnectIsAlive(priv->conn) <= 0)
+return NULL;
+
+if (vshCommandOptBool(cmd, "config"))
+domainXMLFlags = VIR_DOMAIN_XML_INACTIVE;
+
+if (virshDomainGetXML(ctl, cmd, domainXMLFlags, , ) < 0)
+goto error;
+
+ninterfaces = virXPathNodeSet("./devices/interface", ctxt, );
+if (ninterfaces < 0)
+goto error;
+
+if (VIR_ALLOC_N(ret, ninterfaces + 1) < 0)
+goto error;
+
+for (i = 0; i < ninterfaces; i++) {
+ctxt->node = interfaces[i];
+
+if (!(flags & VIRSH_DOMAIN_INTERFACE_COMPLETER_MAC) &&
+(ret[i] = virXPathString("string(./target/@dev)", ctxt)))
+continue;
+
+/* In case we are dealing with inactive domain XML there's no
+ * . Offer MAC addresses then. */
+if (!(ret[i] = virXPathString("string(./mac/@address)", ctxt)))
+goto error;
+}
+
+VIR_FREE(interfaces);
+xmlFreeDoc(xmldoc);
+xmlXPathFreeContext(ctxt);
+return ret;
+
+ error:
+VIR_FREE(interfaces);
+xmlFreeDoc(xmldoc);
+xmlXPathFreeContext(ctxt);
+virStringListFree(ret);
+return NULL;
+}
diff --git a/tools/virsh-completer.h b/tools/virsh-completer.h
index 288e17909..680cd12ff 100644
--- a/tools/virsh-completer.h
+++ b/tools/virsh-completer.h
@@ -30,4 +30,12 @@ char ** virshDomainNameCompleter(vshControl *ctl,
  const vshCmd *cmd,
  unsigned int flags);
 
+enum {
+VIRSH_DOMAIN_INTERFACE_COMPLETER_MAC = 1 << 1, /* Return just MACs */
+};
+
+char ** virshDomainInterfaceCompleter(vshControl *ctl,
+  const vshCmd *cmd,
+  unsigned int flags);
+
 #endif
diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
index a09eb010c..32a42707e 100644
--- a/tools/virsh-domain-monitor.c
+++ b/tools/virsh-domain-monitor.c
@@ -659,6 +659,7 @@ static const vshCmdOptDef opts_domif_getlink[] = {
 {.name = "interface",
  .type = VSH_OT_DATA,
  .flags = VSH_OFLAG_REQ,
+ .completer = virshDomainInterfaceCompleter,
  .help = N_("interface device (MAC Address)")
 },
 {.name = "persistent",
@@ -995,6 +996,7 @@ static const vshCmdOptDef opts_domifstat[] = {
 {.name = "interface",
  .type = VSH_OT_DATA,
  .flags = VSH_OFLAG_REQ,
+ .completer = virshDomainInterfaceCompleter,
  .help = N_("interface device specified by name or MAC Address")
 },
 {.name = NULL}
@@ -2149,6 +2151,7 @@ static const vshCmdOptDef opts_domifaddr[] = {
 {.name = "interface",
  .type = VSH_OT_STRING,
  .flags = VSH_OFLAG_NONE,
+ .completer = virshDomainInterfaceCompleter,
  .help = N_("network interface name")},
 {.name = "full",
  .type = VSH_OT_BOOL,
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 13f8db3dd..0f329d6d7 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -2977,6 +2977,7 @@ static const vshCmdOptDef opts_domif_setlink[] = {
 {.name = "interface",
  .type = VSH_OT_DATA,
  .flags = VSH_OFLAG_REQ,
+ .completer = virshDomainInterfaceCompleter,
  .help = N_("interface device (MAC Address)")
 },
 {.name = "state",
@@ -3147,6 +3148,7 @@ static const vshCmdOptDef opts_domiftune[] = {
 {.name = "interface",
  .type = VSH_OT_DATA,
  .flags = VSH_OFLAG_REQ,
+ 

[libvirt] [PATCH 18/18] virt-admin: Introduce vshAdmServerCompleter

[Michal Privoznik]

Signed-off-by: Michal Privoznik 
---
 tools/Makefile.am|  9 ++
 tools/virt-admin-completer.c | 76 
 tools/virt-admin-completer.h | 33 +++
 tools/virt-admin.c   |  8 +
 4 files changed, 126 insertions(+)
 create mode 100644 tools/virt-admin-completer.c
 create mode 100644 tools/virt-admin-completer.h

diff --git a/tools/Makefile.am b/tools/Makefile.am
index 7466d8282..48125f516 100644
--- a/tools/Makefile.am
+++ b/tools/Makefile.am
@@ -258,6 +258,15 @@ virt_admin_SOURCES = \
virt-admin.c virt-admin.h \
$(NULL)
 
+VIRT_ADMIN_COMPLETER = \
+   virt-admin-completer.c virt-admin-completer.h
+
+if WITH_READLINE
+virt_admin_SOURCES += $(VIRT_ADMIN_COMPLETER)
+else ! WITH_READLINE
+EXTRA_DIST += $(VIRT_ADMIN_COMPLETER)
+endif ! WITH_READLINE
+
 virt_admin_LDFLAGS = \
$(AM_LDFLAGS) \
$(COVERAGE_LDFLAGS) \
diff --git a/tools/virt-admin-completer.c b/tools/virt-admin-completer.c
new file mode 100644
index 0..2cd471f32
--- /dev/null
+++ b/tools/virt-admin-completer.c
@@ -0,0 +1,76 @@
+/*
+ * virt-admin-completer.c: virt-admin completer callbacks
+ *
+ * Copyright (C) 2017 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * .
+ *
+ *  Michal Privoznik 
+ *
+ */
+
+#include 
+
+#include "virt-admin-completer.h"
+#include "internal.h"
+#include "virt-admin.h"
+#include "viralloc.h"
+#include "virstring.h"
+
+
+char **
+vshAdmServerCompleter(vshControl *ctl,
+  const vshCmd *cmd ATTRIBUTE_UNUSED,
+  unsigned int flags)
+{
+vshAdmControlPtr priv = ctl->privData;
+virAdmServerPtr *srvs = NULL;
+int nsrvs = 0;
+size_t i = 0;
+char **ret = NULL;
+
+virCheckFlags(0, NULL);
+
+if (!priv->conn || virAdmConnectIsAlive(priv->conn) <= 0)
+return NULL;
+
+/* Obtain a list of available servers on the daemon */
+if ((nsrvs = virAdmConnectListServers(priv->conn, , 0)) < 0)
+return NULL;
+
+if (VIR_ALLOC_N(ret, nsrvs + 1) < 0)
+goto error;
+
+for (i = 0; i < nsrvs; i++) {
+const char *name = virAdmServerGetName(srvs[i]);
+
+if (VIR_STRDUP(ret[i], name) < 0)
+goto error;
+
+virAdmServerFree(srvs[i]);
+}
+VIR_FREE(srvs);
+
+return ret;
+
+ error:
+for (; i < nsrvs; i++)
+virAdmServerFree(srvs[i]);
+VIR_FREE(srvs);
+for (i = 0; i < nsrvs; i++)
+VIR_FREE(ret[i]);
+VIR_FREE(ret);
+return ret;
+}
diff --git a/tools/virt-admin-completer.h b/tools/virt-admin-completer.h
new file mode 100644
index 0..7507b95c1
--- /dev/null
+++ b/tools/virt-admin-completer.h
@@ -0,0 +1,33 @@
+/*
+ * virt-admin-completer.h: virt-admin completer callbacks
+ *
+ * Copyright (C) 2017 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * .
+ *
+ *  Michal Privoznik 
+ *
+ */
+
+#ifndef VIRT_ADMIN_COMPLETER
+# define VIRT_ADMIN_COMPLETER
+
+# include "vsh.h"
+
+char **
+vshAdmServerCompleter(vshControl *ctl,
+  const vshCmd *cmd,
+  unsigned int flags);
+#endif
diff --git a/tools/virt-admin.c b/tools/virt-admin.c
index ef5bada63..c86b5763a 100644
--- a/tools/virt-admin.c
+++ b/tools/virt-admin.c
@@ -39,6 +39,7 @@
 #include "virthread.h"
 #include "virgettext.h"
 #include "virtime.h"
+#include "virt-admin-completer.h"
 
 /* Gnulib doesn't guarantee SA_SIGINFO support.  */
 #ifndef SA_SIGINFO
@@ -428,6 +429,7 @@ static const vshCmdOptDef opts_srv_threadpool_info[] = {
 {.name = "server",
  .type = VSH_OT_DATA,
  .flags = 

[libvirt] [PATCH 05/18] vshReadlineParse: Drop code duplication

[Michal Privoznik]

Now that we have a way of retrieving partly parsed command we
don't need duplicate code that parses the user's input.

Yes, this code removes call of opt's completer, but:
  a) current implementation is broken anyway, and
  b) it will be added back shortly

Signed-off-by: Michal Privoznik 
---
 tools/vsh.c | 214 +++-
 1 file changed, 24 insertions(+), 190 deletions(-)

diff --git a/tools/vsh.c b/tools/vsh.c
index 34eb592ef..c3423d6e3 100644
--- a/tools/vsh.c
+++ b/tools/vsh.c
@@ -2697,216 +2697,50 @@ vshReadlineOptionsGenerator(const char *text, int 
state, const vshCmdDef *cmd_pa
 static char *
 vshReadlineParse(const char *text, int state)
 {
-static vshCommandParser parser, sanitizer;
-vshCommandToken tk;
+static vshCmd *partial;
 static const vshCmdDef *cmd;
-const vshCmdOptDef *opt = NULL;
-char *tkdata, *optstr, *const_tkdata, *completed_name;
 char *res = NULL;
-static char *ctext, *sanitized_text;
-static char **completed_list;
-static unsigned int completed_list_index;
-static uint64_t const_opts_need_arg, const_opts_required, const_opts_seen;
-uint64_t opts_seen;
-size_t opt_index;
-static bool cmd_exists, opts_filled, opt_exists;
-static bool non_bool_opt_exists, data_complete;
 
 if (!state) {
-parser.pos = rl_line_buffer;
-parser.getNextArg = vshCommandStringGetArg;
-
-ctext = vshStrdup(NULL, text);
-sanitizer.pos = ctext;
-sanitizer.getNextArg = vshCommandStringGetArg;
-
-const_tkdata = NULL;
-tkdata = NULL;
-sanitized_text = NULL;
-optstr = NULL;
-
-completed_list = NULL;
-completed_list_index = 0;
-
-/* Sanitize/de-quote the autocomplete text */
-tk = sanitizer.getNextArg(NULL, , _text, false);
-
-/* No autocomplete if sanitized text is a token error or token end */
-if (tk == VSH_TK_ERROR)
-goto error;
-
-tk = parser.getNextArg(NULL, , _tkdata, false);
-
-if (tk == VSH_TK_ERROR)
-goto error;
-
-/* Free-ing purposes */
-tkdata = const_tkdata;
-/* Skip leading space */
-virSkipSpaces((const char**));
-
-/* Handle ';'s */
-while (tk == VSH_TK_SUBCMD_END) {
-tk = parser.getNextArg(NULL, , _tkdata, false);
-tkdata = const_tkdata;
-}
-
-/* Skip trailing space after ;*/
-virSkipSpaces((const char**));
-
-cmd_exists = false;
-opts_filled = false;
-non_bool_opt_exists = false;
-data_complete = false;
-
-const_opts_need_arg = 0;
-const_opts_required = 0;
-const_opts_seen = 0;
-
-opt_index = 0;
+char *buf = vshStrdup(NULL, rl_line_buffer);
 
+vshCommandFree(partial);
+partial = NULL;
 cmd = NULL;
-opt = NULL;
 
-/* Parse till text to be auto-completed is reached */
-while (STRNEQ(tkdata, sanitized_text)) {
-if (!cmd) {
-if (!(cmd = vshCmddefSearch(tkdata)))
-goto error;
-if (cmd->flags & VSH_CMD_FLAG_ALIAS)
-cmd = vshCmddefSearch(cmd->alias);
+*(buf + rl_point) = '\0';
 
-cmd_exists = true;
-if (vshCmddefOptParse(cmd, _opts_need_arg,
-  _opts_required) < 0)
-goto error;
-opts_seen = const_opts_seen;
-opts_filled = true;
-} else if (tkdata[0] == '-' && tkdata[1] == '-' &&
-   c_isalnum(tkdata[2])) {
-/* Command retrieved successfully, move to options */
-optstr = strchr(tkdata + 2, '=');
-opt_index = 0;
+vshCommandStringParse(NULL, buf, );
 
-if (optstr) {
-*optstr = '\0';
-optstr = vshStrdup(NULL, optstr + 1);
-}
+VIR_FREE(buf);
 
-if (!(opt = vshCmddefGetOption(NULL, cmd, tkdata + 2,
-   _seen, _index,
-   , false))) {
-/* Parsing failed wrt autocomplete */
-VIR_FREE(optstr);
-goto error;
-}
+if (partial)
+cmd = partial->def;
 
-opts_seen = const_opts_seen;
-opt_exists = true;
-VIR_FREE(const_tkdata);
-if (opt->type != VSH_OT_BOOL) {
-non_bool_opt_exists = true;
-/* Opt exists and check for option data */
-if (optstr) {
-const_tkdata = optstr;
-tkdata = const_tkdata;
-} else {
-VIR_FREE(const_tkdata);
- 

[libvirt] [PATCH 13/18] vsh: Filter --options

[Michal Privoznik]

Similarly to the previous commit, once we've presented an
--option for a command to the user it makes no sense to offer it
again. Therefore, we can prune all already specified options. For
instance, after this patch:

  virsh # migrate --verbose 

will no longer offer --verbose option.

Signed-off-by: Michal Privoznik 
---
 tools/vsh.c | 60 +++-
 1 file changed, 59 insertions(+), 1 deletion(-)

diff --git a/tools/vsh.c b/tools/vsh.c
index 49e8033bd..f061783e0 100644
--- a/tools/vsh.c
+++ b/tools/vsh.c
@@ -2769,6 +2769,60 @@ 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)
 {
@@ -2817,9 +2871,13 @@ vshReadlineParse(const char *text, int state)
 if (!cmd) {
 list = vshReadlineCommandGenerator(text);
 } else {
-if (!opt || opt->type != VSH_OT_DATA)
+if (!opt || opt->type != VSH_OT_DATA) {
 list = vshReadlineOptionsGenerator(text, cmd);
 
+if (vshReadlineOptionsPrune(, partial) < 0)
+goto cleanup;
+}
+
 if (opt && opt->completer) {
 char **completer_list = opt->completer(autoCompleteOpaque,
partial,
-- 
2.13.6

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

[libvirt] [PATCH 09/18] util: Introduce virStringListMerge

[Michal Privoznik]

For two string lists merge one into the other one.

Signed-off-by: Michal Privoznik 
---
 src/libvirt_private.syms |  1 +
 src/util/virstring.c | 36 
 src/util/virstring.h |  3 +++
 3 files changed, 40 insertions(+)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index d5c3b9abb..d807cdca6 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2764,6 +2764,7 @@ virStringListGetFirstWithPrefix;
 virStringListHasString;
 virStringListJoin;
 virStringListLength;
+virStringListMerge;
 virStringListRemove;
 virStringMatch;
 virStringParsePort;
diff --git a/src/util/virstring.c b/src/util/virstring.c
index b2ebce27f..47d9efba2 100644
--- a/src/util/virstring.c
+++ b/src/util/virstring.c
@@ -239,6 +239,42 @@ virStringListRemove(char ***strings,
 }
 
 
+/**
+ * virStringListMerge:
+ * @dst: a NULL-terminated array of strings to expand
+ * @src: a NULL-terminated array of strings
+ *
+ * Merges @src into @dst. Upon successful return from this
+ * function, @dst is resized to $(dst + src) elements and @src is
+ * freed.
+ *
+ * Returns 0 on success, -1 otherwise.
+ */
+int
+virStringListMerge(char ***dst,
+   char ***src)
+{
+size_t dst_len, src_len, i;
+
+if (!src || !*src)
+return 0;
+
+dst_len = virStringListLength((const char **) *dst);
+src_len = virStringListLength((const char **) *src);
+
+if (VIR_REALLOC_N(*dst, dst_len + src_len + 1) < 0)
+return -1;
+
+for (i = 0; i <= src_len; i++)
+(*dst)[i + dst_len] = (*src)[i];
+
+/* Don't call virStringListFree() as it would free strings in
+ * @src. */
+VIR_FREE(*src);
+return 0;
+}
+
+
 /**
  * virStringListCopy:
  * @dst: where to store the copy of @strings
diff --git a/src/util/virstring.h b/src/util/virstring.h
index b19abaf9f..f42aaff62 100644
--- a/src/util/virstring.h
+++ b/src/util/virstring.h
@@ -46,6 +46,9 @@ char **virStringListAdd(const char **strings,
 void virStringListRemove(char ***strings,
  const char *item);
 
+int virStringListMerge(char ***dst,
+   char ***src);
+
 int virStringListCopy(char ***dst,
   const char **src);
 
-- 
2.13.6

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

[libvirt] [PATCH 08/18] vshCommandOpt: Allow caller avoiding assert()

[Michal Privoznik]

In the future, completer callbacks will receive partially parsed
command (and thus possibly incomplete). However, we still want
them to use command options fetching APIs we already have (e.g.
vshCommandOpt*()) and at the same time don't report any errors
(nor call any asserts).

Signed-off-by: Michal Privoznik 
---
 tools/vsh.c | 7 ---
 tools/vsh.h | 3 ++-
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/tools/vsh.c b/tools/vsh.c
index ebc8d9cb1..d27acb95b 100644
--- a/tools/vsh.c
+++ b/tools/vsh.c
@@ -815,8 +815,8 @@ vshCommandFree(vshCmd *cmd)
  * to the option if found, 0 with *OPT set to NULL if the name is
  * valid and the option is not required, -1 with *OPT set to NULL if
  * the option is required but not present, and assert if NAME is not
- * valid (which indicates a programming error).  No error messages are
- * issued if a value is returned.
+ * valid (which indicates a programming error) unless cmd->skipChecks
+ * is set. No error messages are issued if a value is returned.
  */
 static int
 vshCommandOpt(const vshCmd *cmd, const char *name, vshCmdOpt **opt,
@@ -829,7 +829,8 @@ vshCommandOpt(const vshCmd *cmd, const char *name, 
vshCmdOpt **opt,
 /* See if option is valid and/or required.  */
 *opt = NULL;
 while (valid) {
-assert(valid->name);
+if (!cmd->skipChecks)
+assert(valid->name);
 if (STREQ(name, valid->name))
 break;
 valid++;
diff --git a/tools/vsh.h b/tools/vsh.h
index 8f7df9ff8..112b1b57d 100644
--- a/tools/vsh.h
+++ b/tools/vsh.h
@@ -188,7 +188,8 @@ struct _vshCmdDef {
 struct _vshCmd {
 const vshCmdDef *def;   /* command definition */
 vshCmdOpt *opts;/* list of command arguments */
-vshCmd *next;  /* next command */
+vshCmd *next;   /* next command */
+bool skipChecks;/* skip validity checks when retrieving opts */
 };
 
 /*
-- 
2.13.6

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

[libvirt] [PATCH 06/18] vshReadlineParse: Escape returned results if needed

[Michal Privoznik]

When returning a string that needs escaping there are two
scenarios that can happen. Firstly, user already started the
string with a quote (or double quote) in which case we don't need
to do anything - readline takes care of that. However, if they
haven't typed anything yet, we need to escape the string
ourselves.

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

diff --git a/tools/vsh.c b/tools/vsh.c
index c3423d6e3..bcabf4231 100644
--- a/tools/vsh.c
+++ b/tools/vsh.c
@@ -2735,6 +2735,14 @@ vshReadlineParse(const char *text, int state)
 res = vshReadlineOptionsGenerator(text, state, cmd);
 }
 
+if (res &&
+!rl_completion_quote_character) {
+virBuffer buf = VIR_BUFFER_INITIALIZER;
+virBufferEscapeShell(, res);
+VIR_FREE(res);
+res = virBufferContentAndReset();
+}
+
 if (!res) {
 vshCommandFree(partial);
 partial = NULL;
@@ -2754,6 +2762,16 @@ vshReadlineCompletion(const char *text,
 return matches;
 }
 
+
+static int
+vshReadlineCharIsQuoted(char *line, int idx)
+{
+return idx > 0 &&
+   line[idx - 1] == '\\' &&
+   !vshReadlineCharIsQuoted(line, idx - 1);
+}
+
+
 # define HISTSIZE_MAX 50
 
 static int
@@ -2765,6 +2783,7 @@ vshReadlineInit(vshControl *ctl)
 char *histsize_env = NULL;
 const char *histsize_str = NULL;
 const char *break_characters = " \t\n\\`@$><=;|&{(";
+const char *quote_characters = "\"'";
 
 /* Opaque data for autocomplete callbacks. */
 autoCompleteOpaque = ctl;
@@ -2788,6 +2807,14 @@ vshReadlineInit(vshControl *ctl)
 rl_basic_word_break_characters = (char *) break_characters;
 # endif
 
+# if defined(RL_READLINE_VERSION) && RL_READLINE_VERSION > 0x0402
+rl_completer_quote_characters = quote_characters;
+rl_char_is_quoted_p = vshReadlineCharIsQuoted;
+# else
+rl_completer_quote_characters = (char *) quote_characters;
+rl_char_is_quoted_p = (Function *) vshReadlineCharIsQuoted;
+# endif
+
 if (virAsprintf(_env, "%s_HISTSIZE", ctl->env_prefix) < 0)
 goto cleanup;
 
-- 
2.13.6

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

[libvirt] [PATCH 04/18] vshCommandStringParse: Allow retrieving partial result

[Michal Privoznik]

In the future, this function is going to be called from
vshReadlineParse() to provide parsed input for completer
callbacks. The idea is to allow the callbacks to provide more
specific data. For instance, for the following input:

  virsh # domifaddr --domain fedora --interface 

the --interface completer callback is going to be called. Now, it
is more user friendly if the completer offers only those
interfaces found in 'fedora' domain. But in order to do that it
needs to be able to retrieve partially parsed result.

Signed-off-by: Michal Privoznik 
---
 tools/virsh.c  |   4 +-
 tools/virt-admin.c |   4 +-
 tools/vsh.c| 111 +
 tools/vsh.h|   2 +-
 4 files changed, 82 insertions(+), 39 deletions(-)

diff --git a/tools/virsh.c b/tools/virsh.c
index 89a2bea10..933d6b4c9 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -817,7 +817,7 @@ virshParseArgv(vshControl *ctl, int argc, char **argv)
 ctl->imode = false;
 if (argc - optind == 1) {
 vshDebug(ctl, VSH_ERR_INFO, "commands: \"%s\"\n", argv[optind]);
-return vshCommandStringParse(ctl, argv[optind]);
+return vshCommandStringParse(ctl, argv[optind], NULL);
 } else {
 return vshCommandArgvParse(ctl, argc - optind, argv + optind);
 }
@@ -954,7 +954,7 @@ main(int argc, char **argv)
 #if WITH_READLINE
 add_history(ctl->cmdstr);
 #endif
-if (vshCommandStringParse(ctl, ctl->cmdstr))
+if (vshCommandStringParse(ctl, ctl->cmdstr, NULL))
 vshCommandRun(ctl, ctl->cmd);
 }
 VIR_FREE(ctl->cmdstr);
diff --git a/tools/virt-admin.c b/tools/virt-admin.c
index e529a2891..b8b33af19 100644
--- a/tools/virt-admin.c
+++ b/tools/virt-admin.c
@@ -1335,7 +1335,7 @@ vshAdmParseArgv(vshControl *ctl, int argc, char **argv)
 ctl->imode = false;
 if (argc - optind == 1) {
 vshDebug(ctl, VSH_ERR_INFO, "commands: \"%s\"\n", argv[optind]);
-return vshCommandStringParse(ctl, argv[optind]);
+return vshCommandStringParse(ctl, argv[optind], NULL);
 } else {
 return vshCommandArgvParse(ctl, argc - optind, argv + optind);
 }
@@ -1555,7 +1555,7 @@ main(int argc, char **argv)
 #if WITH_READLINE
 add_history(ctl->cmdstr);
 #endif
-if (vshCommandStringParse(ctl, ctl->cmdstr))
+if (vshCommandStringParse(ctl, ctl->cmdstr, NULL))
 vshCommandRun(ctl, ctl->cmd);
 }
 VIR_FREE(ctl->cmdstr);
diff --git a/tools/vsh.c b/tools/vsh.c
index 2366b7b71..34eb592ef 100644
--- a/tools/vsh.c
+++ b/tools/vsh.c
@@ -1386,26 +1386,34 @@ struct _vshCommandParser {
 };
 
 static bool
-vshCommandParse(vshControl *ctl, vshCommandParser *parser)
+vshCommandParse(vshControl *ctl, vshCommandParser *parser, vshCmd **partial)
 {
 char *tkdata = NULL;
 vshCmd *clast = NULL;
 vshCmdOpt *first = NULL;
+const vshCmdDef *cmd = NULL;
 
-vshCommandFree(ctl->cmd);
-ctl->cmd = NULL;
+if (!partial) {
+vshCommandFree(ctl->cmd);
+ctl->cmd = NULL;
+}
 
 while (1) {
 vshCmdOpt *last = NULL;
-const vshCmdDef *cmd = NULL;
 vshCommandToken tk;
 bool data_only = false;
 uint64_t opts_need_arg = 0;
 uint64_t opts_required = 0;
 uint64_t opts_seen = 0;
 
+cmd = NULL;
 first = NULL;
 
+if (partial) {
+vshCommandFree(*partial);
+*partial = NULL;
+}
+
 while (1) {
 const vshCmdOptDef *opt = NULL;
 
@@ -1422,7 +1430,8 @@ vshCommandParse(vshControl *ctl, vshCommandParser *parser)
 if (cmd == NULL) {
 /* first token must be command name */
 if (!(cmd = vshCmddefSearch(tkdata))) {
-vshError(ctl, _("unknown command: '%s'"), tkdata);
+if (!partial)
+vshError(ctl, _("unknown command: '%s'"), tkdata);
 goto syntaxError;   /* ... or ignore this command only? */
 }
 
@@ -1434,9 +1443,10 @@ vshCommandParse(vshControl *ctl, vshCommandParser 
*parser)
 }
 if (vshCmddefOptParse(cmd, _need_arg,
   _required) < 0) {
-vshError(ctl,
- _("internal error: bad options in command: '%s'"),
- tkdata);
+if (!partial)
+vshError(ctl,
+ _("internal error: bad options in command: 
'%s'"),
+ tkdata);
 goto syntaxError;
 }
 VIR_FREE(tkdata);
@@ -1454,7 +1464,7 @@ vshCommandParse(vshControl *ctl, vshCommandParser *parser)
 

[libvirt] [PATCH 07/18] vshReadlineParse: Use string list

[Michal Privoznik]

It's better to fetch list of either commands or options just once
and then iterate over it. Moreover, it makes future completers
way simpler as they will return string lists too.

Signed-off-by: Michal Privoznik 
---
 tools/vsh.c | 121 
 1 file changed, 73 insertions(+), 48 deletions(-)

diff --git a/tools/vsh.c b/tools/vsh.c
index bcabf4231..ebc8d9cb1 100644
--- a/tools/vsh.c
+++ b/tools/vsh.c
@@ -2605,25 +2605,25 @@ vshTreePrint(vshControl *ctl, vshTreeLookup lookup, 
void *opaque,
  * -
  */
 
-/*
- * Generator function for command completion.  STATE lets us
- * know whether to start from scratch; without any state
- * (i.e. STATE == 0), then we start at the top of the list.
+/**
+ * vshReadlineCommandGenerator:
+ * @text: optional command prefix
+ *
+ * Generator function for command completion.
+ *
+ * Returns a string list of commands with @text prefix,
+ * NULL if there's no such command.
  */
-static char *
-vshReadlineCommandGenerator(const char *text, int state)
+static char **
+vshReadlineCommandGenerator(const char *text)
 {
-static unsigned int grp_list_index, cmd_list_index;
-static size_t len;
+size_t grp_list_index = 0, cmd_list_index = 0;
+size_t len = strlen(text);
 const char *name;
 const vshCmdGrp *grp;
 const vshCmdDef *cmds;
-
-if (!state) {
-grp_list_index = 0;
-cmd_list_index = 0;
-len = strlen(text);
-}
+size_t ret_size = 0;
+char **ret = NULL;
 
 grp = cmdGroups;
 
@@ -2638,8 +2638,16 @@ vshReadlineCommandGenerator(const char *text, int state)
 if (cmds[cmd_list_index++].flags & VSH_CMD_FLAG_ALIAS)
 continue;
 
-if (STREQLEN(name, text, len))
-return vshStrdup(NULL, name);
+if (STREQLEN(name, text, len)) {
+if (VIR_REALLOC_N(ret, ret_size + 2) < 0) {
+virStringListFree(ret);
+return NULL;
+}
+ret[ret_size] = vshStrdup(NULL, name);
+ret_size++;
+/* Terminate the string list properly. */
+ret[ret_size] = NULL;
+}
 }
 } else {
 cmd_list_index = 0;
@@ -2647,23 +2655,17 @@ vshReadlineCommandGenerator(const char *text, int state)
 }
 }
 
-/* If no names matched, then return NULL. */
-return NULL;
+return ret;
 }
 
-static char *
-vshReadlineOptionsGenerator(const char *text, int state, const vshCmdDef 
*cmd_parsed)
+static char **
+vshReadlineOptionsGenerator(const char *text, const vshCmdDef *cmd)
 {
-static unsigned int list_index;
-static size_t len;
-static const vshCmdDef *cmd;
+size_t list_index = 0;
+size_t len = strlen(text);
 const char *name;
-
-if (!state) {
-cmd = cmd_parsed;
-list_index = 0;
-len = strlen(text);
-}
+size_t ret_size = 0;
+char **ret = NULL;
 
 if (!cmd)
 return NULL;
@@ -2672,7 +2674,7 @@ vshReadlineOptionsGenerator(const char *text, int state, 
const vshCmdDef *cmd_pa
 return NULL;
 
 while ((name = cmd->opts[list_index].name)) {
-char *res;
+size_t name_len;
 
 list_index++;
 
@@ -2685,28 +2687,40 @@ vshReadlineOptionsGenerator(const char *text, int 
state, const vshCmdDef *cmd_pa
 } else if (STRNEQLEN(text, "--", len)) {
 return NULL;
 }
-res = vshMalloc(NULL, strlen(name) + 3);
-snprintf(res, strlen(name) + 3,  "--%s", name);
-return res;
+
+if (VIR_REALLOC_N(ret, ret_size + 2) < 0) {
+virStringListFree(ret);
+return NULL;
+}
+
+name_len = strlen(name);
+ret[ret_size] = vshMalloc(NULL, name_len + 3);
+snprintf(ret[ret_size], name_len + 3,  "--%s", name);
+ret_size++;
+/* Terminate the string list properly. */
+ret[ret_size] = NULL;
 }
 
-/* If no names matched, then return NULL. */
-return NULL;
+return ret;
 }
 
 static char *
 vshReadlineParse(const char *text, int state)
 {
 static vshCmd *partial;
-static const vshCmdDef *cmd;
-char *res = NULL;
+static char **list;
+static size_t list_index;
+const vshCmdDef *cmd = NULL;
+char *ret = NULL;
 
 if (!state) {
 char *buf = vshStrdup(NULL, rl_line_buffer);
 
 vshCommandFree(partial);
 partial = NULL;
-cmd = NULL;
+virStringListFree(list);
+list = NULL;
+list_index = 0;
 
 *(buf + rl_point) = '\0';
 
@@ -2729,26 +2743,37 @@ vshReadlineParse(const char *text, int state)
 }
 }
 
-if (!cmd) {
-res = vshReadlineCommandGenerator(text, state);
-} else {
-res = vshReadlineOptionsGenerator(text, state, cmd);
+if (!list) {
+ 

[libvirt] [PATCH 11/18] vsh: Call vshCmdOptDef completer

[Michal Privoznik]

Now that we have everything prepared we can call options'
completer again. At the same time, pass partially parsed input to
the completer callback - it will help the callbacks to narrow
down the list of returned options based on user's input. For
instance, if the completer is supposed to return list of
interfaces depending on user input it may return just those
interfaces defined for already specified domain. Of course,
completers might ignore this parameter.

Signed-off-by: Michal Privoznik 
---
 tools/vsh.c | 51 +--
 tools/vsh.h |  1 +
 2 files changed, 50 insertions(+), 2 deletions(-)

diff --git a/tools/vsh.c b/tools/vsh.c
index f99d941d9..10a4ef69c 100644
--- a/tools/vsh.c
+++ b/tools/vsh.c
@@ -2705,6 +2705,36 @@ vshReadlineOptionsGenerator(const char *text, const 
vshCmdDef *cmd)
 return ret;
 }
 
+
+static const vshCmdOptDef *
+vshReadlineCommandFindOpt(const vshCmd *partial,
+  const char *text)
+{
+const vshCmd *tmp = partial;
+
+while (tmp && tmp->next) {
+if (tmp->def == tmp->next->def &&
+!tmp->next->opts)
+break;
+tmp = tmp->next;
+}
+
+if (tmp && tmp->opts) {
+const vshCmdOpt *opt = tmp->opts;
+
+while (opt) {
+if (STREQ_NULLABLE(opt->data, text) ||
+STREQ_NULLABLE(opt->data, " "))
+return opt->def;
+
+opt = opt->next;
+}
+}
+
+return NULL;
+}
+
+
 static char *
 vshReadlineParse(const char *text, int state)
 {
@@ -2712,6 +2742,7 @@ vshReadlineParse(const char *text, int state)
 static char **list;
 static size_t list_index;
 const vshCmdDef *cmd = NULL;
+const vshCmdOptDef *opt = NULL;
 char *ret = NULL;
 
 if (!state) {
@@ -2729,8 +2760,10 @@ vshReadlineParse(const char *text, int state)
 
 VIR_FREE(buf);
 
-if (partial)
+if (partial) {
 cmd = partial->def;
+partial->skipChecks = true;
+}
 
 if (cmd && STREQ(cmd->name, text)) {
 /* Corner case - some commands share prefix (e.g.
@@ -2742,13 +2775,26 @@ vshReadlineParse(const char *text, int state)
  */
 cmd = NULL;
 }
+
+opt = vshReadlineCommandFindOpt(partial, text);
 }
 
 if (!list) {
 if (!cmd) {
 list = vshReadlineCommandGenerator(text);
 } else {
-list = vshReadlineOptionsGenerator(text, cmd);
+if (!opt || opt->type != VSH_OT_DATA)
+list = vshReadlineOptionsGenerator(text, cmd);
+
+if (opt && opt->completer) {
+char **completer_list = opt->completer(autoCompleteOpaque,
+   partial,
+   opt->completer_flags);
+if (virStringListMerge(, _list) < 0) {
+virStringListFree(completer_list);
+goto cleanup;
+}
+}
 }
 }
 
@@ -2765,6 +2811,7 @@ vshReadlineParse(const char *text, int state)
 ret = virBufferContentAndReset();
 }
 
+ cleanup:
 if (!ret) {
 vshCommandFree(partial);
 partial = NULL;
diff --git a/tools/vsh.h b/tools/vsh.h
index 51f8ef213..ae40fb4e8 100644
--- a/tools/vsh.h
+++ b/tools/vsh.h
@@ -124,6 +124,7 @@ typedef struct _vshCmdOptDef vshCmdOptDef;
 typedef struct _vshControl vshControl;
 
 typedef char **(*vshCompleter)(vshControl *ctl,
+   const vshCmd *cmd,
unsigned int flags);
 
 /*
-- 
2.13.6

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

[libvirt] [PATCH 03/18] vshCommandParse: Don't leak @tkdata

[Michal Privoznik]

When parsing cmd line which has "--" on it, this is leaked.
Problem is, parser->getNextArg() allocates new string and stores
it into tkdata. But as soon as "--" is detected 'continue' is
issued without any free of the allocated memory.

  ==5304== 3 bytes in 1 blocks are definitely lost in loss record 1 of 782
  ==5304==at 0x4C2AF50: malloc (vg_replace_malloc.c:299)
  ==5304==by 0x8BB5AA9: strdup (strdup.c:42)
  ==5304==by 0x55842CA: virStrdup (virstring.c:941)
  ==5304==by 0x172B21: _vshStrdup (vsh.c:162)
  ==5304==by 0x175E8E: vshCommandArgvGetArg (vsh.c:1622)
  ==5304==by 0x17551D: vshCommandParse (vsh.c:1418)
  ==5304==by 0x175F25: vshCommandArgvParse (vsh.c:1638)
  ==5304==by 0x130940: virshParseArgv (virsh.c:820)
  ==5304==by 0x130C49: main (virsh.c:922)

Signed-off-by: Michal Privoznik 
---
 tools/vsh.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/vsh.c b/tools/vsh.c
index a21e1d1de..2366b7b71 100644
--- a/tools/vsh.c
+++ b/tools/vsh.c
@@ -1491,6 +1491,7 @@ vshCommandParse(vshControl *ctl, vshCommandParser *parser)
 }
 } else if (tkdata[0] == '-' && tkdata[1] == '-' &&
tkdata[2] == '\0') {
+VIR_FREE(tkdata);
 data_only = true;
 continue;
 } else {
-- 
2.13.6

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

[libvirt] [PATCH 12/18] vsh: Prune string list returned by completer

[Michal Privoznik]

Instead of having completers prune returned string list based on
user's input we can do that right after the callback is called.
Only strings matching the prefix will be presented to the user
then.

Signed-off-by: Michal Privoznik 
---
 tools/vsh.c | 43 ++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/tools/vsh.c b/tools/vsh.c
index 10a4ef69c..49e8033bd 100644
--- a/tools/vsh.c
+++ b/tools/vsh.c
@@ -2735,6 +2735,40 @@ vshReadlineCommandFindOpt(const vshCmd *partial,
 }
 
 
+static int
+vshCompleterFilter(char ***list,
+   const char *text)
+{
+char **newList = NULL;
+size_t newList_len = 0;
+size_t list_len;
+size_t i;
+
+if (!list || !*list)
+return -1;
+
+list_len = virStringListLength((const char **) *list);
+
+if (VIR_ALLOC_N(newList, list_len + 1) < 0)
+return -1;
+
+for (i = 0; i < list_len; i++) {
+if (!STRPREFIX((*list)[i], text)) {
+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)
 {
@@ -2790,7 +2824,14 @@ vshReadlineParse(const char *text, int state)
 char **completer_list = opt->completer(autoCompleteOpaque,
partial,
opt->completer_flags);
-if (virStringListMerge(, _list) < 0) {
+
+/* For string list returned by completer we have to do
+ * filtering based on @text because completer returns all
+ * possible strings. */
+
+if (completer_list &&
+(vshCompleterFilter(_list, text) < 0 ||
+ virStringListMerge(, _list) < 0)) {
 virStringListFree(completer_list);
 goto cleanup;
 }
-- 
2.13.6

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

[libvirt] [PATCH 10/18] vsh: Fix vshCompleter signature

[Michal Privoznik]

The first argument passed to this function is vshControl *.
There's no need to use void pointer.

Signed-off-by: Michal Privoznik 
---
 tools/vsh.c | 2 +-
 tools/vsh.h | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/vsh.c b/tools/vsh.c
index d27acb95b..f99d941d9 100644
--- a/tools/vsh.c
+++ b/tools/vsh.c
@@ -66,7 +66,7 @@
 
 #ifdef WITH_READLINE
 /* For autocompletion */
-void *autoCompleteOpaque;
+vshControl *autoCompleteOpaque;
 #endif
 
 /* NOTE: It would be much nicer to have these two as part of vshControl
diff --git a/tools/vsh.h b/tools/vsh.h
index 112b1b57d..51f8ef213 100644
--- a/tools/vsh.h
+++ b/tools/vsh.h
@@ -123,7 +123,8 @@ typedef struct _vshCmdOpt vshCmdOpt;
 typedef struct _vshCmdOptDef vshCmdOptDef;
 typedef struct _vshControl vshControl;
 
-typedef char **(*vshCompleter)(void *opaque, unsigned int flags);
+typedef char **(*vshCompleter)(vshControl *ctl,
+   unsigned int flags);
 
 /*
  * vshCmdInfo -- name/value pair for information about command
-- 
2.13.6

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

[libvirt] [PATCH 02/18] vsh: Drop useless check for cmd != NULL

[Michal Privoznik]

All our internal *Free() functions are capable of handling NULL.

Signed-off-by: Michal Privoznik 
---
 tools/vsh.c | 12 
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/tools/vsh.c b/tools/vsh.c
index e38dcec92..a21e1d1de 100644
--- a/tools/vsh.c
+++ b/tools/vsh.c
@@ -1392,10 +1392,8 @@ vshCommandParse(vshControl *ctl, vshCommandParser 
*parser)
 vshCmd *clast = NULL;
 vshCmdOpt *first = NULL;
 
-if (ctl->cmd) {
-vshCommandFree(ctl->cmd);
-ctl->cmd = NULL;
-}
+vshCommandFree(ctl->cmd);
+ctl->cmd = NULL;
 
 while (1) {
 vshCmdOpt *last = NULL;
@@ -1576,10 +1574,8 @@ vshCommandParse(vshControl *ctl, vshCommandParser 
*parser)
 return true;
 
  syntaxError:
-if (ctl->cmd) {
-vshCommandFree(ctl->cmd);
-ctl->cmd = NULL;
-}
+vshCommandFree(ctl->cmd);
+ctl->cmd = NULL;
 vshCommandOptFree(first);
 VIR_FREE(tkdata);
 return false;
-- 
2.13.6

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

[libvirt] [PATCH 00/18] Make bash completion great again

[Michal Privoznik]

Technically, this is a v2 of [1], but this implements the feature from
different angle and therefore it's a start of new series.


What's implemented?
===
Auto completion for both interactive and non-interactive
virsh/virt-admin.


Known limitations
=
Currently, just options completers work. I mean, to bring up list
of domains you have to:

  virsh # start --domain 

Doing bare:

  virsh # start 

brings up list of --options. With the new approach implemented
here it should be easy to implement this. But that can be saved
for later.


How to test this?
=

Interactive completion should work out of the box. Just make sure
you're connected. Completers don't connect! You certainly don't
want ssh's 'Password:' prompt show on , do you?
Non-interactive completers do connect, but in order to avoid any
password prompts, /dev/stdin is closed before anything else is
done. In order to test it, just:

  libvirt.git $ source tools/bash-completion/vsh

Now, bash completion should work:

  libvirt.git $ ./tools/virsh -c qemu:///system start --domain 


Notes
=

As usual, you can find all the patches on my github [2]. I've
tried to work in all the review suggestions from v1. Due to
changes in design (reusing parse code instead of duplicating it)
not all suggestions were possible to work in though.

Michal

1: https://www.redhat.com/archives/libvir-list/2017-November/msg00213.html
2: https://github.com/zippy2/libvirt/tree/bash_autocomplete_v3  (yeah v3, don't 
ask)

Michal Privoznik (18):
  vsh: Drop useless check for opts != NULL
  vsh: Drop useless check for cmd != NULL
  vshCommandParse: Don't leak @tkdata
  vshCommandStringParse: Allow retrieving partial result
  vshReadlineParse: Drop code duplication
  vshReadlineParse: Escape returned results if needed
  vshReadlineParse: Use string list
  vshCommandOpt: Allow caller avoiding assert()
  util: Introduce virStringListMerge
  vsh: Fix vshCompleter signature
  vsh: Call vshCmdOptDef completer
  vsh: Prune string list returned by completer
  vsh: Filter --options
  vsh: Introduce complete command
  tools: Provide bash autompletion file
  virsh: Introduce virshDomainNameCompleter
  virsh: Introduce virshDomainInterfaceCompleter
  virt-admin: Introduce vshAdmServerCompleter

 configure.ac |   3 +
 libvirt.spec.in  |   3 +
 m4/virt-bash-completion.m4   |  74 +
 src/libvirt_private.syms |   1 +
 src/util/virstring.c |  36 +++
 src/util/virstring.h |   3 +
 tools/Makefile.am|  40 ++-
 tools/bash-completion/vsh|  67 +
 tools/virsh-completer.c  | 150 ++
 tools/virsh-completer.h  |  41 +++
 tools/virsh-domain-monitor.c |  33 ++-
 tools/virsh-domain.c | 186 ++--
 tools/virsh-snapshot.c   |  24 +-
 tools/virsh.c|   5 +-
 tools/virsh.h|   7 +-
 tools/virt-admin-completer.c |  76 +
 tools/virt-admin-completer.h |  33 +++
 tools/virt-admin.c   |  13 +-
 tools/vsh.c  | 663 ++-
 tools/vsh.h  |  23 +-
 20 files changed, 1097 insertions(+), 384 deletions(-)
 create mode 100644 m4/virt-bash-completion.m4
 create mode 100644 tools/bash-completion/vsh
 create mode 100644 tools/virsh-completer.c
 create mode 100644 tools/virsh-completer.h
 create mode 100644 tools/virt-admin-completer.c
 create mode 100644 tools/virt-admin-completer.h

-- 
2.13.6

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

[libvirt] [PATCH 01/18] vsh: Drop useless check for opts != NULL

[Michal Privoznik]

All our internal *Free() functions are capable of handling NULL.

Signed-off-by: Michal Privoznik 
---
 tools/vsh.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/tools/vsh.c b/tools/vsh.c
index e878119b9..e38dcec92 100644
--- a/tools/vsh.c
+++ b/tools/vsh.c
@@ -799,8 +799,7 @@ vshCommandFree(vshCmd *cmd)
 
 c = c->next;
 
-if (tmp->opts)
-vshCommandOptFree(tmp->opts);
+vshCommandOptFree(tmp->opts);
 VIR_FREE(tmp);
 }
 }
@@ -1581,8 +1580,7 @@ vshCommandParse(vshControl *ctl, vshCommandParser *parser)
 vshCommandFree(ctl->cmd);
 ctl->cmd = NULL;
 }
-if (first)
-vshCommandOptFree(first);
+vshCommandOptFree(first);
 VIR_FREE(tkdata);
 return false;
 }
-- 
2.13.6

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

Re: [libvirt] [PATCH v3 0/5] Improvements to CPU frequency reporting

[Andrea Bolognani]

A very 2018 ping :)

On Thu, 2017-12-14 at 13:33 +0100, Andrea Bolognani wrote:
> Changes from [v2]:
> 
> * improve the parser;
> * print the architecture name instead of "your architecture".
> 
> Changes from [v1]:
> 
> * adopt Bjoern's approach to refactoring.
> 
> [v2] https://www.redhat.com/archives/libvir-list/2017-December/msg00467.html
> [v1] https://www.redhat.com/archives/libvir-list/2017-December/msg00356.html
> 
> Andrea Bolognani (4):
>   tests: Add host CPU data for Moonshot (RHEL 7.4)
>   util: Print architecture name in /proc/cpuinfo parser
>   util: Improve CPU frequency parsing
>   util: Don't report CPU frequency for ARM hosts
> 
> Bjoern Walk (1):
>   util: virhostcpu: factor out frequency parsing

-- 
Andrea Bolognani / Red Hat / Virtualization

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

[libvirt] [PATCH] docs: Fix serial console configuration examples

[Andrea Bolognani]

Signed-off-by: Andrea Bolognani 
---
Pushed as trivial.

 docs/formatdomain.html.in | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 01db83e60..d272cc1ba 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -6603,6 +6603,7 @@ qemu-kvm -net nic,model=? /dev/null
 
 
 ...
+devices
   !-- KVM virtio console --
   console type='pty'
 source path='/dev/pts/5'/
@@ -6694,7 +6695,7 @@ qemu-kvm -net nic,model=? /dev/null
 
 
 ...
-/devices
+devices
   console type='pty'
 target type='serial'/
   /console
@@ -6719,21 +6720,21 @@ qemu-kvm -net nic,model=? /dev/null
 
 
 ...
-/devices
+devices
   serial type='pty'/
 /devices
 ...
 
 
 ...
-/devices
+devices
   console type='pty'/
 /devices
 ...
 
 
 ...
-/devices
+devices
   serial type='pty'/
   console type='pty'/
 /devices
-- 
2.14.3

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

Re: [libvirt] [PATCH 3/5] qemu: add a public API to trigger QEMU driver to connect to running guest

[John Ferlan]

On 12/20/2017 11:47 AM, Daniel P. Berrange wrote:
> Currently the QEMU driver will reconnect to running guests during libvirtd
> startup. To support the ability to launch a fully supported guest externally
> to the main libvirtd daemon, this adds a QEMU specific public API
> 
>   virDomainQemuReconnect(conn, name, flags);
> 
> This accepts a domain name, and simply runs the normal reconnect logic that
> the QEMU driver would do at startup.
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  include/libvirt/libvirt-qemu.h |  4 
>  src/driver-hypervisor.h|  5 +
>  src/libvirt-qemu.c | 48 
> ++
>  src/libvirt_qemu.syms  |  5 +
>  src/remote/qemu_protocol.x | 18 +++-
>  src/remote/remote_driver.c |  1 +
>  6 files changed, 80 insertions(+), 1 deletion(-)
> 

Build for me failed on this patch during build of various
src/*_protocol_struct files:

--- qemu_protocol-structs   2016-01-13 07:49:19.459819838 -0500
+++ qemu_protocol-struct-t3 2017-12-21 13:42:37.836063667 -0500
@@ -47,6 +47,13 @@
 u_int  micros;
 remote_string  details;
 };
+struct qemu_domain_reconnect_args {
+remote_nonnull_string  name;
+u_int  flags;
+};
+struct qemu_domain_reconnect_ret {
+remote_nonnull_domain  dom;
+};
 enum qemu_procedure {
 QEMU_PROC_DOMAIN_MONITOR_COMMAND = 1,
 QEMU_PROC_DOMAIN_ATTACH = 2,
@@ -54,4 +61,5 @@
 QEMU_PROC_CONNECT_DOMAIN_MONITOR_EVENT_REGISTER = 4,
 QEMU_PROC_CONNECT_DOMAIN_MONITOR_EVENT_DEREGISTER = 5,
 QEMU_PROC_DOMAIN_MONITOR_EVENT = 6,
+QEMU_PROC_DOMAIN_RECONNECT = 7,
 };


Otherwise, looks reasonable/right to me.

John

> diff --git a/include/libvirt/libvirt-qemu.h b/include/libvirt/libvirt-qemu.h
> index 2bb8ee8685..0d38d2f9f8 100644
> --- a/include/libvirt/libvirt-qemu.h
> +++ b/include/libvirt/libvirt-qemu.h
> @@ -44,6 +44,10 @@ virDomainPtr virDomainQemuAttach(virConnectPtr domain,
>   unsigned int pid_value,
>   unsigned int flags);
>  
> +virDomainPtr virDomainQemuReconnect(virConnectPtr domain,
> +const char *name,
> +unsigned int flags);
> +
>  typedef enum {
>  VIR_DOMAIN_QEMU_AGENT_COMMAND_MIN = -2,
>  VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK = -2,
> diff --git a/src/driver-hypervisor.h b/src/driver-hypervisor.h
> index ce0e2b2525..2e923e730f 100644
> --- a/src/driver-hypervisor.h
> +++ b/src/driver-hypervisor.h
> @@ -848,6 +848,10 @@ typedef virDomainPtr
>  (*virDrvDomainQemuAttach)(virConnectPtr conn,
>unsigned int pid_value,
>unsigned int flags);
> +typedef virDomainPtr
> +(*virDrvDomainQemuReconnect)(virConnectPtr conn,
> + const char *name,
> + unsigned int flags);
>  
>  typedef int
>  (*virDrvConnectDomainQemuMonitorEventRegister)(virConnectPtr conn,
> @@ -1463,6 +1467,7 @@ struct _virHypervisorDriver {
>  virDrvDomainSnapshotDelete domainSnapshotDelete;
>  virDrvDomainQemuMonitorCommand domainQemuMonitorCommand;
>  virDrvDomainQemuAttach domainQemuAttach;
> +virDrvDomainQemuReconnect domainQemuReconnect;
>  virDrvDomainQemuAgentCommand domainQemuAgentCommand;
>  virDrvConnectDomainQemuMonitorEventRegister 
> connectDomainQemuMonitorEventRegister;
>  virDrvConnectDomainQemuMonitorEventDeregister 
> connectDomainQemuMonitorEventDeregister;
> diff --git a/src/libvirt-qemu.c b/src/libvirt-qemu.c
> index 43f63839eb..c5859cf312 100644
> --- a/src/libvirt-qemu.c
> +++ b/src/libvirt-qemu.c
> @@ -164,6 +164,54 @@ virDomainQemuAttach(virConnectPtr conn,
>  return NULL;
>  }
>  
> +/**
> + * virDomainQemuReconnect:
> + * @conn: pointer to a hypervisor connection
> + * @name: the libvirt guest name
> + * @flags: optional flags, currently unused
> + *
> + * This API is QEMU specific, so it will only work with hypervisor
> + * connections to the QEMU driver.
> + *
> + * This API will attach to an QEMU process that a separate libvirt
> + * helper has launched. The external QEMU must fully follow the
> + * normal libvirt QEMU configuration.
> + *
> + * If successful, then the guest will appear in the list of running
> + * domains for this connection, and other APIs should operate
> + * normally (provided the above requirements were honored).
> + *
> + * Returns a new domain object on success, NULL otherwise
> + */
> +virDomainPtr
> +virDomainQemuReconnect(virConnectPtr conn,
> +   const char *name,
> +   unsigned int flags)
> +{
> +VIR_DEBUG("conn=%p, name=%s, flags=0x%x", conn, name, flags);
> +
> +virResetLastError();
> +
> +virCheckConnectReturn(conn, NULL);
> +virCheckNonNullArgGoto(name, error);
> 

Re: [libvirt] [PATCH 2/5] conf: expose APIs to let drivers load individual config / status files

[John Ferlan]

On 12/20/2017 11:47 AM, Daniel P. Berrange wrote:
> Currently drivers can only do a bulk load of config / status files for
> their guests. This exposes some helper methods to allow individual
> guests to be loaded.
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  src/conf/virdomainobjlist.c | 98 
> -
>  src/conf/virdomainobjlist.h | 17 
>  src/libvirt_private.syms|  2 +
>  3 files changed, 89 insertions(+), 28 deletions(-)
> 
> diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c
> index 87a742b1ea..37f795fe6d 100644
> --- a/src/conf/virdomainobjlist.c
> +++ b/src/conf/virdomainobjlist.c
> @@ -443,16 +443,15 @@ void virDomainObjListRemoveLocked(virDomainObjListPtr 
> doms,
>  virObjectUnlock(dom);
>  }
>  
> -
>  static virDomainObjPtr
> -virDomainObjListLoadConfig(virDomainObjListPtr doms,
> -   virCapsPtr caps,
> -   virDomainXMLOptionPtr xmlopt,
> -   const char *configDir,
> -   const char *autostartDir,
> -   const char *name,
> -   virDomainLoadConfigNotify notify,
> -   void *opaque)
> +virDomainObjListLoadConfigLocked(virDomainObjListPtr doms,
> + virCapsPtr caps,
> + virDomainXMLOptionPtr xmlopt,
> + const char *configDir,
> + const char *autostartDir,
> + const char *name,
> + virDomainLoadConfigNotify notify,
> + void *opaque)
>  {
>  char *configFile = NULL, *autostartLink = NULL;
>  virDomainDefPtr def = NULL;
> @@ -496,14 +495,36 @@ virDomainObjListLoadConfig(virDomainObjListPtr doms,
>  }
>  
>  
> -static virDomainObjPtr
> -virDomainObjListLoadStatus(virDomainObjListPtr doms,
> -   const char *statusDir,
> -   const char *name,
> +virDomainObjPtr
> +virDomainObjListLoadConfig(virDomainObjListPtr doms,
> virCapsPtr caps,
> virDomainXMLOptionPtr xmlopt,
> +   const char *configDir,
> +   const char *autostartDir,
> +   const char *name,
> virDomainLoadConfigNotify notify,
> void *opaque)
> +{
> +virDomainObjPtr vm = NULL;
> +virObjectRWLockWrite(doms);
> +
> +vm = virDomainObjListLoadConfigLocked(doms, caps, xmlopt, configDir,
> +  autostartDir, name, notify, 
> opaque);
> +virObjectRef(vm);

 IIRC this Ref is done because the virDomainObjListAddLocked
doesn't add enough refs. One of those things that's been on my list to
get to after finishing up the storage/nwfilter "common object"
adjustments.  IOW: AddLocked is broken because it only adds one ref even
though the obj is in both the UUID and Name tables - so callers are
forced to "adjust". When we leave AddLocked there should be 3 refs
and 1 lock, but there are 2 refs and 1 lock.

The other concerns here would be when virDomainObjListLoadAllConfigs
returns a valid @dom will:

1. Set "dom->persistent" based upon the @liveStatus pool passed in

2. Call virObjectUnlock(dom)

Neither happens in these new API's. So all new consumers would need to
know/remember to unlock (or EndAPI) and the LoadConfig caller would need
to set persistent (unless we did so in "this" function").

Probably something that needs to be documented in the two new functions.

John

> +
> +virObjectRWUnlock(doms);
> +return vm;
> +}
> +
> +
> +static virDomainObjPtr
> +virDomainObjListLoadStatusLocked(virDomainObjListPtr doms,
> + const char *statusDir,
> + const char *name,
> + virCapsPtr caps,
> + virDomainXMLOptionPtr xmlopt,
> + virDomainLoadConfigNotify notify,
> + void *opaque)
>  {
>  char *statusFile = NULL;
>  virDomainObjPtr obj = NULL;
> @@ -555,6 +576,27 @@ virDomainObjListLoadStatus(virDomainObjListPtr doms,
>  }
>  
>  
> +virDomainObjPtr
> +virDomainObjListLoadStatus(virDomainObjListPtr doms,
> +   const char *statusDir,
> +   const char *name,
> +   virCapsPtr caps,
> +   virDomainXMLOptionPtr xmlopt,
> +   virDomainLoadConfigNotify notify,
> +   void *opaque)
> +{
> +virDomainObjPtr vm = NULL;
> +virObjectRWLockWrite(doms);
> +
> +vm = virDomainObjListLoadStatusLocked(doms, statusDir, name, caps, 
> xmlopt,
> +

Re: [libvirt] [PATCH 1/5] conf: allow different resource registration modes

[John Ferlan]

I know it's a POC type series, but figured I'd take a look primarily at
the first 4 patches, learn a bit in the 5th one, and provide some review
feedback in order to help move things along...  There's perhaps a few
adjustments in here that could be made into multiple patches.

On 12/20/2017 11:47 AM, Daniel P. Berrange wrote:
> Currently the QEMU driver has three ways of setting up cgroups. It either
> skips them entirely (if non-root), or uses systemd-machined, or uses
> cgroups directly.
> 
> It is further possible to register directly with systemd and bypass
> machined. We don't support this by systemd-nsspawn does and we ought

s/by/but/   (I think)

Still a bit odd to add an option that isn't supported unless there was a
plan to add the support when formulating the non POC patches...

> to.
> 
> This change adds ability to configure the mechanism for registering
> resources between all these options explicitly. via

s/. via/ via:/  (e.g. remove the '.' and add ':')

> 
>   
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  src/conf/domain_conf.c   | 42 ++---
>  src/conf/domain_conf.h   | 12 +
>  src/libvirt_private.syms |  2 ++
>  src/lxc/lxc_cgroup.c | 34 
>  src/lxc/lxc_cgroup.h |  3 +++
>  src/lxc/lxc_process.c| 11 
>  src/qemu/qemu_cgroup.c   | 69 
> +---
>  src/util/vircgroup.c | 55 --
>  src/util/vircgroup.h | 10 ++-
>  9 files changed, 194 insertions(+), 44 deletions(-)
> 


formatdomain.html.in would need to be augmented to describe the new
attribute.

domaincommon.rng modified to add the new attribute.

A tests either added or existing one adjusted to exhibit the new
attribute option(s).

> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 9a62bc472c..fb8e7a0ec7 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -910,6 +910,14 @@ VIR_ENUM_IMPL(virDomainHPTResizing,
>"required",
>  );
>  
> +VIR_ENUM_IMPL(virDomainResourceRegister,
> +  VIR_DOMAIN_RESOURCE_REGISTER_LAST,
> +  "default",
> +  "none",
> +  "cgroup",
> +  "machined",
> +  "systemd");
> +
>  /* Internal mapping: subset of block job types that can be present in
>   *  XML (remaining types are not two-phase). */
>  VIR_ENUM_DECL(virDomainBlockJob)
> @@ -17698,16 +17706,20 @@ virDomainResourceDefParse(xmlNodePtr node,
>  {
>  virDomainResourceDefPtr def = NULL;
>  xmlNodePtr tmp = ctxt->node;
> +char *reg;

= NULL;

>  
>  ctxt->node = node;
>  
>  if (VIR_ALLOC(def) < 0)
>  goto error;
>  
> -/* Find out what type of virtualization to use */
> -if (!(def->partition = virXPathString("string(./partition)", ctxt))) {
> -virReportError(VIR_ERR_INTERNAL_ERROR,
> -   "%s", _("missing resource partition attribute"));
> +def->partition = virXPathString("string(./partition)", ctxt);

It seems partition is now optional, although formatdomain seems to have
already indicated that... However, that's not a perfect match to
domaincommon.rng where only "respartition" is optional. (This type of
change could perhaps be it's own patch).

> +
> +reg = virXMLPropString(node, "register");
> +if (reg != NULL &&
> +(def->reg = virDomainResourceRegisterTypeFromString(reg)) <= 0) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +   "%s", _("Invalid register attribute"));
>  goto error;
>  }

Need a VIR_FREE(reg); since we haven't GO-ified yet in both error and
normal paths unless this is rewritten a bit...

>  
> @@ -25568,11 +25580,23 @@ static void
>  virDomainResourceDefFormat(virBufferPtr buf,
> virDomainResourceDefPtr def)
>  {
> -virBufferAddLit(buf, "\n");
> -virBufferAdjustIndent(buf, 2);
> -virBufferEscapeString(buf, "%s\n", 
> def->partition);
> -virBufferAdjustIndent(buf, -2);
> -virBufferAddLit(buf, "\n");
> +if (def->reg == VIR_DOMAIN_RESOURCE_REGISTER_DEFAULT &&
> +def->partition == NULL)
> +return;
> +
> +virBufferAddLit(buf, " +if (def->reg != VIR_DOMAIN_RESOURCE_REGISTER_DEFAULT)
> +virBufferAsprintf(buf, " register='%s'", 
> virDomainResourceRegisterTypeToString(def->reg));
> +
> +if (def->partition) {
> +virBufferAddLit(buf, ">\n");
> +virBufferAdjustIndent(buf, 2);
> +virBufferEscapeString(buf, "%s\n", 
> def->partition);
> +virBufferAdjustIndent(buf, -2);
> +virBufferAddLit(buf, "\n");
> +} else {
> +virBufferAddLit(buf, "/>\n");
> +}
>  }
>  
>  
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 6f7f96b3dd..a7a6628a36 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -2145,9 +2145,20 @@ struct 

Re: [libvirt] [PATCH] maint: Update to latest gnulib

[Michal Privoznik]

On 01/02/2018 02:09 PM, John Ferlan wrote:
> 
> 
> On 01/02/2018 04:28 AM, Michal Privoznik wrote:
>> Unfortunately, since gnulib's commit of 2c5d558745 there's an
>> unused parameter to stat_time_normalize() function which gnulib
>> developers don't want to fix [1]. Therefore, we have to work
>> around it by temporarily suspending -Wunused-parameter.
>>
>> 1: http://lists.gnu.org/archive/html/bug-gnulib/2018-01/msg0.html
>>
>> Signed-off-by: Michal Privoznik 
>> ---
>>
>> While we have 'gnulib update free' push rule, this one is not trivial at
>> all and thus I have not pushed it. It's ugly and I don't like it. So any
>> ideas are welcome.
>>
>>  .gnulib| 2 +-
>>  bootstrap  | 4 ++--
>>  src/storage/storage_util.c | 3 +++
>>  3 files changed, 6 insertions(+), 3 deletions(-)
>>
> 
> 
> No bright ideas on this other than perhaps only including changes just
> prior to the particular one that breaks things

Unfortunately, that is not possible. The gnulib's commit that breaks
things was merged 23rd of November and we need the most recent commit
which fixes the stupid year check.

> or somehow revert just
> that one in our local copy.

I'm worried that in the end this would make us diverge from the gnulib's
upstream too much.

>
> Other thoughts require additional local changes just to "work around"
> the broken definition.  Such as grabbing the "old" stat-time.h, rename
> it, save it in libvirt sources, then use that new name instead of the
> new .gnulib file. That's probably worse than what you've done though.

Agreed.

> 
> At this point, I think your solution to minimize the impact to one
> include file is perhaps the easiest/best solution albeit not perfect.
> 
> It's interesting that there is no desire to fix this problem in .gnulib
> especially since there are already 2 patches proposed and in reality the
> change fundamentally breaks on every platform other than __sun when
> STAT_TIMESPEC is defined just because it's possible (or more desirable
> in the reviewers feedback) to compile with ignore unused-parameter.
> Wonder what would happen if someone posted a patch to .gnulib to revert
> the change for the reason that it breaks other platforms that don't
> desire to configure in that manner. Perhaps Eric Blake would have some
> thoughts or additional muscle with the .gnulib maintainers.

That's what I hope for. Since the problem lies in a header file I think
it's impossible to require all projects that use gnulib to compile
without -Wunused-parameter.

Another solution might be to move the function that causes troubles into
.c since it's purely internal function and nobody outside gnulib
calls/should call it. At least then we could use different sets of
CFLAGS for libvirt and gnulib (not that I'd fancy it or know how to do
it).

Michal

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

Re: [libvirt] [PATCH] maint: Update to latest gnulib

[John Ferlan]

On 01/02/2018 04:28 AM, Michal Privoznik wrote:
> Unfortunately, since gnulib's commit of 2c5d558745 there's an
> unused parameter to stat_time_normalize() function which gnulib
> developers don't want to fix [1]. Therefore, we have to work
> around it by temporarily suspending -Wunused-parameter.
> 
> 1: http://lists.gnu.org/archive/html/bug-gnulib/2018-01/msg0.html
> 
> Signed-off-by: Michal Privoznik 
> ---
> 
> While we have 'gnulib update free' push rule, this one is not trivial at
> all and thus I have not pushed it. It's ugly and I don't like it. So any
> ideas are welcome.
> 
>  .gnulib| 2 +-
>  bootstrap  | 4 ++--
>  src/storage/storage_util.c | 3 +++
>  3 files changed, 6 insertions(+), 3 deletions(-)
> 


No bright ideas on this other than perhaps only including changes just
prior to the particular one that breaks things or somehow revert just
that one in our local copy.

Other thoughts require additional local changes just to "work around"
the broken definition.  Such as grabbing the "old" stat-time.h, rename
it, save it in libvirt sources, then use that new name instead of the
new .gnulib file. That's probably worse than what you've done though.

At this point, I think your solution to minimize the impact to one
include file is perhaps the easiest/best solution albeit not perfect.

It's interesting that there is no desire to fix this problem in .gnulib
especially since there are already 2 patches proposed and in reality the
change fundamentally breaks on every platform other than __sun when
STAT_TIMESPEC is defined just because it's possible (or more desirable
in the reviewers feedback) to compile with ignore unused-parameter.
Wonder what would happen if someone posted a patch to .gnulib to revert
the change for the reason that it breaks other platforms that don't
desire to configure in that manner. Perhaps Eric Blake would have some
thoughts or additional muscle with the .gnulib maintainers.

John


> diff --git a/.gnulib b/.gnulib
> index 5e9abf871..c2cb55b34 16
> --- a/.gnulib
> +++ b/.gnulib
> @@ -1 +1 @@
> -Subproject commit 5e9abf87163ad4aeaefef0b02961f8674b0a4879
> +Subproject commit c2cb55b34e76546479f195c14202dfcc870c4914
> diff --git a/bootstrap b/bootstrap
> index 85b85c530..25920e991 100755
> --- a/bootstrap
> +++ b/bootstrap
> @@ -4,7 +4,7 @@ scriptversion=2017-09-19.08; # UTC
>  
>  # Bootstrap this package from checked-out sources.
>  
> -# Copyright (C) 2003-2017 Free Software Foundation, Inc.
> +# Copyright (C) 2003-2018 Free Software Foundation, 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
> @@ -792,7 +792,7 @@ symlink_to_dir()
># aren't confused into doing unnecessary builds.  Conversely, if the
># existing symlink's timestamp is older than the source, make it 
> afresh,
># so that broken tools aren't confused into skipping needed builds.  
> See
> -  # 
> .
> +  # .
>test -h "$dst" &&
>src_ls=$(ls -diL "$src" 2>/dev/null) && set $src_ls && src_i=$1 &&
>dst_ls=$(ls -diL "$dst" 2>/dev/null) && set $dst_ls && dst_i=$1 &&
> diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c
> index 899a55758..da6381f52 100644
> --- a/src/storage/storage_util.c
> +++ b/src/storage/storage_util.c
> @@ -64,7 +64,10 @@
>  #include "virfile.h"
>  #include "virjson.h"
>  #include "virqemu.h"
> +#pragma GCC diagnostic push
> +#pragma GCC diagnostic ignored "-Wunused-parameter"
>  #include "stat-time.h"
> +#pragma GCC diagnostic pop
>  #include "virstring.h"
>  #include "virxml.h"
>  #include "virfdstream.h"
> 

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

[libvirt] virt-install ERROR Host does not support any virtualization options

[Mihamina RAKOTOMANDIMBY]

Hello,

I face this problem and I am willing to provide a patch in order to have 
a more informative message.

This, of course with our help. I am not quite certain.

First of all: What is the problem?

On a fresh Install (ArchLinux for me), if ever forget to install Qemu 
and launch "virt-install" with "--type kvm", I get the message:


"Host does not support any virtualization options"


According to me, this message should be improved: if Qemu is not found, 
this should be "Did not find Qemu, please install it".


As I searched, the message is generated by this piece of code:

https://github.com/virt-manager/virt-manager/blob/c92aade081687b19f5a60cddfe331b2fb6d09f98/virtinst/capabilities.py#L390

As I write now, I cant find the pice of code actually calling the 
"quemu" binary. I think I should add a try/catch block there and Raise 
the right Exception.

Would someone help me a bit?

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

Re: [libvirt] [PATCH] conf: honor maxnames in nodeListDevices API

[Michal Privoznik]

On 01/02/2018 10:28 AM, Pavel Hrdina wrote:
> Introduced by commit <4ae9dbea99c>.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1528572
> 
> Signed-off-by: Pavel Hrdina 
> ---
>  src/conf/virnodedeviceobj.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c
> index 872ec1fd4b..c4e3a40d3a 100644
> --- a/src/conf/virnodedeviceobj.c
> +++ b/src/conf/virnodedeviceobj.c
> @@ -798,6 +798,9 @@ virNodeDeviceObjListGetNamesCallback(void *payload,
>  if (data->error)
>  return 0;
>  
> +if (data->nnames >= data->maxnames)
> +return 0;
> +
>  virObjectLock(obj);
>  def = obj->def;
>  
> 

ACK

Michal

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

[libvirt] [PATCH] maint: Update to latest gnulib

[Michal Privoznik]

Unfortunately, since gnulib's commit of 2c5d558745 there's an
unused parameter to stat_time_normalize() function which gnulib
developers don't want to fix [1]. Therefore, we have to work
around it by temporarily suspending -Wunused-parameter.

1: http://lists.gnu.org/archive/html/bug-gnulib/2018-01/msg0.html

Signed-off-by: Michal Privoznik 
---

While we have 'gnulib update free' push rule, this one is not trivial at
all and thus I have not pushed it. It's ugly and I don't like it. So any
ideas are welcome.

 .gnulib| 2 +-
 bootstrap  | 4 ++--
 src/storage/storage_util.c | 3 +++
 3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/.gnulib b/.gnulib
index 5e9abf871..c2cb55b34 16
--- a/.gnulib
+++ b/.gnulib
@@ -1 +1 @@
-Subproject commit 5e9abf87163ad4aeaefef0b02961f8674b0a4879
+Subproject commit c2cb55b34e76546479f195c14202dfcc870c4914
diff --git a/bootstrap b/bootstrap
index 85b85c530..25920e991 100755
--- a/bootstrap
+++ b/bootstrap
@@ -4,7 +4,7 @@ scriptversion=2017-09-19.08; # UTC
 
 # Bootstrap this package from checked-out sources.
 
-# Copyright (C) 2003-2017 Free Software Foundation, Inc.
+# Copyright (C) 2003-2018 Free Software Foundation, 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
@@ -792,7 +792,7 @@ symlink_to_dir()
   # aren't confused into doing unnecessary builds.  Conversely, if the
   # existing symlink's timestamp is older than the source, make it afresh,
   # so that broken tools aren't confused into skipping needed builds.  See
-  # .
+  # .
   test -h "$dst" &&
   src_ls=$(ls -diL "$src" 2>/dev/null) && set $src_ls && src_i=$1 &&
   dst_ls=$(ls -diL "$dst" 2>/dev/null) && set $dst_ls && dst_i=$1 &&
diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c
index 899a55758..da6381f52 100644
--- a/src/storage/storage_util.c
+++ b/src/storage/storage_util.c
@@ -64,7 +64,10 @@
 #include "virfile.h"
 #include "virjson.h"
 #include "virqemu.h"
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wunused-parameter"
 #include "stat-time.h"
+#pragma GCC diagnostic pop
 #include "virstring.h"
 #include "virxml.h"
 #include "virfdstream.h"
-- 
2.13.6

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

[libvirt] [PATCH] conf: honor maxnames in nodeListDevices API

[Pavel Hrdina]

Introduced by commit <4ae9dbea99c>.

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

Signed-off-by: Pavel Hrdina 
---
 src/conf/virnodedeviceobj.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c
index 872ec1fd4b..c4e3a40d3a 100644
--- a/src/conf/virnodedeviceobj.c
+++ b/src/conf/virnodedeviceobj.c
@@ -798,6 +798,9 @@ virNodeDeviceObjListGetNamesCallback(void *payload,
 if (data->error)
 return 0;
 
+if (data->nnames >= data->maxnames)
+return 0;
+
 virObjectLock(obj);
 def = obj->def;
 
-- 
2.14.3

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

Re: [libvirt] error: operation failed: domain save job: unexpectedly failed

[Michal Privoznik]

On 12/27/2017 11:00 PM, Oscar Segarra wrote:
> Hi, 
> 
> For any unkown reason the virsh save raises an error in my environment
> Centos 7.2:
> 
> [root@vdicnode02 ~]# virsh list
>  Id    Name                           State
> 
>  2     vdicsunstone01                 running
>  3     one-27                         running
> 
> [root@vdicnode02 ~]# export LIBVIRT_DEBUG=0
> [root@vdicnode02 ~]# export LIBVIRT_LOG_OUTPUTS="1:file:virsh.log"
> [root@vdicnode02 ~]# virsh --debug 0 --connect qemu:///system save
> one-27 /var/lib/one//datastores/100/27/checkpoint
> save: domain(optdata): one-27
> save: file(optdata): /var/lib/one//datastores/100/27/checkpoint
> save: found option : one-27
> save:  trying as domain NAME
> save: found option : one-27
> save:  trying as domain NAME
> error: Failed to save domain one-27 to
> /var/lib/one//datastores/100/27/checkpoint
> error: operation failed: domain save job: unexpectedly failed

Virsh logs are useless. You need to look at domain log which can be
usually found under /var/log/libvirt/qemu/$domain.log. Smells like qemu
was unable to perform some action and thus the whole operation failed.

http://wiki.libvirt.org/page/DebugLogs

Michal

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