Re: [PATCH 13/13] ssh: add GUri-based URI parsing

["Daniel P. Berrangé" <berrange@redhat.com>]

On Thu, Jul 23, 2020 at 04:58:48PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Thu, Jul 23, 2020 at 4:33 PM Richard W.M. Jones <rjones@redhat.com>
> wrote:
> 
> > On Thu, Jul 09, 2020 at 11:42:34PM +0400, Marc-André Lureau wrote:
> > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > ---
> > >  block/ssh.c | 75 +++++++++++++++++++++++++++++++++++++++++------------
> > >  1 file changed, 58 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/block/ssh.c b/block/ssh.c
> > > index c8f6ad79e3c..d2bc6277613 100644
> > > --- a/block/ssh.c
> > > +++ b/block/ssh.c
> > > @@ -180,9 +180,37 @@ static void sftp_error_trace(BDRVSSHState *s, const
> > char *op)
> > >
> > >  static int parse_uri(const char *filename, QDict *options, Error **errp)
> > >  {
> > > +    g_autofree char *port_str = NULL;
> > > +    const char *scheme, *server, *path, *user, *key, *value;
> > > +    gint port;
> > > +
> > > +#ifdef HAVE_GLIB_GURI
> > > +    g_autoptr(GUri) uri = NULL;
> > > +    g_autoptr(GHashTable) params = NULL;
> > > +    g_autoptr(GError) err = NULL;
> > > +    GHashTableIter iter;
> > > +
> > > +    uri = g_uri_parse(filename, G_URI_FLAGS_ENCODED_QUERY, &err);
> > > +    if (!uri) {
> > > +        error_setg(errp, "Failed to parse SSH URI: %s", err->message);
> > > +        return -EINVAL;
> > > +    }
> > > +
> > > +    params = g_uri_parse_params(g_uri_get_query(uri), -1,
> > > +                                "&;", G_URI_PARAMS_NONE, &err);
> > > +    if (err) {
> > > +        error_report("Failed to parse SSH URI query: %s", err->message);
> > > +        return -EINVAL;
> > > +    }
> > > +
> > > +    scheme = g_uri_get_scheme(uri);
> > > +    user = g_uri_get_user(uri);
> > > +    server = g_uri_get_host(uri);
> > > +    path = g_uri_get_path(uri);
> > > +    port = g_uri_get_port(uri);
> > > +#else
> > >      g_autoptr(URI) uri = NULL;
> > >      g_autoptr(QueryParams) qp = NULL;
> > > -    g_autofree char *port_str = NULL;
> > >      int i;
> >
> > As Dan said already, this conditional code looks horrible and is going
> > to be a maintenance burden.  Was there a later version of this patch
> > series that resolved this?  I don't think I saw one.
> >
> 
> The patch is quite experimental. glib didn't even yet receive a release
> with GUri! But since I am working on the glib side, I wanted to make sure
> it covers qemu needs.
> 
> I will revisit the series once GUri is frozen & released (in
> mid-september),and use a copy version fallback.
> 
> Although, as I said in the cover, this is a bit riskier than having a
> transition period with both the old libxml-based parser and glib-based one
> for very recent distro.

The risk here is that the GUri has different semantics/behaviour to the
libxml one.

If that risk is large, then we shouldn't use GUri at all.

If the risk is low enough that we consider GUri a viable option, then
we should use it exclusively IMHO. This guarantees that all deployments
of QEMU will have identical behaviour, which will lower our support
burden in the event that bugs do appear, as we'll only have one codepath
to worry about.

The test suite you provided for GUri looks pretty comprehensive, and
QEMU's iotests are pretty decent too, so I think between the two we
have enough coverage that we should have confidence in using GUri
exclusively. If we hit obscure edge cases, we can just deal with it
as it arises.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|