Re: [libvirt] [PATCH v2 3/3] libxl: add tunnelled migration support

[Jim Fehlig]

On 02/07/2017 05:35 PM, Joao Martins wrote:

From: Bob Liu 

Tunnelled migration doesn't require any extra network connections beside
the libvirt daemon.  It's capable of strong encryption and the default
option of openstack-nova.

This patch adds the tunnelled migration(Tunnel3params) support to libxl.
On the source side, the data flow is:

 * libxlDoMigrateSend() -> pipe libxlTunnel3MigrationFunc() polls pipe
 * out and then write to dest stream.

While on the destination side:
 * Stream -> pipe -> 'recvfd of libxlDomainStartRestore'

The usage is the same as p2p migration, execpt adding one extra
'--tunnelled' to the libvirt p2p migration command.

Signed-off-by: Bob Liu 
Signed-off-by: Joao Martins 
---
v2 (comments from Jim):
 * Adjust common preparetunnel function reflecting v1 suggestions
   - Using common top-level Prepare function by adding is_tunnel variable
   - Using libxl_migration PrepareAny added in patch 1
   - virHook support in PrepareTunnel3
 * Move virThreadPtr from libxlTunnelMigrationThread into libxlTunnelControl
 * Rename function local variable from tmThreadPtr to arg
 * Remove redundant assignment "tmThreadPtr = >tmThread;" always being not
 null.
