4:46 a.m.
On 06/12/13 21:34, Laine Stump wrote:
On 06/10/2013 08:44 AM, Peter Krempa wrote:
Can you include in the commit log a link to the BZ describing this
problem? It helps *immensely* when trying to trace things months/years
later.
There isn't any publicly available BZ describing this problem so I chose
not to put the private link in the commit. I can do it if you insist but
having private links in a public repo doesn't seem right to me.
> Without the socket path explicitly specified, the remote driver tried to
> connect to the "/system" instance socket even if "/session" was
> specified in the uri. With this patch this configuration now produces an
> error.
>
> It is still possible to initiate a session connection with specifying
> the path to the socket manually and also manually starting the session
> daemon. This was also possible prior to this patch,
>
> This is a minimal fix. We may decide to support remote session
> connections using ssh but this will require changes to the remote driver
> code so this fix shouldn't cause regressions in the case we decide to do
> that.
> ---
> src/remote/remote_driver.c | 40 ++++++++++++++++++++++++++++++----------
> 1 file changed, 30 insertions(+), 10 deletions(-)
>
> diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
> index fcf45d3..bd5646a 100644
> --- a/src/remote/remote_driver.c
> +++ b/src/remote/remote_driver.c
> @@ -631,11 +631,21 @@ doRemoteOpen(virConnectPtr conn,
> break;
>
> case trans_libssh2:
> - if (!sockname &&
> - VIR_STRDUP(sockname,
> - flags & VIR_DRV_OPEN_REMOTE_RO ?
> - LIBVIRTD_PRIV_UNIX_SOCKET_RO : LIBVIRTD_PRIV_UNIX_SOCKET)
< 0)
> - goto failed;
> + if (!sockname) {
> + /* Right now we don't support default session connections */
> + if (STREQ(conn->uri->path, "/session")) {
> + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> + _("Connecting to session instance without
"
> + "socket path is not supported by the libssh2
"
> + "connection driver"));
> + goto failed;
> + }
A totally naive question: do we want to only allow "/system"? or
'anything except "/session"'?
We want to forbid only "/session" with the default socket path which
won't work right now. A user is able to start a session daemon and
successfully connect to it even with this patch. The user has to
manually specify a socket path.
Rejecting everything except "/system" would break drivers that use
different path. A (stupid) example:
$ virsh -c test+ssh://root@localhost/default
Welcome to virsh, the virtualization interactive terminal.
Type: 'help' for help with commands
'quit' to quit
virsh # list --all
Id Name State
----------------------------------------------------
1 test running
> +
> + if (VIR_STRDUP(sockname,
> + flags & VIR_DRV_OPEN_REMOTE_RO ?
> + LIBVIRTD_PRIV_UNIX_SOCKET_RO : LIBVIRTD_PRIV_UNIX_SOCKET)
< 0)
> + goto failed;
> + }
>
> VIR_DEBUG("Starting LibSSH2 session");
>
> @@ -698,11 +708,21 @@ doRemoteOpen(virConnectPtr conn,
> if (!command && VIR_STRDUP(command, "ssh") < 0)
> goto failed;
>
> - if (!sockname &&
> - VIR_STRDUP(sockname,
> - flags & VIR_DRV_OPEN_REMOTE_RO ?
> - LIBVIRTD_PRIV_UNIX_SOCKET_RO : LIBVIRTD_PRIV_UNIX_SOCKET)
< 0)
> - goto failed;
> + if (!sockname) {
> + /* Right now we don't support default session connections */
> + if (STREQ(conn->uri->path, "/session")) {
> + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> + _("Connecting to session instance without
"
> + "socket path is not supported by the ssh
"
> + "connection driver"));
> + goto failed;
> + }
> +
> + if (VIR_STRDUP(sockname,
> + flags & VIR_DRV_OPEN_REMOTE_RO ?
> + LIBVIRTD_PRIV_UNIX_SOCKET_RO : LIBVIRTD_PRIV_UNIX_SOCKET)
< 0)
> + goto failed;
> + }
>
> if (!(priv->client = virNetClientNewSSH(priv->hostname,
> port,
ACK once the BZ link is included in the commit log (and pending my
question about whether we want to check for == "/session" or !=
"/system").
Peter