6:42 p.m.
If a user invokes 'virsh -c $URI', then within that batch shell,
they probably want 'connect' to revert to $URI rather than the
normal default URI you get for passing in NULL.
In particular, I had a setup where qemu:///session was failing,
but took 20 seconds to fail; since 'make -C tests check TESTS=virsh-all'
exercises the command 'virsh -c test:///default connect' without
arguments, it was locking up for 20 seconds trying to connect to
qemu, even though the testsuite specifically wants to limit itself
to the test:///default URI.
* tools/virsh.c (__vshControl): Add member.
(main, vshParseArgv): Set it.
(vshDeinit): Clean it up.
(cmdConnect): Use it to reopen to original connection.
---
This doesn't fix the root cause of this problem:
https://www.redhat.com/archives/libvir-list/2012-June/msg00237.html
but at least it makes my 'make check' runs faster while
I still investigate the root problem.
tools/virsh.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c
index 0453b95..b28dc49 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -241,6 +241,7 @@ typedef struct __vshCmd {
*/
typedef struct __vshControl {
char *name; /* connection name */
+ char *default_name; /* -c or env-var default when name is NULL */
virConnectPtr conn; /* connection to hypervisor (MAY BE NULL) */
vshCmd *cmd; /* the current command */
char *cmdstr; /* string with command */
@@ -856,7 +857,7 @@ cmdConnect(vshControl *ctl, const vshCmd *cmd)
vshError(ctl, "%s", _("Please specify valid connection
URI"));
return false;
}
- ctl->name = vshStrdup(ctl, name);
+ ctl->name = vshStrdup(ctl, name ? name : ctl->default_name);
ctl->useGetInfo = false;
ctl->useSnapshotOld = false;
@@ -19922,6 +19923,7 @@ vshDeinit(vshControl *ctl)
vshReadlineDeinit(ctl);
vshCloseLogFile(ctl);
VIR_FREE(ctl->name);
+ VIR_FREE(ctl->default_name);
if (ctl->conn) {
int ret;
if ((ret = virConnectClose(ctl->conn)) != 0) {
@@ -20176,7 +20178,8 @@ vshParseArgv(vshControl *ctl, int argc, char **argv)
ctl->timing = true;
break;
case 'c':
- ctl->name = vshStrdup(ctl, optarg);
+ VIR_FREE(ctl->default_name);
+ ctl->default_name = vshStrdup(ctl, optarg);
break;
case 'v':
if (STRNEQ_NULLABLE(optarg, "long")) {
@@ -20211,6 +20214,7 @@ vshParseArgv(vshControl *ctl, int argc, char **argv)
exit(EXIT_FAILURE);
}
}
+ ctl->name = vshStrdup(ctl, ctl->default_name);
if (argc > optind) {
/* parse command */
@@ -20268,7 +20272,7 @@ main(int argc, char **argv)
progname++;
if ((defaultConn = getenv("VIRSH_DEFAULT_CONNECT_URI"))) {
- ctl->name = vshStrdup(ctl, defaultConn);
+ ctl->default_name = vshStrdup(ctl, defaultConn);
}
if (!vshParseArgv(ctl, argc, argv)) {
--
1.7.10.2
11:35 p.m.
On 2012年06月09日 07:42, Eric Blake wrote:
If a user invokes 'virsh -c $URI', then within that batch
shell,
they probably want 'connect' to revert to $URI rather than the
normal default URI you get for passing in NULL.
In particular, I had a setup where qemu:///session was failing,
but took 20 seconds to fail; since 'make -C tests check TESTS=virsh-all'
exercises the command 'virsh -c test:///default connect' without
arguments, it was locking up for 20 seconds trying to connect to
qemu, even though the testsuite specifically wants to limit itself
to the test:///default URI.
* tools/virsh.c (__vshControl): Add member.
(main, vshParseArgv): Set it.
(vshDeinit): Clean it up.
(cmdConnect): Use it to reopen to original connection.
---
This doesn't fix the root cause of this problem:
https://www.redhat.com/archives/libvir-list/2012-June/msg00237.html
but at least it makes my 'make check' runs faster while
I still investigate the root problem.
tools/virsh.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c
index 0453b95..b28dc49 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -241,6 +241,7 @@ typedef struct __vshCmd {
*/
typedef struct __vshControl {
char *name; /* connection name */
+ char *default_name; /* -c or env-var default when name is NULL */
virConnectPtr conn; /* connection to hypervisor (MAY BE NULL) */
vshCmd *cmd; /* the current command */
char *cmdstr; /* string with command */
@@ -856,7 +857,7 @@ cmdConnect(vshControl *ctl, const vshCmd *cmd)
vshError(ctl, "%s", _("Please specify valid connection
URI"));
return false;
}
- ctl->name = vshStrdup(ctl, name);
+ ctl->name = vshStrdup(ctl, name ? name : ctl->default_name);
ctl->useGetInfo = false;
ctl->useSnapshotOld = false;
@@ -19922,6 +19923,7 @@ vshDeinit(vshControl *ctl)
vshReadlineDeinit(ctl);
vshCloseLogFile(ctl);
VIR_FREE(ctl->name);
+ VIR_FREE(ctl->default_name);
if (ctl->conn) {
int ret;
if ((ret = virConnectClose(ctl->conn)) != 0) {
@@ -20176,7 +20178,8 @@ vshParseArgv(vshControl *ctl, int argc, char **argv)
ctl->timing = true;
break;
case 'c':
- ctl->name = vshStrdup(ctl, optarg);
+ VIR_FREE(ctl->default_name);
+ ctl->default_name = vshStrdup(ctl, optarg);
break;
case 'v':
if (STRNEQ_NULLABLE(optarg, "long")) {
@@ -20211,6 +20214,7 @@ vshParseArgv(vshControl *ctl, int argc, char **argv)
exit(EXIT_FAILURE);
}
}
+ ctl->name = vshStrdup(ctl, ctl->default_name);
if (argc> optind) {
/* parse command */
@@ -20268,7 +20272,7 @@ main(int argc, char **argv)
progname++;
if ((defaultConn = getenv("VIRSH_DEFAULT_CONNECT_URI"))) {
- ctl->name = vshStrdup(ctl, defaultConn);
+ ctl->default_name = vshStrdup(ctl, defaultConn);
}
if (!vshParseArgv(ctl, argc, argv)) {
Isn't the following patch enough for the fix? The problem is just
caused by ctl->name is freed even if no --name is specified for
command 'connect'. Or I missed something obviously?
diff --git a/tools/virsh.c b/tools/virsh.c
index abcfbff..0c8b44d 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -851,12 +851,15 @@ cmdConnect(vshControl *ctl, const vshCmd *cmd)
ctl->conn = NULL;
}
- VIR_FREE(ctl->name);
if (vshCommandOptString(cmd, "name", &name) < 0) {
vshError(ctl, "%s", _("Please specify valid connection
URI"));
return false;
}
- ctl->name = vshStrdup(ctl, name);
+
+ if (name) {
+ VIR_FREE(ctl->name);
+ ctl->name = vshStrdup(ctl, name);
+ }
ctl->useGetInfo = false;
ctl->useSnapshotOld = false;
Regards,
Osier
7:35 a.m.
On 06/10/2012 10:35 PM, Osier Yang wrote:
On 2012年06月09日 07:42, Eric Blake wrote:
> If a user invokes 'virsh -c $URI', then within that batch shell,
> they probably want 'connect' to revert to $URI rather than the
> normal default URI you get for passing in NULL.
> if ((defaultConn =
getenv("VIRSH_DEFAULT_CONNECT_URI"))) {
> - ctl->name = vshStrdup(ctl, defaultConn);
> + ctl->default_name = vshStrdup(ctl, defaultConn);
> }
>
> if (!vshParseArgv(ctl, argc, argv)) {
Isn't the following patch enough for the fix? The problem is just
caused by ctl->name is freed even if no --name is specified for
command 'connect'. Or I missed something obviously?
Your proposal also has merit, if for no other reason than that we don't
lose the current connection on failure to connect elsewhere, but I still
think we want a combination of both approaches. Consider:
virsh -c test:///default 'list; connect qemu:///session; list; connect'
Under my proposal, the second 'conect' would return us to
test:///default, since we used -c to establish that as the default
connection when 'connect' is used without args. Under your patch in
isolation, the second 'connect' would leave us stuck in qemu:///session,
and there is no way to return to our initial default connection except
to type it out in full.
diff --git a/tools/virsh.c b/tools/virsh.c
index abcfbff..0c8b44d 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -851,12 +851,15 @@ cmdConnect(vshControl *ctl, const vshCmd *cmd)
ctl->conn = NULL;
}
- VIR_FREE(ctl->name);
if (vshCommandOptString(cmd, "name", &name) < 0) {
vshError(ctl, "%s", _("Please specify valid connection
URI"));
return false;
}
- ctl->name = vshStrdup(ctl, name);
+
+ if (name) {
+ VIR_FREE(ctl->name);
+ ctl->name = vshStrdup(ctl, name);
+ }
But I definitely like this aspect of your patch:
virsh -c test:///default 'uri; connect foo://; uri'
as listing the same test:///default uri twice (that is, we don't lose
our current connection until the new connection is successful), compared
to the current code that sets name to NULL before attempting the foo://
connection, and therefore the second uri falls back to whatever
libvirt.so defaults for a NULL uri.
I'll submit a v2 that combines our two approaches.
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
8:01 a.m.
On Mon, Jun 11, 2012 at 06:35:10AM -0600, Eric Blake wrote:
On 06/10/2012 10:35 PM, Osier Yang wrote:
> On 2012年06月09日 07:42, Eric Blake wrote:
>> If a user invokes 'virsh -c $URI', then within that batch shell,
>> they probably want 'connect' to revert to $URI rather than the
>> normal default URI you get for passing in NULL.
>> if ((defaultConn = getenv("VIRSH_DEFAULT_CONNECT_URI"))) {
>> - ctl->name = vshStrdup(ctl, defaultConn);
>> + ctl->default_name = vshStrdup(ctl, defaultConn);
>> }
>>
>> if (!vshParseArgv(ctl, argc, argv)) {
>
> Isn't the following patch enough for the fix? The problem is just
> caused by ctl->name is freed even if no --name is specified for
> command 'connect'. Or I missed something obviously?
Your proposal also has merit, if for no other reason than that we don't
lose the current connection on failure to connect elsewhere, but I still
think we want a combination of both approaches. Consider:
virsh -c test:///default 'list; connect qemu:///session; list; connect'
Under my proposal, the second 'conect' would return us to
test:///default, since we used -c to establish that as the default
connection when 'connect' is used without args. Under your patch in
isolation, the second 'connect' would leave us stuck in qemu:///session,
and there is no way to return to our initial default connection except
to type it out in full.
Personally I think we should return a fatal error if the user
attempts to use 'connect' in non-interactive mode, and not try
to hack it to behave the same as --connect/-c.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
10:33 a.m.
On 06/11/2012 07:01 AM, Daniel P. Berrange wrote:
Personally I think we should return a fatal error if the user
attempts to use 'connect' in non-interactive mode, and not try
to hack it to behave the same as --connect/-c.
I still think that 'connect URI' is useful in non-interactive mode - I
can see batching up a series of commands that crosses several
connections, and therefore where using 'connect' in that batch to swap
connections makes sense.
But I think I can agree to the idea that if we are non-interactive (ie.
stdin is not a tty), then 'connect' without arguments always giving an
error instead of (re-)trying a NULL connection or even remembering the
-c option would make sense. As for the particular issue I was trying to
solve, having 'connect' without arguments error out would at least avoid
my issue of 'make check' taking forever when it gets to the 'virsh
connect' call.
And I _still_ think that we need to fix 'connect URI' to not lose the
current connection until after the new connection is established, as it
is better than the current behavior that kills the current connection
first even if the new connection is not possible.
I'll try to incorporate all of this into a v2.
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
6:06 p.m.
On 06/11/2012 09:33 AM, Eric Blake wrote:
On 06/11/2012 07:01 AM, Daniel P. Berrange wrote:
> Personally I think we should return a fatal error if the user
> attempts to use 'connect' in non-interactive mode, and not try
> to hack it to behave the same as --connect/-c.
I still think that 'connect URI' is useful in non-interactive mode - I
can see batching up a series of commands that crosses several
connections, and therefore where using 'connect' in that batch to swap
connections makes sense.
But I think I can agree to the idea that if we are non-interactive (ie.
stdin is not a tty), then 'connect' without arguments always giving an
error instead of (re-)trying a NULL connection or even remembering the
-c option would make sense. As for the particular issue I was trying to
solve, having 'connect' without arguments error out would at least avoid
my issue of 'make check' taking forever when it gets to the 'virsh
connect' call.
I'm worried that making 'virsh -c $URI connect' fail would break
existing scripts that probe whether a connection is viable. We don't
really have any other way to do a non-invasive probe of whether a
connection has sufficient access rights, although I suppose you can do
things like 'virsh -c $URI list' and ignore the output, for almost the
same effect.
Also, remember that commit d218344e changed things to avoid connecting
until we HAVE to use the connection, so things like 'virsh -c $URI echo'
are not a viable way to test for a valid connection, since 'echo'
doesn't need a connection to operate. That is, 'virsh -c' is documented
as behaving like 'connect URI', except that the connection is only
attempted if the rest of the command line has to use the connection.
And I _still_ think that we need to fix 'connect URI' to not lose the
current connection until after the new connection is established, as it
is better than the current behavior that kills the current connection
first even if the new connection is not possible.
I'll try to incorporate all of this into a v2.
Still not written, because I don't think we have settled on semantics.
What about this proposal:
Add a new option --default to 'virsh connect'. An explicit 'connect
--default' (whether interactive or specified in batch mode on the
command line) allows you to connect to the NULL URI (and thus we have a
way to rely on libvirt.so's default connection rules without having to
type in our desired default connection), no matter what your current
connection state is.
Obviously, 'connect URI' means to attempt to URI, but we fix things so
that if the new connection fails, virsh remains in the same connection
state that it was previously (whether or not it has already connected to
a previous domain).
Finally, 'connect' without either URI or --default changes meaning to
force connection to the current URI, rather than the old semantics of
opening a new connection to the NULL URI. Thus, 'virsh -c
test:///default connect' would now mean that we ensure that we are
connected to test:///default; 'virsh connect' is shorthand for 'virsh
connect --default'; 'virsh "connect $URI; list; connect --default;
list"' lets us toggle between two connections in one batch command, and
'virsh "connect $URI; list; connect"' treats the second connect as a
no-op because we are already in a connected state with $URI and
--default was not given.
Scripts that use connect to probe for valid ability to connect will
still continue to work. And the new option --default means that users
can still trigger a connection to a NULL URI, but the extra verbosity
means that 'connect' in isolation will no longer be quite so surprising.
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
3:13 a.m.
On 06/12/12 01:06, Eric Blake wrote:
On 06/11/2012 09:33 AM, Eric Blake wrote:
> On 06/11/2012 07:01 AM, Daniel P. Berrange wrote:
>
>> Personally I think we should return a fatal error if the user
>> attempts to use 'connect' in non-interactive mode, and not try
>> to hack it to behave the same as --connect/-c.
>
> I still think that 'connect URI' is useful in non-interactive mode - I
> can see batching up a series of commands that crosses several
> connections, and therefore where using 'connect' in that batch to swap
> connections makes sense.
>
> But I think I can agree to the idea that if we are non-interactive (ie.
> stdin is not a tty), then 'connect' without arguments always giving an
> error instead of (re-)trying a NULL connection or even remembering the
> -c option would make sense. As for the particular issue I was trying to
> solve, having 'connect' without arguments error out would at least avoid
> my issue of 'make check' taking forever when it gets to the 'virsh
> connect' call.
I'm worried that making 'virsh -c $URI connect' fail would break
existing scripts that probe whether a connection is viable. We don't
really have any other way to do a non-invasive probe of whether a
connection has sufficient access rights, although I suppose you can do
things like 'virsh -c $URI list' and ignore the output, for almost the
same effect.
Yes, it would break some scripts. Libvirt-guests init script is using
this mechanism since commit 05313770 and there may be lots of scripts
other people are using and are out of our control, so we definitely
should keep that piece of functionality.
Also, remember that commit d218344e changed things to avoid connecting
until we HAVE to use the connection, so things like 'virsh -c $URI echo'
are not a viable way to test for a valid connection, since 'echo'
doesn't need a connection to operate. That is, 'virsh -c' is documented
as behaving like 'connect URI', except that the connection is only
attempted if the rest of the command line has to use the connection.
>
> And I _still_ think that we need to fix 'connect URI' to not lose the
> current connection until after the new connection is established, as it
> is better than the current behavior that kills the current connection
> first even if the new connection is not possible.
On the other hand I am thinking about some use cases where keeping the
old connection would be unfortunate. For example if you paste a "script"
into virsh that might look like this:
connect qemu://production.server/system <- this succeeds
do_something_unimportant
connect qemu://testing.server/system <- this fails
destroy machine_that_has_same_name_as_some_important_production_machine
The last command succeeds on the previous connection and you caused a
disaster. It's a little bit overrated but in my opinion if you specify
"connect" then you don't want to use the old connection anymore.
>
> I'll try to incorporate all of this into a v2.
Still not written, because I don't think we have settled on semantics.
What about this proposal:
Add a new option --default to 'virsh connect'. An explicit 'connect
--default' (whether interactive or specified in batch mode on the
command line) allows you to connect to the NULL URI (and thus we have a
way to rely on libvirt.so's default connection rules without having to
type in our desired default connection), no matter what your current
connection state is.
Obviously, 'connect URI' means to attempt to URI, but we fix things so
that if the new connection fails, virsh remains in the same connection
state that it was previously (whether or not it has already connected to
a previous domain).
I don't agree with this approach. As stated above, I think we should
reject all commands until the user specifies a new valid connection to
state that he/she is aware what he/she is doing. To lessen the
inconvenience of having to re-write the previous URI we could add a new
flag eg. --previous to re-connect back to the last working connection.
Finally, 'connect' without either URI or --default changes meaning to
force connection to the current URI, rather than the old semantics of
opening a new connection to the NULL URI. Thus, 'virsh -c
test:///default connect' would now mean that we ensure that we are
connected to test:///default; 'virsh connect' is shorthand for 'virsh
connect --default'; 'virsh "connect $URI; list; connect --default;
list"' lets us toggle between two connections in one batch command, and
'virsh "connect $URI; list; connect"' treats the second connect as a
no-op because we are already in a connected state with $URI and
--default was not given.
This looks OK to me. Hopefuly nobody is using the old semantics to open
a connection to default URI in their scripts. But I honestly think that
nobody is using connect for anything else than checking if the URI is
working in their scripts.
Scripts that use connect to probe for valid ability to connect will
still continue to work. And the new option --default means that users
can still trigger a connection to a NULL URI, but the extra verbosity
means that 'connect' in isolation will no longer be quite so surprising.
Peter
4:18 a.m.
On Mon, Jun 11, 2012 at 05:06:37PM -0600, Eric Blake wrote:
On 06/11/2012 09:33 AM, Eric Blake wrote:
What about this proposal:
Add a new option --default to 'virsh connect'. An explicit 'connect
--default' (whether interactive or specified in batch mode on the
command line) allows you to connect to the NULL URI (and thus we have a
way to rely on libvirt.so's default connection rules without having to
type in our desired default connection), no matter what your current
connection state is.
Do we actually need that. We accept empty string as an alternative
to NULL, so can't you do virsh connect "" ?
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
4547
days inactive
4551
days old
7 comments
4 participants
participants (4)
-
Daniel P. Berrange -
Eric Blake -
Osier Yang -
Peter Krempa