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@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list