Re: [Qemu-devel] [PATCH for-2.9 3/5] rbd: Rewrite the code to extract list-valued options

[Eric Blake <eblake@redhat.com>]

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

On 03/23/2017 01:27 PM, Markus Armbruster wrote:

>>> -
>>> -    num_entries = qdict_array_entries(options, prefix);
>>> +    for (i = 0;; i++) {
>>> +        sprintf(keybuf, "auth-supported.%d.auth", i);
>>
>> By my count, and including a trailing NUL, this is 21 bytes + the
>> maximum size of a formatted int to fit in keybuf[32]; 32-bit INT_MIN is
>> indeed 11 bytes.  Cutting it close there, but I don't see an overflow
>> (if gcc 7's new -Wformat-truncation spots something, then gcc is too
>> strict.)
> 
> 11 decimal digits take a hell of a list :)
> 
> Could double the buffer if it makes anyone sleep better.

I'm okay with its current size.


>>> -            value = host;
>>> -            if (port) {
>>> -                /* check for ipv6 */
>>> -                if (strchr(host, ':')) {
>>> -                    strbuf = g_strdup_printf("[%s]:%s", host, port);
>>> -                } else {
>>> -                    strbuf = g_strdup_printf("%s:%s", host, port);
>>
>> The old code only prints port information if it is present...
>>

>>> +        host = qstring_get_str(qobject_to_qstring(val));
>>> +        sprintf(keybuf, "server.%d.port", i);
>>> +        port = qdict_get_str(options, keybuf);
>>>  
>>> -
>>> -        /* each iteration in the for loop will build upon the string, and if
>>> -         * rados_str is NULL then it is our first pass */
>>> -        if (rados_str) {
>>> -            /* separate options with ';', as that  is what rados_conf_set()
>>> -             * requires */
>>> -            rados_str_tmp = rados_str;
>>> -            rados_str = g_strdup_printf("%s;%s", rados_str_tmp, value);
>>> -            g_free(rados_str_tmp);
>>> +        if (strchr(host, ':')) {
>>> +            vals[i] = g_strdup_printf("[%s]:%s", host, port);
>>>          } else {
>>> -            rados_str = g_strdup(value);
>>> +            vals[i] = g_strdup_printf("%s:%s", host, port);
>>
>> ...but the new code unconditionally prints port information, even when
>> port == NULL.  Oops.
> 
> How can port be null?  SocketAddress member port is mandatory...

Indeed. Does that mean the old code had a dead branch? Looks like it!

So, if I'm not overlooking something, that means you've resolved all my
open questions, and can submit the patch as written with:

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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