Re: [libvirt] [PATCHv3] qemu: fix crash when mixing sync and async monitor jobs
At 07/30/2011 05:37 AM, Eric Blake write: On 07/29/2011 03:32 PM, Eric Blake wrote: Currently, we attempt to run sync job and async job at the same time. It means that the monitor commands for two jobs can be run in any order. v3: incorporate Wen's feedback - in particular, virProcessStartCPUs now checks for return type, restarting libvirt does not use an async job, and I didn't hit the deadlock in the same scenarios as I tested under v2. I still need to do migration testing before I'm convinced that this is right, but it's doing a lot better. Nope, just got a deadlock, by sending SIGINT (^C) during the middle of virsh managedsave. I'll have to keep looking for that culprit... I think it's not a deadlock. Some threads of libvirtd quited, but the thread to do managedsave does not quit, and it is blocked in qemuMonitorSend(), and there is no thread to do monitor IO. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv4 2/2] save: generate idempotent inactive xml for running domain
On 07/29/2011 02:56 PM, Laine Stump wrote: On 07/29/2011 01:15 PM, Eric Blake wrote: Originally noticed by comparing the xml generated by virDomainSave with the xml produced by reparsing and redumping that xml, but I also did an audit of every last use of VIR_DOMAIN_XML_INACTIVE in domain_conf.c to ensure that no other discrepancies exist. Picky Picky Picky :-) ACK. Thanks; series pushed. -- Eric Blake ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv3] qemu: fix crash when mixing sync and async monitor jobs
On 07/29/2011 03:32 PM, Eric Blake wrote: Currently, we attempt to run sync job and async job at the same time. It means that the monitor commands for two jobs can be run in any order. v3: incorporate Wen's feedback - in particular, virProcessStartCPUs now checks for return type, restarting libvirt does not use an async job, and I didn't hit the deadlock in the same scenarios as I tested under v2. I still need to do migration testing before I'm convinced that this is right, but it's doing a lot better. Nope, just got a deadlock, by sending SIGINT (^C) during the middle of virsh managedsave. I'll have to keep looking for that culprit... -- Eric Blake ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] network: don't forward DNS requests from isolated networks
On 07/29/2011 04:43 PM, Eric Blake wrote:
On 07/29/2011 02:35 PM, Laine Stump wrote:
This is in response to:
https://bugzilla.redhat.com/show_bug.cgi?id=723862
which points out that a guest on an "isolated" network could
potentially exploit the DNS forwarding provided by dnsmasq to create a
communication channel to the outside.
This patch eliminates that possibility by adding the "--no-resolv"
argument to the dnsmasq commandline, which tells dnsmasq to not
forward on any requests that it can't resolv itself (by looking at its
s/resolv/resolve/
own static hosts files and runtime lsit of dhcp clients), but to
s/lsit/list/
instead return a failure for those requests.
This shouldn't cause any undesirable change from current
behavior, even in the case where a guest is currently configured with
multiple interfaces, one of them being connected to an isolated
network, and another to a network that does have connectivity to the
outside. If the isolated network's DNS server is queried for a name
it doesn't know, it will return "Refused" rather than "Unknown", which
indicates to the guest that it should query other servers, so it then
queries the connected DNS server, and gets the desired response.
---
src/network/bridge_driver.c | 11 ---
tests/networkxml2argvdata/isolated-network.argv |3 ++-
2 files changed, 10 insertions(+), 4 deletions(-)
A bug fix rather than a feature, and safe enough for inclusion in 0.9.4.
-if (network->def->forwardType == VIR_NETWORK_FORWARD_NONE)
-virCommandAddArg(cmd, "--dhcp-option=3");
+if (network->def->forwardType == VIR_NETWORK_FORWARD_NONE) {
+virCommandAddArgList(cmd, "--dhcp-option=3",
+ "--no-resolv", NULL);
+}
ACK.
Thanks, pushed with the indicated typos fixed.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Fix bug #611823 prohibit pools with duplicate storage
Hi Lei Li,
This patch doesn't seem to apply for me. Please recreate the patch
against an up to date git repository. Make sure to test that you can
apply the patch yourself.
On 07/28/2011 11:34 PM, Lei Li wrote:
> To make sure the unique storage pool defined and created from different
> directory to avoid inconsistent version of volume pool created, I add
> two API be called by storage driver to check for the probable duplicate
> pools and refused the duplicate pool.
>
> virStoragePoolObjFindByPath() provide a method to find pool object by
> target path in pool list.
> virStoragePoolTargetDuplicate() implement the function to check if there
> is duplicate pool.
> Add judgement for storagePoolCreate&storagePoolDefine by calling
> virStoragePoolTargetDuplicate() to avoid both transient storage pool and
> persistent storage pool be created repeatedly in storage driver.
>
>
> Signed-off-by: Lei Li
> ---
> src/conf/storage_conf.c | 39
> +++
> src/conf/storage_conf.h |4
> src/libvirt_private.syms |2 ++
> src/storage/storage_driver.c |6 ++
> 4 files changed, 51 insertions(+), 0 deletions(-)
>
> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
> index 995f9a6..a499e82 100644
> --- a/src/conf/storage_conf.c
> +++ b/src/conf/storage_conf.c
> @@ -1317,6 +1317,22 @@
> virStoragePoolObjFindByName(virStoragePoolObjListPtr pools,
> return NULL;
> }
>
> +virStoragePoolObjPtr
> +virStoragePoolObjFindByPath(virStoragePoolObjListPtr pools,
> +const char *path)
> +{
> +unsigned int i;
> +
> +for (i = 0 ; i< pools->count ; i++) {
You have some whitespace damage and coding style problems here.
for (i = 0; i < pools->count; i++) {
would be better.
> +virStoragePoolObjLock(pools->objs[i]);
> +if (STREQ(pools->objs[i]->def->target.path, path))
> +return pools->objs[i];
Here you have some more problems with indentation.
> +virStoragePoolObjUnlock(pools->objs[i]);
> +}
> +
> +return NULL;
> +}
> +
> void
> virStoragePoolObjClearVols(virStoragePoolObjPtr pool)
> {
> @@ -1707,6 +1723,29 @@ cleanup:
> return ret;
> }
>
> +int virStoragePoolTargetDuplicate(virStoragePoolObjListPtr pools,
> + virStoragePoolDefPtr def)
> +{
> +int ret = 1;
> +virStoragePoolObjPtr pool = NULL;
> +
> +/*check pool list if defined target path already exist*/
Spaces needed between opening /* and the actual comment. Same with the
closing */.
> +pool = virStoragePoolObjFindByPath(pools, def->target.path);
> +
> +if (pool) {
> +virStorageReportError(VIR_ERR_OPERATION_FAILED,
> + _("target path '%s' is already in use"),
> + pool->def->target.path);
> +ret = -1;
> +goto cleanup;
You can optimize this a bit by placing the virStoragePoolObjUnlock()
call in here and getting rid of the cleanup: label altogether.
> +}
> +
> +
> +cleanup:
> +if (pool)
> +virStoragePoolObjUnlock(pool);
> +return ret;
> +}
>
> void virStoragePoolObjLock(virStoragePoolObjPtr obj)
> {
> diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h
> index 271441a..454c43d 100644
> --- a/src/conf/storage_conf.h
> +++ b/src/conf/storage_conf.h
> @@ -335,6 +335,8 @@ virStoragePoolObjPtr
> virStoragePoolObjFindByUUID(virStoragePoolObjListPtr pools,
> const unsigned char
> *uuid);
> virStoragePoolObjPtr
> virStoragePoolObjFindByName(virStoragePoolObjListPtr pools,
> const char *name);
> +virStoragePoolObjPtr
> virStoragePoolObjFindByPath(virStoragePoolObjListPtr pools,
> + const char *path);
>
> virStorageVolDefPtr virStorageVolDefFindByKey(virStoragePoolObjPtr pool,
>const char *key);
> @@ -387,6 +389,8 @@ char
> *virStoragePoolSourceListFormat(virStoragePoolSourceListPtr def);
> int virStoragePoolObjIsDuplicate(virStoragePoolObjListPtr pools,
> virStoragePoolDefPtr def,
> unsigned int check_active);
> +int virStoragePoolTargetDuplicate(virStoragePoolObjListPtr pools,
> + virStoragePoolDefPtr def);
>
> void virStoragePoolObjLock(virStoragePoolObjPtr obj);
> void virStoragePoolObjUnlock(virStoragePoolObjPtr obj);
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 853ee62..ef323f5 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -930,7 +930,9 @@ virStoragePoolObjClearVols;
> virStoragePoolObjDeleteDef;
> virStoragePoolObjFindByName;
> virStoragePoolObjFindByUUID;
> +virStoragePoolObjFindByPath;
> virStoragePoolObjIsDuplicate;
> +virStoragePoolTargetDuplicate;
> virStoragePoolObjListFree;
Re: [libvirt] [PATCHv4 2/2] save: generate idempotent inactive xml for running domain
On 07/29/2011 01:15 PM, Eric Blake wrote: Originally noticed by comparing the xml generated by virDomainSave with the xml produced by reparsing and redumping that xml, but I also did an audit of every last use of VIR_DOMAIN_XML_INACTIVE in domain_conf.c to ensure that no other discrepancies exist. Picky Picky Picky :-) ACK. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv4 1/2] conf: make 'vnet' prefix a macro
On 07/29/2011 01:15 PM, Eric Blake wrote: Using a macro ensures that all the code is looking for the same prefix. * src/conf/domain_conf.h (VIR_NET_GENERATED_PREFIX): New macro. * src/conf/domain_conf.c (virDomainNetDefParseXML): Use it. * src/uml/uml_conf.c (umlConnectTapDevice): Likewise. * src/qemu/qemu_command.c (qemuNetworkIfaceConnect): Likewise. Suggested by Laine Stump. --- ACK -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] network: don't forward DNS requests from isolated networks
On 07/29/2011 02:35 PM, Laine Stump wrote:
This is in response to:
https://bugzilla.redhat.com/show_bug.cgi?id=723862
which points out that a guest on an "isolated" network could
potentially exploit the DNS forwarding provided by dnsmasq to create a
communication channel to the outside.
This patch eliminates that possibility by adding the "--no-resolv"
argument to the dnsmasq commandline, which tells dnsmasq to not
forward on any requests that it can't resolv itself (by looking at its
s/resolv/resolve/
own static hosts files and runtime lsit of dhcp clients), but to
s/lsit/list/
instead return a failure for those requests.
This shouldn't cause any undesirable change from current
behavior, even in the case where a guest is currently configured with
multiple interfaces, one of them being connected to an isolated
network, and another to a network that does have connectivity to the
outside. If the isolated network's DNS server is queried for a name
it doesn't know, it will return "Refused" rather than "Unknown", which
indicates to the guest that it should query other servers, so it then
queries the connected DNS server, and gets the desired response.
---
src/network/bridge_driver.c | 11 ---
tests/networkxml2argvdata/isolated-network.argv |3 ++-
2 files changed, 10 insertions(+), 4 deletions(-)
A bug fix rather than a feature, and safe enough for inclusion in 0.9.4.
-if (network->def->forwardType == VIR_NETWORK_FORWARD_NONE)
-virCommandAddArg(cmd, "--dhcp-option=3");
+if (network->def->forwardType == VIR_NETWORK_FORWARD_NONE) {
+virCommandAddArgList(cmd, "--dhcp-option=3",
+ "--no-resolv", NULL);
+}
ACK.
--
Eric Blake ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] network: don't forward DNS requests from isolated networks
This is in response to:
https://bugzilla.redhat.com/show_bug.cgi?id=723862
which points out that a guest on an "isolated" network could
potentially exploit the DNS forwarding provided by dnsmasq to create a
communication channel to the outside.
This patch eliminates that possibility by adding the "--no-resolv"
argument to the dnsmasq commandline, which tells dnsmasq to not
forward on any requests that it can't resolv itself (by looking at its
own static hosts files and runtime lsit of dhcp clients), but to
instead return a failure for those requests.
This shouldn't cause any undesirable change from current
behavior, even in the case where a guest is currently configured with
multiple interfaces, one of them being connected to an isolated
network, and another to a network that does have connectivity to the
outside. If the isolated network's DNS server is queried for a name
it doesn't know, it will return "Refused" rather than "Unknown", which
indicates to the guest that it should query other servers, so it then
queries the connected DNS server, and gets the desired response.
---
src/network/bridge_driver.c | 11 ---
tests/networkxml2argvdata/isolated-network.argv |3 ++-
2 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index b8c6c97..0a60bb8 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -531,10 +531,15 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network,
/* If this is an isolated network, set the default route option
* (3) to be empty to avoid setting a default route that's
- * guaranteed to not work.
+ * guaranteed to not work, and set --no-resolv so that no dns
+ * requests are forwarded on to the dns server listed in the
+ * host's /etc/resolv.conf (since this could be used as a channel
+ * to build a connection to the outside).
*/
-if (network->def->forwardType == VIR_NETWORK_FORWARD_NONE)
-virCommandAddArg(cmd, "--dhcp-option=3");
+if (network->def->forwardType == VIR_NETWORK_FORWARD_NONE) {
+virCommandAddArgList(cmd, "--dhcp-option=3",
+ "--no-resolv", NULL);
+}
if (network->def->dns != NULL) {
virNetworkDNSDefPtr dns = network->def->dns;
diff --git a/tests/networkxml2argvdata/isolated-network.argv
b/tests/networkxml2argvdata/isolated-network.argv
index f801396..7ea2e94 100644
--- a/tests/networkxml2argvdata/isolated-network.argv
+++ b/tests/networkxml2argvdata/isolated-network.argv
@@ -1,5 +1,6 @@
/usr/sbin/dnsmasq --strict-order --bind-interfaces --conf-file= \
---except-interface lo --dhcp-option=3 --listen-address 192.168.152.1 \
+--except-interface lo --dhcp-option=3 --no-resolv \
+--listen-address 192.168.152.1 \
--dhcp-range 192.168.152.2,192.168.152.254 \
--dhcp-leasefile=/var/lib/libvirt/dnsmasq/private.leases --dhcp-lease-max=253 \
--dhcp-no-override\
--
1.7.3.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] build: fix include path for cygwin
Without this, cygwin failed to compile:
In file included from ../src/rpc/virnetmessage.h:24,
from ../src/rpc/virnetclient.h:27,
from remote/remote_driver.c:31:
../src/rpc/virnetprotocol.h:9:21: error: rpc/rpc.h: No such file or directory
With that fixed, compilation warned:
rpc/virnetsocket.c: In function 'virNetSocketNewListenUNIX':
rpc/virnetsocket.c:347: warning: format '%d' expects type 'int', but argument 8
has type 'gid_t' [-Wformat]
rpc/virnetsocket.c: In function 'virNetSocketGetLocalIdentity':
rpc/virnetsocket.c:743: warning: pointer targets in passing argument 5 of
'getsockopt' differ in signedness
* src/Makefile.am (libvirt_driver_remote_la_CFLAGS)
(libvirt_net_rpc_client_la_CFLAGS)
(libvirt_net_rpc_server_la_CFLAGS): Include XDR_CFLAGS, for rpc
headers on cygwin.
* src/rpc/virnetsocket.c (virNetSocketNewListenUNIX)
(virNetSocketGetLocalIdentity): Avoid compiler warnings.
---
Pushing under the build-breaker rule.
src/Makefile.am|5 -
src/rpc/virnetsocket.c |6 +++---
2 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/src/Makefile.am b/src/Makefile.am
index b7e4991..009ff25 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -578,6 +578,7 @@ libvirt_la_BUILT_LIBADD += libvirt_driver_remote.la
endif
libvirt_driver_remote_la_CFLAGS = \
$(GNUTLS_CFLAGS)\
+ $(XDR_CFLAGS) \
-I@top_srcdir@/src/conf \
-I@top_srcdir@/src/rpc \
$(AM_CFLAGS)
@@ -1293,6 +1294,7 @@ EXTRA_DIST += \
endif
libvirt_net_rpc_server_la_CFLAGS = \
$(AVAHI_CFLAGS) \
+ $(XDR_CFLAGS) \
$(AM_CFLAGS) \
$(POLKIT_CFLAGS)
libvirt_net_rpc_server_la_LDFLAGS = \
@@ -1309,7 +1311,8 @@ libvirt_net_rpc_client_la_SOURCES = \
rpc/virnetclientstream.h rpc/virnetclientstream.c \
rpc/virnetclient.h rpc/virnetclient.c
libvirt_net_rpc_client_la_CFLAGS = \
- $(AM_CFLAGS)
+ $(AM_CFLAGS) \
+ $(XDR_CFLAGS)
libvirt_net_rpc_client_la_LDFLAGS = \
$(AM_LDFLAGS) \
$(CYGWIN_EXTRA_LDFLAGS) \
diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c
index dcdc937..41b691a 100644
--- a/src/rpc/virnetsocket.c
+++ b/src/rpc/virnetsocket.c
@@ -345,8 +345,8 @@ int virNetSocketNewListenUNIX(const char *path,
*/
if (grp != 0 && chown(path, -1, grp)) {
virReportSystemError(errno,
- _("Failed to change group ID of '%s' to %d"),
- path, grp);
+ _("Failed to change group ID of '%s' to %u"),
+ path, (unsigned int) grp);
goto error;
}
@@ -737,7 +737,7 @@ int virNetSocketGetLocalIdentity(virNetSocketPtr sock,
pid_t *pid)
{
struct ucred cr;
-unsigned int cr_len = sizeof (cr);
+socklen_t cr_len = sizeof (cr);
virMutexLock(&sock->lock);
if (getsockopt(sock->fd, SOL_SOCKET, SO_PEERCRED, &cr, &cr_len) < 0) {
--
1.7.4.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Start of freeze for libvirt-0.9.4 and availability of rc1
2011/7/26 Jason Helfman : > On Tue, Jul 26, 2011 at 12:26:20PM -0600, Eric Blake thus spake: >> >> On 07/26/2011 12:16 PM, Jason Helfman wrote: >>> >>> remote.c: At top level: >>> remote.c:409: error: negative width in bit-field >>> '_gl_verify_error_if_negative' >>> remote.c: In function 'remoteDispatchDomainGetBlockJobInfo': >>> remote.c:1630: error: 'virDomainBlockJobInfo' undeclared (first use in >>> this >>> function) >> >> Ah. You're running into the same problem that has been previously >> reported of compiling against the stale installed libvirt.h instead of >> the just-built in-tree libvirt.h. >> >> Matthias had started a patch for that, but it never got finished. >> >> https://www.redhat.com/archives/libvir-list/2011-May/msg01926.html >> >> > I de-installed the port, and then continued with the make process, and it > installed just fine. > > What is the status of this patch? If this isn't going to make it into the > release, I can warn users that this port needs to be de-installed prior to > building the port. It's fixed in git now http://libvirt.org/git/?p=libvirt.git;a=commit;h=b590866bdb0aea20eda5b96883b8744fedbba88d But it missed 0.9.4 RC2, so depending on how Daniel plans to do the release of 0.9.4 it might not be included. -- Matthias Bolte http://photron.blogspot.com -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] build: avoid non-portable shell in test setup
On 07/29/2011 10:07 AM, Matthias Bolte wrote: 2011/7/29 Eric Blake: POSIX states that 'a=1; a=2 b=$a command' has unspecified results for the value of $b visible within command. In particular, on BSD, this resulted in PATH not picking up the in-test ssh. * tests/Makefile.am (lv_abs_top_builddir): New macro. (path_add, TESTS_ENVIRONMENT): Use it to avoid referring to an environment variable set previously within the same command line. Reported by Matthias Bolte. --- ACK, this make gmake check pass completely on FreeBSD. Pushed. -- Eric Blake ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] How to connect to the running VM
On Fri, Jul 29, 2011 at 10:36:48AM -0700, bala suru wrote: > Hi, > Can you suggest any links /docs which explain the kvm image creation steps > ..? Replying on list for the benefit of everyone. I don't know what you're referring to when you say kvm image. Are you asking how to install the OS in the guest? Dave > rgds > > On Fri, Jul 29, 2011 at 9:17 AM, Dave Allan wrote: > > > On Fri, Jul 29, 2011 at 05:06:06PM +0530, bala suru wrote: > > > Hi, > > > I have deployed some VM on to the KVM-qemu and installed libvirtd .. > > > > > > I could see the VM running by command virsh list . > > > > > > but how to login to the VMs other than SSH ..? i tried virsh vncdisplay , > > > but no output .. > > > > virt-viewer > > > > will give you the graphical console if you have one set up. > > > > virt-manager gives you both graphical console and a graphical > > interface to configure the VM. > > > > Dave > > > > > regards > > > bala > > > > > -- > > > libvir-list mailing list > > > libvir-list@redhat.com > > > https://www.redhat.com/mailman/listinfo/libvir-list > > > > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC: PATCHv3 3/3] save: generate idempotent inactive xml for running domain
On 07/28/2011 12:59 PM, Laine Stump wrote:
On 07/22/2011 12:21 AM, Eric Blake wrote:
Noticed by comparing the xml generated by virDomainSave with the
xml produced by reparsing and redumping that xml.
* src/conf/domain_conf.c (virDomainDeviceInfoIsSet): Add
parameter, and update all callers. Make static.
(virDomainNetDefFormat): Skip generated ifname.
* src/conf/domain_conf.h (virDomainDeviceInfoIsSet): Delete.
* src/libvirt_private.syms (domain_conf.h): Update.
---
Sending this now, to get review started, but I still have some
more fixing to do - right now, active domains still include:
+
which is not present on reparse, but I'm too tired to find out why.
I know the feeling :-)
Now that I've had some sleep (and 6 days have elapsed), I've finally
gotten back to this patch. :-)
So does it turn out that this is important, or not?
It _would_ be, if we cared about non-empty model on inactive parse.
That is, if we _wanted_ to force a dynamic security model of selinux
instead of apparmor, then the inactive parse needs to be taught to parse
model, and enforce that the model is supported by the current host (and
prevent migrations between selinux and apparmor machines). But since
that particular merely represents the default, and by default
you want a secure machine regardless of which security model your host
supports, I simply fixed the formatter to omit default information
rather than teaching the parser to honor an explicit model (that is,
existing behavior has always been to ignore model on inactive parse).
+
+ if (def->ifname&&
+ !((flags& VIR_DOMAIN_XML_INACTIVE)&&
+ (STRPREFIX(def->ifname, "vnet" {
+ /* Skip auto-generated target names for inactive config. */
It's kind of bothersome that use of this magic device name prefix isn't
self-contained in domain_conf.c (or somewhere else). Perhaps the string
could be defined in domain_conf.h, then used here and in qemu_command.c
(is it used any place else?).
Split into a separate patch - uml_conf.c also used it.
v4 now posted, and my audit of domain_conf.c is now complete.
https://www.redhat.com/archives/libvir-list/2011-July/msg02064.html
--
Eric Blake ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv4 2/2] save: generate idempotent inactive xml for running domain
Originally noticed by comparing the xml generated by virDomainSave
with the xml produced by reparsing and redumping that xml, but I
also did an audit of every last use of VIR_DOMAIN_XML_INACTIVE in
domain_conf.c to ensure that no other discrepancies exist.
* src/conf/domain_conf.c (virDomainDeviceInfoIsSet): Add
parameter, and update all callers. Make static.
(virDomainNetDefFormat): Skip generated ifname.
(virDomainDefFormatInternal): Skip default .
(virDomainChrSourceDefParseXML): Skip generated pty path, and add
parameter. Update callers.
* src/conf/domain_conf.h (virDomainDeviceInfoIsSet): Delete.
* src/libvirt_private.syms (domain_conf.h): Update.
---
v4: also tweak virDomainDefFormatInternal, virDomainChrSourceDefParseXML,
patch is now fully tested and complete
v3: new patch in RFC state
src/conf/domain_conf.c | 41 -
src/conf/domain_conf.h |1 -
src/libvirt_private.syms |1 -
3 files changed, 24 insertions(+), 19 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 72eccde..e182cd6 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1411,11 +1411,12 @@ int virDomainDeviceVirtioSerialAddressIsValid(
}
-int virDomainDeviceInfoIsSet(virDomainDeviceInfoPtr info)
+static int
+virDomainDeviceInfoIsSet(virDomainDeviceInfoPtr info, unsigned int flags)
{
if (info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE)
return 1;
-if (info->alias)
+if (info->alias && !(flags & VIR_DOMAIN_XML_INACTIVE))
return 1;
return 0;
}
@@ -3297,7 +3298,7 @@ error:
* , which is used by but not ). */
static int
virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
- xmlNodePtr cur)
+ xmlNodePtr cur, unsigned int flags)
{
char *bindHost = NULL;
char *bindService = NULL;
@@ -3320,7 +3321,10 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr
def,
case VIR_DOMAIN_CHR_TYPE_FILE:
case VIR_DOMAIN_CHR_TYPE_PIPE:
case VIR_DOMAIN_CHR_TYPE_UNIX:
-if (path == NULL)
+/* PTY path is only parsed from live xml. */
+if (path == NULL &&
+(def->type != VIR_DOMAIN_CHR_TYPE_PTY ||
+ !(flags & VIR_DOMAIN_XML_INACTIVE)))
path = virXMLPropString(cur, "path");
break;
@@ -3571,7 +3575,7 @@ virDomainChrDefParseXML(virCapsPtr caps,
}
cur = node->children;
-remaining = virDomainChrSourceDefParseXML(&def->source, cur);
+remaining = virDomainChrSourceDefParseXML(&def->source, cur, flags);
if (remaining < 0)
goto error;
if (remaining) {
@@ -3703,7 +3707,7 @@ virDomainSmartcardDefParseXML(xmlNodePtr node,
}
cur = node->children;
-if (virDomainChrSourceDefParseXML(&def->data.passthru, cur) < 0)
+if (virDomainChrSourceDefParseXML(&def->data.passthru, cur, flags) < 0)
goto error;
if (def->data.passthru.type == VIR_DOMAIN_CHR_TYPE_SPICEVMC) {
@@ -8736,7 +8740,7 @@ virDomainControllerDefFormat(virBufferPtr buf,
break;
}
-if (virDomainDeviceInfoIsSet(&def->info)) {
+if (virDomainDeviceInfoIsSet(&def->info, flags)) {
virBufferAddLit(buf, ">\n");
if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0)
return -1;
@@ -8966,9 +8970,14 @@ virDomainNetDefFormat(virBufferPtr buf,
break;
}
-if (def->ifname)
+
+if (def->ifname &&
+!((flags & VIR_DOMAIN_XML_INACTIVE) &&
+ (STRPREFIX(def->ifname, VIR_NET_GENERATED_PREFIX {
+/* Skip auto-generated target names for inactive config. */
virBufferEscapeString(buf, " \n",
def->ifname);
+}
if (def->model) {
virBufferEscapeString(buf, " \n",
def->model);
@@ -9204,7 +9213,7 @@ virDomainChrDefFormat(virBufferPtr buf,
break;
}
-if (virDomainDeviceInfoIsSet(&def->info)) {
+if (virDomainDeviceInfoIsSet(&def->info, flags)) {
if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0)
return -1;
}
@@ -9232,7 +9241,7 @@ virDomainSmartcardDefFormat(virBufferPtr buf,
virBufferAsprintf(buf, "type) {
case VIR_DOMAIN_SMARTCARD_TYPE_HOST:
-if (!virDomainDeviceInfoIsSet(&def->info)) {
+if (!virDomainDeviceInfoIsSet(&def->info, flags)) {
virBufferAddLit(buf, "/>\n");
return 0;
}
@@ -9282,7 +9291,7 @@ virDomainSoundDefFormat(virBufferPtr buf,
virBufferAsprintf(buf, "info)) {
+if (virDomainDeviceInfoIsSet(&def->info, flags)) {
virBufferAddLit(buf, ">\n");
if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0)
return -1;
@@ -9311,7 +9320,7 @@ virDomainMemballoonDefFormat
[libvirt] [PATCHv4 1/2] conf: make 'vnet' prefix a macro
Using a macro ensures that all the code is looking for the same
prefix.
* src/conf/domain_conf.h (VIR_NET_GENERATED_PREFIX): New macro.
* src/conf/domain_conf.c (virDomainNetDefParseXML): Use it.
* src/uml/uml_conf.c (umlConnectTapDevice): Likewise.
* src/qemu/qemu_command.c (qemuNetworkIfaceConnect): Likewise.
Suggested by Laine Stump.
---
v4: new patch
src/conf/domain_conf.c |2 +-
src/conf/domain_conf.h |4
src/qemu/qemu_command.c |8
src/uml/uml_conf.c |8
4 files changed, 13 insertions(+), 9 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 257a1ea..72eccde 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2819,7 +2819,7 @@ virDomainNetDefParseXML(virCapsPtr caps,
ifname = virXMLPropString(cur, "dev");
if ((ifname != NULL) &&
((flags & VIR_DOMAIN_XML_INACTIVE) &&
- (STRPREFIX((const char*)ifname, "vnet" {
+ (STRPREFIX(ifname, VIR_NET_GENERATED_PREFIX {
/* An auto-generated target name, blank it out */
VIR_FREE(ifname);
}
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index c748f52..dd33eb0 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -429,6 +429,10 @@ struct _virDomainNetDef {
virBandwidthPtr bandwidth;
};
+/* Used for prefix of ifname of any network name generated dynamically
+ * by libvirt, and cannot be used for a persistent network name. */
+# define VIR_NET_GENERATED_PREFIX "vnet"
+
enum virDomainChrDeviceType {
VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL = 0,
VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL,
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index ee42f1d..6a2e2ae 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -188,7 +188,7 @@ qemuNetworkIfaceConnect(virDomainDefPtr def,
int err;
int tapfd = -1;
int vnet_hdr = 0;
-int template_ifname = 0;
+bool template_ifname = false;
unsigned char tapmac[VIR_MAC_BUFLEN];
int actualType = virDomainNetGetActualType(net);
@@ -244,15 +244,15 @@ qemuNetworkIfaceConnect(virDomainDefPtr def,
}
if (!net->ifname ||
-STRPREFIX(net->ifname, "vnet") ||
+STRPREFIX(net->ifname, VIR_NET_GENERATED_PREFIX) ||
strchr(net->ifname, '%')) {
VIR_FREE(net->ifname);
-if (!(net->ifname = strdup("vnet%d"))) {
+if (!(net->ifname = strdup(VIR_NET_GENERATED_PREFIX "%d"))) {
virReportOOMError();
goto cleanup;
}
/* avoid exposing vnet%d in getXMLDesc or error outputs */
-template_ifname = 1;
+template_ifname = true;
}
if (qemuCapsGet(qemuCaps, QEMU_CAPS_VNET_HDR) &&
diff --git a/src/uml/uml_conf.c b/src/uml/uml_conf.c
index 417271e..7b5e094 100644
--- a/src/uml/uml_conf.c
+++ b/src/uml/uml_conf.c
@@ -115,7 +115,7 @@ umlConnectTapDevice(virConnectPtr conn,
const char *bridge)
{
brControl *brctl = NULL;
-int template_ifname = 0;
+bool template_ifname = false;
int err;
unsigned char tapmac[VIR_MAC_BUFLEN];
@@ -126,13 +126,13 @@ umlConnectTapDevice(virConnectPtr conn,
}
if (!net->ifname ||
-STRPREFIX(net->ifname, "vnet") ||
+STRPREFIX(net->ifname, VIR_NET_GENERATED_PREFIX) ||
strchr(net->ifname, '%')) {
VIR_FREE(net->ifname);
-if (!(net->ifname = strdup("vnet%d")))
+if (!(net->ifname = strdup(VIR_NET_GENERATED_PREFIX "%d")))
goto no_memory;
/* avoid exposing vnet%d in getXMLDesc or error outputs */
-template_ifname = 1;
+template_ifname = true;
}
memcpy(tapmac, net->mac, VIR_MAC_BUFLEN);
--
1.7.4.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] How to connect to the running VM
? 2011?07?29? 19:36, bala suru ??: Hi, I have deployed some VM on to the KVM-qemu and installed libvirtd .. I could see the VM running by command virsh list . but how to login to the VMs other than SSH ..? i tried virsh vncdisplay , but no output .. This means you don't configure a "vnc" graphic for your guest. See # virsh help vncdisplay For what the command does. Except the vnc method, you might want to use text console, do like below will work: 1. Add console=ttyS0,115200 to guest kernel line. 2. Add the following XML section to a domain. 3. # virsh start $domain 4. # virsh console $domain Osier -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] How to connect to the running VM
On 07/29/11 17:32, bala suru wrote: > Hi, > Thanks for the information i'll try this .. > > How to create a kvm image from machine which already has the kvm setup > and virt-manager ..? > > So far I was using the images created for KVM for the VM .. now I want to > create a kvm image my self from iso image ... > > > regards > Bala You're welcome. As for images, use % virsh; and 'vol-create-as', or % qemu-img; command. % virsh -c qemu:///system ; and ' virsh # help vol-create-as ;' for details about this command. There is also % qemu-img;, however do not do things behind libvirt's back or it will backfire on you. I highly recommend to use either some cli tool/scripts eg. for VM definition or virt-manager or some other GUI tool. It makes your life easier and things go faster(and sometimes smoother). http://www.linux-kvm.org/page/Management_Tools Regards, Z. > On Fri, Jul 29, 2011 at 7:06 AM, Zdenek Styblik wrote: > >> On 07/29/11 13:36, bala suru wrote: >>> Hi, >>> I have deployed some VM on to the KVM-qemu and installed libvirtd .. >>> >>> I could see the VM running by command virsh list . >>> >>> but how to login to the VMs other than SSH ..? i tried virsh vncdisplay , >>> but no output .. >>> >>> regards >>> bala >>> >>> >>> >>> >>> -- >>> libvir-list mailing list >>> libvir-list@redhat.com >>> https://www.redhat.com/mailman/listinfo/libvir-list >> >> Hello, >> >> if VM is running at localhost eg. your workstation, I do: >> >> % netstat -nlp; >> >> look for '127.0.0.1:590x' as libvirt assigns VNC ports automatically and >> in incremental order. (Note: this, however, is not a rule. And you can >> assign whatever port you want by hand.) >> >> ~~~ SNIP ~~~ >> tcp0 0 localhost:5901 *:* >> LISTEN - >> ~~~ SNIP ~~~ >> >> Then I just use VNC client like % vncviewer localhost:5901; to connect >> to VM. >> >> Have you tried to hit a key or move the mouse? Console/screen might be >> in suspend mode in order to "save" power. >> >> Also, there might be few catches which depend on your setup. >> 1] are you sure VM has VNC console assigned? Use % virsh; and 'dumpxml >> ' command to check out. >> >> ~~~ SNIP ~~~ >> >> ~~~ SNIP ~~~ >> >> 2] it might be password protected, TLS might be required etc. etc. >> >> If it's at remote host, I recommend to use SSH to tunnel VNC port. You >> can find how-to at internet. >> You can also use 'virt-manager' which is included as package in many >> distributions. Well, at least in Fedora, Debian and, I believe, Ubuntu. >> >> I hope lines above help you a bit. >> >> Regards, >> Zdenek >> >> -- >> Zdenek Styblik >> email: sty...@turnovfree.net >> jabber: sty...@jabber.turnovfree.net >> > -- Zdenek Styblik email: sty...@turnovfree.net jabber: sty...@jabber.turnovfree.net -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] How to connect to the running VM
On Fri, Jul 29, 2011 at 05:06:06PM +0530, bala suru wrote: > Hi, > I have deployed some VM on to the KVM-qemu and installed libvirtd .. > > I could see the VM running by command virsh list . > > but how to login to the VMs other than SSH ..? i tried virsh vncdisplay , > but no output .. virt-viewer will give you the graphical console if you have one set up. virt-manager gives you both graphical console and a graphical interface to configure the VM. Dave > regards > bala > -- > libvir-list mailing list > libvir-list@redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] build: avoid non-portable shell in test setup
2011/7/29 Eric Blake :
> POSIX states that 'a=1; a=2 b=$a command' has unspecified results
> for the value of $b visible within command. In particular, on
> BSD, this resulted in PATH not picking up the in-test ssh.
>
> * tests/Makefile.am (lv_abs_top_builddir): New macro.
> (path_add, TESTS_ENVIRONMENT): Use it to avoid referring to an
> environment variable set previously within the same command line.
> Reported by Matthias Bolte.
> ---
>
> Spotted by inspection based on an IRC report; hopefully someone
> on BSD can test if it actually makes a difference.
>
> tests/Makefile.am | 10 +++---
> 1 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index 43a4301..f4afcb9 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -259,13 +259,17 @@ TESTS += interfacexml2xmltest
>
> TESTS += cputest
>
> -path_add =
> $$abs_top_builddir/daemon$(PATH_SEPARATOR)$$abs_top_builddir/tools$(PATH_SEPARATOR)$$abs_top_builddir/tests
> -
> # NB, automake < 1.10 does not provide the real
> # abs_top_{src/build}dir or builddir variables, so don't rely
> # on them here. Fake them with 'pwd'
> +# Also, BSD sh doesn't like 'a=b b=$$a', so we can't use an
> +# intermediate shell variable, but must do all the expansion in make
> +
> +lv_abs_top_builddir=`cd '$(top_builddir)'; pwd`
> +path_add =
> $(lv_abs_top_builddir)/daemon$(PATH_SEPARATOR)$(lv_abs_top_builddir)/tools$(PATH_SEPARATOR)$(lv_abs_top_builddir)/tests
> +
> TESTS_ENVIRONMENT = \
> - abs_top_builddir=`cd '$(top_builddir)'; pwd` \
> + abs_top_builddir=$(lv_abs_top_builddir) \
> abs_top_srcdir=`cd '$(top_srcdir)'; pwd` \
> abs_builddir=`pwd` \
> abs_srcdir=`cd '$(srcdir)'; pwd` \
> --
> 1.7.4.4
ACK, this make gmake check pass completely on FreeBSD.
--
Matthias Bolte
http://photron.blogspot.com
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] tests: Don't use bash if we don't have to
2011/7/29 Eric Blake : > On 07/29/2011 06:19 AM, Matthias Bolte wrote: >> >> This tested failed on FreeBSD because it was using bash, that might >> not be installed. >> --- >> tests/int-overflow | 2 +- >> 1 files changed, 1 insertions(+), 1 deletions(-) >> >> diff --git a/tests/int-overflow b/tests/int-overflow >> index baf2eef..36e5536 100755 >> --- a/tests/int-overflow >> +++ b/tests/int-overflow >> @@ -1,4 +1,4 @@ >> -#!/bin/bash >> +#!/bin/sh > > This script sources test-lib.sh, which in turn uses features like $() that > are not portable to Solaris /bin/sh. But I didn't see any bash-isms - > everything in those two files appears to be safe for use with POSIX sh, so > I'm okay with the patch as is: > > ACK. Thanks, pushed. -- Matthias Bolte http://photron.blogspot.com -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] build: avoid non-portable shell in test setup
POSIX states that 'a=1; a=2 b=$a command' has unspecified results
for the value of $b visible within command. In particular, on
BSD, this resulted in PATH not picking up the in-test ssh.
* tests/Makefile.am (lv_abs_top_builddir): New macro.
(path_add, TESTS_ENVIRONMENT): Use it to avoid referring to an
environment variable set previously within the same command line.
Reported by Matthias Bolte.
---
Spotted by inspection based on an IRC report; hopefully someone
on BSD can test if it actually makes a difference.
tests/Makefile.am | 10 +++---
1 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 43a4301..f4afcb9 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -259,13 +259,17 @@ TESTS += interfacexml2xmltest
TESTS += cputest
-path_add =
$$abs_top_builddir/daemon$(PATH_SEPARATOR)$$abs_top_builddir/tools$(PATH_SEPARATOR)$$abs_top_builddir/tests
-
# NB, automake < 1.10 does not provide the real
# abs_top_{src/build}dir or builddir variables, so don't rely
# on them here. Fake them with 'pwd'
+# Also, BSD sh doesn't like 'a=b b=$$a', so we can't use an
+# intermediate shell variable, but must do all the expansion in make
+
+lv_abs_top_builddir=`cd '$(top_builddir)'; pwd`
+path_add =
$(lv_abs_top_builddir)/daemon$(PATH_SEPARATOR)$(lv_abs_top_builddir)/tools$(PATH_SEPARATOR)$(lv_abs_top_builddir)/tests
+
TESTS_ENVIRONMENT =\
- abs_top_builddir=`cd '$(top_builddir)'; pwd` \
+ abs_top_builddir=$(lv_abs_top_builddir) \
abs_top_srcdir=`cd '$(top_srcdir)'; pwd` \
abs_builddir=`pwd` \
abs_srcdir=`cd '$(srcdir)'; pwd` \
--
1.7.4.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] utils: More useful error message for hook script failure
于 2011年07月29日 19:48, Eric Blake 写道:
On 07/29/2011 04:23 AM, Osier Yang wrote:
Commit 3709a386 ported hooks codes to new command execution API,
together with the useful error message removed. Though we can't
get "errbuf" from the new command execution API anymore, still
we can give a more useful error.
https://bugzilla.redhat.com/show_bug.cgi?id=726398
---
src/util/hooks.c | 9 -
1 files changed, 8 insertions(+), 1 deletions(-)
diff --git a/src/util/hooks.c b/src/util/hooks.c
index 64adfcb..00f3a01 100644
--- a/src/util/hooks.c
+++ b/src/util/hooks.c
@@ -193,6 +193,7 @@ int
virHookCall(int driver, const char *id, int op, int sub_op, const
char *extra,
const char *input) {
int ret;
+ int exitstatus;
char *path;
virCommandPtr cmd;
const char *drvstr;
@@ -257,7 +258,13 @@ virHookCall(int driver, const char *id, int op,
int sub_op, const char *extra,
if (input)
virCommandSetInputBuffer(cmd, input);
- ret = virCommandRun(cmd, NULL);
+ ret = virCommandRun(cmd,&exitstatus);
+ if (exitstatus != 0) {
Needs to be: if (ret == 0 && exitstatus != 0).
If ret is -1 (possible if the command completely failed, such as if
you are OOM or the child died due to a signal rather than a normal
exit), then exitstatus might be undefined.
Make sense, and I pushed with the addition, Thanks
Osier
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: fix crash when mixing sync and async monitor jobs
At 07/29/2011 09:34 PM, Eric Blake write:
On 07/29/2011 02:59 AM, Wen Congyang wrote:
At 07/29/2011 07:47 AM, Eric Blake Write:
Currently, we attempt to run sync job and async job at the same time. It
means that the monitor commands for two jobs can be run in any order.
In the function qemuDomainObjEnterMonitorInternal():
if (priv->job.active == QEMU_JOB_NONE&& priv->job.asyncJob) {
if (qemuDomainObjBeginNestedJob(driver, obj)< 0)
We check whether the caller is an async job by priv->job.active and
priv->job.asynJob. But when an async job is running, and a sync job is
also running at the time of the check, then priv->job.active is not
QEMU_JOB_NONE. So we cannot check whether the caller is an async job
in the function qemuDomainObjEnterMonitorInternal(), and must instead
put the burden on the caller to tell us when an async command wants
to do a nested job.
---
My initial smoke testing shows that this fixes 'virsh managedsave',
but I still have more testing to do before I'm convinced I got
everything (for example, I need to test migration still).
I test this patch with save by virt-manager, and find that it will cause
libvirt to be deadlock.
I'm seeing deadlock as well, in my further testing.
With this patch, we can ignore the return value of
qemuDomainObjEnterMonitor(WithDriver),
because these two functions always return 0. But we can not ignore the
return value of qemuDomainObjEnterMonitorAsync().
If qemuDomainObjEnterMonitorAsync() failed, we do nothing in this
function.
So it's very dangerous to call qemuDomainObjExitMonitorWithDriver() when
qemuDomainObjEnterMonitorAsync() failed.
I think this problem already exists before this patch.
First, a meta-question - is the approach of this patch better than the
approach of your patch (that is, this patch was attempting to make the
sync job condvar be the only condition used for starting a monitor
command, and the async job condvar merely ensures that only one async
job can be run at once and that an async monitor command corresponds to
the current async job)? Or is this patch beyond hope with its deadlock
problems, so that we should go with your patch (adding a new condvar in
the monitor to allow both sync job and async job to request a monitor
command, with the monitor doing the serialization)?
First, I think your patch is better.
The reason of the deadlock problem is that: we ignore the return value of
qemuDomainObjEnterMonitor(WithDriver). Without your patch, it's very very
dangerous to ignore it if the job is async job.
With your patch, qemuDomainObjEnterMonitor(WithDriver) is changed to
qemuDomainObjEnterMonitorAsync()
if the job is async job. So with your patch, we can ignore
qemuDomainObjEnterMonitor(WithDriver)'s
return value safely. But we must not ignore
qemuDomainObjEnterMonitorAsync()'s return value.
-static int ATTRIBUTE_NONNULL(1)
+static int
qemuDomainObjEnterMonitorInternal(struct qemud_driver *driver,
bool driver_locked,
- virDomainObjPtr obj)
+ virDomainObjPtr obj,
+ enum qemuDomainAsyncJob asyncJob)
{
qemuDomainObjPrivatePtr priv = obj->privateData;
- if (priv->job.active == QEMU_JOB_NONE&& priv->job.asyncJob) {
+ if (asyncJob != QEMU_ASYNC_JOB_NONE) {
+ if (asyncJob != priv->job.asyncJob) {
When we recover a async job after livbirtd restart, priv->job.asyncJob
is QEMU_ASYNC_JOB_NONE,
because we call qemuDomainObjRestoreJob() to reset priv->job in the
function qemuProcessReconnect().
Can that be fixed with a tweak to qemuDomainObjRestoreJob()?
Maybe. But I think we can use QEMU_ASYNC_JOB_NONE when we recover or
cancel a job.
+ qemuReportError(VIR_ERR_INTERNAL_ERROR,
+ _("unepxected async job %d"), asyncJob);
+ return -1;
+ }
if (qemuDomainObjBeginJobInternal(driver, driver_locked, obj,
QEMU_JOB_ASYNC_NESTED,
QEMU_ASYNC_JOB_NONE)< 0)
return -1;
if (!virDomainObjIsActive(obj)) {
qemuReportError(VIR_ERR_OPERATION_FAILED, "%s",
_("domain is no longer running"));
return -1;
}
if the domain is not active after calling
qemuDomainObjBeginJobInternal(), we set
priv->job.active to QEMU_JOB_ASYNC_NESTED, but we do not clear it and
notify the other
thread which is waiting priv->job.cond.
Good catch.
@@ -2424,7 +2430,8 @@ qemuProcessRecoverJob(struct qemud_driver *driver,
reason == VIR_DOMAIN_PAUSED_SAVE) ||
reason == VIR_DOMAIN_PAUSED_UNKNOWN)) {
if (qemuProcessStartCPUs(driver, vm, conn,
- VIR_DOMAIN_RUNNING_UNPAUSED)< 0) {
+ VIR_DOMAIN_RUNNING_UNPAUSED,
+ job->asyncJob)< 0) {
As the above mentioned, we set priv->job.asynJob to
QEMU_ASYNC_JOB_NONE, and
priv->job.active is QEMU_JOB_MODIFY. I think we can use
QEMU_ASYNC_JOB_NONE
instead of job->asyncJob safely.
I'll give it a shot.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] How to connect to the running VM
On 07/29/11 13:36, bala suru wrote: > Hi, > I have deployed some VM on to the KVM-qemu and installed libvirtd .. > > I could see the VM running by command virsh list . > > but how to login to the VMs other than SSH ..? i tried virsh vncdisplay , > but no output .. > > regards > bala > > > > > -- > libvir-list mailing list > libvir-list@redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list Hello, if VM is running at localhost eg. your workstation, I do: % netstat -nlp; look for '127.0.0.1:590x' as libvirt assigns VNC ports automatically and in incremental order. (Note: this, however, is not a rule. And you can assign whatever port you want by hand.) ~~~ SNIP ~~~ tcp0 0 localhost:5901 *:* LISTEN - ~~~ SNIP ~~~ Then I just use VNC client like % vncviewer localhost:5901; to connect to VM. Have you tried to hit a key or move the mouse? Console/screen might be in suspend mode in order to "save" power. Also, there might be few catches which depend on your setup. 1] are you sure VM has VNC console assigned? Use % virsh; and 'dumpxml ' command to check out. ~~~ SNIP ~~~ ~~~ SNIP ~~~ 2] it might be password protected, TLS might be required etc. etc. If it's at remote host, I recommend to use SSH to tunnel VNC port. You can find how-to at internet. You can also use 'virt-manager' which is included as package in many distributions. Well, at least in Fedora, Debian and, I believe, Ubuntu. I hope lines above help you a bit. Regards, Zdenek -- Zdenek Styblik email: sty...@turnovfree.net jabber: sty...@jabber.turnovfree.net -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 2/2] freebsd: Fix build problem due to picking up the wrong libvirt.h
On 07/29/2011 07:16 AM, Matthias Bolte wrote: 2011/7/28 Eric Blake: From: Matthias Bolte Gettext annoyingly modifies CPPFLAGS in-place, putting -I/usr/local/include into the search patch if libintl headers must be used from that location. But since we must support automake 1.9.6 which lacks AM_CPPFLAGS, and since CPPFLAGS is used prior to INCLUDES, this means that the build picks up the _old_ installed libvirt.h in priority to the in-tree version, leading to all sorts of weird build failures on FreeBSD. Fix this by teaching configure to undo gettext's actions, but to keep any changes required by gettext at the end of INCLUDES after all in-tree locations are used first. Also requires adding a wrapper Makefile.am and making gnulib-tool create just gnulib.mk files during the bootstrap process. --- v1 is here: https://www.redhat.com/archives/libvir-list/2011-July/msg01984.html v2: update bootstrap.conf and gnulib/*/Makefile.am to fix gnulib compilation, update .gitignore to allow committing new files. Works. ACK. Thanks for testing. Now applied. -- Eric Blake ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: fix crash when mixing sync and async monitor jobs
On 07/29/2011 02:59 AM, Wen Congyang wrote:
At 07/29/2011 07:47 AM, Eric Blake Write:
Currently, we attempt to run sync job and async job at the same time. It
means that the monitor commands for two jobs can be run in any order.
In the function qemuDomainObjEnterMonitorInternal():
if (priv->job.active == QEMU_JOB_NONE&& priv->job.asyncJob) {
if (qemuDomainObjBeginNestedJob(driver, obj)< 0)
We check whether the caller is an async job by priv->job.active and
priv->job.asynJob. But when an async job is running, and a sync job is
also running at the time of the check, then priv->job.active is not
QEMU_JOB_NONE. So we cannot check whether the caller is an async job
in the function qemuDomainObjEnterMonitorInternal(), and must instead
put the burden on the caller to tell us when an async command wants
to do a nested job.
---
My initial smoke testing shows that this fixes 'virsh managedsave',
but I still have more testing to do before I'm convinced I got
everything (for example, I need to test migration still).
I test this patch with save by virt-manager, and find that it will cause
libvirt to be deadlock.
I'm seeing deadlock as well, in my further testing.
With this patch, we can ignore the return value of
qemuDomainObjEnterMonitor(WithDriver),
because these two functions always return 0. But we can not ignore the
return value of qemuDomainObjEnterMonitorAsync().
If qemuDomainObjEnterMonitorAsync() failed, we do nothing in this function.
So it's very dangerous to call qemuDomainObjExitMonitorWithDriver() when
qemuDomainObjEnterMonitorAsync() failed.
I think this problem already exists before this patch.
First, a meta-question - is the approach of this patch better than the
approach of your patch (that is, this patch was attempting to make the
sync job condvar be the only condition used for starting a monitor
command, and the async job condvar merely ensures that only one async
job can be run at once and that an async monitor command corresponds to
the current async job)? Or is this patch beyond hope with its deadlock
problems, so that we should go with your patch (adding a new condvar in
the monitor to allow both sync job and async job to request a monitor
command, with the monitor doing the serialization)?
-static int ATTRIBUTE_NONNULL(1)
+static int
qemuDomainObjEnterMonitorInternal(struct qemud_driver *driver,
bool driver_locked,
- virDomainObjPtr obj)
+ virDomainObjPtr obj,
+ enum qemuDomainAsyncJob asyncJob)
{
qemuDomainObjPrivatePtr priv = obj->privateData;
-if (priv->job.active == QEMU_JOB_NONE&& priv->job.asyncJob) {
+if (asyncJob != QEMU_ASYNC_JOB_NONE) {
+if (asyncJob != priv->job.asyncJob) {
When we recover a async job after livbirtd restart, priv->job.asyncJob is
QEMU_ASYNC_JOB_NONE,
because we call qemuDomainObjRestoreJob() to reset priv->job in the function
qemuProcessReconnect().
Can that be fixed with a tweak to qemuDomainObjRestoreJob()?
+qemuReportError(VIR_ERR_INTERNAL_ERROR,
+_("unepxected async job %d"), asyncJob);
+return -1;
+}
if (qemuDomainObjBeginJobInternal(driver, driver_locked, obj,
QEMU_JOB_ASYNC_NESTED,
QEMU_ASYNC_JOB_NONE)< 0)
return -1;
if (!virDomainObjIsActive(obj)) {
qemuReportError(VIR_ERR_OPERATION_FAILED, "%s",
_("domain is no longer running"));
return -1;
}
if the domain is not active after calling qemuDomainObjBeginJobInternal(), we
set
priv->job.active to QEMU_JOB_ASYNC_NESTED, but we do not clear it and notify
the other
thread which is waiting priv->job.cond.
Good catch.
@@ -2424,7 +2430,8 @@ qemuProcessRecoverJob(struct qemud_driver *driver,
reason == VIR_DOMAIN_PAUSED_SAVE) ||
reason == VIR_DOMAIN_PAUSED_UNKNOWN)) {
if (qemuProcessStartCPUs(driver, vm, conn,
- VIR_DOMAIN_RUNNING_UNPAUSED)< 0) {
+ VIR_DOMAIN_RUNNING_UNPAUSED,
+ job->asyncJob)< 0) {
As the above mentioned, we set priv->job.asynJob to QEMU_ASYNC_JOB_NONE, and
priv->job.active is QEMU_JOB_MODIFY. I think we can use QEMU_ASYNC_JOB_NONE
instead of job->asyncJob safely.
I'll give it a shot.
--
Eric Blake ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] tests: Don't use bash if we don't have to
On 07/29/2011 06:19 AM, Matthias Bolte wrote: This tested failed on FreeBSD because it was using bash, that might not be installed. --- tests/int-overflow |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/tests/int-overflow b/tests/int-overflow index baf2eef..36e5536 100755 --- a/tests/int-overflow +++ b/tests/int-overflow @@ -1,4 +1,4 @@ -#!/bin/bash +#!/bin/sh This script sources test-lib.sh, which in turn uses features like $() that are not portable to Solaris /bin/sh. But I didn't see any bash-isms - everything in those two files appears to be safe for use with POSIX sh, so I'm okay with the patch as is: ACK. (If we later want to be portable to Solaris, we should resync test-lib.sh with coreutils and/or gnulib, from where it was originally forked, since the latter now have means of finding and re-execing under a POSIX-like shell if /bin/sh wasn't strong enough.) -- Eric Blake ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 2/2] freebsd: Fix build problem due to picking up the wrong libvirt.h
2011/7/28 Eric Blake : > From: Matthias Bolte > > Gettext annoyingly modifies CPPFLAGS in-place, putting > -I/usr/local/include into the search patch if libintl headers > must be used from that location. But since we must support > automake 1.9.6 which lacks AM_CPPFLAGS, and since CPPFLAGS is used > prior to INCLUDES, this means that the build picks up the _old_ > installed libvirt.h in priority to the in-tree version, leading > to all sorts of weird build failures on FreeBSD. > > Fix this by teaching configure to undo gettext's actions, but > to keep any changes required by gettext at the end of INCLUDES > after all in-tree locations are used first. Also requires > adding a wrapper Makefile.am and making gnulib-tool create > just gnulib.mk files during the bootstrap process. > --- > > v1 is here: > https://www.redhat.com/archives/libvir-list/2011-July/msg01984.html > > v2: update bootstrap.conf and gnulib/*/Makefile.am to fix gnulib > compilation, update .gitignore to allow committing new files. Works. ACK. -- Matthias Bolte http://photron.blogspot.com -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] ANNOUNCE: ruby-libvirt 0.4.0
All, I'm pleased to announce the release of ruby-libvirt 0.4.0. ruby-libvirt is a ruby wrapper around the libvirt API. Version 0.4.0 brings new APIs, more documentation, and bugfixes: * Updated Domain class, implementing dom.memory_parameters=, dom.memory_parameters, dom.updated?, dom.migrate2, dom.migrate_to_uri2, dom.migrate_set_max_speed, dom.qemu_monitor_command, dom.blkio_parameters, dom.blkio_parameters=, dom.state, dom.open_console, dom.screenshot, and dom.inject_nmi * Implementation of the Stream class, which covers the libvirt virStream APIs * Add the ability to build against non-system libvirt libraries * Updated Error object, which now includes the libvirt code, component and level of the error, as well as all of the error constants from libvirt.h * Updated Connect class, implementing conn.sys_info, conn.stream, conn.interface_change_begin, conn.interface_change_commit, and conn.interface_change_rollback * Updated StorageVol class, implementing vol.download and vol.upload * Various bugfixes Version 0.4.0 is available from http://libvirt.org/ruby: Tarball: http://libvirt.org/ruby/download/ruby-libvirt-0.4.0.tgz Gem: http://libvirt.org/ruby/download/ruby-libvirt-0.4.0.gem It is also available from rubygems.org; to get the latest version, run: $ gem install ruby-libvirt As usual, if you run into questions, problems, or bugs, please feel free to mail me (clala...@redhat.com) and/or the libvirt mailing list. -- Chris Lalancette -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] tests: Don't use bash if we don't have to
This tested failed on FreeBSD because it was using bash, that might not be installed. --- tests/int-overflow |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/tests/int-overflow b/tests/int-overflow index baf2eef..36e5536 100755 --- a/tests/int-overflow +++ b/tests/int-overflow @@ -1,4 +1,4 @@ -#!/bin/bash +#!/bin/sh # Ensure that an invalid domain ID isn't interpreted as a valid one. # Before, an ID of 2^32+2 would be treated just like an ID of 2. -- 1.7.4.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] utils: More useful error message for hook script failure
On 07/29/2011 04:23 AM, Osier Yang wrote:
Commit 3709a386 ported hooks codes to new command execution API,
together with the useful error message removed. Though we can't
get "errbuf" from the new command execution API anymore, still
we can give a more useful error.
https://bugzilla.redhat.com/show_bug.cgi?id=726398
---
src/util/hooks.c |9 -
1 files changed, 8 insertions(+), 1 deletions(-)
diff --git a/src/util/hooks.c b/src/util/hooks.c
index 64adfcb..00f3a01 100644
--- a/src/util/hooks.c
+++ b/src/util/hooks.c
@@ -193,6 +193,7 @@ int
virHookCall(int driver, const char *id, int op, int sub_op, const char *extra,
const char *input) {
int ret;
+int exitstatus;
char *path;
virCommandPtr cmd;
const char *drvstr;
@@ -257,7 +258,13 @@ virHookCall(int driver, const char *id, int op, int
sub_op, const char *extra,
if (input)
virCommandSetInputBuffer(cmd, input);
-ret = virCommandRun(cmd, NULL);
+ret = virCommandRun(cmd,&exitstatus);
+if (exitstatus != 0) {
Needs to be: if (ret == 0 && exitstatus != 0).
If ret is -1 (possible if the command completely failed, such as if you
are OOM or the child died due to a signal rather than a normal exit),
then exitstatus might be undefined.
--
Eric Blake ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] How to connect to the running VM
Hi, I have deployed some VM on to the KVM-qemu and installed libvirtd .. I could see the VM running by command virsh list . but how to login to the VMs other than SSH ..? i tried virsh vncdisplay , but no output .. regards bala -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] Fix bug #611823 prohibit pools with duplicate storage
To make sure the unique storage pool defined and created from different
directory to avoid inconsistent version of volume pool created, I add two API
be called by storage driver to check for the probable duplicate pools and
refused the duplicate pool.
virStoragePoolObjFindByPath() provide a method to find pool object by target
path in pool list.
virStoragePoolTargetDuplicate() implement the function to check if there is
duplicate pool.
Add judgement for storagePoolCreate&storagePoolDefine by calling
virStoragePoolTargetDuplicate() to avoid both transient storage pool and persistent
storage pool be created repeatedly in storage driver.
Signed-off-by: Lei Li
---
src/conf/storage_conf.c | 39 +++
src/conf/storage_conf.h |4
src/libvirt_private.syms |2 ++
src/storage/storage_driver.c |6 ++
4 files changed, 51 insertions(+), 0 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 995f9a6..a499e82 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -1317,6 +1317,22 @@ virStoragePoolObjFindByName(virStoragePoolObjListPtr
pools,
return NULL;
}
+virStoragePoolObjPtr
+virStoragePoolObjFindByPath(virStoragePoolObjListPtr pools,
+const char *path)
+{
+unsigned int i;
+
+for (i = 0 ; i< pools->count ; i++) {
+virStoragePoolObjLock(pools->objs[i]);
+ if (STREQ(pools->objs[i]->def->target.path, path))
+ return pools->objs[i];
+ virStoragePoolObjUnlock(pools->objs[i]);
+}
+
+return NULL;
+}
+
void
virStoragePoolObjClearVols(virStoragePoolObjPtr pool)
{
@@ -1707,6 +1723,29 @@ cleanup:
return ret;
}
+int virStoragePoolTargetDuplicate(virStoragePoolObjListPtr pools,
+ virStoragePoolDefPtr def)
+{
+int ret = 1;
+virStoragePoolObjPtr pool = NULL;
+
+/*check pool list if defined target path already exist*/
+pool = virStoragePoolObjFindByPath(pools, def->target.path);
+
+if (pool) {
+virStorageReportError(VIR_ERR_OPERATION_FAILED,
+ _("target path '%s' is already in use"),
+ pool->def->target.path);
+ret = -1;
+goto cleanup;
+}
+
+
+cleanup:
+if (pool)
+virStoragePoolObjUnlock(pool);
+return ret;
+}
void virStoragePoolObjLock(virStoragePoolObjPtr obj)
{
diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h
index 271441a..454c43d 100644
--- a/src/conf/storage_conf.h
+++ b/src/conf/storage_conf.h
@@ -335,6 +335,8 @@ virStoragePoolObjPtr
virStoragePoolObjFindByUUID(virStoragePoolObjListPtr pools,
const unsigned char *uuid);
virStoragePoolObjPtr virStoragePoolObjFindByName(virStoragePoolObjListPtr
pools,
const char *name);
+virStoragePoolObjPtr virStoragePoolObjFindByPath(virStoragePoolObjListPtr
pools,
+ const char *path);
virStorageVolDefPtr virStorageVolDefFindByKey(virStoragePoolObjPtr pool,
const char *key);
@@ -387,6 +389,8 @@ char
*virStoragePoolSourceListFormat(virStoragePoolSourceListPtr def);
int virStoragePoolObjIsDuplicate(virStoragePoolObjListPtr pools,
virStoragePoolDefPtr def,
unsigned int check_active);
+int virStoragePoolTargetDuplicate(virStoragePoolObjListPtr pools,
+ virStoragePoolDefPtr def);
void virStoragePoolObjLock(virStoragePoolObjPtr obj);
void virStoragePoolObjUnlock(virStoragePoolObjPtr obj);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 853ee62..ef323f5 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -930,7 +930,9 @@ virStoragePoolObjClearVols;
virStoragePoolObjDeleteDef;
virStoragePoolObjFindByName;
virStoragePoolObjFindByUUID;
+virStoragePoolObjFindByPath;
virStoragePoolObjIsDuplicate;
+virStoragePoolTargetDuplicate;
virStoragePoolObjListFree;
virStoragePoolObjLock;
virStoragePoolObjRemove;
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index 9c353e3..8ee63f6 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -536,6 +536,9 @@ storagePoolCreate(virConnectPtr conn,
if (virStoragePoolObjIsDuplicate(&driver->pools, def, 1)< 0)
goto cleanup;
+if (virStoragePoolTargetDuplicate(&driver->pools, def)< 0)
+goto cleanup;
+
if ((backend = virStorageBackendForType(def->type)) == NULL)
goto cleanup;
@@ -589,6 +592,9 @@ storagePoolDefine(virConnectPtr conn,
if (virStoragePoolObjIsDuplicate(&driver->pools, def, 0)< 0)
goto cleanup;
+if (virStoragePoolTargetDuplicate(&driver->pools, def)< 0)
+goto cleanup;
+
if (virStorage
Re: [libvirt] [PATCH] utils: More useful error message for hook script failure
On Fri, Jul 29, 2011 at 06:23:41PM +0800, Osier Yang wrote:
> Commit 3709a386 ported hooks codes to new command execution API,
> together with the useful error message removed. Though we can't
> get "errbuf" from the new command execution API anymore, still
> we can give a more useful error.
>
> https://bugzilla.redhat.com/show_bug.cgi?id=726398
> ---
> src/util/hooks.c |9 -
> 1 files changed, 8 insertions(+), 1 deletions(-)
>
> diff --git a/src/util/hooks.c b/src/util/hooks.c
> index 64adfcb..00f3a01 100644
> --- a/src/util/hooks.c
> +++ b/src/util/hooks.c
> @@ -193,6 +193,7 @@ int
> virHookCall(int driver, const char *id, int op, int sub_op, const char
> *extra,
> const char *input) {
> int ret;
> +int exitstatus;
> char *path;
> virCommandPtr cmd;
> const char *drvstr;
> @@ -257,7 +258,13 @@ virHookCall(int driver, const char *id, int op, int
> sub_op, const char *extra,
> if (input)
> virCommandSetInputBuffer(cmd, input);
>
> -ret = virCommandRun(cmd, NULL);
> +ret = virCommandRun(cmd, &exitstatus);
> +if (exitstatus != 0) {
> +virHookReportError(VIR_ERR_HOOK_SCRIPT_FAILED,
> + _("Hook script %s %s failed with error code %d"),
> + path, drvstr, exitstatus);
> +ret = -1;
> +}
>
> virCommandFree(cmd);
>
ACK,
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
dan...@veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] freebsd: Avoid /bin/true in commandtest
2011/7/28 Eric Blake : > On 07/28/2011 09:52 AM, Matthias Bolte wrote: >> >> Rely on PATH and use just true, because on FreeBSD it's /usr/bin/true. > > What fun. The autoconf manual has this gem under true, apropos to the > current patch: > > | when asked whether false is more portable than true Alexandre Oliva > answered: > | > | In a sense, yes, because if it doesn't exist, the shell will produce > an exit status of failure, which is correct for false, but not for true. > > http://www.gnu.org/software/autoconf/manual/autoconf.html#Limitations-of-Builtins > >> --- >> tests/commanddata/test16.log | 2 +- >> tests/commandtest.c | 6 +++--- >> 2 files changed, 4 insertions(+), 4 deletions(-) > > ACK. Thanks, pushed. -- Matthias Bolte http://photron.blogspot.com -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] tests: Unify style of test skipping code
2011/7/28 Eric Blake : > On 07/28/2011 09:52 AM, Matthias Bolte wrote: >> >> Prefer 'return EXIT_AM_SKIP' over 'exit(EXIT_AM_SKIP)'. >> >> Prefer 'int main(void)' over 'int main(int argc, char **argv)'. >> >> Fix mymain signature in commandtest and nodeinfotest. >> --- >> tests/commandtest.c | 10 +- >> tests/nodeinfotest.c | 10 +- >> tests/qemuargv2xmltest.c | 6 +- >> tests/qemuxml2xmltest.c | 6 +- >> tests/virnettlscontexttest.c | 6 -- >> tests/virshtest.c | 26 +- >> 6 files changed, 41 insertions(+), 23 deletions(-) > > ACK, all cosmetic, so no problem pushing prior to 0.9.4. > Thanks, pushed. -- Matthias Bolte http://photron.blogspot.com -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] utils: More useful error message for hook script failure
Commit 3709a386 ported hooks codes to new command execution API,
together with the useful error message removed. Though we can't
get "errbuf" from the new command execution API anymore, still
we can give a more useful error.
https://bugzilla.redhat.com/show_bug.cgi?id=726398
---
src/util/hooks.c |9 -
1 files changed, 8 insertions(+), 1 deletions(-)
diff --git a/src/util/hooks.c b/src/util/hooks.c
index 64adfcb..00f3a01 100644
--- a/src/util/hooks.c
+++ b/src/util/hooks.c
@@ -193,6 +193,7 @@ int
virHookCall(int driver, const char *id, int op, int sub_op, const char *extra,
const char *input) {
int ret;
+int exitstatus;
char *path;
virCommandPtr cmd;
const char *drvstr;
@@ -257,7 +258,13 @@ virHookCall(int driver, const char *id, int op, int
sub_op, const char *extra,
if (input)
virCommandSetInputBuffer(cmd, input);
-ret = virCommandRun(cmd, NULL);
+ret = virCommandRun(cmd, &exitstatus);
+if (exitstatus != 0) {
+virHookReportError(VIR_ERR_HOOK_SCRIPT_FAILED,
+ _("Hook script %s %s failed with error code %d"),
+ path, drvstr, exitstatus);
+ret = -1;
+}
virCommandFree(cmd);
--
1.7.6
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/2] Use virBufferQuoteString in virNetSocketNewConnectSSH
to quote the netcat command since it's passed to the shell. Adjust
expected test case output accordingly.
---
src/rpc/virnetsocket.c | 25 -
tests/virnetsockettest.c | 10 +-
2 files changed, 25 insertions(+), 10 deletions(-)
diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c
index cba58c6..5ba9d96 100644
--- a/src/rpc/virnetsocket.c
+++ b/src/rpc/virnetsocket.c
@@ -607,7 +607,10 @@ int virNetSocketNewConnectSSH(const char *nodename,
const char *path,
virNetSocketPtr *retsock)
{
+char *quoted;
virCommandPtr cmd;
+virBuffer buf = VIR_BUFFER_INITIALIZER;
+
*retsock = NULL;
cmd = virCommandNew(binary ? binary : "ssh");
@@ -630,6 +633,19 @@ int virNetSocketNewConnectSSH(const char *nodename,
virCommandAddArgList(cmd, "-o", "StrictHostKeyChecking=no", NULL);
virCommandAddArgList(cmd, nodename, "sh", "-c", NULL);
+
+virBufferQuoteString(&buf, netcat ? netcat : "nc");
+if (virBufferError(&buf)) {
+virBufferFreeAndReset(&buf);
+virReportOOMError();
+return -1;
+}
+quoted = virBufferContentAndReset(&buf);
+if (quoted == NULL) {
+virReportOOMError();
+return -1;
+}
+
/*
* This ugly thing is a shell script to detect availability of
* the -q option for 'nc': debian and suse based distros need this
@@ -641,14 +657,13 @@ int virNetSocketNewConnectSSH(const char *nodename,
* behavior.
*/
virCommandAddArgFormat(cmd,
- "'if %s -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then"
+ "'if '%s' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1;
then"
" ARG=-q0;"
"fi;"
- "%s $ARG -U %s'",
- netcat ? netcat : "nc",
- netcat ? netcat : "nc",
- path);
+ "'%s' $ARG -U %s'",
+ quoted, quoted, path);
+VIR_FREE(quoted);
return virNetSocketNewConnectCommand(cmd, retsock);
}
diff --git a/tests/virnetsockettest.c b/tests/virnetsockettest.c
index 3816b3c..00bee28 100644
--- a/tests/virnetsockettest.c
+++ b/tests/virnetsockettest.c
@@ -496,7 +496,7 @@ mymain(void)
struct testSSHData sshData1 = {
.nodename = "somehost",
.path = "/tmp/socket",
-.expectOut = "somehost sh -c 'if nc -q 2>&1 | grep \"requires an
argument\" >/dev/null 2>&1; then ARG=-q0;fi;nc $ARG -U /tmp/socket'\n",
+.expectOut = "somehost sh -c 'if ''nc'' -q 2>&1 | grep \"requires an
argument\" >/dev/null 2>&1; then ARG=-q0;fi;''nc'' $ARG -U /tmp/socket'\n",
};
if (virtTestRun("SSH test 1", 1, testSocketSSH, &sshData1) < 0)
ret = -1;
@@ -509,7 +509,7 @@ mymain(void)
.noTTY = true,
.noVerify = false,
.path = "/tmp/socket",
-.expectOut = "-p 9000 -l fred -T -o BatchMode=yes -e none somehost sh
-c 'if netcat -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then
ARG=-q0;fi;netcat $ARG -U /tmp/socket'\n",
+.expectOut = "-p 9000 -l fred -T -o BatchMode=yes -e none somehost sh
-c 'if ''netcat'' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then
ARG=-q0;fi;''netcat'' $ARG -U /tmp/socket'\n",
};
if (virtTestRun("SSH test 2", 1, testSocketSSH, &sshData2) < 0)
ret = -1;
@@ -522,7 +522,7 @@ mymain(void)
.noTTY = false,
.noVerify = true,
.path = "/tmp/socket",
-.expectOut = "-p 9000 -l fred -o StrictHostKeyChecking=no somehost sh
-c 'if netcat -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then
ARG=-q0;fi;netcat $ARG -U /tmp/socket'\n",
+.expectOut = "-p 9000 -l fred -o StrictHostKeyChecking=no somehost sh
-c 'if ''netcat'' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then
ARG=-q0;fi;''netcat'' $ARG -U /tmp/socket'\n",
};
if (virtTestRun("SSH test 3", 1, testSocketSSH, &sshData3) < 0)
ret = -1;
@@ -538,7 +538,7 @@ mymain(void)
struct testSSHData sshData5 = {
.nodename = "crashyhost",
.path = "/tmp/socket",
-.expectOut = "crashyhost sh -c 'if nc -q 2>&1 | grep \"requires an
argument\" >/dev/null 2>&1; then ARG=-q0;fi;nc $ARG -U /tmp/socket'\n",
+.expectOut = "crashyhost sh -c 'if ''nc'' -q 2>&1 | grep \"requires an
argument\" >/dev/null 2>&1; then ARG=-q0;fi;''nc'' $ARG -U /tmp/socket'\n",
.dieEarly = true,
};
@@ -550,7 +550,7 @@ mymain(void)
.path = "/tmp/socket",
.keyfile = "/root/.ssh/example_key",
.noVerify = true,
-.expectOut = "-i /root/.ssh/example_key -o StrictHostKeyChecking=no
example.com sh -c 'if nc -q 2>&1 | grep \"requires an argument\" >/dev/null
2>&1; then ARG=-q0;fi;nc $ARG -U /tmp/socket'\n",
+.expectOut = "-i /root/.ssh/example_key -o StrictHostKeyChecking=no
example.com sh -c 'if ''nc'' -q 2>&1 | grep \"requires an argument\" >/dev/null
2>&1;
[libvirt] [PATCH 1/2] Add virBufferQuoteString
Quote strings so they're safe to pass to the shell. Based on glib's
g_quote_string.
---
src/libvirt_private.syms |1 +
src/util/buf.c | 29 +
src/util/buf.h |1 +
3 files changed, 31 insertions(+), 0 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 853ee62..eb16fb6 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -28,6 +28,7 @@ virBufferError;
virBufferEscapeSexpr;
virBufferEscapeString;
virBufferFreeAndReset;
+virBufferQuoteString;
virBufferStrcat;
virBufferURIEncodeString;
virBufferUse;
diff --git a/src/util/buf.c b/src/util/buf.c
index 5002486..8106231 100644
--- a/src/util/buf.c
+++ b/src/util/buf.c
@@ -472,6 +472,35 @@ virBufferURIEncodeString (virBufferPtr buf, const char
*str)
}
/**
+ * virBufferQuoteString:
+ * @buf: the buffer to append to
+ * @str: an unquoted string
+ *
+ * Quotes a string so that the shell (/bin/sh) will interpret the
+ * quoted string as unquoted_string.
+ */
+void
+virBufferQuoteString(const virBufferPtr buf, const char *unquoted_str)
+{
+const char *p;
+
+virBufferAddChar(buf, '\'');
+p = unquoted_str;
+
+while (*p) {
+/* Replace literal ' with a close ', a \', and a open ' */
+if (*p == '\'')
+virBufferAddLit(buf, "'\\''");
+else
+virBufferAddChar(buf, *p);
+++p;
+}
+
+/* close the quote */
+virBufferAddChar(buf, '\'');
+}
+
+/**
* virBufferStrcat:
* @buf: the buffer to dump
* @...: the variable list of strings, the last argument must be NULL
diff --git a/src/util/buf.h b/src/util/buf.h
index 06d01ba..26a2f35 100644
--- a/src/util/buf.h
+++ b/src/util/buf.h
@@ -51,6 +51,7 @@ void virBufferStrcat(const virBufferPtr buf, ...)
void virBufferEscapeString(const virBufferPtr buf, const char *format, const
char *str);
void virBufferEscapeSexpr(const virBufferPtr buf, const char *format, const
char *str);
void virBufferURIEncodeString (const virBufferPtr buf, const char *str);
+void virBufferQuoteString(const virBufferPtr buf, const char *str);
# define virBufferAddLit(buf_, literal_string_) \
virBufferAdd (buf_, "" literal_string_ "", sizeof literal_string_ - 1)
--
1.7.5.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: fix crash when mixing sync and async monitor jobs
At 07/29/2011 07:47 AM, Eric Blake Write:
> Currently, we attempt to run sync job and async job at the same time. It
> means that the monitor commands for two jobs can be run in any order.
>
> In the function qemuDomainObjEnterMonitorInternal():
> if (priv->job.active == QEMU_JOB_NONE && priv->job.asyncJob) {
> if (qemuDomainObjBeginNestedJob(driver, obj) < 0)
> We check whether the caller is an async job by priv->job.active and
> priv->job.asynJob. But when an async job is running, and a sync job is
> also running at the time of the check, then priv->job.active is not
> QEMU_JOB_NONE. So we cannot check whether the caller is an async job
> in the function qemuDomainObjEnterMonitorInternal(), and must instead
> put the burden on the caller to tell us when an async command wants
> to do a nested job.
>
> * src/qemu/THREADS.txt: Reflect new rules.
> * src/qemu/qemu_domain.h (qemuDomainObjEnterMonitorAsync): New
> prototype.
> * src/qemu/qemu_process.h (qemuProcessStartCPUs)
> (qemuProcessStopCPUs): Add parameter.
> * src/qemu/qemu_migration.h (qemuMigrationToFile): Likewise.
> (qemuMigrationWaitForCompletion): Make static.
> * src/qemu/qemu_domain.c (qemuDomainObjEnterMonitorInternal): Add
> parameter.
> (qemuDomainObjEnterMonitorAsync): New function.
> (qemuDomainObjEnterMonitor, qemuDomainObjEnterMonitorWithDriver):
> Update callers.
> * src/qemu/qemu_driver.c (qemuDomainSaveInternal)
> (qemudDomainCoreDump, doCoreDump, processWatchdogEvent)
> (qemudDomainSuspend, qemudDomainResume, qemuDomainSaveImageStartVM)
> (qemuDomainSnapshotCreateActive, qemuDomainRevertToSnapshot):
> Likewise.
> * src/qemu/qemu_process.c (qemuProcessStopCPUs)
> (qemuProcessFakeReboot, qemuProcessRecoverMigration)
> (qemuProcessRecoverJob, qemuProcessStart): Likewise.
> * src/qemu/qemu_migration.c (qemuMigrationToFile)
> (qemuMigrationWaitForCompletion, qemuMigrationUpdateJobStatus)
> (qemuMigrationJobStart, qemuDomainMigrateGraphicsRelocate)
> (doNativeMigrate, doTunnelMigrate, qemuMigrationPerformJob)
> (qemuMigrationPerformPhase, qemuMigrationFinish)
> (qemuMigrationConfirm): Likewise.
> ---
>
> My initial smoke testing shows that this fixes 'virsh managedsave',
> but I still have more testing to do before I'm convinced I got
> everything (for example, I need to test migration still).
I test this patch with save by virt-manager, and find that it will cause
libvirt to be deadlock.
With this patch, we can ignore the return value of
qemuDomainObjEnterMonitor(WithDriver),
because these two functions always return 0. But we can not ignore the
return value of qemuDomainObjEnterMonitorAsync().
If qemuDomainObjEnterMonitorAsync() failed, we do nothing in this function.
So it's very dangerous to call qemuDomainObjExitMonitorWithDriver() when
qemuDomainObjEnterMonitorAsync() failed.
I think this problem already exists before this patch.
>
> src/qemu/THREADS.txt |8 --
> src/qemu/qemu_domain.c| 43 +++---
> src/qemu/qemu_domain.h|4 +++
> src/qemu/qemu_driver.c| 39 +--
> src/qemu/qemu_migration.c | 55
> src/qemu/qemu_migration.h |5 +--
> src/qemu/qemu_process.c | 32 ++
> src/qemu/qemu_process.h |7 -
> 8 files changed, 133 insertions(+), 60 deletions(-)
>
> diff --git a/src/qemu/THREADS.txt b/src/qemu/THREADS.txt
> index e73076c..125bd5d 100644
> --- a/src/qemu/THREADS.txt
> +++ b/src/qemu/THREADS.txt
> @@ -374,7 +374,7 @@ Design patterns
> qemuDriverUnlock(driver);
>
>
> - * Running asynchronous job
> + * Running asynchronous job with driver lock held
>
> virDomainObjPtr obj;
> qemuDomainObjPrivatePtr priv;
> @@ -387,7 +387,8 @@ Design patterns
>
> ...do prep work...
>
> - if (qemuDomainObjEnterMonitorWithDriver(driver, obj) < 0) {
> + if (qemuDomainObjEnterMonitorAsync(driver, obj,
> +QEMU_ASYNC_JOB_TYPE) < 0) {
> /* domain died in the meantime */
> goto error;
> }
> @@ -395,7 +396,8 @@ Design patterns
> qemuDomainObjExitMonitorWithDriver(driver, obj);
>
> while (!finished) {
> - if (qemuDomainObjEnterMonitorWithDriver(driver, obj) < 0) {
> + if (qemuDomainObjEnterMonitorAsync(driver, true, obj,
> +QEMU_ASYNC_JOB_TYPE) < 0) {
> /* domain died in the meantime */
> goto error;
> }
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 2eaaf3a..4cf6888 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -863,14 +863,20 @@ qemuDomainObjEndAsyncJob(struct qemud_driver *driver,
> virDomainObjPtr obj)
> return virDomainObjUnref(obj);
> }
>
> -static int ATTRIBUTE_NONNULL(1)
> +static int
> qemuDomainObjEnterMonitorInternal(struct qemud_driver *driver,
>b
[libvirt] [PATCH] openvz: detect when a domain was shut down from the inside
This patch adds an internal function openvzGetVEStatus to
get the real state of the domain. This function is used in
various places in the driver, in particular to detect when
the domain has been shut down by the user with the "halt"
command.
---
src/openvz/openvz_driver.c | 76 +++-
1 files changed, 68 insertions(+), 8 deletions(-)
diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c
index 4e7cb03..3b3b079 100644
--- a/src/openvz/openvz_driver.c
+++ b/src/openvz/openvz_driver.c
@@ -72,6 +72,7 @@ static int openvzDomainSetVcpusInternal(virDomainObjPtr vm,
unsigned int nvcpus);
static int openvzDomainSetMemoryInternal(virDomainObjPtr vm,
unsigned long memory);
+static int openvzGetVEStatus(virDomainObjPtr vm, int *status, int *reason);
static void openvzDriverLock(struct openvz_driver *driver)
{
@@ -340,6 +341,7 @@ static int openvzDomainGetInfo(virDomainPtr dom,
virDomainInfoPtr info) {
struct openvz_driver *driver = dom->conn->privateData;
virDomainObjPtr vm;
+int state;
int ret = -1;
openvzDriverLock(driver);
@@ -352,9 +354,11 @@ static int openvzDomainGetInfo(virDomainPtr dom,
goto cleanup;
}
-info->state = virDomainObjGetState(vm, NULL);
+if (openvzGetVEStatus(vm, &state, NULL) == -1)
+goto cleanup;
+info->state = state;
-if (!virDomainObjIsActive(vm)) {
+if (info->state != VIR_DOMAIN_RUNNING) {
info->cpuTime = 0;
} else {
if (openvzGetProcessInfo(&(info->cpuTime), dom->id) < 0) {
@@ -398,8 +402,7 @@ openvzDomainGetState(virDomainPtr dom,
goto cleanup;
}
-*state = virDomainObjGetState(vm, reason);
-ret = 0;
+ret = openvzGetVEStatus(vm, state, reason);
cleanup:
if (vm)
@@ -584,6 +587,7 @@ openvzDomainShutdownFlags(virDomainPtr dom,
virDomainObjPtr vm;
const char *prog[] = {VZCTL, "--quiet", "stop", PROGRAM_SENTINAL, NULL};
int ret = -1;
+int status;
virCheckFlags(0, -1);
@@ -596,9 +600,12 @@ openvzDomainShutdownFlags(virDomainPtr dom,
_("no domain with matching uuid"));
goto cleanup;
}
+
+if (openvzGetVEStatus(vm, &status, NULL) == -1)
+goto cleanup;
openvzSetProgramSentinal(prog, vm->def->name);
-if (virDomainObjGetState(vm, NULL) != VIR_DOMAIN_RUNNING) {
+if (status != VIR_DOMAIN_RUNNING) {
openvzError(VIR_ERR_INTERNAL_ERROR, "%s",
_("domain is not in running state"));
goto cleanup;
@@ -631,6 +638,7 @@ static int openvzDomainReboot(virDomainPtr dom,
virDomainObjPtr vm;
const char *prog[] = {VZCTL, "--quiet", "restart", PROGRAM_SENTINAL, NULL};
int ret = -1;
+int status;
virCheckFlags(0, -1);
@@ -644,8 +652,11 @@ static int openvzDomainReboot(virDomainPtr dom,
goto cleanup;
}
+if (openvzGetVEStatus(vm, &status, NULL) == -1)
+goto cleanup;
+
openvzSetProgramSentinal(prog, vm->def->name);
-if (virDomainObjGetState(vm, NULL) != VIR_DOMAIN_RUNNING) {
+if (status != VIR_DOMAIN_RUNNING) {
openvzError(VIR_ERR_INTERNAL_ERROR, "%s",
_("domain is not in running state"));
goto cleanup;
@@ -1052,6 +1063,7 @@ openvzDomainCreateWithFlags(virDomainPtr dom, unsigned
int flags)
virDomainObjPtr vm;
const char *prog[] = {VZCTL, "--quiet", "start", PROGRAM_SENTINAL, NULL };
int ret = -1;
+int status;
virCheckFlags(0, -1);
@@ -1064,8 +1076,11 @@ openvzDomainCreateWithFlags(virDomainPtr dom, unsigned
int flags)
_("no domain with matching id"));
goto cleanup;
}
+
+if (openvzGetVEStatus(vm, &status, NULL) == -1)
+goto cleanup;
-if (virDomainObjGetState(vm, NULL) != VIR_DOMAIN_SHUTOFF) {
+if (status != VIR_DOMAIN_SHUTOFF) {
openvzError(VIR_ERR_OPERATION_DENIED, "%s",
_("domain is not in shutoff state"));
goto cleanup;
@@ -1102,6 +1117,7 @@ openvzDomainUndefineFlags(virDomainPtr dom,
virDomainObjPtr vm;
const char *prog[] = { VZCTL, "--quiet", "destroy", PROGRAM_SENTINAL, NULL
};
int ret = -1;
+int status;
virCheckFlags(0, -1);
@@ -1113,7 +1129,10 @@ openvzDomainUndefineFlags(virDomainPtr dom,
goto cleanup;
}
-if (virDomainObjIsActive(vm)) {
+if (openvzGetVEStatus(vm, &status, NULL) == -1)
+goto cleanup;
+
+if (status != VIR_DOMAIN_SHUTOFF) {
openvzError(VIR_ERR_OPERATION_INVALID, "%s",
_("cannot delete active domain"));
goto cleanup;
@@ -1610,6 +1629,47 @@ cleanup:
return -1;
}
+static int
+openvzGetVEStatus(virDomainObjPtr vm, int *status, int *reason)
+{
+virCommandPtr cmd;
+char *outbuf;
+char *line;
+int state;
+int ret = -1
Re: [libvirt] [PATCH RFC] virsh: Add option to undefine storage with domains
On 07/28/2011 04:10 PM, Dave Allan wrote:
On Thu, Jul 28, 2011 at 03:41:01PM +0200, Peter Krempa wrote:
Adds an option to virsh undefine command to undefine managed
storage volumes along with (inactive) domains. Storage volumes
are enumerated and the user may interactivly choose volumes
to delete.
Unmanaged volumes are listed and the user shall delete them
manualy.
---
I marked this as a RFC because I am concerned about my "naming scheme" of the
added parameters.
I couldn't decide which of the following "volumes/storage/disks/..." to use.
I'd appreciate your
comments on this.
This is my second approach to this problem after I got some really good
critique from Eric,
Daniel and Dave. The user has the choice to activate an interactive mode, that
allows to select on a
per-device basis which volumes/disks to remove along with the domain.
To avoid possible problems, I only allowed to remove storage for inactive
domains and unmanaged
I think you mean managed images, right?
Yes, managed images.
images (which sidetracked me a lot on my previous attempt) are left to a action
of the user.
(the user is notified about any unmanaged image for the domain).
My next concern is about interactive of the user. I tried to implement a
boolean query function,
but I'd like to know if I took the right path, as I couldn't find any example
in virsh from which I
could learn.
We haven't previously implemented that much interactivity in virsh,
and I'm not sure I want to go down that road. virsh is generally a
pretty straight passthrough to the API. I'm sure others will have an
opinion on that question as well.
Well, yes, I found that out while looking for an interactive query
method which I didn't find.
Other option, that comes into my mind is to add a command to list
storage volumes for domains
and then let the user specify volumes to be removed as parameters to the
command "undefine".
Thanks for your comments (and time) :)
A few comments inline.
Dave
Peter Krempa
tools/virsh.c | 265 +++--
1 files changed, 259 insertions(+), 6 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c
index 61f69f0..3795d2b 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -295,6 +295,9 @@ static int vshCommandOptULongLong(const vshCmd *cmd, const
char *name,
static bool vshCommandOptBool(const vshCmd *cmd, const char *name);
static const vshCmdOpt *vshCommandOptArgv(const vshCmd *cmd,
const vshCmdOpt *opt);
+static int vshInteractiveBoolPrompt(vshControl *ctl,
+const char *prompt,
+ bool *confirm);
#define VSH_BYID (1<< 1)
#define VSH_BYUUID (1<< 2)
@@ -1422,6 +1425,8 @@ static const vshCmdInfo info_undefine[] = {
static const vshCmdOptDef opts_undefine[] = {
{"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name or uuid")},
{"managed-save", VSH_OT_BOOL, 0, N_("remove domain managed state file")},
+{"disks", VSH_OT_BOOL, 0, N_("remove associated disk images managed in storage
pools (interactive)")},
+{"disks-all", VSH_OT_BOOL, 0, N_("remove all associated disk images managed in
storage pools")},
I think it would be clearer stated as "remove all associated storage
volumes", and correspondingly, call the option "storage-volumes".
That definitely looks better. Thanks.
{NULL, 0, 0, NULL}
};
@@ -1434,9 +1439,25 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd)
int id;
int flags = 0;
int managed_save = vshCommandOptBool(cmd, "managed-save");
+int remove_disks = vshCommandOptBool(cmd, "disks");
+int remove_all_disks = vshCommandOptBool(cmd, "disks-all");
int has_managed_save = 0;
int rc = -1;
+char *domxml;
+xmlDocPtr xml = NULL;
+xmlXPathContextPtr ctxt = NULL;
+xmlXPathObjectPtr obj = NULL;
+xmlNodePtr cur = NULL;
+int i = 0;
+char *source = NULL;
+char *target = NULL;
+char *type = NULL;
+xmlBufferPtr xml_buf = NULL;
+virStorageVolPtr volume = NULL;
+int state;
+bool confirm = false;
+
if (managed_save)
flags |= VIR_DOMAIN_UNDEFINE_MANAGED_SAVE;
@@ -1475,15 +1496,172 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd)
}
}
-if (flags == -1) {
-if (has_managed_save == 1) {
+
+if (flags == -1&& has_managed_save == 1) {
+vshError(ctl,
+ _("Refusing to undefine while domain managed save "
+ "image exists"));
How does this interact with --managed-save? Can a user specify both
--managed-save and --disks to remove both managed save and storage volumes?
Definitely yes. It's kind of hard to see in the patch, because the contexts
got chosen very badly, but the user may specify both together and gets
expected results.
+virDomainFree(dom);
+return false;
+}
+
+if (remove_disks || remove_all_disk
