[Qemu-devel] [PATCH 0/2] block/ssh: Implement .bdrv_refresh_filename()

[Qemu-devel] [PATCH 0/2] block/ssh: Implement .bdrv_refresh_filename()

[Max Reitz]

This series implements .bdrv_refresh_filename() for the ssh block
driver, along with an appropriate .bdrv_dirname() so we don't chop off
query strings for backing files with relative filenames.

This series depends on my “block: Fix some filename generation issues”
series and on Pino's “ssh: switch from libssh2 to libssh” patch.

Based-on: 20180205151835.20812-1-mreitz@redhat.com
Based-on: 20180118164439.2120-1-ptoscano@redhat.com


Max Reitz (2):
  block/ssh: Implement .bdrv_refresh_filename()
  block/ssh: Implement .bdrv_dirname()

 block/ssh.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 65 insertions(+), 7 deletions(-)

-- 
2.14.3

[Qemu-devel] [PATCH 1/2] block/ssh: Implement .bdrv_refresh_filename()

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/ssh.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 46 insertions(+), 7 deletions(-)

diff --git a/block/ssh.c b/block/ssh.c
index c31b2686c3..a5b7e831a4 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -75,6 +75,12 @@ typedef struct BDRVSSHState {
 
     /* Used to warn if 'flush' is not supported. */
     bool unsafe_flush_warning;
+
+    /* Store the user name for ssh_refresh_filename() because the
+     * default depends on the system you are on -- therefore, when we
+     * generate a filename, it should always contain the user name we
+     * are actually using */
+    char *user;
 } BDRVSSHState;
 
 static void ssh_state_init(BDRVSSHState *s)
@@ -86,6 +92,8 @@ static void ssh_state_init(BDRVSSHState *s)
 
 static void ssh_state_free(BDRVSSHState *s)
 {
+    g_free(s->user);
+
     if (s->attrs) {
         sftp_attributes_free(s->attrs);
     }
@@ -569,7 +577,7 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options,
     int r, ret;
     QemuOpts *opts = NULL;
     Error *local_err = NULL;
-    const char *user, *path, *host_key_check;
+    const char *path, *host_key_check;
     long port = 0;
     unsigned long portU = 0;
     int new_sock = -1;
@@ -594,10 +602,10 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options,
         goto err;
     }
 
