Re: [libvirt] [PATCH v2] qemuOpenVhostNet: Decrease vhostfdSize on open failure
On 28.05.2013 18:43, Laine Stump wrote:
On 05/27/2013 02:44 AM, Michal Privoznik wrote:
Currently, if there's an error opening /dev/vhost-net (e.g. because
it doesn't exist) but it's not required we proceed with vhostfd array
filled with -1 and vhostfdSize unchanged. Later, when constructing
the qemu command line only non-negative items within vhostfd array
are taken into account. This means, vhostfdSize may be greater than
the actual count of non-negative items in vhostfd array. This results
in improper command line arguments being generated, e.g.:
-netdev tap,fd=21,id=hostnet0,vhost=on,vhostfd=(null)
---
src/qemu/qemu_command.c | 14 --
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 0373626..c4a162a 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -486,6 +486,10 @@ qemuOpenVhostNet(virDomainDefPtr def,
but is unavailable));
goto error;
}
+VIR_WARN(Unable to open vhost-net. Opened so far %d, requested
%d,
+ i, *vhostfdSize);
+*vhostfdSize = i;
+break;
}
}
virDomainAuditNetDevice(def, net, /dev/vhost-net, *vhostfdSize);
@@ -6560,12 +6564,10 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd,
}
for (i = 0; i vhostfdSize; i++) {
-if (vhostfd[i] = 0) {
-virCommandTransferFD(cmd, vhostfd[i]);
-if (virAsprintf(vhostfdName[i], %d, vhostfd[i]) 0) {
-virReportOOMError();
-goto cleanup;
-}
+virCommandTransferFD(cmd, vhostfd[i]);
+if (virAsprintf(vhostfdName[i], %d, vhostfd[i]) 0) {
+virReportOOMError();
+goto cleanup;
}
}
ACK.
Thanks, pushed.
michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Put the audio work on Windows 2008 Server 64bits
Thank you for your answer. I try to install Ulteo (http://www.ulteo.com/) with one Linux Application Server and one Windows Application Server and i need audio support on Windows APS. On, Linux the audio works ok, but on Windows 2008 Server, the device is detected but the driver is not working very well. So, i think i don't need Spice work for do that, but i will ask on spice list if they have some experiences with Windows audio drivers. Thank you again. Best Regards, -- Carlos Rodrigues c...@eurotux.com Engenheiro de Software Sénior Eurotux Informática, S.A. | www.eurotux.com (t) +351 253 680 300 On Tue, 2013-05-28 at 14:33 -0600, Eric Blake wrote: On 05/28/2013 11:06 AM, Carlos Rodrigues wrote: Hello, I am trying to put the audio audio work on Windows 2008 Server with 64bits but without success. I have tested with sound model ac97 and hda, and i always get the same error: the devices appears on Windows Device Manager with following message - Windows cannot load the device driver for this hardware. The driver may be corrupted or missing. (Code 39). I have qemu-kvm 1.2 and libvirt 0.10.2. I also tried different drivers with different sound model combinations but no success. Any suggestion of drivers that can i use and how can i get audio device working? Have you considered using SPICE for your audio? Personally, I haven't tried audio passthrough to my one windows guest, so I don't have many ideas; it may be better to ask on spice lists, precisely because that's where you will find more developers that actually play with that sort of thing. signature.asc Description: This is a digitally signed message part -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] storage: Refresh pool after creating volume
https://bugzilla.redhat.com/show_bug.cgi?id=965442 One has to refresh the pool to get the correct pool info, this patch refreshes the pool after creating a volume in code instead. Pool refreshing failure is fine to ignore with a warning. --- src/storage/storage_driver.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index a2b0c21..2a55095 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1443,6 +1443,9 @@ storageVolCreateXML(virStoragePoolPtr obj, } +if (backend-refreshPool backend-refreshPool(obj-conn, pool) 0) +VIR_WARN(Failed to refresh pool after creating volume); + VIR_INFO(Creating volume '%s' in storage pool '%s', volobj-name, pool-def-name); ret = volobj; @@ -1606,6 +1609,9 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj, goto cleanup; } +if (backend-refreshPool backend-refreshPool(obj-conn, pool) 0) +VIR_WARN(Failed to refresh pool after creating volume); + VIR_INFO(Creating volume '%s' in storage pool '%s', volobj-name, pool-def-name); ret = volobj; -- 1.8.1.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 12/12] storage_conf: Use uid_t/gid_t instead of int to cast the value
On 24/05/13 22:11, Eric Blake wrote:
On 05/22/2013 06:05 AM, Osier Yang wrote:
And error out if the casted value is not same with the original
one, which prevents the bug on platform(s) where uid_t/gid_t
has different size with long.
---
src/conf/storage_conf.c | 13 -
1 file changed, 8 insertions(+), 5 deletions(-)
} else {
-if (virXPathLong(number(./owner), ctxt, v) 0) {
+if (virXPathLong(number(./owner), ctxt, val) 0 ||
+(uid_t)val != val) {
Once you have made this check...
virReportError(VIR_ERR_XML_ERROR, %s,
_(malformed owner element));
goto error;
}
-perms-uid = (int)v;
+
+perms-uid = (uid_t)val;
...the cast here is redundant. You could write 'perms-uid = val'.
}
if (virXPathNode(./group, ctxt) == NULL) {
perms-gid = (gid_t) -1;
} else {
-if (virXPathLong(number(./group), ctxt, v) 0) {
+if (virXPathLong(number(./group), ctxt, val) 0 ||
+(gid_t)val != val) {
virReportError(VIR_ERR_XML_ERROR, %s,
_(malformed group element));
goto error;
}
-perms-gid = (int)v;
+perms-gid = (gid_t)val;
Likewise.
ACK with that simplification, and with your followup that explicitly
allows -1.
Thanks for the reviewing, I pushed the left patches (except 10/12) with the
small nits pointed by you guys fixed.
Osier
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/3 v3] Cgroup fixes
ping? On 24/05/13 17:08, Osier Yang wrote: Instead of only post v2 of 5/7 (v2) in the old mail thread, this resend the left patches of v2 with a independant thread for easy reviewing. v2 - v3: * 1/7 ~ 4/7 of v1 are pushed. * goto label cleanup is kept with Daniel's suggestion for 5/7 (v1) * Commit log of 6/7 (v1) are updated a bit to reflect more clear about the cause v1 - v2: * Just resending v2: https://www.redhat.com/archives/libvir-list/2013-May/msg01314.html Osier Yang (3): qemu: Abstract code for the cpu controller setting into a helper qemu: Set cpuset.cpus for domain process qemu: Prohibit getting the numa parameters if mode is not strict src/qemu/qemu_cgroup.c | 94 -- src/qemu/qemu_driver.c | 11 -- 2 files changed, 78 insertions(+), 27 deletions(-) -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] doc: cpu topology values should be equal to maxmium vcpus
On 05/29/2013 11:14 AM, Eric Blake wrote: On 05/28/2013 08:53 PM, Guannan Ren wrote: --- docs/formatdomain.html.in | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) There are little document about how to set values of sockets, cores, threads properly. After going through qemu code, I summarize the package/cores/threads/ algorithm which can be verified by 'cpuid' tool. The following only explains the case of all the four number are given in qemu commandline. smp = n (argument to -smp) nr_threads = smp / (cores * sockets) nr_cores = cores -smp 4, sockets=1,cores=4,threads=2 leads to: 1(physical cpu) *4(cores) *1(thread) One physical cpu contains four cores log processor which each contains one thread logical processor. 0 means only one physical cpu package pkg_id = cpu_index/nr_threads/nr_cores 0 = 0 / 1 / 4 0 = 1 / 1 / 4 0 = 2 / 1 / 4 0 = 3 / 1 / 4 3 means there are four multi-cores logical processor core_id = cpu_index/nr_threads % nr_cores 0 = 0 / 1 % 4 1 = 1 / 1 % 4 2 = 2 / 1 % 4 3 = 3 / 1 % 4 0 means only one thread logical processor smt_id = cpu_index % nr_threads 0 = 0 % 1 0 = 1 % 1 0 = 2 % 1 0 = 3 % 1 And the result could be verified by 'cpuid' tool running in fedora guest. 1st vcpu: (APIC synth): PKG_ID=0 CORE_ID=0 SMT_ID=0 2nd vcpu: (APIC synth): PKG_ID=0 CORE_ID=1 SMT_ID=0 3rd vcpu: (APIC synth): PKG_ID=0 CORE_ID=2 SMT_ID=0 4th vcpu: (APIC synth): PKG_ID=0 CORE_ID=3 SMT_ID=0 I tried it on several cpu topology: -smp 16, sockets=2,cores=2,threads=1 leads to 2 * 2 * 4 There are two physical cpu package. Each of them contains two cores logical processor and each of the cores contains four hyper-threading logical processor. The result could be verified by cpuid tool. If the calculation is true, I think my patch is self-acked. Guannan -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] storage: Refresh pool after creating volume
On 05/29/2013 11:53 AM, Osier Yang wrote: https://bugzilla.redhat.com/show_bug.cgi?id=965442 One has to refresh the pool to get the correct pool info, this patch refreshes the pool after creating a volume in code instead. Pool refreshing failure is fine to ignore with a warning. --- src/storage/storage_driver.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index a2b0c21..2a55095 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1443,6 +1443,9 @@ storageVolCreateXML(virStoragePoolPtr obj, } +if (backend-refreshPool backend-refreshPool(obj-conn, pool) 0) +VIR_WARN(Failed to refresh pool after creating volume); + VIR_INFO(Creating volume '%s' in storage pool '%s', volobj-name, pool-def-name); ret = volobj; @@ -1606,6 +1609,9 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj, goto cleanup; } +if (backend-refreshPool backend-refreshPool(obj-conn, pool) 0) +VIR_WARN(Failed to refresh pool after creating volume); + VIR_INFO(Creating volume '%s' in storage pool '%s', volobj-name, pool-def-name); ret = volobj; I don't want to say NACK just like that, but I think the bug indicates there's a problem in the storage driver. It should automatically reflect the changes made to that pool. What's the structure that gets updated only with refresh and not after the vol is created? Does it do with all the drivers? Long story short; I think this bug fixes the symptom, not the problem. Martin -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] doc: cpu topology values should be equal to maxmium vcpus
If the calculation is true, I think my patch is self-acked. s/self-acked/self-nacked/, sorry. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] target-i386: Disable CPUID_EXT_MONITOR when KVM is enabled
(CCing libvirt people) On Tue, May 28, 2013 at 06:48:52PM +0200, Andreas Färber wrote: Am 28.05.2013 18:46, schrieb Paolo Bonzini: Il 28/05/2013 18:34, Bandan Das ha scritto: Eduardo Habkost ehabk...@redhat.com writes: On Mon, May 27, 2013 at 02:21:36PM +0200, Paolo Bonzini wrote: Il 27/05/2013 14:09, Eduardo Habkost ha scritto: On Sat, May 25, 2013 at 08:25:49AM +0200, Paolo Bonzini wrote: Il 25/05/2013 03:21, Bandan Das ha scritto: There is one user-visible effect: -cpu ...,enforce will stop failing because of missing KVM support for CPUID_EXT_MONITOR. But that's exactly the point: there's no point in having CPU model definitions that would never work as-is with neither TCG or KVM. This patch is changing the meaning of (e.g.) -machine ...,accel=kvm -cpu Opteron_G3 to match what was already happening in practice. But then -cpu Opteron_G3 does not match a real Opteron G3. Is it worth it? No models match a real CPU this way, because neither TCG or KVM support all features supported by a real CPU. I ask the opposite question: is it worth maintaining an accurate CPU model definition that would never work without feature-bit tweaking in the command-line? It would work with TCG. Changing TCG to KVM should not change hardware if you use -cpu ...,enforce, so it is right that it fails when starting with KVM. Changing between KVM and TCG _does_ change hardware, today (with or without check/enforce). All CPU models on TCG have features not supported by TCG automatically removed. See the if (!kvm_enabled()) block at x86_cpu_realizefn(). Yes, this is exactly why I was inclined to remove the monitor flag. We already have uses of kvm_enabled() to set (or remove) kvm specific stuff, and this change is no different. Do any of these affect something that is part of x86_def_t? The vendor comes to mind. I believe we can still consider the vendor field a special one: if other components care about the TCG/KVM difference regarding the vendor field, they can simply set vendor explicitly on the command-line. I can see Paolo's point though, having a common definition probably makes sense too. Paolo is convincing me that keeping the rest of the features exactly the same on TCG and KVM modes (and making check/enforce work for TCG as well) would simplify the logic a lot. This will add a little extra work for libvirt, that will probably need to use -cpu Opteron_G3,-monitor once it implements enforce-mode (to make sure the results really match existing libvirt assumptions about the Opteron_G* models), but it is probably worth it. I will give it a try and send a proposal soon. (That's why I argue that we need separate classes/names for TCG and KVM modes. Otherwise our predefined models get less useful as they will require low-level feature-bit fiddling on the libvirt side to make them work as expected.) Agreed. From a user's perspective, I think the more a CPU model just works, whether it's KVM or TCG, the better. Yes, that's right. But I think extending the same expectation to -cpu ...,enforce is not necessary, and perhaps even wrong for -cpu ...,check since it's only a warning rather than a fatal error. Paolo -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg -- Eduardo -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv2] build: fix build with newer gnutls
Building with gnutls 3.2.0 (such as shipped with current cygwin) fails with: rpc/virnettlscontext.c: In function 'virNetTLSSessionGetKeySize': rpc/virnettlscontext.c:1358:5: error: implicit declaration of function 'gnutls_cipher_get_key_size' [-Wimplicit-function-declaration] Yeah, it's stupid that gnutls broke API by moving their declaration into a new header without including that header from the old one, but it's easy enough to work around, all without breaking on gnutls 1.4.1 (hello RHEL 5) that lacked the new header. * src/rpc/virnettlscontext.c (includes): Include additional header. * configure.ac (gnutls): Check for gnutls/crypto.h. Signed-off-by: Eric Blake ebl...@redhat.com --- This version passed testing; pushing under the build-breaker rule. configure.ac | 7 +-- src/rpc/virnettlscontext.c | 3 +++ 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/configure.ac b/configure.ac index 2055637..8efbb04 100644 --- a/configure.ac +++ b/configure.ac @@ -1103,10 +1103,13 @@ if test x$with_gnutls != xno; then dnl it explicitly for the calls to gcry_control/check_version GNUTLS_LIBS=$GNUTLS_LIBS -lgcrypt -dnl We're not using gcrypt deprecated features so define GCRYPT_NO_DEPRECATED -dnl to avoid deprecated warnings +dnl We're not using gcrypt deprecated features so define +dnl GCRYPT_NO_DEPRECATED to avoid deprecated warnings GNUTLS_CFLAGS=$GNUTLS_CFLAGS -DGCRYPT_NO_DEPRECATED +dnl gnutls 3.x moved some declarations to a new header +AC_CHECK_HEADERS([gnutls/crypto.h], [], [], [[#include gnutls/gnutls.h]]) + with_gnutls=yes fi diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c index e29c439..f2ac551 100644 --- a/src/rpc/virnettlscontext.c +++ b/src/rpc/virnettlscontext.c @@ -25,6 +25,9 @@ #include stdlib.h #include gnutls/gnutls.h +#if HAVE_GNULTLS_CRYPTO_H +# include gnutls/crypto.h +#endif #include gnutls/x509.h #include gnutls_1_0_compat.h -- 1.8.1.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] build: fix build without libvirtd
Building when configured --with-libvirtd=no fails with: In file included from ../src/qemu/qemu_command.h:30:0, from testutilsqemu.h:4, from networkxml2xmltest.c:14: ../src/qemu/qemu_conf.h:175:5: error: expected specifier-qualifier-list before 'virStateInhibitCallback' * src/libvirt_internal.h (virStateInhibitCallback): Move outside of conditional. Signed-off-by: Eric Blake ebl...@redhat.com --- Pushing under build-breaker rule src/libvirt_internal.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libvirt_internal.h b/src/libvirt_internal.h index 5c83ffa..29f2043 100644 --- a/src/libvirt_internal.h +++ b/src/libvirt_internal.h @@ -1,7 +1,7 @@ /* - * libvirt.h: publically exported APIs, not for public use + * libvirt_internal.h: internally exported APIs, not for public use * - * Copyright (C) 2006-2008, 2011 Red Hat, Inc. + * Copyright (C) 2006-2013 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -27,10 +27,10 @@ # include internal.h -# ifdef WITH_LIBVIRTD typedef void (*virStateInhibitCallback)(bool inhibit, void *opaque); +# ifdef WITH_LIBVIRTD int virStateInitialize(bool privileged, virStateInhibitCallback inhibit, void *opaque); -- 1.8.1.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] storage: Refresh pool after creating volume
On 29/05/13 20:51, Martin Kletzander wrote: On 05/29/2013 11:53 AM, Osier Yang wrote: https://bugzilla.redhat.com/show_bug.cgi?id=965442 One has to refresh the pool to get the correct pool info, this patch refreshes the pool after creating a volume in code instead. Pool refreshing failure is fine to ignore with a warning. --- src/storage/storage_driver.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index a2b0c21..2a55095 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1443,6 +1443,9 @@ storageVolCreateXML(virStoragePoolPtr obj, } +if (backend-refreshPool backend-refreshPool(obj-conn, pool) 0) +VIR_WARN(Failed to refresh pool after creating volume); + VIR_INFO(Creating volume '%s' in storage pool '%s', volobj-name, pool-def-name); ret = volobj; @@ -1606,6 +1609,9 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj, goto cleanup; } +if (backend-refreshPool backend-refreshPool(obj-conn, pool) 0) +VIR_WARN(Failed to refresh pool after creating volume); + VIR_INFO(Creating volume '%s' in storage pool '%s', volobj-name, pool-def-name); ret = volobj; I don't want to say NACK just like that, but I think the bug indicates there's a problem in the storage driver. It should automatically reflect the changes made to that pool. That's the thing I mentioned long time ago. Using things like inotify to update the pool's info (though inotify doesn't work for all pool types, such as iscsi). What's the structure that gets updated only with refresh and not after the vol is created? Can you explain more about this? I guess you mean the vol is created outside of libvirt? such as a iscsi target create a new LUN? Does it do with all the drivers? Not sure what do you mean for drivers. But I guess you mean storage backends here? if so, yes. All the current storage backends support refreshPool/ Long story short; I think this bug fixes the symptom, not the problem. As said above, you have a right opinion. The pool should be notified asynchronously, but it's thing I don't have enough time to do currently. Osier -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] storage: Refresh pool after creating volume
On 05/29/2013 05:55 PM, Osier Yang wrote: On 29/05/13 20:51, Martin Kletzander wrote: On 05/29/2013 11:53 AM, Osier Yang wrote: https://bugzilla.redhat.com/show_bug.cgi?id=965442 One has to refresh the pool to get the correct pool info, this patch refreshes the pool after creating a volume in code instead. Pool refreshing failure is fine to ignore with a warning. --- src/storage/storage_driver.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index a2b0c21..2a55095 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1443,6 +1443,9 @@ storageVolCreateXML(virStoragePoolPtr obj, } +if (backend-refreshPool backend-refreshPool(obj-conn, pool) 0) +VIR_WARN(Failed to refresh pool after creating volume); + VIR_INFO(Creating volume '%s' in storage pool '%s', volobj-name, pool-def-name); ret = volobj; @@ -1606,6 +1609,9 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj, goto cleanup; } +if (backend-refreshPool backend-refreshPool(obj-conn, pool) 0) +VIR_WARN(Failed to refresh pool after creating volume); + VIR_INFO(Creating volume '%s' in storage pool '%s', volobj-name, pool-def-name); ret = volobj; I don't want to say NACK just like that, but I think the bug indicates there's a problem in the storage driver. It should automatically reflect the changes made to that pool. That's the thing I mentioned long time ago. Using things like inotify to update the pool's info (though inotify doesn't work for all pool types, such as iscsi). I didn't mean responding to user-created volumes. The user has to do pool-refresh after that, that's their business as they do something behind libvirt's back. But when the driver does something (with the pool), it should update its info accordingly without the need to refresh it. What's the structure that gets updated only with refresh and not after the vol is created? Can you explain more about this? I guess you mean the vol is created outside of libvirt? such as a iscsi target create a new LUN? Does it do with all the drivers? Not sure what do you mean for drivers. But I guess you mean storage backends here? if so, yes. All the current storage backends support refreshPool/ Yes, backends. I meant what drivers has this issue. Long story short; I think this bug fixes the symptom, not the problem. As said above, you have a right opinion. The pool should be notified asynchronously, but it's thing I don't have enough time to do currently. Not quite what I meant, corrected myself above. Martin -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] storage: Refresh pool after creating volume
On 30/05/13 00:04, Martin Kletzander wrote: On 05/29/2013 05:55 PM, Osier Yang wrote: On 29/05/13 20:51, Martin Kletzander wrote: On 05/29/2013 11:53 AM, Osier Yang wrote: https://bugzilla.redhat.com/show_bug.cgi?id=965442 One has to refresh the pool to get the correct pool info, this patch refreshes the pool after creating a volume in code instead. Pool refreshing failure is fine to ignore with a warning. --- src/storage/storage_driver.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index a2b0c21..2a55095 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1443,6 +1443,9 @@ storageVolCreateXML(virStoragePoolPtr obj, } +if (backend-refreshPool backend-refreshPool(obj-conn, pool) 0) +VIR_WARN(Failed to refresh pool after creating volume); + VIR_INFO(Creating volume '%s' in storage pool '%s', volobj-name, pool-def-name); ret = volobj; @@ -1606,6 +1609,9 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj, goto cleanup; } +if (backend-refreshPool backend-refreshPool(obj-conn, pool) 0) +VIR_WARN(Failed to refresh pool after creating volume); + VIR_INFO(Creating volume '%s' in storage pool '%s', volobj-name, pool-def-name); ret = volobj; I don't want to say NACK just like that, but I think the bug indicates there's a problem in the storage driver. It should automatically reflect the changes made to that pool. That's the thing I mentioned long time ago. Using things like inotify to update the pool's info (though inotify doesn't work for all pool types, such as iscsi). I didn't mean responding to user-created volumes. The user has to do pool-refresh after that, that's their business as they do something behind libvirt's back. Right in principle. But when the driver does something (with the pool), it should update its info accordingly without the need to refresh it. Right too. This applies to deleteVol/resizeVol too. But as I said, though calliing refreshPool in these APIs is not the best method, but it's the generic thing what current storage driver does. Assuming that we won't reply on 3rd project/tools, we have to introduce something like event to let the pool refresh itself. It's just not much better than calling refreshPool in the APIs, as it should call refreshPool anyway finally. What's the structure that gets updated only with refresh and not after the vol is created? Can you explain more about this? I guess you mean the vol is created outside of libvirt? such as a iscsi target create a new LUN? Does it do with all the drivers? Not sure what do you mean for drivers. But I guess you mean storage backends here? if so, yes. All the current storage backends support refreshPool/ Yes, backends. I meant what drivers has this issue. Long story short; I think this bug fixes the symptom, not the problem. As said above, you have a right opinion. The pool should be notified asynchronously, but it's thing I don't have enough time to do currently. Not quite what I meant, corrected myself above. Martin -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/3] qemu: migration: Improve p2p error if we can't open conn
On 05/28/2013 05:04 PM, Eric Blake wrote:
On 05/28/2013 02:44 PM, Cole Robinson wrote:
By actually showing the Open() error to the user
---
src/qemu/qemu_migration.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
Does qemuDomainObjExitRemote(vm) risk clobbering the last error message?
Since it potentially unrefs vm on the last reference, I'm wondering if
we have to cache the last error at the right point in time instead of
relying on current behavior. But obviously it worked in your testing as
written.
Yeah seems to work, and doesn't look like anything in virobject.c touches the
error APIs, so it think it's okay. Error objects are thread local, not tied to
any VM reference.
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 19b1236..9ac9be4 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -3472,7 +3472,8 @@ static int doPeer2PeerMigrate(virQEMUDriverPtr driver,
qemuDomainObjExitRemote(vm);
if (dconn == NULL) {
virReportError(VIR_ERR_OPERATION_FAILED,
- _(Failed to connect to remote libvirt URI %s),
dconnuri);
+ _(Failed to connect to remote libvirt URI %s: %s),
+ dconnuri, virGetLastErrorMessage());
virObjectUnref(cfg);
return -1;
Another alternative is to just return -1 directly instead of doing a
virReportError() that overwrites the original error, although I'm not
sure if that loses some context that would help the user. Can you
include a sample error message in your commit log that shows the
improvement? If so, I'm inclined to ACK this.
I think the current way is preferred, with migration it could be ambiguous
what connection the error is coming from. With the original code you see:
error: operation failed: Failed to connect to remote libvirt URI
qemu+ssh://root@192.168.123.142/system
If we just raised the connection error you get:
Cannot recv data: Host key verification failed.: Connection reset by peer
With my fix you get:
error: operation failed: Failed to connect to remote libvirt URI
qemu+ssh://root@192.168.123.142/system: Cannot recv data: Host key
verification failed.: Connection reset by peer
Unfortunately I just pushed before adding this to the commit message, sorry.
Thanks for the review.
- Cole
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/3] qemu: migration: error if tunnelled + storage specified
On 28.05.2013 22:44, Cole Robinson wrote:
Since as the code indicates it doesn't work yet, so let's be
explicit about it.
---
src/qemu/qemu_migration.c | 24 ++--
1 file changed, 10 insertions(+), 14 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 9ac9be4..ffc86a4 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -1900,12 +1900,13 @@ char *qemuMigrationBegin(virQEMUDriverPtr driver,
if (flags (VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_INC)
virQEMUCapsGet(priv-qemuCaps, QEMU_CAPS_NBD_SERVER)) {
/* TODO support NBD for TUNNELLED migration */
-if (flags VIR_MIGRATE_TUNNELLED)
-VIR_DEBUG(NBD in tunnelled migration is currently not
supported);
-else {
-cookieFlags |= QEMU_MIGRATION_COOKIE_NBD;
-priv-nbdPort = 0;
+if (flags VIR_MIGRATE_TUNNELLED) {
+virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, %s,
+_(NBD in tunnelled migration is currently not supported));
+goto cleanup;
}
+cookieFlags |= QEMU_MIGRATION_COOKIE_NBD;
+priv-nbdPort = 0;
}
if (!(mig = qemuMigrationEatCookie(driver, vm, NULL, 0, 0)))
@@ -2200,16 +2201,11 @@ done:
if (mig-nbd
flags (VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_INC)
virQEMUCapsGet(priv-qemuCaps, QEMU_CAPS_NBD_SERVER)) {
-/* TODO support NBD for TUNNELLED migration */
-if (flags VIR_MIGRATE_TUNNELLED)
-VIR_DEBUG(NBD in tunnelled migration is currently not
supported);
-else {
-if (qemuMigrationStartNBDServer(driver, vm, listenAddr) 0) {
-/* error already reported */
-goto endjob;
-}
-cookieFlags |= QEMU_MIGRATION_COOKIE_NBD;
+if (qemuMigrationStartNBDServer(driver, vm, listenAddr) 0) {
+/* error already reported */
+goto endjob;
}
+cookieFlags |= QEMU_MIGRATION_COOKIE_NBD;
}
if (qemuMigrationBakeCookie(mig, driver, vm, cookieout,
I know you've already pushed this but I don't think this is desired.
There are currently two ways to copy the storage during migration:
1) new one using nbd server
2) old one, using an 'copy_storage' attribute to 'migrate' command
The first one is preferred and turned on whenever libvirt detects both
sides support it. There's no way for a user to enforce one or another
way of copying the storage. So if the old way works even for tunnelled
migration, we should fall back to using the old way rather then erroring
out. The debug message was really just a debug message :)
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/3] qemu: migration: error if tunnelled + storage specified
On 05/29/2013 03:16 PM, Michal Privoznik wrote:
On 28.05.2013 22:44, Cole Robinson wrote:
Since as the code indicates it doesn't work yet, so let's be
explicit about it.
---
src/qemu/qemu_migration.c | 24 ++--
1 file changed, 10 insertions(+), 14 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 9ac9be4..ffc86a4 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -1900,12 +1900,13 @@ char *qemuMigrationBegin(virQEMUDriverPtr driver,
if (flags (VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_INC)
virQEMUCapsGet(priv-qemuCaps, QEMU_CAPS_NBD_SERVER)) {
/* TODO support NBD for TUNNELLED migration */
-if (flags VIR_MIGRATE_TUNNELLED)
-VIR_DEBUG(NBD in tunnelled migration is currently not
supported);
-else {
-cookieFlags |= QEMU_MIGRATION_COOKIE_NBD;
-priv-nbdPort = 0;
+if (flags VIR_MIGRATE_TUNNELLED) {
+virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, %s,
+_(NBD in tunnelled migration is currently not supported));
+goto cleanup;
}
+cookieFlags |= QEMU_MIGRATION_COOKIE_NBD;
+priv-nbdPort = 0;
}
if (!(mig = qemuMigrationEatCookie(driver, vm, NULL, 0, 0)))
@@ -2200,16 +2201,11 @@ done:
if (mig-nbd
flags (VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_INC)
virQEMUCapsGet(priv-qemuCaps, QEMU_CAPS_NBD_SERVER)) {
-/* TODO support NBD for TUNNELLED migration */
-if (flags VIR_MIGRATE_TUNNELLED)
-VIR_DEBUG(NBD in tunnelled migration is currently not
supported);
-else {
-if (qemuMigrationStartNBDServer(driver, vm, listenAddr) 0) {
-/* error already reported */
-goto endjob;
-}
-cookieFlags |= QEMU_MIGRATION_COOKIE_NBD;
+if (qemuMigrationStartNBDServer(driver, vm, listenAddr) 0) {
+/* error already reported */
+goto endjob;
}
+cookieFlags |= QEMU_MIGRATION_COOKIE_NBD;
}
if (qemuMigrationBakeCookie(mig, driver, vm, cookieout,
I know you've already pushed this but I don't think this is desired.
There are currently two ways to copy the storage during migration:
1) new one using nbd server
2) old one, using an 'copy_storage' attribute to 'migrate' command
The first one is preferred and turned on whenever libvirt detects both
sides support it. There's no way for a user to enforce one or another
way of copying the storage. So if the old way works even for tunnelled
migration, we should fall back to using the old way rather then erroring
out. The debug message was really just a debug message :)
Indeed I didn't consider that, sorry.
However given that the old storage migration support is 1) considered not very
useful and 2) more or less deprecated in upstream qemu, I still think my patch
is an improvement. The fact that a common combination of cli options falls
back to a kinda-working-but-not-really-supported version with virtually no
indication to the user is pretty confusing. More motivation to actually
implement it for tunnelled migration :)
But I won't put up a fight, so if you'd prefer a revert I'll ACK that.
- Cole
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] build: work around broken sasl header
Compilation on cygwin failed due to a bug in the sasl headers present on that platform (libsasl2-devel 2.1.26): In file included from rpc/virnetserverclient.c:27:0: /usr/include/sasl/sasl.h:230:38: error: expected declaration specifiers or '...' before 'size_t' * src/rpc/virnetserverclient.c (includes): Ensure size_t is defined before using sasl.h. Signed-off-by: Eric Blake ebl...@redhat.com --- Pushing under the build-breaker rule. src/rpc/virnetserverclient.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c index 0bd88ad..2fc4838 100644 --- a/src/rpc/virnetserverclient.c +++ b/src/rpc/virnetserverclient.c @@ -1,7 +1,7 @@ /* * virnetserverclient.c: generic network RPC server client * - * Copyright (C) 2006-2011 Red Hat, Inc. + * Copyright (C) 2006-2013 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -23,6 +23,7 @@ #include config.h +#include internal.h #if WITH_SASL # include sasl/sasl.h #endif -- 1.8.1.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] build: use correct rpc.h for lockd
On cygwin, the build failed with: In file included from ./rpc/virnetmessage.h:24:0, from ./rpc/virnetclient.h:29, from locking/lock_driver_lockd.c:31: ./rpc/virnetprotocol.h:9:21: fatal error: rpc/rpc.h: No such file or directory * src/Makefile.am (lockd_la_CFLAGS): Add XDR_CFLAGS. Signed-off-by: Eric Blake ebl...@redhat.com --- Pushing under build-breaker rule. src/Makefile.am | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/Makefile.am b/src/Makefile.am index 430a356..7c404b3 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -1727,7 +1727,9 @@ lockd_la_SOURCES = \ $(LOCK_DRIVER_LOCKD_SOURCES) \ $(LOCK_PROTOCOL_GENERATED) \ $(NULL) -lockd_la_CFLAGS = -I$(top_srcdir)/src/conf $(AM_CFLAGS) +lockd_la_CFLAGS = -I$(top_srcdir)/src/conf \ + $(XDR_CFLAGS) \ + $(AM_CFLAGS) lockd_la_LDFLAGS = -module -avoid-version lockd_la_LIBADD = ../gnulib/lib/libgnu.la libvirt-net-rpc.la libvirt-net-rpc-client.la augeas_DATA += locking/libvirt_lockd.aug -- 1.8.1.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] build: port vbox to cygwin
I have no idea if a Cygwin app can be made to directly interact with VBoxXPCOMC.dll, but this at least lets compilation of vbox finish rather than requiring me to ./configure --without-vbox. * src/vbox/vbox_XPCOMCGlue.c (DYNLIB_NAME): Assume .dll under cygwin. Signed-off-by: Eric Blake ebl...@redhat.com --- Although I'm tempted to push this under the build-breaker rule, and although I suspect no one else is trying to build libvirt on cygwin, I'll wait for a review on this one. An alternative, more conservative, patch might be to hack configure.ac to declare vbox and cygwin as incompatible. src/vbox/vbox_XPCOMCGlue.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/vbox/vbox_XPCOMCGlue.c b/src/vbox/vbox_XPCOMCGlue.c index 9719014..9cc41b0 100644 --- a/src/vbox/vbox_XPCOMCGlue.c +++ b/src/vbox/vbox_XPCOMCGlue.c @@ -3,6 +3,7 @@ */ /* + * Copyright (C) 2013 Red Hat, Inc. * Copyright (C) 2008-2009 Sun Microsystems, Inc. * * This file is part of a free software library; you can redistribute @@ -54,7 +55,7 @@ # define DYNLIB_NAMEVBoxXPCOMC.so #elif defined(__APPLE__) # define DYNLIB_NAMEVBoxXPCOMC.dylib -#elif defined(_MSC_VER) || defined(__OS2__) +#elif defined(_MSC_VER) || defined(__OS2__) || defined(__CYGWIN__) # define DYNLIB_NAMEVBoxXPCOMC.dll #else # error Port me -- 1.8.1.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] build: port qemu to cygwin
A cygwin build of the qemu driver fails with: qemu/qemu_process.c: In function 'qemuPrepareCpumap': qemu/qemu_process.c:1803:31: error: 'CPU_SETSIZE' undeclared (first use in this function) CPU_SETSIZE is a Linux extension in sched.h; a bit more portable is using sysconf if _SC_NPROCESSORS_CONF is defined (several platforms have it, including Cygwin). Ultimately, I would have preferred to use gnulib's 'nproc' module, but it is currently under an incompatible license. * src/qemu/qemu_conf.h (QEMUD_CPUMASK_LEN): Provide definition on cygwin. Signed-off-by: Eric Blake ebl...@redhat.com --- I'll wait for a review on this one, particularly since I'm still trying to solve another qemu failure on cygwin: qemu/qemu_monitor.c:418:9: error: passing argument 2 of 'sendmsg' from incmpatible pointer type /usr/include/sys/socket.h:42:11: note: expected 'const struct msghdr *' but argument is of type 'struct msghdr *' src/qemu/qemu_conf.h | 8 +++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index df0791e..42566b4 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -45,7 +45,13 @@ # include locking/lock_manager.h # include qemu_capabilities.h -# define QEMUD_CPUMASK_LEN CPU_SETSIZE +# ifdef CPU_SETSIZE /* Linux */ +# define QEMUD_CPUMASK_LEN CPU_SETSIZE +# elif defined(_SC_NPROCESSORS_CONF) /* Cygwin */ +# define QEMUD_CPUMASK_LEN (sysconf(_SC_NPROCESSORS_CONF)) +# else +# error Port me +# endif typedef struct _virQEMUCloseCallbacks virQEMUCloseCallbacks; typedef virQEMUCloseCallbacks *virQEMUCloseCallbacksPtr; -- 1.8.1.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
