[libvirt] libvirtd not responding to virsh, results in virsh hanging

[Chris Friesen]

Hi,

We've recently run into an issue with libvirt 1.2.17 in the context of an 
OpenStack deployment.


Occasionally after doing live migrations from a compute node with libvirt 1.2.17 
to a compute node with libvirt 2.0.0 we see libvirtd on the 1.2.17 side stop 
responding.  When this happens, if you run a command like "sudo virsh list" then 
it just hangs waiting for a response from libvirtd.


Running "ps -elfT|grep libvirtd" shows many threads waiting on a futex, but two 
threads in poll_schedule_timeout() as part of the poll() syscall.  On a non-hung 
libvirtd I only see one thread in poll_schedule_timeout().


If I kill and restart libvirtd (this took two tries, it didn't actually die the 
first time) then the problem seems to go away.


I just tried attaching gdb to the "hung" libvirtd process and running "thread 
apply all backtrace".  This printed backtraces for the threads, including the 
one that was apparently stuck in poll():


Thread 17 (Thread 0x7f0573fff700 (LWP 186865)):
#0  0x7f05b59d769d in poll () from /lib64/libc.so.6
#1  0x7f05b7f01b9a in virNetClientIOEventLoop () from /lib64/libvirt.so.0
#2  0x7f05b7f0234b in virNetClientSendInternal () from /lib64/libvirt.so.0
#3  0x7f05b7f036f3 in virNetClientSendWithReply () from /lib64/libvirt.so.0
#4  0x7f05b7f04eb3 in virNetClientStreamSendPacket () from 
/lib64/libvirt.so.0
#5  0x7f05b7ed8db5 in remoteStreamFinish () from /lib64/libvirt.so.0
#6  0x7f05b7ec7eaa in virStreamFinish () from /lib64/libvirt.so.0
#7  0x7f059bd9323d in qemuMigrationIOFunc () from 
/usr/lib64/libvirt/connection-driver/libvirt_driver_qemu.so

#8  0x7f05b7e09aa2 in virThreadHelper () from /lib64/libvirt.so.0
#9  0x7f05b5cb4dc5 in start_thread () from /lib64/libpthread.so.0
#10 0x7f05b59e1ced in clone () from /lib64/libc.so.6


Interestingly, when I hit "c" to continue in the debugger, I got this:

(gdb) c
Continuing.

Program received signal SIGPIPE, Broken pipe.
[Switching to Thread 0x7f0573fff700 (LWP 186865)]
0x7f05b5cbb1cd in write () from /lib64/libpthread.so.0
(gdb) c
Continuing.
[Thread 0x7f0573fff700 (LWP 186865) exited]
(gdb) quit
A debugging session is active.

Inferior 1 [process 37471] will be detached.

Quit anyway? (y or n) y
Detaching from program: /usr/sbin/libvirtd, process 37471


Now thread 186865 seems to be gone, and libvirtd is no longer hung.

Has anyone seen anything like this before?  Anyone have an idea where to start 
looking?


Thanks,
Chris


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

Re: [libvirt] [PATCH resend V10 03/12] Resctrl: Add new xml element to support cache tune

[Martin Kletzander]

On Wed, Mar 15, 2017 at 05:48:09PM -0300, Marcelo Tosatti wrote:

On Wed, Mar 15, 2017 at 08:49:57PM +0100, Martin Kletzander wrote:

