Re: [PATCH v2 12/13] block/ssh: Use URI parsing code from glib

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On Fri, Apr 12, 2024 at 03:24:14PM +0200, Thomas Huth wrote:
>> Since version 2.66, glib has useful URI parsing functions, too.
>> Use those instead of the QEMU-internal ones to be finally able
>> to get rid of the latter.
>> 
>> Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  block/ssh.c | 75 ++++++++++++++++++++++++++++-------------------------
>>  1 file changed, 40 insertions(+), 35 deletions(-)
>> 
>
>>  
>> -    if (g_strcmp0(uri->scheme, "ssh") != 0) {
>> +    if (g_strcmp0(g_uri_get_scheme(uri), "ssh") != 0) {
>
> Yet another case-sensitive spot to consider.
>
>>  
>> -    qdict_put_str(options, "path", uri->path);
>> -
>> -    /* Pick out any query parameters that we understand, and ignore
>> -     * the rest.
>> -     */
>> -    for (i = 0; i < qp->n; ++i) {
>> -        if (strcmp(qp->p[i].name, "host_key_check") == 0) {
>> -            qdict_put_str(options, "host_key_check", qp->p[i].value);
>> +    qdict_put_str(options, "path", uri_path);
>> +
>> +    uri_query = g_uri_get_query(uri);
>> +    if (uri_query) {
>> +        g_uri_params_iter_init(&qp, uri_query, -1, "&", G_URI_PARAMS_NONE);
>> +        while (g_uri_params_iter_next(&qp, &qp_name, &qp_value, &gerror)) {
>> +            if (!qp_name || !qp_value || gerror) {
>> +                warn_report("Failed to parse SSH URI parameters '%s'.",
>> +                            uri_query);
>> +                break;
>> +            }
>> +            /*
>> +             * Pick out the query parameters that we understand, and ignore
>> +             * (or rather warn about) the rest.
>> +             */
>> +            if (g_str_equal(qp_name, "host_key_check")) {
>> +                qdict_put_str(options, "host_key_check", qp_value);
>> +            } else {
>> +                warn_report("Unsupported parameter '%s' in URI.", qp_name);
>
> Do we want the trailing '.' in warn_report?

We do not.

> The warning is new; it was not in the old code, nor mentioned in the
> commit message.  It seems like a good idea, but we should be more
> intentional if we intend to make that change.