On 06/04/16 10:51, Ján Tomko wrote:

Postpone filling out the default connection in ctl->connname after calling virshInit.

This allows printing help without a connection to the daemon. --- tools/virsh.c | 6 +++--- tools/virt-admin.c | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/virsh.c b/tools/virsh.c index 57b4ff3..8c616d6 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -968,9 +968,6 @@ main(int argc, char **argv)

virFileActivateDirOverride(argv[0]);

- if ((defaultConn = virGetEnvBlockSUID("VIRSH_DEFAULT_CONNECT_URI"))) - ctl->connname = vshStrdup(ctl, defaultConn); - if (!vshInit(ctl, cmdGroups, NULL)) exit(EXIT_FAILURE);

@@ -980,6 +977,9 @@ main(int argc, char **argv) exit(EXIT_FAILURE); }

+ if ((defaultConn = virGetEnvBlockSUID("VIRSH_DEFAULT_CONNECT_URI"))) + ctl->connname = vshStrdup(ctl, defaultConn); + if (!ctl->imode) { ret = vshCommandRun(ctl, ctl->cmd); } else {

^^This one's correct.

diff --git a/tools/virt-admin.c b/tools/virt-admin.c index edb8690..da847d2 100644 --- a/tools/virt-admin.c +++ b/tools/virt-admin.c @@ -719,9 +719,6 @@ main(int argc, char **argv)

virFileActivateDirOverride(argv[0]);

- if ((defaultConn = virGetEnvBlockSUID("LIBVIRT_DEFAULT_ADMIN_URI"))) - ctl->connname = vshStrdup(ctl, defaultConn); - if (!vshInit(ctl, cmdGroups, NULL)) exit(EXIT_FAILURE);

@@ -731,6 +728,9 @@ main(int argc, char **argv) exit(EXIT_FAILURE); }

+ if ((defaultConn = virGetEnvBlockSUID("LIBVIRT_DEFAULT_ADMIN_URI"))) + ctl->connname = vshStrdup(ctl, defaultConn); + if (!ctl->imode) { ret = vshCommandRun(ctl, ctl->cmd); } else {

^^But I don't think you need this one here, because unlike VIRSH_DEFAULT_CONNECT_URI which is deprecated by LIBVIRT_DEFAULT_URI and we can't drop it, LIBVIRT_DEFAULT_ADMIN_URI isn't and from my perspective the only way for this to not work would be a new virt-admin client and old libvirt-admin library which would not support NULL as connection name which is impossible since none of those were released yet, so in conclusion I don't think that getting the default conn in virt-admin is necessary at all and could be removed. ACK to the first part (for virsh) of the patch. Erik

On 06/04/16 10:51, Ján Tomko wrote:

vshStrdup returns NULL without exiting on NULL input. --- tools/virsh.c | 4 +--- tools/virt-admin.c | 4 +--- 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/tools/virsh.c b/tools/virsh.c index 8c616d6..fe33839 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -908,7 +908,6 @@ main(int argc, char **argv) { vshControl _ctl, *ctl = &_ctl; virshControl virshCtl; - const char *defaultConn; bool ret = true;

memset(ctl, 0, sizeof(vshControl)); @@ -977,8 +976,7 @@ main(int argc, char **argv) exit(EXIT_FAILURE); }

- if ((defaultConn = virGetEnvBlockSUID("VIRSH_DEFAULT_CONNECT_URI"))) - ctl->connname = vshStrdup(ctl, defaultConn); + ctl->connname = vshStrdup(ctl, virGetEnvBlockSUID("VIRSH_DEFAULT_CONNECT_URI"));

if (!ctl->imode) { ret = vshCommandRun(ctl, ctl->cmd); diff --git a/tools/virt-admin.c b/tools/virt-admin.c index da847d2..18048d1 100644 --- a/tools/virt-admin.c +++ b/tools/virt-admin.c @@ -670,7 +670,6 @@ main(int argc, char **argv) { vshControl _ctl, *ctl = &_ctl; vshAdmControl virtAdminCtl; - const char *defaultConn; bool ret = true;

memset(ctl, 0, sizeof(vshControl)); @@ -728,8 +727,7 @@ main(int argc, char **argv) exit(EXIT_FAILURE); }

- if ((defaultConn = virGetEnvBlockSUID("LIBVIRT_DEFAULT_ADMIN_URI"))) - ctl->connname = vshStrdup(ctl, defaultConn); + ctl->connname = vshStrdup(ctl, virGetEnvBlockSUID("LIBVIRT_DEFAULT_ADMIN_URI"));

if (!ctl->imode) { ret = vshCommandRun(ctl, ctl->cmd);

Like I said in 1/3, if we agree that getting the default connection URI can be done solely in libvirt-admin library, you don't need the second bit of the patch. ACK to the virsh part though. Erik

On 06/04/16 10:51, Ján Tomko wrote:

--- src/libvirt-admin.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/libvirt-admin.c b/src/libvirt-admin.c index 54af90c..6577c87 100644 --- a/src/libvirt-admin.c +++ b/src/libvirt-admin.c @@ -121,10 +121,10 @@ getSocketPath(virURIPtr uri) }

if (!sock_path) { - if (STRNEQ(uri->scheme, "libvirtd")) { + if (STRNEQ_NULLABLE(uri->scheme, "libvirtd")) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Unsupported URI scheme '%s'"), - uri->scheme); + NULLSTR(uri->scheme)); goto error; } if (STREQ_NULLABLE(uri->path, "/system")) {

ACK, we should also check the URI form if user provided socket path as well, I added that one on my TODO list. Erik