[libvirt] [PATCH 1/2] util: honor reportError parameter in virSocketAddrParseInternal

[Jim Fehlig]

Signed-off-by: Jim Fehlig 
---
 src/util/virsocketaddr.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/util/virsocketaddr.c b/src/util/virsocketaddr.c
index 95b527436..31a740cb8 100644
--- a/src/util/virsocketaddr.c
+++ b/src/util/virsocketaddr.c
@@ -107,7 +107,8 @@ virSocketAddrParseInternal(struct addrinfo **res,
 int err;
 
 if (val == NULL) {
-virReportError(VIR_ERR_INVALID_ARG, "%s", _("Missing address"));
+if (reportError)
+virReportError(VIR_ERR_INVALID_ARG, "%s", _("Missing address"));
 return -1;
 }
 
-- 
2.16.2

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

[libvirt] [PATCH 2/2] util: introduce virSocketAddrParseAny

[Jim Fehlig]

When preparing for migration, the libxl driver creates a new TCP listen
socket for the incoming migration by calling virNetSocketNewListenTCP,
passing the destination host name. virNetSocketNewListenTCP calls
virSocketAddrParse to check if the host name is a wildcard address, in
which case it avoids adding the AI_ADDRCONFIG flag to the hints passed to
getaddrinfo. If the host name is not an IP address, virSocketAddrParse
reports an error

error : virSocketAddrParseInternal:121 : Cannot parse socket address
'myhost.example.com': Name or service not known

But virNetSocketNewListenTCP succeeds regardless and the overall migration
operation succeeds.

Introduce virSocketAddrParseAny and use it when simply testing if a host
name/addr is parsable.

Signed-off-by: Jim Fehlig 
---

Essentially a V2 of

https://www.redhat.com/archives/libvir-list/2018-March/msg01120.html

It takes a slightly different approach by creating a function that can
parse host names or IP addresses.

 src/libvirt_private.syms |  1 +
 src/rpc/virnetsocket.c   |  2 +-
 src/util/virsocketaddr.c | 50 +---
 src/util/virsocketaddr.h |  5 +
 4 files changed, 54 insertions(+), 4 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 03fe3b315..f6cdcde72 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2725,6 +2725,7 @@ virSocketAddrMask;
 virSocketAddrMaskByPrefix;
 virSocketAddrNumericFamily;
 virSocketAddrParse;
+virSocketAddrParseAny;
 virSocketAddrParseIPv4;
 virSocketAddrParseIPv6;
 virSocketAddrPrefixToNetmask;
diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c
index 2d41a716b..f362a0955 100644
--- a/src/rpc/virnetsocket.c
+++ b/src/rpc/virnetsocket.c
@@ -333,7 +333,7 @@ int virNetSocketNewListenTCP(const char *nodename,
  * startup in most cases.
  */
 if (nodename &&
-!(virSocketAddrParse(&tmp_addr, nodename, AF_UNSPEC) > 0 &&
+!(virSocketAddrParseAny(&tmp_addr, nodename, AF_UNSPEC, false) > 0 &&
   virSocketAddrIsWildcard(&tmp_addr)))
 hints.ai_flags |= AI_ADDRCONFIG;
 
diff --git a/src/util/virsocketaddr.c b/src/util/virsocketaddr.c
index 31a740cb8..84610560f 100644
--- a/src/util/virsocketaddr.c
+++ b/src/util/virsocketaddr.c
@@ -101,6 +101,7 @@ static int
 virSocketAddrParseInternal(struct addrinfo **res,
const char *val,
int family,
+   int ai_flags,
bool reportError)
 {
 struct addrinfo hints;
@@ -114,7 +115,7 @@ virSocketAddrParseInternal(struct addrinfo **res,
 
 memset(&hints, 0, sizeof(hints));
 hints.ai_family = family;
-hints.ai_flags = AI_NUMERICHOST;
+hints.ai_flags = ai_flags;
 if ((err = getaddrinfo(val, NULL, &hints, res)) != 0) {
 if (reportError)
 virReportError(VIR_ERR_SYSTEM_ERROR,
@@ -143,7 +144,7 @@ int virSocketAddrParse(virSocketAddrPtr addr, const char 
*val, int family)
 int len;
 struct addrinfo *res;
 
-if (virSocketAddrParseInternal(&res, val, family, true) < 0)
+if (virSocketAddrParseInternal(&res, val, family, AI_NUMERICHOST, true) < 
0)
 return -1;
 
 if (res == NULL) {
@@ -163,6 +164,49 @@ int virSocketAddrParse(virSocketAddrPtr addr, const char 
*val, int family)
 return len;
 }
 
+/**
+ * virSocketAddrParseAny:
+ * @addr: where to store the return value, optional.
+ * @val: a network host name or a numeric network address IPv4 or IPv6
+ * @family: address family to pass down to getaddrinfo
+ * @reportError: boolean to control error reporting
+ *
+ * Mostly a wrapper for getaddrinfo() extracting the address storage
+ * from a host name like acme.example.com or a numeric string like 1.2.3.4
+ * or 2001:db8:85a3:0:0:8a2e:370:7334
+ *
+ * Returns the length of the network address or -1 in case of error.
+ */
+int virSocketAddrParseAny(virSocketAddrPtr addr,
+  const char *val,
+  int family,
+  bool reportError)
+{
+int len;
+struct addrinfo *res;
+
+if (virSocketAddrParseInternal(&res, val, family, 0, reportError) < 0)
+return -1;
+
+if (res == NULL) {
+if (reportError) {
+virReportError(VIR_ERR_SYSTEM_ERROR,
+   _("No socket addresses found for '%s'"),
+   val);
+}
+return -1;
+}
+
+len = res->ai_addrlen;
+if (addr != NULL) {
+memcpy(&addr->data.stor, res->ai_addr, len);
+addr->len = res->ai_addrlen;
+}
+
+freeaddrinfo(res);
+return len;
+}
+
 /*
  * virSocketAddrParseIPv4:
  * @val: an IPv4 numeric address
@@ -1105,7 +1149,7 @@ virSocketAddrNumericFamily(const char *address)
 struct addrinfo *res;
 unsigned short family;
 
-if (virSocketAddrParseInternal(&res, address, AF_UNSPEC, false) < 0)
+if (virSocketAddrParseInternal(&res, address, A

[libvirt] [PATCH 0/2] utils: squelch errors from virNetSocketNewConnectTCP

[Jim Fehlig]

Patch1 fixes a case where error is reported even when the reportError
parameter is false.

Patch2 is essentially a V2 of a patch posted earlier, incorporating
some ideas from jferlan's review

https://www.redhat.com/archives/libvir-list/2018-March/msg01120.html

Jim Fehlig (2):
  util: honor reportError parameter in virSocketAddrParseInternal
  util: introduce virSocketAddrParseAny

 src/libvirt_private.syms |  1 +
 src/rpc/virnetsocket.c   |  2 +-
 src/util/virsocketaddr.c | 53 
 src/util/virsocketaddr.h |  5 +
 4 files changed, 56 insertions(+), 5 deletions(-)

-- 
2.16.2

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

Re: [libvirt] [dbus PATCH v2] Add 'Version' property for virConnectGetVersion

[Katerina Koukiou]

On Mon, 2018-03-26 at 18:09 +0200, Pavel Hrdina wrote:
> On Mon, Mar 26, 2018 at 05:19:22PM +0200, Katerina Koukiou wrote:
> > ---
> >  data/org.libvirt.Connect.xml |  4 
> >  src/connect.c| 25 -
> >  test/test_connect.py |  9 +
> >  3 files changed, 37 insertions(+), 1 deletion(-)
> 
> Looks like that the git hook that checks Signed-off-by is enabled
> also
> for libvirt-dbus repository it needs to be added.  If you agree with
> it I can add it there before pushing.
Sure, thanks.
>   Next time please make sure that
> the Signed-off-by line is in the commit message.
> 
> Since this patch is not pushed, I'll fix the URL as well before
> pushing.
Ok.
> Pavel

Katerina

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

Re: [libvirt] [dbus PATCH 2/4] spec: cleanup spec file based on fedora package review

[Pavel Hrdina]

On Mon, Mar 26, 2018 at 05:54:48PM +0200, Ján Tomko wrote:
> On Mon, Mar 26, 2018 at 03:33:51PM +0200, Pavel Hrdina wrote:
> > All of these changes are simple enough so I've put the into single
> > commit:
> > 
> >- remove obsolete tags and commands
> >- %global macro is preferred over %define macro
> >- use %autosetup, %make_build and %make_install macros
> >- COPYING file should be listed using %license macro
> > 
> > Signed-off-by: Pavel Hrdina 
> > ---
> > libvirt-dbus.spec.in | 26 ++
> > 1 file changed, 10 insertions(+), 16 deletions(-)
> > 
> 
> ACK if you leave the testament of your laziness in the commit message.

Thanks, I'll keep the commit message as it is :).

Pavel


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

Re: [libvirt] [dbus PATCH v2] Add 'Version' property for virConnectGetVersion

[Pavel Hrdina]

On Mon, Mar 26, 2018 at 05:19:22PM +0200, Katerina Koukiou wrote:
> ---
>  data/org.libvirt.Connect.xml |  4 
>  src/connect.c| 25 -
>  test/test_connect.py |  9 +
>  3 files changed, 37 insertions(+), 1 deletion(-)

Looks like that the git hook that checks Signed-off-by is enabled also
for libvirt-dbus repository it needs to be added.  If you agree with
it I can add it there before pushing.  Next time please make sure that
the Signed-off-by line is in the commit message.

Since this patch is not pushed, I'll fix the URL as well before pushing.

Pavel


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

Re: [libvirt] [dbus PATCH v2] Add 'Version' property for virConnectGetVersion

[Pavel Hrdina]

On Mon, Mar 26, 2018 at 05:54:17PM +0200, Ján Tomko wrote:
> On Mon, Mar 26, 2018 at 05:19:22PM +0200, Katerina Koukiou wrote:
> > ---
> > data/org.libvirt.Connect.xml |  4 
> > src/connect.c| 25 -
> > test/test_connect.py |  9 +
> > 3 files changed, 37 insertions(+), 1 deletion(-)
> > 
> > diff --git a/data/org.libvirt.Connect.xml b/data/org.libvirt.Connect.xml
> > index e47c2f5..56a1126 100644
> > --- a/data/org.libvirt.Connect.xml
> > +++ b/data/org.libvirt.Connect.xml
> > @@ -3,6 +3,10 @@
> > 
> > 
> >   
> > +
> > +   > +value="See 
> > https://libvirt.org/html/libvirt-libvirt-domain.html#virConnectGetVersion"/>
> 
> This one is documented in the libvirt-host file.
> 
> Jan

Nice catch, I'll fix it, thanks.

Pavel


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

Re: [libvirt] [PATCH sandbox] Delete the virt-sandbox-service command

[Cedric Bosdonnat]

On Mon, 2018-03-26 at 15:35 +0100, Daniel P. Berrangé wrote:
> This command attempted to create sandboxed containers for running
> systemd services that exist on the host. This code has proved very
> fragile, however, since it needs heuristics to figure out which dirs
> need to be made private in the container vs shared with the host. Even
> a relatively simple "httpd.service" sandbox no longer works with
> current Fedora.
> 
> Users wanting to sandbox services are better served by using systemd's
> native container functionality, or using Docker container images. The
> virt-sandbox-image tool can even run Docker/virt-builder images directly.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  TODO|   24 -
>  bin/Makefile.am |   80 +-
>  bin/virt-sandbox-service| 1314 
> ---
>  bin/virt-sandbox-service-bash-completion.sh |  141 ---
>  bin/virt-sandbox-service-clone.pod  |  100 --
>  bin/virt-sandbox-service-connect.pod|   59 --
>  bin/virt-sandbox-service-create.pod |  264 --
>  bin/virt-sandbox-service-delete.pod |   65 --
>  bin/virt-sandbox-service-execute.pod|   71 --
>  bin/virt-sandbox-service-reload.pod |   63 --
>  bin/virt-sandbox-service-upgrade.pod|   74 --
>  bin/virt-sandbox-service-util.c |  305 ---
>  bin/virt-sandbox-service.logrotate  |9 -
>  bin/virt-sandbox-service.pod|   85 --
>  cfg.mk  |2 +-
>  libvirt-sandbox.spec.in |7 -
>  libvirt-sandbox/tests/containers_test.sh|   37 -
>  po/POTFILES.in  |1 -
>  18 files changed, 3 insertions(+), 2698 deletions(-)
>  delete mode 100644 TODO
>  delete mode 100755 bin/virt-sandbox-service
>  delete mode 100755 bin/virt-sandbox-service-bash-completion.sh
>  delete mode 100644 bin/virt-sandbox-service-clone.pod
>  delete mode 100644 bin/virt-sandbox-service-connect.pod
>  delete mode 100644 bin/virt-sandbox-service-create.pod
>  delete mode 100644 bin/virt-sandbox-service-delete.pod
>  delete mode 100644 bin/virt-sandbox-service-execute.pod
>  delete mode 100644 bin/virt-sandbox-service-reload.pod
>  delete mode 100644 bin/virt-sandbox-service-upgrade.pod
>  delete mode 100644 bin/virt-sandbox-service-util.c
>  delete mode 100644 bin/virt-sandbox-service.logrotate
>  delete mode 100644 bin/virt-sandbox-service.pod
>  delete mode 100755 libvirt-sandbox/tests/containers_test.sh
> 
> diff --git a/TODO b/TODO
> deleted file mode 100644
> index fc63361..000
> --- a/TODO
> +++ /dev/null
> @@ -1,24 +0,0 @@
> -  libvirt-sandbox TODO list
> -  =
> -
> -systemd-tmpfiles --create needs to be run within the container, before any
> -apps are started, since it will populate /run (Completed)
> -
> -CGROUPFS: integration so libvirt does it rather then systemd within the 
> container
> -  We need kernel labeling support for cgroupfs so we can allow systemd 
> to write to its section of the
> cgroupfs.
> -
> -SYSLOG:  Currently syslog messages are going no where within the container.
> -If we run a syslog within the container will it get messages from the 
> outside?  Should we just use systemd-
> journal.  I think sysadmins will want to be able to look in /var/log/messages 
> within the container. (systemd-journal
> is now running within a container)
> -
> -EXECUTE:
> - virt-sandbox-service execute --command "BLAH" does not work.  We need 
> to have the ability to execute any
> random command within the container, and get stdin, stdout, stderror outside 
> the container. (Partially Completed)
> -Still needs kernel to implement missing container namespace files under 
> /proc/PID/ns, Also need a mechanism to get
> the PID of systemd from libvirt.
> -
> -HOSTNAME:
> - Currently if I execute hostname within the container it sees the name 
> of the host not the name based on the
> container name or the IP Address associated with dhclient. (Completed)
> -
> -virt-sandbox-service connect NAME hangs when you attempt to end the 
> connection.
> -^d should bring you back to the host terminal.
> -
> -Need a mechanism to allow admins to specify additional services to run within
> -the container.  For example you may want to run mysql and apache within the 
> same container. (Completed) You can do
> this using systemctl enabel BLAH
> diff --git a/bin/Makefile.am b/bin/Makefile.am
> index deedcf6..db0a1d1 100644
> --- a/bin/Makefile.am
> +++ b/bin/Makefile.am
> @@ -1,39 +1,12 @@
>  
>  bin_PROGRAMS = virt-sandbox
>  
> -libexec_PROGRAMS = virt-sandbox-service-util
> +bin_SCRIPTS = virt-sandbox-image
>  
> -bin_SCRIPTS = virt-sandbox-service \
> - virt-sandbox-image
> -
> -virtsandboxcompdir = $(datarootdir)/bash-completion/completions/
> -
> -crondailydir = $(sysconfdir)/cron.daily
> -crondaily_SCRIPTS = virt-sandbox-service.logr

Re: [libvirt] [dbus PATCH 2/4] spec: cleanup spec file based on fedora package review

[Ján Tomko]

On Mon, Mar 26, 2018 at 03:33:51PM +0200, Pavel Hrdina wrote:

All of these changes are simple enough so I've put the into single
commit:

   - remove obsolete tags and commands
   - %global macro is preferred over %define macro
   - use %autosetup, %make_build and %make_install macros
   - COPYING file should be listed using %license macro

Signed-off-by: Pavel Hrdina 
---
libvirt-dbus.spec.in | 26 ++
1 file changed, 10 insertions(+), 16 deletions(-)



ACK if you leave the testament of your laziness in the commit message.

Jan


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

Re: [libvirt] [dbus PATCH v2] Add 'Version' property for virConnectGetVersion

[Ján Tomko]

On Mon, Mar 26, 2018 at 05:19:22PM +0200, Katerina Koukiou wrote:

---
data/org.libvirt.Connect.xml |  4 
src/connect.c| 25 -
test/test_connect.py |  9 +
3 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/data/org.libvirt.Connect.xml b/data/org.libvirt.Connect.xml
index e47c2f5..56a1126 100644
--- a/data/org.libvirt.Connect.xml
+++ b/data/org.libvirt.Connect.xml
@@ -3,6 +3,10 @@


  
+
+  https://libvirt.org/html/libvirt-libvirt-domain.html#virConnectGetVersion"/>


This one is documented in the libvirt-host file.

Jan


+

  https://libvirt.org/html/libvirt-libvirt-domain.html#virConnectListAllDomains"/>


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

Re: [libvirt] [dbus PATCH v2] Add 'Version' property for virConnectGetVersion

[Pavel Hrdina]

On Mon, Mar 26, 2018 at 05:19:22PM +0200, Katerina Koukiou wrote:
> ---
>  data/org.libvirt.Connect.xml |  4 
>  src/connect.c| 25 -
>  test/test_connect.py |  9 +
>  3 files changed, 37 insertions(+), 1 deletion(-)

Thanks, I've pushed the patch.

Pavel


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

Re: [libvirt] [PATCH v2 3/3] news: Document device mapper fix

[Peter Krempa]

On Mon, Mar 26, 2018 at 16:43:03 +0200, Michal Privoznik wrote:
> Signed-off-by: Michal Privoznik 
> ---
>  docs/news.xml | 10 ++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/docs/news.xml b/docs/news.xml
> index 1088895746..6f1ceb6389 100644
> --- a/docs/news.xml
> +++ b/docs/news.xml
> @@ -94,6 +94,16 @@
>
>  
>  
> +  
> +
> +  Improve handling of device mapper targets
> +
> +
> +  When starting a domain with a disk hidden behind
> +  devmapper libvirt needs to allow them both in devices
> +  CGroup: the devmapper target and the disk.

This does not read very well how about:

When starting a domain with a disk backed by a device-mapper volume
libvirt also needs to allow the storage backing the device mapper in
cgroups.

But that acutally brings me to yet another question: You described the
problem with multipath devices. But given that LVM or dm-crypt would
basically suffer from the same problems it would be great if you also
could figure out the root of the problem. Also it might be interresting
to know if it's similarly happening with multiple layers of
device-mapper volumes since 'dmsetup deps' (and the equivalent algorithm
added in this series) does not resolve the device nodes recursively.


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

Re: [libvirt] [dbus PATCH 1/4] maint: remove AUTHORS from repository

[Pavel Hrdina]

On Mon, Mar 26, 2018 at 04:22:02PM +0100, Daniel P. Berrangé wrote:
> On Mon, Mar 26, 2018 at 05:11:06PM +0200, Pavel Hrdina wrote:
> > On Mon, Mar 26, 2018 at 02:56:06PM +0100, Daniel P. Berrangé wrote:
> > > On Mon, Mar 26, 2018 at 03:33:50PM +0200, Pavel Hrdina wrote:
> > > > This is tracked by git itself.  Suggested during fedora package review.
> > > 
> > > AUTHORS is a file that autoconf/automake expects to exist in all
> > > projects. Also the git history is not included with the tar.gz
> > 
> > That is true only if the project decides to follow GNU standard, it
> > requires ChangeLog file which is not present.
> 
> 
> > > dist, so IMHO the AUTHORS is appropriate to keep regardless.
> > 
> > That is correct, however, if someone really wants to know who
> > contributed into the project and what was the contribution the AUTHORS
> > file will not provide you all these information and you will have to
> > look into the git repository.
> 
> If the git repository still exists, and is accessible, when you want to
> look for it. Sadly there are many projects with no git repo anymore after
> services like gitorious.org and code.google.com shutdown. Not that I'm
> suggesting libvirt.org is going away any time soon, but no one can predict
> 10+ years into the future. IMHO, giving authors credit in the distributed
> release tarballs is important, even if git or any other repo exists.

OK, I can agree with this argument :).  I'll drop this patch.

Thanks,
Pavel


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

Re: [libvirt] [PATCH v2 2/3] qemu: Handle device mapper targets properly

[Michal Privoznik]

On 03/26/2018 05:17 PM, Peter Krempa wrote:
> On Mon, Mar 26, 2018 at 16:43:02 +0200, Michal Privoznik wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1557769
>>
>> Problem with device mapper targets is that there can be several
>> other devices 'hidden' behind them. For instance, /dev/dm-1 can
>> consist of /dev/sda, /dev/sdb and /dev/sdc. Therefore, when
>> setting up devices CGroup and namespaces we have to take this
>> into account.
>>
>> Signed-off-by: Michal Privoznik 
>> ---
>>  libvirt.spec.in|  2 ++
>>  src/qemu/qemu_cgroup.c | 42 ++---
>>  src/qemu/qemu_domain.c | 64 
>> ++
>>  3 files changed, 105 insertions(+), 3 deletions(-)
>>
>> diff --git a/libvirt.spec.in b/libvirt.spec.in
>> index b55a947ec9..ebfac10866 100644
>> --- a/libvirt.spec.in
>> +++ b/libvirt.spec.in
>> @@ -796,6 +796,8 @@ Requires: gzip
>>  Requires: bzip2
>>  Requires: lzop
>>  Requires: xz
>> +# For mpath devices
>> +Requires: device-mapper
>>  %if 0%{?fedora} || 0%{?rhel} > 7
>>  Requires: systemd-container
>>  %endif
>> diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
>> index b604edb31c..e17b3d21b5 100644
>> --- a/src/qemu/qemu_cgroup.c
>> +++ b/src/qemu/qemu_cgroup.c
>> @@ -37,6 +37,7 @@
>>  #include "virtypedparam.h"
>>  #include "virnuma.h"
>>  #include "virsystemd.h"
>> +#include "virdevmapper.h"
>>  
>>  #define VIR_FROM_THIS VIR_FROM_QEMU
>>  
>> @@ -60,7 +61,13 @@ qemuSetupImagePathCgroup(virDomainObjPtr vm,
>>  {
>>  qemuDomainObjPrivatePtr priv = vm->privateData;
>>  int perms = VIR_CGROUP_DEVICE_READ;
>> -int ret;
>> +unsigned int *maj = NULL;
>> +unsigned int *min = NULL;
>> +size_t nmaj = 0;
>> +size_t i;
>> +char *devPath = NULL;
>> +int rv;
>> +int ret = -1;
>>  
>>  if (!virCgroupHasController(priv->cgroup, 
>> VIR_CGROUP_CONTROLLER_DEVICES))
>>  return 0;
>> @@ -71,12 +78,41 @@ qemuSetupImagePathCgroup(virDomainObjPtr vm,
>>  VIR_DEBUG("Allow path %s, perms: %s",
>>path, virCgroupGetDevicePermsString(perms));
>>  
>> -ret = virCgroupAllowDevicePath(priv->cgroup, path, perms, true);
>> +rv = virCgroupAllowDevicePath(priv->cgroup, path, perms, true);
>>  
>>  virDomainAuditCgroupPath(vm, priv->cgroup, "allow", path,
>>   virCgroupGetDevicePermsString(perms),
>> - ret);
>> + rv);
>> +if (rv < 0)
>> +goto cleanup;
>>  
>> +if (virDevMapperGetTargets(path, &maj, &min, &nmaj) < 0 &&
>> +errno != ENOSYS && errno != EBADF) {
>> +virReportSystemError(errno,
>> + _("Unable to get devmapper targets for %s"),
>> + path);
>> +goto cleanup;
>> +}
>> +
>> +for (i = 0; i < nmaj; i++) {
>> +if (virAsprintf(&devPath, "/dev/block/%u:%u", maj[i], min[i]) < 0)
>> +goto cleanup;
> 
> So since this path will not help that much in the audit logs ...
> 
>> +
>> +rv = virCgroupAllowDevicePath(priv->cgroup, devPath, perms, true);
> 
> ... you probably should use virCgroupAllowDevice ...
> 
>> +
>> +virDomainAuditCgroupPath(vm, priv->cgroup, "allow", devPath,
>> + virCgroupGetDevicePermsString(perms),
>> + rv);
> 
> ... and add an equivalent of virDomainAuditCgroupMajor that takes both
> major:minor similarly to virDomainAuditCgroupMajor.
> 
>> +if (rv < 0)
>> +goto cleanup;
>> +VIR_FREE(devPath);
>> +}
>> +
>> +ret = 0;
>> + cleanup:
>> +VIR_FREE(devPath);
>> +VIR_FREE(min);
>> +VIR_FREE(maj);
>>  return ret;
>>  }
>>  
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index 580e0f830d..5f56324502 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -54,6 +54,7 @@
>>  #include "secret_util.h"
>>  #include "logging/log_manager.h"
>>  #include "locking/domain_lock.h"
>> +#include "virdevmapper.h"
>>  
>>  #ifdef MAJOR_IN_MKDEV
>>  # include 
>> @@ -10108,6 +10109,11 @@ qemuDomainSetupDisk(virQEMUDriverConfigPtr cfg 
>> ATTRIBUTE_UNUSED,
>>  {
>>  virStorageSourcePtr next;
>>  char *dst = NULL;
>> +unsigned int *maj = NULL;
>> +unsigned int *min = NULL;
>> +size_t nmaj = 0;
>> +char *devPath = NULL;
>> +size_t i;
>>  int ret = -1;
>>  
>>  for (next = disk->src; virStorageSourceIsBacking(next); next = 
>> next->backingStore) {
>> @@ -10118,10 +10124,31 @@ qemuDomainSetupDisk(virQEMUDriverConfigPtr cfg 
>> ATTRIBUTE_UNUSED,
>>  
>>  if (qemuDomainCreateDevice(next->path, data, false) < 0)
>>  goto cleanup;
>> +
>> +if (virDevMapperGetTargets(next->path, &maj, &min, &nmaj) < 0 &&
>> +errno != ENOSYS && errno != EBADF) {
>> +virReportSystemError(errno,
>> + _

Re: [libvirt] [dbus PATCH 1/4] maint: remove AUTHORS from repository

[Daniel P . Berrangé]

On Mon, Mar 26, 2018 at 05:11:06PM +0200, Pavel Hrdina wrote:
> On Mon, Mar 26, 2018 at 02:56:06PM +0100, Daniel P. Berrangé wrote:
> > On Mon, Mar 26, 2018 at 03:33:50PM +0200, Pavel Hrdina wrote:
> > > This is tracked by git itself.  Suggested during fedora package review.
> > 
> > AUTHORS is a file that autoconf/automake expects to exist in all
> > projects. Also the git history is not included with the tar.gz
> 
> That is true only if the project decides to follow GNU standard, it
> requires ChangeLog file which is not present.


> > dist, so IMHO the AUTHORS is appropriate to keep regardless.
> 
> That is correct, however, if someone really wants to know who
> contributed into the project and what was the contribution the AUTHORS
> file will not provide you all these information and you will have to
> look into the git repository.

If the git repository still exists, and is accessible, when you want to
look for it. Sadly there are many projects with no git repo anymore after
services like gitorious.org and code.google.com shutdown. Not that I'm
suggesting libvirt.org is going away any time soon, but no one can predict
10+ years into the future. IMHO, giving authors credit in the distributed
release tarballs is important, even if git or any other repo exists.

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

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

[libvirt] [dbus PATCH v2] Add 'Version' property for virConnectGetVersion

[Katerina Koukiou]

---
 data/org.libvirt.Connect.xml |  4 
 src/connect.c| 25 -
 test/test_connect.py |  9 +
 3 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/data/org.libvirt.Connect.xml b/data/org.libvirt.Connect.xml
index e47c2f5..56a1126 100644
--- a/data/org.libvirt.Connect.xml
+++ b/data/org.libvirt.Connect.xml
@@ -3,6 +3,10 @@
 
 
   
+
+  https://libvirt.org/html/libvirt-libvirt-domain.html#virConnectGetVersion"/>
+
 
   https://libvirt.org/html/libvirt-libvirt-domain.html#virConnectListAllDomains"/>
diff --git a/src/connect.c b/src/connect.c
index bf97cd5..8898e6d 100644
--- a/src/connect.c
+++ b/src/connect.c
@@ -80,6 +80,24 @@ virtDBusConnectOpen(virtDBusConnect *connect,
 return TRUE;
 }
 
+static void
+virtDBusConnectGetVersion(const gchar *objectPath G_GNUC_UNUSED,
+  gpointer userData,
+  GVariant **value,
+  GError **error)
+{
+virtDBusConnect *connect = userData;
+gulong hvVer;
+
+if (!virtDBusConnectOpen(connect, error))
+return;
+
+if (virConnectGetVersion(connect->connection, &hvVer) < 0)
+return virtDBusUtilSetLastVirtError(error);
+
+*value = g_variant_new("t", hvVer);
+}
+
 static void
 virtDBusConnectListDomains(GVariant *inArgs,
GUnixFDList *inFDs G_GNUC_UNUSED,
@@ -177,6 +195,11 @@ virtDBusConnectDefineXML(GVariant *inArgs,
 *outArgs = g_variant_new("(o)", path);
 }
 
+static virtDBusGDBusPropertyTable virtDBusConnectPropertyTable[] = {
+{ "Version", virtDBusConnectGetVersion, NULL },
+{ NULL, NULL, NULL }
+};
+
 static virtDBusGDBusMethodTable virtDBusConnectMethodTable[] = {
 { "ListDomains", virtDBusConnectListDomains },
 { "CreateXML", virtDBusConnectCreateXML },
@@ -228,7 +251,7 @@ virtDBusConnectNew(virtDBusConnect **connectp,
 connect->connectPath,
 interfaceInfo,
 virtDBusConnectMethodTable,
-NULL,
+virtDBusConnectPropertyTable,
 connect);
 
 virtDBusDomainRegister(connect, error);
diff --git a/test/test_connect.py b/test/test_connect.py
index a52140c..01d4d41 100755
--- a/test/test_connect.py
+++ b/test/test_connect.py
@@ -2,6 +2,7 @@
 
 import dbus
 import libvirttest
+import pytest
 
 
 class TestConnect(libvirttest.BaseTestClass):
@@ -53,6 +54,14 @@ class TestConnect(libvirttest.BaseTestClass):
 
 self.main_loop()
 
+@pytest.mark.parametrize("property_name,expected_type", [
+("Version", dbus.UInt64),
+])
+def test_connect_properties_return_type(self, property_name, 
expected_type):
+obj = self.bus.get_object('org.libvirt', '/org/libvirt/Test')
+props = obj.GetAll('org.libvirt.Connect', 
dbus_interface=dbus.PROPERTIES_IFACE)
+assert isinstance(props[property_name], expected_type)
+
 
 if __name__ == '__main__':
 libvirttest.run()
-- 
2.15.0

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

Re: [libvirt] [dbus PATCH 2/4] spec: cleanup spec file based on fedora package review

[Pavel Hrdina]

On Mon, Mar 26, 2018 at 03:43:02PM +0200, Ján Tomko wrote:
> On Mon, Mar 26, 2018 at 03:33:51PM +0200, Pavel Hrdina wrote:
> > All of these changes are simple enough so I've put the into single
> > commit:
> > 
> 
> Sigh,
> 
> Jan

This means that you want me to send a v2 where I split the changes into
separate commits?  At first I had a single patch for all the spec file
changes, I could have keep it like that and call it
"rewrite spec file" :).

Pavel


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

Re: [libvirt] [PATCH v2 2/3] qemu: Handle device mapper targets properly

[Peter Krempa]

On Mon, Mar 26, 2018 at 16:43:02 +0200, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1557769
> 
> Problem with device mapper targets is that there can be several
> other devices 'hidden' behind them. For instance, /dev/dm-1 can
> consist of /dev/sda, /dev/sdb and /dev/sdc. Therefore, when
> setting up devices CGroup and namespaces we have to take this
> into account.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  libvirt.spec.in|  2 ++
>  src/qemu/qemu_cgroup.c | 42 ++---
>  src/qemu/qemu_domain.c | 64 
> ++
>  3 files changed, 105 insertions(+), 3 deletions(-)
> 
> diff --git a/libvirt.spec.in b/libvirt.spec.in
> index b55a947ec9..ebfac10866 100644
> --- a/libvirt.spec.in
> +++ b/libvirt.spec.in
> @@ -796,6 +796,8 @@ Requires: gzip
>  Requires: bzip2
>  Requires: lzop
>  Requires: xz
> +# For mpath devices
> +Requires: device-mapper
>  %if 0%{?fedora} || 0%{?rhel} > 7
>  Requires: systemd-container
>  %endif
> diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
> index b604edb31c..e17b3d21b5 100644
> --- a/src/qemu/qemu_cgroup.c
> +++ b/src/qemu/qemu_cgroup.c
> @@ -37,6 +37,7 @@
>  #include "virtypedparam.h"
>  #include "virnuma.h"
>  #include "virsystemd.h"
> +#include "virdevmapper.h"
>  
>  #define VIR_FROM_THIS VIR_FROM_QEMU
>  
> @@ -60,7 +61,13 @@ qemuSetupImagePathCgroup(virDomainObjPtr vm,
>  {
>  qemuDomainObjPrivatePtr priv = vm->privateData;
>  int perms = VIR_CGROUP_DEVICE_READ;
> -int ret;
> +unsigned int *maj = NULL;
> +unsigned int *min = NULL;
> +size_t nmaj = 0;
> +size_t i;
> +char *devPath = NULL;
> +int rv;
> +int ret = -1;
>  
>  if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES))
>  return 0;
> @@ -71,12 +78,41 @@ qemuSetupImagePathCgroup(virDomainObjPtr vm,
>  VIR_DEBUG("Allow path %s, perms: %s",
>path, virCgroupGetDevicePermsString(perms));
>  
> -ret = virCgroupAllowDevicePath(priv->cgroup, path, perms, true);
> +rv = virCgroupAllowDevicePath(priv->cgroup, path, perms, true);
>  
>  virDomainAuditCgroupPath(vm, priv->cgroup, "allow", path,
>   virCgroupGetDevicePermsString(perms),
> - ret);
> + rv);
> +if (rv < 0)
> +goto cleanup;
>  
> +if (virDevMapperGetTargets(path, &maj, &min, &nmaj) < 0 &&
> +errno != ENOSYS && errno != EBADF) {
> +virReportSystemError(errno,
> + _("Unable to get devmapper targets for %s"),
> + path);
> +goto cleanup;
> +}
> +
> +for (i = 0; i < nmaj; i++) {
> +if (virAsprintf(&devPath, "/dev/block/%u:%u", maj[i], min[i]) < 0)
> +goto cleanup;

So since this path will not help that much in the audit logs ...

> +
> +rv = virCgroupAllowDevicePath(priv->cgroup, devPath, perms, true);

... you probably should use virCgroupAllowDevice ...

> +
> +virDomainAuditCgroupPath(vm, priv->cgroup, "allow", devPath,
> + virCgroupGetDevicePermsString(perms),
> + rv);

... and add an equivalent of virDomainAuditCgroupMajor that takes both
major:minor similarly to virDomainAuditCgroupMajor.

> +if (rv < 0)
> +goto cleanup;
> +VIR_FREE(devPath);
> +}
> +
> +ret = 0;
> + cleanup:
> +VIR_FREE(devPath);
> +VIR_FREE(min);
> +VIR_FREE(maj);
>  return ret;
>  }
>  
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 580e0f830d..5f56324502 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -54,6 +54,7 @@
>  #include "secret_util.h"
>  #include "logging/log_manager.h"
>  #include "locking/domain_lock.h"
> +#include "virdevmapper.h"
>  
>  #ifdef MAJOR_IN_MKDEV
>  # include 
> @@ -10108,6 +10109,11 @@ qemuDomainSetupDisk(virQEMUDriverConfigPtr cfg 
> ATTRIBUTE_UNUSED,
>  {
>  virStorageSourcePtr next;
>  char *dst = NULL;
> +unsigned int *maj = NULL;
> +unsigned int *min = NULL;
> +size_t nmaj = 0;
> +char *devPath = NULL;
> +size_t i;
>  int ret = -1;
>  
>  for (next = disk->src; virStorageSourceIsBacking(next); next = 
> next->backingStore) {
> @@ -10118,10 +10124,31 @@ qemuDomainSetupDisk(virQEMUDriverConfigPtr cfg 
> ATTRIBUTE_UNUSED,
>  
>  if (qemuDomainCreateDevice(next->path, data, false) < 0)
>  goto cleanup;
> +
> +if (virDevMapperGetTargets(next->path, &maj, &min, &nmaj) < 0 &&
> +errno != ENOSYS && errno != EBADF) {
> +virReportSystemError(errno,
> + _("Unable to get mpath targets for %s"),
> + next->path);
> +goto cleanup;
> +}
> +
> +for (i = 0; i < nmaj; i++) {
> +if (virAsprin

Re: [libvirt] [dbus PATCH 2/2] Add 'NumOfDomains' property for virConnectGetNumOfDomains

[Katerina Koukiou]

On Mon, 2018-03-26 at 16:07 +0100, Daniel P. Berrangé wrote:
> On Mon, Mar 26, 2018 at 04:58:09PM +0200, Katerina Koukiou wrote:
> > ---
> >  data/org.libvirt.Connect.xml |  4 
> >  src/connect.c| 20 
> >  test/test_connect.py |  1 +
> >  3 files changed, 25 insertions(+)
> > 
> > diff --git a/data/org.libvirt.Connect.xml
> > b/data/org.libvirt.Connect.xml
> > index 56a1126..a58504d 100644
> > --- a/data/org.libvirt.Connect.xml
> > +++ b/data/org.libvirt.Connect.xml
> > @@ -7,6 +7,10 @@
> > >  value="See https://libvirt.org/html/libvirt-libvirt-domain
> > .html#virConnectGetVersion"/>;
> >  
> > +
> > +   > +value="See https://libvirt.org/html/libvirt-libvirt-domain
> > .html#virConnectNumOfDomains"/>;
> > +
> 
> I'm not convinced this makes sense to expose. This is part of the old
> way, very inefficient way, of of listing guests.
> 
> The new virConnectListAllDomains() method is much more efficient
> approach in general.

Thanks for pointing this out. I will remove the patch and I will not
implement any other similar *ConnectNumOf* API in the future.
> 
> Regards,
> Daniel

Regards,
Katerina

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

Re: [libvirt] [PATCH v2 1/3] util: Introduce virDevMapperGetTargets

[Peter Krempa]

On Mon, Mar 26, 2018 at 16:43:01 +0200, Michal Privoznik wrote:
> This helper fetches dependencies for given device mapper target.
> 
> At the same time, we need to provide a dummy log function because
> by default libdevmapper prints out error messages to stderr which
> we need to suppress.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/libvirt_private.syms |   4 ++
>  src/util/Makefile.inc.am |   2 +
>  src/util/virdevmapper.c  | 160 
> +++
>  src/util/virdevmapper.h  |  32 ++
>  4 files changed, 198 insertions(+)
>  create mode 100644 src/util/virdevmapper.c
>  create mode 100644 src/util/virdevmapper.h

[...]

> diff --git a/src/util/virdevmapper.c b/src/util/virdevmapper.c
> new file mode 100644
> index 00..9717482da8
> --- /dev/null
> +++ b/src/util/virdevmapper.c
> @@ -0,0 +1,160 @@
> +/*
> + * virdevmapper.c: Functions for handling devmapper
> + *
> + * Copyright (C) 2018 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
> + * .
> + *
> + * Authors:
> + * Michal Privoznik 
> + */
> +
> +#include 
> +
> +#ifdef MAJOR_IN_MKDEV
> +# include 
> +#elif MAJOR_IN_SYSMACROS
> +# include 
> +#endif
> +
> +#ifdef WITH_DEVMAPPER
> +# include 
> +#endif
> +
> +#include "virdevmapper.h"
> +#include "internal.h"
> +#include "virthread.h"
> +#include "viralloc.h"
> +
> +#ifdef WITH_DEVMAPPER
> +static void
> +virDevMapperDummyLogger(int level ATTRIBUTE_UNUSED,
> +const char *file ATTRIBUTE_UNUSED,
> +int line ATTRIBUTE_UNUSED,
> +int dm_errno ATTRIBUTE_UNUSED,
> +const char *fmt ATTRIBUTE_UNUSED,
> +...)
> +{
> +return;
> +}
> +
> +static int
> +virDevMapperOnceInit(void)
> +{
> +/* Ideally, we would not need this. But libdevmapper prints
> + * error messages to stderr by default. Sad but true. */
> +dm_log_with_errno_init(virDevMapperDummyLogger);
> +return 0;
> +}
> +
> +
> +VIR_ONCE_GLOBAL_INIT(virDevMapper)
> +
> +/**
> + * virDevMapperGetTargets:
> + * @path: multipath device
> + * @maj: returned array of MAJOR device numbers
> + * @min: returner array of MINOR device numbers
> + * @nmaj: number of items in @maj array
> + *
> + * For given @path figure out its targets, and store them in @maj
> + * and @min arrays. Both arrays have the same number of items
> + * upon return.
> + *
> + * If @path is not a multipath device, @ndevs is set to 0 and
> + * success is returned.
> + *
> + * If we don't have permissions to talk to kernel, -1 is returned
> + * and errno is set to EBADF.
> + *
> + * Returns 0 on success,
> + *-1 otherwise (with errno set, no libvirt error is
> + *reported)
> + */
> +int
> +virDevMapperGetTargets(const char *path,
> +   unsigned int **maj,
> +   unsigned int **min,
> +   size_t *nmaj)
> +{
> +struct dm_task *dmt = NULL;
> +struct dm_deps *deps;
> +struct dm_info info;
> +size_t i;
> +int ret = -1;
> +
> +*nmaj = 0;
> +
> +if (virDevMapperInitialize() < 0)
> +goto cleanup;

[1]

> +
> +if (!(dmt = dm_task_create(DM_DEVICE_DEPS)))
> +goto cleanup;

[1]

> +
> +if (!dm_task_set_name(dmt, path)) {
> +if (errno == ENOENT) {
> +/* It's okay, @path is not managed by devmapper =>
> + * not a multipath device. */
> +ret = 0;
> +}
> +goto cleanup;
> +}
> +
> +dm_task_no_open_count(dmt);
> +
> +if (!dm_task_run(dmt))
> +goto cleanup;
> +
> +if (!dm_task_get_info(dmt, &info))
> +goto cleanup;
> +
> +if (!info.exists) {
> +ret = 0;
> +goto cleanup;
> +}
> +
> +if (!(deps = dm_task_get_deps(dmt)))
> +goto cleanup;
> +
> +if (VIR_ALLOC_N_QUIET(*maj, deps->count) < 0 ||
> +VIR_ALLOC_N_QUIET(*min, deps->count) < 0) {
> +VIR_FREE(*maj);
> +goto cleanup;
> +}
> +*nmaj = deps->count;
> +
> +for (i = 0; i < deps->count; i++) {
> +(*maj)[i] = major(deps->device[i]);
> +(*min)[i] = minor(deps->device[i]);
> +}
> +
> +ret = 0;
> + cleanup:
> +dm_task_destroy(dmt);

[1] dm_task_destroy dereferences the argument without checking it

Re: [libvirt] [dbus PATCH 1/4] maint: remove AUTHORS from repository

[Pavel Hrdina]

On Mon, Mar 26, 2018 at 02:56:06PM +0100, Daniel P. Berrangé wrote:
> On Mon, Mar 26, 2018 at 03:33:50PM +0200, Pavel Hrdina wrote:
> > This is tracked by git itself.  Suggested during fedora package review.
> 
> AUTHORS is a file that autoconf/automake expects to exist in all
> projects. Also the git history is not included with the tar.gz

That is true only if the project decides to follow GNU standard, it
requires ChangeLog file which is not present.

> dist, so IMHO the AUTHORS is appropriate to keep regardless.

That is correct, however, if someone really wants to know who
contributed into the project and what was the contribution the AUTHORS
file will not provide you all these information and you will have to
look into the git repository.

Pavel


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

Re: [libvirt] [dbus PATCH 2/2] Add 'NumOfDomains' property for virConnectGetNumOfDomains

[Daniel P . Berrangé]

On Mon, Mar 26, 2018 at 04:58:09PM +0200, Katerina Koukiou wrote:
> ---
>  data/org.libvirt.Connect.xml |  4 
>  src/connect.c| 20 
>  test/test_connect.py |  1 +
>  3 files changed, 25 insertions(+)
> 
> diff --git a/data/org.libvirt.Connect.xml b/data/org.libvirt.Connect.xml
> index 56a1126..a58504d 100644
> --- a/data/org.libvirt.Connect.xml
> +++ b/data/org.libvirt.Connect.xml
> @@ -7,6 +7,10 @@
>  value="See 
> https://libvirt.org/html/libvirt-libvirt-domain.html#virConnectGetVersion"/>
>  
> +
> +   +value="See 
> https://libvirt.org/html/libvirt-libvirt-domain.html#virConnectNumOfDomains"/>
> +

I'm not convinced this makes sense to expose. This is part of the old
way, very inefficient way, of of listing guests.

The new virConnectListAllDomains() method is much more efficient
approach in general.

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

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

Re: [libvirt] [dbus PATCH 1/2] Add 'Version' property for virConnectGetVersion

[Katerina Koukiou]

On Mon, 2018-03-26 at 16:58 +0200, Katerina Koukiou wrote:
> ---
>  data/org.libvirt.Connect.xml |  4 
>  src/connect.c| 25 -
>  test/test_connect.py |  8 
>  3 files changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/data/org.libvirt.Connect.xml
> b/data/org.libvirt.Connect.xml
> index e47c2f5..56a1126 100644
> --- a/data/org.libvirt.Connect.xml
> +++ b/data/org.libvirt.Connect.xml
> @@ -3,6 +3,10 @@
>  
>  
>
> +
> +   +value="See https://libvirt.org/html/libvirt-libvirt-domain.h
> tml#virConnectGetVersion"/>;
> +
>  
>  value="See https://libvirt.org/html/libvirt-libvirt-domain.h
> tml#virConnectListAllDomains"/>;
> diff --git a/src/connect.c b/src/connect.c
> index bf97cd5..8898e6d 100644
> --- a/src/connect.c
> +++ b/src/connect.c
> @@ -80,6 +80,24 @@ virtDBusConnectOpen(virtDBusConnect *connect,
>  return TRUE;
>  }
>  
> +static void
> +virtDBusConnectGetVersion(const gchar *objectPath G_GNUC_UNUSED,
> +  gpointer userData,
> +  GVariant **value,
> +  GError **error)
> +{
> +virtDBusConnect *connect = userData;
> +gulong hvVer;
> +
> +if (!virtDBusConnectOpen(connect, error))
> +return;
> +
> +if (virConnectGetVersion(connect->connection, &hvVer) < 0)
> +return virtDBusUtilSetLastVirtError(error);
> +
> +*value = g_variant_new("t", hvVer);
> +}
> +
>  static void
>  virtDBusConnectListDomains(GVariant *inArgs,
> GUnixFDList *inFDs G_GNUC_UNUSED,
> @@ -177,6 +195,11 @@ virtDBusConnectDefineXML(GVariant *inArgs,
>  *outArgs = g_variant_new("(o)", path);
>  }
>  
> +static virtDBusGDBusPropertyTable virtDBusConnectPropertyTable[] = {
> +{ "Version", virtDBusConnectGetVersion, NULL },
> +{ NULL, NULL, NULL }
> +};
> +
>  static virtDBusGDBusMethodTable virtDBusConnectMethodTable[] = {
>  { "ListDomains", virtDBusConnectListDomains },
>  { "CreateXML", virtDBusConnectCreateXML },
> @@ -228,7 +251,7 @@ virtDBusConnectNew(virtDBusConnect **connectp,
>  connect->connectPath,
>  interfaceInfo,
>  virtDBusConnectMethodTable,
> -NULL,
> +virtDBusConnectPropertyTable,
>  connect);
>  
>  virtDBusDomainRegister(connect, error);
> diff --git a/test/test_connect.py b/test/test_connect.py
> index a52140c..02a7161 100755
> --- a/test/test_connect.py
> +++ b/test/test_connect.py
> @@ -2,6 +2,7 @@
>  
>  import dbus
>  import libvirttest
> +import pytest
>  
>  
>  class TestConnect(libvirttest.BaseTestClass):
> @@ -53,6 +54,13 @@ class TestConnect(libvirttest.BaseTestClass):
>  
>  self.main_loop()
>  
> +@pytest.mark.parametrize("property_name,expected_type", [
> +("Version", dbus.UInt64),
Bad indentation. Will repost.
> +])
> +def test_connect_properties_return_type(self, property_name,
> expected_type):
> +obj = self.bus.get_object('org.libvirt',
> '/org/libvirt/Test')
> +props = obj.GetAll('org.libvirt.Connect',
> dbus_interface=dbus.PROPERTIES_IFACE)
> +assert isinstance(props[property_name], expected_type)
>  
Should be two blank lines here. 
>  if __name__ == '__main__':
>  libvirttest.run()

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

[libvirt] [dbus PATCH 1/2] Add 'Version' property for virConnectGetVersion

[Katerina Koukiou]

---
 data/org.libvirt.Connect.xml |  4 
 src/connect.c| 25 -
 test/test_connect.py |  8 
 3 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/data/org.libvirt.Connect.xml b/data/org.libvirt.Connect.xml
index e47c2f5..56a1126 100644
--- a/data/org.libvirt.Connect.xml
+++ b/data/org.libvirt.Connect.xml
@@ -3,6 +3,10 @@
 
 
   
+
+  https://libvirt.org/html/libvirt-libvirt-domain.html#virConnectGetVersion"/>
+
 
   https://libvirt.org/html/libvirt-libvirt-domain.html#virConnectListAllDomains"/>
diff --git a/src/connect.c b/src/connect.c
index bf97cd5..8898e6d 100644
--- a/src/connect.c
+++ b/src/connect.c
@@ -80,6 +80,24 @@ virtDBusConnectOpen(virtDBusConnect *connect,
 return TRUE;
 }
 
+static void
+virtDBusConnectGetVersion(const gchar *objectPath G_GNUC_UNUSED,
+  gpointer userData,
+  GVariant **value,
+  GError **error)
+{
+virtDBusConnect *connect = userData;
+gulong hvVer;
+
+if (!virtDBusConnectOpen(connect, error))
+return;
+
+if (virConnectGetVersion(connect->connection, &hvVer) < 0)
+return virtDBusUtilSetLastVirtError(error);
+
+*value = g_variant_new("t", hvVer);
+}
+
 static void
 virtDBusConnectListDomains(GVariant *inArgs,
GUnixFDList *inFDs G_GNUC_UNUSED,
@@ -177,6 +195,11 @@ virtDBusConnectDefineXML(GVariant *inArgs,
 *outArgs = g_variant_new("(o)", path);
 }
 
+static virtDBusGDBusPropertyTable virtDBusConnectPropertyTable[] = {
+{ "Version", virtDBusConnectGetVersion, NULL },
+{ NULL, NULL, NULL }
+};
+
 static virtDBusGDBusMethodTable virtDBusConnectMethodTable[] = {
 { "ListDomains", virtDBusConnectListDomains },
 { "CreateXML", virtDBusConnectCreateXML },
@@ -228,7 +251,7 @@ virtDBusConnectNew(virtDBusConnect **connectp,
 connect->connectPath,
 interfaceInfo,
 virtDBusConnectMethodTable,
-NULL,
+virtDBusConnectPropertyTable,
 connect);
 
 virtDBusDomainRegister(connect, error);
diff --git a/test/test_connect.py b/test/test_connect.py
index a52140c..02a7161 100755
--- a/test/test_connect.py
+++ b/test/test_connect.py
@@ -2,6 +2,7 @@
 
 import dbus
 import libvirttest
+import pytest
 
 
 class TestConnect(libvirttest.BaseTestClass):
@@ -53,6 +54,13 @@ class TestConnect(libvirttest.BaseTestClass):
 
 self.main_loop()
 
+@pytest.mark.parametrize("property_name,expected_type", [
+("Version", dbus.UInt64),
+])
+def test_connect_properties_return_type(self, property_name, 
expected_type):
+obj = self.bus.get_object('org.libvirt', '/org/libvirt/Test')
+props = obj.GetAll('org.libvirt.Connect', 
dbus_interface=dbus.PROPERTIES_IFACE)
+assert isinstance(props[property_name], expected_type)
 
 if __name__ == '__main__':
 libvirttest.run()
-- 
2.15.0

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

[libvirt] [dbus PATCH 2/2] Add 'NumOfDomains' property for virConnectGetNumOfDomains

[Katerina Koukiou]

---
 data/org.libvirt.Connect.xml |  4 
 src/connect.c| 20 
 test/test_connect.py |  1 +
 3 files changed, 25 insertions(+)

diff --git a/data/org.libvirt.Connect.xml b/data/org.libvirt.Connect.xml
index 56a1126..a58504d 100644
--- a/data/org.libvirt.Connect.xml
+++ b/data/org.libvirt.Connect.xml
@@ -7,6 +7,10 @@
   https://libvirt.org/html/libvirt-libvirt-domain.html#virConnectGetVersion"/>
 
+
+  https://libvirt.org/html/libvirt-libvirt-domain.html#virConnectNumOfDomains"/>
+
 
   https://libvirt.org/html/libvirt-libvirt-domain.html#virConnectListAllDomains"/>
diff --git a/src/connect.c b/src/connect.c
index 8898e6d..3f01dca 100644
--- a/src/connect.c
+++ b/src/connect.c
@@ -98,6 +98,25 @@ virtDBusConnectGetVersion(const gchar *objectPath 
G_GNUC_UNUSED,
 *value = g_variant_new("t", hvVer);
 }
 
+static void
+virtDBusConnectNumOfDomains(const gchar *objectPath G_GNUC_UNUSED,
+gpointer userData,
+GVariant **value,
+GError **error)
+{
+virtDBusConnect *connect = userData;
+gint numdomains;
+
+if (!virtDBusConnectOpen(connect, error))
+return;
+
+numdomains = virConnectNumOfDomains(connect->connection);
+if (numdomains < 0)
+return virtDBusUtilSetLastVirtError(error);
+
+*value = g_variant_new("u", numdomains);
+}
+
 static void
 virtDBusConnectListDomains(GVariant *inArgs,
GUnixFDList *inFDs G_GNUC_UNUSED,
@@ -197,6 +216,7 @@ virtDBusConnectDefineXML(GVariant *inArgs,
 
 static virtDBusGDBusPropertyTable virtDBusConnectPropertyTable[] = {
 { "Version", virtDBusConnectGetVersion, NULL },
+{ "NumOfDomains", virtDBusConnectNumOfDomains, NULL },
 { NULL, NULL, NULL }
 };
 
diff --git a/test/test_connect.py b/test/test_connect.py
index 02a7161..c40c677 100755
--- a/test/test_connect.py
+++ b/test/test_connect.py
@@ -56,6 +56,7 @@ class TestConnect(libvirttest.BaseTestClass):
 
 @pytest.mark.parametrize("property_name,expected_type", [
 ("Version", dbus.UInt64),
+("NumOfDomains", dbus.UInt32),
 ])
 def test_connect_properties_return_type(self, property_name, 
expected_type):
 obj = self.bus.get_object('org.libvirt', '/org/libvirt/Test')
-- 
2.15.0

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

[libvirt] [dbus PATCH 0/2] Add missing properties from LIBVIRT 0.0.3

[Katerina Koukiou]

Implemented the following properties:
* virConnectGetVersion
* virConnectNumOfDomains

Katerina Koukiou (2):
  Add 'Version' property for virConnectGetVersion
  Add 'NumOfDomains' property for virConnectGetNumOfDomains

 data/org.libvirt.Connect.xml |  8 
 src/connect.c| 45 +++-
 test/test_connect.py |  9 +
 3 files changed, 61 insertions(+), 1 deletion(-)

-- 
2.15.0

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

[libvirt] [PATCH v2 1/3] util: Introduce virDevMapperGetTargets

[Michal Privoznik]

This helper fetches dependencies for given device mapper target.

At the same time, we need to provide a dummy log function because
by default libdevmapper prints out error messages to stderr which
we need to suppress.

Signed-off-by: Michal Privoznik 
---
 src/libvirt_private.syms |   4 ++
 src/util/Makefile.inc.am |   2 +
 src/util/virdevmapper.c  | 160 +++
 src/util/virdevmapper.h  |  32 ++
 4 files changed, 198 insertions(+)
 create mode 100644 src/util/virdevmapper.c
 create mode 100644 src/util/virdevmapper.h

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 03fe3b315f..3b7a19cff1 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1669,6 +1669,10 @@ virDBusMessageUnref;
 virDBusSetSharedBus;
 
 
+# util/virdevmapper.h
+virDevMapperGetTargets;
+
+
 # util/virdnsmasq.h
 dnsmasqAddDhcpHost;
 dnsmasqAddHost;
diff --git a/src/util/Makefile.inc.am b/src/util/Makefile.inc.am
index a3c3b711fd..9624fb687c 100644
--- a/src/util/Makefile.inc.am
+++ b/src/util/Makefile.inc.am
@@ -35,6 +35,8 @@ UTIL_SOURCES = \
util/virdbus.c \
util/virdbus.h \
util/virdbuspriv.h \
+   util/virdevmapper.c \
+   util/virdevmapper.h \
util/virdnsmasq.c \
util/virdnsmasq.h \
util/virebtables.c \
diff --git a/src/util/virdevmapper.c b/src/util/virdevmapper.c
new file mode 100644
index 00..9717482da8
--- /dev/null
+++ b/src/util/virdevmapper.c
@@ -0,0 +1,160 @@
+/*
+ * virdevmapper.c: Functions for handling devmapper
+ *
+ * Copyright (C) 2018 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
+ * .
+ *
+ * Authors:
+ * Michal Privoznik 
+ */
+
+#include 
+
+#ifdef MAJOR_IN_MKDEV
+# include 
+#elif MAJOR_IN_SYSMACROS
+# include 
+#endif
+
+#ifdef WITH_DEVMAPPER
+# include 
+#endif
+
+#include "virdevmapper.h"
+#include "internal.h"
+#include "virthread.h"
+#include "viralloc.h"
+
+#ifdef WITH_DEVMAPPER
+static void
+virDevMapperDummyLogger(int level ATTRIBUTE_UNUSED,
+const char *file ATTRIBUTE_UNUSED,
+int line ATTRIBUTE_UNUSED,
+int dm_errno ATTRIBUTE_UNUSED,
+const char *fmt ATTRIBUTE_UNUSED,
+...)
+{
+return;
+}
+
+static int
+virDevMapperOnceInit(void)
+{
+/* Ideally, we would not need this. But libdevmapper prints
+ * error messages to stderr by default. Sad but true. */
+dm_log_with_errno_init(virDevMapperDummyLogger);
+return 0;
+}
+
+
+VIR_ONCE_GLOBAL_INIT(virDevMapper)
+
+/**
+ * virDevMapperGetTargets:
+ * @path: multipath device
+ * @maj: returned array of MAJOR device numbers
+ * @min: returner array of MINOR device numbers
+ * @nmaj: number of items in @maj array
+ *
+ * For given @path figure out its targets, and store them in @maj
+ * and @min arrays. Both arrays have the same number of items
+ * upon return.
+ *
+ * If @path is not a multipath device, @ndevs is set to 0 and
+ * success is returned.
+ *
+ * If we don't have permissions to talk to kernel, -1 is returned
+ * and errno is set to EBADF.
+ *
+ * Returns 0 on success,
+ *-1 otherwise (with errno set, no libvirt error is
+ *reported)
+ */
+int
+virDevMapperGetTargets(const char *path,
+   unsigned int **maj,
+   unsigned int **min,
+   size_t *nmaj)
+{
+struct dm_task *dmt = NULL;
+struct dm_deps *deps;
+struct dm_info info;
+size_t i;
+int ret = -1;
+
+*nmaj = 0;
+
+if (virDevMapperInitialize() < 0)
+goto cleanup;
+
+if (!(dmt = dm_task_create(DM_DEVICE_DEPS)))
+goto cleanup;
+
+if (!dm_task_set_name(dmt, path)) {
+if (errno == ENOENT) {
+/* It's okay, @path is not managed by devmapper =>
+ * not a multipath device. */
+ret = 0;
+}
+goto cleanup;
+}
+
+dm_task_no_open_count(dmt);
+
+if (!dm_task_run(dmt))
+goto cleanup;
+
+if (!dm_task_get_info(dmt, &info))
+goto cleanup;
+
+if (!info.exists) {
+ret = 0;
+goto cleanup;
+}
+
+if (!(deps = dm_task_get_deps(dmt)))
+goto cleanup;
+
+if (VIR_ALLOC_N_QUIET(*maj, deps->count) < 0 ||
+VIR_ALLOC_N_QUIET(*min, deps->count) 

[libvirt] [PATCH v2 2/3] qemu: Handle device mapper targets properly

[Michal Privoznik]

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

Problem with device mapper targets is that there can be several
other devices 'hidden' behind them. For instance, /dev/dm-1 can
consist of /dev/sda, /dev/sdb and /dev/sdc. Therefore, when
setting up devices CGroup and namespaces we have to take this
into account.

Signed-off-by: Michal Privoznik 
---
 libvirt.spec.in|  2 ++
 src/qemu/qemu_cgroup.c | 42 ++---
 src/qemu/qemu_domain.c | 64 ++
 3 files changed, 105 insertions(+), 3 deletions(-)

diff --git a/libvirt.spec.in b/libvirt.spec.in
index b55a947ec9..ebfac10866 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -796,6 +796,8 @@ Requires: gzip
 Requires: bzip2
 Requires: lzop
 Requires: xz
+# For mpath devices
+Requires: device-mapper
 %if 0%{?fedora} || 0%{?rhel} > 7
 Requires: systemd-container
 %endif
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index b604edb31c..e17b3d21b5 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -37,6 +37,7 @@
 #include "virtypedparam.h"
 #include "virnuma.h"
 #include "virsystemd.h"
+#include "virdevmapper.h"
 
 #define VIR_FROM_THIS VIR_FROM_QEMU
 
@@ -60,7 +61,13 @@ qemuSetupImagePathCgroup(virDomainObjPtr vm,
 {
 qemuDomainObjPrivatePtr priv = vm->privateData;
 int perms = VIR_CGROUP_DEVICE_READ;
-int ret;
+unsigned int *maj = NULL;
+unsigned int *min = NULL;
+size_t nmaj = 0;
+size_t i;
+char *devPath = NULL;
+int rv;
+int ret = -1;
 
 if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES))
 return 0;
@@ -71,12 +78,41 @@ qemuSetupImagePathCgroup(virDomainObjPtr vm,
 VIR_DEBUG("Allow path %s, perms: %s",
   path, virCgroupGetDevicePermsString(perms));
 
-ret = virCgroupAllowDevicePath(priv->cgroup, path, perms, true);
+rv = virCgroupAllowDevicePath(priv->cgroup, path, perms, true);
 
 virDomainAuditCgroupPath(vm, priv->cgroup, "allow", path,
  virCgroupGetDevicePermsString(perms),
- ret);
+ rv);
+if (rv < 0)
+goto cleanup;
 
+if (virDevMapperGetTargets(path, &maj, &min, &nmaj) < 0 &&
+errno != ENOSYS && errno != EBADF) {
+virReportSystemError(errno,
+ _("Unable to get devmapper targets for %s"),
+ path);
+goto cleanup;
+}
+
+for (i = 0; i < nmaj; i++) {
+if (virAsprintf(&devPath, "/dev/block/%u:%u", maj[i], min[i]) < 0)
+goto cleanup;
+
+rv = virCgroupAllowDevicePath(priv->cgroup, devPath, perms, true);
+
+virDomainAuditCgroupPath(vm, priv->cgroup, "allow", devPath,
+ virCgroupGetDevicePermsString(perms),
+ rv);
+if (rv < 0)
+goto cleanup;
+VIR_FREE(devPath);
+}
+
+ret = 0;
+ cleanup:
+VIR_FREE(devPath);
+VIR_FREE(min);
+VIR_FREE(maj);
 return ret;
 }
 
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 580e0f830d..5f56324502 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -54,6 +54,7 @@
 #include "secret_util.h"
 #include "logging/log_manager.h"
 #include "locking/domain_lock.h"
+#include "virdevmapper.h"
 
 #ifdef MAJOR_IN_MKDEV
 # include 
@@ -10108,6 +10109,11 @@ qemuDomainSetupDisk(virQEMUDriverConfigPtr cfg 
ATTRIBUTE_UNUSED,
 {
 virStorageSourcePtr next;
 char *dst = NULL;
+unsigned int *maj = NULL;
+unsigned int *min = NULL;
+size_t nmaj = 0;
+char *devPath = NULL;
+size_t i;
 int ret = -1;
 
 for (next = disk->src; virStorageSourceIsBacking(next); next = 
next->backingStore) {
@@ -10118,10 +10124,31 @@ qemuDomainSetupDisk(virQEMUDriverConfigPtr cfg 
ATTRIBUTE_UNUSED,
 
 if (qemuDomainCreateDevice(next->path, data, false) < 0)
 goto cleanup;
+
+if (virDevMapperGetTargets(next->path, &maj, &min, &nmaj) < 0 &&
+errno != ENOSYS && errno != EBADF) {
+virReportSystemError(errno,
+ _("Unable to get mpath targets for %s"),
+ next->path);
+goto cleanup;
+}
+
+for (i = 0; i < nmaj; i++) {
+if (virAsprintf(&devPath, "/dev/block/%u:%u", maj[i], min[i]) < 0)
+goto cleanup;
+
+if (qemuDomainCreateDevice(devPath, data, false) < 0)
+goto cleanup;
+
+VIR_FREE(devPath);
+}
 }
 
 ret = 0;
  cleanup:
+VIR_FREE(devPath);
+VIR_FREE(min);
+VIR_FREE(maj);
 VIR_FREE(dst);
 return ret;
 }
@@ -11131,6 +11158,12 @@ qemuDomainNamespaceSetupDisk(virDomainObjPtr vm,
 virStorageSourcePtr next;
 char **paths = NULL;
 size_t npaths = 0;
+unsigned int *maj = NULL;
+unsigned int *min = NULL;
+size_t

[libvirt] [PATCH v2 3/3] news: Document device mapper fix

[Michal Privoznik]

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

diff --git a/docs/news.xml b/docs/news.xml
index 1088895746..6f1ceb6389 100644
--- a/docs/news.xml
+++ b/docs/news.xml
@@ -94,6 +94,16 @@
   
 
 
+  
+
+  Improve handling of device mapper targets
+
+
+  When starting a domain with a disk hidden behind
+  devmapper libvirt needs to allow them both in devices
+  CGroup: the devmapper target and the disk.
+
+  
 
   
   
-- 
2.16.1

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

[libvirt] [PATCH v2 0/3] Improve handling of multipath devices

[Michal Privoznik]

v2 of:

https://www.redhat.com/archives/libvir-list/2018-March/msg01541.html

diff to v1:
- Moved code to new file virdevmapper.c
- Rename to virDevMapperGetTargets because this bug is not specific just
  to multipath rather than all device mapper targets
- Reworked logging as suggested in review to v1

Michal Privoznik (3):
  util: Introduce virDevMapperGetTargets
  qemu: Handle device mapper targets properly
  news: Document device mapper fix

 docs/news.xml|  10 +++
 libvirt.spec.in  |   2 +
 src/libvirt_private.syms |   4 ++
 src/qemu/qemu_cgroup.c   |  42 -
 src/qemu/qemu_domain.c   |  64 +++
 src/util/Makefile.inc.am |   2 +
 src/util/virdevmapper.c  | 160 +++
 src/util/virdevmapper.h  |  32 ++
 8 files changed, 313 insertions(+), 3 deletions(-)
 create mode 100644 src/util/virdevmapper.c
 create mode 100644 src/util/virdevmapper.h

-- 
2.16.1

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

[libvirt] [PATCH sandbox] Delete the virt-sandbox-service command

[Daniel P . Berrangé]

This command attempted to create sandboxed containers for running
systemd services that exist on the host. This code has proved very
fragile, however, since it needs heuristics to figure out which dirs
need to be made private in the container vs shared with the host. Even
a relatively simple "httpd.service" sandbox no longer works with
current Fedora.

Users wanting to sandbox services are better served by using systemd's
native container functionality, or using Docker container images. The
virt-sandbox-image tool can even run Docker/virt-builder images directly.

Signed-off-by: Daniel P. Berrangé 
---
 TODO|   24 -
 bin/Makefile.am |   80 +-
 bin/virt-sandbox-service| 1314 ---
 bin/virt-sandbox-service-bash-completion.sh |  141 ---
 bin/virt-sandbox-service-clone.pod  |  100 --
 bin/virt-sandbox-service-connect.pod|   59 --
 bin/virt-sandbox-service-create.pod |  264 --
 bin/virt-sandbox-service-delete.pod |   65 --
 bin/virt-sandbox-service-execute.pod|   71 --
 bin/virt-sandbox-service-reload.pod |   63 --
 bin/virt-sandbox-service-upgrade.pod|   74 --
 bin/virt-sandbox-service-util.c |  305 ---
 bin/virt-sandbox-service.logrotate  |9 -
 bin/virt-sandbox-service.pod|   85 --
 cfg.mk  |2 +-
 libvirt-sandbox.spec.in |7 -
 libvirt-sandbox/tests/containers_test.sh|   37 -
 po/POTFILES.in  |1 -
 18 files changed, 3 insertions(+), 2698 deletions(-)
 delete mode 100644 TODO
 delete mode 100755 bin/virt-sandbox-service
 delete mode 100755 bin/virt-sandbox-service-bash-completion.sh
 delete mode 100644 bin/virt-sandbox-service-clone.pod
 delete mode 100644 bin/virt-sandbox-service-connect.pod
 delete mode 100644 bin/virt-sandbox-service-create.pod
 delete mode 100644 bin/virt-sandbox-service-delete.pod
 delete mode 100644 bin/virt-sandbox-service-execute.pod
 delete mode 100644 bin/virt-sandbox-service-reload.pod
 delete mode 100644 bin/virt-sandbox-service-upgrade.pod
 delete mode 100644 bin/virt-sandbox-service-util.c
 delete mode 100644 bin/virt-sandbox-service.logrotate
 delete mode 100644 bin/virt-sandbox-service.pod
 delete mode 100755 libvirt-sandbox/tests/containers_test.sh

diff --git a/TODO b/TODO
deleted file mode 100644
index fc63361..000
--- a/TODO
+++ /dev/null
@@ -1,24 +0,0 @@
-libvirt-sandbox TODO list
-=
-
-systemd-tmpfiles --create needs to be run within the container, before any
-apps are started, since it will populate /run (Completed)
-
-CGROUPFS: integration so libvirt does it rather then systemd within the 
container
-We need kernel labeling support for cgroupfs so we can allow systemd 
to write to its section of the cgroupfs.
-
-SYSLOG:  Currently syslog messages are going no where within the container.
-If we run a syslog within the container will it get messages from the outside? 
 Should we just use systemd-journal.  I think sysadmins will want to be able to 
look in /var/log/messages within the container. (systemd-journal is now running 
within a container)
-
-EXECUTE:
-   virt-sandbox-service execute --command "BLAH" does not work.  We need 
to have the ability to execute any random command within the container, and get 
stdin, stdout, stderror outside the container. (Partially Completed)
-Still needs kernel to implement missing container namespace files under 
/proc/PID/ns, Also need a mechanism to get the PID of systemd from libvirt.
-
-HOSTNAME:
-   Currently if I execute hostname within the container it sees the name 
of the host not the name based on the container name or the IP Address 
associated with dhclient. (Completed)
-
-virt-sandbox-service connect NAME hangs when you attempt to end the connection.
-^d should bring you back to the host terminal.
-
-Need a mechanism to allow admins to specify additional services to run within
-the container.  For example you may want to run mysql and apache within the 
same container. (Completed) You can do this using systemctl enabel BLAH
diff --git a/bin/Makefile.am b/bin/Makefile.am
index deedcf6..db0a1d1 100644
--- a/bin/Makefile.am
+++ b/bin/Makefile.am
@@ -1,39 +1,12 @@
 
 bin_PROGRAMS = virt-sandbox
 
-libexec_PROGRAMS = virt-sandbox-service-util
+bin_SCRIPTS = virt-sandbox-image
 
-bin_SCRIPTS = virt-sandbox-service \
-   virt-sandbox-image
-
-virtsandboxcompdir = $(datarootdir)/bash-completion/completions/
-
-crondailydir = $(sysconfdir)/cron.daily
-crondaily_SCRIPTS = virt-sandbox-service.logrotate
-
-POD_FILES = \
-   virt-sandbox-service.pod \
-   virt-sandbox-service-execute.pod \
-   virt-sandbox-service-create.pod \
-   virt-sandbox-service-clone.pod \
-   virt-sandbox-service-connect.pod \
-   virt-sandbox-service-delete.pod \
-  

Re: [libvirt] [PATCH v3 1/9] qemu: provide support to query the SEV capability

[Andrea Bolognani]

On Mon, 2018-03-26 at 08:52 -0500, Brijesh Singh wrote:
> > I tried applying just this patch, but kept getting:
> > 
> > 29) caps_2.12.0(x86_64)
> > ... libvirt: QEMU Driver error : internal error: query-cpu-definitions
> > reply data was not an array
> > 
> > a small bit of debugging found that qemuMonitorJSONGetCPUDefinitions was
> > returning NULL for @data after/when the "query-sev-capabilities".
> > 
> > I narrowed it down into the virQEMUCapsInitQMPMonitor when run during
> > qemucapabilitiestest (see [1])
> 
> I have not tried latest libvirt yet, I will try this today and debug to 
> see what we are missing. I did the 'make check' before submitting the 
> patch but at that time QEMU 2.12 was not available and we did not had 
> updated caps_2.12.0.x86_64.xml and caps_2.12.0.x86_64.replies.

I thought the lack of churn in tests/qemucapabilitiesdata/ was
weird, but thanks to your explanation it makes perfect sense now.

Your code adds a call to query-sev-capabilities, but the replies
file doesn't contain the corresponding return data, which makes
the parser go out of sync.

You're going to have to either fetch capabilities from your own
QEMU 2.12 binary or hack it up by adding the return data in the
right spot and call tests/qemucapsfixreplies to re-align the ids.

I think you can get away with the latter, as we're going to want
to refresh the replies files once 2.12 is released anyway.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

[libvirt] Plans for next release

[Daniel Veillard]

 I was reminded that the end of the month is very soon :-)
I suggest to enter freeze tomorrow morning, then going RC2 on Thursday
and then push the release over the week-end or Monday morning

  hope this works for all !

Daniel

-- 
Daniel Veillard  | Red Hat Developers Tools http://developer.redhat.com/
veill...@redhat.com  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | virtualization library  http://libvirt.org/

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

Re: [libvirt] [dbus PATCH 4/4] spec: fix D-Bus spelling and improve description

[Ján Tomko]

On Mon, Mar 26, 2018 at 03:33:53PM +0200, Pavel Hrdina wrote:

Signed-off-by: Pavel Hrdina 
---
libvirt-dbus.spec.in | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)



ACK

Jan


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

Re: [libvirt] [dbus PATCH 3/4] spec: Require dbus and polkit

[Ján Tomko]

On Mon, Mar 26, 2018 at 03:33:52PM +0200, Pavel Hrdina wrote:

libvirt-dbus installs configuration files for these packages
therefore it should depend on it.

Signed-off-by: Pavel Hrdina 
---
libvirt-dbus.spec.in | 2 ++
1 file changed, 2 insertions(+)



ACK

Jan


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

Re: [libvirt] [dbus PATCH 1/4] maint: remove AUTHORS from repository

[Ján Tomko]

On Mon, Mar 26, 2018 at 03:33:50PM +0200, Pavel Hrdina wrote:

This is tracked by git itself.  Suggested during fedora package review.

Signed-off-by: Pavel Hrdina 
---
AUTHORS.in   | 13 -
Makefile.am  | 14 --
libvirt-dbus.spec.in |  2 +-
3 files changed, 1 insertion(+), 28 deletions(-)
delete mode 100644 AUTHORS.in



ACK

Jan


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

Re: [libvirt] [dbus PATCH 1/4] maint: remove AUTHORS from repository

[Daniel P . Berrangé]

On Mon, Mar 26, 2018 at 03:33:50PM +0200, Pavel Hrdina wrote:
> This is tracked by git itself.  Suggested during fedora package review.

AUTHORS is a file that autoconf/automake expects to exist in all
projects. Also the git history is not included with the tar.gz
dist, so IMHO the AUTHORS is appropriate to keep regardless.

> 
> Signed-off-by: Pavel Hrdina 
> ---
>  AUTHORS.in   | 13 -
>  Makefile.am  | 14 --
>  libvirt-dbus.spec.in |  2 +-
>  3 files changed, 1 insertion(+), 28 deletions(-)
>  delete mode 100644 AUTHORS.in
> 
> diff --git a/AUTHORS.in b/AUTHORS.in
> deleted file mode 100644
> index 988dd6a..000
> --- a/AUTHORS.in
> +++ /dev/null
> @@ -1,13 +0,0 @@
> -   libvirt-dbus Authors
> -   
> -
> -The primary maintainers of libvirt-dbus are:
> -
> -Lars Karlitski 
> -Pavel Hrdina 
> -
> -Patches have been received from:
> -
> -#authorslist#
> -
> -   ... send patches to get your name added ...
> diff --git a/Makefile.am b/Makefile.am
> index a890ff1..0c792c1 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -6,7 +6,6 @@ ACLOCAL_AMFLAGS = -I m4
>  EXTRA_DIST = \
>   $(PACKAGE).spec \
>   $(PACKAGE).spec.in \
> - AUTHORS.in \
>   HACKING.md \
>   README.md \
>   $(NULL)
> @@ -15,16 +14,3 @@ DISTCLEAN_FILES = $(PACKAGE).spec
>  
>  rpm: clean
>   @(unset CDPATH ; $(MAKE) dist && rpmbuild -ta $(distdir).tar.gz)
> -
> -dist-hook: gen-AUTHORS
> -
> -# Generate the AUTHORS file (with all entries since the switch to git)
> -# and insert it into the directory we're about to use to create a tarball.
> -.PHONY: gen-AUTHORS
> -gen-AUTHORS:
> - $(AM_V_GEN)if test -d $(srcdir)/.git; then  \
> -   out="`cd $(srcdir) && git log --pretty=format:'%aN <%aE>' | sort -u`" 
> && \
> -   perl -p -e "s/#authorslist#// and print '$$out'"\
> - < $(srcdir)/AUTHORS.in > $(distdir)/AUTHORS-tmp &&\
> -   mv -f $(distdir)/AUTHORS-tmp $(distdir)/AUTHORS ;   \
> - fi
> diff --git a/libvirt-dbus.spec.in b/libvirt-dbus.spec.in
> index 1cf5922..3a04ad6 100644
> --- a/libvirt-dbus.spec.in
> +++ b/libvirt-dbus.spec.in
> @@ -52,7 +52,7 @@ exit 0
>  
>  %files
>  %defattr(-,root,root,-)
> -%doc README.md HACKING.md COPYING AUTHORS NEWS
> +%doc README.md HACKING.md COPYING NEWS
>  %{_sysconfdir}/polkit-1/rules.d/libvirt-dbus.rules
>  %{_bindir}/libvirt-dbus
>  %{_datadir}/dbus-1/services/org.libvirt.service
> -- 
> 2.14.3
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

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

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

Re: [libvirt] [PATCH v3 1/9] qemu: provide support to query the SEV capability

[Brijesh Singh]

Hi John,


On 03/23/2018 02:18 PM, John Ferlan wrote:



On 03/14/2018 11:44 AM, Brijesh Singh wrote:

QEMU version >= 2.12 provides support for launching an encrypted VMs on
AMD x86 platform using Secure Encrypted Virtualization (SEV) feature.
This patch adds support to query the SEV capability from the qemu.

Reviewed-by: "Daniel P. Berrangé" 
Signed-off-by: Brijesh Singh 
---
  src/conf/domain_capabilities.h | 13 
  src/qemu/qemu_capabilities.c   | 38 ++
  src/qemu/qemu_capabilities.h   |  1 +
  src/qemu/qemu_capspriv.h   |  4 +++
  src/qemu/qemu_monitor.c|  9 ++
  src/qemu/qemu_monitor.h|  3 ++
  src/qemu/qemu_monitor_json.c   | 73 ++
  src/qemu/qemu_monitor_json.h   |  3 ++
  8 files changed, 144 insertions(+)



Now that the 2.12 capabilities were added, I started looking through
each of the upstream patch series that would make use of it - this is
just "next" on my list.

I tried applying just this patch, but kept getting:

29) caps_2.12.0(x86_64)
... libvirt: QEMU Driver error : internal error: query-cpu-definitions
reply data was not an array

a small bit of debugging found that qemuMonitorJSONGetCPUDefinitions was
returning NULL for @data after/when the "query-sev-capabilities".

I narrowed it down into the virQEMUCapsInitQMPMonitor when run during
qemucapabilitiestest (see [1])


I have not tried latest libvirt yet, I will try this today and debug to 
see what we are missing. I did the 'make check' before submitting the 
patch but at that time QEMU 2.12 was not available and we did not had 
updated caps_2.12.0.x86_64.xml and caps_2.12.0.x86_64.replies.


I will investigate a bit more and update with my findings.

thanks




diff --git a/src/conf/domain_capabilities.h b/src/conf/domain_capabilities.h
index fa4c1e442f57..72e9daf9120f 100644
--- a/src/conf/domain_capabilities.h
+++ b/src/conf/domain_capabilities.h
@@ -137,6 +137,19 @@ struct _virDomainCapsCPU {
  virDomainCapsCPUModelsPtr custom;
  };
  
+/*

+ * SEV capabilities
+ */
+typedef struct _virSEVCapability virSEVCapability;
+typedef virSEVCapability *virSEVCapabilityPtr;
+struct _virSEVCapability {
+char *pdh;
+char *cert_chain;
+unsigned int cbitpos;
+unsigned int reduced_phys_bits;
+};
+
+
  struct _virDomainCaps {
  virObjectLockable parent;
  
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c

index 3eb5ed6d1a60..6da7cf7477c7 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -459,6 +459,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
"pl011",
"machine.pseries.max-cpu-compat",
"dump-completed",
+  "sev",


This should also be "sev-guest" to be consistent with other
virQEMUCapsObjectTypes entries naming.

BTW: You can/should pull out this entry, the one in
virQEMUCapsObjectTypes and the qemu_capabilities.h change to add the
entry to virQEMUCapsFlags, then build and run:

   VIR_TEST_REGENERATE_OUTPUT=1 tests/qemucapabilitiestest

in order to get/see the changed
tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml file.



  );
  
  
@@ -525,6 +526,8 @@ struct _virQEMUCaps {

  size_t ngicCapabilities;
  virGICCapability *gicCapabilities;
  
+virSEVCapability *sevCapabilities;

+
  virQEMUCapsHostCPUData kvmCPU;
  virQEMUCapsHostCPUData tcgCPU;
  };
@@ -1694,6 +1697,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = {
  { "sclplmconsole", QEMU_CAPS_DEVICE_SCLPLMCONSOLE },
  { "isa-serial", QEMU_CAPS_DEVICE_ISA_SERIAL },
  { "pl011", QEMU_CAPS_DEVICE_PL011 },
+{ "sev-guest", QEMU_CAPS_SEV },
  };
  
  static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsVirtioBalloon[] = {

@@ -2770,6 +2774,21 @@ virQEMUCapsSetGICCapabilities(virQEMUCapsPtr qemuCaps,
  qemuCaps->ngicCapabilities = ncapabilities;
  }
  
+void

+virQEMUCapsSetSEVCapabilities(virQEMUCapsPtr qemuCaps,
+  virSEVCapability *capabilities)
+{
+virSEVCapability *cap = qemuCaps->sevCapabilities;
+
+if (cap) {
+VIR_FREE(cap->pdh);
+VIR_FREE(cap->cert_chain);
+}
+
+VIR_FREE(qemuCaps->sevCapabilities);
+
+qemuCaps->sevCapabilities = capabilities;
+}
  
  static int

  virQEMUCapsProbeQMPCommands(virQEMUCapsPtr qemuCaps,
@@ -3273,6 +3292,19 @@ virQEMUCapsProbeQMPGICCapabilities(virQEMUCapsPtr 
qemuCaps,
  return 0;
  }
  
+static int

+virQEMUCapsProbeQMPSEVCapabilities(virQEMUCapsPtr qemuCaps,
+   qemuMonitorPtr mon)
+{
+virSEVCapability *caps = NULL;
+
+if (qemuMonitorGetSEVCapabilities(mon, &caps) < 0)
+return -1;
+
+virQEMUCapsSetSEVCapabilities(qemuCaps, caps);
+
+return 0;
+}
  
  bool

  virQEMUCapsCPUFilterFeatures(const char *name,
@@ -4906,6 +4938,12 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps,
  virQEMUCapsGet(qemuCaps, QEMU_CAPS_QUERY_CPU_MODEL_EXPANS

Re: [libvirt] [dbus PATCH 2/4] spec: cleanup spec file based on fedora package review

[Ján Tomko]

On Mon, Mar 26, 2018 at 03:33:51PM +0200, Pavel Hrdina wrote:

All of these changes are simple enough so I've put the into single
commit:



Sigh,

Jan


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

[libvirt] [dbus PATCH 0/4] remove AUTHORS file and cleanup spec file

[Pavel Hrdina]

Pavel Hrdina (4):
  maint: remove AUTHORS from repository
  spec: cleanup spec file based on fedora package review
  spec: Require dbus and polkit
  spec: fix D-Bus spelling and improve description

 AUTHORS.in   | 13 -
 Makefile.am  | 14 --
 libvirt-dbus.spec.in | 34 +++---
 3 files changed, 15 insertions(+), 46 deletions(-)
 delete mode 100644 AUTHORS.in

-- 
2.14.3

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

[libvirt] [dbus PATCH 2/4] spec: cleanup spec file based on fedora package review

[Pavel Hrdina]

All of these changes are simple enough so I've put the into single
commit:

- remove obsolete tags and commands
- %global macro is preferred over %define macro
- use %autosetup, %make_build and %make_install macros
- COPYING file should be listed using %license macro

Signed-off-by: Pavel Hrdina 
---
 libvirt-dbus.spec.in | 26 ++
 1 file changed, 10 insertions(+), 16 deletions(-)

diff --git a/libvirt-dbus.spec.in b/libvirt-dbus.spec.in
index 3a04ad6..4b8e752 100644
--- a/libvirt-dbus.spec.in
+++ b/libvirt-dbus.spec.in
@@ -1,19 +1,17 @@
 # -*- rpm-spec -*-
 
-%define glib2_version @GLIB2_REQUIRED@
-%define libvirt_version @LIBVIRT_REQUIRED@
-%define libvirt_glib_version @LIBVIRT_GLIB_REQUIRED@
-%define system_user @SYSTEM_USER@
+%global glib2_version @GLIB2_REQUIRED@
+%global libvirt_version @LIBVIRT_REQUIRED@
+%global libvirt_glib_version @LIBVIRT_GLIB_REQUIRED@
+%global system_user @SYSTEM_USER@
 
 Name: @PACKAGE@
 Version: @VERSION@
-Release: 1%{?dist}%{?extra_release}
+Release: 1%{?dist}
 Summary: libvirt dbus API binding
-Group: Development/Libraries
 License: LGPLv2+
 URL: https://libvirt.org/
 Source0: https://libvirt.org/sources/dbus/%{name}-%{version}.tar.gz
-BuildRoot: %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
 
 BuildRequires: gcc
 BuildRequires: glib2-devel >= %{glib2_version}
@@ -30,18 +28,14 @@ Requires(pre): shadow-utils
 This package provides integration between libvirt and the DBus
 
 %prep
-%setup -q
+%autosetup
 
 %build
 %configure
-%__make %{?_smp_mflags}
+%make_build
 
 %install
-rm -rf $RPM_BUILD_ROOT
-%__make install  DESTDIR=$RPM_BUILD_ROOT
-
-%clean
-rm -rf $RPM_BUILD_ROOT
+%make_install
 
 %pre
 getent group %{system_user} >/dev/null || groupadd -r %{system_user}
@@ -51,8 +45,8 @@ getent passwd %{system_user} >/dev/null || \
 exit 0
 
 %files
-%defattr(-,root,root,-)
-%doc README.md HACKING.md COPYING NEWS
+%doc README.md HACKING.md NEWS
+%license COPYING
 %{_sysconfdir}/polkit-1/rules.d/libvirt-dbus.rules
 %{_bindir}/libvirt-dbus
 %{_datadir}/dbus-1/services/org.libvirt.service
-- 
2.14.3

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

[libvirt] [dbus PATCH 1/4] maint: remove AUTHORS from repository

[Pavel Hrdina]

This is tracked by git itself.  Suggested during fedora package review.

Signed-off-by: Pavel Hrdina 
---
 AUTHORS.in   | 13 -
 Makefile.am  | 14 --
 libvirt-dbus.spec.in |  2 +-
 3 files changed, 1 insertion(+), 28 deletions(-)
 delete mode 100644 AUTHORS.in

diff --git a/AUTHORS.in b/AUTHORS.in
deleted file mode 100644
index 988dd6a..000
--- a/AUTHORS.in
+++ /dev/null
@@ -1,13 +0,0 @@
-   libvirt-dbus Authors
-   
-
-The primary maintainers of libvirt-dbus are:
-
-Lars Karlitski 
-Pavel Hrdina 
-
-Patches have been received from:
-
-#authorslist#
-
-   ... send patches to get your name added ...
diff --git a/Makefile.am b/Makefile.am
index a890ff1..0c792c1 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -6,7 +6,6 @@ ACLOCAL_AMFLAGS = -I m4
 EXTRA_DIST = \
$(PACKAGE).spec \
$(PACKAGE).spec.in \
-   AUTHORS.in \
HACKING.md \
README.md \
$(NULL)
@@ -15,16 +14,3 @@ DISTCLEAN_FILES = $(PACKAGE).spec
 
 rpm: clean
@(unset CDPATH ; $(MAKE) dist && rpmbuild -ta $(distdir).tar.gz)
-
-dist-hook: gen-AUTHORS
-
-# Generate the AUTHORS file (with all entries since the switch to git)
-# and insert it into the directory we're about to use to create a tarball.
-.PHONY: gen-AUTHORS
-gen-AUTHORS:
-   $(AM_V_GEN)if test -d $(srcdir)/.git; then  \
- out="`cd $(srcdir) && git log --pretty=format:'%aN <%aE>' | sort -u`" 
&& \
- perl -p -e "s/#authorslist#// and print '$$out'"\
-   < $(srcdir)/AUTHORS.in > $(distdir)/AUTHORS-tmp &&\
- mv -f $(distdir)/AUTHORS-tmp $(distdir)/AUTHORS ;   \
-   fi
diff --git a/libvirt-dbus.spec.in b/libvirt-dbus.spec.in
index 1cf5922..3a04ad6 100644
--- a/libvirt-dbus.spec.in
+++ b/libvirt-dbus.spec.in
@@ -52,7 +52,7 @@ exit 0
 
 %files
 %defattr(-,root,root,-)
-%doc README.md HACKING.md COPYING AUTHORS NEWS
+%doc README.md HACKING.md COPYING NEWS
 %{_sysconfdir}/polkit-1/rules.d/libvirt-dbus.rules
 %{_bindir}/libvirt-dbus
 %{_datadir}/dbus-1/services/org.libvirt.service
-- 
2.14.3

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

[libvirt] [dbus PATCH 4/4] spec: fix D-Bus spelling and improve description

[Pavel Hrdina]

Signed-off-by: Pavel Hrdina 
---
 libvirt-dbus.spec.in | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/libvirt-dbus.spec.in b/libvirt-dbus.spec.in
index f0e63b5..675f463 100644
--- a/libvirt-dbus.spec.in
+++ b/libvirt-dbus.spec.in
@@ -8,7 +8,7 @@
 Name: @PACKAGE@
 Version: @VERSION@
 Release: 1%{?dist}
-Summary: libvirt dbus API binding
+Summary: libvirt D-Bus API binding
 License: LGPLv2+
 URL: https://libvirt.org/
 Source0: https://libvirt.org/sources/dbus/%{name}-%{version}.tar.gz
@@ -27,7 +27,7 @@ Requires: polkit
 Requires(pre): shadow-utils
 
 %description
-This package provides integration between libvirt and the DBus
+This package provides D-Bus API for libvirt
 
 %prep
 %autosetup
@@ -43,7 +43,7 @@ This package provides integration between libvirt and the DBus
 getent group %{system_user} >/dev/null || groupadd -r %{system_user}
 getent passwd %{system_user} >/dev/null || \
 useradd -r -g %{system_user} -d / -s /sbin/nologin \
--c "Libvirt DBus bridge" %{system_user}
+-c "Libvirt D-Bus bridge" %{system_user}
 exit 0
 
 %files
-- 
2.14.3

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

[libvirt] [dbus PATCH 3/4] spec: Require dbus and polkit

[Pavel Hrdina]

libvirt-dbus installs configuration files for these packages
therefore it should depend on it.

Signed-off-by: Pavel Hrdina 
---
 libvirt-dbus.spec.in | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/libvirt-dbus.spec.in b/libvirt-dbus.spec.in
index 4b8e752..f0e63b5 100644
--- a/libvirt-dbus.spec.in
+++ b/libvirt-dbus.spec.in
@@ -18,9 +18,11 @@ BuildRequires: glib2-devel >= %{glib2_version}
 BuildRequires: libvirt-devel >= %{libvirt_version}
 BuildRequires: libvirt-glib-devel >= %{libvirt_glib_version}
 
+Requires: dbus
 Requires: glib2 >= %{glib2_version}
 Requires: libvirt-libs >= %{libvirt_version}
 Requires: libvirt-glib >= %{libvirt_glib_version}
+Requires: polkit
 
 Requires(pre): shadow-utils
 
-- 
2.14.3

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

Re: [libvirt] [PATCH 2/3] qemu: support passing pre-opened UNIX socket listen FD

[Daniel P . Berrangé]

On Mon, Mar 26, 2018 at 03:19:36PM +0200, Ján Tomko wrote:
> On Wed, Mar 14, 2018 at 05:33:05PM +, Daniel P. Berrangé wrote:
> > There is a race condition when spawning QEMU where libvirt has spawned
> > QEMU but the monitor socket is not yet open. Libvirt has to repeatedly
> > try to connect() to QEMU's monitor until eventually it succeeds, or
> > times out. We use kill() to check if QEMU is still alive so we avoid
> > waiting a long time if QEMU exited, but having a timeout at all is still
> > unpleasant.
> > 
> > With QEMU 2.12 we can pass in a pre-opened FD for UNIX domain or TCP
> > sockets. If libvirt has called bind() and listen() on this FD, then we
> > have a guarantee that libvirt can immediately call connect() and
> > succeed without any race.
> > 
> > Although we only really care about this for the monitor socket and agent
> > socket, this patch does FD passing for all UNIX socket based character
> > devices since there appears to be no downside to it.
> > 
> > We don't do FD passing for TCP sockets, however, because it is only
> > possible to pass a single FD, while some hostnames may require listening
> > on multiple FDs to cover IPv4 and IPv6 concurrently.
> > 
> > Signed-off-by: Daniel P. Berrangé 
> > ---
> > src/qemu/qemu_command.c | 58 
> > +++--
> > 1 file changed, 56 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > index fa0aa5d5c3..291aad13cf 100644
> > --- a/src/qemu/qemu_command.c
> > +++ b/src/qemu/qemu_command.c
> > @@ -5010,8 +5010,62 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager,
> > break;
> > 
> > case VIR_DOMAIN_CHR_TYPE_UNIX:
> > -virBufferAsprintf(&buf, "socket,id=%s,path=", charAlias);
> > -virQEMUBuildBufferEscapeComma(&buf, dev->data.nix.path);
> > +#ifndef WIN32
> 
> Why the ifdef?
> 
> Do we even build QEMU driver on WIN32?

Good point - this was just habit for any UNIX sockets code.

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

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

Re: [libvirt] [PATCH 2/3] qemu: support passing pre-opened UNIX socket listen FD

[Ján Tomko]

On Wed, Mar 14, 2018 at 05:33:05PM +, Daniel P. Berrangé wrote:

There is a race condition when spawning QEMU where libvirt has spawned
QEMU but the monitor socket is not yet open. Libvirt has to repeatedly
try to connect() to QEMU's monitor until eventually it succeeds, or
times out. We use kill() to check if QEMU is still alive so we avoid
waiting a long time if QEMU exited, but having a timeout at all is still
unpleasant.

With QEMU 2.12 we can pass in a pre-opened FD for UNIX domain or TCP
sockets. If libvirt has called bind() and listen() on this FD, then we
have a guarantee that libvirt can immediately call connect() and
succeed without any race.

Although we only really care about this for the monitor socket and agent
socket, this patch does FD passing for all UNIX socket based character
devices since there appears to be no downside to it.

We don't do FD passing for TCP sockets, however, because it is only
possible to pass a single FD, while some hostnames may require listening
on multiple FDs to cover IPv4 and IPv6 concurrently.

Signed-off-by: Daniel P. Berrangé 
---
src/qemu/qemu_command.c | 58 +++--
1 file changed, 56 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index fa0aa5d5c3..291aad13cf 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -5010,8 +5010,62 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager,
break;

case VIR_DOMAIN_CHR_TYPE_UNIX:
-virBufferAsprintf(&buf, "socket,id=%s,path=", charAlias);
-virQEMUBuildBufferEscapeComma(&buf, dev->data.nix.path);
+#ifndef WIN32


Why the ifdef?

Do we even build QEMU driver on WIN32?

Jan


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

Re: [libvirt] [PATCHv2 2/6] qemu: use query-cpus-fast in JSON monitor

[John Ferlan]

On 03/26/2018 07:28 AM, Viktor Mihajlovski wrote:
> On 26.03.2018 10:12, Viktor Mihajlovski wrote:
>> On 23.03.2018 17:05, John Ferlan wrote:
>> [...]
 diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
 index 8b4efc8..4079fb3 100644
 --- a/src/qemu/qemu_domain.c
 +++ b/src/qemu/qemu_domain.c
 @@ -8683,7 +8683,12 @@ qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver,
  if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
  return -1;
  
 -rc = qemuMonitorGetCPUInfo(qemuDomainGetMonitor(vm), &info, maxvcpus, 
 hotplug);
 +rc = qemuMonitorGetCPUInfo(qemuDomainGetMonitor(vm),
 +   &info,
 +   maxvcpus,
 +   hotplug,
 +   
 virQEMUCapsGet(QEMU_DOMAIN_PRIVATE(vm)->qemuCaps,
 +  QEMU_CAPS_QUERY_CPUS_FAST));
>>>
>>> Perhaps we should create a qemuMonitorGetCPUInfoFast API since we should
>>> also be able to return the @props {core-id, thread-id, socket-id} and
>>> report them. I'm not steadfast on this suggestion - maybe Peter has a
>>> stronger opinion one way or the other. Still it allows future
>>> adjustments and additions to -fast to be more easily "handled".
>>>
>>> It would seem those values could be useful although I do see there could
>>> be some "impact" with how hotplug works and the default setting to -1 of
>>> the values by qemuMonitorCPUInfoClear.
>> Actually, query-cpus[-fast] is not reporting the topology (socket, core,
>> thread). The reported thread id is referring to the QEMU CPU thread.
> Well, I have to take that back as I ignored that QEMU is providing the
> props since 2.10 with query-cpus (and now with query-cpus-fast). If we
> decide to parse the props, we could do it with the pre-existing
> qemuMonitorGetCPUInfo function, but I would prefer this to be done in
> separate patch.
> [...]
> 

I didn't even look at the query-cpus and @props - I was just considering
the -fast code.  And yes, looking at @props for query-cpus would have to
be a separate patch with it's own capability - if of course it was even
deemed worthwhile to do...

John

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

Re: [libvirt] [PATCH 2/3] qemu: support passing pre-opened UNIX socket listen FD

[John Ferlan]

On 03/14/2018 01:33 PM, Daniel P. Berrangé wrote:
> There is a race condition when spawning QEMU where libvirt has spawned
> QEMU but the monitor socket is not yet open. Libvirt has to repeatedly
> try to connect() to QEMU's monitor until eventually it succeeds, or
> times out. We use kill() to check if QEMU is still alive so we avoid
> waiting a long time if QEMU exited, but having a timeout at all is still
> unpleasant.
> 
> With QEMU 2.12 we can pass in a pre-opened FD for UNIX domain or TCP
> sockets. If libvirt has called bind() and listen() on this FD, then we
> have a guarantee that libvirt can immediately call connect() and
> succeed without any race.
> 
> Although we only really care about this for the monitor socket and agent
> socket, this patch does FD passing for all UNIX socket based character
> devices since there appears to be no downside to it.
> 
> We don't do FD passing for TCP sockets, however, because it is only
> possible to pass a single FD, while some hostnames may require listening
> on multiple FDs to cover IPv4 and IPv6 concurrently.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  src/qemu/qemu_command.c | 58 
> +++--
>  1 file changed, 56 insertions(+), 2 deletions(-)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index fa0aa5d5c3..291aad13cf 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -5010,8 +5010,62 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager,
>  break;
>  
>  case VIR_DOMAIN_CHR_TYPE_UNIX:
> -virBufferAsprintf(&buf, "socket,id=%s,path=", charAlias);
> -virQEMUBuildBufferEscapeComma(&buf, dev->data.nix.path);
> +#ifndef WIN32
> +if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_CHARDEV_FD_PASS)) {
> +struct sockaddr_un addr;
> +socklen_t addrlen = sizeof(addr);
> +int fd;
> +
> +if ((fd = socket(AF_UNIX, SOCK_STREAM, 0)) < 0) {
> +virReportSystemError(errno, "%s",
> + _("Unable to create UNIX socket"));
> +goto cleanup;
> +}
> +
> +memset(&addr, 0, sizeof(addr));
> +addr.sun_family = AF_UNIX;
> +if (virStrcpyStatic(addr.sun_path, dev->data.nix.path) == NULL) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("Monitor path %s too big for destination"),
> +   dev->data.nix.path);

The commit message implies more general than monitor path...

> +VIR_FORCE_CLOSE(fd);
> +goto cleanup;
> +}
> +
> +if (unlink(dev->data.nix.path) < 0 && errno != ENOENT) {
> +virReportSystemError(errno,
> + _("Unable to unlink %s"),
> + dev->data.nix.path);
> +VIR_FORCE_CLOSE(fd);
> +goto cleanup;
> +}
> +
> +if (bind(fd, (struct sockaddr *)&addr, addrlen) < 0) {
> +virReportSystemError(errno,
> + _("Unable to bind to monitor %s"),
> + dev->data.nix.path);

Same here...

> +VIR_FORCE_CLOSE(fd);
> +goto cleanup;
> +}
> +
> +if (listen(fd, 1) < 0) {
> +virReportSystemError(errno,
> + _("Unable to listen to monitor %s"),
> + dev->data.nix.path);

Again...

W/ minor adjustments,

Reviewed-by: John Ferlan 

John

> +VIR_FORCE_CLOSE(fd);
> +goto cleanup;
> +}
> +
> +virBufferAsprintf(&buf, "socket,id=%s,fd=%d", charAlias, fd);
> +
> +virCommandPassFD(cmd, fd, VIR_COMMAND_PASS_FD_CLOSE_PARENT);
> +} else {
> +#endif /* WIN32 */
> +virBufferAsprintf(&buf, "socket,id=%s,path=", charAlias);
> +virQEMUBuildBufferEscapeComma(&buf, dev->data.nix.path);
> +#ifndef WIN32
> +}
> +#endif /* WIN32 */
>  if (dev->data.nix.listen)
>  virBufferAdd(&buf, nowait ? ",server,nowait" : ",server", -1);
>  
> 

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

Re: [libvirt] [PATCHv2 4/6] qemu: add architecture-specific CPU info handling

[Viktor Mihajlovski]

On 23.03.2018 17:08, John Ferlan wrote:
[...]
>> +static void
>> +qemuMonitorJSONExtractCPUS390Info(virJSONValuePtr jsoncpu,
>> +  struct qemuMonitorQueryCpusEntry *cpu)
>> +{
>> +const char *cpu_state = virJSONValueObjectGetString(jsoncpu, 
>> "cpu-state");
>> +
>> +if (STREQ_NULLABLE(cpu_state, "operating") ||
>> +STREQ_NULLABLE(cpu_state, "load"))
>> +cpu->halted = false;
>> +else if (STREQ_NULLABLE(cpu_state, "stopped") ||
>> + STREQ_NULLABLE(cpu_state, "check-stop"))
>> +cpu->halted = true;
> 
> Realistically, since false is the default, then only "stopped" and
> "check-stop" need to be checked... Even 'uninitialized' would show up as
> false since it's not checked.
> 
As you say, it's not necessary to handle the false case explicitly.
Still, I would like to be explicit here.
> Perhaps you should create and carry up the calling stack a copy of the
> cpu-state that way eventually it could be printed in a 'stats' output as
> well...
> 
> I suppose another concern is that some future halted state is created
> and this code does account for that leading to incorrect reporting and
> well some other issues since @halted is used for various decisions.
> 
At this point in time I'm mainly concerned about providing the same
client behaviour with both query-cpus and query-cpus-fast.
In the long run it might make sense to provide architecture specific CPU
information, but this will require more thought and discussion.
>> +}
>> +
>> +/**
>> + * qemuMonitorJSONExtractCPUArchInfo:
>> + * @arch: virtual CPU's architecture
>> + * @jsoncpu: pointer to a single JSON cpu entry
>> + * @cpu: pointer to a single cpu entry
>>   *
>> + * Extracts architecure specific virtual CPU data for a single
>> + * CPU from the QAPI response using an architecture specific
>> + * function.
>> + *
>> + */
>> +static void
>> +qemuMonitorJSONExtractCPUArchInfo(const char *arch,
>> +  virJSONValuePtr jsoncpu,
>> +  struct qemuMonitorQueryCpusEntry *cpu)
>> +{
>> +if (STREQ_NULLABLE(arch, "s390"))
>> +qemuMonitorJSONExtractCPUS390Info(jsoncpu, cpu);
>> +}
>> +
>> +/**
>> + * qemuMonitorJSONExtractCPUArchInfo:
> 
> ^^ This is still qemuMonitorJSONExtractCPUInfo
> 
oops
>> + * @data: JSON response data
>> + * @entries: filled with detected cpu entries on success
>> + * @nentries: number of entries returned
>> + * @fast: true if this is a response from query-cpus-fast
>> + *
>> + * The JSON response @data will have the following format
>> + * in case @fast == false
>>   * [{ "arch": "x86",
>>   *"current": true,
>>   *"CPU": 0,
>>   *"qom_path": "/machine/unattached/device[0]",
>>   *"pc": -2130415978,
>>   *"halted": true,
>> - *"thread_id": 2631237},
>> + *"thread_id": 2631237,
>> + *...},
>> + *{...}
>> + *  ]
>> + * and for @fast == true
>> + * [{ "arch": "x86",
>> + *"cpu-index": 0,
>> + *"qom-path": "/machine/unattached/device[0]",
>> + *"thread_id": 2631237,
>> + *...},
> 
> May as well show the whole example and even provide a s390 example so
> that it's more obvious...
> can do
>>   *{...}
>>   *  ]
>>   */
>> @@ -1486,6 +1556,7 @@ qemuMonitorJSONExtractCPUInfo(virJSONValuePtr data,
>>  int thread = 0;
>>  bool halted = false;
>>  const char *qom_path;
>> +const char *arch;
>>  if (!entry) {
>>  ret = -2;
>>  goto cleanup;
>> @@ -1511,6 +1582,10 @@ qemuMonitorJSONExtractCPUInfo(virJSONValuePtr data,
>>  cpus[i].halted = halted;
>>  if (VIR_STRDUP(cpus[i].qom_path, qom_path) < 0)
>>  goto cleanup;
>> +
>> +/* process optional architecture-specific data */
>> +arch = virJSONValueObjectGetString(entry, "arch");
>> +qemuMonitorJSONExtractCPUArchInfo(arch, entry, cpus + i);
> 
> Since you're passing @entry anyway, you could fetch @arch in
> qemuMonitorJSONExtractCPUArchInfo and since it's only valid for "fast ==
> true", calling should be gated on that; otherwise, this would be called
> for both fast options.
I can push the architecture extraction down the stack, but I wouldn't
make the call optional. While not used, there's still
architecture-specific information returned in query-cpus.
> 
> BTW: Rather than "cpus[i]" and "cpus + i", could we just create a
> local/stack "cpu" and use it?
> 
I'll have a look.
> 
> John
> 
>>  }
>>  
>>  VIR_STEAL_PTR(*entries, cpus);
>>
> 


-- 
Regards,
 Viktor Mihajlovski

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

Re: [libvirt] [PATCH 3/3] qemu: don't retry connect() if doing FD passing

[John Ferlan]

On 03/14/2018 01:33 PM, Daniel P. Berrangé wrote:
> Since libvirt called bind() and listen() on the UNIX socket, it is
> guaranteed that connect() will immediately succeed, if QEMU is running
> normally. It will only fail if QEMU has closed the monitor socket by
> mistake or if QEMU has exited, letting the kernel close it.
> 
> With this in mind we can remove the retry loop and timeout when
> connecting to the QEMU monitor if we are doing FD passing. Libvirt can
> go straight to sending the QMP greeting and will simply block waiting
> for a reply until QEMU is ready.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  src/qemu/qemu_capabilities.c |  2 +-
>  src/qemu/qemu_monitor.c  | 54 
> ++--
>  src/qemu/qemu_monitor.h  |  1 +
>  src/qemu/qemu_process.c  | 27 --
>  tests/qemumonitortestutils.c |  1 +
>  5 files changed, 55 insertions(+), 30 deletions(-)
> 

So just doing the monitor for now... Eventually the agent too?

How about having a news.xml patch - is this a newsworthy change?


[...]

> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 57c06c7c15..9950461810 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -1771,7 +1771,7 @@ qemuProcessInitMonitor(virQEMUDriverPtr driver,
>  
>  static int
>  qemuConnectMonitor(virQEMUDriverPtr driver, virDomainObjPtr vm, int asyncJob,
> -   qemuDomainLogContextPtr logCtxt)
> +   bool retry, qemuDomainLogContextPtr logCtxt)
>  {
>  qemuDomainObjPrivatePtr priv = vm->privateData;
>  qemuMonitorPtr mon = NULL;
> @@ -1799,6 +1799,7 @@ qemuConnectMonitor(virQEMUDriverPtr driver, 
> virDomainObjPtr vm, int asyncJob,
>  mon = qemuMonitorOpen(vm,
>priv->monConfig,
>priv->monJSON,
> +  retry,
>timeout,
>&monitorCallbacks,
>driver);
> @@ -2174,17 +2175,23 @@ qemuProcessWaitForMonitor(virQEMUDriverPtr driver,
>  {
>  int ret = -1;
>  virHashTablePtr info = NULL;
> -qemuDomainObjPrivatePtr priv;
> +qemuDomainObjPrivatePtr priv = vm->privateData;
> +bool retry = true;

This is also called from qemuProcessAttach where it wouldn't seem
there'd be necessarily be a chardev on the command line with the
pre-opened fd, but then again since it's being used for an already
running qemu instance, that would "I hope" work properly...

With that assumption in place,

Reviewed-by: John Ferlan 

John

> +
> +if (priv->qemuCaps &&
> +virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_CHARDEV_FD_PASS))
> +retry = false;
>  
> -VIR_DEBUG("Connect monitor to %p '%s'", vm, vm->def->name);
> -if (qemuConnectMonitor(driver, vm, asyncJob, logCtxt) < 0)
> +VIR_DEBUG("Connect monitor to vm=%p name='%s' retry=%d",
> +  vm, vm->def->name, retry);
> +
> +if (qemuConnectMonitor(driver, vm, asyncJob, retry, logCtxt) < 0)
>  goto cleanup;
>  
>  /* Try to get the pty path mappings again via the monitor. This is much 
> more
>   * reliable if it's available.
>   * Note that the monitor itself can be on a pty, so we still need to try 
> the
>   * log output method. */
> -priv = vm->privateData;
>  if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
>  goto cleanup;
>  ret = qemuMonitorGetChardevInfo(priv->mon, &info);

[...]

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

Re: [libvirt] [PATCH 1/3] qemu: probe for -chardev 'fd' parameter for FD passing

[John Ferlan]

On 03/14/2018 01:33 PM, Daniel P. Berrangé wrote:
> QEMU >= 2.12 will support passing of pre-opened file descriptors for
> socket based character devices.
> 
>  should we include 2.12 pre-release git snapshot of capaibilities ?
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  src/qemu/qemu_capabilities.c | 2 ++
>  src/qemu/qemu_capabilities.h | 1 +
>  2 files changed, 3 insertions(+)
> 

Going to need some merging and the above commit message will need some
tweaking and of course the qemucapabilitiestest run to update the 2.12
caps files to add the flag... With those adjustments,

Reviewed-by: John Ferlan 

John

> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 3eb5ed6d1a..c20dc32126 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -459,6 +459,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
>"pl011",
>"machine.pseries.max-cpu-compat",
>"dump-completed",
> +  "chardev-fd-pass",
>  );
>  
>  
> @@ -3196,6 +3197,7 @@ static struct virQEMUCapsCommandLineProps 
> virQEMUCapsCommandLine[] = {
>  { "machine", "loadparm", QEMU_CAPS_LOADPARM },
>  { "vnc", "vnc", QEMU_CAPS_VNC_MULTI_SERVERS },
>  { "chardev", "reconnect", QEMU_CAPS_CHARDEV_RECONNECT },
> +{ "chardev", "fd", QEMU_CAPS_CHARDEV_FD_PASS },
>  };
>  
>  static int
> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
> index c2ec2be193..aedf3c2d5e 100644
> --- a/src/qemu/qemu_capabilities.h
> +++ b/src/qemu/qemu_capabilities.h
> @@ -444,6 +444,7 @@ typedef enum {
>  QEMU_CAPS_DEVICE_PL011, /* -device pl011 (not user-instantiable) */
>  QEMU_CAPS_MACHINE_PSERIES_MAX_CPU_COMPAT, /* -machine 
> pseries,max-cpu-compat= */
>  QEMU_CAPS_DUMP_COMPLETED, /* DUMP_COMPLETED event */
> +QEMU_CAPS_CHARDEV_FD_PASS, /* Passing pre-opened FDs for chardevs */
>  
>  QEMU_CAPS_LAST /* this must always be the last item */
>  } virQEMUCapsFlags;
> 

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

Re: [libvirt] [PATCHv2 2/6] qemu: use query-cpus-fast in JSON monitor

[Viktor Mihajlovski]

On 26.03.2018 10:12, Viktor Mihajlovski wrote:
> On 23.03.2018 17:05, John Ferlan wrote:
> [...]
>>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>>> index 8b4efc8..4079fb3 100644
>>> --- a/src/qemu/qemu_domain.c
>>> +++ b/src/qemu/qemu_domain.c
>>> @@ -8683,7 +8683,12 @@ qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver,
>>>  if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
>>>  return -1;
>>>  
>>> -rc = qemuMonitorGetCPUInfo(qemuDomainGetMonitor(vm), &info, maxvcpus, 
>>> hotplug);
>>> +rc = qemuMonitorGetCPUInfo(qemuDomainGetMonitor(vm),
>>> +   &info,
>>> +   maxvcpus,
>>> +   hotplug,
>>> +   
>>> virQEMUCapsGet(QEMU_DOMAIN_PRIVATE(vm)->qemuCaps,
>>> +  QEMU_CAPS_QUERY_CPUS_FAST));
>>
>> Perhaps we should create a qemuMonitorGetCPUInfoFast API since we should
>> also be able to return the @props {core-id, thread-id, socket-id} and
>> report them. I'm not steadfast on this suggestion - maybe Peter has a
>> stronger opinion one way or the other. Still it allows future
>> adjustments and additions to -fast to be more easily "handled".
>>
>> It would seem those values could be useful although I do see there could
>> be some "impact" with how hotplug works and the default setting to -1 of
>> the values by qemuMonitorCPUInfoClear.
> Actually, query-cpus[-fast] is not reporting the topology (socket, core,
> thread). The reported thread id is referring to the QEMU CPU thread.
Well, I have to take that back as I ignored that QEMU is providing the
props since 2.10 with query-cpus (and now with query-cpus-fast). If we
decide to parse the props, we could do it with the pre-existing
qemuMonitorGetCPUInfo function, but I would prefer this to be done in
separate patch.
[...]

-- 
Regards,
 Viktor Mihajlovski

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

Re: [libvirt] [PATCH sandbox] Fix argparser incompatibilities with newer python 3

[Cedric Bosdonnat]

On Fri, 2018-03-23 at 16:44 +, Daniel P. Berrangé wrote:
> Python 3 changes such that if no subparser command is listed, it just
> throws an error. To get back the old behavior we need to set the
> 'required'  attribute and a dest name.
> 
> Since we treat 'debug' as a global attribute we need to set that on the
> top level parser too, otherwise we get a missing attribute error with
> newish python 3 too.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  libvirt-sandbox/image/cli.py | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/libvirt-sandbox/image/cli.py b/libvirt-sandbox/image/cli.py
> index 490c5e0..d2035de 100644
> --- a/libvirt-sandbox/image/cli.py
> +++ b/libvirt-sandbox/image/cli.py
> @@ -202,7 +202,6 @@ Example supported URI formats:
>  def gen_purge_args(subparser):
>  parser = gen_command_parser(subparser, "purge",
>  _("Purge cached template"))
> -requires_debug(parser)
>  requires_template(parser)
>  requires_template_dir(parser)
>  parser.set_defaults(func=purge)
> @@ -210,7 +209,6 @@ def gen_purge_args(subparser):
>  def gen_prepare_args(subparser):
>  parser = gen_command_parser(subparser, "prepare",
>  _("Prepare local template"))
> -requires_debug(parser)
>  requires_template(parser)
>  requires_connect(parser)
>  requires_template_dir(parser)
> @@ -219,7 +217,6 @@ def gen_prepare_args(subparser):
>  def gen_run_args(subparser):
>  parser = gen_command_parser(subparser, "run",
>  _("Run an instance of a template"))
> -requires_debug(parser)
>  requires_name(parser)
>  requires_template(parser)
>  requires_connect(parser)
> @@ -238,7 +235,6 @@ def gen_run_args(subparser):
>  def gen_list_args(subparser):
>  parser = gen_command_parser(subparser, "list",
>  _("List locally cached images"))
> -requires_debug(parser)
>  requires_template_dir(parser)
>  
>  parser.add_argument("-s","--source",
> @@ -249,7 +245,11 @@ def gen_list_args(subparser):
>  def main():
>  parser = argparse.ArgumentParser(description="Sandbox Container Image 
> Tool")
>  
> +requires_debug(parser)
> +
>  subparser = parser.add_subparsers(help=_("commands"))
> +subparser.required = True
> +subparser.dest = "command"
>  gen_purge_args(subparser)
>  gen_prepare_args(subparser)
>  gen_run_args(subparser)

ACK
--
Cedric

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

Re: [libvirt] [PATCHv2 3/6] tests: add qemumonitorjson tests for query-cpus-fast

[Viktor Mihajlovski]

On 23.03.2018 17:07, John Ferlan wrote:
> 
> 
> On 03/05/2018 06:44 AM, Viktor Mihajlovski wrote:
>> Extended the json monitor test program with support for query-cpus-fast
>> and added a sample file set for x86 data obtained using the it.
>>
>> Signed-off-by: Viktor Mihajlovski 
>> ---
>>  ...qemumonitorjson-cpuinfo-x86-full-fast-cpus.json |  71 +
>>  ...umonitorjson-cpuinfo-x86-full-fast-hotplug.json | 115 
>> 
>>  .../qemumonitorjson-cpuinfo-x86-full-fast.data | 109 +++
>>  tests/qemumonitorjsontest.c| 118 
>> -
>>  tests/qemumonitortestutils.c   |   6 ++
>>  tests/qemumonitortestutils.h   |   1 +
>>  6 files changed, 394 insertions(+), 26 deletions(-)
>>  create mode 100644 
>> tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-x86-full-fast-cpus.json
>>  create mode 100644 
>> tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-x86-full-fast-hotplug.json
>>  create mode 100644 
>> tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-x86-full-fast.data
>>
>> diff --git 
>> a/tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-x86-full-fast-cpus.json 
>> b/tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-x86-full-fast-cpus.json
>> new file mode 100644
>> index 000..88fd2b8
>> --- /dev/null
>> +++ 
>> b/tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-x86-full-fast-cpus.json
>> @@ -0,0 +1,71 @@
>> +{
>> +  "return": [
>> +{
>> +  "arch": "x86",
>> +  "cpu-index": 0,
>> +  "qom-path": "/machine/unattached/device[0]",
>> +  "thread-id": 895040
>> +},
> 
> Each of these seems to be missing the "props" structure even though the
> hotplug does have it.  As noted in my 2/6 review - that could be useful
> data to parse (and return and save for printing in the stats output). In
> fact I recall those values being the most painful when dealing with this
> the last time I was reviewing in here...
> 
Hm ... you are right, QEMUs implementing query-cpus-fast will have the
props structure filled, so we should account for it.
>> +{
>> +  "arch": "x86",
>> +  "cpu-index": 1,
>> +  "qom-path": "/machine/peripheral/vcpu1",
>> +  "thread-id": 895056
>> +},
> 
> [...]
> 
>> diff --git 
>> a/tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-x86-full-fast-hotplug.json
>>  
>> b/tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-x86-full-fast-hotplug.json
>> new file mode 100644
>> index 000..aff5aa3
>> --- /dev/null
>> +++ 
>> b/tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-x86-full-fast-hotplug.json
>> @@ -0,0 +1,115 @@
>> +{
>> +  "return": [
>> +{
>> +  "props": {
>> +"core-id": 0,
>> +"thread-id": 0,
>> +"socket-id": 10
>> +  },
>> +  "vcpus-count": 1,
>> +  "qom-path": "/machine/peripheral/vcpu10",
>> +  "type": "Broadwell-x86_64-cpu"
>> +},
> 
> See you do show the props here...
> 
>> +{
>> +  "props": {
>> +"core-id": 0,
>> +"thread-id": 0,
>> +"socket-id": 9
>> +  },
>> +  "vcpus-count": 1,
>> +  "qom-path": "/machine/peripheral/vcpu9",
>> +  "type": "Broadwell-x86_64-cpu"
>> +},
> 
> [...]
> 
> 
>> diff --git a/tests/qemumonitortestutils.c b/tests/qemumonitortestutils.c
>> index 5e30fb0..b20d59a 100644
>> --- a/tests/qemumonitortestutils.c
>> +++ b/tests/qemumonitortestutils.c
>> @@ -1439,3 +1439,9 @@ qemuMonitorTestGetAgent(qemuMonitorTestPtr test)
>>  {
>>  return test->agent;
>>  }
> 
> Format here is 2 blank lines between methods
OK
> 
> John
> 
>> +
>> +virDomainObjPtr
>> +qemuMonitorTestGetDomainObj(qemuMonitorTestPtr test)
>> +{
>> +return test->vm;
>> +}
>> diff --git a/tests/qemumonitortestutils.h b/tests/qemumonitortestutils.h
>> index 8b19b37..3604a1b 100644
>> --- a/tests/qemumonitortestutils.h
>> +++ b/tests/qemumonitortestutils.h
>> @@ -96,5 +96,6 @@ void qemuMonitorTestFree(qemuMonitorTestPtr test);
>>  
>>  qemuMonitorPtr qemuMonitorTestGetMonitor(qemuMonitorTestPtr test);
>>  qemuAgentPtr qemuMonitorTestGetAgent(qemuMonitorTestPtr test);
>> +virDomainObjPtr qemuMonitorTestGetDomainObj(qemuMonitorTestPtr test);
>>  
>>  #endif /* __VIR_QEMU_MONITOR_TEST_UTILS_H__ */
>>
> 


-- 
Regards,
 Viktor Mihajlovski

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

Re: [libvirt] [PATCH sandbox] rpm: fix references to python 2 packages / files

[Cedric Bosdonnat]

On Fri, 2018-03-23 at 15:57 +, Daniel P. Berrangé wrote:
> Since we switched to python 3, we should have deps on the python 3 based
> packages, and look at the python 3 sitelib directory.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  libvirt-sandbox.spec.in | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/libvirt-sandbox.spec.in b/libvirt-sandbox.spec.in
> index 17e9dd9..f5868c1 100644
> --- a/libvirt-sandbox.spec.in
> +++ b/libvirt-sandbox.spec.in
> @@ -36,12 +36,12 @@ BuildRequires: zlib-devel >= 1.2.0, zlib-static
>  BuildRequires: libtirpc-devel
>  BuildRequires: rpcgen
>  %endif
> -Requires: rpm-python
> +Requires: python3-rpm
>  # For virsh lxc-enter-namespace command
>  Requires: libvirt-client >= %{libvirt_version}
>  Requires: systemd >= 198
>  Requires: pygobject3-base
> -Requires: libselinux-python
> +Requires: libselinux-python3
>  Requires: %{name}-libs = %{version}-%{release}
>  Requires: %{_bindir}/virt-builder
>  
> @@ -108,7 +108,7 @@ rm -rf $RPM_BUILD_ROOT
>  %{_bindir}/virt-sandbox-service
>  %{_bindir}/virt-sandbox-image
>  %{_libexecdir}/virt-sandbox-service-util
> -%{python_sitelib}/libvirt_sandbox
> +%{python3_sitelib}/libvirt_sandbox
>  %{_mandir}/man1/virt-sandbox.1*
>  %{_mandir}/man1/virt-sandbox-service.1*
>  %{_mandir}/man1/virt-sandbox-service-*.1*

ACK
--
Cedric

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

Re: [libvirt] [PATCH sandbox] Explicitly check for python3 in configure.ac

[Cedric Bosdonnat]

On Fri, 2018-03-23 at 15:57 +, Daniel P. Berrangé wrote:
> A bare AM_PATH_PYTHON statement is satisfied by any version of python >=
> 2.0.0, but we converted to Python 3, so should be explicit about it.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  configure.ac | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/configure.ac b/configure.ac
> index cb7a483..28305d2 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -130,7 +130,7 @@ dnl Should be in m4/virt-gettext.m4 but intltoolize is too
>  dnl dumb to find it there
>  IT_PROG_INTLTOOL([0.35.0])
>  
> -AM_PATH_PYTHON
> +AM_PATH_PYTHON([3])
>  
>  AC_OUTPUT(Makefile
>libvirt-sandbox/Makefile

ACK
--
Cedric

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

[libvirt] [PATCH] docs: formatdomain: Clarify CPU feature policy option "require"

[Kashyap Chamarthy]

(Thanks: Jiri Denemark, for clarifying this on IRC.)

Signed-off-by: Kashyap Chamarthy 
---
 docs/formatdomain.html.in | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 6fd2189cd..2410c92af 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -1532,7 +1532,7 @@
 of it being supported by host CPU.
   require
   Guest creation will fail unless the feature is supported by host
-CPU.
+CPU or the hypervisor is able to emulate it".
   optional
   The feature will be supported by virtual CPU if and only if it
 is supported by host CPU.
-- 
2.13.6

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

Re: [libvirt] [PATCH 4/5] qemu: Handle multipath devices properly

[Peter Krempa]

On Mon, Mar 26, 2018 at 07:16:45 +0200, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1557769
> 
> Problem with multipath devices is that there can be several other
> devices 'hidden' behind them. For instance, /dev/dm-1 can
> consist of /dev/sda, /dev/sdb and /dev/sdc. Therefore, when
> setting up devices CGroup and namespaces we have to take this
> into account.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  libvirt.spec.in|  2 ++
>  src/qemu/qemu_cgroup.c | 43 ++---
>  src/qemu/qemu_domain.c | 58 
> ++
>  3 files changed, 100 insertions(+), 3 deletions(-)
> 
> diff --git a/libvirt.spec.in b/libvirt.spec.in
> index b55a947ec9..ebfac10866 100644
> --- a/libvirt.spec.in
> +++ b/libvirt.spec.in
> @@ -796,6 +796,8 @@ Requires: gzip
>  Requires: bzip2
>  Requires: lzop
>  Requires: xz
> +# For mpath devices
> +Requires: device-mapper
>  %if 0%{?fedora} || 0%{?rhel} > 7
>  Requires: systemd-container
>  %endif
> diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
> index b604edb31c..a2198c9789 100644
> --- a/src/qemu/qemu_cgroup.c
> +++ b/src/qemu/qemu_cgroup.c
> @@ -60,7 +60,12 @@ qemuSetupImagePathCgroup(virDomainObjPtr vm,
>  {
>  qemuDomainObjPrivatePtr priv = vm->privateData;
>  int perms = VIR_CGROUP_DEVICE_READ;
> -int ret;
> +unsigned long long *mpathdevs = NULL;
> +size_t nmpathdevs = 0;
> +size_t i;
> +char *devPath = NULL;
> +int rv;
> +int ret = -1;
>  
>  if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES))
>  return 0;
> @@ -71,12 +76,44 @@ qemuSetupImagePathCgroup(virDomainObjPtr vm,
>  VIR_DEBUG("Allow path %s, perms: %s",
>path, virCgroupGetDevicePermsString(perms));
>  
> -ret = virCgroupAllowDevicePath(priv->cgroup, path, perms, true);
> +rv = virCgroupAllowDevicePath(priv->cgroup, path, perms, true);
>  
>  virDomainAuditCgroupPath(vm, priv->cgroup, "allow", path,
>   virCgroupGetDevicePermsString(perms),
> - ret);
> + rv);
> +if (rv < 0)
> +goto cleanup;
>  
> +if (virFileGetMPathTargets(path, &mpathdevs, &nmpathdevs) < 0 &&
> +errno != ENOSYS && errno != EBADF) {
> +virReportSystemError(errno,
> + _("Unable to get mpath targets for %s"),
> + path);
> +goto cleanup;
> +}
> +
> +for (i = 0; i < nmpathdevs; i++) {
> +if (virFileMajMinToName(mpathdevs[i], &devPath) < 0) {

So for cgroups this really isn't necessary. You can use
virCgroupAllowDevice which already takes the major/minor numbers rather
than a path. Given that virCgroupAllowDevicePath would convert the path
to major/minor numbers, this conversion is really wasteful.

> +virReportSystemError(errno,
> + _("Unable to translate %llx to device 
> path"),
> + mpathdevs[i]);

This error message would be very useless, given that it's not coverted
into major/minor.

> +goto cleanup;
> +}
> +
> +rv = virCgroupAllowDevicePath(priv->cgroup, devPath, perms, true);
> +
> +virDomainAuditCgroupPath(vm, priv->cgroup, "allow", devPath,
> + virCgroupGetDevicePermsString(perms),
> + rv);

Hmm, right. The message in the audit log when allowing the device would
not contain the path to the device, which would make it slightly harder
to use.


> +if (rv < 0)
> +goto cleanup;
> +VIR_FREE(devPath);
> +}
> +
> +ret = 0;
> + cleanup:
> +VIR_FREE(devPath);
> +VIR_FREE(mpathdevs);
>  return ret;

The only thing is that I'm not really a fan of calling the detection
code twice for every device you are going to add to the VM, once for
cgroups and once for namespaces. I think that the disk image setup code
should be extracted, so that the data needs to be obtained only once and
afterwards can be used to setup all the required images.

I'm not going to insist, but that also means that nobody will ever do
it. Especially since we for some reason hide all cgroup code setup into
qemu_cgroup. This also means that it has to be duplicated for
namespaces.

The code will require some changes if you implement my suggestions, but
it looks reasonable.


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

Re: [libvirt] Compiling Libvirt 3.0.0 failed: cannot stat t...@kkcor.gmo

[Daniel P . Berrangé]

On Mon, Mar 26, 2018 at 01:10:30PM +0300, Mathieu Tarral wrote:
> Hi Daniel,
> 
> > This is over a year old now - if building & reporting errors against
> > git, please only use current git master.
> 
> The reason why i use this release is because i'm trying to reproduce a bug
> that i have with the Debian Stretch package, which is release 3.0.0:
> https://packages.debian.org/stretch/libvirt-daemon
> 
> I have been told on this mailing-list that i should build my own libvirt from
> git to have better debugging information in the binaries rather than
> using the -dbgsym packages.

Sigh, I think that was bad advice. Debian will have various extra packages
on top of 3.0.0, so by building from Git you'll be testing different code
from what your Debian build had.

Just install the -dbgsym package and then collect the stack trace from
libvirtd daemon.

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

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

Re: [libvirt] Call to virDomainIsActive hangs forever

[Daniel P . Berrangé]

On Fri, Mar 23, 2018 at 01:24:46PM +0100, Erik Skultety wrote:
> On Thu, Mar 22, 2018 at 06:10:49PM +0200, Mathieu Tarral wrote:
> > Hi !
> >
> > I'm submitting my messages on this mailing list to request a bit of
> > help on a case that I have
> > where a Python application makes a call to virDomainIsActive, and the
> > call never returns.
> >
> > I have tried to investigate, but as there are no debug symbols for
> > libvirt on Debian Stretch,
> > i can only have the following GDB backtrace:
> >
> > (gdb) bt
> > #0  pthread_cond_wait@@GLIBC_2.3.2 () at
> > ../sysdeps/unix/sysv/linux/x86_64/pthread_cond_wait.S:185
> > #1  0x7f49026f5b76 in virCondWait () from /usr/lib/libvirt.so.0
> > #2  0x7f4902808bab in ?? () from /usr/lib/libvirt.so.0
> > #3  0x7f490280a433 in virNetClientSendWithReply () from
> > /usr/lib/libvirt.so.0
> > #4  0x7f490280abe2 in virNetClientProgramCall () from 
> > /usr/lib/libvirt.so.0
> > #5  0x7f49027e0ea4 in ?? () from /usr/lib/libvirt.so.0
> > #6  0x7f49027ea1bb in ?? () from /usr/lib/libvirt.so.0
> > #7  0x7f49027b0ef3 in virDomainIsActive () from /usr/lib/libvirt.so.0
> > #8  0x7f4902b7fbd0 in libvirt_virDomainIsActive () from
> > /usr/lib/python3/dist-packages/libvirtmod.cpython-35m-x86_64-linux-gnu.so
> > #9  0x558eeec696df in PyCFunction_Call () at 
> > ../Objects/methodobject.c:109
> >
> > The libvirt driver used is QEMU, and i have a specific monitoring in
> > place using virtual machine introspection:
> > https://github.com/KVM-VMI/kvm-vmi
> >
> > Now this specific monitoring somehow triggers this bug, and at this
> > point, i don't know if
> > it's a corner case in the libvirt QEMU driver or not.
> > That's why i would like to have your lights on this.
> >
> > libvirt version: 3.0.0-4
> >
> > -> Could you tell me where i should look in the code ?
> 
> You're probably looking at virLogManagerDomain* methods located in
> src/logging/log_manager.c and the wait call is issued from virNetClientIO.
> 
> > -> Do you have more information about this virCondWait ? Which
> > condition is it waiting for ?
> > -> How can i get the symbols without having the recompile libvirt and
> > install it system wide, erasing the binaries installed by the package
> > ?
> 
> To be honest, I think it's always worth debugging a custom built binary from
> sources, since the debug symbols shipped via distro package are most likely
> generated with optimizations which makes any kind of interactive debugging
> painful. The problem is that you're going to built v3.0.0 tag on a new distro,
> since new GCCs will complain about a lot of stuff, I looked at the code, tried
> a few things, but honestly I didn't see a path where you could get to
> virNetClientProgrammCall from virDomainIsActive (since I don't know what the
> original call was), so unless you post a full backtrace, we can't help you 
> much
> here.

That's easy - virDomainIsActive() calls into the driver APIs. This is client
side trace, so will get into the remote driver client, which will then call
virNetClientProgramCall.


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

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

Re: [libvirt] [PATCH 3/5] virfile: Introduce virFileMajMinToName

[Michal Privoznik]

On 03/26/2018 11:58 AM, Peter Krempa wrote:
> On Mon, Mar 26, 2018 at 07:16:44 +0200, Michal Privoznik wrote:
>> This function can be used to translate MAJ:MIN device pair into
>> /dev/blah name.
>>
>> Signed-off-by: Michal Privoznik 
>> ---
>>  src/libvirt_private.syms |  1 +
>>  src/util/virfile.c   | 53 
>> 
>>  src/util/virfile.h   |  3 +++
>>  3 files changed, 57 insertions(+)
>>
>> diff --git a/src/util/virfile.c b/src/util/virfile.c
>> index f89e4bd823..62620862a1 100644
>> --- a/src/util/virfile.c
>> +++ b/src/util/virfile.c
>> @@ -68,6 +68,12 @@
>>  # include 
>>  #endif
>>  
>> +#ifdef MAJOR_IN_MKDEV
>> +# include 
>> +#elif MAJOR_IN_SYSMACROS
>> +# include 
>> +#endif
>> +
>>  #include "configmake.h"
>>  #include "intprops.h"
>>  #include "viralloc.h"
>> @@ -4317,3 +4323,50 @@ virFileGetMPathTargets(const char *path 
>> ATTRIBUTE_UNUSED,
>>  return -1;
>>  }
>>  #endif /* ! WITH_DEVMAPPER */
>> +
>> +
>> +/**
>> + * virFileMajMinToName:
>> + * @device: device (rdev) to translate
>> + * @name: returned name
> 
> I'd prefer if you supply the device identifiers already separated. That
> means that the function in the previous patch should preferrably already
> split them so that we don't need to do it here.

Okay.

> 
>> + *
>> + * For given MAJ:MIN pair (stored in one integer like in st_rdev)
>> + * fetch device name, e.g. 8:0 is translated to "/dev/sda".
>> + * Caller is responsible for freeing @name when no longer needed.
> 
> There is no info on how to treat errors.
> 
>> + *
>> + * Returns 0 on success, -1 otherwise.
> 
> Why isn't the path returned directly?

No reason. It's just that I'm used to functions returning an integer. I
can change it.

> 
>> + */
>> +int
>> +virFileMajMinToName(unsigned long long device,
>> +char **name)
>> +{
>> +struct stat sb;
>> +char *sysfsPath = NULL;
>> +char *link = NULL;
>> +int ret = -1;
>> +
>> +*name = NULL;
>> +if (virAsprintf(&sysfsPath, "/sys/dev/block/%u:%u",
>> +major(device), minor(device)) < 0)
> 
> So the libdevmapper code is using a similar path to figure out the sysfs
> path, but only for devicemapper devices since it accesses
> /sys/dev/block/%u:%u/dm/name.
> 
> I've also found that '/dev/block/' has the information you might need.
> 
> Is there any particular reason to use sysfs? I'm not really persuaded
> that the directory name in sysfs is good enough so if you can provide
> where you've inspired yourself it will be beneficial.

Okay, I'll switch to /dev/block/... Looks like I took too much
inspiration from other projects O:-)

> 
> Additionally virAsprintf reports libvirt errors but this function does
> not. 
> 
>> +goto cleanup;
>> +
>> +if (lstat(sysfsPath, &sb) < 0)
>> +goto cleanup;
>> +
>> +if (!S_ISLNK(sb.st_mode)) {
>> +errno = ENXIO;
>> +goto cleanup;
>> +}
>> +
>> +if (virFileReadLink(sysfsPath, &link) < 0)
>> +goto cleanup;
>> +
>> +if (virAsprintf(name, "/dev/%s", last_component(link)) < 0)
>> +goto cleanup;
> 
> So this is the fishy path. I'd prefer /dev/block since the path already
> points to something in the /dev/ filesystem and isn't conjured up from
> something that looks the same.
> 

Ah, good point. So if I do plain:

virAsprintf(&path, "/dev/block/%u:%u", major(), minor())

I immediately get the correct symlink with both namespace and cgroup
codes know how to deal with. Cool, so this patch might be dropped.

Michal

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

Re: [libvirt] Compiling Libvirt 3.0.0 failed: cannot stat t...@kkcor.gmo

[Mathieu Tarral]

Hi Daniel,

> This is over a year old now - if building & reporting errors against
> git, please only use current git master.

The reason why i use this release is because i'm trying to reproduce a bug
that i have with the Debian Stretch package, which is release 3.0.0:
https://packages.debian.org/stretch/libvirt-daemon

I have been told on this mailing-list that i should build my own libvirt from
git to have better debugging information in the binaries rather than
using the -dbgsym packages.

> You've cut off too much of the build log to see what has happened
> here I'm fraid.

I hope this one will be enough:

: --update hr.po libvirt.pot


 [0/1958]
: --update ia.po libvirt.pot
: --update id.po libvirt.pot
: --update ilo.po libvirt.pot
: --update is.po libvirt.pot
: --update it.po libvirt.pot
: --update ja.po libvirt.pot
: --update ka.po libvirt.pot
: --update kk.po libvirt.pot
: --update km.po libvirt.pot
: --update ko.po libvirt.pot
: --update kn.po libvirt.pot
: --update kw.po libvirt.pot
rm -f k...@kkcor.gmo && : -c --statistics -o k...@kkcor.gmo k...@kkcor.po
rm -f k...@uccor.gmo && : -c --statistics -o k...@uccor.gmo k...@uccor.po
rm -f kw_GB.gmo && : -c --statistics -o kw_GB.gmo kw_GB.po
rm -f ky.gmo && : -c --statistics -o ky.gmo ky.po
rm -f lt.gmo && : -c --statistics -o lt.gmo lt.po
rm -f lv.gmo && : -c --statistics -o lv.gmo lv.po
mv: cannot stat 't...@kkcor.gmo': No such file or directory
Makefile:448: recipe for target 'k...@kkcor.gmo' failed
make[3]: *** [k...@kkcor.gmo] Error 1
make[3]: *** Waiting for unfinished jobs
mv: cannot stat 't...@uccor.gmo': No such file or directory
rm -f mk.gmo && : -c --statistics -o mk.gmo mk.po
mv: cannot stat 't-kw_GB.gmo': No such file or directory
Makefile:448: recipe for target 'k...@uccor.gmo' failed
make[3]: *** [k...@uccor.gmo] Error 1
Makefile:448: recipe for target 'kw_GB.gmo' failed
make[3]: *** [kw_GB.gmo] Error 1
rm -f mai.gmo && : -c --statistics -o mai.gmo mai.po
mv: cannot stat 't-lv.gmo': No such file or directory
mv: cannot stat 't-ky.gmo': No such file or directory
mv: cannot stat 't-lt.gmo'Makefile:448: recipe for target 'lv.gmo' failed
: No such file or directory
make[3]: *** [lv.gmo] Error 1
Makefile:448: recipe for target 'ky.gmo' failed
make[3]: *** [ky.gmo] Error 1
Makefile:448: recipe for target 'lt.gmo' failed
make[3]: *** [lt.gmo] Error 1
mv: cannot stat 't-mk.gmo': No such file or directory
Makefile:448: recipe for target 'mk.gmo' failed
make[3]: *** [mk.gmo] Error 1
mv: cannot stat 't-mai.gmo': No such file or directory
Makefile:448: recipe for target 'mai.gmo' failed
make[3]: *** [mai.gmo] Error 1
make[3]: Leaving directory '/var/ansible/libvirt/po'
Makefile:485: recipe for target 'stamp-po' failed
make[2]: *** [stamp-po] Error 2
make[2]: Leaving directory '/var/ansible/libvirt/po'
Makefile:2080: recipe for target 'all-recursive' failed
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory '/var/ansible/libvirt'
Makefile:1977: recipe for target 'all' failed
make: *** [all] Error 2

Thank you.

Best regards,

-- 
Mathieu Tarral

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

Re: [libvirt] Compiling Libvirt 3.0.0 failed: cannot stat t...@kkcor.gmo

[Daniel P . Berrangé]

On Mon, Mar 26, 2018 at 12:56:13PM +0300, Mathieu Tarral wrote:
> Hi,
> 
> I tried to compile libvirt 3.0.0, from git, on a Debian Stretch.

This is over a year old now - if building & reporting errors against
git, please only use current git master.

> It failed with these weird errors that i'm not able to fix by myself:
> 
> rm -f ky.gmo && : -c --statistics -o ky.gmo ky.po
> rm -f lt.gmo && : -c --statistics -o lt.gmo lt.po
> mv: cannot stat 't...@kkcor.gmo': No such file or directory
> mv: cannot stat 't...@uccor.gmo': No such file or directory
> Makefile:448: recipe for target 'k...@kkcor.gmo' failed
> make[3]: *** [k...@kkcor.gmo] Error 1
> make[3]: *** Waiting for unfinished jobs
> rm -f lv.gmo && : -c --statistics -o lv.gmo lv.po
> Makefile:448: recipe for target 'k...@uccor.gmo' failed
> make[3]: *** [k...@uccor.gmo] Error 1
> mv: cannot stat 't-kw_GB.gmo': No such file or directory
> 
> I have no idea what this "t...@kkcor.gmo" is about ??

"k...@kkcor.gmo" is one of the translation languages.

> Could you guys help ?

You've cut off too much of the build log to see what has happened
here I'm fraid.

> 
> Note: the build failed erlier because uuid-dev was missing.
> You might want to add a check for that lib in the configure.ac

Libvirt doesn't use the uuid-dev library itself. So if that caused a failure
it is probably due to a 3rd party library we depend on, not having correct
package dependancies setup.

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

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

Re: [libvirt] [PATCH 3/5] virfile: Introduce virFileMajMinToName

[Peter Krempa]

On Mon, Mar 26, 2018 at 07:16:44 +0200, Michal Privoznik wrote:
> This function can be used to translate MAJ:MIN device pair into
> /dev/blah name.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/libvirt_private.syms |  1 +
>  src/util/virfile.c   | 53 
> 
>  src/util/virfile.h   |  3 +++
>  3 files changed, 57 insertions(+)
> 
> diff --git a/src/util/virfile.c b/src/util/virfile.c
> index f89e4bd823..62620862a1 100644
> --- a/src/util/virfile.c
> +++ b/src/util/virfile.c
> @@ -68,6 +68,12 @@
>  # include 
>  #endif
>  
> +#ifdef MAJOR_IN_MKDEV
> +# include 
> +#elif MAJOR_IN_SYSMACROS
> +# include 
> +#endif
> +
>  #include "configmake.h"
>  #include "intprops.h"
>  #include "viralloc.h"
> @@ -4317,3 +4323,50 @@ virFileGetMPathTargets(const char *path 
> ATTRIBUTE_UNUSED,
>  return -1;
>  }
>  #endif /* ! WITH_DEVMAPPER */
> +
> +
> +/**
> + * virFileMajMinToName:
> + * @device: device (rdev) to translate
> + * @name: returned name

I'd prefer if you supply the device identifiers already separated. That
means that the function in the previous patch should preferrably already
split them so that we don't need to do it here.

> + *
> + * For given MAJ:MIN pair (stored in one integer like in st_rdev)
> + * fetch device name, e.g. 8:0 is translated to "/dev/sda".
> + * Caller is responsible for freeing @name when no longer needed.

There is no info on how to treat errors.

> + *
> + * Returns 0 on success, -1 otherwise.

Why isn't the path returned directly?

> + */
> +int
> +virFileMajMinToName(unsigned long long device,
> +char **name)
> +{
> +struct stat sb;
> +char *sysfsPath = NULL;
> +char *link = NULL;
> +int ret = -1;
> +
> +*name = NULL;
> +if (virAsprintf(&sysfsPath, "/sys/dev/block/%u:%u",
> +major(device), minor(device)) < 0)

So the libdevmapper code is using a similar path to figure out the sysfs
path, but only for devicemapper devices since it accesses
/sys/dev/block/%u:%u/dm/name.

I've also found that '/dev/block/' has the information you might need.

Is there any particular reason to use sysfs? I'm not really persuaded
that the directory name in sysfs is good enough so if you can provide
where you've inspired yourself it will be beneficial.

Additionally virAsprintf reports libvirt errors but this function does
not. 

> +goto cleanup;
> +
> +if (lstat(sysfsPath, &sb) < 0)
> +goto cleanup;
> +
> +if (!S_ISLNK(sb.st_mode)) {
> +errno = ENXIO;
> +goto cleanup;
> +}
> +
> +if (virFileReadLink(sysfsPath, &link) < 0)
> +goto cleanup;
> +
> +if (virAsprintf(name, "/dev/%s", last_component(link)) < 0)
> +goto cleanup;

So this is the fishy path. I'd prefer /dev/block since the path already
points to something in the /dev/ filesystem and isn't conjured up from
something that looks the same.

> +
> +ret = 0;
> + cleanup:
> +VIR_FREE(link);
> +VIR_FREE(sysfsPath);
> +return ret;
> +}
> diff --git a/src/util/virfile.h b/src/util/virfile.h
> index 0db8bf0b99..390940dc98 100644
> --- a/src/util/virfile.h
> +++ b/src/util/virfile.h
> @@ -362,4 +362,7 @@ int virFileGetMPathTargets(const char *path,
> unsigned long long **devs,
> size_t *ndevs);
>  
> +int virFileMajMinToName(unsigned long long device,
> +char **name);
> +
>  #endif /* __VIR_FILE_H */
> -- 
> 2.16.1
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list


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

[libvirt] Compiling Libvirt 3.0.0 failed: cannot stat t...@kkcor.gmo

[Mathieu Tarral]

Hi,

I tried to compile libvirt 3.0.0, from git, on a Debian Stretch.

It failed with these weird errors that i'm not able to fix by myself:

rm -f ky.gmo && : -c --statistics -o ky.gmo ky.po
rm -f lt.gmo && : -c --statistics -o lt.gmo lt.po
mv: cannot stat 't...@kkcor.gmo': No such file or directory
mv: cannot stat 't...@uccor.gmo': No such file or directory
Makefile:448: recipe for target 'k...@kkcor.gmo' failed
make[3]: *** [k...@kkcor.gmo] Error 1
make[3]: *** Waiting for unfinished jobs
rm -f lv.gmo && : -c --statistics -o lv.gmo lv.po
Makefile:448: recipe for target 'k...@uccor.gmo' failed
make[3]: *** [k...@uccor.gmo] Error 1
mv: cannot stat 't-kw_GB.gmo': No such file or directory

I have no idea what this "t...@kkcor.gmo" is about ??
Could you guys help ?

Note: the build failed erlier because uuid-dev was missing.
You might want to add a check for that lib in the configure.ac

Best regards.


-- 
Mathieu Tarral

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

Re: [libvirt] [PATCH sandbox] Python 3 renamed string.lowercase to string.ascii_lowercase

[Daniel P . Berrangé]

On Sun, Mar 25, 2018 at 11:32:04AM +0200, Ján Tomko wrote:
> On Fri, Mar 23, 2018 at 04:44:51PM +, Daniel P. Berrangé wrote:
> > Signed-off-by: Daniel P. Berrangé 
> 
> ACK, but the commit message is misleading.
> 
> Python2 has ascii_lowercase as early as 2.2 and in Python3, lowercase
> disappeared.
> 
> lowercase is the locale-dependent version, which seems to be identical
> to ascii_lowercase for UTF-8 locales, but if you use a non-UTF one, fun
> things happen:
> > > > locale.setlocale(locale.LC_ALL, 'sk_SK')
> 'sk_SK'
> > > > print string.lowercase
> abcdefghijklmnopqrstuvwxyząłľśšşťźžżßŕáâăäĺćçčéęëěíîďđńňóôőöřůúűüýţ

Oh fun, we definitely should have used ascii_lowercase from the start
then.


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

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

Re: [libvirt] [PATCH 2/5] utils: Introduce virFileGetMPathTargets

[Peter Krempa]

On Mon, Mar 26, 2018 at 07:16:43 +0200, Michal Privoznik wrote:
> This helper fetches targets for multipath devices.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/libvirt_private.syms |  1 +
>  src/util/virfile.c   | 90 
> 
>  src/util/virfile.h   |  4 +++
>  3 files changed, 95 insertions(+)

As I've pointed in the conversation regarding patch 1/5, this should be
put into a separate file, so that it's obvious what needs to be removed
if this functionality will need to be put into the storage driver.

Additionally it then will be justified to use a virOnce there to set the
logging function for libdevmapper.

> 
> diff --git a/src/util/virfile.c b/src/util/virfile.c
> index 5e9bd2007a..f89e4bd823 100644
> --- a/src/util/virfile.c
> +++ b/src/util/virfile.c

> @@ -4227,3 +4231,89 @@ virFileWaitForExists(const char *path,
>  
>  return 0;
>  }
> +
> +
> +#ifdef WITH_DEVMAPPER
> +/**
> + * virFileGetMPathTargets:
> + * @path: multipath device
> + * @devs: returned array of st_rdevs of @path targets
> + * @ndevs: size of @devs and @devNames

There's no 'devNames' parameter.

> + *
> + * For given @path figure out its targets, and store them in
> + * @devs. Items from @devs are meant to be used in
> + * minor() and major() to receive device MAJ:MIN pairs.
> + *
> + * If @path is not a multipath device, @ndevs is set to 0 and
> + * success is returned.
> + *
> + * If we don't have permissions to talk to kernel, -1 is returned
> + * and errno is set to EBADF.

You need to document that this function does not use libvirt errors.

> + *
> + * Returns 0 on success, -1 otherwise.
> + */
> +int
> +virFileGetMPathTargets(const char *path,
> +   unsigned long long **devs,

struct 'dm_deps' uses uint64_t for this.

> +   size_t *ndevs)
> +{
> +struct dm_deps *deps;
> +struct dm_task *dmt;
> +struct dm_info info;
> +size_t i;
> +int ret = -1;
> +
> +*ndevs = 0;
> +
> +if (!(dmt = dm_task_create(DM_DEVICE_DEPS)))
> +goto cleanup;
> +
> +if (!dm_task_set_name(dmt, path)) {
> +if (errno == ENOENT) {
> +/* It's okay, @path is not managed by devmapper =>
> + * not a multipath device. */
> +ret = 0;
> +}
> +goto cleanup;
> +}
> +
> +dm_task_no_open_count(dmt);
> +
> +if (!dm_task_run(dmt))
> +goto cleanup;
> +
> +if (!dm_task_get_info(dmt, &info))
> +goto cleanup;
> +
> +if (!info.exists) {
> +ret = 0;
> +goto cleanup;
> +}
> +
> +if (!(deps = dm_task_get_deps(dmt)))
> +goto cleanup;
> +
> +if (VIR_ALLOC_N(*devs, deps->count) < 0)

VIR_ALLOC_N_QUIET, since we don't do libvirt errors.

> +goto cleanup;
> +*ndevs = deps->count;
> +
> +for (i = 0; i < deps->count; i++)
> +(*devs)[i] = deps->device[i];
> +
> +ret = 0;
> + cleanup:
> +dm_task_destroy(dmt);
> +return ret;
> +}


Okay this code is stol^W inspired by the 'dmsetup deps' functionality.
You might want to point that fact out, since it took some time to find
it.

The code looks good, but since it requires the virOnce stuff, v2 will be
needed.


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

Re: [libvirt] [PATCH 1/5] virlog: Don't log devmapper

[Michal Privoznik]

On 03/26/2018 10:44 AM, Peter Krempa wrote:
> On Mon, Mar 26, 2018 at 09:49:08 +0200, Peter Krempa wrote:
>> On Mon, Mar 26, 2018 at 07:16:42 +0200, Michal Privoznik wrote:
>>> Unless overridden, libdevmapper logs all messages to stderr.
>>> Therefore if something goes wrong in storage_backend_mpath.c or
>>> parthelper.c we don't honour user set logging targets.
> 
> 'parthelper.c' does not use our logging code and also does not use any
> libdevmapper.h functions. According to my experiment the header file
> include can be dropped.
> 
>>>
>>> Signed-off-by: Michal Privoznik 
>>> ---
>>>  src/util/virlog.c | 23 +++
>>>  1 file changed, 23 insertions(+)
>>
>> I think this should be done in the function which registers the mpath
>> storage backend. For now it should work as is, but given that we are
>> going to split the daemons this code will not really be necessary in
>> other places.
> 
> So looking at the new code you've added in this series, it still might
> be desirable to move all the libdevmapper use into the storage driver
> and keep it there, so users wanting to use multipath would need to
> install the corresponding storage driver.
> 
> Given that moving it there might be impractical, I'll suggest that you
> move it to a separate util file and add virOnce infrastructure to set
> the error helpers.
> 

Okay, fair enough. I'll wait for review on the rest of the patches
before I post v2.

Thanks,
Michal

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

Re: [libvirt] [PATCH 1/5] virlog: Don't log devmapper

[Peter Krempa]

On Mon, Mar 26, 2018 at 10:16:54 +0200, Michal Privoznik wrote:
> On 03/26/2018 09:49 AM, Peter Krempa wrote:
> > On Mon, Mar 26, 2018 at 07:16:42 +0200, Michal Privoznik wrote:
> >> Unless overridden, libdevmapper logs all messages to stderr.
> >> Therefore if something goes wrong in storage_backend_mpath.c or
> >> parthelper.c we don't honour user set logging targets.
> >>
> >> Signed-off-by: Michal Privoznik 
> >> ---
> >>  src/util/virlog.c | 23 +++
> >>  1 file changed, 23 insertions(+)
> > 
> > I think this should be done in the function which registers the mpath
> > storage backend. For now it should work as is, but given that we are
> > going to split the daemons this code will not really be necessary in
> > other places.
> > 
> > And it really does not have to do much with our logging code.
> > 
> 
> Well, in the next patch I'm introducing virFileGetMPathTargets() which
> uses dm_* a lot. Now, since the function lives in src/util/ it can be
> used by many drivers. Do we expect each one of them to provide dummy
> callback on their own? I don't think so. I agree this is not a nice
> patch, but I don't really see any better place for it.

I've written another response to my original reply which explains a bit
more.

Basically, the code in the next patch should be put in a new file along
with a virOnce function to silence the logs if you don't want to move it
into the storage driver directly.


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

Re: [libvirt] [PATCH 1/5] virlog: Don't log devmapper

[Peter Krempa]

On Mon, Mar 26, 2018 at 09:49:08 +0200, Peter Krempa wrote:
> On Mon, Mar 26, 2018 at 07:16:42 +0200, Michal Privoznik wrote:
> > Unless overridden, libdevmapper logs all messages to stderr.
> > Therefore if something goes wrong in storage_backend_mpath.c or
> > parthelper.c we don't honour user set logging targets.

'parthelper.c' does not use our logging code and also does not use any
libdevmapper.h functions. According to my experiment the header file
include can be dropped.

> > 
> > Signed-off-by: Michal Privoznik 
> > ---
> >  src/util/virlog.c | 23 +++
> >  1 file changed, 23 insertions(+)
> 
> I think this should be done in the function which registers the mpath
> storage backend. For now it should work as is, but given that we are
> going to split the daemons this code will not really be necessary in
> other places.

So looking at the new code you've added in this series, it still might
be desirable to move all the libdevmapper use into the storage driver
and keep it there, so users wanting to use multipath would need to
install the corresponding storage driver.

Given that moving it there might be impractical, I'll suggest that you
move it to a separate util file and add virOnce infrastructure to set
the error helpers.


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

Re: [libvirt] [GSoC] Design ideas for implementing cleanup attribute

[Daniel P . Berrangé]

On Sun, Mar 25, 2018 at 01:55:07AM +0530, Sukrit Bhatnagar wrote:
> Hi,
> 
> I am interested in implementing the GCC cleanup attribute for automatic
> resource freeing as part of GSoC'18. I have shared a proposal for the same.
> 
> This mail is to discuss the code design for implementing it.
> 
> 
> Here are some of my ideas:
> 
> This attribute requires a cleanup function that is called automatically
> when the corresponding variable goes out of scope. There are some functions
> whose logic can be reused:
> 
> - Functions such as virCommandFree, virConfFreeList and virCgroupFree can
> be directly used as cleanup functions. They have parameter and return type
> valid for a cleanup function.
> 
> - Functions such as virFileClose and virFileFclose need some additional
> consideration as they return a value. I think we can set some global
> variable in a separate source file (just like errno variable from errno.h).
> Then the value to be returned can be accessed globally.

Note we call VIR_FORCE_CLOSE and VIR_FORCE_FCLOSE in cleanup scenarios
to explicitly ignore the return value. We only care about checking
return value for VIR_CLOSE and VIR_FCLOSE in a tiny set of cases.

> 
> - Functions such as virDomainEventGraphicsDispose need an entirely new
> design. They are used as callbacks in object classes and passed as an
> argument in virClassNew. This would require making changes to
> virObjectUnref's code too. *This is the part I am not sure how to implement
> cleanup logic for.*

There shouldn't be any need to touch the DIspose funtions - that's internal
helper code. To "free" an object variable, you just want to end up invoking
the regular virObjectUnref function as the cleanup func.

> Also, since the __attribute__((__cleanup__(anyfunc))) looks ugly, a macro
> like autoclean (ideas for macro name welcome!) can be used instead. As
> Martin pointed out in my proposal, for some types, this can be done right
> after typedef declarations, so that the type itself contains this attribute.

Just suffix it with "Auto", eg   virDomainAutoPtr, vs virDomainPtr.

> I can create new files vircleanup.{c,h} for defining cleanup functions for
> types which do not have an existing cleanup/free function. This can be done
> separately for each driver supported.
> For example, cleanups pertaining to lxc driver will be in
> src/lxc/lxc_cleanup.c.

I'm not rally seeing a compelling need for this - just add it in appropriate
existing files.

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

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

Re: [libvirt] [PATCH 1/5] virlog: Don't log devmapper

[Michal Privoznik]

On 03/26/2018 09:49 AM, Peter Krempa wrote:
> On Mon, Mar 26, 2018 at 07:16:42 +0200, Michal Privoznik wrote:
>> Unless overridden, libdevmapper logs all messages to stderr.
>> Therefore if something goes wrong in storage_backend_mpath.c or
>> parthelper.c we don't honour user set logging targets.
>>
>> Signed-off-by: Michal Privoznik 
>> ---
>>  src/util/virlog.c | 23 +++
>>  1 file changed, 23 insertions(+)
> 
> I think this should be done in the function which registers the mpath
> storage backend. For now it should work as is, but given that we are
> going to split the daemons this code will not really be necessary in
> other places.
> 
> And it really does not have to do much with our logging code.
> 

Well, in the next patch I'm introducing virFileGetMPathTargets() which
uses dm_* a lot. Now, since the function lives in src/util/ it can be
used by many drivers. Do we expect each one of them to provide dummy
callback on their own? I don't think so. I agree this is not a nice
patch, but I don't really see any better place for it.

Michal

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

Re: [libvirt] [PATCHv2 1/6] qemu: add capability detection for query-cpus-fast

[Viktor Mihajlovski]

On 23.03.2018 17:03, John Ferlan wrote:
> 
> 
> On 03/05/2018 06:44 AM, Viktor Mihajlovski wrote:
>> Detect whether QEMU supports the QMP query-cpus-fast API
>> and set QEMU_CAPS_QUERY_CPUS_FAST in this case.
>>
>> Signed-off-by: Viktor Mihajlovski 
>> Reviewed-by: Boris Fiuczynski 
>> Reviewed-by: Marc Hartmayer 
>> Acked-by: Peter Krempa 
>> ---
>>  src/qemu/qemu_capabilities.c | 4 +++-
>>  src/qemu/qemu_capabilities.h | 1 +
>>  2 files changed, 4 insertions(+), 1 deletion(-)
>>
Thanks for the thorough review of the series.
> 
> Now that the 2.12 caps update has been pushed... It's "open season" on
> the qemu_capabilities.{c,h}. Since this series has been on list the
> longest, I'll start here...
> 
> This particular patch will need an update to add the new flag to the
> various caps_2.12*.xml files (VIR_TEST_REGENERATE_OUTPUT=1
> tests/qemucapabilitiestest does the trick).
> 
OK.
>> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
>> index b5eb8cf..6635f5e 100644
>> --- a/src/qemu/qemu_capabilities.c
>> +++ b/src/qemu/qemu_capabilities.c
>> @@ -459,6 +459,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
>>"pl011",
>>"machine.pseries.max-cpu-compat",
>>"dump-completed",
>> +  "query-cpus-fast",
>>  );
>>  
>>  
>> @@ -1579,7 +1580,8 @@ struct virQEMUCapsStringFlags virQEMUCapsCommands[] = {
>>  { "query-qmp-schema", QEMU_CAPS_QUERY_QMP_SCHEMA },
>>  { "query-cpu-model-expansion", QEMU_CAPS_QUERY_CPU_MODEL_EXPANSION},
>>  { "query-cpu-definitions", QEMU_CAPS_QUERY_CPU_DEFINITIONS},
>> -{ "query-named-block-nodes", QEMU_CAPS_QUERY_NAMED_BLOCK_NODES}
>> +{ "query-named-block-nodes", QEMU_CAPS_QUERY_NAMED_BLOCK_NODES},
>> +{ "query-cpus-fast", QEMU_CAPS_QUERY_CPUS_FAST}
> 
> I know this has been ACK'd; however, I recently pushed a small fixup in
> this area that you'll have a merge with, see commit id '1706bef6' (I saw
> that while working on something else, but noted this patch is affected).
> 
> Still for this new entry, it'd be better to use the format :
> 
>   { "query-cpus-fast", QEMU_CAPS_QUERY_CPUS_FAST },
> 
I agree, will change.
> 
> John
> 
>>  };
>>  
>>  struct virQEMUCapsStringFlags virQEMUCapsMigration[] = {
>> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
>> index c2ec2be..e3c31ab 100644
>> --- a/src/qemu/qemu_capabilities.h
>> +++ b/src/qemu/qemu_capabilities.h
>> @@ -444,6 +444,7 @@ typedef enum {
>>  QEMU_CAPS_DEVICE_PL011, /* -device pl011 (not user-instantiable) */
>>  QEMU_CAPS_MACHINE_PSERIES_MAX_CPU_COMPAT, /* -machine 
>> pseries,max-cpu-compat= */
>>  QEMU_CAPS_DUMP_COMPLETED, /* DUMP_COMPLETED event */
>> +QEMU_CAPS_QUERY_CPUS_FAST, /* query-cpus-fast command */
>>  
>>  QEMU_CAPS_LAST /* this must always be the last item */
>>  } virQEMUCapsFlags;
>>
> 


-- 
Regards,
 Viktor Mihajlovski

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

Re: [libvirt] [PATCHv2 2/6] qemu: use query-cpus-fast in JSON monitor

[Viktor Mihajlovski]

On 23.03.2018 17:05, John Ferlan wrote:
[...]
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index 8b4efc8..4079fb3 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -8683,7 +8683,12 @@ qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver,
>>  if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
>>  return -1;
>>  
>> -rc = qemuMonitorGetCPUInfo(qemuDomainGetMonitor(vm), &info, maxvcpus, 
>> hotplug);
>> +rc = qemuMonitorGetCPUInfo(qemuDomainGetMonitor(vm),
>> +   &info,
>> +   maxvcpus,
>> +   hotplug,
>> +   
>> virQEMUCapsGet(QEMU_DOMAIN_PRIVATE(vm)->qemuCaps,
>> +  QEMU_CAPS_QUERY_CPUS_FAST));
> 
> Perhaps we should create a qemuMonitorGetCPUInfoFast API since we should
> also be able to return the @props {core-id, thread-id, socket-id} and
> report them. I'm not steadfast on this suggestion - maybe Peter has a
> stronger opinion one way or the other. Still it allows future
> adjustments and additions to -fast to be more easily "handled".
> 
> It would seem those values could be useful although I do see there could
> be some "impact" with how hotplug works and the default setting to -1 of
> the values by qemuMonitorCPUInfoClear.
Actually, query-cpus[-fast] is not reporting the topology (socket, core,
thread). The reported thread id is referring to the QEMU CPU thread.
> 
> FWIW: Below this point as the returned @info structure is parsed,
> there's a comment about query-cpus that would then of course not
> necessarily be true or at the very least "adjusted properly" for this
> new -fast model.
> 
Agreed, this one escaped me. Will change.
>>  
>>  if (qemuDomainObjExitMonitor(driver, vm) < 0)
>>  goto cleanup;
>> @@ -8803,7 +8808,10 @@ qemuDomainRefreshVcpuHalted(virQEMUDriverPtr driver,
>>  if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
>>  return -1;
>>  
>> -haltedmap = qemuMonitorGetCpuHalted(qemuDomainGetMonitor(vm), maxvcpus);
>> +haltedmap = qemuMonitorGetCpuHalted(qemuDomainGetMonitor(vm),
>> +maxvcpus,
>> +
>> virQEMUCapsGet(QEMU_DOMAIN_PRIVATE(vm)->qemuCaps,
>> +   
>> QEMU_CAPS_QUERY_CPUS_FAST));
> 
> Maybe this one should pass 'false' for now until patch 4 comes along to
> add the code that fetches the cpu-state and alters halted for the -fast
> model. Depending on how one feels about larger patches vs having a 2
> patch gap to not have the support for -fast... I dunno, I could see
> reason to merge patch 4 here too in order to be in a patch feature for
> feature compatible.>
FWIW, I intended to make the patches deprecation-safe, i.e. prevent
calling query-cpus on a QEMU not supporting it anymore. I see your point
though and would have no principal objections to merging patches 2 and 4
(plus the testcase patches 3 and 5). Would be great to hear more opinions...
>>  
>>  if (qemuDomainObjExitMonitor(driver, vm) < 0 || !haltedmap)
>>  goto cleanup;
> 
> [...]
> 
>> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
>> index a09e93e..6a5fb12 100644
>> --- a/src/qemu/qemu_monitor_json.c
>> +++ b/src/qemu/qemu_monitor_json.c
>> @@ -1466,7 +1466,8 @@ int qemuMonitorJSONSystemReset(qemuMonitorPtr mon)
>>  static int
>>  qemuMonitorJSONExtractCPUInfo(virJSONValuePtr data,
>>struct qemuMonitorQueryCpusEntry **entries,
>> -  size_t *nentries)
>> +  size_t *nentries,
>> +  bool fast)
>>  {
>>  struct qemuMonitorQueryCpusEntry *cpus = NULL;
>>  int ret = -1;
>> @@ -1491,11 +1492,19 @@ qemuMonitorJSONExtractCPUInfo(virJSONValuePtr data,
>>  }
>>  
>>  /* Some older qemu versions don't report the thread_id so treat 
>> this as
>> - * non-fatal, simply returning no data */
>> -ignore_value(virJSONValueObjectGetNumberInt(entry, "CPU", &cpuid));
>> -ignore_value(virJSONValueObjectGetNumberInt(entry, "thread_id", 
>> &thread));
>> -ignore_value(virJSONValueObjectGetBoolean(entry, "halted", 
>> &halted));
>> -qom_path = virJSONValueObjectGetString(entry, "qom_path");
>> + * non-fatal, simply returning no data.
>> + * The return data of query-cpus-fast has different field names
>> + */
>> +if (fast) {
>> +ignore_value(virJSONValueObjectGetNumberInt(entry, "cpu-index", 
>> &cpuid));
>> +ignore_value(virJSONValueObjectGetNumberInt(entry, "thread-id", 
>> &thread));
>> +qom_path = virJSONValueObjectGetString(entry, "qom-path");
>> +} else {
>> +ignore_value(virJSONValueObjectGetNumberInt(entry, "CPU", 
>> &cpuid));
>> 

Re: [libvirt] [dbus PATCH v2 0/6] Use API calls supporting flags

[Pavel Hrdina]

On Fri, Mar 23, 2018 at 05:57:58PM +0100, Katerina Koukiou wrote:
> * Use API calls with flags parameter when possible
> * Since libvirt-dbus API doesn't follow strictly the naming
>   of libvirt API there should be documentation regarding which
>   libvirt API is used for each libvirt-dbus API.
>   This patch introduces documentation annotations in D-Bus Interface XML
>   files which will be used later to generate proper documentation.
> 
> Katerina Koukiou (6):
>   virtDBusDomainGetVcpus: Should be implemented as method and not as
> property
>   virtDBusDomainShutdown: Use virDomainShutdownFlags instead of
> virDomainShutdown
>   virtDBusDomainCreate: Use virDomainCreateWithFlags instead of
> virDomainCreate
>   virtDBusDomainUndefine: Use virDomainUndefineFlags instead of
> virDomainUndefine
>   virtDBusDomainDestroy: Use virDomainDestroyFlags instead of
> virDomainDestroy
>   Use Documentation Annotations in D-Bus Interface XML

Thanks for the patches, I'll fix the issues before pushing.

Reviewed-by: Pavel Hrdina 


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

Re: [libvirt] [dbus PATCH v2 5/6] virtDBusDomainDestroy: Use virDomainDestroyFlags instead of virDomainDestroy

[Pavel Hrdina]

On Fri, Mar 23, 2018 at 05:58:03PM +0100, Katerina Koukiou wrote:
> We need to catch Exceptions when testing this API call, since 
> virDomainDestroyFlags
> will be supported in test-driver in 4.2.0 libvirt release.
> 
> Better version checks for unsupported API calls need to be done in all tests,
> but it's not relevant to this commit.
> When virConnectGetVersion will be supported we can move to better 
> implementation,
> and python logging should be used to provide proper warnings for skipped 
> tests.

Long lines in commit message, the unwritten rule (suggestion) is to
keep it under 72 characters per line.

Pavel


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

Re: [libvirt] [dbus PATCH v2 1/6] virtDBusDomainGetVcpus: Should be implemented as method and not as property

[Pavel Hrdina]

On Fri, Mar 23, 2018 at 05:57:59PM +0100, Katerina Koukiou wrote:
> Signed-off-by: Katerina Koukiou 
> ---
>  data/org.libvirt.Domain.xml |  5 -
>  src/domain.c| 51 
> ++---
>  test/test_domain.py |  3 ++-
>  3 files changed, 35 insertions(+), 24 deletions(-)

The $subject can be shorten to
"virtDBusDomainGetVcpus: Implement as method not as property"

Pavel


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

Re: [libvirt] [PATCH 1/5] virlog: Don't log devmapper

[Peter Krempa]

On Mon, Mar 26, 2018 at 07:16:42 +0200, Michal Privoznik wrote:
> Unless overridden, libdevmapper logs all messages to stderr.
> Therefore if something goes wrong in storage_backend_mpath.c or
> parthelper.c we don't honour user set logging targets.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/util/virlog.c | 23 +++
>  1 file changed, 23 insertions(+)

I think this should be done in the function which registers the mpath
storage backend. For now it should work as is, but given that we are
going to split the daemons this code will not really be necessary in
other places.

And it really does not have to do much with our logging code.


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

Re: [libvirt] [dbus PATCH v2 6/6] Use Documentation Annotations in D-Bus Interface XML

[Pavel Hrdina]

On Fri, Mar 23, 2018 at 05:58:04PM +0100, Katerina Koukiou wrote:
> Since we don't follow the exact naming of libvirt API for libvirt-dbus,
> documentation should clarify which API call is used internally each time.
> 
> Signed-off-by: Katerina Koukiou 
> ---
>  data/org.libvirt.Connect.xml | 24 
>  data/org.libvirt.Domain.xml  | 66 
> ++--
>  2 files changed, 82 insertions(+), 8 deletions(-)
> 
> diff --git a/data/org.libvirt.Connect.xml b/data/org.libvirt.Connect.xml
> index 787cd8d..9849abe 100644
> --- a/data/org.libvirt.Connect.xml
> +++ b/data/org.libvirt.Connect.xml
> @@ -4,51 +4,75 @@
>  
>
>  
> +   +value="See 
> https://libvirt.org/html/libvirt-libvirt-domain.html#virConnectListDomains"/>

The function should be virConnectListAllDomains.  I'll fix that before
pushing.

>
>
>  
>  
> +   +value="See 
> https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainCreateXML"/>
>
>
>
>  
>  
> +   +value="See 
> https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainDefineXML"/>
>
>
>  
>  
> +   +value="See 
> https://libvirt.org/html/libvirt-libvirt-domain.html#VIR_DOMAIN_EVENT_CRASHED"/>
>
>
>  

I was thinking about these domain lifecycle signals and maybe we should
rewrite it into one signal and as a doc string we should mention the
event callback function, which describes the arguments as well
.

And unrelated to this patch itself, I've just noticed, that the XML
describes the first argument as reason, which is not correct, the code
sets the first argument to the domain name.  So I'm thinking about
having the signal like this:


  https://libvirt.org/html/libvirt-libvirt-domain.html#virConnectDomainEventCallback"/>
  
  
  


I'll fix the links for the other events to point to the callback
function and the domain lifecycle events can be fixed by followup
patches.

>  
> +   +value="See 
> https://libvirt.org/html/libvirt-libvirt-domain.html#VIR_DOMAIN_EVENT_DEFINED"/>
>
>
>  
>  
> +   +value="See 
> https://libvirt.org/html/libvirt-libvirt-domain.html#VIR_DOMAIN_EVENT_SUSPENDED"/>
>
>
>  
>  
> +   +value="See 
> https://libvirt.org/html/libvirt-libvirt-domain.html#VIR_DOMAIN_EVENT_RESUMED"/>
>
>
>  
>  
> +   +value="See 
> https://libvirt.org/html/libvirt-libvirt-domain.html#VIR_DOMAIN_EVENT_SHUTDOWN"/>
>
>
>  
>  
> +   +value="See 
> https://libvirt.org/html/libvirt-libvirt-domain.html#VIR_DOMAIN_EVENT_STARTED"/>
>
>
>  
>  
> +   +value="See 
> https://libvirt.org/html/libvirt-libvirt-domain.html#VIR_DOMAIN_EVENT_STOPPED"/>
>
>
>  
>  
> +   +value="See 
> https://libvirt.org/html/libvirt-libvirt-domain.html#VIR_DOMAIN_EVENT_SUSPENDED"/>
>
>
>  
>  
> +   +value="See 
> https://libvirt.org/html/libvirt-libvirt-domain.html#VIR_DOMAIN_EVENT_UNDEFINED"/>
>
>
>  
> diff --git a/data/org.libvirt.Domain.xml b/data/org.libvirt.Domain.xml
> index e79fb5e..4b927a8 100644
> --- a/data/org.libvirt.Domain.xml
> +++ b/data/org.libvirt.Domain.xml
> @@ -3,58 +3,108 @@
>  
>  
>
> -
> -
> -
> -
> -
> -
> -
> -
> +
> +   +value="See 
> https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainGetName"/>
> +
> +
> +   +value="See 
> https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainGetUUID"/>

This should be virDomainGetUUIDString, I'll fix that before pushing.

Pavel


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

Re: [libvirt] [tck PATCH] require libguestfs-tool package

[Erik Skultety]

On Sun, Mar 25, 2018 at 06:47:14PM -0400, Laine Stump wrote:
> The nwfilter tests use virt-builder to download and configure a guest
> appliance, but we don't require the libguestfs-tools package. Up until
> now it's just happened that it was always already installed on the
> test systems, so it wasn't a problem, but it's annoying to have to
> type the one extra install command when setting up a new machine to
> run the tests.
>
> Signed-off-by: Laine Stump 
> ---
>  perl-Sys-Virt-TCK.spec.PL | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/perl-Sys-Virt-TCK.spec.PL b/perl-Sys-Virt-TCK.spec.PL
> index 7c83be9..9694227 100644
> --- a/perl-Sys-Virt-TCK.spec.PL
> +++ b/perl-Sys-Virt-TCK.spec.PL
> @@ -75,6 +75,7 @@ Requires: perl(TAP::Formatter::JUnit)
>  Requires: perl(TAP::Harness::Archive)
>  Requires: perl(Net::OpenSSH)
>  Requires: perl(IO::Pty)
> +Requires: libguestfs-tools
>  Requires: /usr/bin/mkisofs
>  vuildArchitectures: noarch

Reviewed-by: Erik Skultety 

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