-    user = qemu_opt_get(opts, "user");
-    if (!user) {
-        user = g_get_user_name();
-        if (!user) {
+    s->user = g_strdup(qemu_opt_get(opts, "user"));
+    if (!s->user) {
+        s->user = g_strdup(g_get_user_name());
+        if (!s->user) {
             error_setg_errno(errp, errno, "Can't get user name");
             ret = -errno;
             goto err;
@@ -642,7 +650,7 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options,
      */
     ssh_set_blocking(s->session, 1);
 
-    r = ssh_options_set(s->session, SSH_OPTIONS_USER, user);
+    r = ssh_options_set(s->session, SSH_OPTIONS_USER, s->user);
     if (r < 0) {
         ret = -EINVAL;
         session_error_setg(errp, s,
@@ -704,7 +712,7 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options,
     }
 
     /* Authenticate. */
-    ret = authenticate(s, user, errp);
+    ret = authenticate(s, s->user, errp);
     if (ret < 0) {
         goto err;
     }
@@ -1156,6 +1164,36 @@ static int64_t ssh_getlength(BlockDriverState *bs)
     return length;
 }
 
+static void ssh_refresh_filename(BlockDriverState *bs)
+{
+    BDRVSSHState *s = bs->opaque;
+    const char *path, *host_key_check;
+    int ret;
+
+    /* None of these options can be represented in a plain "host:port"
+     * format, so if any was given, we have to abort */
+    if (s->inet->has_ipv4 || s->inet->has_ipv6 || s->inet->has_to ||
+        s->inet->has_numeric)
+    {
+        return;
+    }
+
+    path = qdict_get_try_str(bs->full_open_options, "path");
+    assert(path); /* mandatory option */
+
+    host_key_check = qdict_get_try_str(bs->full_open_options, "host_key_check");
+
+    ret = snprintf(bs->exact_filename, sizeof(bs->exact_filename),
+                   "ssh://%s@%s:%s%s%s%s",
+                   s->user, s->inet->host, s->inet->port, path,
+                   host_key_check ? "?host_key_check=" : "",
+                   host_key_check ?: "");
+    if (ret >= sizeof(bs->exact_filename)) {
+        /* An overflow makes the filename unusable, so do not report any */
+        bs->exact_filename[0] = '\0';
+    }
+}
+
 static const char *const ssh_sgfnt_runtime_opts[] = {
     "host",
     "port",
@@ -1180,6 +1218,7 @@ static BlockDriver bdrv_ssh = {
     .bdrv_co_writev               = ssh_co_writev,
     .bdrv_getlength               = ssh_getlength,
     .bdrv_co_flush_to_disk        = ssh_co_flush,
+    .bdrv_refresh_filename        = ssh_refresh_filename,
     .create_opts                  = &ssh_create_opts,
     .sgfnt_runtime_opts           = ssh_sgfnt_runtime_opts,
 };
-- 
2.14.3

[Qemu-devel] [PATCH 2/2] block/ssh: Implement .bdrv_dirname()

[Max Reitz]

ssh_bdrv_dirname() is basically the generic bdrv_dirname(), except it
takes care not to silently chop off any query string (i.e.,
host_key_check).

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/ssh.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/block/ssh.c b/block/ssh.c
index a5b7e831a4..7dbe55cec1 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -1194,6 +1194,24 @@ static void ssh_refresh_filename(BlockDriverState *bs)
     }
 }
 
+static char *ssh_bdrv_dirname(BlockDriverState *bs, Error **errp)
+{
+    if (qdict_haskey(bs->full_open_options, "host_key_check")) {
+        /* We cannot generate a simple prefix if we would have to
+         * append a query string */
+        error_setg(errp,
+                   "Cannot generate a base directory with host_key_check set");
+        return NULL;
+    }
+
+    if (bs->exact_filename[0] == '\0') {
+        error_setg(errp, "Cannot generate a base directory for this ssh node");
+        return NULL;
+    }
+
+    return path_combine(bs->exact_filename, "");
+}
+
 static const char *const ssh_sgfnt_runtime_opts[] = {
     "host",
     "port",
@@ -1219,6 +1237,7 @@ static BlockDriver bdrv_ssh = {
     .bdrv_getlength               = ssh_getlength,
     .bdrv_co_flush_to_disk        = ssh_co_flush,
     .bdrv_refresh_filename        = ssh_refresh_filename,
+    .bdrv_dirname                 = ssh_bdrv_dirname,
     .create_opts                  = &ssh_create_opts,
     .sgfnt_runtime_opts           = ssh_sgfnt_runtime_opts,
 };
-- 
2.14.3

Re: [Qemu-devel] [PATCH 0/2] block/ssh: Implement .bdrv_refresh_filename()

[Richard W.M. Jones]

On Mon, Feb 05, 2018 at 09:22:30PM +0100, Max Reitz wrote:
> This series implements .bdrv_refresh_filename() for the ssh block
> driver, along with an appropriate .bdrv_dirname() so we don't chop off
> query strings for backing files with relative filenames.
> 
> This series depends on my “block: Fix some filename generation issues”
> series and on Pino's “ssh: switch from libssh2 to libssh” patch.
> 
> Based-on: 20180205151835.20812-1-mreitz@redhat.com
> Based-on: 20180118164439.2120-1-ptoscano@redhat.com
> 
> 
> Max Reitz (2):
>   block/ssh: Implement .bdrv_refresh_filename()
>   block/ssh: Implement .bdrv_dirname()
> 
>  block/ssh.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++++++------

I reviewed the patches and I can't see anything wrong with them.

I guess that on "weird" remote machines getting the directory from a
filename is an impossible mission, but for reasonable hosts it's going
to be fine.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

Re: [Qemu-devel] [PATCH 0/2] block/ssh: Implement .bdrv_refresh_filename()

[Richard W.M. Jones]

I assume the context of all this is making relative backing file names work
in qcow2 files?

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

Re: [Qemu-devel] [PATCH 0/2] block/ssh: Implement .bdrv_refresh_filename()

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 357 bytes --]

On 2018-02-05 21:46, Richard W.M. Jones wrote:
> 
> I assume the context of all this is making relative backing file names work
> in qcow2 files?

Yes, mostly.  (https://bugzilla.redhat.com/show_bug.cgi?id=1528931)
But it's generally nice for block nodes to print a readable filename
when queried and not a json:{} monster all of the time.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH 0/2] block/ssh: Implement .bdrv_refresh_filename()

[John Snow]

On 02/05/2018 03:22 PM, Max Reitz wrote:
> This series implements .bdrv_refresh_filename() for the ssh block
> driver, along with an appropriate .bdrv_dirname() so we don't chop off
> query strings for backing files with relative filenames.
> 
> This series depends on my “block: Fix some filename generation issues”
> series and on Pino's “ssh: switch from libssh2 to libssh” patch.
> 
> Based-on: 20180205151835.20812-1-mreitz@redhat.com
> Based-on: 20180118164439.2120-1-ptoscano@redhat.com
> 
> 
> Max Reitz (2):
>   block/ssh: Implement .bdrv_refresh_filename()
>   block/ssh: Implement .bdrv_dirname()
> 
>  block/ssh.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 65 insertions(+), 7 deletions(-)
> 

Did this one rot on the vine?

>1 month old.

Re: [Qemu-devel] [Qemu-block] [PATCH 0/2] block/ssh: Implement .bdrv_refresh_filename()

[Kevin Wolf]

Am 06.03.2018 um 22:51 hat John Snow geschrieben:
> On 02/05/2018 03:22 PM, Max Reitz wrote:
> > This series implements .bdrv_refresh_filename() for the ssh block
> > driver, along with an appropriate .bdrv_dirname() so we don't chop off
> > query strings for backing files with relative filenames.
> > 
> > This series depends on my “block: Fix some filename generation issues”
> > series and on Pino's “ssh: switch from libssh2 to libssh” patch.
> > 
> > Based-on: 20180205151835.20812-1-mreitz@redhat.com
> > Based-on: 20180118164439.2120-1-ptoscano@redhat.com
> > 
> > 
> > Max Reitz (2):
> >   block/ssh: Implement .bdrv_refresh_filename()
> >   block/ssh: Implement .bdrv_dirname()
> > 
> >  block/ssh.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++++++------
> >  1 file changed, 65 insertions(+), 7 deletions(-)
> 
> Did this one rot on the vine?
> 
> >1 month old.

The Based-on tags are the problem, in particular the first one. But yes,
we could possibly do more to review the dependencies...

Kevin

Re: [Qemu-devel] [Qemu-block] [PATCH 0/2] block/ssh: Implement .bdrv_refresh_filename()

[John Snow]

On 03/07/2018 05:04 AM, Kevin Wolf wrote:
> Am 06.03.2018 um 22:51 hat John Snow geschrieben:
>> On 02/05/2018 03:22 PM, Max Reitz wrote:
>>> This series implements .bdrv_refresh_filename() for the ssh block
>>> driver, along with an appropriate .bdrv_dirname() so we don't chop off
>>> query strings for backing files with relative filenames.
>>>
>>> This series depends on my “block: Fix some filename generation issues”
>>> series and on Pino's “ssh: switch from libssh2 to libssh” patch.
>>>
>>> Based-on: 20180205151835.20812-1-mreitz@redhat.com
>>> Based-on: 20180118164439.2120-1-ptoscano@redhat.com
>>>
>>>
>>> Max Reitz (2):
>>>   block/ssh: Implement .bdrv_refresh_filename()
>>>   block/ssh: Implement .bdrv_dirname()
>>>
>>>  block/ssh.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++++++------
>>>  1 file changed, 65 insertions(+), 7 deletions(-)
>>
>> Did this one rot on the vine?
>>
>>> 1 month old.
> 
> The Based-on tags are the problem, in particular the first one. But yes,
> we could possibly do more to review the dependencies...
> 
> Kevin
> 

Yeah, sorry, I'm not pulling my review weight right now. I do try to
keep my block-devel inbox a rough "todo," though, so for patches that
get to that one month mark with no conclusion I like to ping them before
I archive them.

It's something I'd like to see patchew do, actually:

"Here's a list of what's on the list that has no reviews or NACKs, and
needs some love"

coupled with a 30 day "Hey, nobody looked at this" ping to the list
before it NACKs a set for being too old.

I hope nobody reads these >1 month pings as me wondering why nobody ELSE
has reviewed it, which is not my intent...

--js

Re: [Qemu-devel] [Qemu-block] [PATCH 0/2] block/ssh: Implement .bdrv_refresh_filename()

[Fam Zheng]

On Wed, 03/07 12:50, John Snow wrote:
> It's something I'd like to see patchew do, actually:
> 
> "Here's a list of what's on the list that has no reviews or NACKs, and
> needs some love"

It's not hard to define a search condition for that:

http://patchew.org/search-help

http://patchew.org/search?q=project%3AQEMU+age%3A%3E1m+not%3Areviewed+not%3Areplied+not%3Amerged+is%3Atested+to%3Aqemu-block

> 
> coupled with a 30 day "Hey, nobody looked at this" ping to the list
> before it NACKs a set for being too old.

If the initial landing of the patch didn't get enough attention, chances are the
pings will not change much about it especially it's from a bot.

A summary list sounds good, though.

Fam

Re: [Qemu-devel] [Qemu-block] [PATCH 0/2] block/ssh: Implement .bdrv_refresh_filename()

[John Snow]

On 03/07/2018 09:12 PM, Fam Zheng wrote:
> On Wed, 03/07 12:50, John Snow wrote:
>> It's something I'd like to see patchew do, actually:
>>
>> "Here's a list of what's on the list that has no reviews or NACKs, and
>> needs some love"
> 
> It's not hard to define a search condition for that:
> 
> http://patchew.org/search-help
> 
> http://patchew.org/search?q=project%3AQEMU+age%3A%3E1m+not%3Areviewed+not%3Areplied+not%3Amerged+is%3Atested+to%3Aqemu-block
> 
>>
>> coupled with a 30 day "Hey, nobody looked at this" ping to the list
>> before it NACKs a set for being too old.
> 
> If the initial landing of the patch didn't get enough attention, chances are the
> pings will not change much about it especially it's from a bot.
> 
> A summary list sounds good, though.
> 
> Fam
> 

It's not really to *get* the patch attention as much as it's meant to
inform the sender that it's not under consideration anymore.

"Yeah, but everyone knows this...?"

Consider it a formalization of the process.

--js