On Wed, Mar 15, 2017 at 02:11:23PM -0300, Marcelo Tosatti wrote:
>On Wed, Mar 15, 2017 at 03:59:31PM +0100, Martin Kletzander wrote:
>>On Wed, Mar 15, 2017 at 02:23:26PM +, Daniel P. Berrange wrote:
>>>On Wed, Mar 15, 2017 at 03:11:26PM +0100, Martin Kletzander wrote:
On Mon, Mar 06, 2017 at 06:06:32PM +0800, Eli Qiao wrote:
> This patch adds new xml element to support cache tune as:
>
> 
>  ...
>  >>>
Again, this was already discussed, probably, I just can't find the
source of it.  But host_id actually already selects what cache is
supposed to be used, so instead of type='l3' we only need scope='both'
(or data/instruction) there.  Or am I missing something?  What I'm
concerned about is that the host_id is mostly stable on one host (when
the previously mentioned problems are fixed), but it will make no sense
when the VM is migrated to another one.
>
>This is the same conditions as pinning a vcpu to a pcpu.
>
>So yes, it might be that you want to migrate to a host where
>a different "host ID" number is used (which might or might not
>be a different socket).
>
>>  Unfortunately, the only
solution I can think of is using multiple keys to precisely describe the
bank we want (e.g. host's cpu id, cache level and scope), but that seems
very unclean.
>>>
>>>I tend to view use of this cachetune setting as being similar to
>>>using host CPU passthrough - you're intentionally trading off
>>>migratability of your  guest to get a perf boost.
>>>
>>>Even without the host_id bit, this is still non-portable, as you
>>>might be requesting separate regions for code + data, but the
>>>target host of migration may only support shared regions.
>
>Then migration should fail.
>

Yes, it will automatically fail when you can't do the same allocations,
that's easy.  The difference with host_id is that you cannot foresee
whether keeping the host_id makes sense on the destination as well.


But on the destination, the user can use a different vcpu<->pcpu
configuration, right?

That is, you do not enforce that if the source has say vcpu0 <-> pcpu2,
the destination must have vcpu0 <-> pcpu2 (the configuration for that
part is different).



Yes, you can change it by supplying different destination XML to the
migration API.


So, the same thing can be applied to cache configuration.



Yes, it can.  However it is way easier to do when you select the
particular cache bank by socket/pcpu, level and scope, then just
host_id.


>>Sure, but those are things we can check during migration.  I'd be OK
>>with disabling migration (or making it --unsafe) when migrating with
>>cachetune.
>
>Migration support is required (for NFV usecase they want to migrate
>VMs around).
>
>

So we at least need a check for the fact that either the caches are the
somehow same or destination XML is supplied.

The other option would be what I hinted above, that is to change
host_id= to something like level, scope, and pcpu(s)/socket(s).


host_id maps to L3 socket via:

commit d57e3ab7e34c51a8badeea1b500bfb738d0af66e
Author: Fenghua Yu 
Date:   Sat Oct 22 06:19:50 2016 -0700

   x86/intel_cacheinfo: Enable cache id in cache info

   Cache id is retrieved from APIC ID and CPUID leaf 4 on x86.

   For more details please see the section on "Cache ID Extraction
   Parameters" in "Intel 64 Architecture Processor Topology
Enumeration".

   Also the documentation of the CPUID instruction in the "Intel 64 and
   IA-32 Architectures Software Developer's Manual"

So on the destination the user has to configure host_id= of the PCPU
which the guest is pinned to.



Oh, OK, I missed that it is actually taken from the host.  that's
because the code generates host_id numbers by using and incrementing
global variable, so it does not correspond to the one kernel knows
about.  Is cache/index*/id the one from APIC?


That is
fairly easy change from the code point of view, but I'm trying to think
more about the user experience.

Martin



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


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

Re: [libvirt] [PATCH resend V10 03/12] Resctrl: Add new xml element to support cache tune

[Marcelo Tosatti]

On Wed, Mar 15, 2017 at 08:49:57PM +0100, Martin Kletzander wrote:
> On Wed, Mar 15, 2017 at 02:11:23PM -0300, Marcelo Tosatti wrote:
> >On Wed, Mar 15, 2017 at 03:59:31PM +0100, Martin Kletzander wrote:
> >>On Wed, Mar 15, 2017 at 02:23:26PM +, Daniel P. Berrange wrote:
> >>>On Wed, Mar 15, 2017 at 03:11:26PM +0100, Martin Kletzander wrote:
> On Mon, Mar 06, 2017 at 06:06:32PM +0800, Eli Qiao wrote:
> > This patch adds new xml element to support cache tune as:
> >
> > 
> >  ...
> >   
> Again, this was already discussed, probably, I just can't find the
> source of it.  But host_id actually already selects what cache is
> supposed to be used, so instead of type='l3' we only need scope='both'
> (or data/instruction) there.  Or am I missing something?  What I'm
> concerned about is that the host_id is mostly stable on one host (when
> the previously mentioned problems are fixed), but it will make no sense
> when the VM is migrated to another one.
> >
> >This is the same conditions as pinning a vcpu to a pcpu.
> >
> >So yes, it might be that you want to migrate to a host where
> >a different "host ID" number is used (which might or might not
> >be a different socket).
> >
> >>  Unfortunately, the only
> solution I can think of is using multiple keys to precisely describe the
> bank we want (e.g. host's cpu id, cache level and scope), but that seems
> very unclean.
> >>>
> >>>I tend to view use of this cachetune setting as being similar to
> >>>using host CPU passthrough - you're intentionally trading off
> >>>migratability of your  guest to get a perf boost.
> >>>
> >>>Even without the host_id bit, this is still non-portable, as you
> >>>might be requesting separate regions for code + data, but the
> >>>target host of migration may only support shared regions.
> >
> >Then migration should fail.
> >
> 
> Yes, it will automatically fail when you can't do the same allocations,
> that's easy.  The difference with host_id is that you cannot foresee
> whether keeping the host_id makes sense on the destination as well.

But on the destination, the user can use a different vcpu<->pcpu
configuration, right? 

That is, you do not enforce that if the source has say vcpu0 <-> pcpu2, 
the destination must have vcpu0 <-> pcpu2 (the configuration for that 
part is different).

So, the same thing can be applied to cache configuration.

> >>Sure, but those are things we can check during migration.  I'd be OK
> >>with disabling migration (or making it --unsafe) when migrating with
> >>cachetune.
> >
> >Migration support is required (for NFV usecase they want to migrate
> >VMs around).
> >
> >
> 
> So we at least need a check for the fact that either the caches are the
> somehow same or destination XML is supplied.
> 
> The other option would be what I hinted above, that is to change
> host_id= to something like level, scope, and pcpu(s)/socket(s).  

host_id maps to L3 socket via: 

commit d57e3ab7e34c51a8badeea1b500bfb738d0af66e
Author: Fenghua Yu 
Date:   Sat Oct 22 06:19:50 2016 -0700

x86/intel_cacheinfo: Enable cache id in cache info

Cache id is retrieved from APIC ID and CPUID leaf 4 on x86.

For more details please see the section on "Cache ID Extraction
Parameters" in "Intel 64 Architecture Processor Topology
Enumeration".

Also the documentation of the CPUID instruction in the "Intel 64 and
IA-32 Architectures Software Developer's Manual"

So on the destination the user has to configure host_id= of the PCPU
which the guest is pinned to.

> That is
> fairly easy change from the code point of view, but I'm trying to think
> more about the user experience.
> 
> Martin


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

[libvirt] [PATCH v3 7/7] qemu: Set up the migration TLS objects for source

[John Ferlan]

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

If the migration flags indicate this migration will be using TLS,
then while we have connection in the Begin phase check and setup the
TLS environment that will be used by virMigrationRun during the Perform
phase for the source to configure TLS.

This creates at least an "-object tls-creds-x509,endpoint=client,..."
and potentially an "-object secret,..." to handle the passphrase response
to access the TLS credentials. The alias/id used for the TLS objects
will contain "libvirt_migrate" as a mechanism to signify that libvirt
started the migration on the source (reaping benefits possibly).

Once the objects are created, the code will set the "tls-creds" and
"tls-hostname" migration parameters to signify usage of TLS.

Since qemuProcessRecoverMigrationOut will cancel outgoing migrations
that are still in the QEMU_MIGRATION_PHASE_PERFORM{2|3} stages, there's
no need to do anything special as the Perform cleanup and Cancel phases
will reset the environment.

Signed-off-by: John Ferlan 
---
 src/qemu/qemu_migration.c | 53 +++
 1 file changed, 53 insertions(+)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 42074f0..5acae6e 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -3453,6 +3453,7 @@ qemuMigrationBegin(virConnectPtr conn,
unsigned long flags)
 {
 virQEMUDriverPtr driver = conn->privateData;
+virQEMUDriverConfigPtr cfg = NULL;
 char *xml = NULL;
 qemuDomainAsyncJob asyncJob;
 
@@ -3486,6 +3487,12 @@ qemuMigrationBegin(virConnectPtr conn,
 nmigrate_disks, migrate_disks, flags)))
 goto endjob;
 
+if (flags & VIR_MIGRATE_TLS) {
+cfg = virQEMUDriverGetConfig(driver);
+if (qemuMigrationCheckSetupTLS(conn, driver, cfg, vm, asyncJob) < 0)
+goto endjob;
+}
+
 if ((flags & VIR_MIGRATE_CHANGE_PROTECTION)) {
 /* We keep the job active across API calls until the confirm() call.
  * This prevents any other APIs being invoked while migration is taking
@@ -3502,6 +3509,7 @@ qemuMigrationBegin(virConnectPtr conn,
 }
 
  cleanup:
+virObjectUnref(cfg);
 virDomainObjEndAPI();
 return xml;
 
@@ -5010,8 +5018,11 @@ qemuMigrationRun(virQEMUDriverPtr driver,
 {
 int ret = -1;
 unsigned int migrate_flags = QEMU_MONITOR_MIGRATE_BACKGROUND;
+virQEMUDriverConfigPtr cfg = NULL;
 qemuDomainObjPrivatePtr priv = vm->privateData;
 qemuMigrationCookiePtr mig = NULL;
+char *tlsAlias = NULL;
+char *secAlias = NULL;
 qemuMigrationIOThreadPtr iothread = NULL;
 int fd = -1;
 unsigned long migrate_speed = resource ? resource : priv->migMaxBandwidth;
@@ -5075,6 +5086,38 @@ qemuMigrationRun(virQEMUDriverPtr driver,
 if (qemuDomainMigrateGraphicsRelocate(driver, vm, mig, graphicsuri) < 0)
 VIR_WARN("unable to provide data for graphics client relocation");
 
+if (flags & VIR_MIGRATE_TLS) {
+cfg = virQEMUDriverGetConfig(driver);
+
+/* Begin/CheckSetupTLS already set up migTLSAlias, the following
+ * assumes that and adds the TLS objects to the domain. */
+if (qemuMigrationAddTLSObjects(driver, vm, cfg, false,
+   QEMU_ASYNC_JOB_MIGRATION_OUT,
+   , , migParams) < 0)
+goto cleanup;
+
+/* We need to add the tls-hostname only for special circumstances,
+ * e.g. for a fd: or exec: based migration. As it turns out the
+ * CONNECT_HOST turns into an FD migration (see below). */
+if (spec->destType == MIGRATION_DEST_CONNECT_HOST ||
+spec->destType == MIGRATION_DEST_FD) {
+if (VIR_STRDUP(migParams->migrateTLSHostname,
+   spec->dest.host.name) < 0)
+goto cleanup;
+} else {
+/* Be sure there's nothing from a previous migration */
+if (VIR_STRDUP(migParams->migrateTLSHostname, "") < 0)
+goto cleanup;
+}
+} else {
+/* If we support setting the tls-creds, be sure to always reset
+ * the migration parameters when this migration isn't using TLS */
+if ((qemuMigrationCheckTLSCreds(driver, vm,
+QEMU_ASYNC_JOB_MIGRATION_OUT) < 0) ||
+(qemuMigrationSetEmptyTLSParams(priv, migParams) < 0))
+goto cleanup;
+}
+
 if (migrate_flags & (QEMU_MONITOR_MIGRATE_NON_SHARED_DISK |
  QEMU_MONITOR_MIGRATE_NON_SHARED_INC)) {
 if (mig->nbd) {
@@ -5255,6 +5298,14 @@ qemuMigrationRun(virQEMUDriverPtr driver,
 ret = -1;
 }
 
+if (qemuMigrationDeconstructTLS(driver, vm, QEMU_ASYNC_JOB_MIGRATION_OUT,
+tlsAlias, secAlias) < 0)
+ret = -1;
+
+

[libvirt] [PATCH v3 6/7] qemu: Set up the migrate TLS objects for target

[John Ferlan]

If the migration flags indicate this migration will be using TLS,
then set up the destination during the prepare phase once the target
domain has been started to add the TLS objects to perform the migration.

This will create at least an "-object tls-creds-x509,endpoint=server,..."
and potentially an "-object secret,..." to handle the passphrase response
to access the TLS credentials. The alias/id used for the TLS objects
will contain "libvirt_migrate" as a mechanism to signify that libvirt
started the migration on the target (reaping benefits possibly).

Once the objects are created, the code will set the "tls-creds" and
"tls-hostname" migration parameters to signify usage of TLS.

During the Finish phase we'll be sure to attempt to clear the
migration parameters and delete those objects (whether or not they
were created).

Since incoming migrations that don't reach the Finish stage will be
killed in qemuProcessRecoverMigrationIn and the only purpose at that
point would be to free memory, it's not necessary to set up any sort
of recovery. Additionally, subsequent migrations will check if the
migration parameters are set and adjust them appropriately if for
some reason libvirtd restarts after setting the Finish marker, but
before actually resetting the environment.

Signed-off-by: John Ferlan 
---
 src/qemu/qemu_domain.c|   7 +-
 src/qemu/qemu_domain.h|  91 +++-
 src/qemu/qemu_migration.c | 344 ++
 3 files changed, 403 insertions(+), 39 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index c239a06..f4636ed 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -782,7 +782,7 @@ qemuDomainSecretAESClear(qemuDomainSecretAES secret)
 }
 
 
-static void
+void
 qemuDomainSecretInfoFree(qemuDomainSecretInfoPtr *secinfo)
 {
 if (!*secinfo)
@@ -1186,7 +1186,7 @@ qemuDomainSecretInfoNew(virConnectPtr conn,
  *
  * Returns qemuDomainSecretInfoPtr or NULL on error.
  */
-static qemuDomainSecretInfoPtr
+qemuDomainSecretInfoPtr
 qemuDomainSecretInfoTLSNew(virConnectPtr conn,
qemuDomainObjPrivatePtr priv,
const char *srcAlias,
@@ -1677,6 +1677,9 @@ qemuDomainObjPrivateFree(void *data)
 
 VIR_FREE(priv->libDir);
 VIR_FREE(priv->channelTargetDir);
+
+qemuDomainSecretInfoFree(>migSecinfo);
+VIR_FREE(priv->migTLSAlias);
 qemuDomainMasterKeyFree(priv);
 
 VIR_FREE(priv);
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 1f266bf..1dd3b1c 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -175,6 +175,43 @@ VIR_ENUM_DECL(qemuDomainNamespace)
 bool qemuDomainNamespaceEnabled(virDomainObjPtr vm,
 qemuDomainNamespace ns);
 
+/* Type of domain secret */
+typedef enum {
+VIR_DOMAIN_SECRET_INFO_TYPE_PLAIN = 0,
+VIR_DOMAIN_SECRET_INFO_TYPE_AES,  /* utilize GNUTLS_CIPHER_AES_256_CBC */
+
+VIR_DOMAIN_SECRET_INFO_TYPE_LAST
+} qemuDomainSecretInfoType;
+
+typedef struct _qemuDomainSecretPlain qemuDomainSecretPlain;
+typedef struct _qemuDomainSecretPlain *qemuDomainSecretPlainPtr;
+struct _qemuDomainSecretPlain {
+char *username;
+uint8_t *secret;
+size_t secretlen;
+};
+
+# define QEMU_DOMAIN_AES_IV_LEN 16   /* 16 bytes for 128 bit random */
+ /*initialization vector */
+typedef struct _qemuDomainSecretAES qemuDomainSecretAES;
+typedef struct _qemuDomainSecretAES *qemuDomainSecretAESPtr;
+struct _qemuDomainSecretAES {
+char *username;
+char *alias;  /* generated alias for secret */
+char *iv; /* base64 encoded initialization vector */
+char *ciphertext; /* encoded/encrypted secret */
+};
+
+typedef struct _qemuDomainSecretInfo qemuDomainSecretInfo;
+typedef qemuDomainSecretInfo *qemuDomainSecretInfoPtr;
+struct _qemuDomainSecretInfo {
+qemuDomainSecretInfoType type;
+union {
+qemuDomainSecretPlain plain;
+qemuDomainSecretAES aes;
+} s;
+};
+
 typedef struct _qemuDomainObjPrivate qemuDomainObjPrivate;
 typedef qemuDomainObjPrivate *qemuDomainObjPrivatePtr;
 struct _qemuDomainObjPrivate {
@@ -246,47 +283,18 @@ struct _qemuDomainObjPrivate {
 
 /* note whether memory device alias does not correspond to slot number */
 bool memAliasOrderMismatch;
-};
 
-# define QEMU_DOMAIN_PRIVATE(vm)   \
-((qemuDomainObjPrivatePtr) (vm)->privateData)
+/* for migrations using TLS with a secret (not to be saved in our */
+/* private XML). */
+qemuDomainSecretInfoPtr migSecinfo;
 
-/* Type of domain secret */
-typedef enum {
-VIR_DOMAIN_SECRET_INFO_TYPE_PLAIN = 0,
-VIR_DOMAIN_SECRET_INFO_TYPE_AES,  /* utilize GNUTLS_CIPHER_AES_256_CBC */
-
-VIR_DOMAIN_SECRET_INFO_TYPE_LAST
-} qemuDomainSecretInfoType;
-
-typedef struct _qemuDomainSecretPlain qemuDomainSecretPlain;
-typedef struct _qemuDomainSecretPlain *qemuDomainSecretPlainPtr;
-struct 

[libvirt] [PATCH v3 1/7] qemu: Create #define for TLS configuration setup.

[John Ferlan]

Create GET_CONFIG_TLS_CERT to set up the TLS for 'chardev' TLS setting.
Soon to be reused.

Signed-off-by: John Ferlan 
---
 src/qemu/qemu_conf.c | 39 +--
 1 file changed, 25 insertions(+), 14 deletions(-)

diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 0a338d7..9db2bc3 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -530,22 +530,33 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr 
cfg,
 if (virConfGetValueBool(conf, "spice_auto_unix_socket", 
>spiceAutoUnixSocket) < 0)
 goto cleanup;
 
+#define GET_CONFIG_TLS_CERTINFO(val)\
+do {\
+if ((rv = virConfGetValueBool(conf, #val "_tls_x509_verify",\
+  >val## TLSx509verify)) < 0)  \
+goto cleanup;   \
+if (rv == 0)\
+cfg->val## TLSx509verify = cfg->defaultTLSx509verify;   \
+if (virConfGetValueString(conf, #val "_tls_x509_cert_dir",  \
+  >val## TLSx509certdir) < 0)  \
+goto cleanup;   \
+if (virConfGetValueString(conf, \
+  #val "_tls_x509_secret_uuid", \
+  >val## TLSx509secretUUID) < 0)   \
+goto cleanup;   \
+if (!cfg->val## TLSx509secretUUID &&\
+cfg->defaultTLSx509secretUUID) {\
+if (VIR_STRDUP(cfg->val## TLSx509secretUUID,\
+   cfg->defaultTLSx509secretUUID) < 0)  \
+goto cleanup;   \
+}   \
+} while (false);
+
 if (virConfGetValueBool(conf, "chardev_tls", >chardevTLS) < 0)
 goto cleanup;
-if (virConfGetValueString(conf, "chardev_tls_x509_cert_dir", 
>chardevTLSx509certdir) < 0)
-goto cleanup;
-if ((rv = virConfGetValueBool(conf, "chardev_tls_x509_verify", 
>chardevTLSx509verify)) < 0)
-goto cleanup;
-if (rv == 0)
-cfg->chardevTLSx509verify = cfg->defaultTLSx509verify;
-if (virConfGetValueString(conf, "chardev_tls_x509_secret_uuid",
-  >chardevTLSx509secretUUID) < 0)
-goto cleanup;
-if (!cfg->chardevTLSx509secretUUID && cfg->defaultTLSx509secretUUID) {
-if (VIR_STRDUP(cfg->chardevTLSx509secretUUID,
-   cfg->defaultTLSx509secretUUID) < 0)
-goto cleanup;
-}
+GET_CONFIG_TLS_CERTINFO(chardev);
+
+#undef GET_CONFIG_TLS_CERTINFO
 
 if (virConfGetValueUInt(conf, "remote_websocket_port_min", 
>webSocketPortMin) < 0)
 goto cleanup;
-- 
2.9.3

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

[libvirt] [PATCH v3 2/7] conf: Introduce migrate_tls_x509_cert_dir

[John Ferlan]

Add a new TLS X.509 certificate type - "migrate". This will handle the
creation of a TLS certificate capability (and possibly repository) to
be used for migrations. Similar to chardev's, credentials will be handled
via a libvirt secrets; however, unlike chardev's enablement and usage
will be via a CLI flag instead of a conf flag and a domain XML attribute.
The migrations will also require the client-cert.pem and client-key.pem
files to be present in the clients TLS directory.

Signed-off-by: John Ferlan 
---
 src/qemu/libvirtd_qemu.aug |  5 +
 src/qemu/qemu.conf | 37 +
 src/qemu/qemu_conf.c   |  6 ++
 src/qemu/qemu_conf.h   |  4 
 src/qemu/test_libvirtd_qemu.aug.in |  3 +++
 5 files changed, 55 insertions(+)

diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug
index 82bae9e..e1983d1 100644
--- a/src/qemu/libvirtd_qemu.aug
+++ b/src/qemu/libvirtd_qemu.aug
@@ -54,6 +54,10 @@ module Libvirtd_qemu =
  | bool_entry "chardev_tls_x509_verify"
  | str_entry "chardev_tls_x509_secret_uuid"
 
+   let migrate_entry = str_entry "migrate_tls_x509_cert_dir"
+ | bool_entry "migrate_tls_x509_verify"
+ | str_entry "migrate_tls_x509_secret_uuid"
+
let nogfx_entry = bool_entry "nographics_allow_host_audio"
 
let remote_display_entry = int_entry "remote_display_port_min"
@@ -116,6 +120,7 @@ module Libvirtd_qemu =
  | vnc_entry
  | spice_entry
  | chardev_entry
+ | migrate_entry
  | nogfx_entry
  | remote_display_entry
  | security_entry
diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
index 9925ac9..40bcec3 100644
--- a/src/qemu/qemu.conf
+++ b/src/qemu/qemu.conf
@@ -13,6 +13,11 @@
 #
 #  dh-params.pem - the DH params configuration file
 #
+# When using TLS for migrations, the directory must also contain
+#
+#  client-cert.pem - the client certificate signed with the ca-cert.pem
+#  client-key.pem - the client private key
+#
 #default_tls_x509_cert_dir = "/etc/pki/qemu"
 
 
@@ -238,6 +243,38 @@
 #chardev_tls_x509_secret_uuid = "----"
 
 
+# In order to override the default TLS certificate location for migration
+# certificates, supply a valid path to the certificate directory. If the
+# provided path does not exist then the default_tls_x509_cert_dir path
+# will be used. Once/if a default certificate is enabled/defined, migration
+# will then be able to use the certificate via migration API flags.
+#
+#migrate_tls_x509_cert_dir = "/etc/pki/libvirt-migrate"
+
+
+# The default TLS configuration only uses certificates for the server
+# allowing the client to verify the server's identity and establish
+# an encrypted channel.
+#
+# It is possible to use x509 certificates for authentication too, by
+# issuing a x509 certificate to every client who needs to connect.
+#
+# Enabling this option will reject any client who does not have a
+# certificate signed by the CA in /etc/pki/libvirt-migrate/ca-cert.pem
+#
+#migrate_tls_x509_verify = 1
+
+
+# Uncomment and use the following option to override the default secret
+# UUID provided in the default_tls_x509_secret_uuid parameter.
+#
+# NB This default all-zeros UUID will not work. Replace it with the
+# output from the UUID for the TLS secret from a 'virsh secret-list'
+# command and then uncomment the entry
+#
+#migrate_tls_x509_secret_uuid = "----"
+
+
 # By default, if no graphical front end is configured, libvirt will disable
 # QEMU audio output since directly talking to alsa/pulseaudio may not work
 # with various security settings. If you know what you're doing, enable
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 9db2bc3..4c271cd 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -280,6 +280,7 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool 
privileged)
 SET_TLS_X509_CERT_DEFAULT(vnc);
 SET_TLS_X509_CERT_DEFAULT(spice);
 SET_TLS_X509_CERT_DEFAULT(chardev);
+SET_TLS_X509_CERT_DEFAULT(migrate);
 
 #undef SET_TLS_X509_CERT_DEFAULT
 
@@ -395,6 +396,9 @@ static void virQEMUDriverConfigDispose(void *obj)
 VIR_FREE(cfg->chardevTLSx509certdir);
 VIR_FREE(cfg->chardevTLSx509secretUUID);
 
+VIR_FREE(cfg->migrateTLSx509certdir);
+VIR_FREE(cfg->migrateTLSx509secretUUID);
+
 while (cfg->nhugetlbfs) {
 cfg->nhugetlbfs--;
 VIR_FREE(cfg->hugetlbfs[cfg->nhugetlbfs].mnt_dir);
@@ -556,6 +560,8 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
 goto cleanup;
 GET_CONFIG_TLS_CERTINFO(chardev);
 
+GET_CONFIG_TLS_CERTINFO(migrate);
+
 #undef GET_CONFIG_TLS_CERTINFO
 
 if (virConfGetValueUInt(conf, "remote_websocket_port_min", 
>webSocketPortMin) < 0)
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index e585f81..1407eef 

[libvirt] [PATCH v3 0/7] Add TLS support for migration

[John Ferlan]

v2: http://www.redhat.com/archives/libvir-list/2017-February/msg01374.html

NB: Parts of the original series were split out, patches posted, ACK'd,
and committed separately, see:

http://www.redhat.com/archives/libvir-list/2017-March/msg00037.html

Changes since v2:

First and foremost after some gentle encouragement and lots of hours cursing
at no one in particular, I was finally able to configure a couple of nested
host guests, create a guest on each, and actually migrate that guest without
TLS back and forth between the two nested hosts. Suffice to say there are
dribs and drabs of "howto"'s that help, but it is quite a confusing setup
and I was certainly left wondering how customers do this. OK beyond paying
someone set things up for them!

Now that I had the basics, the next task was creating the TLS environment.
MANY more curse words uttered, including some I didn't know I knew, but I
was actually able to test the changes in my environment. Suffice to say it's
"somewhat confusing" because the TLS environment that QEMU expects (ok, the
filenames) differ slightly than those that a libvirt TLS environment would
expects. What really got frustrating is that all I'd get back is the 
following error message after 'migrate' started on the client and after
the server was essentially waiting for the migration:

error: operation failed: migration job: unexpectedly failed

Turns out that I was missing the '/etc/pki/qemu/client-cert.pem' and
'/etc/pki/qemu/client-key.pem' files, but got no message. Of course I
did have something in my /etc/pki/libvirt directory, but of course those
weren't being used. Still my brain wasn't exactly telling me that. The
other piece I was missing was the "details" about when "tls-hostname"
must be provided. In the previous patches, I would search on "fd" or
"spec" in spec->dest.host.protocol; however, that wasn't quite right.
What I needed to search on was MIGRATION_DEST_CONNECT_HOST or
MIGRATION_DEST_FD in spec->destType (and _CONNECT_HOST only because it
turns into _FD 'eventually').

Anyway, so happy days are here again and I can now liberally celebrate
St Patrick's Day (not that I wasn't going to before, but now moreso!).

Beyond the above description from my testing the real "key" behind all
of this is ensuring that the 'right' QEMU bits are installed by fetching
the "tls-creds" via qemuMonitorGetMigrationParams and as long as *something*
is returned, then we are assured that we can proceed. This relates to QEMU
commit '4af245dc3' (after v2.9.0-rc0) which sets a default for tls-creds
and more importantly for our purposes allows us to "clear" the tls-creds
and tls-hostname migration parameters by passing a "" (empty string).

With that in place the rest of the logic falls into place with regard
to qemuMigrationCheckSetupTLS ensuring that TLS for migration is available
for libvirt to use.

Once we know we can use TLS, we'll use qemuMigrationAddTLSObjects to
add the "secret" and "tls-creds-x509" objects to the domain for either
server (target) or client (source). Doing this also required an adjustment
to the existing code to allow the Async monitor usage.

Once those objects are created, we'll set the "tls-creds" and "tls-hostname"
parameters appropriate. If TLS isn't being used, we'll be sure to clear
them. This is only made tricky by needing to ensure the qemu that's running
can support setting them to "", hence the qemuMigrationCheckTLSCreds prior
to that clearing. If this wasn't done, older QEMU would be less than pleased
to find "" in tls-creds and tls-hostname.

Finally when the migration is successful, call qemuMigrationDeconstructTLS
(naming credit goes to my wife!). Failures will call just qemuMigrationResetTLS
in order to achieve the same call; however, for those paths we do not still
have the original 'tlsAlias' and 'secAlias' available, so we have to recreate
it on the fly'

Have I won the prize for the longest cover letter yet?



John Ferlan (7):
  qemu: Create #define for TLS configuration setup.
  conf: Introduce migrate_tls_x509_cert_dir
  Add new migration flag VIR_MIGRATE_TLS
  qemu: Add TLS params to _qemuMonitorMigrationParams
  qemu: Add job for qemuDomain{Add|Del}TLSObjects
  qemu: Set up the migrate TLS objects for target
  qemu: Set up the migration TLS objects for source

 include/libvirt/libvirt-domain.h   |   8 +
 src/qemu/libvirtd_qemu.aug |   5 +
 src/qemu/qemu.conf |  37 
 src/qemu/qemu_conf.c   |  45 ++--
 src/qemu/qemu_conf.h   |   4 +
 src/qemu/qemu_domain.c |   7 +-
 src/qemu/qemu_domain.h |  91 
 src/qemu/qemu_driver.c |   4 +-
 src/qemu/qemu_hotplug.c|  24 ++-
 src/qemu/qemu_hotplug.h|   2 +
 src/qemu/qemu_migration.c  | 423 -
 src/qemu/qemu_migration.h  |   9 +-
 src/qemu/qemu_monitor.c|  11 +-
 src/qemu/qemu_monitor.h|   3 +
 

[libvirt] [PATCH v3 5/7] qemu: Add job for qemuDomain{Add|Del}TLSObjects

[John Ferlan]

Add an asyncJob argument for add/delete TLS Objects. A future patch will
add/delete TLS objects from a migration which may hae a job to join.

Signed-off-by: John Ferlan 
---
 src/qemu/qemu_hotplug.c | 24 
 src/qemu/qemu_hotplug.h |  2 ++
 2 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index ddcbc5e..9adb04a 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1531,6 +1531,7 @@ qemuDomainAttachHostPCIDevice(virQEMUDriverPtr driver,
 void
 qemuDomainDelTLSObjects(virQEMUDriverPtr driver,
 virDomainObjPtr vm,
+qemuDomainAsyncJob asyncJob,
 const char *secAlias,
 const char *tlsAlias)
 {
@@ -1542,7 +1543,8 @@ qemuDomainDelTLSObjects(virQEMUDriverPtr driver,
 
 orig_err = virSaveLastError();
 
-qemuDomainObjEnterMonitor(driver, vm);
+if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
+goto cleanup;
 
 if (tlsAlias)
 ignore_value(qemuMonitorDelObject(priv->mon, tlsAlias));
@@ -1552,6 +1554,7 @@ qemuDomainDelTLSObjects(virQEMUDriverPtr driver,
 
 ignore_value(qemuDomainObjExitMonitor(driver, vm));
 
+ cleanup:
 if (orig_err) {
 virSetError(orig_err);
 virFreeError(orig_err);
@@ -1562,6 +1565,7 @@ qemuDomainDelTLSObjects(virQEMUDriverPtr driver,
 int
 qemuDomainAddTLSObjects(virQEMUDriverPtr driver,
 virDomainObjPtr vm,
+qemuDomainAsyncJob asyncJob,
 const char *secAlias,
 virJSONValuePtr *secProps,
 const char *tlsAlias,
@@ -1574,7 +1578,8 @@ qemuDomainAddTLSObjects(virQEMUDriverPtr driver,
 if (!tlsAlias && !secAlias)
 return 0;
 
-qemuDomainObjEnterMonitor(driver, vm);
+if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
+return -1;
 
 if (secAlias) {
 rc = qemuMonitorAddObject(priv->mon, "secret",
@@ -1601,7 +1606,7 @@ qemuDomainAddTLSObjects(virQEMUDriverPtr driver,
 virSetError(orig_err);
 virFreeError(orig_err);
 }
-qemuDomainDelTLSObjects(driver, vm, secAlias, tlsAlias);
+qemuDomainDelTLSObjects(driver, vm, asyncJob, secAlias, tlsAlias);
 
 return -1;
 }
@@ -1682,8 +1687,8 @@ qemuDomainAddChardevTLSObjects(virConnectPtr conn,
 goto cleanup;
 dev->data.tcp.tlscreds = true;
 
-if (qemuDomainAddTLSObjects(driver, vm, *secAlias, ,
-*tlsAlias, ) < 0)
+if (qemuDomainAddTLSObjects(driver, vm, QEMU_ASYNC_JOB_NONE,
+*secAlias, , *tlsAlias, ) < 
0)
 goto cleanup;
 
 ret = 0;
@@ -1773,7 +1778,8 @@ int qemuDomainAttachRedirdevDevice(virConnectPtr conn,
 virSetError(orig_err);
 virFreeError(orig_err);
 }
-qemuDomainDelTLSObjects(driver, vm, secAlias, tlsAlias);
+qemuDomainDelTLSObjects(driver, vm, QEMU_ASYNC_JOB_NONE,
+secAlias, tlsAlias);
 goto audit;
 }
 
@@ -2034,7 +2040,8 @@ int qemuDomainAttachChrDevice(virConnectPtr conn,
 virFreeError(orig_err);
 }
 
-qemuDomainDelTLSObjects(driver, vm, secAlias, tlsAlias);
+qemuDomainDelTLSObjects(driver, vm, QEMU_ASYNC_JOB_NONE,
+secAlias, tlsAlias);
 goto audit;
 }
 
@@ -2186,7 +2193,8 @@ qemuDomainAttachRNGDevice(virConnectPtr conn,
 virFreeError(orig_err);
 }
 
-qemuDomainDelTLSObjects(driver, vm, secAlias, tlsAlias);
+qemuDomainDelTLSObjects(driver, vm, QEMU_ASYNC_JOB_NONE,
+secAlias, tlsAlias);
 goto audit;
 }
 
diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h
index 73f2b1f..f06f232 100644
--- a/src/qemu/qemu_hotplug.h
+++ b/src/qemu/qemu_hotplug.h
@@ -36,11 +36,13 @@ int qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
 
 void qemuDomainDelTLSObjects(virQEMUDriverPtr driver,
  virDomainObjPtr vm,
+ qemuDomainAsyncJob asyncJob,
  const char *secAlias,
  const char *tlsAlias);
 
 int qemuDomainAddTLSObjects(virQEMUDriverPtr driver,
 virDomainObjPtr vm,
+qemuDomainAsyncJob asyncJob,
 const char *secAlias,
 virJSONValuePtr *secProps,
 const char *tlsAlias,
-- 
2.9.3

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

[libvirt] [PATCH v3 4/7] qemu: Add TLS params to _qemuMonitorMigrationParams

[John Ferlan]

Add the fields to support setting tls-creds and tls-hostname during
a migration (either source or target). Modify the query migration
function to check for the presence and set the field for future
consumers to determine which of 3 conditions is being met (not
present, present and set to "", or present and sent to something).

Modify code paths that either allocate or use stack space in order
to call qemuMigrationParamsClear or qemuMigrationParamsFree for cleanup.

Signed-off-by: John Ferlan 
---
 src/qemu/qemu_driver.c   |  4 +++-
 src/qemu/qemu_migration.c| 26 +-
 src/qemu/qemu_migration.h|  6 ++
 src/qemu/qemu_monitor.c  | 11 ---
 src/qemu/qemu_monitor.h  |  3 +++
 src/qemu/qemu_monitor_json.c | 28 
 tests/qemumonitorjsontest.c  | 25 -
 7 files changed, 97 insertions(+), 6 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index dcd823f..03e3f8d 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -11845,6 +11845,7 @@ qemuDomainMigratePerform(virDomainPtr dom,
flags, dname, resource, false);
 
  cleanup:
+qemuMigrationParamsClear();
 VIR_FREE(compression);
 return ret;
 }
@@ -12253,6 +12254,7 @@ qemuDomainMigratePerform3(virDomainPtr dom,
flags, dname, resource, true);
 
  cleanup:
+qemuMigrationParamsClear();
 VIR_FREE(compression);
 return ret;
 }
@@ -12343,7 +12345,7 @@ qemuDomainMigratePerform3Params(virDomainPtr dom,
flags, dname, bandwidth, true);
  cleanup:
 VIR_FREE(compression);
-VIR_FREE(migParams);
+qemuMigrationParamsFree();
 VIR_FREE(migrate_disks);
 return ret;
 }
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index f5711bc..66a5062 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -3508,6 +3508,28 @@ qemuMigrationSetCompression(virQEMUDriverPtr driver,
 }
 
 
+void
+qemuMigrationParamsClear(qemuMonitorMigrationParamsPtr migParams)
+{
+if (!migParams)
+return;
+
+VIR_FREE(migParams->migrateTLSAlias);
+VIR_FREE(migParams->migrateTLSHostname);
+}
+
+
+void
+qemuMigrationParamsFree(qemuMonitorMigrationParamsPtr *migParams)
+{
+if (!*migParams)
+return;
+
+qemuMigrationParamsClear(*migParams);
+VIR_FREE(*migParams);
+}
+
+
 qemuMonitorMigrationParamsPtr
 qemuMigrationParams(virTypedParameterPtr params,
 int nparams,
@@ -3549,7 +3571,7 @@ qemuMigrationParams(virTypedParameterPtr params,
 return migParams;
 
  error:
-VIR_FREE(migParams);
+qemuMigrationParamsFree();
 return NULL;
 }
 
@@ -3909,6 +3931,7 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver,
 virDomainObjRemoveTransientDef(vm);
 qemuDomainRemoveInactive(driver, vm);
 }
+qemuMigrationParamsClear();
 virDomainObjEndAPI();
 qemuDomainEventQueue(driver, event);
 qemuMigrationCookieFree(mig);
@@ -5244,6 +5267,7 @@ static int doPeer2PeerMigrate2(virQEMUDriverPtr driver,
 virSetError(orig_err);
 virFreeError(orig_err);
 }
+qemuMigrationParamsClear();
 VIR_FREE(uri_out);
 VIR_FREE(cookie);
 VIR_FREE(compression);
diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h
index bcebf06..4c8f2c9 100644
--- a/src/qemu/qemu_migration.h
+++ b/src/qemu/qemu_migration.h
@@ -121,6 +121,12 @@ int 
qemuMigrationCompressionDump(qemuMigrationCompressionPtr compression,
  int *maxparams,
  unsigned long *flags);
 
+void
+qemuMigrationParamsClear(qemuMonitorMigrationParamsPtr migParams);
+
+void
+qemuMigrationParamsFree(qemuMonitorMigrationParamsPtr *migParams);
+
 qemuMonitorMigrationParamsPtr
 qemuMigrationParams(virTypedParameterPtr params,
 int nparams,
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 79da472..ee0e116 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -2530,12 +2530,15 @@ qemuMonitorSetMigrationParams(qemuMonitorPtr mon,
 {
 VIR_DEBUG("compressLevel=%d:%d compressThreads=%d:%d "
   "decompressThreads=%d:%d cpuThrottleInitial=%d:%d "
-  "cpuThrottleIncrement=%d:%d",
+  "cpuThrottleIncrement=%d:%d tlsAlias=%s "
+  "tlsHostname=%s",
   params->compressLevel_set, params->compressLevel,
   params->compressThreads_set, params->compressThreads,
   params->decompressThreads_set, params->decompressThreads,
   params->cpuThrottleInitial_set, params->cpuThrottleInitial,
-  params->cpuThrottleIncrement_set, params->cpuThrottleIncrement);
+  params->cpuThrottleIncrement_set, params->cpuThrottleIncrement,
+  NULLSTR(params->migrateTLSAlias),
+  NULLSTR(params->migrateTLSHostname));
 

[libvirt] [PATCH v3 3/7] Add new migration flag VIR_MIGRATE_TLS

[John Ferlan]

Signed-off-by: John Ferlan 
---
 include/libvirt/libvirt-domain.h | 8 
 src/qemu/qemu_migration.h| 3 ++-
 tools/virsh-domain.c | 7 +++
 3 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
index c490d71..620606c 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -815,6 +815,14 @@ typedef enum {
  * post-copy mode. See virDomainMigrateStartPostCopy for more details.
  */
 VIR_MIGRATE_POSTCOPY  = (1 << 15),
+
+/* Setting the VIR_MIGRATE_TLS flag will cause the migration to attempt
+ * to use the TLS environment configured by the hypervisor in order to
+ * perform the migration. If incorrectly configured on either source or
+ * destination, the migration will fail.
+ */
+VIR_MIGRATE_TLS   = (1 << 16),
+
 } virDomainMigrateFlags;
 
 
diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h
index 14c6178..bcebf06 100644
--- a/src/qemu/qemu_migration.h
+++ b/src/qemu/qemu_migration.h
@@ -45,7 +45,8 @@ typedef qemuMigrationCompression *qemuMigrationCompressionPtr;
  VIR_MIGRATE_ABORT_ON_ERROR |   \
  VIR_MIGRATE_AUTO_CONVERGE |\
  VIR_MIGRATE_RDMA_PIN_ALL | \
- VIR_MIGRATE_POSTCOPY)
+ VIR_MIGRATE_POSTCOPY | \
+ VIR_MIGRATE_TLS)
 
 /* All supported migration parameters and their types. */
 # define QEMU_MIGRATION_PARAMETERS\
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 09a9f82..ebd4b33 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -10222,6 +10222,10 @@ static const vshCmdOptDef opts_migrate[] = {
  .type = VSH_OT_STRING,
  .help = N_("filename containing updated persistent XML for the target")
 },
+{.name = "tls",
+ .type = VSH_OT_BOOL,
+ .help = N_("use TLS for migration")
+},
 {.name = NULL}
 };
 
@@ -10463,6 +10467,9 @@ doMigrate(void *opaque)
 if (vshCommandOptBool(cmd, "postcopy"))
 flags |= VIR_MIGRATE_POSTCOPY;
 
+if (vshCommandOptBool(cmd, "tls"))
+flags |= VIR_MIGRATE_TLS;
+
 if (flags & VIR_MIGRATE_PEER2PEER || vshCommandOptBool(cmd, "direct")) {
 if (virDomainMigrateToURI3(dom, desturi, params, nparams, flags) == 0)
 ret = '0';
-- 
2.9.3

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

Re: [libvirt] [PATCH] network: don't add "no-resolv" if we still need DNS servers from resolv.conf

[Laine Stump]

On 03/17/2017 12:58 PM, Jiri Denemark wrote:
> On Fri, Mar 17, 2017 at 12:33:14 -0400, Laine Stump wrote:
>> It was pointed out here:
>>
>>   https://bugzilla.redhat.com/show_bug.cgi?id=1331796#c4
>>
>> that we shouldn't be adding a "no-resolv" to the dnsmasq.conf file for
>> a network if there isn't any  element that specifies an IP
>> address but no qualifying domain. If there is such an element, it will
>> handle all DNS requests that weren't otherwise handled by one of the
>> forwarder entries with a matching domain attribute. If not, then DNS
>> requests that don't match the domain of any  would not be
>> resolved if we added no-resolv.
>>
>> So, only add "no-resolv" when there is at least one 
>> element that specifies an IP address but no qualifying domain.
> ...
>> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
>> index c5ec282..32c5ab7 100644
>> --- a/src/network/bridge_driver.c
>> +++ b/src/network/bridge_driver.c
>> @@ -1085,7 +1085,15 @@ networkDnsmasqConfContents(virNetworkObjPtr network,
>>  virBufferAddLit(, "port=0\n");
>>  
>>  if (wantDNS && network->def->dns.forwarders) {
>> -virBufferAddLit(, "no-resolv\n");
>> +/* addNoResolv should be set to true if there are any entries
>> + * that specify an IP address for requests, but no domain
>> + * qualifier (implying that all requests otherwise "unclaimed"
>> + * should be sent to that address). if it is still false when
>> + * we've looked at all entries, it means we still need the
>> + * host's resolv.conf for some cases.
>> + */
>> +bool addNoResolv = false;
>> +
>>  for (i = 0; i < network->def->dns.nfwds; i++) {
>>  virNetworkDNSForwarderPtr fwd = 
>> >def->dns.forwarders[i];
>>  
>> @@ -1099,11 +1107,15 @@ networkDnsmasqConfContents(virNetworkObjPtr network,
>>  goto cleanup;
>>  virBufferAsprintf(, "%s\n", addr);
>>  VIR_FREE(addr);
>> +if (!fwd->domain)
>> +addNoResolv = true;
>>  } else {
>>  /* "don't forward requests for this domain" */
>>  virBufferAddLit(, "#\n");
>>  }
>>  }
>> +if (addNoResolv)
>> +virBufferAddLit(, "no-resolv\n");
>>  }
>>  
>>  if (network->def->domain) {
> 
> So what if the network is isolated and supposed to only resolve names
> according to its database. Such network does not have any 
> element and yet no-resolve should be added in the configuration.

You forced me to remember that I had fixed exactly that hole a *long
time* ago (far before  was added). I looked it up and found
commit 513122ae:

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

which adds no-resolv if the network is isolated. I was momentarily
afraid that the no-resolv added in that patch had been "messed with" at
some later time, causing a regression in my fix, but found that it's
still there (look around line 1216).

So in the case of an isolated network, we still add no-resolv, no matter
whether we've added it due to  or not.

But before we give up on this train of thought, is there maybe *some
other* situation I haven't considered?

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

Re: [libvirt] [PATCH] network: don't add "no-resolv" if we still need DNS servers from resolv.conf

[Jiri Denemark]

On Fri, Mar 17, 2017 at 12:33:14 -0400, Laine Stump wrote:
> It was pointed out here:
> 
>   https://bugzilla.redhat.com/show_bug.cgi?id=1331796#c4
> 
> that we shouldn't be adding a "no-resolv" to the dnsmasq.conf file for
> a network if there isn't any  element that specifies an IP
> address but no qualifying domain. If there is such an element, it will
> handle all DNS requests that weren't otherwise handled by one of the
> forwarder entries with a matching domain attribute. If not, then DNS
> requests that don't match the domain of any  would not be
> resolved if we added no-resolv.
> 
> So, only add "no-resolv" when there is at least one 
> element that specifies an IP address but no qualifying domain.
...
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index c5ec282..32c5ab7 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -1085,7 +1085,15 @@ networkDnsmasqConfContents(virNetworkObjPtr network,
>  virBufferAddLit(, "port=0\n");
>  
>  if (wantDNS && network->def->dns.forwarders) {
> -virBufferAddLit(, "no-resolv\n");
> +/* addNoResolv should be set to true if there are any entries
> + * that specify an IP address for requests, but no domain
> + * qualifier (implying that all requests otherwise "unclaimed"
> + * should be sent to that address). if it is still false when
> + * we've looked at all entries, it means we still need the
> + * host's resolv.conf for some cases.
> + */
> +bool addNoResolv = false;
> +
>  for (i = 0; i < network->def->dns.nfwds; i++) {
>  virNetworkDNSForwarderPtr fwd = >def->dns.forwarders[i];
>  
> @@ -1099,11 +1107,15 @@ networkDnsmasqConfContents(virNetworkObjPtr network,
>  goto cleanup;
>  virBufferAsprintf(, "%s\n", addr);
>  VIR_FREE(addr);
> +if (!fwd->domain)
> +addNoResolv = true;
>  } else {
>  /* "don't forward requests for this domain" */
>  virBufferAddLit(, "#\n");
>  }
>  }
> +if (addNoResolv)
> +virBufferAddLit(, "no-resolv\n");
>  }
>  
>  if (network->def->domain) {

So what if the network is isolated and supposed to only resolve names
according to its database. Such network does not have any 
element and yet no-resolve should be added in the configuration.

Jirka

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

[libvirt] [PATCH 09/14] cputest: Add cpuidLeaf helper to cpu-cpuid.py

[Jiri Denemark]

Signed-off-by: Jiri Denemark 
---
 tests/cputestdata/cpu-cpuid.py | 21 +
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/tests/cputestdata/cpu-cpuid.py b/tests/cputestdata/cpu-cpuid.py
index 9ea858d98..f4cf6d440 100755
--- a/tests/cputestdata/cpu-cpuid.py
+++ b/tests/cputestdata/cpu-cpuid.py
@@ -189,6 +189,18 @@ def cpuidIsSet(cpuid, feature):
 (edx > 0 and leaf["edx"] & edx > 0))
 
 
+def cpuidLeaf(cpuid, in_eax, in_ecx):
+if in_eax not in cpuid:
+cpuid[in_eax] = {}
+leaf = cpuid[in_eax]
+
+if in_ecx not in leaf:
+leaf[in_ecx] = {"eax": 0, "ebx": 0, "ecx": 0, "edx": 0}
+leaf = leaf[in_ecx]
+
+return leaf
+
+
 def parseFeatureWords(path):
 features = None
 
@@ -222,14 +234,7 @@ def parseFeatureWords(path):
 if "cpuid-input-ecx" in feat:
 in_ecx = feat["cpuid-input-ecx"]
 
-if in_eax not in cpuid:
-cpuid[in_eax] = {}
-leaf = cpuid[in_eax]
-
-if in_ecx not in leaf:
-leaf[in_ecx] = {"eax": 0, "ebx": 0, "ecx": 0, "edx": 0}
-leaf = leaf[in_ecx]
-
+leaf = cpuidLeaf(cpuid, in_eax, in_ecx)
 leaf[feat["cpuid-register"].lower()] = feat["features"]
 
 return props, cpuid
-- 
2.12.0

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

[libvirt] [PATCH 00/14] Add tests for virCPUUpdateLive API

[Jiri Denemark]

Jiri Denemark (14):
  cpu_conf: Introduce virCPUDefFreeFeatures
  cpu: Introduce virCPUExpandFeatures
  cpu: Drop unused flags from cpuArchDecode
  cpu: Move feature expansion out of cpuBaseline
  cpu: Do not pass virConnectBaselineCPUFlags to cpuBaseline
  cputest: Move instantiation of JSONDecoder in cpu-convert.py
  cputest: Rename cpu-convert.py script as cpu-cpuid.py
  cputest: Add cpuidIsSet helper to cpu-cpuid.py
  cputest: Add cpuidLeaf helper to cpu-cpuid.py
  cputest: Add "diff" command to cpu-cpuid.py
  cputest: Generate data for virCPUUpdateLive
  cputest: Disable TSX on broken models
  cputest: Disable "cmt" feature unknown to QEMU
  cputest: Add tests for virCPUUpdateLive API

 src/conf/cpu_conf.c|  21 ++-
 src/conf/cpu_conf.h|   3 +
 src/cpu/cpu.c  |  74 +++--
 src/cpu/cpu.h  |  15 +-
 src/cpu/cpu_arm.c  |   5 +-
 src/cpu/cpu_ppc64.c|  12 +-
 src/cpu/cpu_x86.c  | 109 --
 src/libvirt_private.syms   |   2 +
 tests/cputest.c| 160 +++-
 tests/cputestdata/{cpu-convert.py => cpu-cpuid.py} | 165 +
 tests/cputestdata/cpu-parse.sh |   3 +-
 tests/cputestdata/x86_64-baseline-3-expanded.xml   |  48 +++---
 tests/cputestdata/x86_64-baseline-4-expanded.xml   |  68 -
 tests/cputestdata/x86_64-baseline-5-expanded.xml   |  70 -
 .../x86_64-cpuid-A10-5800K-disabled.xml|   7 +
 .../cputestdata/x86_64-cpuid-A10-5800K-enabled.xml |   8 +
 .../x86_64-cpuid-Core-i5-2500-disabled.xml |   5 +
 .../x86_64-cpuid-Core-i5-2500-enabled.xml  |   9 ++
 .../x86_64-cpuid-Core-i5-2540M-disabled.xml|   5 +
 .../x86_64-cpuid-Core-i5-2540M-enabled.xml |   9 ++
 .../x86_64-cpuid-Core-i5-4670T-disabled.xml|   7 +
 .../x86_64-cpuid-Core-i5-4670T-enabled.xml |   8 +
 tests/cputestdata/x86_64-cpuid-Core-i5-4670T.json  |   4 +-
 .../x86_64-cpuid-Core-i5-6600-disabled.xml |   5 +
 .../x86_64-cpuid-Core-i5-6600-enabled.xml  |   9 ++
 .../x86_64-cpuid-Core-i7-2600-disabled.xml |   6 +
 .../x86_64-cpuid-Core-i7-2600-enabled.xml  |   8 +
 .../x86_64-cpuid-Core-i7-3740QM-disabled.xml   |   6 +
 .../x86_64-cpuid-Core-i7-3740QM-enabled.xml|   8 +
 .../x86_64-cpuid-Core-i7-3770-disabled.xml |   6 +
 .../x86_64-cpuid-Core-i7-3770-enabled.xml  |   8 +
 .../x86_64-cpuid-Core-i7-4510U-disabled.xml|   5 +
 .../x86_64-cpuid-Core-i7-4510U-enabled.xml |   9 ++
 .../x86_64-cpuid-Core-i7-4600U-disabled.xml|   5 +
 .../x86_64-cpuid-Core-i7-4600U-enabled.xml |   9 ++
 .../x86_64-cpuid-Core-i7-5600U-disabled.xml|   5 +
 .../x86_64-cpuid-Core-i7-5600U-enabled.xml |   9 ++
 .../x86_64-cpuid-Core2-E6850-disabled.xml  |   4 +
 .../x86_64-cpuid-Core2-E6850-enabled.xml   |   7 +
 .../x86_64-cpuid-Opteron-2350-disabled.xml |   7 +
 .../x86_64-cpuid-Opteron-2350-enabled.xml  |   7 +
 .../x86_64-cpuid-Opteron-6234-disabled.xml |   7 +
 .../x86_64-cpuid-Opteron-6234-enabled.xml  |   9 ++
 .../x86_64-cpuid-Phenom-B95-disabled.xml   |   7 +
 .../x86_64-cpuid-Phenom-B95-enabled.xml|   7 +
 .../x86_64-cpuid-Xeon-E3-1245-disabled.xml |   6 +
 .../x86_64-cpuid-Xeon-E3-1245-enabled.xml  |   9 ++
 .../x86_64-cpuid-Xeon-E5-2630-disabled.xml |   7 +
 .../x86_64-cpuid-Xeon-E5-2630-enabled.xml  |   8 +
 .../x86_64-cpuid-Xeon-E5-2650-disabled.xml |   8 +
 .../x86_64-cpuid-Xeon-E5-2650-enabled.xml  |   7 +
 .../x86_64-cpuid-Xeon-E7-4820-disabled.xml |   6 +
 .../x86_64-cpuid-Xeon-E7-4820-enabled.xml  |   7 +
 .../x86_64-cpuid-Xeon-W3520-disabled.xml   |   5 +
 .../x86_64-cpuid-Xeon-W3520-enabled.xml|   7 +
 55 files changed, 834 insertions(+), 206 deletions(-)
 rename tests/cputestdata/{cpu-convert.py => cpu-cpuid.py} (83%)
 create mode 100644 tests/cputestdata/x86_64-cpuid-A10-5800K-disabled.xml
 create mode 100644 tests/cputestdata/x86_64-cpuid-A10-5800K-enabled.xml
 create mode 100644 tests/cputestdata/x86_64-cpuid-Core-i5-2500-disabled.xml
 create mode 100644 tests/cputestdata/x86_64-cpuid-Core-i5-2500-enabled.xml
 create mode 100644 tests/cputestdata/x86_64-cpuid-Core-i5-2540M-disabled.xml
 create mode 100644 tests/cputestdata/x86_64-cpuid-Core-i5-2540M-enabled.xml
 create mode 100644 tests/cputestdata/x86_64-cpuid-Core-i5-4670T-disabled.xml
 create mode 100644 tests/cputestdata/x86_64-cpuid-Core-i5-4670T-enabled.xml
 create mode 100644 tests/cputestdata/x86_64-cpuid-Core-i5-6600-disabled.xml
 create mode 100644 tests/cputestdata/x86_64-cpuid-Core-i5-6600-enabled.xml
 create mode 100644 

[libvirt] [PATCH 11/14] cputest: Generate data for virCPUUpdateLive

[Jiri Denemark]

Generated with

(cd tests/cputestdata; ./cpu-cpuid.py diff x86_64-cpuid-*.json)

Signed-off-by: Jiri Denemark 
---
 tests/cputestdata/x86_64-cpuid-A10-5800K-disabled.xml  | 7 +++
 tests/cputestdata/x86_64-cpuid-A10-5800K-enabled.xml   | 8 
 tests/cputestdata/x86_64-cpuid-Core-i5-2500-disabled.xml   | 5 +
 tests/cputestdata/x86_64-cpuid-Core-i5-2500-enabled.xml| 9 +
 tests/cputestdata/x86_64-cpuid-Core-i5-2540M-disabled.xml  | 5 +
 tests/cputestdata/x86_64-cpuid-Core-i5-2540M-enabled.xml   | 9 +
 tests/cputestdata/x86_64-cpuid-Core-i5-4670T-disabled.xml  | 6 ++
 tests/cputestdata/x86_64-cpuid-Core-i5-4670T-enabled.xml   | 8 
 tests/cputestdata/x86_64-cpuid-Core-i5-6600-disabled.xml   | 5 +
 tests/cputestdata/x86_64-cpuid-Core-i5-6600-enabled.xml| 9 +
 tests/cputestdata/x86_64-cpuid-Core-i7-2600-disabled.xml   | 6 ++
 tests/cputestdata/x86_64-cpuid-Core-i7-2600-enabled.xml| 8 
 tests/cputestdata/x86_64-cpuid-Core-i7-3740QM-disabled.xml | 6 ++
 tests/cputestdata/x86_64-cpuid-Core-i7-3740QM-enabled.xml  | 8 
 tests/cputestdata/x86_64-cpuid-Core-i7-3770-disabled.xml   | 6 ++
 tests/cputestdata/x86_64-cpuid-Core-i7-3770-enabled.xml| 8 
 tests/cputestdata/x86_64-cpuid-Core-i7-4510U-disabled.xml  | 5 +
 tests/cputestdata/x86_64-cpuid-Core-i7-4510U-enabled.xml   | 9 +
 tests/cputestdata/x86_64-cpuid-Core-i7-4600U-disabled.xml  | 5 +
 tests/cputestdata/x86_64-cpuid-Core-i7-4600U-enabled.xml   | 9 +
 tests/cputestdata/x86_64-cpuid-Core-i7-5600U-disabled.xml  | 5 +
 tests/cputestdata/x86_64-cpuid-Core-i7-5600U-enabled.xml   | 9 +
 tests/cputestdata/x86_64-cpuid-Core2-E6850-disabled.xml| 4 
 tests/cputestdata/x86_64-cpuid-Core2-E6850-enabled.xml | 7 +++
 tests/cputestdata/x86_64-cpuid-Opteron-2350-disabled.xml   | 7 +++
 tests/cputestdata/x86_64-cpuid-Opteron-2350-enabled.xml| 7 +++
 tests/cputestdata/x86_64-cpuid-Opteron-6234-disabled.xml   | 7 +++
 tests/cputestdata/x86_64-cpuid-Opteron-6234-enabled.xml| 9 +
 tests/cputestdata/x86_64-cpuid-Phenom-B95-disabled.xml | 7 +++
 tests/cputestdata/x86_64-cpuid-Phenom-B95-enabled.xml  | 7 +++
 tests/cputestdata/x86_64-cpuid-Xeon-E3-1245-disabled.xml   | 6 ++
 tests/cputestdata/x86_64-cpuid-Xeon-E3-1245-enabled.xml| 9 +
 tests/cputestdata/x86_64-cpuid-Xeon-E5-2630-disabled.xml   | 6 ++
 tests/cputestdata/x86_64-cpuid-Xeon-E5-2630-enabled.xml| 8 
 tests/cputestdata/x86_64-cpuid-Xeon-E5-2650-disabled.xml   | 8 
 tests/cputestdata/x86_64-cpuid-Xeon-E5-2650-enabled.xml| 7 +++
 tests/cputestdata/x86_64-cpuid-Xeon-E7-4820-disabled.xml   | 6 ++
 tests/cputestdata/x86_64-cpuid-Xeon-E7-4820-enabled.xml| 7 +++
 tests/cputestdata/x86_64-cpuid-Xeon-W3520-disabled.xml | 5 +
 tests/cputestdata/x86_64-cpuid-Xeon-W3520-enabled.xml  | 7 +++
 40 files changed, 279 insertions(+)
 create mode 100644 tests/cputestdata/x86_64-cpuid-A10-5800K-disabled.xml
 create mode 100644 tests/cputestdata/x86_64-cpuid-A10-5800K-enabled.xml
 create mode 100644 tests/cputestdata/x86_64-cpuid-Core-i5-2500-disabled.xml
 create mode 100644 tests/cputestdata/x86_64-cpuid-Core-i5-2500-enabled.xml
 create mode 100644 tests/cputestdata/x86_64-cpuid-Core-i5-2540M-disabled.xml
 create mode 100644 tests/cputestdata/x86_64-cpuid-Core-i5-2540M-enabled.xml
 create mode 100644 tests/cputestdata/x86_64-cpuid-Core-i5-4670T-disabled.xml
 create mode 100644 tests/cputestdata/x86_64-cpuid-Core-i5-4670T-enabled.xml
 create mode 100644 tests/cputestdata/x86_64-cpuid-Core-i5-6600-disabled.xml
 create mode 100644 tests/cputestdata/x86_64-cpuid-Core-i5-6600-enabled.xml
 create mode 100644 tests/cputestdata/x86_64-cpuid-Core-i7-2600-disabled.xml
 create mode 100644 tests/cputestdata/x86_64-cpuid-Core-i7-2600-enabled.xml
 create mode 100644 tests/cputestdata/x86_64-cpuid-Core-i7-3740QM-disabled.xml
 create mode 100644 tests/cputestdata/x86_64-cpuid-Core-i7-3740QM-enabled.xml
 create mode 100644 tests/cputestdata/x86_64-cpuid-Core-i7-3770-disabled.xml
 create mode 100644 tests/cputestdata/x86_64-cpuid-Core-i7-3770-enabled.xml
 create mode 100644 tests/cputestdata/x86_64-cpuid-Core-i7-4510U-disabled.xml
 create mode 100644 tests/cputestdata/x86_64-cpuid-Core-i7-4510U-enabled.xml
 create mode 100644 tests/cputestdata/x86_64-cpuid-Core-i7-4600U-disabled.xml
 create mode 100644 tests/cputestdata/x86_64-cpuid-Core-i7-4600U-enabled.xml
 create mode 100644 tests/cputestdata/x86_64-cpuid-Core-i7-5600U-disabled.xml
 create mode 100644 tests/cputestdata/x86_64-cpuid-Core-i7-5600U-enabled.xml
 create mode 100644 tests/cputestdata/x86_64-cpuid-Core2-E6850-disabled.xml
 create mode 100644 tests/cputestdata/x86_64-cpuid-Core2-E6850-enabled.xml
 create mode 100644 tests/cputestdata/x86_64-cpuid-Opteron-2350-disabled.xml
 create mode 100644 

[libvirt] [PATCH 02/14] cpu: Introduce virCPUExpandFeatures

[Jiri Denemark]

Having to use cpuBaseline with VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES
flag to expand CPU features is strange. Not to mention that cpuBaseline
can only expand host CPU definitions (i.e., it completely ignores
feature policies). The new virCPUExpandFeatures API is designed to work
with both host and guest CPU definitions.

Signed-off-by: Jiri Denemark 
---
 src/cpu/cpu.c| 50 +
 src/cpu/cpu.h|  8 
 src/cpu/cpu_x86.c| 53 
 src/libvirt_private.syms |  1 +
 4 files changed, 112 insertions(+)

diff --git a/src/cpu/cpu.c b/src/cpu/cpu.c
index 1461190ba..5604db1d1 100644
--- a/src/cpu/cpu.c
+++ b/src/cpu/cpu.c
@@ -1063,3 +1063,53 @@ virCPUConvertLegacy(virArch arch,
 VIR_DEBUG("model=%s", NULLSTR(cpu->model));
 return 0;
 }
+
+
+static int
+virCPUFeatureCompare(const void *p1,
+ const void *p2)
+{
+const virCPUFeatureDef *f1 = p1;
+const virCPUFeatureDef *f2 = p2;
+
+return strcmp(f1->name, f2->name);
+}
+
+
+/**
+ * virCPUExpandFeatures:
+ *
+ * @arch: CPU architecture
+ * @cpu: CPU definition to be expanded
+ *
+ * Add all features implicitly enabled by the CPU model to the list of
+ * features. The @cpu is expected to be either a host or a guest representation
+ * of a host CPU, i.e., only VIR_CPU_FEATURE_REQUIRE and
+ * VIR_CPU_FEATURE_DISABLE policies are supported.
+ *
+ * The updated list of features in the CPU definition is sorted.
+ *
+ * Return -1 on error, 0 on success.
+ */
+int
+virCPUExpandFeatures(virArch arch,
+ virCPUDefPtr cpu)
+{
+struct cpuArchDriver *driver;
+
+VIR_DEBUG("arch=%s, cpu=%p, model=%s, nfeatures=%zu",
+  virArchToString(arch), cpu, NULLSTR(cpu->model), cpu->nfeatures);
+
+if (!(driver = cpuGetSubDriver(arch)))
+return -1;
+
+if (driver->expandFeatures &&
+driver->expandFeatures(cpu) < 0)
+return -1;
+
+qsort(cpu->features, cpu->nfeatures, sizeof(*cpu->features),
+  virCPUFeatureCompare);
+
+VIR_DEBUG("nfeatures=%zu", cpu->nfeatures);
+return 0;
+}
diff --git a/src/cpu/cpu.h b/src/cpu/cpu.h
index 7d6d3e921..5a8728bce 100644
--- a/src/cpu/cpu.h
+++ b/src/cpu/cpu.h
@@ -116,6 +116,9 @@ typedef int
 typedef int
 (*virCPUArchConvertLegacy)(virCPUDefPtr cpu);
 
+typedef int
+(*virCPUArchExpandFeatures)(virCPUDefPtr cpu);
+
 struct cpuArchDriver {
 const char *name;
 const virArch *arch;
@@ -135,6 +138,7 @@ struct cpuArchDriver {
 virCPUArchGetModels getModels;
 virCPUArchTranslate translate;
 virCPUArchConvertLegacy convertLegacy;
+virCPUArchExpandFeatures expandFeatures;
 };
 
 
@@ -245,6 +249,10 @@ virCPUConvertLegacy(virArch arch,
 virCPUDefPtr cpu)
 ATTRIBUTE_NONNULL(2);
 
+int
+virCPUExpandFeatures(virArch arch,
+ virCPUDefPtr cpu);
+
 /* virCPUDataFormat and virCPUDataParse are implemented for unit tests only and
  * have no real-life usage
  */
diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c
index 9e208b094..442f0ce44 100644
--- a/src/cpu/cpu_x86.c
+++ b/src/cpu/cpu_x86.c
@@ -2891,6 +2891,58 @@ virCPUx86Translate(virCPUDefPtr cpu,
 }
 
 
+static int
+virCPUx86ExpandFeatures(virCPUDefPtr cpu)
+{
+virCPUx86MapPtr map;
+virCPUDefPtr expanded = NULL;
+virCPUx86ModelPtr model = NULL;
+bool host = cpu->type == VIR_CPU_TYPE_HOST;
+size_t i;
+int ret = -1;
+
+if (!(map = virCPUx86GetMap()))
+goto cleanup;
+
+if (!(expanded = virCPUDefCopy(cpu)))
+goto cleanup;
+
+virCPUDefFreeFeatures(expanded);
+
+if (!(model = x86ModelFind(map, cpu->model))) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("unknown CPU model %s"), cpu->model);
+goto cleanup;
+}
+
+if (!(model = x86ModelCopy(model)) ||
+x86DataToCPUFeatures(expanded, host ? -1 : VIR_CPU_FEATURE_REQUIRE,
+ >data, map) < 0)
+goto cleanup;
+
+for (i = 0; i < cpu->nfeatures; i++) {
+virCPUFeatureDefPtr f = cpu->features + i;
+
+if (!host &&
+f->policy != VIR_CPU_FEATURE_REQUIRE &&
+f->policy != VIR_CPU_FEATURE_DISABLE)
+continue;
+
+if (virCPUDefUpdateFeature(expanded, f->name, f->policy) < 0)
+goto cleanup;
+}
+
+virCPUDefFreeModel(cpu);
+
+ret = virCPUDefCopyModel(cpu, expanded, false);
+
+ cleanup:
+virCPUDefFree(expanded);
+x86ModelFree(model);
+return ret;
+}
+
+
 int
 virCPUx86DataAddCPUID(virCPUDataPtr cpuData,
   const virCPUx86CPUID *cpuid)
@@ -2965,4 +3017,5 @@ struct cpuArchDriver cpuDriverX86 = {
 .dataParse  = virCPUx86DataParse,
 .getModels  = virCPUx86GetModels,
 .translate  = virCPUx86Translate,
+.expandFeatures = virCPUx86ExpandFeatures,
 };
diff --git a/src/libvirt_private.syms 

[libvirt] [PATCH 06/14] cputest: Move instantiation of JSONDecoder in cpu-convert.py

[Jiri Denemark]

Let's make the object local to the parseFeatureWords function which uses
it.

Signed-off-by: Jiri Denemark 
---
 tests/cputestdata/cpu-convert.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/cputestdata/cpu-convert.py b/tests/cputestdata/cpu-convert.py
index 898d42f10..e069408c8 100755
--- a/tests/cputestdata/cpu-convert.py
+++ b/tests/cputestdata/cpu-convert.py
@@ -174,6 +174,8 @@ cpuidMap = [
 def parseFeatureWords(path):
 features = None
 
+dec = json.JSONDecoder()
+
 with open(path, "r") as f:
 s = f.read()
 
@@ -220,8 +222,6 @@ def propAdd(props, feature, value):
 props[name] = value
 
 
-dec = json.JSONDecoder()
-
 for path in sys.argv[1:]:
 props, cpuid = parseFeatureWords(path)
 
-- 
2.12.0

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

[libvirt] [PATCH 13/14] cputest: Disable "cmt" feature unknown to QEMU

[Jiri Denemark]

All CPU features which QEMU does not know about but libvirt knows them
(currently "cmt" is the only one) are implicitly disabled by QEMU and
should be present in x86_64-cpuid-*-disabled.xml.

Signed-off-by: Jiri Denemark 
---
 tests/cputestdata/cpu-cpuid.py   | 1 +
 tests/cputestdata/x86_64-cpuid-Xeon-E5-2630-disabled.xml | 1 +
 tests/cputestdata/x86_64-cpuid-Xeon-E5-2650-disabled.xml | 2 +-
 3 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/tests/cputestdata/cpu-cpuid.py b/tests/cputestdata/cpu-cpuid.py
index 4b9b04ace..85c7c94b1 100755
--- a/tests/cputestdata/cpu-cpuid.py
+++ b/tests/cputestdata/cpu-cpuid.py
@@ -79,6 +79,7 @@ cpuidMap = [
 {"in_eax": 0x0007, "in_ecx": 0, "eax": 0, "ebx": 0x0200, "ecx": 0, 
"edx": 0, "names": ["erms"]},
 {"in_eax": 0x0007, "in_ecx": 0, "eax": 0, "ebx": 0x0400, "ecx": 0, 
"edx": 0, "names": ["invpcid"]},
 {"in_eax": 0x0007, "in_ecx": 0, "eax": 0, "ebx": 0x0800, "ecx": 0, 
"edx": 0, "names": ["rtm"]},
+{"in_eax": 0x0007, "in_ecx": 0, "eax": 0, "ebx": 0x1000, "ecx": 0, 
"edx": 0, "names": []}, # cmt is unknown to QEMU
 {"in_eax": 0x0007, "in_ecx": 0, "eax": 0, "ebx": 0x4000, "ecx": 0, 
"edx": 0, "names": ["mpx"]},
 {"in_eax": 0x0007, "in_ecx": 0, "eax": 0, "ebx": 0x0001, "ecx": 0, 
"edx": 0, "names": ["avx512f"]},
 {"in_eax": 0x0007, "in_ecx": 0, "eax": 0, "ebx": 0x0002, "ecx": 0, 
"edx": 0, "names": ["avx512dq"]},
diff --git a/tests/cputestdata/x86_64-cpuid-Xeon-E5-2630-disabled.xml 
b/tests/cputestdata/x86_64-cpuid-Xeon-E5-2630-disabled.xml
index ea2906506..e6f4ce761 100644
--- a/tests/cputestdata/x86_64-cpuid-Xeon-E5-2630-disabled.xml
+++ b/tests/cputestdata/x86_64-cpuid-Xeon-E5-2630-disabled.xml
@@ -2,5 +2,6 @@
 
   
   
+  
   
 
diff --git a/tests/cputestdata/x86_64-cpuid-Xeon-E5-2650-disabled.xml 
b/tests/cputestdata/x86_64-cpuid-Xeon-E5-2650-disabled.xml
index a66b5bdd6..9e25e6a94 100644
--- a/tests/cputestdata/x86_64-cpuid-Xeon-E5-2650-disabled.xml
+++ b/tests/cputestdata/x86_64-cpuid-Xeon-E5-2650-disabled.xml
@@ -2,7 +2,7 @@
 
   
   
-  
+  
   
   
 
-- 
2.12.0

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

[libvirt] [PATCH 07/14] cputest: Rename cpu-convert.py script as cpu-cpuid.py

[Jiri Denemark]

The new script is going to be more general and the original
functionality can be requested by "cpu-cpuid.py convert".

Signed-off-by: Jiri Denemark 
---
 tests/cputestdata/{cpu-convert.py => cpu-cpuid.py} | 17 -
 tests/cputestdata/cpu-parse.sh |  2 +-
 2 files changed, 17 insertions(+), 2 deletions(-)
 rename tests/cputestdata/{cpu-convert.py => cpu-cpuid.py} (98%)

diff --git a/tests/cputestdata/cpu-convert.py b/tests/cputestdata/cpu-cpuid.py
similarity index 98%
rename from tests/cputestdata/cpu-convert.py
rename to tests/cputestdata/cpu-cpuid.py
index e069408c8..a4dc23378 100755
--- a/tests/cputestdata/cpu-convert.py
+++ b/tests/cputestdata/cpu-cpuid.py
@@ -222,7 +222,7 @@ def propAdd(props, feature, value):
 props[name] = value
 
 
-for path in sys.argv[1:]:
+def convert(path):
 props, cpuid = parseFeatureWords(path)
 
 for feature in cpuidMap:
@@ -247,3 +247,18 @@ for path in sys.argv[1:]:
"id": "model-expansion"},
   f, indent = 2, separators = (',', ': '))
 f.write("\n")
+
+
+if len(sys.argv) < 3:
+print "Usage: %s convert json_file..." % sys.argv[0]
+sys.exit(1)
+
+action = sys.argv[1]
+args = sys.argv[2:]
+
+if action == "convert":
+for path in args:
+convert(path)
+else:
+print "Unknown action: " + action
+sys.exit(1)
diff --git a/tests/cputestdata/cpu-parse.sh b/tests/cputestdata/cpu-parse.sh
index 86bcb030d..d823c399b 100755
--- a/tests/cputestdata/cpu-parse.sh
+++ b/tests/cputestdata/cpu-parse.sh
@@ -53,7 +53,7 @@ json <<<"$data" >$fname.json
 if [[ -s $fname.json ]]; then
 echo $fname.json
 if ! grep -q model-expansion $fname.json; then
-$(dirname $0)/cpu-convert.py $fname.json
+$(dirname $0)/cpu-cpuid.py convert $fname.json
 fi
 else
 rm $fname.json
-- 
2.12.0

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

[libvirt] [PATCH 08/14] cputest: Add cpuidIsSet helper to cpu-cpuid.py

[Jiri Denemark]

Signed-off-by: Jiri Denemark 
---
 tests/cputestdata/cpu-cpuid.py | 41 +
 1 file changed, 21 insertions(+), 20 deletions(-)

diff --git a/tests/cputestdata/cpu-cpuid.py b/tests/cputestdata/cpu-cpuid.py
index a4dc23378..9ea858d98 100755
--- a/tests/cputestdata/cpu-cpuid.py
+++ b/tests/cputestdata/cpu-cpuid.py
@@ -171,6 +171,24 @@ cpuidMap = [
 ]
 
 
+def cpuidIsSet(cpuid, feature):
+in_eax = feature["in_eax"]
+in_ecx = feature["in_ecx"]
+eax = feature["eax"]
+ebx = feature["ebx"]
+ecx = feature["ecx"]
+edx = feature["edx"]
+
+if in_eax not in cpuid or in_ecx not in cpuid[in_eax]:
+return False
+else:
+leaf = cpuid[in_eax][in_ecx]
+return ((eax > 0 and leaf["eax"] & eax > 0) or
+(ebx > 0 and leaf["ebx"] & ebx > 0) or
+(ecx > 0 and leaf["ecx"] & ecx > 0) or
+(edx > 0 and leaf["edx"] & edx > 0))
+
+
 def parseFeatureWords(path):
 features = None
 
@@ -217,30 +235,13 @@ def parseFeatureWords(path):
 return props, cpuid
 
 
-def propAdd(props, feature, value):
-for name in feature["names"]:
-props[name] = value
-
-
 def convert(path):
 props, cpuid = parseFeatureWords(path)
 
 for feature in cpuidMap:
-in_eax = feature["in_eax"]
-in_ecx = feature["in_ecx"]
-eax = feature["eax"]
-ebx = feature["ebx"]
-ecx = feature["ecx"]
-edx = feature["edx"]
-
-if in_eax not in cpuid or in_ecx not in cpuid[in_eax]:
-propAdd(props, feature, False)
-else:
-leaf = cpuid[in_eax][in_ecx]
-propAdd(props, feature, ((eax > 0 and leaf["eax"] & eax > 0) or
- (ebx > 0 and leaf["ebx"] & ebx > 0) or
- (ecx > 0 and leaf["ecx"] & ecx > 0) or
- (edx > 0 and leaf["edx"] & edx > 0)))
+value = cpuidIsSet(cpuid, feature)
+for name in feature["names"]:
+props[name] = value
 
 with open(path, "w") as f:
 json.dump({"return": {"model": {"name": "base", "props": props}},
-- 
2.12.0

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

[libvirt] [PATCH 14/14] cputest: Add tests for virCPUUpdateLive API

[Jiri Denemark]

The test takes

  x86-cpuid-Something-guest.xml CPU (the CPU libvirt would use for
host-model on a CPU described by x86_64-cpuid-Something.xml without
talking to QEMU about what it supports on the host)

and updates it according to CPUID data from QEMU:

  x86_64-cpuid-Something-enabled.xml (reported as "feature-words"
property of the CPU device)

and

  x86_64-cpuid-Something-disabled.xml (reported as "filtered-features"
property of the CPU device).

The result is compared to

  x86_64-cpuid-Something-json.xml (the CPU libvirt would use as
host-model based on the reply from query-cpu-model-expansion).

The comparison is a bit tricky because the *-json.xml CPU contains fewer
disabled features. Only the features which are included in the base CPU
model, but listed as disabled in *.json will be disabled in *-json.xml.
The CPU computed by virCPUUpdateLive from the test data will list all
features present in the host's CPUID data and not enabled in *.json as
disabled. The cpuTestUpdateLiveCompare function checks that the computed
and expected sets of enabled features match.

Signed-off-by: Jiri Denemark 
---
 tests/cputest.c | 149 
 1 file changed, 149 insertions(+)

diff --git a/tests/cputest.c b/tests/cputest.c
index 4a4d427e1..2c64c2cd0 100644
--- a/tests/cputest.c
+++ b/tests/cputest.c
@@ -523,6 +523,151 @@ cpuTestGuestCPUID(const void *arg)
 }
 
 
+static int
+cpuTestUpdateLiveCompare(virArch arch,
+ virCPUDefPtr actual,
+ virCPUDefPtr expected)
+{
+size_t i, j;
+int ret = 0;
+
+if (virCPUExpandFeatures(arch, actual) < 0 ||
+virCPUExpandFeatures(arch, expected) < 0)
+return -1;
+
+if (STRNEQ(actual->model, expected->model)) {
+VIR_TEST_VERBOSE("Actual CPU model '%s', expected '%s'\n",
+ actual->model, expected->model);
+return -1;
+}
+
+i = j = 0;
+while (i < actual->nfeatures || j < expected->nfeatures) {
+virCPUFeatureDefPtr featAct = NULL;
+virCPUFeatureDefPtr featExp = NULL;
+int cmp;
+
+if (i < actual->nfeatures)
+featAct = actual->features + i;
+
+if (j < expected->nfeatures)
+featExp = expected->features + j;
+
+/*
+ * Act < Exp => cmp < 0 (missing entry in Exp)
+ * Act = Exp => cmp = 0
+ * Act > Exp => cmp > 0 (missing entry in Act)
+ *
+ * NULL > name for any name != NULL
+ */
+if (featAct && featExp)
+cmp = strcmp(featAct->name, featExp->name);
+else
+cmp = featExp ? 1 : -1;
+
+if (cmp <= 0)
+i++;
+if (cmp >= 0)
+j++;
+
+/* Possible combinations of cmp, featAct->policy, and featExp->policy:
+ *  cmp Act Exp result
+ * -
+ *   0  dis dis  ok
+ *   0  dis req missing
+ *   0  req dis extra
+ *   0  req req  ok
+ * -
+ *   -  dis  X   ok # ignoring extra disabled features
+ *   -  req  X  extra
+ * -
+ *   +   X  dis extra
+ *   +   X  req missing
+ */
+if ((cmp == 0 &&
+ featAct->policy == VIR_CPU_FEATURE_DISABLE &&
+ featExp->policy == VIR_CPU_FEATURE_REQUIRE) ||
+(cmp > 0 &&
+ featExp->policy == VIR_CPU_FEATURE_REQUIRE)) {
+VIR_TEST_VERBOSE("Actual CPU lacks feature '%s'\n",
+ featExp->name);
+ret = -1;
+continue;
+}
+
+if ((cmp == 0 &&
+ featAct->policy == VIR_CPU_FEATURE_REQUIRE &&
+ featExp->policy == VIR_CPU_FEATURE_DISABLE) ||
+(cmp < 0 &&
+ featAct->policy == VIR_CPU_FEATURE_REQUIRE) ||
+(cmp > 0 &&
+ featExp->policy == VIR_CPU_FEATURE_DISABLE)) {
+VIR_TEST_VERBOSE("Actual CPU has extra feature '%s'\n",
+ featAct->name);
+ret = -1;
+}
+}
+
+return ret;
+}
+
+
+static int
+cpuTestUpdateLive(const void *arg)
+{
+const struct data *data = arg;
+char *cpuFile = NULL;
+virCPUDefPtr cpu = NULL;
+char *enabledFile = NULL;
+char *enabled = NULL;
+virCPUDataPtr enabledData = NULL;
+char *disabledFile = NULL;
+char *disabled = NULL;
+virCPUDataPtr disabledData = NULL;
+char *expectedFile = NULL;
+virCPUDefPtr expected = NULL;
+int ret = -1;
+
+if (virAsprintf(, "cpuid-%s-guest", data->host) < 0 ||
+!(cpu = cpuTestLoadXML(data->arch, cpuFile)))
+goto cleanup;
+
+if (virAsprintf(, "%s/cputestdata/%s-cpuid-%s-enabled.xml",
+  

[libvirt] [PATCH 04/14] cpu: Move feature expansion out of cpuBaseline

[Jiri Denemark]

cpuBaseline is responsible for computing a baseline CPU while feature
expansion is done by virCPUExpandFeatures. The cpuBaselineXML wrapper
(used by hypervisor drivers to implement virConnectBaselineCPU API)
calls cpuBaseline followed by virCPUExpandFeatures if requested by
VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES flag.

The features in the three changed test files had to be sorted using
"sort -k 3" because virCPUExpandFeatures returns a sorted list of
features.

Signed-off-by: Jiri Denemark 
---
 src/cpu/cpu.c|  8 +++
 src/cpu/cpu_x86.c| 33 ---
 tests/cputest.c  |  8 +++
 tests/cputestdata/x86_64-baseline-3-expanded.xml | 48 
 tests/cputestdata/x86_64-baseline-4-expanded.xml | 68 +++
 tests/cputestdata/x86_64-baseline-5-expanded.xml | 70 
 6 files changed, 109 insertions(+), 126 deletions(-)

diff --git a/src/cpu/cpu.c b/src/cpu/cpu.c
index 33581e5fe..d53a7ef2c 100644
--- a/src/cpu/cpu.c
+++ b/src/cpu/cpu.c
@@ -498,6 +498,10 @@ cpuBaselineXML(const char **xmlCPUs,
 size_t i;
 
 VIR_DEBUG("ncpus=%u, nmodels=%u", ncpus, nmodels);
+
+virCheckFlags(VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES |
+  VIR_CONNECT_BASELINE_CPU_MIGRATABLE, NULL);
+
 if (xmlCPUs) {
 for (i = 0; i < ncpus; i++)
 VIR_DEBUG("xmlCPUs[%zu]=%s", i, NULLSTR(xmlCPUs[i]));
@@ -538,6 +542,10 @@ cpuBaselineXML(const char **xmlCPUs,
 if (!(cpu = cpuBaseline(cpus, ncpus, models, nmodels, flags)))
 goto error;
 
+if ((flags & VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES) &&
+virCPUExpandFeatures(cpus[0]->arch, cpu) < 0)
+goto error;
+
 cpustr = virCPUDefFormat(cpu, NULL, false);
 
  cleanup:
diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c
index 9c480398f..388102f35 100644
--- a/src/cpu/cpu_x86.c
+++ b/src/cpu/cpu_x86.c
@@ -967,28 +967,6 @@ x86FeaturesLoad(virCPUx86MapPtr map,
 return 0;
 }
 
-static int
-x86DataFromCPUFeatures(virCPUx86Data *data,
-   virCPUDefPtr cpu,
-   virCPUx86MapPtr map)
-{
-size_t i;
-
-for (i = 0; i < cpu->nfeatures; i++) {
-virCPUx86FeaturePtr feature;
-if (!(feature = x86FeatureFind(map, cpu->features[i].name))) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("Unknown CPU feature %s"), cpu->features[i].name);
-return -1;
-}
-
-if (x86DataAdd(data, >data) < 0)
-return -1;
-}
-
-return 0;
-}
-
 
 static virCPUx86ModelPtr
 x86ModelNew(void)
@@ -1948,17 +1926,6 @@ x86Decode(virCPUDefPtr cpu,
 }
 }
 
-if (flags & VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES) {
-if (x86DataCopy(, >data) < 0 ||
-x86DataFromCPUFeatures(, cpuModel, map) < 0)
-goto cleanup;
-
-x86DataSubtract(, );
-if (x86DataToCPUFeatures(cpuModel, VIR_CPU_FEATURE_REQUIRE,
- , map) < 0)
-goto cleanup;
-}
-
 if (vendor && VIR_STRDUP(cpu->vendor, vendor->name) < 0)
 goto cleanup;
 
diff --git a/tests/cputest.c b/tests/cputest.c
index 5e205c501..6396e8670 100644
--- a/tests/cputest.c
+++ b/tests/cputest.c
@@ -325,6 +325,14 @@ cpuTestBaseline(const void *arg)
 goto cleanup;
 
 baseline = cpuBaseline(cpus, ncpus, NULL, 0, data->flags);
+
+if (baseline &&
+(data->flags & VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES) &&
+virCPUExpandFeatures(data->arch, baseline) < 0) {
+virCPUDefFree(baseline);
+baseline = NULL;
+}
+
 if (data->result < 0) {
 virResetLastError();
 if (!baseline) {
diff --git a/tests/cputestdata/x86_64-baseline-3-expanded.xml 
b/tests/cputestdata/x86_64-baseline-3-expanded.xml
index f0c2273d8..82857e3d4 100644
--- a/tests/cputestdata/x86_64-baseline-3-expanded.xml
+++ b/tests/cputestdata/x86_64-baseline-3-expanded.xml
@@ -1,35 +1,35 @@
 
   Westmere
-  
-  
-  
-  
-  
-  
-  
-  
+  
   
-  
-  
-  
-  
-  
-  
-  
   
-  
+  
+  
+  
+  
+  
   
+  
+  
+  
+  
+  
+  
+  
+  
+  
+  
+  
+  
+  
+  
+  
+  
   
   
-  
-  
-  
   
   
-  
-  
+  
   
-  
-  
-  
+  
 
diff --git a/tests/cputestdata/x86_64-baseline-4-expanded.xml 
b/tests/cputestdata/x86_64-baseline-4-expanded.xml
index 7e4578e1a..e54eca026 100644
--- a/tests/cputestdata/x86_64-baseline-4-expanded.xml
+++ b/tests/cputestdata/x86_64-baseline-4-expanded.xml
@@ -1,46 +1,46 @@
 
   Westmere
   Intel
-  
-  
-  
-  
-  
-  
-  
-  
-  
-  
-  
-  
-  
-  
-  
-  
-  
-  
+  
   
-  
-  
-  
-  
-  
-  
-  
+  
   
-  
+  
+  
+  
+  
+  
   
+  
+  
+  
+  
+  
+  
+  
+  
+  
+  
+  
+  
+  
+  
+  
+  
+  
+  
+  
+  
+  
   
   
-  
-  
-  
   
   
-  
-  
+  
   
-  
-  
-  
+  
+  
+  
+  
+  
 
diff --git a/tests/cputestdata/x86_64-baseline-5-expanded.xml 

[libvirt] [PATCH 12/14] cputest: Disable TSX on broken models

[Jiri Denemark]

Commit v3.1.0-26-gd60012b4e started filtering hle and rtm features from
broken Intel Haswell CPUs. QEMU implemented similar functionality and
thus it doesn't report rtm and hle features as enabled for Core i5-4670T
CPU anymore.

Signed-off-by: Jiri Denemark 
---
 tests/cputestdata/x86_64-cpuid-Core-i5-4670T-disabled.xml | 1 +
 tests/cputestdata/x86_64-cpuid-Core-i5-4670T-enabled.xml  | 2 +-
 tests/cputestdata/x86_64-cpuid-Core-i5-4670T.json | 4 ++--
 3 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/tests/cputestdata/x86_64-cpuid-Core-i5-4670T-disabled.xml 
b/tests/cputestdata/x86_64-cpuid-Core-i5-4670T-disabled.xml
index 601554a81..0c6b68a57 100644
--- a/tests/cputestdata/x86_64-cpuid-Core-i5-4670T-disabled.xml
+++ b/tests/cputestdata/x86_64-cpuid-Core-i5-4670T-disabled.xml
@@ -2,5 +2,6 @@
 
   
   
+  
   
 
diff --git a/tests/cputestdata/x86_64-cpuid-Core-i5-4670T-enabled.xml 
b/tests/cputestdata/x86_64-cpuid-Core-i5-4670T-enabled.xml
index 8a714379c..31893c0b8 100644
--- a/tests/cputestdata/x86_64-cpuid-Core-i5-4670T-enabled.xml
+++ b/tests/cputestdata/x86_64-cpuid-Core-i5-4670T-enabled.xml
@@ -1,7 +1,7 @@
 
 
   
-  
+  
   
   
   
diff --git a/tests/cputestdata/x86_64-cpuid-Core-i5-4670T.json 
b/tests/cputestdata/x86_64-cpuid-Core-i5-4670T.json
index bfcfcfd46..58b1d0369 100644
--- a/tests/cputestdata/x86_64-cpuid-Core-i5-4670T.json
+++ b/tests/cputestdata/x86_64-cpuid-Core-i5-4670T.json
@@ -5,7 +5,7 @@
   "props": {
 "pfthreshold": false,
 "pku": false,
-"rtm": true,
+"rtm": false,
 "tsc_adjust": true,
 "tsc-deadline": true,
 "xstore-en": false,
@@ -69,7 +69,7 @@
 "avx512-4vnniw": false,
 "xsave": true,
 "erms": true,
-"hle": true,
+"hle": false,
 "nodeid_msr": false,
 "est": false,
 "svm_lock": false,
-- 
2.12.0

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

[libvirt] [PATCH 10/14] cputest: Add "diff" command to cpu-cpuid.py

[Jiri Denemark]

The new command can be used to generate test data for virCPUUpdateLive.

When "cpu-cpuid.py diff x86-cpuid-Something.json" is run, it reads raw
CPUID data stored in x86-cpuid-Something.xml and CPUID data from QEMU
stored in x86-cpuid-Something.json to produce two more CPUID files:
x86-cpuid-Something-enabled.xml and x86-cpuid-Something-disabled.xml.

- x86-cpuid-Something-enabled.xml will contain CPUID bits present in
x86-cpuid-Something.json (i.e., enabled by QEMU for the "host" CPU)

- x86-cpuid-Something-disabled.xml will contain all CPUID bits from
x86-cpuid-Something.xml which are not present in
x86-cpuid-Something.json (i.e., CPUID bits which the host CPU
supports, but QEMU does not enable them for the "host" CPU)

Signed-off-by: Jiri Denemark 
---
 tests/cputestdata/cpu-cpuid.py | 89 +-
 tests/cputestdata/cpu-parse.sh |  1 +
 2 files changed, 89 insertions(+), 1 deletion(-)

diff --git a/tests/cputestdata/cpu-cpuid.py b/tests/cputestdata/cpu-cpuid.py
index f4cf6d440..4b9b04ace 100755
--- a/tests/cputestdata/cpu-cpuid.py
+++ b/tests/cputestdata/cpu-cpuid.py
@@ -2,6 +2,7 @@
 
 import sys
 import json
+import xmltodict
 
 # This is a list of x86 CPU features as of QEMU 2.8.50 and it won't need any
 # updates since in the future because query-cpu-model-expansion will be used
@@ -171,6 +172,16 @@ cpuidMap = [
 ]
 
 
+def reverseCpuidMap():
+features = {}
+
+for feature in cpuidMap:
+for name in feature["names"]:
+features[name] = feature
+
+return features
+
+
 def cpuidIsSet(cpuid, feature):
 in_eax = feature["in_eax"]
 in_ecx = feature["in_ecx"]
@@ -201,6 +212,12 @@ def cpuidLeaf(cpuid, in_eax, in_ecx):
 return leaf
 
 
+def cpuidAdd(cpuid, feature):
+leaf = cpuidLeaf(cpuid, feature["in_eax"], feature["in_ecx"])
+for reg in ["eax", "ebx", "ecx", "edx"]:
+leaf[reg] |= feature[reg]
+
+
 def parseFeatureWords(path):
 features = None
 
@@ -240,6 +257,50 @@ def parseFeatureWords(path):
 return props, cpuid
 
 
+def parseQemu(path, features):
+cpuid = {}
+with open(path, "r") as f:
+data = json.load(f)
+
+for (prop, val) in data["return"]["model"]["props"].iteritems():
+if val and prop in features:
+cpuidAdd(cpuid, features[prop])
+
+return cpuid
+
+
+def parseCpuid(path):
+cpuid = {}
+with open(path, "r") as f:
+data = xmltodict.parse(f)
+
+for leaf in data["cpudata"]["cpuid"]:
+leaf["in_eax"] = int(leaf["@eax_in"], 0)
+leaf["in_ecx"] = int(leaf["@ecx_in"], 0)
+for reg in ["eax", "ebx", "ecx", "edx"]:
+leaf[reg] = int(leaf["@" + reg], 0)
+
+cpuidAdd(cpuid, leaf)
+
+return cpuid
+
+
+def formatCpuid(cpuid, path, comment):
+with open(path, "w") as f:
+f.write("\n")
+f.write("\n")
+for in_eax in sorted(cpuid.keys()):
+for in_ecx in sorted(cpuid[in_eax].keys()):
+leaf = cpuid[in_eax][in_ecx]
+line = "  \n")
+
+
 def convert(path):
 props, cpuid = parseFeatureWords(path)
 
@@ -255,8 +316,30 @@ def convert(path):
 f.write("\n")
 
 
+def diff(features, path):
+base = path.replace(".json", "")
+jsonFile = path
+cpuidFile = base + ".xml"
+enabledFile = base + "-enabled.xml"
+disabledFile = base + "-disabled.xml"
+
+cpuid = parseCpuid(cpuidFile)
+qemu = parseQemu(jsonFile, features)
+
+enabled = {}
+disabled = {}
+for feature in cpuidMap:
+if cpuidIsSet(qemu, feature):
+cpuidAdd(enabled, feature)
+elif cpuidIsSet(cpuid, feature):
+cpuidAdd(disabled, feature)
+
+formatCpuid(enabled, enabledFile, "Features enabled by QEMU")
+formatCpuid(disabled, disabledFile, "Features disabled by QEMU")
+
+
 if len(sys.argv) < 3:
-print "Usage: %s convert json_file..." % sys.argv[0]
+print "Usage: %s convert|diff json_file..." % sys.argv[0]
 sys.exit(1)
 
 action = sys.argv[1]
@@ -265,6 +348,10 @@ args = sys.argv[2:]
 if action == "convert":
 for path in args:
 convert(path)
+elif action == "diff":
+features = reverseCpuidMap()
+for path in args:
+diff(features, path)
 else:
 print "Unknown action: " + action
 sys.exit(1)
diff --git a/tests/cputestdata/cpu-parse.sh b/tests/cputestdata/cpu-parse.sh
index d823c399b..cd1ab024b 100755
--- a/tests/cputestdata/cpu-parse.sh
+++ b/tests/cputestdata/cpu-parse.sh
@@ -55,6 +55,7 @@ if [[ -s $fname.json ]]; then
 if ! grep -q model-expansion $fname.json; then
 $(dirname $0)/cpu-cpuid.py convert $fname.json
 fi
+$(dirname $0)/cpu-cpuid.py diff $fname.json
 else
 rm $fname.json
 fi
-- 
2.12.0

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

[libvirt] [PATCH 05/14] cpu: Do not pass virConnectBaselineCPUFlags to cpuBaseline

[Jiri Denemark]

The public API flags are handled by the cpuBaselineXML wrapper. The
internal cpuBaseline API only needs to know whether it is supposed to
drop non-migratable features.

Signed-off-by: Jiri Denemark 
---
 src/cpu/cpu.c   | 14 +-
 src/cpu/cpu.h   |  4 ++--
 src/cpu/cpu_arm.c   |  5 +
 src/cpu/cpu_ppc64.c |  5 +
 src/cpu/cpu_x86.c   | 18 ++
 tests/cputest.c |  3 ++-
 6 files changed, 17 insertions(+), 32 deletions(-)

diff --git a/src/cpu/cpu.c b/src/cpu/cpu.c
index d53a7ef2c..8e07e0ce4 100644
--- a/src/cpu/cpu.c
+++ b/src/cpu/cpu.c
@@ -539,7 +539,8 @@ cpuBaselineXML(const char **xmlCPUs,
 doc = NULL;
 }
 
-if (!(cpu = cpuBaseline(cpus, ncpus, models, nmodels, flags)))
+if (!(cpu = cpuBaseline(cpus, ncpus, models, nmodels,
+!!(flags & VIR_CONNECT_BASELINE_CPU_MIGRATABLE
 goto error;
 
 if ((flags & VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES) &&
@@ -573,18 +574,13 @@ cpuBaselineXML(const char **xmlCPUs,
  * @ncpus: number of CPUs in @cpus
  * @models: list of CPU models that can be considered for the baseline CPU
  * @nmodels: number of CPU models in @models
- * @flags: bitwise-OR of virConnectBaselineCPUFlags
+ * @migratable: requests non-migratable features to be removed from the result
  *
  * Computes the most feature-rich CPU which is compatible with all given
  * host CPUs. If @models array is NULL, all models supported by libvirt will
  * be considered when computing the baseline CPU model, otherwise the baseline
  * CPU model will be one of the provided CPU @models.
  *
- * If @flags includes VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES then libvirt
- * will explicitly list all CPU features that are part of the host CPU,
- * without this flag features that are part of the CPU model will not be
- * listed.
- *
  * Returns baseline CPU definition or NULL on error.
  */
 virCPUDefPtr
@@ -592,7 +588,7 @@ cpuBaseline(virCPUDefPtr *cpus,
 unsigned int ncpus,
 const char **models,
 unsigned int nmodels,
-unsigned int flags)
+bool migratable)
 {
 struct cpuArchDriver *driver;
 size_t i;
@@ -647,7 +643,7 @@ cpuBaseline(virCPUDefPtr *cpus,
 return NULL;
 }
 
-return driver->baseline(cpus, ncpus, models, nmodels, flags);
+return driver->baseline(cpus, ncpus, models, nmodels, migratable);
 }
 
 
diff --git a/src/cpu/cpu.h b/src/cpu/cpu.h
index 92b0f788e..096eabe4c 100644
--- a/src/cpu/cpu.h
+++ b/src/cpu/cpu.h
@@ -79,7 +79,7 @@ typedef virCPUDefPtr
  unsigned int ncpus,
  const char **models,
  unsigned int nmodels,
- unsigned int flags);
+ bool migratable);
 
 typedef int
 (*virCPUArchUpdate)(virCPUDefPtr guest,
@@ -198,7 +198,7 @@ cpuBaseline (virCPUDefPtr *cpus,
  unsigned int ncpus,
  const char **models,
  unsigned int nmodels,
- unsigned int flags)
+ bool migratable)
 ATTRIBUTE_NONNULL(1);
 
 int
diff --git a/src/cpu/cpu_arm.c b/src/cpu/cpu_arm.c
index a1aba2554..474777656 100644
--- a/src/cpu/cpu_arm.c
+++ b/src/cpu/cpu_arm.c
@@ -77,13 +77,10 @@ armBaseline(virCPUDefPtr *cpus,
 unsigned int ncpus ATTRIBUTE_UNUSED,
 const char **models ATTRIBUTE_UNUSED,
 unsigned int nmodels ATTRIBUTE_UNUSED,
-unsigned int flags)
+bool migratable ATTRIBUTE_UNUSED)
 {
 virCPUDefPtr cpu = NULL;
 
-virCheckFlags(VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES |
-  VIR_CONNECT_BASELINE_CPU_MIGRATABLE, NULL);
-
 if (VIR_ALLOC(cpu) < 0 ||
 VIR_STRDUP(cpu->model, cpus[0]->model) < 0) {
 virCPUDefFree(cpu);
diff --git a/src/cpu/cpu_ppc64.c b/src/cpu/cpu_ppc64.c
index 0ad8d17d4..f64592b55 100644
--- a/src/cpu/cpu_ppc64.c
+++ b/src/cpu/cpu_ppc64.c
@@ -768,7 +768,7 @@ ppc64DriverBaseline(virCPUDefPtr *cpus,
 unsigned int ncpus,
 const char **models ATTRIBUTE_UNUSED,
 unsigned int nmodels ATTRIBUTE_UNUSED,
-unsigned int flags)
+bool migratable ATTRIBUTE_UNUSED)
 {
 struct ppc64_map *map;
 const struct ppc64_model *model;
@@ -776,9 +776,6 @@ ppc64DriverBaseline(virCPUDefPtr *cpus,
 virCPUDefPtr cpu = NULL;
 size_t i;
 
-virCheckFlags(VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES |
-  VIR_CONNECT_BASELINE_CPU_MIGRATABLE, NULL);
-
 if (!(map = ppc64LoadMap()))
 goto error;
 
diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c
index 388102f35..48648a7f4 100644
--- a/src/cpu/cpu_x86.c
+++ b/src/cpu/cpu_x86.c
@@ -1823,7 +1823,7 @@ x86Decode(virCPUDefPtr cpu,
   const char **models,
   unsigned int nmodels,
   const char *preferred,
-  unsigned int flags)
+  bool migratable)
 {
 int ret = -1;
 

[libvirt] [PATCH 03/14] cpu: Drop unused flags from cpuArchDecode

[Jiri Denemark]

Signed-off-by: Jiri Denemark 
---
 src/cpu/cpu.c   | 2 +-
 src/cpu/cpu.h   | 3 +--
 src/cpu/cpu_ppc64.c | 7 ++-
 src/cpu/cpu_x86.c   | 7 +++
 4 files changed, 7 insertions(+), 12 deletions(-)

diff --git a/src/cpu/cpu.c b/src/cpu/cpu.c
index 5604db1d1..33581e5fe 100644
--- a/src/cpu/cpu.c
+++ b/src/cpu/cpu.c
@@ -250,7 +250,7 @@ cpuDecode(virCPUDefPtr cpu,
 return -1;
 }
 
-return driver->decode(cpu, data, models, nmodels, preferred, 0);
+return driver->decode(cpu, data, models, nmodels, preferred);
 }
 
 
diff --git a/src/cpu/cpu.h b/src/cpu/cpu.h
index 5a8728bce..92b0f788e 100644
--- a/src/cpu/cpu.h
+++ b/src/cpu/cpu.h
@@ -54,8 +54,7 @@ typedef int
  const virCPUData *data,
  const char **models,
  unsigned int nmodels,
- const char *preferred,
- unsigned int flags);
+ const char *preferred);
 
 typedef int
 (*cpuArchEncode)(virArch arch,
diff --git a/src/cpu/cpu_ppc64.c b/src/cpu/cpu_ppc64.c
index 6e16ffd13..0ad8d17d4 100644
--- a/src/cpu/cpu_ppc64.c
+++ b/src/cpu/cpu_ppc64.c
@@ -665,15 +665,12 @@ ppc64DriverDecode(virCPUDefPtr cpu,
   const virCPUData *data,
   const char **models,
   unsigned int nmodels,
-  const char *preferred ATTRIBUTE_UNUSED,
-  unsigned int flags)
+  const char *preferred ATTRIBUTE_UNUSED)
 {
 int ret = -1;
 struct ppc64_map *map;
 const struct ppc64_model *model;
 
-virCheckFlags(VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES, -1);
-
 if (!data || !(map = ppc64LoadMap()))
 return -1;
 
@@ -740,7 +737,7 @@ virCPUppc64GetHost(virCPUDefPtr cpu,
 #endif
 data->pvr[0].mask = 0xul;
 
-ret = ppc64DriverDecode(cpu, cpuData, models, nmodels, NULL, 0);
+ret = ppc64DriverDecode(cpu, cpuData, models, nmodels, NULL);
 
  cleanup:
 virCPUppc64DataFree(cpuData);
diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c
index 442f0ce44..9c480398f 100644
--- a/src/cpu/cpu_x86.c
+++ b/src/cpu/cpu_x86.c
@@ -1984,10 +1984,9 @@ x86DecodeCPUData(virCPUDefPtr cpu,
  const virCPUData *data,
  const char **models,
  unsigned int nmodels,
- const char *preferred,
- unsigned int flags)
+ const char *preferred)
 {
-return x86Decode(cpu, >data.x86, models, nmodels, preferred, flags);
+return x86Decode(cpu, >data.x86, models, nmodels, preferred, 0);
 }
 
 
@@ -2452,7 +2451,7 @@ virCPUx86GetHost(virCPUDefPtr cpu,
 cpuidSet(CPUX86_EXTENDED, cpuData) < 0)
 goto cleanup;
 
-ret = x86DecodeCPUData(cpu, cpuData, models, nmodels, NULL, 0);
+ret = x86DecodeCPUData(cpu, cpuData, models, nmodels, NULL);
 
  cleanup:
 virCPUx86DataFree(cpuData);
-- 
2.12.0

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

[libvirt] [PATCH 01/14] cpu_conf: Introduce virCPUDefFreeFeatures

[Jiri Denemark]

Signed-off-by: Jiri Denemark 
---
 src/conf/cpu_conf.c  | 21 +++--
 src/conf/cpu_conf.h  |  3 +++
 src/libvirt_private.syms |  1 +
 3 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c
index d7c8b8ff2..b78531e60 100644
--- a/src/conf/cpu_conf.c
+++ b/src/conf/cpu_conf.c
@@ -62,18 +62,27 @@ VIR_ENUM_IMPL(virCPUFeaturePolicy, VIR_CPU_FEATURE_LAST,
   "disable",
   "forbid")
 
-void ATTRIBUTE_NONNULL(1)
-virCPUDefFreeModel(virCPUDefPtr def)
+void
+virCPUDefFreeFeatures(virCPUDefPtr def)
 {
 size_t i;
 
-VIR_FREE(def->model);
-VIR_FREE(def->vendor);
-VIR_FREE(def->vendor_id);
-
 for (i = 0; i < def->nfeatures; i++)
 VIR_FREE(def->features[i].name);
 VIR_FREE(def->features);
+
+def->nfeatures = def->nfeatures_max = 0;
+}
+
+
+void ATTRIBUTE_NONNULL(1)
+virCPUDefFreeModel(virCPUDefPtr def)
+{
+
+VIR_FREE(def->model);
+VIR_FREE(def->vendor);
+VIR_FREE(def->vendor_id);
+virCPUDefFreeFeatures(def);
 }
 
 void
diff --git a/src/conf/cpu_conf.h b/src/conf/cpu_conf.h
index 9118f037e..3e02deed4 100644
--- a/src/conf/cpu_conf.h
+++ b/src/conf/cpu_conf.h
@@ -125,6 +125,9 @@ struct _virCPUDef {
 
 
 void ATTRIBUTE_NONNULL(1)
+virCPUDefFreeFeatures(virCPUDefPtr def);
+
+void ATTRIBUTE_NONNULL(1)
 virCPUDefFreeModel(virCPUDefPtr def);
 
 void
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 165d8cb25..7ac5e533f 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -71,6 +71,7 @@ virCPUDefCopyWithoutModel;
 virCPUDefFormat;
 virCPUDefFormatBuf;
 virCPUDefFree;
+virCPUDefFreeFeatures;
 virCPUDefFreeModel;
 virCPUDefParseXML;
 virCPUDefStealModel;
-- 
2.12.0

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

[libvirt] [PATCH] network: don't add "no-resolv" if we still need DNS servers from resolv.conf

[Laine Stump]

It was pointed out here:

  https://bugzilla.redhat.com/show_bug.cgi?id=1331796#c4

that we shouldn't be adding a "no-resolv" to the dnsmasq.conf file for
a network if there isn't any  element that specifies an IP
address but no qualifying domain. If there is such an element, it will
handle all DNS requests that weren't otherwise handled by one of the
forwarder entries with a matching domain attribute. If not, then DNS
requests that don't match the domain of any  would not be
resolved if we added no-resolv.

So, only add "no-resolv" when there is at least one 
element that specifies an IP address but no qualifying domain.
---
 src/network/bridge_driver.c| 14 +-
 .../nat-network-dns-forwarder-no-resolv.conf   | 12 
 .../nat-network-dns-forwarder-no-resolv.xml| 11 +++
 tests/networkxml2confdata/nat-network-dns-forwarders.conf  |  2 +-
 tests/networkxml2conftest.c|  1 +
 .../nat-network-dns-forwarder-no-resolv.xml| 11 +++
 .../nat-network-dns-forwarder-no-resolv.xml| 11 +++
 tests/networkxml2xmltest.c |  1 +
 8 files changed, 61 insertions(+), 2 deletions(-)
 create mode 100644 
tests/networkxml2confdata/nat-network-dns-forwarder-no-resolv.conf
 create mode 100644 
tests/networkxml2confdata/nat-network-dns-forwarder-no-resolv.xml
 create mode 100644 
tests/networkxml2xmlin/nat-network-dns-forwarder-no-resolv.xml
 create mode 100644 
tests/networkxml2xmlout/nat-network-dns-forwarder-no-resolv.xml

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index c5ec282..32c5ab7 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -1085,7 +1085,15 @@ networkDnsmasqConfContents(virNetworkObjPtr network,
 virBufferAddLit(, "port=0\n");
 
 if (wantDNS && network->def->dns.forwarders) {
-virBufferAddLit(, "no-resolv\n");
+/* addNoResolv should be set to true if there are any entries
+ * that specify an IP address for requests, but no domain
+ * qualifier (implying that all requests otherwise "unclaimed"
+ * should be sent to that address). if it is still false when
+ * we've looked at all entries, it means we still need the
+ * host's resolv.conf for some cases.
+ */
+bool addNoResolv = false;
+
 for (i = 0; i < network->def->dns.nfwds; i++) {
 virNetworkDNSForwarderPtr fwd = >def->dns.forwarders[i];
 
@@ -1099,11 +1107,15 @@ networkDnsmasqConfContents(virNetworkObjPtr network,
 goto cleanup;
 virBufferAsprintf(, "%s\n", addr);
 VIR_FREE(addr);
+if (!fwd->domain)
+addNoResolv = true;
 } else {
 /* "don't forward requests for this domain" */
 virBufferAddLit(, "#\n");
 }
 }
+if (addNoResolv)
+virBufferAddLit(, "no-resolv\n");
 }
 
 if (network->def->domain) {
diff --git a/tests/networkxml2confdata/nat-network-dns-forwarder-no-resolv.conf 
b/tests/networkxml2confdata/nat-network-dns-forwarder-no-resolv.conf
new file mode 100644
index 000..52d000a
--- /dev/null
+++ b/tests/networkxml2confdata/nat-network-dns-forwarder-no-resolv.conf
@@ -0,0 +1,12 @@
+##WARNING:  THIS IS AN AUTO-GENERATED FILE. CHANGES TO IT ARE LIKELY TO BE
+##OVERWRITTEN AND LOST.  Changes to this configuration should be made using:
+##virsh net-edit default
+## or other application using the libvirt API.
+##
+## dnsmasq conf file created by libvirt
+strict-order
+server=/example.com/192.168.1.1
+except-interface=lo
+bind-dynamic
+interface=virbr0
+addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts
diff --git a/tests/networkxml2confdata/nat-network-dns-forwarder-no-resolv.xml 
b/tests/networkxml2confdata/nat-network-dns-forwarder-no-resolv.xml
new file mode 100644
index 000..9661ce5
--- /dev/null
+++ b/tests/networkxml2confdata/nat-network-dns-forwarder-no-resolv.xml
@@ -0,0 +1,11 @@
+
+  default
+  81ff0d90-c91e-6742-64da-4a736edb9a9c
+  
+  
+  
+
+  
+  
+  
+
diff --git a/tests/networkxml2confdata/nat-network-dns-forwarders.conf 
b/tests/networkxml2confdata/nat-network-dns-forwarders.conf
index 0bd76bf..1b0c94c 100644
--- a/tests/networkxml2confdata/nat-network-dns-forwarders.conf
+++ b/tests/networkxml2confdata/nat-network-dns-forwarders.conf
@@ -5,11 +5,11 @@
 ##
 ## dnsmasq conf file created by libvirt
 strict-order
-no-resolv
 server=8.8.8.8
 server=8.8.4.4
 server=/example.com/192.168.1.1
 server=/www.example.com/#
+no-resolv
 except-interface=lo
 bind-dynamic
 interface=virbr0
diff --git a/tests/networkxml2conftest.c b/tests/networkxml2conftest.c
index 9b61077..e2522fc 100644
--- a/tests/networkxml2conftest.c
+++ b/tests/networkxml2conftest.c
@@ -137,6 +137,7 @@ mymain(void)
 

Re: [libvirt] [PATCH 0/3] log|lock daemon: Don't spam logs with IO error messages after client disconnects

[Peter Krempa]

On Fri, Mar 17, 2017 at 16:15:03 +, Daniel Berrange wrote:
> On Fri, Mar 17, 2017 at 04:48:48PM +0100, Peter Krempa wrote:
> > See patch 3 for explanation
> > 
> > Peter Krempa (3):
> >   rpc: socket: Add possibility to suppress errors on read hangup
> >   rpc: serverclient: Add option to suppress errors on EOF
> >   (log|lock)daemon: Don't spam logs with IO error messages after client
> > disconnects
> 
> ACK, if you add  'ret = -2' in the first patch where mentioned

Thanks, I've tweaked it and pushed.

Peter


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

Re: [libvirt] [PATCH 0/3] log|lock daemon: Don't spam logs with IO error messages after client disconnects

[Daniel P. Berrange]

On Fri, Mar 17, 2017 at 04:48:48PM +0100, Peter Krempa wrote:
> See patch 3 for explanation
> 
> Peter Krempa (3):
>   rpc: socket: Add possibility to suppress errors on read hangup
>   rpc: serverclient: Add option to suppress errors on EOF
>   (log|lock)daemon: Don't spam logs with IO error messages after client
> disconnects

ACK, if you add  'ret = -2' in the first patch where mentioned


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 1/3] rpc: socket: Add possibility to suppress errors on read hangup

[Peter Krempa]

On Fri, Mar 17, 2017 at 16:10:35 +, Daniel Berrange wrote:
> On Fri, Mar 17, 2017 at 04:48:49PM +0100, Peter Krempa wrote:
> > In some cases a read error due to connection hangup is expected. This
> > patch adds a flag that removes the logging of a virError in such case.
> > ---
> >  src/rpc/virnetsocket.c | 33 +++--
> >  src/rpc/virnetsocket.h |  2 ++
> >  2 files changed, 29 insertions(+), 6 deletions(-)

[...]

> > +} else {
> > +if (errout)
> > +virReportSystemError(EIO,
> > + _("End of file while reading data: 
> > %s"),
> > + errout);
> > +else
> > +virReportSystemError(EIO, "%s",
> > + _("End of file while reading data"));
> > +}
> >  ret = -1;
> 
> I'm a little uncomfortable with the idea of returning '-1' without reporting
> an error message. I would suggest returning 0, but we used that to indicate
> EAGAIN condition. Can we at least make it return '-2' as a distinct code
> when we don't report errors.

That is fine by me. The callers that will ignore the error don't really
care why the read failed currently.


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

Re: [libvirt] [PATCH 3/3] (log|lock)daemon: Don't spam logs with IO error messages after client disconnects

[Daniel P. Berrange]

On Fri, Mar 17, 2017 at 04:48:51PM +0100, Peter Krempa wrote:
> The log and lock protocol don't have an extra handshake to close the
> connection. Instead they just close the socket. Unfortunately that
> resulted into a lot of spurious garbage logged to the system log files:
> 
> 2017-03-17 14:00:09.730+: 4714: error : virNetSocketReadWire:1800 : End 
> of file while reading data: Input/output error
> 
> or in the journal as:
> 
> Mar 13 16:19:33  virtlogd[32360]: End of file while reading data: 
> Input/output error
> 
> Use the new facility in the netserverclient to suppress the IO error
> report from the virNetSocket layer.
> ---
>  src/locking/lock_daemon.c | 3 +++
>  src/logging/log_daemon.c  | 3 +++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c
> index 1c94ddd05..12485e966 100644
> --- a/src/locking/lock_daemon.c
> +++ b/src/locking/lock_daemon.c
> @@ -712,6 +712,9 @@ virLockDaemonClientNew(virNetServerClientPtr client,
>  }
>  }
> 
> +/* there's no closing handshake in the locking protocol */
> +virNetServerClientSetQuietEOF(client);
> +
>  return priv;
> 
>   error:
> diff --git a/src/logging/log_daemon.c b/src/logging/log_daemon.c
> index 5997cce9d..d878efa63 100644
> --- a/src/logging/log_daemon.c
> +++ b/src/logging/log_daemon.c
> @@ -572,6 +572,9 @@ virLogDaemonClientNew(virNetServerClientPtr client,
>  }
>  }
> 
> +/* there's no closing handshake in the logging protocol */
> +virNetServerClientSetQuietEOF(client);
> +
>  return priv;
>

Ok, so you're setting this flag server side, and the virNetSocketRead
is triggered by virNetServerClientRead, in turn by n event handler
virNetServerClientDispatchRead. This method doesn't care about the
error message reported by virNetSocketRead at all - it simply marks
the client as needing closing.

So yeah, returning without setting an error is reasonable, though
I'd still prefer to have it return -2 rather than -1 for the case
where no error is reported.

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 1/3] rpc: socket: Add possibility to suppress errors on read hangup

[Daniel P. Berrange]

On Fri, Mar 17, 2017 at 04:48:49PM +0100, Peter Krempa wrote:
> In some cases a read error due to connection hangup is expected. This
> patch adds a flag that removes the logging of a virError in such case.
> ---
>  src/rpc/virnetsocket.c | 33 +++--
>  src/rpc/virnetsocket.h |  2 ++
>  2 files changed, 29 insertions(+), 6 deletions(-)
> 
> diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c
> index 325a7c7cf..4d1dc6446 100644
> --- a/src/rpc/virnetsocket.c
> +++ b/src/rpc/virnetsocket.c
> @@ -82,6 +82,7 @@ struct _virNetSocket {
>  int errfd;
>  bool client;
>  bool ownsFd;
> +bool quietEOF;
> 
>  /* Event callback fields */
>  virNetSocketIOFunc func;
> @@ -1792,12 +1793,18 @@ static ssize_t virNetSocketReadWire(virNetSocketPtr 
> sock, char *buf, size_t len)
>   _("Cannot recv data"));
>  ret = -1;
>  } else if (ret == 0) {
> -if (errout)
> -virReportSystemError(EIO,
> - _("End of file while reading data: %s"), 
> errout);
> -else
> -virReportSystemError(EIO, "%s",
> - _("End of file while reading data"));
> +if (sock->quietEOF) {
> +VIR_DEBUG("socket='%p' EOF while reading: errout='%s'",
> +  socket, NULLSTR(errout));
> +} else {
> +if (errout)
> +virReportSystemError(EIO,
> + _("End of file while reading data: %s"),
> + errout);
> +else
> +virReportSystemError(EIO, "%s",
> + _("End of file while reading data"));
> +}
>  ret = -1;

I'm a little uncomfortable with the idea of returning '-1' without reporting
an error message. I would suggest returning 0, but we used that to indicate
EAGAIN condition. Can we at least make it return '-2' as a distinct code
when we don't report errors.


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 v2 4/5] util: add virNetDevGetName() function

[Laine Stump]

On 03/15/2017 10:45 AM, Cédric Bosdonnat wrote:
> Add a function getting the name of a network interface out of its index.
> ---
>  src/libvirt_private.syms |  1 +
>  src/util/virnetdev.c | 19 +++
>  src/util/virnetdev.h |  2 ++
>  3 files changed, 22 insertions(+)
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 1f25e42d8..0fe88c3fa 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1982,6 +1982,7 @@ virNetDevFeatureTypeToString;
>  virNetDevGetFeatures;
>  virNetDevGetIndex;
>  virNetDevGetLinkInfo;
> +virNetDevGetName;
>  virNetDevGetMAC;
>  virNetDevGetMTU;
>  virNetDevGetOnline;

Oops! I just now noticed (when I ran make check after adding this patch
locally) that you have the ordering mixed up here - virNetDevGetName
should be just after virNetDevGetMTU. Please fix that before pushing.


> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
> index d12324878..91a5274aa 100644
> --- a/src/util/virnetdev.c
> +++ b/src/util/virnetdev.c
> @@ -899,6 +899,25 @@ virNetDevGetRcvAllMulti(const char *ifname,
>  return virNetDevGetIFFlag(ifname, VIR_IFF_ALLMULTI, receive);
>  }
>  
> +char *virNetDevGetName(int ifindex)
> +{
> +char name[IFNAMSIZ];
> +char *ifname = NULL;
> +
> +memset(, 0, sizeof(name));
> +
> +if (!if_indextoname(ifindex, name)) {
> +virReportSystemError(errno,
> + _("Failed to convert interface index %d to a 
> name"),
> + ifindex);
> +goto cleanup;
> +}
> +
> +   ignore_value(VIR_STRDUP(ifname, name));
> +
> + cleanup:
> + return ifname;
> +}
>  
>  /**
>   * virNetDevGetIndex:
> diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h
> index 236cf83ef..01e9c5b95 100644
> --- a/src/util/virnetdev.h
> +++ b/src/util/virnetdev.h
> @@ -157,6 +157,8 @@ int virNetDevSetNamespace(const char *ifname, pid_t 
> pidInNs)
>  int virNetDevSetName(const char *ifname, const char *newifname)
>  ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
>  
> +char *virNetDevGetName(int ifindex)
> +ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
>  int virNetDevGetIndex(const char *ifname, int *ifindex)
>  ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
>  
> 

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

[libvirt] [PATCH 3/3] (log|lock)daemon: Don't spam logs with IO error messages after client disconnects

[Peter Krempa]

The log and lock protocol don't have an extra handshake to close the
connection. Instead they just close the socket. Unfortunately that
resulted into a lot of spurious garbage logged to the system log files:

2017-03-17 14:00:09.730+: 4714: error : virNetSocketReadWire:1800 : End of 
file while reading data: Input/output error

or in the journal as:

Mar 13 16:19:33  virtlogd[32360]: End of file while reading data: 
Input/output error

Use the new facility in the netserverclient to suppress the IO error
report from the virNetSocket layer.
---
 src/locking/lock_daemon.c | 3 +++
 src/logging/log_daemon.c  | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c
index 1c94ddd05..12485e966 100644
--- a/src/locking/lock_daemon.c
+++ b/src/locking/lock_daemon.c
@@ -712,6 +712,9 @@ virLockDaemonClientNew(virNetServerClientPtr client,
 }
 }

+/* there's no closing handshake in the locking protocol */
+virNetServerClientSetQuietEOF(client);
+
 return priv;

  error:
diff --git a/src/logging/log_daemon.c b/src/logging/log_daemon.c
index 5997cce9d..d878efa63 100644
--- a/src/logging/log_daemon.c
+++ b/src/logging/log_daemon.c
@@ -572,6 +572,9 @@ virLogDaemonClientNew(virNetServerClientPtr client,
 }
 }

+/* there's no closing handshake in the logging protocol */
+virNetServerClientSetQuietEOF(client);
+
 return priv;

  error:
-- 
2.12.0

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

[libvirt] [PATCH 1/3] rpc: socket: Add possibility to suppress errors on read hangup

[Peter Krempa]

In some cases a read error due to connection hangup is expected. This
patch adds a flag that removes the logging of a virError in such case.
---
 src/rpc/virnetsocket.c | 33 +++--
 src/rpc/virnetsocket.h |  2 ++
 2 files changed, 29 insertions(+), 6 deletions(-)

diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c
index 325a7c7cf..4d1dc6446 100644
--- a/src/rpc/virnetsocket.c
+++ b/src/rpc/virnetsocket.c
@@ -82,6 +82,7 @@ struct _virNetSocket {
 int errfd;
 bool client;
 bool ownsFd;
+bool quietEOF;

 /* Event callback fields */
 virNetSocketIOFunc func;
@@ -1792,12 +1793,18 @@ static ssize_t virNetSocketReadWire(virNetSocketPtr 
sock, char *buf, size_t len)
  _("Cannot recv data"));
 ret = -1;
 } else if (ret == 0) {
-if (errout)
-virReportSystemError(EIO,
- _("End of file while reading data: %s"), 
errout);
-else
-virReportSystemError(EIO, "%s",
- _("End of file while reading data"));
+if (sock->quietEOF) {
+VIR_DEBUG("socket='%p' EOF while reading: errout='%s'",
+  socket, NULLSTR(errout));
+} else {
+if (errout)
+virReportSystemError(EIO,
+ _("End of file while reading data: %s"),
+ errout);
+else
+virReportSystemError(EIO, "%s",
+ _("End of file while reading data"));
+}
 ret = -1;
 }

@@ -2233,3 +2240,17 @@ void virNetSocketClose(virNetSocketPtr sock)

 virObjectUnlock(sock);
 }
+
+
+/**
+ * virNetSocketSetQuietEOF:
+ * @sock: socket object pointer
+ *
+ * Disables reporting I/O errors as a virError when @socket is closed while
+ * reading data.
+ */
+void
+virNetSocketSetQuietEOF(virNetSocketPtr sock)
+{
+sock->quietEOF = true;
+}
diff --git a/src/rpc/virnetsocket.h b/src/rpc/virnetsocket.h
index 56c75c030..1e75ee62b 100644
--- a/src/rpc/virnetsocket.h
+++ b/src/rpc/virnetsocket.h
@@ -143,6 +143,8 @@ int virNetSocketGetSELinuxContext(virNetSocketPtr sock,
 int virNetSocketSetBlocking(virNetSocketPtr sock,
 bool blocking);

+void virNetSocketSetQuietEOF(virNetSocketPtr sock);
+
 ssize_t virNetSocketRead(virNetSocketPtr sock, char *buf, size_t len);
 ssize_t virNetSocketWrite(virNetSocketPtr sock, const char *buf, size_t len);

-- 
2.12.0

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

[libvirt] [PATCH 0/3] log|lock daemon: Don't spam logs with IO error messages after client disconnects

[Peter Krempa]

See patch 3 for explanation

Peter Krempa (3):
  rpc: socket: Add possibility to suppress errors on read hangup
  rpc: serverclient: Add option to suppress errors on EOF
  (log|lock)daemon: Don't spam logs with IO error messages after client
disconnects

 src/locking/lock_daemon.c|  3 +++
 src/logging/log_daemon.c |  3 +++
 src/rpc/virnetserverclient.c | 13 +
 src/rpc/virnetserverclient.h |  2 ++
 src/rpc/virnetsocket.c   | 33 +++--
 src/rpc/virnetsocket.h   |  2 ++
 6 files changed, 50 insertions(+), 6 deletions(-)

-- 
2.12.0

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

[libvirt] [PATCH 2/3] rpc: serverclient: Add option to suppress errors on EOF

[Peter Krempa]

The protocol may not use an explicit API to close the connection and
just close the socket instead. Add option to suppress errors in such
case.
---
 src/rpc/virnetserverclient.c | 13 +
 src/rpc/virnetserverclient.h |  2 ++
 2 files changed, 15 insertions(+)

diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c
index 81da82cab..85857bc3e 100644
--- a/src/rpc/virnetserverclient.c
+++ b/src/rpc/virnetserverclient.c
@@ -1637,3 +1637,16 @@ virNetServerClientGetInfo(virNetServerClientPtr client,
 virObjectUnlock(client);
 return ret;
 }
+
+
+/**
+ * virNetServerClientSetQuietEOF:
+ *
+ * Don't report errors for protocols that close connection by hangup of the
+ * socket rather than calling an API to close it.
+ */
+void
+virNetServerClientSetQuietEOF(virNetServerClientPtr client)
+{
+virNetSocketSetQuietEOF(client->sock);
+}
diff --git a/src/rpc/virnetserverclient.h b/src/rpc/virnetserverclient.h
index a53cc00b2..e45c78882 100644
--- a/src/rpc/virnetserverclient.h
+++ b/src/rpc/virnetserverclient.h
@@ -152,4 +152,6 @@ int virNetServerClientGetInfo(virNetServerClientPtr client,
   bool *readonly, char **sock_addr,
   virIdentityPtr *identity);

+void virNetServerClientSetQuietEOF(virNetServerClientPtr client);
+
 #endif /* __VIR_NET_SERVER_CLIENT_H__ */
-- 
2.12.0

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

Re: [libvirt] [REPOST PATCH 0/4] Split out storage object into its own module

[Michal Privoznik]

On 03/16/2017 02:07 PM, John Ferlan wrote:

Repost after merging with latest changes of:
http://www.redhat.com/archives/libvir-list/2017-March/msg00305.html

Continuing down the pile of drivers from my RFC for making a common pool
object - we're now at storage...  For reference see patch 3 of:

http://www.redhat.com/archives/libvir-list/2017-February/msg00519.html

This series works through the storage conf adjustments. This pile is
fairly straightforward.  The virStorageVol* API's make it a bit more
interesting, but like other series we split out anything referencing
virStoragePoolObj and go from there.


John Ferlan (4):
  conf: Introduce virstorageobj
  conf: Adjust coding style for storage conf sources
  conf: Alter coding style of storage conf function prototypes
  conf: Use consistent function name prefixes for virstorageobj

 po/POTFILES.in|   1 +
 src/Makefile.am   |   3 +-
 src/conf/storage_conf.c   | 975 ++---
 src/conf/storage_conf.h   | 174 ++--
 src/conf/virstorageobj.c  | 997 ++
 src/conf/virstorageobj.h  | 156 +++
 src/libvirt_private.syms  |  34 +-
 src/storage/storage_backend.h |   2 +-
 src/storage/storage_driver.c  |  24 +-
 src/storage/storage_driver.h  |   2 +-
 src/storage/storage_util.h|   1 -
 src/test/test_driver.c|   1 +
 12 files changed, 1263 insertions(+), 1107 deletions(-)
 create mode 100644 src/conf/virstorageobj.c
 create mode 100644 src/conf/virstorageobj.h



ACK series

Michal

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

Re: [libvirt] [PATCH 11/19] util: new functions virNetDev(Save|Read|Set)NetConfig()

[Daniel P. Berrange]

On Fri, Mar 17, 2017 at 03:40:33PM +0100, Michal Privoznik wrote:
> On 03/17/2017 02:58 PM, Laine Stump wrote:
> > 
> > Why JSON rather than XML though? I don't have a real preference over one
> > or the other, but libvirt lore is *steeped* in XML, and all the other
> > libvirt config files are XML...
> 
> As discussed on IRC, I can write the code to save/parse the JSON here.
> You've done your part. However, I'm not sure I will manage to make it happen
> today, but maybe beginning of the next week if that is okay with you.
> 
> And for the future reference: I prefer JSON over XML because I find it
> producing smaller files in terms of size. And also easier to read by us
> humans at a first glance. These are the reasons I've gone with JSON in NSS
> modules. Unfortunately, we have to stick with XML for out public APIs, but
> for storing some pieces of internal information, we can use other formats
> too.

Agreed, if I were starting libvirt from scratch I don't think we'd use XML,
but rather JSON / YAML in APIs. Given where we are today though, our normal
practice should be to use XML in any public APIs. For stuff not related to
public APIs, we should choose the virConf format or JSON as appropriate.
virConf if a simple flat set of data, JSON for anything needing structure

Definitely don't invent any new text formats, unless it is needed for
interoperability with a pre-existing apps we need to integrate with.

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 0/4] Split out storage object into its own module

[John Ferlan]

[...]

> 
> Sorry, but these patches do not apply cleanly anymore. Should have
> reviewed later. Can you please rebase & resend?
> 
> Michal


I already had done so:

http://www.redhat.com/archives/libvir-list/2017-March/msg00749.html

Just forgot to update this...

John

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

Re: [libvirt] [PATCH 11/19] util: new functions virNetDev(Save|Read|Set)NetConfig()

[Michal Privoznik]

On 03/17/2017 02:58 PM, Laine Stump wrote:

On 03/17/2017 09:32 AM, Michal Privoznik wrote:

On 03/10/2017 09:35 PM, Laine Stump wrote:

These three functions are destined to replace
virNetDev(Replace|Restore)NetConfig() and
virNetDev(Replace|Restore)MacAddress(), which both do the save and set
together as a single step. We need to separate the save, read, and set
steps because there will be situations where we need to do something
else in between (in particular, we will need to rebind a VF's driver
after save but before set).

This patch creates the new functions, but doesn't call them - that
will come in a subsequent patch.
---
 src/libvirt_private.syms |   3 +
 src/util/virnetdev.c | 531
+++
 src/util/virnetdev.h |  22 ++
 3 files changed, 556 insertions(+)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index e9705ae..c983438 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1998,6 +1998,7 @@ virNetDevIfStateTypeFromString;
 virNetDevIfStateTypeToString;
 virNetDevIsVirtualFunction;
 virNetDevPFGetVF;
+virNetDevReadNetConfig;
 virNetDevReplaceMacAddress;
 virNetDevReplaceNetConfig;
 virNetDevRestoreMacAddress;
@@ -2007,11 +2008,13 @@ virNetDevRxFilterFree;
 virNetDevRxFilterModeTypeFromString;
 virNetDevRxFilterModeTypeToString;
 virNetDevRxFilterNew;
+virNetDevSaveNetConfig;
 virNetDevSetMAC;
 virNetDevSetMTU;
 virNetDevSetMTUFromDevice;
 virNetDevSetName;
 virNetDevSetNamespace;
+virNetDevSetNetConfig;
 virNetDevSetOnline;
 virNetDevSetPromiscuous;
 virNetDevSetRcvAllMulti;
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 49a11f3..feb5ba7 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -1899,6 +1899,496 @@ virNetDevRestoreNetConfig(const char *linkdev,
int vf, const char *stateDir)
 return ret;
 }

+
+/**
+ * virNetDevSaveNetConfig:
+ * @linkdev: name of the interface
+ * @vf: vf index if linkdev is a pf
+ * @stateDir: directory to store old net config
+ * @saveVlan: false if we shouldn't attempt to save vlan tag info
+ *(eg for interfaces using 802.1Qbg, since it handles
+ *vlan tags internally)
+ *
+ * Save current MAC address and (if linkdev itself is a VF, or if @vf
+ * >= 0) the "admin MAC address" and vlan tag the device described by
+ * @linkdev:@vf to @stateDir. (the "admin MAC address" is stored in
+ * the PF, and is what the VF MAC will be initialized to the next time
+ * its driver is reloaded (either on host or guest).
+ *
+ * File Name and Format:
+ *
+ *  If the device is a VF and we're allowed to save vlan tag info, the
+ *  file will be named ${pfDevName_vf#{vf} (e.g. "enp2s0f0_vf5") and
+ *  will contain 2 or 3 lines of text:
+ *
+ *  line 1 - admin MAC address
+ *  line 2 - vlan tag
+ *  line 3 - VF MAC address (or missing if VF has no host net
driver)


I'd rather see a JSON format or something. This can bite us in the
future. What are your thoughts?



Every time I touch the code that uses these files I have the same
thought - "%$&*($% this is ugly, and it's bound to cause a headache
someday. We should change it." But that's always a secondary issue about
an existing condition, and I'm chasing something "more important" (and I
figure that when the day comes that we really *need* to switch to a
better format, it will be no more difficult to detect which is being
used than it would be today; that makes it morally easier to procrastinate)

I suppose I can spend some time and write a function that parses
something more expandable, with a fallback to the "old" method based on
what's seen at the beginning of the file.

Why JSON rather than XML though? I don't have a real preference over one
or the other, but libvirt lore is *steeped* in XML, and all the other
libvirt config files are XML...


As discussed on IRC, I can write the code to save/parse the JSON here. 
You've done your part. However, I'm not sure I will manage to make it 
happen today, but maybe beginning of the next week if that is okay with you.


And for the future reference: I prefer JSON over XML because I find it 
producing smaller files in terms of size. And also easier to read by us 
humans at a first glance. These are the reasons I've gone with JSON in 
NSS modules. Unfortunately, we have to stick with XML for out public 
APIs, but for storing some pieces of internal information, we can use 
other formats too.








+ *
+ *  If the device isn't a VF, or we're not allowed to save vlan tag
+ *  info, the file will be named ${linkdev} (e.g. "enp3s0f0") and will
+ *  contain a single line of text containing linkdev's MAC address.
+ *
+ * Returns 0 on success, -1 on failure
+ *
+ */
+int
+virNetDevSaveNetConfig(const char *linkdev, int vf,
+   const char *stateDir,
+   bool saveVlan)
+{
+int ret = -1;
+const char *pfDevName = NULL;
+char *pfDevOrig = NULL;
+char *vfDevOrig = NULL;
+virMacAddr oldMAC 

Re: [libvirt] [PATCH 19/19] util: try *really* hard to set the MAC address of an SRIOV VF

[Laine Stump]

On 03/17/2017 09:32 AM, Michal Privoznik wrote:
> On 03/10/2017 09:35 PM, Laine Stump wrote:
>>   * "administratively set" flag for the VF in the PF's
>> @@ -2109,6 +2172,7 @@ virNetDevSetNetConfig(const char *linkdev, int vf,
>>   cleanup:
>>  VIR_FREE(pfDevOrig);
>>  VIR_FREE(vfDevOrig);
>> +VIR_FREE(vfPCIDevice);
> 
> No. this needs to be virPCIDeviceFree().

Oops! *hides face in shame*

> ACK with that fixed.

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

Re: [libvirt] [PATCH 18/19] util: if setting admin MAC to 00:00:00:00:00:00 fails, try 02:00:00:00:00:00

[Laine Stump]

On 03/17/2017 09:32 AM, Michal Privoznik wrote:
> On 03/10/2017 09:35 PM, Laine Stump wrote:
>> Some PF drivers allow setting the admin MAC (that is the MAC address
>> that the VF will be initialized to the next time the VF's driver is
>> loaded) to 00:00:00:00:00:00, and some don't. Multiple drivers
>> initialize the admin MACs to all 0, but don't allow setting it to that
>> very same value. It has been an uphill battle convincing the driver
>> people that it's reasonable to expect The argument that's used is
>> that an all 0 device MAC address on a device is invalid; however, from
>> an outsider's point of view, when the admin MAC is set to 0 at the
>> time the VF driver is loaded, the VF's MAC is *not* set to 0, but to a
>> random non-0 value. But that's beside the point - even if I could
>> convince one or two SRIOV driver maintainers to permit setting the
>> admin MAC to 0, there are still several other drivers.
>>
>> So rather than fighting that losing battle, this patch checks for a
>> failure to set the admin MAC due to an all 0 value, and retries it
>> with 02:00:00:00:00:00. That won't result in a random value being set
>> in the VF MAC at next VF driver init, but that's okay, because we
>> always want to set a specific value anyway. Rather, the "almost 0"
>> setting makes it easy to visually detect from the output of "ip link
>> show" which VFs are currently in use and which are free.
>> ---
>>  src/util/virnetdev.c | 42 ++
>>  1 file changed, 38 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
>> index 2b1cebc..6cf0463 100644
>> --- a/src/util/virnetdev.c
>> +++ b/src/util/virnetdev.c
>> @@ -1387,6 +1387,14 @@ virNetDevSysfsFile(char **pf_sysfs_device_link
>> ATTRIBUTE_UNUSED,
>>  #if defined(__linux__) && defined(HAVE_LIBNL) && defined(IFLA_VF_MAX)
>>
>>
>> +static virMacAddr zeroMAC = { 0 };
>> +
>> +/* if a net driver doesn't allow setting MAC to all 0, try setting
>> + * to this (the only bit that is set is the "locally administered" bit")
>> + */
>> +static virMacAddr altZeroMAC = { .addr = { 0x02, 0, 0, 0, 0, 0, } };
>> +
>> +
>>  static struct nla_policy ifla_vf_policy[IFLA_VF_MAX+1] = {
>>  [IFLA_VF_MAC]   = { .type = NLA_UNSPEC,
>>  .maxlen = sizeof(struct ifla_vf_mac) },
>> @@ -1397,7 +1405,8 @@ static struct nla_policy
>> ifla_vf_policy[IFLA_VF_MAX+1] = {
>>
>>  static int
>>  virNetDevSetVfConfig(const char *ifname, int vf,
>> - const virMacAddr *macaddr, int vlanid)
>> + const virMacAddr *macaddr, int vlanid,
>> + bool *allowRetry)
>>  {
>>  int rc = -1;
>>  struct nlmsghdr *resp = NULL;
>> @@ -1474,7 +1483,15 @@ virNetDevSetVfConfig(const char *ifname, int vf,
>>  if (resp->nlmsg_len < NLMSG_LENGTH(sizeof(*err)))
>>  goto malformed_resp;
>>
>> -if (err->error) {
>> +/* if allowRetry is true and the error was EINVAL, then
>> + * silently return a failure so the caller can retry with a
>> + * different MAC address
>> + */
>> +if (-err->error == EINVAL && *allowRetry &&
> 
> No, please no. if (err->error == -EINVAL ...) is way better.

Yeah, sure. I just copy-pasta'd that from somewhere else. Consider it done.

> 
>> +macaddr && !virMacAddrCmp(macaddr, )) {
>> +goto cleanup;
>> +} else if (err->error) {
>> +/* other errors are permanent */
>>  char macstr[VIR_MAC_STRING_BUFLEN];
>>
>>  virReportSystemError(-err->error,
> 
> ACK with that fixed.
> 
> Michal
> 

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

Re: [libvirt] [PATCH 08/19] util: new function virPCIDeviceRebind()

[Laine Stump]

On 03/17/2017 09:32 AM, Michal Privoznik wrote:
> On 03/10/2017 09:35 PM, Laine Stump wrote:
>> This function unbinds a device from its driver, then immediately
>> rebinds it to its driver again.
>> ---
>>  src/libvirt_private.syms |  1 +
>>  src/util/virpci.c| 25 +
>>  src/util/virpci.h|  1 +
>>  3 files changed, 27 insertions(+)
>>
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index b44a6ee..ef027cc 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -2217,6 +2217,7 @@ virPCIDeviceListSteal;
>>  virPCIDeviceListStealIndex;
>>  virPCIDeviceNew;
>>  virPCIDeviceReattach;
>> +virPCIDeviceRebind;
>>  virPCIDeviceReset;
>>  virPCIDeviceSetManaged;
>>  virPCIDeviceSetRemoveSlot;
>> diff --git a/src/util/virpci.c b/src/util/virpci.c
>> index 9878398..a007eea 100644
>> --- a/src/util/virpci.c
>> +++ b/src/util/virpci.c
>> @@ -1101,6 +1101,31 @@ virPCIDeviceUnbind(virPCIDevicePtr dev)
>>  return ret;
>>  }
>>
>> +
>> +/**
>> + * virPCIDeviceRebind:
>> + *  @dev: virPCIDevice object describing the device to rebind
>> + *
>> + * unbind a device from its driver, then immediately rebind it.
>> + *
>> + * Returns 0 on success, -1 on failure
>> + */
>> +int virPCIDeviceRebind(virPCIDevicePtr dev)
>> +{
>> +if (virPCIDeviceUnbind(dev) < 0)
>> +return -1;
>> +
>> +if (virFileWriteStr(PCI_SYSFS "drivers_probe", dev->name, 0) < 0) {
>> +virReportSystemError(errno,
>> + _("Failed to trigger a probe for PCI
>> device '%s'"),
>> + dev->name);
>> +return -1;
>> +}
>> +
>> +return 0;
>> +}
>> +
> 
> This is the same code as in virPCIDeviceBindWithDriverOverride(). ACK if
> you replace it with call to this function.

Ah, okay. It took me a second to parse your request - at first I was
thinking "No, it's not the same as virPCIDeviceBindWithDriverOverride()
- there's extra stuff in that function!". Then I realized you meant I
should replace those lines in WithDriverOverride() with a call to
this new function. Sure, that makes sense.

> 
> Michal
> 

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

Re: [libvirt] [PATCH 11/19] util: new functions virNetDev(Save|Read|Set)NetConfig()

[Laine Stump]

On 03/17/2017 09:32 AM, Michal Privoznik wrote:
> On 03/10/2017 09:35 PM, Laine Stump wrote:
>> These three functions are destined to replace
>> virNetDev(Replace|Restore)NetConfig() and
>> virNetDev(Replace|Restore)MacAddress(), which both do the save and set
>> together as a single step. We need to separate the save, read, and set
>> steps because there will be situations where we need to do something
>> else in between (in particular, we will need to rebind a VF's driver
>> after save but before set).
>>
>> This patch creates the new functions, but doesn't call them - that
>> will come in a subsequent patch.
>> ---
>>  src/libvirt_private.syms |   3 +
>>  src/util/virnetdev.c | 531
>> +++
>>  src/util/virnetdev.h |  22 ++
>>  3 files changed, 556 insertions(+)
>>
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index e9705ae..c983438 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -1998,6 +1998,7 @@ virNetDevIfStateTypeFromString;
>>  virNetDevIfStateTypeToString;
>>  virNetDevIsVirtualFunction;
>>  virNetDevPFGetVF;
>> +virNetDevReadNetConfig;
>>  virNetDevReplaceMacAddress;
>>  virNetDevReplaceNetConfig;
>>  virNetDevRestoreMacAddress;
>> @@ -2007,11 +2008,13 @@ virNetDevRxFilterFree;
>>  virNetDevRxFilterModeTypeFromString;
>>  virNetDevRxFilterModeTypeToString;
>>  virNetDevRxFilterNew;
>> +virNetDevSaveNetConfig;
>>  virNetDevSetMAC;
>>  virNetDevSetMTU;
>>  virNetDevSetMTUFromDevice;
>>  virNetDevSetName;
>>  virNetDevSetNamespace;
>> +virNetDevSetNetConfig;
>>  virNetDevSetOnline;
>>  virNetDevSetPromiscuous;
>>  virNetDevSetRcvAllMulti;
>> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
>> index 49a11f3..feb5ba7 100644
>> --- a/src/util/virnetdev.c
>> +++ b/src/util/virnetdev.c
>> @@ -1899,6 +1899,496 @@ virNetDevRestoreNetConfig(const char *linkdev,
>> int vf, const char *stateDir)
>>  return ret;
>>  }
>>
>> +
>> +/**
>> + * virNetDevSaveNetConfig:
>> + * @linkdev: name of the interface
>> + * @vf: vf index if linkdev is a pf
>> + * @stateDir: directory to store old net config
>> + * @saveVlan: false if we shouldn't attempt to save vlan tag info
>> + *(eg for interfaces using 802.1Qbg, since it handles
>> + *vlan tags internally)
>> + *
>> + * Save current MAC address and (if linkdev itself is a VF, or if @vf
>> + * >= 0) the "admin MAC address" and vlan tag the device described by
>> + * @linkdev:@vf to @stateDir. (the "admin MAC address" is stored in
>> + * the PF, and is what the VF MAC will be initialized to the next time
>> + * its driver is reloaded (either on host or guest).
>> + *
>> + * File Name and Format:
>> + *
>> + *  If the device is a VF and we're allowed to save vlan tag info, the
>> + *  file will be named ${pfDevName_vf#{vf} (e.g. "enp2s0f0_vf5") and
>> + *  will contain 2 or 3 lines of text:
>> + *
>> + *  line 1 - admin MAC address
>> + *  line 2 - vlan tag
>> + *  line 3 - VF MAC address (or missing if VF has no host net
>> driver)
> 
> I'd rather see a JSON format or something. This can bite us in the
> future. What are your thoughts?


Every time I touch the code that uses these files I have the same
thought - "%$&*($% this is ugly, and it's bound to cause a headache
someday. We should change it." But that's always a secondary issue about
an existing condition, and I'm chasing something "more important" (and I
figure that when the day comes that we really *need* to switch to a
better format, it will be no more difficult to detect which is being
used than it would be today; that makes it morally easier to procrastinate)

I suppose I can spend some time and write a function that parses
something more expandable, with a fallback to the "old" method based on
what's seen at the beginning of the file.

Why JSON rather than XML though? I don't have a real preference over one
or the other, but libvirt lore is *steeped* in XML, and all the other
libvirt config files are XML...


> 
>> + *
>> + *  If the device isn't a VF, or we're not allowed to save vlan tag
>> + *  info, the file will be named ${linkdev} (e.g. "enp3s0f0") and will
>> + *  contain a single line of text containing linkdev's MAC address.
>> + *
>> + * Returns 0 on success, -1 on failure
>> + *
>> + */
>> +int
>> +virNetDevSaveNetConfig(const char *linkdev, int vf,
>> +   const char *stateDir,
>> +   bool saveVlan)
>> +{
>> +int ret = -1;
>> +const char *pfDevName = NULL;
>> +char *pfDevOrig = NULL;
>> +char *vfDevOrig = NULL;
>> +virMacAddr oldMAC = { 0 };
> 
> This causes a compile error for me. calling memset(, 0,
> sizeof(oldMAC));

Strange. You must be running a newer compiler than me (I'm doing
everything on F25 + updates-testing). I also wonder why I thought I
needed to initialize it, and why I did it that way, since in other cases
I use "= { .addr = { blah } };". (I think most likely at some 

Re: [libvirt] [PATCH 0/4] Split out storage object into its own module

[Michal Privoznik]

On 03/07/2017 10:35 PM, John Ferlan wrote:

Continuing down the pile of drivers from my RFC for making a common pool
object - we're now at storage...  For reference see patch 3 of:

http://www.redhat.com/archives/libvir-list/2017-February/msg00519.html

This series works through the storage conf adjustments. This pile is
fairly straightforward.  The virStorageVol* API's make it a bit more
interesting, but like other series we split out anything referencing
virStoragePoolObj and go from there.

John Ferlan (4):
  conf: Introduce virstorageobj
  conf: Adjust coding style for storage conf sources
  conf: Alter coding style of storage conf function prototypes
  conf: Use consistent function name prefixes for virstorageobj

 po/POTFILES.in|   1 +
 src/Makefile.am   |   3 +-
 src/conf/storage_conf.c   | 965 ++---
 src/conf/storage_conf.h   | 174 ++--
 src/conf/virstorageobj.c  | 986 ++
 src/conf/virstorageobj.h  | 156 +++
 src/libvirt_private.syms  |  34 +-
 src/storage/storage_backend.h |   2 +-
 src/storage/storage_driver.c  |  24 +-
 src/storage/storage_driver.h  |   2 +-
 src/storage/storage_util.h|   1 -
 src/test/test_driver.c|   1 +
 12 files changed, 1255 insertions(+), 1094 deletions(-)
 create mode 100644 src/conf/virstorageobj.c
 create mode 100644 src/conf/virstorageobj.h



Sorry, but these patches do not apply cleanly anymore. Should have 
reviewed later. Can you please rebase & resend?


Michal

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

[libvirt] [PATCH v2 2/2] Add asyncio event loop implementation

[Wojtek Porczyk]

This is usable only on python >= 3.4 (or 3.3 with out-of-tree asyncio),
however it should be harmless for anyone with older python versions.

In simplest case, to have the callbacks queued on the default loop:

>>> import libvirtaio
>>> libvirtaio.virEventRegisterAsyncIOImpl()

The function is not present on non-compatible platforms.

Signed-off-by: Wojtek Porczyk 
---
 libvirt-python.spec.in |   1 +
 libvirtaio.py  | 401 +
 sanitytest.py  |   2 +-
 setup.py   |  12 ++
 4 files changed, 415 insertions(+), 1 deletion(-)
 create mode 100644 libvirtaio.py

diff --git a/libvirt-python.spec.in b/libvirt-python.spec.in
index 3021ebd..0ee535e 100644
--- a/libvirt-python.spec.in
+++ b/libvirt-python.spec.in
@@ -86,6 +86,7 @@ rm -f %{buildroot}%{_libdir}/python*/site-packages/*egg-info
 %defattr(-,root,root)
 %doc ChangeLog AUTHORS NEWS README COPYING COPYING.LESSER examples/
 %{_libdir}/python3*/site-packages/libvirt.py*
+%{_libdir}/python3*/site-packages/libvirtaio.py*
 %{_libdir}/python3*/site-packages/libvirt_qemu.py*
 %{_libdir}/python3*/site-packages/libvirt_lxc.py*
 %{_libdir}/python3*/site-packages/__pycache__/libvirt.cpython-*.py*
diff --git a/libvirtaio.py b/libvirtaio.py
new file mode 100644
index 000..8428f71
--- /dev/null
+++ b/libvirtaio.py
@@ -0,0 +1,401 @@
+#
+# libvirtaio -- asyncio adapter for libvirt
+# Copyright (C) 2017  Wojtek Porczyk 
+#
+# 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
+# .
+#
+
+'''Libvirt event loop implementation using asyncio
+
+Register the implementation of default loop:
+
+>>> import libvirtaio
+>>> libvirtaio.virEventRegisterAsyncIOImpl()
+
+.. seealso::
+https://libvirt.org/html/libvirt-libvirt-event.html
+'''
+
+__author__ = 'Wojtek Porczyk '
+__license__ = 'LGPL-2.1+'
+__all__ = ['virEventAsyncIOImpl', 'virEventRegisterAsyncIOImpl']
+
+import asyncio
+import itertools
+import logging
+import warnings
+
+import libvirt
+
+try:
+from asyncio import ensure_future
+except ImportError:
+from asyncio import async as ensure_future
+
+
+class Callback(object):
+'''Base class for holding callback
+
+:param virEventAsyncIOImpl impl: the implementation in which we run
+:param cb: the callback itself
+:param opaque: the opaque tuple passed by libvirt
+'''
+# pylint: disable=too-few-public-methods
+
+_iden_counter = itertools.count()
+
+def __init__(self, impl, cb, opaque, *args, **kwargs):
+super().__init__(*args, **kwargs)
+self.iden = next(self._iden_counter)
+self.impl = impl
+self.cb = cb
+self.opaque = opaque
+
+assert self.iden not in self.impl.callbacks, \
+'found {} callback: {!r}'.format(
+self.iden, self.impl.callbacks[self.iden])
+self.impl.callbacks[self.iden] = self
+
+def __repr__(self):
+return '<{} iden={}>'.format(self.__clas__.__name__, self.iden)
+
+def close(self):
+'''Schedule *ff* callback'''
+self.impl.log.debug('callback %d close(), scheduling ff', self.iden)
+self.impl.schedule_ff_callback(self.opaque)
+
+#
+# file descriptors
+#
+
+class Descriptor(object):
+'''Manager of one file descriptor
+
+:param virEventAsyncIOImpl impl: the implementation in which we run
+:param int fd: the file descriptor
+'''
+def __init__(self, impl, fd):
+self.impl = impl
+self.fd = fd
+self.callbacks = {}
+
+def _handle(self, event):
+'''Dispatch the event to the descriptors
+
+:param int event: The event (from libvirt's constants) being dispatched
+'''
+for callback in self.callbacks.values():
+if callback.event is not None and callback.event & event:
+callback.cb(callback.iden, self.fd, event, callback.opaque)
+
+def update(self):
+'''Register or unregister callbacks at event loop
+
+This should be called after change of any ``.event`` in callbacks.
+'''
+# It seems like loop.add_{reader,writer} can be run multiple times
+# and will still register the callback only once. Likewise,
+# remove_{reader,writer} may be run even if the reader/writer
+# is not 

[libvirt] [PATCH v2 1/2] Allow for ff callbacks to be called by custom event implementations

[Wojtek Porczyk]

The documentation says:
> If the opaque user data requires free'ing when the handle is
> unregistered, then a 2nd callback can be supplied for this purpose.
> This callback needs to be invoked from a clean stack. If 'ff'
> callbacks are invoked directly from the virEventRemoveHandleFunc they
> will likely deadlock in libvirt.

And they did deadlock. In removeTimeout too. Now we supply a custom
function to pick it from the opaque blob and fire.

Signed-off-by: Wojtek Porczyk 
---
 libvirt-override.c  | 36 
 libvirt-override.py | 39 +++
 sanitytest.py   |  5 +++--
 3 files changed, 54 insertions(+), 26 deletions(-)

diff --git a/libvirt-override.c b/libvirt-override.c
index 9e40f00..37f7ee2 100644
--- a/libvirt-override.c
+++ b/libvirt-override.c
@@ -5223,6 +5223,9 @@ libvirt_virEventAddHandleFunc(int fd,
 
 VIR_PY_TUPLE_SET_GOTO(pyobj_args, 3, cb_args, cleanup);
 
+/* If changing contents of the opaque object, please also change
+ * virEventExecuteFFCallback() in libvirt-override.py
+ */
 VIR_PY_TUPLE_SET_GOTO(cb_args, 0, libvirt_virEventHandleCallbackWrap(cb), 
cleanup);
 VIR_PY_TUPLE_SET_GOTO(cb_args, 1, libvirt_virVoidPtrWrap(opaque), cleanup);
 VIR_PY_TUPLE_SET_GOTO(cb_args, 2, libvirt_virFreeCallbackWrap(ff), 
cleanup);
@@ -5292,20 +5295,11 @@ libvirt_virEventRemoveHandleFunc(int watch)
 VIR_PY_TUPLE_SET_GOTO(pyobj_args, 0, libvirt_intWrap(watch), cleanup);
 
 result = PyEval_CallObject(removeHandleObj, pyobj_args);
-if (!result) {
+if (result) {
+retval = 0;
+} else {
 PyErr_Print();
 PyErr_Clear();
-} else if (!PyTuple_Check(result) || PyTuple_Size(result) != 3) {
-DEBUG("%s: %s must return opaque obj registered with %s"
-  "to avoid leaking libvirt memory\n",
-  __FUNCTION__, NAME(removeHandle), NAME(addHandle));
-} else {
-opaque = PyTuple_GetItem(result, 1);
-ff = PyTuple_GetItem(result, 2);
-cff = PyvirFreeCallback_Get(ff);
-if (cff)
-(*cff)(PyvirVoidPtr_Get(opaque));
-retval = 0;
 }
 
  cleanup:
@@ -5350,6 +5344,9 @@ libvirt_virEventAddTimeoutFunc(int timeout,
 
 VIR_PY_TUPLE_SET_GOTO(pyobj_args, 2, cb_args, cleanup);
 
+/* If changing contents of the opaque object, please also change
+ * virEventExecuteFFCallback() in libvirt-override.py
+ */
 VIR_PY_TUPLE_SET_GOTO(cb_args, 0, libvirt_virEventTimeoutCallbackWrap(cb), 
cleanup);
 VIR_PY_TUPLE_SET_GOTO(cb_args, 1, libvirt_virVoidPtrWrap(opaque), cleanup);
 VIR_PY_TUPLE_SET_GOTO(cb_args, 2, libvirt_virFreeCallbackWrap(ff), 
cleanup);
@@ -5416,20 +5413,11 @@ libvirt_virEventRemoveTimeoutFunc(int timer)
 VIR_PY_TUPLE_SET_GOTO(pyobj_args, 0, libvirt_intWrap(timer), cleanup);
 
 result = PyEval_CallObject(removeTimeoutObj, pyobj_args);
-if (!result) {
+if (result) {
+retval = 0;
+} else {
 PyErr_Print();
 PyErr_Clear();
-} else if (!PyTuple_Check(result) || PyTuple_Size(result) != 3) {
-DEBUG("%s: %s must return opaque obj registered with %s"
-  "to avoid leaking libvirt memory\n",
-  __FUNCTION__, NAME(removeTimeout), NAME(addTimeout));
-} else {
-opaque = PyTuple_GetItem(result, 1);
-ff = PyTuple_GetItem(result, 2);
-cff = PyvirFreeCallback_Get(ff);
-if (cff)
-(*cff)(PyvirVoidPtr_Get(opaque));
-retval = 0;
 }
 
  cleanup:
diff --git a/libvirt-override.py b/libvirt-override.py
index 63f8ecb..3d09d63 100644
--- a/libvirt-override.py
+++ b/libvirt-override.py
@@ -16,6 +16,7 @@ except ImportError:
 if str(cyg_e).count("No module named"):
 raise lib_e
 
+import ctypes
 import types
 
 # The root of all libvirt errors.
@@ -211,3 +212,41 @@ def virEventAddTimeout(timeout, cb, opaque):
 ret = libvirtmod.virEventAddTimeout(timeout, cbData)
 if ret == -1: raise libvirtError ('virEventAddTimeout() failed')
 return ret
+
+
+#
+# a caller for the ff callbacks for custom event loop implementations
+#
+
+ctypes.pythonapi.PyCapsule_GetPointer.restype = ctypes.c_void_p
+ctypes.pythonapi.PyCapsule_GetPointer.argtypes = (
+ctypes.py_object, ctypes.c_char_p)
+
+_virFreeCallback = ctypes.CFUNCTYPE(None, ctypes.c_void_p)
+
+def virEventExecuteFFCallback(opaque):
+"""
+Execute callback which frees the opaque buffer
+
+@opaque: the opaque object passed to addHandle or addTimeout
+
+WARNING: This function should not be called from any call by libvirt's
+core. It will most probably cause deadlock in C-level libvirt code.
+Instead it should be scheduled and called from implementation's stack.
+
+See 
https://libvirt.org/html/libvirt-libvirt-event.html#virEventAddHandleFunc
+for more information.
+
+This function is not dependent on any event loop implementation.
+

[libvirt] [PATCH v2 0/2] libvirt-python: libvirtaio

[Wojtek Porczyk]

Hello, libvirt-list,

This is second attempt at merging libvirtaio, an event loop implementation
which dispatches the callbacks via asyncio's event loop.

The first patch fixes the bug around freeing opaque objects [1][2], and the
second one is the actual implementation.

Since v1 series, as per Daniel Berrange's notes, the second patch has licence
comment changed to LGPL-2.1+ and there is no import into main libvirt module.
The first patch is unchanged.

[1] https://www.redhat.com/archives/libvir-list/2017-January/msg00863.html
[2] https://bugzilla.redhat.com/show_bug.cgi?id=1433028

Wojtek Porczyk (2):
  Allow for ff callbacks to be called by custom event implementations
  Add asyncio event loop implementation

 libvirt-override.c |  36 ++---
 libvirt-override.py|  39 +
 libvirt-python.spec.in |   1 +
 libvirtaio.py  | 401 +
 sanitytest.py  |   5 +-
 setup.py   |  12 ++
 6 files changed, 468 insertions(+), 26 deletions(-)
 create mode 100644 libvirtaio.py

-- 
pozdrawiam / best regards   _.-._
Wojtek Porczyk   .-^'   '^-.
Invisible Things Lab |'-.-^-.-'|
 |  |   |  |
 I do not fear computers,|  '-.-'  |
 I fear lack of them.'-._ :  ,-'
-- Isaac Asimov `^-^-_>


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

Re: [libvirt] [PATCH 09/19] util: new internal function to permit silent failure of virNetDevSetMAC()

[Michal Privoznik]

On 03/10/2017 09:35 PM, Laine Stump wrote:

We will want to allow silent failure of virNetDevSetMAC() in the case
that the SIOSIFHWADDR ioctl fails with errno == EADDRNOTAVAIL. (Yes,
that is very specific, but we really *do* want a logged failure in all
other circumstances, and don't want to duplicate code in the caller
for the other possibilities).

This patch renames the 3 different virNetDevSetMAC() functions to
virNetDevSetMACInternal(), adding a 3rd arg called "quiet" and making
them static (because this extra control will only be needed within
virnetdev.c). A new global virNetDevSetMAC() is defined that calls
whichever of the three *Internal() functions gets compiled with quiet
= false. Callers in virnetdev.c that want to notice a failure with
errno == EADDRNOTAVAIL and retry with a different strategy rather than
immediately failing, can call virNetDevSetMACInternal(..., true).
---
 src/util/virnetdev.c | 51 +--
 1 file changed, 41 insertions(+), 10 deletions(-)

diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 766638d..ffc2fb4 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -222,17 +222,20 @@ int virNetDevExists(const char *ifname)
 #if defined(SIOCGIFHWADDR) && defined(SIOCSIFHWADDR) && \
 defined(HAVE_STRUCT_IFREQ)
 /**
- * virNetDevSetMAC:
+ * virNetDevSetMACInternal:
  * @ifname: interface name to set MTU for
  * @macaddr: MAC address
+ * @quiet: true if a failure to set MAC address with errno == EADDRNOTAVAIL
+ * should be silent (still returns error, but without log)
  *
- * This function sets the @macaddr for a given interface @ifname. This
- * gets rid of the kernel's automatically assigned random MAC.
+ * This function sets the @macaddr for a given interface @ifname.
  *
  * Returns 0 in case of success or -1 on failure
  */
-int virNetDevSetMAC(const char *ifname,
-const virMacAddr *macaddr)
+static int
+virNetDevSetMACInternal(const char *ifname,
+const virMacAddr *macaddr,
+bool quiet)
 {
 int fd = -1;
 int ret = -1;
@@ -254,6 +257,9 @@ int virNetDevSetMAC(const char *ifname,
 if (ioctl(fd, SIOCSIFHWADDR, ) < 0) {
 char macstr[VIR_MAC_STRING_BUFLEN];

+if (quiet && errno == EADDRNOTAVAIL)
+goto cleanup;
+
 virReportSystemError(errno,
  _("Cannot set interface MAC to %s on '%s'"),
  virMacAddrFormat(macaddr, macstr), ifname);


Frankly, I like functions with quiet = true to be really silent. But I 
don't care that much.


ACK

Michal

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

Re: [libvirt] [PATCH 00/19] Fix saving/setting/restoring SR-IOV VF MAC address

[Michal Privoznik]

On 03/10/2017 09:34 PM, Laine Stump wrote:

There are multiple bugs filed against both libvirt and the kernel
related to incorrect restoration of MAC addresses for SR-IOV VFs, both
when used via VFIO device assignment and when used for macvtap
passthrough mode

https://bugzilla.redhat.com/1415609 - libvirt
https://bugzilla.redhat.com/1341248 - kernel (igb)
https://bugzilla.redhat.com/1415609 - kernel (ixgbe)

https://bugzilla.redhat.com/1302166 - kernel (mlx, fixed)

Beyond that, there are other problems with incorrect setting and
restoration of VF MAC addresses that don't have their own bug reports
(although they may be partially described in the comments of the BZes
mentioned above). This patch series attempts (and I hope succeeds!) to
fix all of these problems, which can be summarized in these three
points:

* The VF's own MAC address was not being set prior to using it for
  macvtap passthrough mode. Instead, due to serious misconception on
  my part, the "admin MAC" for that VF was set in the PF. The problem
  is that the admin MAC isn't put into use on the VF immediately;
  rather it is unused until the next time the VF driver is initialized
  (on either the host or in a guest) so setting it had no practical
  effect, meaning that the macvtap device's MAC didn't match the MAC
  of the VF device it was attached to - this meant that traffic
  wouldn't pass unless the device was in promiscuous mode (and
  apparently it was back when the code was changed to behave this
  way?).

* The VF's own MAC address was never restored after it had been used
  (for both VFIO device assignment and for macvtap passthrough mode).
  Only the "admin MAC" address (which, again, won't take effect
  until/unless the VF driver is reloaded/reinitialized) was restored.

* If the original admin MAC was 00:00:00:00:00:00, libvirt would save
  that, then attempt to restore it when the guest was finished with
  the device, but for some/many SRIOV-capable net drivers, an all 0
  MAC is not allowed to be set (even though that is its initial
  value). This would result in the MAC not being changed (of course,
  since this is the admin MAC, that didn't actually matter, but the
  combination of the error message and the VF's own MAC still being
  set to the value used by the guest, users mistakenly believed this
  was the source of networking problems (e.g. when the guest moved to
  another host, but the old host still had an interface showing its
  MAC address).

There are lots of details in the patches. 01 - 07 just clean up and
slightly modify some existing utility functions. 08 - 11 define some
functions to save/restore netdev MAC/vlan settings, and 12-14 change
the existing setup/teardown code for macvtap and hostdev to use the
new functions, then 15 and 17-19 modify their behavior further, while
16 just removes functions that are no longer used.

In the end, the VF's own MAC *and* admin MAC are saved for all SRIOV
VFs when we begin using a VF, and the VF's MAC is restored when
finished. Since the admin MAC doesn't actually have any immediate
practical effect, we don't concern ourselves with assuring it is
restored in all cases (in particular, when use use the VF for VFIO
assignment, we only restore the VF's MAC, but not the admin MAC during
teardown, and when using the VF for macvtap passthrough we avoid
restoring the admin MAC because that would unnecessarily turn on the
"administratively set" flag in the PF driver (described in the commit
log for one of these patches). I might decide to fix the former later,
but for now it just unnecessarily complicates the code for no real
gain).


Laine Stump (19):
  util: permit querying a VF MAC address or VLAN tag by itself
  util: remove unused args from virNetDevSetVfConfig()
  util: use cleanup label consistently in virHostdevNetConfigReplace()
  util: eliminate useless local variable
  util: make virMacAddrParse more versatile
  util: change virPCIGetNetName() to not return error if device has no
net name
  util: make virPCIGetDeviceAddressFromSysfsLink() public
  util: new function virPCIDeviceRebind()
  util: new internal function to permit silent failure of
virNetDevSetMAC()
  util: new function virNetDevPFGetVF()
  util: new functions virNetDev(Save|Read|Set)NetConfig()
  util: use new virNetDev*NetConfig() functions for macvtap
setup/teardown
  util: use new virNetDev*NetConfig() functions for hostdev
setup/teardown
  util: replace virHostdevNetConfigReplace with ...(Save|Set)NetConfig()
  util: save hostdev network device config before unbinding from host
driver
  util: after hostdev assignment, restore VF MAC address via setting
admin MAC
  util: remove unused functions from virnetdev.c
  util: if setting admin MAC to 00:00:00:00:00:00 fails, try
02:00:00:00:00:00
  util: try *really* hard to set the MAC address of an SRIOV VF

 src/libvirt_private.syms|  10 +-
 src/util/virhostdev.c   | 171 ++--
 src/util/virmacaddr.c   |   2 +-
 

Re: [libvirt] [PATCH 10/19] util: new function virNetDevPFGetVF()

[Michal Privoznik]

On 03/10/2017 09:35 PM, Laine Stump wrote:

Given an SRIOV PF netdev name (e.g. "enp2s0f0") and VF#, this new
function returns the netdev name of the referenced VF device
(e.g. "enp2s11f6"), or NULL if the device isn't bound to a net driver.
---
 src/libvirt_private.syms |  1 +
 src/util/virnetdev.c | 58 
 src/util/virnetdev.h |  3 +++
 3 files changed, 62 insertions(+)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index ef027cc..e9705ae 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1997,6 +1997,7 @@ virNetDevGetVLanID;
 virNetDevIfStateTypeFromString;
 virNetDevIfStateTypeToString;
 virNetDevIsVirtualFunction;
+virNetDevPFGetVF;
 virNetDevReplaceMacAddress;
 virNetDevReplaceNetConfig;
 virNetDevRestoreMacAddress;
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index ffc2fb4..49a11f3 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -1311,6 +1311,54 @@ virNetDevGetPhysicalFunction(const char *ifname, char 
**pfname)
 return ret;
 }

+
+/**
+ * virNetDevPFGetVF:
+ *
+ * @pfname: netdev name of the physical function (PF)
+ * @vf: virtual function (VF) number for the device of interest
+ * @vfname: name of the physical function interface name
+ *
+ * Finds the netdev name of VF# @vf of SRIOV PF @pfname, and puts it
+ * in @vfname. The caller must free @vfname when it's finished with
+ * it.
+ *
+ * Returns 0 on success, -1 on failure


Not entirely true. After your patch of 06/19 virPCIGetNetName() can 
return 0 (which in turn is the return value of virNetDevPFGetVF()), and 
still not fetch vfname - if virDirOpenQuiet() fails. I'd recommend to at 
least note this in the comment.



+ *
+ */
+int
+virNetDevPFGetVF(const char *pfname, int vf, char **vfname)
+{
+char *virtfnName = NULL;
+char *virtfnSysfsPath = NULL;
+int ret = -1;
+
+if (virAsprintf(, "virtfn%d", vf) < 0)
+goto cleanup;
+
+/* this provides the path to the VF's directory in sysfs,
+ * e.g. "/sys/class/net/enp2s0f0/virtfn3"
+ */
+if (virNetDevSysfsDeviceFile(, pfname, virtfnName) < 0)
+goto cleanup;
+
+/* and this gets the netdev name associated with it, which is a
+ * directory entry in [virtfnSysfsPath]/net,
+ * e.g. "/sys/class/net/enp2s0f0/virtfn3/net/enp2s11f4" - in this
+ * example the VF for enp2s0f0 vf#3 is "enp2s11f4". (If the VF
+ * isn't bound to a netdev driver, it won't have a netdev name,
+ * and vfname will be NULL).
+ */
+ret = virPCIGetNetName(virtfnSysfsPath, vfname);
+
+ cleanup:
+VIR_FREE(virtfnName);
+VIR_FREE(virtfnSysfsPath);
+
+return ret;
+}
+
+


ACK with that fixed.

Michal

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

Re: [libvirt] [PATCH 19/19] util: try *really* hard to set the MAC address of an SRIOV VF

[Michal Privoznik]

On 03/10/2017 09:35 PM, Laine Stump wrote:

If an SRIOV VF has previously been used for VFIO device assignment,
the "admin MAC" that is stored in the PF driver's table of VF info
will have been set to the MAC address that the virtual machine wanted
the device to have. Setting the admin MAC for a VF also sets a flag in
the PF that is loosely called the "administratively set" flag. Once
that flag is set, it is no longer possible for the net driver of the
VF (either on the host or in a virtual machine) to directly set the
VF's MAC again; this flag isn't reset until the *PF* driver is
restarted, and that requires taking *all* VFs offline, so it's not
really feasible to do.

If the same SRIOV VF is later used for macvtap passthrough mode, the
VF's MAC address must be set, but normally we don't unbind the VF from
its host net driver (since we actually need the host net driver in
this case). Since setting the VF MAC directly will fail, in the past
"we" ("I") had tried to fix the problem by simply setting the admin MAC
(via the PF) instead. This *appeared* to work (and might have at one
time, due to promiscuous mode being turned on somewhere or something),
but it currently creates a non-working interface because only the
value for admin MAC is set to the desired value, *not* the actual MAC
that the VF is using.

Earlier patches in this series reverted that behavior, so that we once
again set the MAC of the VF itself for macvtap passthrough operation,
not the admin MAC. But that brings back the original bug - if the
interface has been used for VFIO device assignment, you can no longer
use it for macvtap passthrough.

This patch solves that problem by noticing when virNetDevSetMAC()
fails for a VF, and in that case it sets the desired MAC to the admin
MAC via the PF, then "bounces" the VF driver (by unbinding and the
immediately rebinding it to the VF). This causes the VF's MAC to be
reinitialized from the admin MAC, and everybody is happy (until the
*next* time someone wants to set the VF's MAC address, since the
"administratively set" bit is still turned on).
---
 src/util/virnetdev.c | 102 +--
 1 file changed, 83 insertions(+), 19 deletions(-)

diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 6cf0463..861d725 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -1053,6 +1053,32 @@ virNetDevSysfsDeviceFile(char **pf_sysfs_device_link, 
const char *ifname,
 return 0;
 }

+
+static virPCIDevicePtr
+virNetDevGetPCIDevice(const char *devName)
+{
+char *vfSysfsDevicePath = NULL;
+virPCIDeviceAddressPtr vfPCIAddr = NULL;
+virPCIDevicePtr vfPCIDevice = NULL;
+
+if (virNetDevSysfsFile(, devName, "device") < 0)
+goto cleanup;
+
+vfPCIAddr = virPCIGetDeviceAddressFromSysfsLink(vfSysfsDevicePath);
+if (!vfPCIAddr)
+goto cleanup;
+
+vfPCIDevice = virPCIDeviceNew(vfPCIAddr->domain, vfPCIAddr->bus,
+  vfPCIAddr->slot, vfPCIAddr->function);
+
+ cleanup:
+VIR_FREE(vfSysfsDevicePath);
+VIR_FREE(vfPCIAddr);
+
+return vfPCIDevice;
+}
+
+
 /**
  * virNetDevGetVirtualFunctions:
  *
@@ -1942,6 +1968,7 @@ virNetDevSetNetConfig(const char *linkdev, int vf,
 char *pfDevOrig = NULL;
 char *vfDevOrig = NULL;
 int vlanTag = -1;
+virPCIDevicePtr vfPCIDevice = NULL;

 if (vf >= 0) {
 /* linkdev is the PF */
@@ -2037,6 +2064,8 @@ virNetDevSetNetConfig(const char *linkdev, int vf,
 }

 if (MAC) {
+int setMACrc;
+
 if (!linkdev) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("VF %d of PF '%s' is not bound to a net driver, "
@@ -2045,27 +2074,61 @@ virNetDevSetNetConfig(const char *linkdev, int vf,
 goto cleanup;
 }

-if (virNetDevSetMAC(linkdev, MAC) < 0) {
-/* This may have failed due to the "administratively
- * set" flag being set in the PF for this VF. For now
- * we will just fail, but in the future we should
- * attempt to set the VF MAC via the PF.
+setMACrc = virNetDevSetMACInternal(linkdev, MAC, !!pfDevOrig);
+if (setMACrc < 0) {
+bool allowRetry = false;
+int retries = 100;
+
+/* if pfDevOrig == NULL, this isn't a VF, so we've failed */
+if (!pfDevOrig || errno != EADDRNOTAVAIL)
+goto cleanup;
+
+/* Otherwise this is a VF, and virNetDevSetMAC failed with
+ * EADDRNOTAVAIL, which could be due to the
+ * "administratively set" flag being set in the PF for
+ * this VF.  When this happens, we can attempt to use an
+ * alternate method to set the VF MAC: first set it into
+ * the admin MAC for this VF in the PF, then unbind/rebind
+ * the VF from its net driver. This causes the VF's MAC to
+ * be initialized to whatever was stored in the admin 

Re: [libvirt] [PATCH 11/19] util: new functions virNetDev(Save|Read|Set)NetConfig()

[Michal Privoznik]

On 03/10/2017 09:35 PM, Laine Stump wrote:

These three functions are destined to replace
virNetDev(Replace|Restore)NetConfig() and
virNetDev(Replace|Restore)MacAddress(), which both do the save and set
together as a single step. We need to separate the save, read, and set
steps because there will be situations where we need to do something
else in between (in particular, we will need to rebind a VF's driver
after save but before set).

This patch creates the new functions, but doesn't call them - that
will come in a subsequent patch.
---
 src/libvirt_private.syms |   3 +
 src/util/virnetdev.c | 531 +++
 src/util/virnetdev.h |  22 ++
 3 files changed, 556 insertions(+)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index e9705ae..c983438 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1998,6 +1998,7 @@ virNetDevIfStateTypeFromString;
 virNetDevIfStateTypeToString;
 virNetDevIsVirtualFunction;
 virNetDevPFGetVF;
+virNetDevReadNetConfig;
 virNetDevReplaceMacAddress;
 virNetDevReplaceNetConfig;
 virNetDevRestoreMacAddress;
@@ -2007,11 +2008,13 @@ virNetDevRxFilterFree;
 virNetDevRxFilterModeTypeFromString;
 virNetDevRxFilterModeTypeToString;
 virNetDevRxFilterNew;
+virNetDevSaveNetConfig;
 virNetDevSetMAC;
 virNetDevSetMTU;
 virNetDevSetMTUFromDevice;
 virNetDevSetName;
 virNetDevSetNamespace;
+virNetDevSetNetConfig;
 virNetDevSetOnline;
 virNetDevSetPromiscuous;
 virNetDevSetRcvAllMulti;
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 49a11f3..feb5ba7 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -1899,6 +1899,496 @@ virNetDevRestoreNetConfig(const char *linkdev, int vf, 
const char *stateDir)
 return ret;
 }

+
+/**
+ * virNetDevSaveNetConfig:
+ * @linkdev: name of the interface
+ * @vf: vf index if linkdev is a pf
+ * @stateDir: directory to store old net config
+ * @saveVlan: false if we shouldn't attempt to save vlan tag info
+ *(eg for interfaces using 802.1Qbg, since it handles
+ *vlan tags internally)
+ *
+ * Save current MAC address and (if linkdev itself is a VF, or if @vf
+ * >= 0) the "admin MAC address" and vlan tag the device described by
+ * @linkdev:@vf to @stateDir. (the "admin MAC address" is stored in
+ * the PF, and is what the VF MAC will be initialized to the next time
+ * its driver is reloaded (either on host or guest).
+ *
+ * File Name and Format:
+ *
+ *  If the device is a VF and we're allowed to save vlan tag info, the
+ *  file will be named ${pfDevName_vf#{vf} (e.g. "enp2s0f0_vf5") and
+ *  will contain 2 or 3 lines of text:
+ *
+ *  line 1 - admin MAC address
+ *  line 2 - vlan tag
+ *  line 3 - VF MAC address (or missing if VF has no host net driver)


I'd rather see a JSON format or something. This can bite us in the 
future. What are your thoughts?



+ *
+ *  If the device isn't a VF, or we're not allowed to save vlan tag
+ *  info, the file will be named ${linkdev} (e.g. "enp3s0f0") and will
+ *  contain a single line of text containing linkdev's MAC address.
+ *
+ * Returns 0 on success, -1 on failure
+ *
+ */
+int
+virNetDevSaveNetConfig(const char *linkdev, int vf,
+   const char *stateDir,
+   bool saveVlan)
+{
+int ret = -1;
+const char *pfDevName = NULL;
+char *pfDevOrig = NULL;
+char *vfDevOrig = NULL;
+virMacAddr oldMAC = { 0 };


This causes a compile error for me. calling memset(, 0, 
sizeof(oldMAC)); solved it for me. Moreover, this variable will always 
be set before use. So your call wether to leave the initialization in or 
out.



+char MACStr[VIR_MAC_STRING_BUFLEN];
+int oldVlanTag = -1;
+char *filePath = NULL;
+char *fileStr = NULL;
+virBuffer buf = VIR_BUFFER_INITIALIZER;


Michal

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

Re: [libvirt] [PATCH 18/19] util: if setting admin MAC to 00:00:00:00:00:00 fails, try 02:00:00:00:00:00

[Michal Privoznik]

On 03/10/2017 09:35 PM, Laine Stump wrote:

Some PF drivers allow setting the admin MAC (that is the MAC address
that the VF will be initialized to the next time the VF's driver is
loaded) to 00:00:00:00:00:00, and some don't. Multiple drivers
initialize the admin MACs to all 0, but don't allow setting it to that
very same value. It has been an uphill battle convincing the driver
people that it's reasonable to expect The argument that's used is
that an all 0 device MAC address on a device is invalid; however, from
an outsider's point of view, when the admin MAC is set to 0 at the
time the VF driver is loaded, the VF's MAC is *not* set to 0, but to a
random non-0 value. But that's beside the point - even if I could
convince one or two SRIOV driver maintainers to permit setting the
admin MAC to 0, there are still several other drivers.

So rather than fighting that losing battle, this patch checks for a
failure to set the admin MAC due to an all 0 value, and retries it
with 02:00:00:00:00:00. That won't result in a random value being set
in the VF MAC at next VF driver init, but that's okay, because we
always want to set a specific value anyway. Rather, the "almost 0"
setting makes it easy to visually detect from the output of "ip link
show" which VFs are currently in use and which are free.
---
 src/util/virnetdev.c | 42 ++
 1 file changed, 38 insertions(+), 4 deletions(-)

diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 2b1cebc..6cf0463 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -1387,6 +1387,14 @@ virNetDevSysfsFile(char **pf_sysfs_device_link 
ATTRIBUTE_UNUSED,
 #if defined(__linux__) && defined(HAVE_LIBNL) && defined(IFLA_VF_MAX)


+static virMacAddr zeroMAC = { 0 };
+
+/* if a net driver doesn't allow setting MAC to all 0, try setting
+ * to this (the only bit that is set is the "locally administered" bit")
+ */
+static virMacAddr altZeroMAC = { .addr = { 0x02, 0, 0, 0, 0, 0, } };
+
+
 static struct nla_policy ifla_vf_policy[IFLA_VF_MAX+1] = {
 [IFLA_VF_MAC]   = { .type = NLA_UNSPEC,
 .maxlen = sizeof(struct ifla_vf_mac) },
@@ -1397,7 +1405,8 @@ static struct nla_policy ifla_vf_policy[IFLA_VF_MAX+1] = {

 static int
 virNetDevSetVfConfig(const char *ifname, int vf,
- const virMacAddr *macaddr, int vlanid)
+ const virMacAddr *macaddr, int vlanid,
+ bool *allowRetry)
 {
 int rc = -1;
 struct nlmsghdr *resp = NULL;
@@ -1474,7 +1483,15 @@ virNetDevSetVfConfig(const char *ifname, int vf,
 if (resp->nlmsg_len < NLMSG_LENGTH(sizeof(*err)))
 goto malformed_resp;

-if (err->error) {
+/* if allowRetry is true and the error was EINVAL, then
+ * silently return a failure so the caller can retry with a
+ * different MAC address
+ */
+if (-err->error == EINVAL && *allowRetry &&


No, please no. if (err->error == -EINVAL ...) is way better.


+macaddr && !virMacAddrCmp(macaddr, )) {
+goto cleanup;
+} else if (err->error) {
+/* other errors are permanent */
 char macstr[VIR_MAC_STRING_BUFLEN];

 virReportSystemError(-err->error,


ACK with that fixed.

Michal

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

Re: [libvirt] [PATCH 08/19] util: new function virPCIDeviceRebind()

[Michal Privoznik]

On 03/10/2017 09:35 PM, Laine Stump wrote:

This function unbinds a device from its driver, then immediately
rebinds it to its driver again.
---
 src/libvirt_private.syms |  1 +
 src/util/virpci.c| 25 +
 src/util/virpci.h|  1 +
 3 files changed, 27 insertions(+)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index b44a6ee..ef027cc 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2217,6 +2217,7 @@ virPCIDeviceListSteal;
 virPCIDeviceListStealIndex;
 virPCIDeviceNew;
 virPCIDeviceReattach;
+virPCIDeviceRebind;
 virPCIDeviceReset;
 virPCIDeviceSetManaged;
 virPCIDeviceSetRemoveSlot;
diff --git a/src/util/virpci.c b/src/util/virpci.c
index 9878398..a007eea 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -1101,6 +1101,31 @@ virPCIDeviceUnbind(virPCIDevicePtr dev)
 return ret;
 }

+
+/**
+ * virPCIDeviceRebind:
+ *  @dev: virPCIDevice object describing the device to rebind
+ *
+ * unbind a device from its driver, then immediately rebind it.
+ *
+ * Returns 0 on success, -1 on failure
+ */
+int virPCIDeviceRebind(virPCIDevicePtr dev)
+{
+if (virPCIDeviceUnbind(dev) < 0)
+return -1;
+
+if (virFileWriteStr(PCI_SYSFS "drivers_probe", dev->name, 0) < 0) {
+virReportSystemError(errno,
+ _("Failed to trigger a probe for PCI device 
'%s'"),
+ dev->name);
+return -1;
+}
+
+return 0;
+}
+


This is the same code as in virPCIDeviceBindWithDriverOverride(). ACK if 
you replace it with call to this function.


Michal

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

Re: [libvirt] [PATCH v2 2/4] qemu: Use generic PCIe Root Ports by default when available

[Laine Stump]

On 03/17/2017 05:06 AM, Andrea Bolognani wrote:
> On Thu, 2017-03-16 at 18:30 -0400, Laine Stump wrote:
>>> @@ -1861,7 +1863,12 @@ 
>>> qemuDomainPCIControllerSetDefaultModelName(virDomainControllerDefPtr cont)
>>>   *modelName = 
>>> VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_I82801B11_BRIDGE;
>>>   break;
>>>   case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT:
>>> -*modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_IOH3420;
>>> +/* Use generic PCIe Root Ports if available, falling back to
>>> + * ioh3420 otherwise */
>>> +if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PCIE_ROOT_PORT))
>>> +*modelName = 
>>> VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCIE_ROOT_PORT;
>>> +else
>>> +*modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_IOH3420;
>>  
>> I wonder if we should check caps for IOH3420 here just to be consistent
>> (and log an error if neither is available). I realize that's not the way
>> it worked before (existing code only checks the caps for a particular
>> device at the time we generate the commandline), but I'll be the first
>> to admit my original code was, err, "less than ideal".
>>  
>> It's up to you though, add it or not.
> 
> We already check when building the QEMU command line, which
> is the appropriate place IMHO. I'd rather not duplicate the
> check here as well.

It all depends on whether you want to get an error when you define the
domain, or not until you try to start it. I prefer the latter, but in
this case it's all just academic, since we know that all qemu binaries
we recognize as having a PCIe root bus also support the ioh3420.

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

Re: [libvirt] [RFC PATCH v2 REBASE 00/18] Introduce vGPU mdev framework to libvirt

[Laine Stump]

On 03/17/2017 01:57 AM, Chen, Xiaoguang wrote:
> 
> 
>> -Original Message-
>> From: sendmail [mailto:justsendmailnothinge...@gmail.com] On Behalf Of Laine
>> Stump
>> Sent: Thursday, March 16, 2017 10:01 PM
>> To: libvir-list@redhat.com
>> Cc: Chen, Xiaoguang ; Erik Skultety
>> ; He, Yongli 
>> Subject: Re: [libvirt] [RFC PATCH v2 REBASE 00/18] Introduce vGPU mdev
>> framework to libvirt
>>
>> On 03/16/2017 03:17 AM, Chen, Xiaoguang wrote:
>>> the screen call trace while start the VM (same for Ubuntu, Win10 etc)
>>> ==
>>>
>>> ubuntu@z-nuc-11:~/vgpu-meta/libvirt-stage$ myvirsh start vgpu-ubuntu
>>> 2017-03-09 19:06:50.483+: 2232: info : libvirt version: 3.1.0
>>> 2017-03-09 19:06:50.483+: 2232: info : hostname: z-nuc-11.maas
>>> 2017-03-09 19:06:50.483+: 2232: warning : qemuDomainObjTaint:4056 :
>>> Domain id=1 name='vgpu-ubuntu'
>>> uuid=972b5e38-0437-11e7-8f97-d36dba74552d
>>> is tainted: high-privileges
>>
>> I haven't considered any of the rest of the log yet, but this caught my eye 
>> on a
>> first pass - "high-privileges" means that you're running qemu as root, so 
>> your test
>> is bypassing several issues that could cause vfio device assignment to fail 
>> on a
>> "standard" system.
> What do you mean for 'cause vfio device assignment to fail on a standard 
> system'?

I mean that there are some device/file setup operations that libvirt
should be performing in order to make vfio device assignment work
properly in the case that the qemu process is unprivileged, and those
are often the source of bugs; if you run your tests with a privileged
qemu process, you're not testing any of the code that performs those
operations.

> 
>> It shouldn't be necessary to run qemu as root in order for
>> device assignment to work. Is there some specific reason that you're doing 
>> it this
>> way? (I'm guessing that you've set "user = root" in /etc/libvirt/qemu.conf)
> No. we will test the v3 using a non-root user.
> 

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

Re: [libvirt] [PATCH v1 2/2] Add asyncio event loop implementation

[Daniel P. Berrange]

On Fri, Mar 17, 2017 at 02:16:37AM +0100, Wojtek Porczyk wrote:
> This is usable only on python >= 3.4 (or 3.3 with out-of-tree asyncio),
> however it should be harmless for anyone with older python versions.
> 
> In simplest case, to have the callbacks queued on the default loop:
> 
> >>> import libvirt
> >>> libvirt.virEventRegisterAsyncIOImpl()
> 
> The function is not present on non-compatible platforms.
> 
> Signed-off-by: Wojtek Porczyk 
> ---
>  libvirt-override.py|   6 +
>  libvirt-python.spec.in |   1 +
>  libvirtaio.py  | 398 
> +
>  sanitytest.py  |   2 +-
>  setup.py   |  12 ++
>  5 files changed, 418 insertions(+), 1 deletion(-)
>  create mode 100644 libvirtaio.py
> 
> diff --git a/libvirt-override.py b/libvirt-override.py
> index 3d09d63..6a28336 100644
> --- a/libvirt-override.py
> +++ b/libvirt-override.py
> @@ -16,6 +16,12 @@ except ImportError:
>  if str(cyg_e).count("No module named"):
>  raise lib_e
>  
> +try:
> +from libvirtaio import virEventAsyncIOImpl, virEventRegisterAsyncIOImpl
> +except (ImportError, SyntaxError):
> +# python < 3.3, or 3.3 and no out-of-tree asyncio
> +pass
> +

I don't think we need this really. IMHO apps can simply do
'import libvirtaio' directly if they know they're using a
python new enough for asyncio.

> diff --git a/libvirtaio.py b/libvirtaio.py
> new file mode 100644
> index 000..44c9a5b
> --- /dev/null
> +++ b/libvirtaio.py
> @@ -0,0 +1,398 @@
> +#
> +# Copyright 2017 Wojtek Porczyk 
> +#
> +# Licensed under the Apache License, Version 2.0 (the 'License');
> +# you may not use this file except in compliance with the License.
> +# You may obtain a copy of the License at
> +#
> +# http://www.apache.org/licenses/LICENSE-2.0
> +#
> +# Unless required by applicable law or agreed to in writing, software
> +# distributed under the License is distributed on an 'AS IS' BASIS,
> +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> +# See the License for the specific language governing permissions and
> +# limitations under the License.
> +#

The libvirt python module is distributed under the LGPLv2+. Could you
resubmit this with that license header fixed.

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 00/12] qemu: Enforce guest CPU specification

[Jiri Denemark]

On Wed, Mar 15, 2017 at 17:39:34 +0100, Ján Tomko wrote:
> On Tue, Mar 14, 2017 at 05:57:39PM +0100, Jiri Denemark wrote:
> >When starting a domain with custom guest CPU specification QEMU may add
> >or remove some CPU features. There are several reasons for this, e.g.,
> >QEMU/KVM does not support some requested features or the definition of
> >the requested CPU model in libvirt's cpu_map.xml differs from the one
> >QEMU is using. We can't really avoid this because CPU models are allowed
> >to change with machine types and libvirt doesn't know (and probably
> >doesn't even want to know) about such changes.
> >
> >Thus when we want to make sure guest ABI doesn't change when a domain
> >gets migrated to another host, we need to update our live CPU definition
> >according to the CPU QEMU created. Once updated, we will change CPU
> >checking to VIR_CPU_CHECK_FULL to make sure the virtual CPU created
> >after migration exactly matches the one on the source.
> >
> >https://bugzilla.redhat.com/show_bug.cgi?id=822148
> >https://bugzilla.redhat.com/show_bug.cgi?id=824989
> >
> >Jiri Denemark (12):
> >  tests: Switch to sparse initialization of virCPUDef
> >  docs: Clarify /domain/cpu/@match description
> >  Introduce /domain/cpu/@check XML attribute
> >  qemu: Set default values for CPU check attribute
> >  qemu: Refactor Hyper-V features check
> >  qemu: Refactor KVM features check
> >  qemu: Refactor CPU features check
> >  qemu: Refactor qemuProcessVerifyGuestCPU
> >  qemu: Use ARCH_IS_X86 in qemuMonitorJSONGetGuestCPU
> >  qemu: Ask QEMU for filtered CPU features
> >  qemu: Update CPU definition according to QEMU
> >  qemu: Enforce guest CPU specification
> 
> ACK series.

Thanks, I implemented a test for the new virCPUUpdateLive API to confirm
the API works as expected (I'll send it to the list later today) and
pushed this series.

Jirka

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

Re: [libvirt] [PATCH 0/4] qemu: fix handling of 'group_name' disk io tuning parameter

[Michal Privoznik]

On 03/17/2017 09:30 AM, Peter Krempa wrote:

Fix a crasher and an invalid configuration.

Peter Krempa (4):
  qemu: Don't steal pointers from 'persistentDef' in
qemuDomainGetBlockIoTune
  qemu: command: Extract blkdeviotune checks into a separate function
  qemu: command: Extract tests for subsets of blkdeviotune settings
  qemu: command: Don't allow setting 'group_name' alone

 src/qemu/qemu_command.c | 181 ++--
 src/qemu/qemu_driver.c  |  22 --
 2 files changed, 127 insertions(+), 76 deletions(-)



ACK series.

Michal

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

Re: [libvirt] [PATCH v2] qemu: skip QMP probing of CPU definitions when missing

[Guido Günther]

On Fri, Mar 17, 2017 at 10:05:45AM +0100, Jiri Denemark wrote:
> On Thu, Mar 16, 2017 at 12:22:05 +0100, Guido Günther wrote:
> > This unbreaks emulators that don't support this command such as
> > qemu-system-mips*.
> > 
> > Reference: http://bugs.debian.org/854125
> 
> ACK

Pushed. Thanks. To to the merge of the PCIe code I had to shift things
slightly so I'm including the pushed version for completeness.
 -- Guido

>From 009c07b9f22a8937c2035b7f27ef80b1b83b0b33 Mon Sep 17 00:00:00 2001
Message-Id: 
<009c07b9f22a8937c2035b7f27ef80b1b83b0b33.1489744468.git@sigxcpu.org>
From: =?UTF-8?q?Guido=20G=C3=BCnther?= 
Date: Thu, 16 Mar 2017 09:19:02 +0100
Subject: [PATCH] qemu: skip QMP probing of CPU definitions when missing
To: libvir-list@redhat.com

This unbreaks emulators that don't support this command such as
qemu-system-mips*.

Reference: http://bugs.debian.org/854125
---
 src/qemu/qemu_capabilities.c| 6 ++
 src/qemu/qemu_capabilities.h| 3 +++
 tests/qemucapabilitiesdata/caps_1.2.2.x86_64.xml| 1 +
 tests/qemucapabilitiesdata/caps_1.3.1.x86_64.xml| 1 +
 tests/qemucapabilitiesdata/caps_1.4.2.x86_64.xml| 1 +
 tests/qemucapabilitiesdata/caps_1.5.3.x86_64.xml| 1 +
 tests/qemucapabilitiesdata/caps_1.6.0.x86_64.xml| 1 +
 tests/qemucapabilitiesdata/caps_1.7.0.x86_64.xml| 1 +
 tests/qemucapabilitiesdata/caps_2.1.1.x86_64.xml| 1 +
 tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml| 1 +
 tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml| 1 +
 tests/qemucapabilitiesdata/caps_2.6.0-gicv2.aarch64.xml | 1 +
 tests/qemucapabilitiesdata/caps_2.6.0-gicv3.aarch64.xml | 1 +
 tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml   | 1 +
 tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml| 1 +
 tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml | 1 +
 tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml| 1 +
 tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml | 1 +
 tests/qemucapabilitiesdata/caps_2.8.0.x86_64.xml| 1 +
 tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml| 1 +
 20 files changed, 27 insertions(+)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index b85ca71d1..60ed31ef9 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -361,6 +361,8 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
   "spice-rendernode",
   "nvdimm",
   "pcie-root-port",
+
+  "query-cpu-definitions", /* 250 */
 );
 
 
@@ -1518,6 +1520,7 @@ struct virQEMUCapsStringFlags virQEMUCapsCommands[] = {
 { "query-hotpluggable-cpus", QEMU_CAPS_QUERY_HOTPLUGGABLE_CPUS },
 { "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},
 };
 
 struct virQEMUCapsStringFlags virQEMUCapsMigration[] = {
@@ -2796,6 +2799,9 @@ virQEMUCapsProbeQMPCPUDefinitions(virQEMUCapsPtr qemuCaps,
 int ret = -1;
 size_t i;
 
+if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_QUERY_CPU_DEFINITIONS))
+return 0;
+
 if ((ncpus = qemuMonitorGetCPUDefinitions(mon, )) < 0)
 return -1;
 
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index ca8a5173c..55777f979 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -397,6 +397,9 @@ typedef enum {
 QEMU_CAPS_DEVICE_NVDIMM, /* -device nvdimm */
 QEMU_CAPS_DEVICE_PCIE_ROOT_PORT, /* -device pcie-root-port */
 
+/* 250 */
+QEMU_CAPS_QUERY_CPU_DEFINITIONS, /* qmp query-cpu-definitions */
+
 QEMU_CAPS_LAST /* this must always be the last item */
 } virQEMUCapsFlags;
 
diff --git a/tests/qemucapabilitiesdata/caps_1.2.2.x86_64.xml 
b/tests/qemucapabilitiesdata/caps_1.2.2.x86_64.xml
index f258c5fd3..5ad406ce1 100644
--- a/tests/qemucapabilitiesdata/caps_1.2.2.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_1.2.2.x86_64.xml
@@ -109,6 +109,7 @@
   
   
   
+  
   1002002
   0
   
diff --git a/tests/qemucapabilitiesdata/caps_1.3.1.x86_64.xml 
b/tests/qemucapabilitiesdata/caps_1.3.1.x86_64.xml
index bc76818d4..4ec731d65 100644
--- a/tests/qemucapabilitiesdata/caps_1.3.1.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_1.3.1.x86_64.xml
@@ -127,6 +127,7 @@
   
   
   
+  
   1003001
   0
   
diff --git a/tests/qemucapabilitiesdata/caps_1.4.2.x86_64.xml 
b/tests/qemucapabilitiesdata/caps_1.4.2.x86_64.xml
index ef4840003..601c62e65 100644
--- a/tests/qemucapabilitiesdata/caps_1.4.2.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_1.4.2.x86_64.xml
@@ -128,6 +128,7 @@
   
   
   
+  
   1004002
   0
   
diff --git a/tests/qemucapabilitiesdata/caps_1.5.3.x86_64.xml 
b/tests/qemucapabilitiesdata/caps_1.5.3.x86_64.xml
index 96b6fdb45..a68c13bbd 100644
--- a/tests/qemucapabilitiesdata/caps_1.5.3.x86_64.xml
+++ 

[libvirt] [PATCH] vz: Remove unused variable

[Mikhail Feoktistov]

emulatedType is not used in prlsdkAddDomainHardDisksInfo()
---
 src/vz/vz_sdk.c | 5 -
 1 file changed, 5 deletions(-)

diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c
index 3fd17db..8e6e89d 100644
--- a/src/vz/vz_sdk.c
+++ b/src/vz/vz_sdk.c
@@ -817,14 +817,9 @@ prlsdkAddDomainHardDisksInfo(vzDriverPtr driver, 
PRL_HANDLE sdkdom, virDomainDef
 
 for (i = 0; i < hddCount; ++i) {
 
-PRL_UINT32 emulatedType;
-
 pret = PrlVmCfg_GetHardDisk(sdkdom, i, );
 prlsdkCheckRetGoto(pret, error);
 
-pret = PrlVmDev_GetEmulatedType(hdd, );
-prlsdkCheckRetGoto(pret, error);
-
 if (IS_CT(def) &&
 prlsdkInBootList(sdkdom, hdd)) {
 
-- 
1.8.3.1

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

Re: [libvirt] [PATCH] perf: remote: Compare perf nparams against the correct constant

[Michal Privoznik]

On 03/16/2017 12:55 PM, Nitesh Konkar wrote:

Currently 'virsh perf domain' errors out as the perf nparams is
incorrectly compared against REMOTE_DOMAIN_MEMORY_PARAMETERS_MAX
instead of REMOTE_DOMAIN_PERF_EVENTS_MAX.

Signed-off-by: Nitesh Konkar 
---
 daemon/remote.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/daemon/remote.c b/daemon/remote.c
index f2b9b9a..1c9708c 100644
--- a/daemon/remote.c
+++ b/daemon/remote.c
@@ -3105,7 +3105,7 @@ remoteDispatchDomainGetPerfEvents(virNetServerPtr server 
ATTRIBUTE_UNUSED,
 if (virDomainGetPerfEvents(dom, , , args->flags) < 0)
 goto cleanup;

-if (nparams > REMOTE_DOMAIN_MEMORY_PARAMETERS_MAX) {
+if (nparams > REMOTE_DOMAIN_PERF_EVENTS_MAX) {
 virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large"));
 goto cleanup;
 }



ACKed and pushed.

Michal

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

Re: [libvirt] [PATCH v2 2/4] qemu: Use generic PCIe Root Ports by default when available

[Andrea Bolognani]

On Thu, 2017-03-16 at 18:30 -0400, Laine Stump wrote:
> > @@ -1861,7 +1863,12 @@ 
> > qemuDomainPCIControllerSetDefaultModelName(virDomainControllerDefPtr cont)
> >  *modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_I82801B11_BRIDGE;
> >  break;
> >  case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT:
> > -*modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_IOH3420;
> > +/* Use generic PCIe Root Ports if available, falling back to
> > + * ioh3420 otherwise */
> > +if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PCIE_ROOT_PORT))
> > +*modelName = 
> > VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCIE_ROOT_PORT;
> > +else
> > +*modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_IOH3420;
> 
> I wonder if we should check caps for IOH3420 here just to be consistent
> (and log an error if neither is available). I realize that's not the way
> it worked before (existing code only checks the caps for a particular
> device at the time we generate the commandline), but I'll be the first
> to admit my original code was, err, "less than ideal".
> 
> It's up to you though, add it or not.

We already check when building the QEMU command line, which
is the appropriate place IMHO. I'd rather not duplicate the
check here as well.


I've pushed the patches now, thanks for reviewing!

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH v2] qemu: skip QMP probing of CPU definitions when missing

[Jiri Denemark]

On Thu, Mar 16, 2017 at 12:22:05 +0100, Guido Günther wrote:
> This unbreaks emulators that don't support this command such as
> qemu-system-mips*.
> 
> Reference: http://bugs.debian.org/854125

ACK

Jirka

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

Re: [libvirt] [PATCH v2] apparmor: allow /usr/lib/qemu/qemu-bridge-helper

[Guido Günther]

On Thu, Mar 16, 2017 at 04:52:04PM +, Daniel P. Berrange wrote:
> On Thu, Mar 16, 2017 at 05:48:47PM +0100, Guido Günther wrote:
> > This is where e.g. Debian puts it.
> > ---
> > This adds lib64 as Dan suggested and also adds these two dirs to the
> > second invocations to make things actually work.
> > 
> >  examples/apparmor/usr.sbin.libvirtd | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/examples/apparmor/usr.sbin.libvirtd 
> > b/examples/apparmor/usr.sbin.libvirtd
> > index 8893e75fe..353b039ac 100644
> > --- a/examples/apparmor/usr.sbin.libvirtd
> > +++ b/examples/apparmor/usr.sbin.libvirtd
> > @@ -67,7 +67,7 @@
> ># allow changing to our UUID-based named profiles
> >change_profile -> 
> > @{LIBVIRT}-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*,
> >  
> > -  /usr/{lib,libexec}/qemu-bridge-helper Cx -> qemu_bridge_helper,
> > +  /usr/{lib,lib64,lib/qemu,libexec}/qemu-bridge-helper Cx -> 
> > qemu_bridge_helper,
> ># child profile for bridge helper process
> >profile qemu_bridge_helper {
> > #include 
> > @@ -83,6 +83,6 @@
> > /etc/qemu/** r,
> > owner @{PROC}/*/status r,
> >  
> > -   /usr/{lib,libexec}/qemu-bridge-helper rmix,
> > +   /usr/{lib,lib64,lib/qemu,libexec}/qemu-bridge-helper rmix,
> >}
> >  }
> 
> ACK

Pushed. Thanks
 -- Guido

> 
> 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 2/4] qemu: command: Extract blkdeviotune checks into a separate function

[Peter Krempa]

qemuBuildDriveStr grew into 'megamoth' proportions. Cut out some parts.
---
 src/qemu/qemu_command.c | 149 ++--
 1 file changed, 80 insertions(+), 69 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 61d9eb94a..300c51b39 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1199,6 +1199,85 @@ qemuGetDriveSourceString(virStorageSourcePtr src,
 }


+static int
+qemuCheckDiskConfigBlkdeviotune(virDomainDiskDefPtr disk,
+virQEMUCapsPtr qemuCaps)
+{
+/* block I/O throttling */
+if ((disk->blkdeviotune.total_bytes_sec ||
+ disk->blkdeviotune.read_bytes_sec ||
+ disk->blkdeviotune.write_bytes_sec ||
+ disk->blkdeviotune.total_iops_sec ||
+ disk->blkdeviotune.read_iops_sec ||
+ disk->blkdeviotune.write_iops_sec) &&
+!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_IOTUNE)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("block I/O throttling not supported with this "
+ "QEMU binary"));
+return -1;
+}
+
+/* block I/O throttling 1.7 */
+if ((disk->blkdeviotune.total_bytes_sec_max ||
+ disk->blkdeviotune.read_bytes_sec_max ||
+ disk->blkdeviotune.write_bytes_sec_max ||
+ disk->blkdeviotune.total_iops_sec_max ||
+ disk->blkdeviotune.read_iops_sec_max ||
+ disk->blkdeviotune.write_iops_sec_max ||
+ disk->blkdeviotune.size_iops_sec) &&
+!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("there are some block I/O throttling parameters "
+ "that are not supported with this QEMU binary"));
+return -1;
+}
+
+/* block I/O group 2.4 */
+if (disk->blkdeviotune.group_name &&
+!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_GROUP)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("the block I/O throttling group parameter is "
+ "not supported with this QEMU binary"));
+return -1;
+}
+
+/* block I/O throttling length 2.6 */
+if ((disk->blkdeviotune.total_bytes_sec_max_length ||
+ disk->blkdeviotune.read_bytes_sec_max_length ||
+ disk->blkdeviotune.write_bytes_sec_max_length ||
+ disk->blkdeviotune.total_iops_sec_max_length ||
+ disk->blkdeviotune.read_iops_sec_max_length ||
+ disk->blkdeviotune.write_iops_sec_max_length) &&
+!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX_LENGTH)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("there are some block I/O throttling length 
parameters "
+ "that are not supported with this QEMU binary"));
+return -1;
+}
+
+if (disk->blkdeviotune.total_bytes_sec > QEMU_BLOCK_IOTUNE_MAX ||
+disk->blkdeviotune.read_bytes_sec > QEMU_BLOCK_IOTUNE_MAX ||
+disk->blkdeviotune.write_bytes_sec > QEMU_BLOCK_IOTUNE_MAX ||
+disk->blkdeviotune.total_iops_sec > QEMU_BLOCK_IOTUNE_MAX ||
+disk->blkdeviotune.read_iops_sec > QEMU_BLOCK_IOTUNE_MAX ||
+disk->blkdeviotune.write_iops_sec > QEMU_BLOCK_IOTUNE_MAX ||
+disk->blkdeviotune.total_bytes_sec_max > QEMU_BLOCK_IOTUNE_MAX ||
+disk->blkdeviotune.read_bytes_sec_max > QEMU_BLOCK_IOTUNE_MAX ||
+disk->blkdeviotune.write_bytes_sec_max > QEMU_BLOCK_IOTUNE_MAX ||
+disk->blkdeviotune.total_iops_sec_max > QEMU_BLOCK_IOTUNE_MAX ||
+disk->blkdeviotune.read_iops_sec_max > QEMU_BLOCK_IOTUNE_MAX ||
+disk->blkdeviotune.write_iops_sec_max > QEMU_BLOCK_IOTUNE_MAX ||
+disk->blkdeviotune.size_iops_sec > QEMU_BLOCK_IOTUNE_MAX) {
+virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED,
+  _("block I/O throttle limit must "
+"be no more than %llu using QEMU"), 
QEMU_BLOCK_IOTUNE_MAX);
+return -1;
+}
+
+return 0;
+}
+
+
 /* Perform disk definition config validity checks */
 int
 qemuCheckDiskConfig(virDomainDiskDefPtr disk)
@@ -1757,76 +1836,8 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk,
 }
 }

-/* block I/O throttling */
-if ((disk->blkdeviotune.total_bytes_sec ||
- disk->blkdeviotune.read_bytes_sec ||
- disk->blkdeviotune.write_bytes_sec ||
- disk->blkdeviotune.total_iops_sec ||
- disk->blkdeviotune.read_iops_sec ||
- disk->blkdeviotune.write_iops_sec) &&
-!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_IOTUNE)) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-   _("block I/O throttling not supported with this "
- "QEMU binary"));
+if (qemuCheckDiskConfigBlkdeviotune(disk, qemuCaps) < 0)
 goto error;
-}
-
-/* block I/O throttling 1.7 */
-if 

[libvirt] [PATCH 3/4] qemu: command: Extract tests for subsets of blkdeviotune settings

[Peter Krempa]

When checking capabilities for qemu we need to check whether subsets of
the disk throttling settings are supported. Extract the checks into a
separate functions as they will be reused in next patch.
---
 src/qemu/qemu_command.c | 59 +
 1 file changed, 40 insertions(+), 19 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 300c51b39..76c915ab6 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1199,17 +1199,49 @@ qemuGetDriveSourceString(virStorageSourcePtr src,
 }


+static bool
+qemuDiskConfigBlkdeviotuneHasBasic(virDomainDiskDefPtr disk)
+{
+return disk->blkdeviotune.total_bytes_sec ||
+   disk->blkdeviotune.read_bytes_sec ||
+   disk->blkdeviotune.write_bytes_sec ||
+   disk->blkdeviotune.total_iops_sec ||
+   disk->blkdeviotune.read_iops_sec ||
+   disk->blkdeviotune.write_iops_sec;
+}
+
+
+static bool
+qemuDiskConfigBlkdeviotuneHasMax(virDomainDiskDefPtr disk)
+{
+return disk->blkdeviotune.total_bytes_sec_max ||
+   disk->blkdeviotune.read_bytes_sec_max ||
+   disk->blkdeviotune.write_bytes_sec_max ||
+   disk->blkdeviotune.total_iops_sec_max ||
+   disk->blkdeviotune.read_iops_sec_max ||
+   disk->blkdeviotune.write_iops_sec_max ||
+   disk->blkdeviotune.size_iops_sec;
+}
+
+
+static bool
+qemuDiskConfigBlkdeviotuneHasMaxLength(virDomainDiskDefPtr disk)
+{
+return disk->blkdeviotune.total_bytes_sec_max_length ||
+   disk->blkdeviotune.read_bytes_sec_max_length ||
+   disk->blkdeviotune.write_bytes_sec_max_length ||
+   disk->blkdeviotune.total_iops_sec_max_length ||
+   disk->blkdeviotune.read_iops_sec_max_length ||
+   disk->blkdeviotune.write_iops_sec_max_length;
+}
+
+
 static int
 qemuCheckDiskConfigBlkdeviotune(virDomainDiskDefPtr disk,
 virQEMUCapsPtr qemuCaps)
 {
 /* block I/O throttling */
-if ((disk->blkdeviotune.total_bytes_sec ||
- disk->blkdeviotune.read_bytes_sec ||
- disk->blkdeviotune.write_bytes_sec ||
- disk->blkdeviotune.total_iops_sec ||
- disk->blkdeviotune.read_iops_sec ||
- disk->blkdeviotune.write_iops_sec) &&
+if (qemuDiskConfigBlkdeviotuneHasBasic(disk) &&
 !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_IOTUNE)) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("block I/O throttling not supported with this "
@@ -1218,13 +1250,7 @@ qemuCheckDiskConfigBlkdeviotune(virDomainDiskDefPtr disk,
 }

 /* block I/O throttling 1.7 */
-if ((disk->blkdeviotune.total_bytes_sec_max ||
- disk->blkdeviotune.read_bytes_sec_max ||
- disk->blkdeviotune.write_bytes_sec_max ||
- disk->blkdeviotune.total_iops_sec_max ||
- disk->blkdeviotune.read_iops_sec_max ||
- disk->blkdeviotune.write_iops_sec_max ||
- disk->blkdeviotune.size_iops_sec) &&
+if (qemuDiskConfigBlkdeviotuneHasMax(disk) &&
 !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX)) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("there are some block I/O throttling parameters "
@@ -1242,12 +1268,7 @@ qemuCheckDiskConfigBlkdeviotune(virDomainDiskDefPtr disk,
 }

 /* block I/O throttling length 2.6 */
-if ((disk->blkdeviotune.total_bytes_sec_max_length ||
- disk->blkdeviotune.read_bytes_sec_max_length ||
- disk->blkdeviotune.write_bytes_sec_max_length ||
- disk->blkdeviotune.total_iops_sec_max_length ||
- disk->blkdeviotune.read_iops_sec_max_length ||
- disk->blkdeviotune.write_iops_sec_max_length) &&
+if (qemuDiskConfigBlkdeviotuneHasMaxLength(disk) &&
 !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX_LENGTH)) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("there are some block I/O throttling length 
parameters "
-- 
2.12.0

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

[libvirt] [PATCH 0/4] qemu: fix handling of 'group_name' disk io tuning parameter

[Peter Krempa]

Fix a crasher and an invalid configuration.

Peter Krempa (4):
  qemu: Don't steal pointers from 'persistentDef' in
qemuDomainGetBlockIoTune
  qemu: command: Extract blkdeviotune checks into a separate function
  qemu: command: Extract tests for subsets of blkdeviotune settings
  qemu: command: Don't allow setting 'group_name' alone

 src/qemu/qemu_command.c | 181 ++--
 src/qemu/qemu_driver.c  |  22 --
 2 files changed, 127 insertions(+), 76 deletions(-)

-- 
2.12.0

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

[libvirt] [PATCH 1/4] qemu: Don't steal pointers from 'persistentDef' in qemuDomainGetBlockIoTune

[Peter Krempa]

While the code path that queries the monitor allocates a separate copy
of the 'group_name' string the path querying the config would not copy
it. The call to virTypedParameterAssign would then steal the pointer
(without clearing it) and the RPC layer freed it. Any subsequent call
resulted into a crash.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1433183
---
 src/qemu/qemu_driver.c | 22 +++---
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 2032fac71..dcd823f53 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -17707,6 +17707,11 @@ qemuDomainGetBlockIoTune(virDomainPtr dom,
 goto endjob;
 }
 reply = disk->blkdeviotune;
+
+/* Group name needs to be copied since qemuMonitorGetBlockIoThrottle
+ * allocates it as well */
+if (VIR_STRDUP(reply.group_name, disk->blkdeviotune.group_name))
+goto endjob;
 }

 #define BLOCK_IOTUNE_ASSIGN(name, var) 
\
@@ -17736,13 +17741,15 @@ qemuDomainGetBlockIoTune(virDomainPtr dom,

 BLOCK_IOTUNE_ASSIGN(SIZE_IOPS_SEC, size_iops_sec);

-/* NB: Cannot use macro since this is a STRING not a ULLONG */
-if (*nparams < maxparams &&
-virTypedParameterAssign([(*nparams)++],
-VIR_DOMAIN_BLOCK_IOTUNE_GROUP_NAME,
-VIR_TYPED_PARAM_STRING,
-reply.group_name) < 0)
-goto endjob;
+if (*nparams < maxparams) {
+if (virTypedParameterAssign([(*nparams)++],
+VIR_DOMAIN_BLOCK_IOTUNE_GROUP_NAME,
+VIR_TYPED_PARAM_STRING,
+reply.group_name) < 0)
+goto endjob;
+
+reply.group_name = NULL;
+}

 BLOCK_IOTUNE_ASSIGN(TOTAL_BYTES_SEC_MAX_LENGTH, 
total_bytes_sec_max_length);
 BLOCK_IOTUNE_ASSIGN(READ_BYTES_SEC_MAX_LENGTH, read_bytes_sec_max_length);
@@ -17759,6 +17766,7 @@ qemuDomainGetBlockIoTune(virDomainPtr dom,
 qemuDomainObjEndJob(driver, vm);

  cleanup:
+VIR_FREE(reply.group_name);
 VIR_FREE(device);
 virDomainObjEndAPI();
 return ret;
-- 
2.12.0

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

[libvirt] [PATCH 4/4] qemu: command: Don't allow setting 'group_name' alone

[Peter Krempa]

The disk tuning group parameter is ignored by qemu if no other
throttling options are set. Reject such configuration, since the name
would not be honored after setting parameters via the live tuning API.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1433180
---
 src/qemu/qemu_command.c | 23 +--
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 76c915ab6..9760fb10f 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1259,12 +1259,23 @@ qemuCheckDiskConfigBlkdeviotune(virDomainDiskDefPtr 
disk,
 }

 /* block I/O group 2.4 */
-if (disk->blkdeviotune.group_name &&
-!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_GROUP)) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-   _("the block I/O throttling group parameter is "
- "not supported with this QEMU binary"));
-return -1;
+if (disk->blkdeviotune.group_name) {
+if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_GROUP)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("the block I/O throttling group parameter is "
+ "not supported with this QEMU binary"));
+return -1;
+}
+
+/* group_name by itself is ignored by qemu */
+if (!qemuDiskConfigBlkdeviotuneHasBasic(disk) &&
+!qemuDiskConfigBlkdeviotuneHasMax(disk) &&
+!qemuDiskConfigBlkdeviotuneHasMaxLength(disk)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("group_name can be configured only together with "
+ "settings"));
+return -1;
+}
 }

 /* block I/O throttling length 2.6 */
-- 
2.12.0

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

Re: [libvirt] [BUG] mlock support breakage

[Luiz Capitulino]

On Wed, 15 Mar 2017 14:29:13 -0400
Luiz Capitulino  wrote:

> > ... we could consider  to be the explicit request for
> > setting an infinite memory locking limit and letting users set a lower
> > limit with hard_limit if they want.  
> 
> That's exactly how I see it! It seems we're total agreement.
> 
> Now, something has just occurred to me: shouldn't VFIO have
> the same problem? It's the same hard limit that's set.

I took a look at this today. While it's the same mlock limit
that's set and while QEMU's allocations can surpass that limit,
I didn't get a crash when using VFIO. The most probable obvious
reason for this is that VFIO is probably mlocking a small region,
although I could not find where this is done in QEMU.

In that case, VFIO is not affected. This issue is specific to
, where the 1GB limit set by libvirt conflicts with
QEMU memory needs.

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

Re: [libvirt] [RFC PATCH v2 REBASE 00/18] Introduce vGPU mdev framework to libvirt

[Chen, Xiaoguang]

>-Original Message-
>From: Erik Skultety [mailto:eskul...@redhat.com]
>Sent: Thursday, March 16, 2017 10:41 PM
>To: Chen, Xiaoguang 
>Cc: libvir-list@redhat.com; He, Yongli 
>Subject: Re: [libvirt] [RFC PATCH v2 REBASE 00/18] Introduce vGPU mdev
>framework to libvirt
>
>> [2] 2005
>> ubuntu@z-nuc-11:~/vgpu-meta/libvirt-stage$
>> ***
>> start libvirt-d
>> 2017-03-09 19:04:57.211+: 2059: info : libvirt version: 3.1.0
>> 2017-03-09 19:04:57.211+: 2059: info : hostname: z-nuc-11.maas
>> 2017-03-09 19:04:57.211+: 2059: error : qemuMonitorOpenUnix:367 :
>> failed to connect to monitor socket: No such process
>> 2017-03-09 19:04:57.213+: 2059: error :
>> virMediatedDeviceGetIOMMUGroupDev:153 : internal error: IOMMU group
>> file
>> /sys/bus/mdev/devices/894f3983-1a36-42b3-b52c-
>1024aca216be/iommu_group
>> is not a symlink
>
>When I saw this error message for the first time in the original thread, I got
>confused, since this just checks whether the symlink exists, if it doesn't, 
>the vfio
>device probably also doesn't exist (but take this with a grain of salt, I 
>haven't
>investigated that deep) and libvirt needs it to pass it onto qemu command 
>line. I
>hit this issue once by accident in the past and at that time I didn't 
>understand
>what caused it, but after a reboot it was gone.
>So seeing it here it caught my eye and I investigated it last week. What I 
>found
>out was that it's caused by the vfio-mdev module not being loaded automatically
>as a dependency. I solved it by autoloading the module on system boot. So this 
>is
>not a libvirt issue, but just for a reference, there is a BZ on this [1].
>
>> 2017-03-09 19:04:57.213+: 2003: info : libvirt version: 3.1.0
>> 2017-03-09 19:04:57.213+: 2003: info : hostname: z-nuc-11.maas
>> 2017-03-09 19:04:57.213+: 2003: error : virNetSocketReadWire:1800 :
>> End of file while reading data: Input/output error
>
>I suppose this corresponds to the problem above, do you hit this error if you 
>work
>around the vfio-mdev module problem described above?
Based on the KVMGT setup guide we should add kvmgt, vfio-iommu-type1 and 
vfio-mdev modules into initfamfs.
So I don't think it is the same problem. But we will double confirm it later. 
Yong li is on vacation this week when he come back we will do the test again.

>
>>
>> the screen call trace while start the VM (same for Ubuntu, Win10 etc)
>> ==
>>
>> ubuntu@z-nuc-11:~/vgpu-meta/libvirt-stage$ myvirsh start vgpu-ubuntu
>> 2017-03-09 19:06:50.483+: 2232: info : libvirt version: 3.1.0
>> 2017-03-09 19:06:50.483+: 2232: info : hostname: z-nuc-11.maas
>> 2017-03-09 19:06:50.483+: 2232: warning : qemuDomainObjTaint:4056 :
>> Domain id=1 name='vgpu-ubuntu'
>> uuid=972b5e38-0437-11e7-8f97-d36dba74552d
>> is tainted: high-privileges
>> 2017-03-09 19:06:50.819+: 2204: info : libvirt version: 3.1.0
>> 2017-03-09 19:06:50.819+: 2232: warning :
>> virDomainAuditHostdev:456
>> : Unexpected hostdev type while encoding audit message: 4
>
>This one's interesting, again, are you able to hit the error when you work 
>around
>the missing vfio-mdev module? I'll have a look at this if you actually can hit 
>the
>error, even if the XML is correct.
>
>I posted v3 of the series and also created a new branch 'mdev-next' on my 
>github
>[2]. I dropped the attribute 'type' from the source address element, so follow 
>the
>example in the updated docs.
Will test the v3 when yong li back from vacation.


>
>Thanks for giving it a try.
>Erik
>
>[1] https://bugzilla.redhat.com/show_bug.cgi?id=1420572
>[2] https://github.com/eskultety/libvirt/commits/mdev-next

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