Eric Blake <ebl...@redhat.com> writes:
> On 03/23/2017 05:55 AM, Markus Armbruster wrote:
>> We have two list-values options:
>>
>> * "server" is a list of InetSocketAddress. We use members "host" and
>> "port", and silently ignore the rest.
>>
>> * "auth-supported" is a list of RbdAuthMethod. We use its only member
>> "auth".
>>
>> Since qemu_rbd_open() takes options as a flattened QDict, options has
>> keys of the form server.%d.host, server.%d.port and
>> auth-supported.%d.auth, where %d counts up from zero.
>>
>> qemu_rbd_array_opts() extracts these values as follows. First, it
>> calls qdict_array_entries() to find the list's length. For each list
>> element, it first formats the list's key prefix (e.g. "server.0."),
>> then creates a new QDict holding the options with that key prefix,
>> then converts that to a QemuOpts, so it can finally get the member
>> values from there.
>>
>> If there's one surefire way to make code using QDict more awkward,
>> it's creating more of them and mixing in QemuOpts for good measure.
>
> No kidding!
>
>>
>> The conversion to QemuOpts abuses runtime_opts, as described in the
>> commit before previous.
>>
>> Rewrite to simply get the values straight from the options QDict.
>> This removes the abuse of runtime_opts, so clean it up.
>>
>> Signed-off-by: Markus Armbruster <arm...@redhat.com>
>> ---
>> block/rbd.c | 151
>> +++++++++++++++++-------------------------------------------
>> 1 file changed, 42 insertions(+), 109 deletions(-)
>>
>> diff --git a/block/rbd.c b/block/rbd.c
>> index 59c822a..8ba0f79 100644
>> --- a/block/rbd.c
>> +++ b/block/rbd.c
>> @@ -13,6 +13,7 @@
>>
>> #include "qemu/osdep.h"
>>
>> +#include <rbd/librbd.h>
>> #include "qapi/error.h"
>> #include "qemu/error-report.h"
>> #include "block/block_int.h"
>> @@ -20,8 +21,6 @@
>> #include "qemu/cutils.h"
>> #include "qapi/qmp/qstring.h"
>>
>> -#include <rbd/librbd.h>
>> -
>
> Not mentioned in the commit message, but also a useful cleanup for
> hoisting <includes> prior to "includes".
>
>> +static char *rbd_auth(QDict *options)
>> {
>> - int num_entries;
>> - QemuOpts *opts = NULL;
>> - QDict *sub_options;
>> - const char *host;
>> - const char *port;
>> - char *str;
>> - char *rados_str = NULL;
>> - Error *local_err = NULL;
>> + const char **vals = g_new(const char *, qdict_size(options));
>> + char keybuf[32];
>> + QObject *val;
>> + char *rados_str;
>> int i;
>>
>> - assert(type == RBD_MON_HOST || type == RBD_AUTH_SUPPORTED);
>> -
>> - 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.
>> +static char *rbd_mon_host(QDict *options)
>> +{
>> + const char **vals = g_new(const char *, qdict_size(options));
>> + char keybuf[32];
>> + QObject *val;
>> + const char *host, *port;
>> + char *rados_str;
>> + int i;
>>
>> - 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...
>
>> - }
>> - value = strbuf;
>> - } else if (strchr(host, ':')) {
>> - strbuf = g_strdup_printf("[%s]", host);
>> - value = strbuf;
>> - }
>> - } else {
>> - value = qemu_opt_get(opts, "auth");
>> + for (i = 0;; i++) {
>> + sprintf(keybuf, "server.%d.host", i);
>
> Here, you've got more breathing room.
>
>> + val = qdict_get(options, keybuf);
>> + if (!val) {
>> + break;
>> }
>> + 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...