---
 src/libxl/libxl_driver.c|  49 ++--
 src/libxl/libxl_migration.c | 280 +---
 src/libxl/libxl_migration.h |   9 ++
 3 files changed, 313 insertions(+), 25 deletions(-)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 7bc8adf..b34f120 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -5938,14 +5938,14 @@ libxlDomainMigratePrepareCommon(virConnectPtr dconn,
 char **cookieout ATTRIBUTE_UNUSED,
 int *cookieoutlen ATTRIBUTE_UNUSED,
 unsigned int flags,
-void *data)
+void *data,
+bool is_tunnel)
 {
 libxlDriverPrivatePtr driver = dconn->privateData;
 virDomainDefPtr def = NULL;
 const char *dom_xml = NULL;
 const char *dname = NULL;
 const char *uri_in = NULL;
-char **uri_out = data;

 #ifdef LIBXL_HAVE_NO_SUSPEND_RESUME
 virReportUnsupportedError();
@@ -5971,12 +5971,25 @@ libxlDomainMigratePrepareCommon(virConnectPtr dconn,
 if (!(def = libxlDomainMigrationPrepareDef(driver, dom_xml, dname)))
 goto error;

-if (virDomainMigratePrepare3ParamsEnsureACL(dconn, def) < 0)
-goto error;
+if (is_tunnel) {
+virStreamPtr st = data;

-if (libxlDomainMigrationPrepare(dconn, , uri_in, uri_out,
-cookiein, cookieinlen, flags) < 0)
-goto error;
+if (virDomainMigratePrepareTunnel3ParamsEnsureACL(dconn, def) < 0)
+goto error;
+
+if (libxlDomainMigrationPrepareTunnel3(dconn, st, , cookiein,
+   cookieinlen, flags) < 0)
+goto error;
+} else {
+char **uri_out = data;
+
+if (virDomainMigratePrepare3ParamsEnsureACL(dconn, def) < 0)
+goto error;
+
+if (libxlDomainMigrationPrepare(dconn, , uri_in, uri_out,
+cookiein, cookieinlen, flags) < 0)
+goto error;
+}


Same comment here about the ACL checks and 'make check. With both patches 
applied

./libxl/libxl_driver.c:5981 Mismatch check 
'virDomainMigratePrepareTunnel3ParamsEnsureACL' for function 
'libxlDomainMigratePrepareCommon'
./libxl/libxl_driver.c:5990 Mismatch check 
'virDomainMigratePrepare3ParamsEnsureACL' for function 
'libxlDomainMigratePrepareCommon'




 return 0;

@@ -5999,7 +6012,24 @@ libxlDomainMigratePrepare3Params(virConnectPtr dconn,
 return libxlDomainMigratePrepareCommon(dconn, params, nparams,
cookiein, cookieinlen,
cookieout, cookieoutlen,
-   flags, uri_out);
+   flags, uri_out, false);
+}
+
+static int
+libxlDomainMigratePrepareTunnel3Params(virConnectPtr dconn,
+   virStreamPtr st,
+   virTypedParameterPtr params,
+   int nparams,
+   const char *cookiein,
+   int cookieinlen,
+   char **cookieout ATTRIBUTE_UNUSED,
+   int *cookieoutlen ATTRIBUTE_UNUSED,
+   unsigned int flags)
+{
+return libxlDomainMigratePrepareCommon(dconn, params, nparams,
+   cookiein, cookieinlen,
+   cookieout, cookieoutlen,
+   

Re: [libvirt] [PATCH v2 2/3] libxl: streamline top-level migrate functions

[Jim Fehlig]

On 02/07/2017 05:35 PM, Joao Martins wrote:

This allows us to reuse a single function for both tunnelled and
non-tunnelled variants.

Signed-off-by: Joao Martins 
---
New in v2
---
 src/libxl/libxl_driver.c | 36 +++-
 1 file changed, 27 insertions(+), 9 deletions(-)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 3a69720..7bc8adf 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -5930,21 +5930,22 @@ libxlDomainMigrateBegin3Params(virDomainPtr domain,
 }

 static int
-libxlDomainMigratePrepare3Params(virConnectPtr dconn,
- virTypedParameterPtr params,
- int nparams,
- const char *cookiein,
- int cookieinlen,
- char **cookieout ATTRIBUTE_UNUSED,
- int *cookieoutlen ATTRIBUTE_UNUSED,
- char **uri_out,
- unsigned int flags)
+libxlDomainMigratePrepareCommon(virConnectPtr dconn,
+virTypedParameterPtr params,
+int nparams,
+const char *cookiein,
+int cookieinlen,
+char **cookieout ATTRIBUTE_UNUSED,
+int *cookieoutlen ATTRIBUTE_UNUSED,
+unsigned int flags,
+void *data)
 {
 libxlDriverPrivatePtr driver = dconn->privateData;
 virDomainDefPtr def = NULL;
 const char *dom_xml = NULL;
 const char *dname = NULL;
 const char *uri_in = NULL;
+char **uri_out = data;

 #ifdef LIBXL_HAVE_NO_SUSPEND_RESUME
 virReportUnsupportedError();
@@ -5985,6 +5986,23 @@ libxlDomainMigratePrepare3Params(virConnectPtr dconn,
 }

 static int
+libxlDomainMigratePrepare3Params(virConnectPtr dconn,
+ virTypedParameterPtr params,
+ int nparams,
+ const char *cookiein,
+ int cookieinlen,
+ char **cookieout ATTRIBUTE_UNUSED,
+ int *cookieoutlen ATTRIBUTE_UNUSED,
+ char **uri_out,
+ unsigned int flags)
+{
+return libxlDomainMigratePrepareCommon(dconn, params, nparams,
+   cookiein, cookieinlen,
+   cookieout, cookieoutlen,
+   flags, uri_out);
+}


It appears the ACL check must be done in libxlDomainMigratePrepare3Params to 
satisfy 'make check'


./libxl/libxl_driver.c:5978 Mismatch check 
'virDomainMigratePrepare3ParamsEnsureACL' for function 
'libxlDomainMigratePrepareCommon'


Regards,
Jim


+
+static int
 libxlDomainMigratePerform3Params(virDomainPtr dom,
  const char *dconnuri,
  virTypedParameterPtr params,



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

[libvirt] [PATCH 1/2] util: storage: split function for JSON backing volume parsing in two

[Tomáš Golembiovský]

Split virStorageSourceParseBackingJSON into two functions so that the
core can be reused by other functions. The new function called
virStorageSourceParseBackingJSONInternal accepts virJSONValuePtr.

Signed-off-by: Tomáš Golembiovský 
---
 src/util/virstoragefile.c | 35 +--
 1 file changed, 25 insertions(+), 10 deletions(-)

diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 3d4bda700..3698eeeda 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -3053,29 +3053,28 @@ 
virStorageSourceParseBackingJSONDeflatten(virJSONValuePtr json)
 
 
 static int
-virStorageSourceParseBackingJSON(virStorageSourcePtr src,
- const char *json)
+virStorageSourceParseBackingJSONInternal(virStorageSourcePtr src,
+ virJSONValuePtr json)
 {
-virJSONValuePtr root = NULL;
 virJSONValuePtr fixedroot = NULL;
 virJSONValuePtr file;
 const char *drvname;
 size_t i;
 int ret = -1;
 
-if (!(root = virJSONValueFromString(json)))
-return -1;
-
-if (!(file = virJSONValueObjectGetObject(root, "file"))) {
-if (!(fixedroot = virStorageSourceParseBackingJSONDeflatten(root)))
+if (!(file = virJSONValueObjectGetObject(json, "file"))) {
+if (!(fixedroot = virStorageSourceParseBackingJSONDeflatten(json)))
 goto cleanup;
 
 file = fixedroot;
 }
 
 if (!(drvname = virJSONValueObjectGetString(file, "driver"))) {
+char *str = virJSONValueToString(json, false);
 virReportError(VIR_ERR_INVALID_ARG, _("JSON backing volume defintion "
-  "'%s' lacks driver name"), json);
+  "'%s' lacks driver name"),
+NULLSTR(str));
+VIR_FREE(str);
 goto cleanup;
 }
 
@@ -3091,12 +3090,28 @@ virStorageSourceParseBackingJSON(virStorageSourcePtr 
src,
  "driver '%s'"), drvname);
 
  cleanup:
-virJSONValueFree(root);
 virJSONValueFree(fixedroot);
 return ret;
 }
 
 
+static int
+virStorageSourceParseBackingJSON(virStorageSourcePtr src,
+ const char *json)
+{
+virJSONValuePtr root = NULL;
+int ret = -1;
+
+if (!(root = virJSONValueFromString(json)))
+return -1;
+
+ret = virStorageSourceParseBackingJSONInternal(src, root);
+
+virJSONValueFree(root);
+return ret;
+}
+
+
 virStorageSourcePtr
 virStorageSourceNewFromBackingAbsolute(const char *path)
 {
-- 
2.11.1

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

[libvirt] [PATCH 2/2] util: storage: add JSON backing volume parser 'raw' block driver

[Tomáš Golembiovský]

The 'raw' block driver in Qemu is not directly interesting from
libvirt's perspective, but it can be layered above some other block
drivers and this may be interesting for the user.

The patch adds support for the 'raw' block driver. The driver is treated
simply as a pass-through and child driver in JSON is queried to get the
necessary information.

Signed-off-by: Tomáš Golembiovský 
---
 src/util/virstoragefile.c | 16 
 tests/virstoragetest.c|  6 ++
 2 files changed, 22 insertions(+)

diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 3698eeeda..0447016bf 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -2648,6 +2648,11 @@ virStorageSourceParseBackingColon(virStorageSourcePtr 
src,
 
 
 static int
+virStorageSourceParseBackingJSONInternal(virStorageSourcePtr src,
+ virJSONValuePtr json);
+
+
+static int
 virStorageSourceParseBackingJSONPath(virStorageSourcePtr src,
  virJSONValuePtr json,
  int type)
@@ -2963,6 +2968,16 @@ virStorageSourceParseBackingJSONRBD(virStorageSourcePtr 
src,
 return -1;
 }
 
+static int
+virStorageSourceParseBackingJSONRaw(virStorageSourcePtr src,
+virJSONValuePtr json,
+int opaque ATTRIBUTE_UNUSED)
+{
+/* There are no interesting attributes in raw driver.
+ * Treat it as pass-through.
+ */
+return virStorageSourceParseBackingJSONInternal(src, json);
+}
 
 struct virStorageSourceJSONDriverParser {
 const char *drvname;
@@ -2985,6 +3000,7 @@ static const struct virStorageSourceJSONDriverParser 
jsonParsers[] = {
 {"sheepdog", virStorageSourceParseBackingJSONSheepdog, 0},
 {"ssh", virStorageSourceParseBackingJSONSSH, 0},
 {"rbd", virStorageSourceParseBackingJSONRBD, 0},
+{"raw", virStorageSourceParseBackingJSONRaw, 0},
 };
 
 
diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c
index f766df115..1bf490c43 100644
--- a/tests/virstoragetest.c
+++ b/tests/virstoragetest.c
@@ -1492,6 +1492,12 @@ mymain(void)
"\n"
"  \n"
"\n");
+TEST_BACKING_PARSE("json:{ \"file\": { "
+"\"driver\": \"raw\","
+"\"file\": {"
+"\"driver\": \"file\","
+"\"filename\": \"/path/to/file\" } } }",
+   "\n");
 
  cleanup:
 /* Final cleanup */
-- 
2.11.1

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

[libvirt] [PATCH 0/2] Add support for 'raw' block driver to Qemu JSON backend parsing code

[Tomáš Golembiovský]

The raw driver is not very interesting on its own but it can be layered on top
of other block drivers. New parameters that allow using only part of the
underlying file/device were recently added to Qemu. This is useful e.g. to
directly access partitions inside a disk image or disks inside an archive (like
OVA).

This feature is utilised in OVA import in virt-v2v tool where we access the
disks directly in OVA without needing to unpack the tar archive first. After
implementing this in virt-v2v we noticed that it does not work when libguestfs
uses libvirt as a backend because libvirt fails with error:

internal error: missing parser implementation for JSON backing volume 
driver 'raw'

Tomáš Golembiovský (2):
  util: storage: split function for JSON backing volume parsing in two
  util: storage: add JSON backing volume parser 'raw' block driver

 src/util/virstoragefile.c | 51 +--
 tests/virstoragetest.c|  6 ++
 2 files changed, 47 insertions(+), 10 deletions(-)

-- 
2.11.1

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

[libvirt] [PATCH] valgrind: add suppression for bash memory leak

[Tomáš Golembiovský]

Add suppression for memory leak in bash observerd with bash 4.4.011 on
Arch Linux.

Signed-off-by: Tomáš Golembiovský 
---
 tests/.valgrind.supp | 9 +
 1 file changed, 9 insertions(+)

diff --git a/tests/.valgrind.supp b/tests/.valgrind.supp
index d4fef857b..d474d63e9 100644
--- a/tests/.valgrind.supp
+++ b/tests/.valgrind.supp
@@ -32,6 +32,15 @@
...
obj:*/bin/bash
 }
+{
+   bashMemoryLeak4
+   Memcheck:Leak
+   match-leak-kinds: definite
+   fun:malloc
+   fun:xmalloc
+   fun:set_default_locale
+   fun:main
+}
 #
 # Failure seen in /usr/lib64/ld-2.15.so
 #
-- 
2.11.1

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

[libvirt] [PATCH] qemu: Set umask before calling mknod()

[Andrea Bolognani]

When we populate the private /dev that's going to be used by
an isolated QEMU process, we take care all metadata matches
what's in the top-level namespace: in particular, we copy the
file permissions directly.

However, since the permissions passed to mknod() are still
affected by the active umask, we need to set it to a very
permissive value before creating device nodes to avoid file
access issues.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1421036
---
 src/qemu/qemu_domain.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index f62bf8f..7993acc 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -7040,6 +7040,7 @@ qemuDomainCreateDeviceRecursive(const char *device,
 #ifdef WITH_SELINUX
 char *tcon = NULL;
 #endif
+mode_t oldUmask = umask((mode_t) 0);
 
 if (!ttl) {
 virReportSystemError(ELOOP,
@@ -7205,6 +7206,7 @@ qemuDomainCreateDeviceRecursive(const char *device,
 #ifdef WITH_SELINUX
 freecon(tcon);
 #endif
+umask(oldUmask);
 return ret;
 }
 
@@ -7678,6 +7680,7 @@ qemuDomainAttachDeviceMknodHelper(pid_t pid 
ATTRIBUTE_UNUSED,
 int ret = -1;
 bool delDevice = false;
 bool isLink = S_ISLNK(data->sb.st_mode);
+mode_t oldUmask = umask((mode_t) 0);
 
 virSecurityManagerPostFork(data->driver->securityManager);
 
@@ -7756,6 +7759,7 @@ qemuDomainAttachDeviceMknodHelper(pid_t pid 
ATTRIBUTE_UNUSED,
 freecon(data->tcon);
 #endif
 virFileFreeACLs(>acl);
+umask(oldUmask);
 return ret;
 }
 
-- 
2.7.4

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

Re: [libvirt] [PATCH] RFC: qemu: add spice/virgl rendernode

[Marc-André Lureau]

Hi

- Original Message -
> On Mon, Feb 13, 2017 at 08:08:20AM -0500, Marc-André Lureau wrote:
> > Hi
> > 
> > - Original Message -
> > > On Mon, Feb 13, 2017 at 07:19:04AM -0500, Marc-André Lureau wrote:
> > > > Hi
> > > > 
> > > > - Original Message -
> > > > > On Mon, Feb 13, 2017 at 03:51:48PM +0400, marcandre.lur...@redhat.com
> > > > > wrote:
> > > > > > From: Marc-André Lureau 
> > > > > > 
> > > > > > I am working on a WIP series to add QEMU Spice/virgl rendernode
> > > > > > option.
> > > > > > Since rendernodes are not stable across reboots, I propose that
> > > > > > QEMU
> > > > > > accepts also a PCI address (other bus types may be added in the
> > > > > > future).
> > > > > 
> > > > > Hmm, can you elaborate on this aspect ?  It feels like a parallel
> > > > > to saying NIC device names are not stable, so we should configure
> > > > > guests using PCI addresses instead of 'eth0', etc but we stuck with
> > > > > using NIC names in libvirt on the basis that you can create udev
> > > > > rules to ensure stable naming ?
> > > > > 
> > > > > So is there not a case to be made that if you want stable render
> > > > > device names when multiple NICs are present, then you should use
> > > > > udev to ensure a given device always maps to the same PCI dev.
> > > > 
> > > > I thought it was simpler to use a PCI address (do you expect users
> > > > to create udev rules for the GPUs?)
> > > 
> > > Well most users will only have 1 GPU so surely this won't be a problem
> > > in the common case.  Is it possible to get some stable naming rules into
> > > udev upstream though, so all distros get stable names by default
> > 
> > Optimus is getting more and more mainstream, see recent Fedora desktop
> > effort (fwiw I have a t460p nouveau/i915). I don't think a random user of
> > such hw/laptop should have to create udev rules.
> > 
> > I suppose systemd-udev could learn to create stable path with help
> > from src/udev/udev-builtin-path_id.c. I will work on it. However,
> > I have virt-manager code to lookup GPU infos/path, using libdrm,
> > and it is unlikely that it will work with the udev rules. So I'll
> > have to patch libdrm to support that too.
> 
> The generic goal is that Libvirt should be providing enough information
> for apps to be able to configure the guest, without resorting to side
> channels like libdrm. This is to ensure that apps can manage guests
> with no loss of functionality even when connected to a remote libvirt.
> e.g. libdrm isn't going to be able to enumerate remote GPUs, for
> virt-manager, so the neccessary info needs to be exposed by libvirt
> via its virNodeDevice APIs. We can already identify the PCI device
> and that its a GPU device, but I imagine we're not reporting any
> data about DRI render paths associated with the GPUs we report.
> So I think that's a gap we'd need to fill

Ah that makes sense, I'll probably have to drop my wip libdrm virt-manager code 
(although I'd like virt-manager to pick the current display GPU by default, 
since it is less likely to have issues, perhaps it will still be useful).

So I assume it's fine for libvirt to link with libdrm (it's not a really a 
graphical library, it's more a system-level library). I'll investigate the 
virNodeDevice changes.

thanks

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

Re: [libvirt] [PATCH] libxl: implement virDomainObjCheckIsActive

[Jim Fehlig]

Sagar Ghuge wrote:
> Hi Jim,
> 
> Thank you for replying. I am planning to participate in GSoC'17 and
> this is my first patch towards it. This is from Byte sized task which
> I picked up and trying to solve it.

Ah, yes, this one

http://wiki.libvirt.org/page/BiteSizedTasks#Add_function_that_raises_error_if_domain_is_not_active

> Yes you are right, pattern is used
> throughout the libxl driver and many others but this is just a
> rudimentary patch which will help me to get suggestion that am I on
> right path. if yes then I can I can go ahead and make changes
> accordingly.

It looks good to me, although I'm not sure how to best split up the work. Maybe
a patch that adds virDomainObjCheckIsActive, and then a patch for each driver
that replaces virDomainObjIsActive with virDomainObjCheckIsActive?

AFAICT, Cole added that task to the wiki. Perhaps he has a suggestion on the
best way to organize the work.

Regards,
Jim

> 
> On Sun, Feb 12, 2017 at 7:23 PM, Jim Fehlig  wrote:
>> On 02/11/2017 01:31 AM, Sagar Ghuge wrote:
>>> Add function which raises error if domain is
>>> not active
>>>
>>> Signed-off-by: Sagar Ghuge 
>>> ---
>>>  src/conf/domain_conf.c   | 13 +
>>>  src/conf/domain_conf.h   |  1 +
>>>  src/libxl/libxl_driver.c |  4 +---
>>>  3 files changed, 15 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>>> index 1bc72a4..10a69af 100644
>>> --- a/src/conf/domain_conf.c
>>> +++ b/src/conf/domain_conf.c
>>> @@ -2995,6 +2995,19 @@ virDomainObjWait(virDomainObjPtr vm)
>>>  }
>>>
>>>
>>> +int
>>> +virDomainObjCheckIsActive(virDomainObjPtr vm)
>>> +{
>>> +if (!virDomainObjIsActive(vm)) {
>>> +virReportError(VIR_ERR_OPERATION_FAILED, "%s",
>>> +   _("domain is not running"));
>>> +return -1;
>>> +}
>>> +
>>> +return 0;
>>> +}
>>> +
>>> +
>>>  /**
>>>   * Waits for domain condition to be triggered for a specific period of
>>> time.
>>>   *
>>> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
>>> index dd79206..b6c7826 100644
>>> --- a/src/conf/domain_conf.h
>>> +++ b/src/conf/domain_conf.h
>>> @@ -2559,6 +2559,7 @@ bool virDomainObjTaint(virDomainObjPtr obj,
>>>
>>>  void virDomainObjBroadcast(virDomainObjPtr vm);
>>>  int virDomainObjWait(virDomainObjPtr vm);
>>> +int virDomainObjCheckIsActive(virDomainObjPtr vm);
>>>  int virDomainObjWaitUntil(virDomainObjPtr vm,
>>>unsigned long long whenms);
>>>
>>> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
>>> index 74cb05a..3a487ac 100644
>>> --- a/src/libxl/libxl_driver.c
>>> +++ b/src/libxl/libxl_driver.c
>>> @@ -1183,10 +1183,8 @@ libxlDomainSuspend(virDomainPtr dom)
>>>  if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0)
>>>  goto cleanup;
>>>
>>> -if (!virDomainObjIsActive(vm)) {
>>> -virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("Domain is not
>>> running"));
>>
>> This is the standard pattern used throughout the libxl driver and many other
>> drivers as well. Why do you only want to change this one instance? IMO if
>> such a change is desired it should be done consistently across the code
>> base.
>>
>> Regards,
>> Jim
>>
>>
>>> +if (virDomainObjCheckIsActive(vm) < 0)
>>>  goto endjob;
>>> -}
>>>
>>>  if (virDomainObjGetState(vm, NULL) != VIR_DOMAIN_PAUSED) {
>>>  if (libxl_domain_pause(cfg->ctx, vm->def->id) != 0) {
>>>
> 
> 
> 

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

Re: [libvirt] [v4 0/9] Support cache tune in libvirt

[Marcelo Tosatti]

On Mon, Feb 13, 2017 at 03:42:04PM +0800, Eli Qiao wrote:
> > > L3data:0=0xff00;...
> > > L3code:0=0xff00;...
> > >  
> > > * Would you please help to test if the functions work.
> >  
> > Setting up CDP machine.
> >  
> > Unrelated:
> >  
> > Found a bug:
> >  
> > The code should scan for all directories in resctrlfs,
> > and then find free CBM space from that:
> >  
> >  
> > free_cbm_space = ~(resctrldir1.CBM_bits &
> > resctrldir2.CBM_bits &
> > ...
> > resctrldirN.CBM_bits)
> >  
> > For all resctrlfs directories.
> >  
> > The bug is as follows:
> >  
> > Create a directory in resctrlfs by hand:
> >  
> > # mkdir newres
> 
> Libvirt will not aware this after it starts running, so we should not allow 
> operate /sys/fs/.

Are you saying that usage of the resctrlfs filesystem should not be
allowed after libvirt starts? I don't think this is correct.

> we will scan for all directors while the libvirt daemon begin running, and 
> libvirt will remove exist directories if no tasks inside of it.  



> 
>   
> > # cd newres
> > # echo "L3:0=3;1=f" > schemata
> > # virsh start guest
> > # cd ../b4c270b5-e0f9-4106-a446-69032872ed7e
> > # cat schemata
> > L3:0=3;1=f
> >  
> > That is, it is using the same CBM space as the "newres"
> > reservation.
> >  
> >  
> 
> 
> As user create a new directory after libvirt running, it don’t notice newly 
> created directory under /sys/fs/resctrl.
> 
> That will lead mess, this should be forbidden.  

Well, this means that only libvirt can use the resctrlfs filesystem.

This forbids other applications which require allocation
of CAT resources from being used. 

Its simple to fix it: move the scanning of resctrlfs data from libvirt
initialization time to:

- VM initialization time
and
- virConnectGetCapabilities time

The second case is necessary to get updated free space information.



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

Re: [libvirt] [PATCH] qemu: Allow empty script path to

[Michal Privoznik]

On 02/02/2017 06:12 PM, Daniel P. Berrange wrote:
> On Thu, Feb 02, 2017 at 04:55:29PM +0100, Kashyap Chamarthy wrote:
>> On Thu, Feb 02, 2017 at 03:29:15PM +, Daniel P. Berrange wrote:
>>> On Thu, Feb 02, 2017 at 04:21:39PM +0100, Michal Privoznik wrote:
 On 02/02/2017 02:56 PM, Daniel P. Berrange wrote:
> On Thu, Feb 02, 2017 at 02:49:48PM +0100, Michal Privoznik wrote:
>> Before 9c17d665fdc5f (v1.3.2 - I know, right?) it was possible to
>> have the following interface configuration:
>>
>>   
>> 
>>   
>>
>> This resulted in -netdev tap,script=,.. Fortunately, qemu helped
>> us to get away with this as it just ignored the empty script
>> path. However, after the commit mentioned above it's libvirtd
>> who is executing the script. Unfortunately without special
>> case-ing empty script path.
>>
>> Signed-off-by: Michal Privoznik 
>
> This was always invalid input and the fact that it happened
> to work in the past is just luck. Since this has been broken since
> 1.3.2 there's plenty of libvirt releases that reject this, and so
> any app realistically has to be fixed to omit the 

Re: [libvirt] [PATCH 7/9] perf: add page_faults_maj software perf event support

[John Ferlan]

On 02/13/2017 01:49 AM, Nitesh Konkar wrote:
> 
> 
> 
> On Fri, Feb 10, 2017 at 3:22 AM, John Ferlan  > wrote:
> 
> 
> 
> On 01/27/2017 06:01 AM, Nitesh Konkar wrote:
> > This patch adds support and documentation
> > for the page_faults_maj perf event.
> >
> > Signed-off-by: Nitesh Konkar  >
> > ---
> >  docs/formatdomain.html.in 
>  
>  |  7 +++
> >  docs/news.xml   |  4 ++--
> >  docs/schemas/domaincommon.rng   |  1 +
> >  include/libvirt/libvirt-domain.h| 10 ++
> >  src/libvirt-domain.c|  3 +++
> >  src/qemu/qemu_driver.c  |  1 +
> >  src/util/virperf.c  |  5 -
> >  src/util/virperf.h  |  1 +
> >  tests/genericxml2xmlindata/generic-perf.xml |  1 +
> >  tools/virsh.pod |  5 +
> >  10 files changed, 35 insertions(+), 3 deletions(-)
> >
> 
> NB: Similar comments from the page_faults_min...
> 
> > diff --git a/docs/formatdomain.html.in 
> b/docs/formatdomain.html.in 
> > index 1857168..50a6bdb 100644
> > --- a/docs/formatdomain.html.in 
> > +++ b/docs/formatdomain.html.in 
> > @@ -1943,6 +1943,7 @@
> >event name='context_switches' enabled='no'/
> >event name='cpu_migrations' enabled='no'/
> >event name='page_faults_min' enabled='no'/
> > +  event name='page_faults_maj' enabled='no'/
> >  /perf
> >  ...
> >  
> > @@ -2052,6 +2053,12 @@
> >platform
> >perf.page_faults_min
> >  
> > +
> > +  page_faults_maj
> > +  the count of major page faults by applications running on the
> > +  platform
> > +  perf.page_faults_maj
> > +
> 
> As already noted in patch 3... is maj+min the same as what patch 3
> provides?
> 
> 
> maj+min is not always exactly the same as page faults. Sometimes there
> is a small offset
> value.
> 
> Eg: perf record -a --event={page-faults,major-faults,minor-faults}
> 47
> page-faults   
>   
> 
> 0
> major-faults  
>   
> 
> 46 minor-faults
> Offset by 1
> 
> Eg:  virsh domstats --perf
> Domain: 'Fedora'
>   perf.page_faults=890
>   perf.page_faults_min=890
>   perf.page_faults_maj=0
> Here maj+min=page_faults
> 
> Thus are all necessary?
> 
> I am not sure on this part. Probably yes as we dont want
> the user to add min+maj to get (approx)total page faults.
>  
> 

I don't mind all 3 being present... still if I'm going to ask the
question, then someone getting the stats will ask the question... they
may also wonder why maj+min != total.

Perhaps something you could dig deeper on with the kernel code
descriptions that are setting the value.

My assumption is it's the "time" of the sample. That is a total page
fault could have been counted even though it hadn't been counted as a
maj or min page fault.

John

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

Re: [libvirt] [PATCH 1/9] perf: add cpu_clock software perf event support

[John Ferlan]

On 02/13/2017 03:01 AM, Nitesh Konkar wrote:
> 
> 
> On Fri, Feb 10, 2017 at 3:10 AM, John Ferlan  > wrote:
> 
> 
> 
> On 01/27/2017 06:01 AM, Nitesh Konkar wrote:
> > This patch adds support and documentation for
> > the cpu_clock perf event.
> >
> > Signed-off-by: Nitesh Konkar  >
> > ---
> >  docs/formatdomain.html.in  
>  |  6 ++
> >  docs/news.xml   |  4 ++--
> >  docs/schemas/domaincommon.rng   |  1 +
> >  include/libvirt/libvirt-domain.h| 10 ++
> >  src/libvirt-domain.c|  2 ++
> >  src/qemu/qemu_driver.c  |  1 +
> >  src/util/virperf.c  |  6 +-
> >  src/util/virperf.h  |  1 +
> >  tests/genericxml2xmlindata/generic-perf.xml |  1 +
> >  tools/virsh.pod |  3 +++
> >  10 files changed, 32 insertions(+), 3 deletions(-)
> >
> > diff --git a/docs/formatdomain.html.in
>  b/docs/formatdomain.html.in
> 
> > index 3f7f875..e44e758 100644
> > --- a/docs/formatdomain.html.in 
> > +++ b/docs/formatdomain.html.in 
> > @@ -1937,6 +1937,7 @@
> >event name='stalled_cycles_frontend' enabled='no'/
> >event name='stalled_cycles_backend' enabled='no'/
> >event name='ref_cpu_cycles' enabled='no'/
> > +  event name='cpu_clock' enabled='no'/
> >  /perf
> >  ...
> >  
> > @@ -2015,6 +2016,11 @@
> >   by applications running on the platform
> >perf.ref_cpu_cycles
> >  
> > +
> > +  cpu_clock
> > +  the count of cpu clock by applications running on the
> platform
> > +  perf.cpu_clock
> > +
> >
> 
> "the count of cpu clock by applications..."
> 
> doesn't convey enough meaning. Is that cycles? Time?  something else?
> 
> The virsh.pod description seems to give the best hint...
> 
> Some subsequent patches had longer descriptions in the virsh.pod, which
> got me thinking - where is the best place to have the longer
> description. I would think either in formatdomain or the
> libvirt-domain.{c|} description.  At least that way you don't need to
> mess with the 80 column formatting that is "nice" to have in virsh.pod
> output
> 
> Lets keep the virsh.pod and libvirt-domain.c explanantion terse and let
> formatdomain have detailed explanation of the events. Do you agree?
> 
> 

I'm fine with that as long as the terse description can provide some
amount of context.

John

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

Re: [libvirt] [RFC] Substitute poll by epoll in virEventPollRunOnce

[Roman Bogorodskiy]

  Daniel P. Berrange wrote:

> > 2) Are ifdef an acceptable solution to epoll avaliability issues?
> 
> There are quite a large number of ifdefs in your patch below.
> 
> I think it could be worth having a separate vireventepoll.c
> for the epoll impl. Then choose to compile vireventpoll.c vs
> vireventepoll.c at configure time.  We could have vireventcommon.c
> if there is some portion of code that can be shared.

If we're going this route, would it make sense to use an existing
abstraction layer like libevent?

(Asking out of curiosity, not from experience)

Roman Bogorodskiy


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

Re: [libvirt] [PATCH] RFC: qemu: add spice/virgl rendernode

[Daniel P. Berrange]

On Mon, Feb 13, 2017 at 08:08:20AM -0500, Marc-André Lureau wrote:
> Hi
> 
> - Original Message -
> > On Mon, Feb 13, 2017 at 07:19:04AM -0500, Marc-André Lureau wrote:
> > > Hi
> > > 
> > > - Original Message -
> > > > On Mon, Feb 13, 2017 at 03:51:48PM +0400, marcandre.lur...@redhat.com
> > > > wrote:
> > > > > From: Marc-André Lureau 
> > > > > 
> > > > > I am working on a WIP series to add QEMU Spice/virgl rendernode 
> > > > > option.
> > > > > Since rendernodes are not stable across reboots, I propose that QEMU
> > > > > accepts also a PCI address (other bus types may be added in the
> > > > > future).
> > > > 
> > > > Hmm, can you elaborate on this aspect ?  It feels like a parallel
> > > > to saying NIC device names are not stable, so we should configure
> > > > guests using PCI addresses instead of 'eth0', etc but we stuck with
> > > > using NIC names in libvirt on the basis that you can create udev
> > > > rules to ensure stable naming ?
> > > > 
> > > > So is there not a case to be made that if you want stable render
> > > > device names when multiple NICs are present, then you should use
> > > > udev to ensure a given device always maps to the same PCI dev.
> > > 
> > > I thought it was simpler to use a PCI address (do you expect users
> > > to create udev rules for the GPUs?)
> > 
> > Well most users will only have 1 GPU so surely this won't be a problem
> > in the common case.  Is it possible to get some stable naming rules into
> > udev upstream though, so all distros get stable names by default
> 
> Optimus is getting more and more mainstream, see recent Fedora desktop effort 
> (fwiw I have a t460p nouveau/i915). I don't think a random user of such 
> hw/laptop should have to create udev rules.
> 
> I suppose systemd-udev could learn to create stable path with help
> from src/udev/udev-builtin-path_id.c. I will work on it. However,
> I have virt-manager code to lookup GPU infos/path, using libdrm,
> and it is unlikely that it will work with the udev rules. So I'll
> have to patch libdrm to support that too.

The generic goal is that Libvirt should be providing enough information
for apps to be able to configure the guest, without resorting to side
channels like libdrm. This is to ensure that apps can manage guests
with no loss of functionality even when connected to a remote libvirt.
e.g. libdrm isn't going to be able to enumerate remote GPUs, for
virt-manager, so the neccessary info needs to be exposed by libvirt
via its virNodeDevice APIs. We can already identify the PCI device
and that its a GPU device, but I imagine we're not reporting any
data about DRI render paths associated with the GPUs we report.
So I think that's a gap we'd need to fill

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|

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

Re: [libvirt] [PATCH] docs: document bhyve e1000 support

[Roman Bogorodskiy]

  Erik Skultety wrote:

> On Sun, Feb 12, 2017 at 08:44:59PM +0400, Roman Bogorodskiy wrote:
> >   Roman Bogorodskiy wrote:
> > 
> > >  * Add bhyve e1000 support entry to release nodes
> >   ^
> > Oops, should be 'notes'.
> > 
> > >  * Update the bhyve driver page with usage sample
> > > ---
> > >  docs/drvbhyve.html.in | 15 +++
> > >  docs/news.xml |  9 +
> > >  2 files changed, 24 insertions(+)
> > 
> > Roman Bogorodskiy
> 
> ACK to the patch.

Pushed with the typo fixed, thanks!

Roman Bogorodskiy


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

Re: [libvirt] [PATCH] RFC: qemu: add spice/virgl rendernode

[Marc-André Lureau]

Hi

- Original Message -
> On Mon, Feb 13, 2017 at 07:19:04AM -0500, Marc-André Lureau wrote:
> > Hi
> > 
> > - Original Message -
> > > On Mon, Feb 13, 2017 at 03:51:48PM +0400, marcandre.lur...@redhat.com
> > > wrote:
> > > > From: Marc-André Lureau 
> > > > 
> > > > I am working on a WIP series to add QEMU Spice/virgl rendernode option.
> > > > Since rendernodes are not stable across reboots, I propose that QEMU
> > > > accepts also a PCI address (other bus types may be added in the
> > > > future).
> > > 
> > > Hmm, can you elaborate on this aspect ?  It feels like a parallel
> > > to saying NIC device names are not stable, so we should configure
> > > guests using PCI addresses instead of 'eth0', etc but we stuck with
> > > using NIC names in libvirt on the basis that you can create udev
> > > rules to ensure stable naming ?
> > > 
> > > So is there not a case to be made that if you want stable render
> > > device names when multiple NICs are present, then you should use
> > > udev to ensure a given device always maps to the same PCI dev.
> > 
> > I thought it was simpler to use a PCI address (do you expect users
> > to create udev rules for the GPUs?)
> 
> Well most users will only have 1 GPU so surely this won't be a problem
> in the common case.  Is it possible to get some stable naming rules into
> udev upstream though, so all distros get stable names by default

Optimus is getting more and more mainstream, see recent Fedora desktop effort 
(fwiw I have a t460p nouveau/i915). I don't think a random user of such 
hw/laptop should have to create udev rules.

I suppose systemd-udev could learn to create stable path with help from 
src/udev/udev-builtin-path_id.c. I will work on it. However, I have 
virt-manager code to lookup GPU infos/path, using libdrm, and it is unlikely 
that it will work with the udev rules. So I'll have to patch libdrm to support 
that too. 

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

Re: [libvirt] [PATCH] network: don't use dhcp-authoritative on static networks

[Martin Wilck]

Laine, Daniel,

On Thu, 2017-01-05 at 16:32 +0100, Martin Wilck wrote:
> On Sun, 2016-12-18 at 20:37 -0500, Laine Stump wrote:
> > > 
> > 
> > On 12/16/2016 11:58 AM, Martin Wilck wrote:
> > > "Static" DHCP networks are those where no dynamic DHCP range is
> > > defined, only a list of host entries is used to serve permanent
> > > IP addresses. On such networks, we don't want dnsmasq to reply
> > > to other requests than those statically defined. But
> > > "dhcp-authoritative" will cause dnsmasq to do just that.
> > > Therefore we can't use "dhcp-authoritative" for static networks.
> > 
> > Not surprising that this simple change would have unexpected 
> > consequences - that seems to be a basic law of the universe :-)
> > 
> > ACK to this, but it has me wondering 1) what is the range for which
> > it 
> > returns a positive response? Is it for anything within the IP 
> > address/netmask of the interface it's listening on? Or something
> > larger 
> > than that? (Does it just blindly ACK any request it gets?) and 2)
> > Do
> > we 
> > know for certain that the same thing doesn't happen when there is
> > also a 
> > dhcp range defined?
> > 
> > I'll wait for an answer to these before I push.
> > 
> 
> Here is a summary of how dnsmasq behaves:
> 
>  A) without "dhcp-authoritative", it only responds to DHCPREQUEST
> messages after a preceding DHCPDISCOVER/DHCPOFFER, or if a non-
> expired
> lease for the requesting MAC address is found. This holds also for
> clients with static host entries.
> 
>  B) with "dhcp-authoritative", dnsmasq always replies to DHCPREQUEST
> messages. It will send DHCPNAK if the lease is currently taken by a
> different MAC address (either via a static entry or via a non-expired
> lease), and DHCPACK otherwise.
> 
> Independent of dhcp-authoritative, dnsmasq responds to DHCPDISCOVER
> if
> and only if it has addresses to hand out, thus either the MAC address
> of the client matches a static entry or there are free addresses in
> the
> dynamic pool. I can see no difference between a "static"
> configuration
> and a configuration with zero-length or exhausted dynamic pool.
> 
> In case A), every attempt to re-obtain a previous IP address is
> delayed
> because the client needs to wait for several DHCPREQUEST messages to
> time out before it sends a new DHCPDISCOVER. In case B) it will
> receive
> either ACK or NAK immediately. This seems to be an advantage of
> "dhcp-
> authoritative".
> 
> So the answer to 1)/2) above is "positive response only in the
> defined
> dynamic dhcp range". If there's no dynamic range, no positive
> responses
> will be sent to unkown clients. *But* in authoritative mode it will
> send DHCPNAKs, which may confuse clients. If there's another DHCP
> server on the virtual network, properly configured (no overlap
> between
> dynamic ranges), and the client sends a DHCPREQUEST, it may get an
> DHCPNAK from the libvirt dnsmasq and DHCPACK from the other DHCP
> server. What then happens is probably dependent on timing (which
> response is received first). RFC2131 is not clear about this
> scenario,
> AFAICS.
> 
> Sooo, if customers run DHCP servers in VMs concurrently with the
> libvirt DHCP server, problems may arise with "dhcp-authoritative". My
> "don't use dhcp-authoritative on static networks" patch fixes this
> for
> cases where the libvirt network is "static", but problems may remain
> for cases with non-overlapping dynamic DHCP ranges. It's not an easy
> question whether or not this outweighs the benefits of using "dhcp-
> authoritative". Of course I'd hate to have my previous patch
> introducing "dhcp-authoritative" reverted :-)
> 
> Maybe we need yet another config option for the network XML. I'm
> unsure
> whether this should be an attribute of the ", , or
> 
> element, or something else. There is a relationship to the
> "localOnly"
> attribute of "", but I think the two should still be
> independent settings. Anyway, I'll wait what you're saying.

have you made up your mind about this?

Regards
Martin

-- 
Dr. Martin Wilck , Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

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

Re: [libvirt] [PATCH] RFC: qemu: add spice/virgl rendernode

[Daniel P. Berrange]

On Mon, Feb 13, 2017 at 07:19:04AM -0500, Marc-André Lureau wrote:
> Hi
> 
> - Original Message -
> > On Mon, Feb 13, 2017 at 03:51:48PM +0400, marcandre.lur...@redhat.com wrote:
> > > From: Marc-André Lureau 
> > > 
> > > I am working on a WIP series to add QEMU Spice/virgl rendernode option.
> > > Since rendernodes are not stable across reboots, I propose that QEMU
> > > accepts also a PCI address (other bus types may be added in the future).
> > 
> > Hmm, can you elaborate on this aspect ?  It feels like a parallel
> > to saying NIC device names are not stable, so we should configure
> > guests using PCI addresses instead of 'eth0', etc but we stuck with
> > using NIC names in libvirt on the basis that you can create udev
> > rules to ensure stable naming ?
> > 
> > So is there not a case to be made that if you want stable render
> > device names when multiple NICs are present, then you should use
> > udev to ensure a given device always maps to the same PCI dev.
> 
> I thought it was simpler to use a PCI address (do you expect users
> to create udev rules for the GPUs?)

Well most users will only have 1 GPU so surely this won't be a problem
in the common case.  Is it possible to get some stable naming rules into
udev upstream though, so all distros get stable names by default


Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|

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

Re: [libvirt] [PATCH] RFC: qemu: add spice/virgl rendernode

[Marc-André Lureau]

Hi

- Original Message -
> On Mon, Feb 13, 2017 at 03:51:48PM +0400, marcandre.lur...@redhat.com wrote:
> > From: Marc-André Lureau 
> > 
> > I am working on a WIP series to add QEMU Spice/virgl rendernode option.
> > Since rendernodes are not stable across reboots, I propose that QEMU
> > accepts also a PCI address (other bus types may be added in the future).
> 
> Hmm, can you elaborate on this aspect ?  It feels like a parallel
> to saying NIC device names are not stable, so we should configure
> guests using PCI addresses instead of 'eth0', etc but we stuck with
> using NIC names in libvirt on the basis that you can create udev
> rules to ensure stable naming ?
> 
> So is there not a case to be made that if you want stable render
> device names when multiple NICs are present, then you should use
> udev to ensure a given device always maps to the same PCI dev.

I thought it was simpler to use a PCI address (do you expect users to create 
udev rules for the GPUs?)

We could also allow both form (this is what I'll propose in qemu, fyi 
https://github.com/elmarco/qemu/commits/spice)

> > This is how I translated it to libvirt. I picked  over
> > , since it seems more generic. Comments welcome!
> 
> 
> Regards,
> Daniel
> --
> |: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
> |: http://libvirt.org  -o- http://virt-manager.org :|
> |: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|
> 

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

Re: [libvirt] [PATCH] RFC: qemu: add spice/virgl rendernode

[Daniel P. Berrange]

On Mon, Feb 13, 2017 at 03:51:48PM +0400, marcandre.lur...@redhat.com wrote:
> From: Marc-André Lureau 
> 
> I am working on a WIP series to add QEMU Spice/virgl rendernode option.
> Since rendernodes are not stable across reboots, I propose that QEMU
> accepts also a PCI address (other bus types may be added in the future).

Hmm, can you elaborate on this aspect ?  It feels like a parallel
to saying NIC device names are not stable, so we should configure
guests using PCI addresses instead of 'eth0', etc but we stuck with
using NIC names in libvirt on the basis that you can create udev
rules to ensure stable naming ?

So is there not a case to be made that if you want stable render
device names when multiple NICs are present, then you should use
udev to ensure a given device always maps to the same PCI dev.

> This is how I translated it to libvirt. I picked  over
> , since it seems more generic. Comments welcome!


Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|

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

[libvirt] [PATCH] RFC: qemu: add spice/virgl rendernode

[marcandre . lureau]

From: Marc-André Lureau 

I am working on a WIP series to add QEMU Spice/virgl rendernode option.
Since rendernodes are not stable across reboots, I propose that QEMU
accepts also a PCI address (other bus types may be added in the future).

This is how I translated it to libvirt. I picked  over
, since it seems more generic. Comments welcome!

Signed-off-by: Marc-André Lureau 
---
 docs/formatdomain.html.in  | 13 ++-
 docs/schemas/domaincommon.rng  |  5 +++
 src/conf/domain_conf.c | 45 --
 src/conf/domain_conf.h |  1 +
 src/qemu/qemu_capabilities.c   |  2 +
 src/qemu/qemu_capabilities.h   |  1 +
 src/qemu/qemu_command.c| 22 +++
 .../qemuxml2argv-video-virtio-gpu-spice-gl.args|  2 +-
 .../qemuxml2argv-video-virtio-gpu-spice-gl.xml |  6 ++-
 tests/qemuxml2argvtest.c   |  1 +
 .../qemuxml2xmlout-video-virtio-gpu-spice-gl.xml   |  6 ++-
 11 files changed, 97 insertions(+), 7 deletions(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 0a115f5dc..5c0f80b3c 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -5619,7 +5619,11 @@ qemu-kvm -net nic,model=? /dev/null
   clipboard copypaste='no'/
   mouse mode='client'/
   filetransfer enable='no'/
-  gl enable='yes'/
+  gl enable='yes'
+gpu
+  address type='pci' domain='0x' bus='0x06' slot='0x12' 
function='0x5'/
+/gpu
+  /gl
 /graphics
 
   Spice supports variable compression settings for audio, images 
and
@@ -5665,6 +5669,13 @@ qemu-kvm -net nic,model=? /dev/null
   the gl element, by setting the enable
   property. (QEMU only, since 1.3.3).
 
+
+  By default, QEMU will pick the first available GPU. You
+  may specify a host GPU to use for rendering with the
+  gpu element that supports a PCI
+  address child element. (QEMU only, since 3.1).
+
   
   rdp
   
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index d715bff29..cc85e07d8 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -3033,6 +3033,11 @@
 
   
 
+
+  
+
+  
+
 
   
 
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 1bc72a4e9..a18db6dd9 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1300,6 +1300,7 @@ void virDomainGraphicsDefFree(virDomainGraphicsDefPtr def)
 break;
 
 case VIR_DOMAIN_GRAPHICS_TYPE_SPICE:
+virDomainDeviceInfoClear(>data.spice.gpu);
 VIR_FREE(def->data.spice.keymap);
 virDomainGraphicsAuthDefClear(>data.spice.auth);
 break;
@@ -12159,6 +12160,13 @@ 
virDomainGraphicsDefParseXMLSpice(virDomainGraphicsDefPtr def,
 VIR_FREE(enable);
 
 def->data.spice.gl = enableVal;
+
+if (cur->children && cur->children->type == XML_ELEMENT_NODE &&
+xmlStrEqual(cur->children->name, BAD_CAST "gpu") &&
+virDomainDeviceInfoParseXML(cur->children, NULL,
+>data.spice.gpu, flags) < 
0)
+goto error;
+
 } else if (xmlStrEqual(cur->name, BAD_CAST "mouse")) {
 char *mode = virXMLPropString(cur, "mode");
 int modeVal;
@@ -22839,6 +22847,36 @@ virDomainGraphicsListenDefFormatAddr(virBufferPtr buf,
 virBufferAsprintf(buf, " listen='%s'", glisten->address);
 }
 
+static int
+virDomainSpiceGLDefFormat(virBufferPtr buf, virDomainGraphicsDefPtr def)
+{
+virBuffer childrenBuf = VIR_BUFFER_INITIALIZER;
+int indent = virBufferGetIndent(buf, false);
+
+if (def->data.spice.gl == VIR_TRISTATE_BOOL_ABSENT) {
+return 0;
+}
+
+virBufferAsprintf(buf, "data.spice.gl));
+
+virBufferAdjustIndent(, indent + 4);
+if (virDomainDeviceInfoFormat(, >data.spice.gpu, 0) < 0)
+return -1;
+if (virBufferUse()) {
+virBufferAddLit(buf, ">\n");
+virBufferAdjustIndent(buf, 2);
+virBufferAddLit(buf, "\n");
+virBufferAddBuffer(buf, );
+virBufferAddLit(buf, "\n");
+virBufferAdjustIndent(buf, -2);
+virBufferAddLit(buf, "\n");
+} else {
+virBufferAddLit(buf, "/>\n");
+}
+virBufferFreeAndReset();
+return 0;
+}
 
 static int
 virDomainGraphicsDefFormat(virBufferPtr buf,
@@ -23082,9 +23120,10 @@ virDomainGraphicsDefFormat(virBufferPtr buf,
 if (def->data.spice.filetransfer)
 virBufferAsprintf(buf, 

[libvirt] [PATCH RESEND] Interface: return appropriate status for bridge device

[Lin Ma]

In function udevInterfaceIsActive, The current design relies on the sys
attribute 'operstate' to determine whether the interface is active.

For the device node of virtual network(say virbr0), There is already a
dummy tap device to maintain a fixed mac on it, So it's available and
its status should be considered as active. But if no anyelse tap device
connects to it, The value of operstate of virbr0 in sysfs is 'down', So
udevInterfaceIsActive returns 0, It causes 'virsh iface-list --all' to
report the status of virbr0 as 'inactive'.

This patch fixes the issue by counting slave devices for bridge device.

Signed-off-by: Lin Ma 
---
 src/interface/interface_backend_udev.c | 22 ++
 1 file changed, 22 insertions(+)

diff --git a/src/interface/interface_backend_udev.c 
b/src/interface/interface_backend_udev.c
index 5d0fc64..9ac4674 100644
--- a/src/interface/interface_backend_udev.c
+++ b/src/interface/interface_backend_udev.c
@@ -1127,6 +1127,11 @@ udevInterfaceIsActive(virInterfacePtr ifinfo)
 struct udev_device *dev;
 virInterfaceDefPtr def = NULL;
 int status = -1;
+struct dirent **member_list = NULL;
+const char *devtype;
+int member_count = 0;
+char *member_path;
+size_t i;
 
 dev = udev_device_new_from_subsystem_sysname(udev, "net",
  ifinfo->name);
@@ -1146,6 +1151,23 @@ udevInterfaceIsActive(virInterfacePtr ifinfo)
 /* Check if it's active or not */
 status = STREQ(udev_device_get_sysattr_value(dev, "operstate"), "up");
 
+if (!status) {
+devtype = udev_device_get_devtype(dev);
+if (STREQ_NULLABLE(devtype, "bridge")) {
+if (virAsprintf(_path, "%s/%s",
+udev_device_get_syspath(dev), "brif") < 0)
+goto cleanup;
+member_count = scandir(member_path, _list,
+udevBridgeScanDirFilter, alphasort);
+if (member_count > 0)
+status = 1;
+for (i = 0; i < member_count; i++)
+VIR_FREE(member_list[i]);
+VIR_FREE(member_list);
+VIR_FREE(member_path);
+}
+}
+
 udev_device_unref(dev);
 
  cleanup:
-- 
2.9.2

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

Re: [libvirt] [PATCH] docs: document bhyve e1000 support

[Erik Skultety]

On Sun, Feb 12, 2017 at 08:44:59PM +0400, Roman Bogorodskiy wrote:
>   Roman Bogorodskiy wrote:
> 
> >  * Add bhyve e1000 support entry to release nodes
>   ^
> Oops, should be 'notes'.
> 
> >  * Update the bhyve driver page with usage sample
> > ---
> >  docs/drvbhyve.html.in | 15 +++
> >  docs/news.xml |  9 +
> >  2 files changed, 24 insertions(+)
> 
> Roman Bogorodskiy

ACK to the patch.

Erik

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

Re: [libvirt] [PATCH 1/9] perf: add cpu_clock software perf event support

[Nitesh Konkar]

On Fri, Feb 10, 2017 at 3:10 AM, John Ferlan  wrote:

>
>
> On 01/27/2017 06:01 AM, Nitesh Konkar wrote:
> > This patch adds support and documentation for
> > the cpu_clock perf event.
> >
> > Signed-off-by: Nitesh Konkar 
> > ---
> >  docs/formatdomain.html.in   |  6 ++
> >  docs/news.xml   |  4 ++--
> >  docs/schemas/domaincommon.rng   |  1 +
> >  include/libvirt/libvirt-domain.h| 10 ++
> >  src/libvirt-domain.c|  2 ++
> >  src/qemu/qemu_driver.c  |  1 +
> >  src/util/virperf.c  |  6 +-
> >  src/util/virperf.h  |  1 +
> >  tests/genericxml2xmlindata/generic-perf.xml |  1 +
> >  tools/virsh.pod |  3 +++
> >  10 files changed, 32 insertions(+), 3 deletions(-)
> >
> > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> > index 3f7f875..e44e758 100644
> > --- a/docs/formatdomain.html.in
> > +++ b/docs/formatdomain.html.in
> > @@ -1937,6 +1937,7 @@
> >event name='stalled_cycles_frontend' enabled='no'/
> >event name='stalled_cycles_backend' enabled='no'/
> >event name='ref_cpu_cycles' enabled='no'/
> > +  event name='cpu_clock' enabled='no'/
> >  /perf
> >  ...
> >  
> > @@ -2015,6 +2016,11 @@
> >   by applications running on the platform
> >perf.ref_cpu_cycles
> >  
> > +
> > +  cpu_clock
> > +  the count of cpu clock by applications running on the
> platform
> > +  perf.cpu_clock
> > +
> >
>
> "the count of cpu clock by applications..."
>
> doesn't convey enough meaning. Is that cycles? Time?  something else?
>
> The virsh.pod description seems to give the best hint...
>
> Some subsequent patches had longer descriptions in the virsh.pod, which
> got me thinking - where is the best place to have the longer
> description. I would think either in formatdomain or the
> libvirt-domain.{c|} description.  At least that way you don't need to
> mess with the 80 column formatting that is "nice" to have in virsh.pod
> output
>
Lets keep the virsh.pod and libvirt-domain.c explanantion terse and let
formatdomain have detailed explanation of the events. Do you agree?

>
>
> >
> >  Devices
> > diff --git a/docs/news.xml b/docs/news.xml
> > index ef7e2db..ec113ab 100644
> > --- a/docs/news.xml
> > +++ b/docs/news.xml
> > @@ -136,8 +136,8 @@
> >  
> >Add support to get the count of branch instructions
> >executed, branch misses, bus cycles, stalled frontend
> > -  cpu cycles, stalled backend cpu cycles, and ref cpu
> > -  cycles by applications running on the platform.
> > +  cpu cycles, stalled backend cpu cycles, ref cpu cycles
> > +  and cpu clock by applications running on the platform.
> >  
> >
> >
>
> So the latest decree is to not integrate news.xml into the patches with
> the code.  So just make one patch at the end that summarizes all the
> adjustments.
>
> Also this patch is wrong because it describes the additions in the 3.0.0
> section while this code would be available in 3.1.0.
>
Noted.

>
> > diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.
> rng
> > index 4d76315..86a39c8 100644
> > --- a/docs/schemas/domaincommon.rng
> > +++ b/docs/schemas/domaincommon.rng
> > @@ -433,6 +433,7 @@
> >stalled_cycles_frontend
> >stalled_cycles_backend
> >ref_cpu_cycles
> > +  cpu_clock
> >  
> >
> >
> > diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-
> domain.h
> > index e303140..bffe955 100644
> > --- a/include/libvirt/libvirt-domain.h
> > +++ b/include/libvirt/libvirt-domain.h
> > @@ -2188,6 +2188,16 @@ void 
> > virDomainStatsRecordListFree(virDomainStatsRecordPtr
> *stats);
> >   */
> >  # define VIR_PERF_PARAM_REF_CPU_CYCLES "ref_cpu_cycles"
> >
> > +/**
> > + * VIR_PERF_PARAM_CPU_CLOCK:
> > + *
> > + * Macro for typed parameter name that represents cpu_clock
> > + * perf event which can be used to measure the count of cpu
> > + * clock by applications running on the platform. It corresponds
> > + * to the "perf.cpu_clock" field in the *Stats APIs.
> > + */
> > +# define VIR_PERF_PARAM_CPU_CLOCK "cpu_clock"
> > +
> >  int virDomainGetPerfEvents(virDomainPtr dom,
> > virTypedParameterPtr *params,
> > int *nparams,
> > diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> > index 5b3e842..001cdd7 100644
> > --- a/src/libvirt-domain.c
> > +++ b/src/libvirt-domain.c
> > @@ -11250,6 +11250,8 @@ virConnectGetDomainCapabilities(virConnectPtr
> conn,
> >   * CPU frequency scaling by applications
> running
> >   * as unsigned long long. It is produced by
> the
> >