[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v19 5/5] block/gluster: add support for multiple
|
From: |
Prasanna Kalever |
|
Subject: |
Re: [Qemu-devel] [PATCH v19 5/5] block/gluster: add support for multiple gluster servers |
|
Date: |
Tue, 19 Jul 2016 00:32:09 +0530 |
On Mon, Jul 18, 2016 at 8:09 PM, Markus Armbruster <address@hidden> wrote:
> Prasanna Kalever <address@hidden> writes:
>
>> On Mon, Jul 18, 2016 at 3:47 PM, Markus Armbruster <address@hidden> wrote:
>>> Prasanna Kumar Kalever <address@hidden> writes:
>>>
>>>> This patch adds a way to specify multiple volfile servers to the gluster
>>>> block backend of QEMU with tcp|rdma transport types and their port numbers.
>>>>
>>>> Problem:
>>>>
>>>> Currently VM Image on gluster volume is specified like this:
>>>>
>>>> file=gluster[+tcp]://host[:port]/testvol/a.img
>>>>
>>>> Say we have three hosts in a trusted pool with replica 3 volume in action.
>>>> When the host mentioned in the command above goes down for some reason,
>>>> the other two hosts are still available. But there's currently no way
>>>> to tell QEMU about them.
>>>>
>>>> Solution:
>>>>
>>>> New way of specifying VM Image on gluster volume with volfile servers:
>>>> (We still support old syntax to maintain backward compatibility)
>>>>
>>>> Basic command line syntax looks like:
>>>>
>>>> Pattern I:
>>>> -drive driver=gluster,
>>>> volume=testvol,path=/path/a.raw,[debug=N,]
>>>> server.0.type=tcp,
>>>> server.0.host=1.2.3.4,
>>>> [server.0.port=24007,]
>>>> server.1.type=unix,
>>>> server.1.socket=/path/socketfile
>>>>
>>>> Pattern II:
>>>> 'json:{"driver":"qcow2","file":{"driver":"gluster",
>>>> "volume":"testvol","path":"/path/a.qcow2",["debug":N,]
>>>> "server":[{hostinfo_1}, ...{hostinfo_N}]}}'
>>>>
>>>> driver => 'gluster' (protocol name)
>>>> volume => name of gluster volume where our VM image resides
>>>> path => absolute path of image in gluster volume
>>>> [debug] => libgfapi loglevel [(0 - 9) default 4 -> Error]
>>>>
>>>> {hostinfo} => {{type:"tcp",host:"1.2.3.4"[,port=24007]},
>>>> {type:"unix",socket:"/path/sockfile"}}
>>>>
>>>> type => transport type used to connect to gluster management
>>>> daemon,
>>>> it can be tcp|unix
>>>> host => host address (hostname/ipv4/ipv6 addresses/socket path)
>>>> [port] => port number on which glusterd is listening. (default
>>>> 24007)
>>>> socket => path to socket file
>>>>
>>>> Examples:
>>>> 1.
>>>> -drive driver=qcow2,file.driver=gluster,
>>>> file.volume=testvol,file.path=/path/a.qcow2,file.debug=9,
>>>> file.server.0.type=tcp,
>>>> file.server.0.host=1.2.3.4,
>>>> file.server.0.port=24007,
>>>> file.server.1.type=tcp,
>>>> file.server.1.socket=/var/run/glusterd.socket
>>>> 2.
>>>> 'json:{"driver":"qcow2","file":{"driver":"gluster","volume":"testvol",
>>>> "path":"/path/a.qcow2","debug":9,"server":
>>>> [{type:"tcp",host:"1.2.3.4",port=24007},
>>>> {type:"unix",socket:"/var/run/glusterd.socket"}] } }'
>>>>
>>>> This patch gives a mechanism to provide all the server addresses, which
>>>> are in
>>>> replica set, so in case host1 is down VM can still boot from any of the
>>>> active hosts.
>>>>
>>>> This is equivalent to the backup-volfile-servers option supported by
>>>> mount.glusterfs (FUSE way of mounting gluster volume)
>>>>
>>>> credits: sincere thanks to all the supporters
>>>>
>>>> Signed-off-by: Prasanna Kumar Kalever <address@hidden>
>>>> ---
>>>> block/gluster.c | 347
>>>> +++++++++++++++++++++++++++++++++++++++++++++------
>>>> qapi/block-core.json | 2 +-
>>>> 2 files changed, 307 insertions(+), 42 deletions(-)
>>>>
>>>> diff --git a/block/gluster.c b/block/gluster.c
>>>> index ff1e783..fd2279d 100644
>>>> --- a/block/gluster.c
>>>> +++ b/block/gluster.c
>>>> @@ -12,8 +12,16 @@
>>>> #include "block/block_int.h"
>>>> #include "qapi/error.h"
>>>> #include "qemu/uri.h"
>>>> +#include "qemu/error-report.h"
>>>>
>>>> #define GLUSTER_OPT_FILENAME "filename"
>>>> +#define GLUSTER_OPT_VOLUME "volume"
>>>> +#define GLUSTER_OPT_PATH "path"
>>>> +#define GLUSTER_OPT_TYPE "type"
>>>> +#define GLUSTER_OPT_SERVER_PATTERN "server."
>>>> +#define GLUSTER_OPT_HOST "host"
>>>> +#define GLUSTER_OPT_PORT "port"
>>>> +#define GLUSTER_OPT_SOCKET "socket"
>>>> #define GLUSTER_OPT_DEBUG "debug"
>>>> #define GLUSTER_DEFAULT_PORT 24007
>>>> #define GLUSTER_DEBUG_DEFAULT 4
>>>> @@ -82,6 +90,77 @@ static QemuOptsList runtime_opts = {
>>>> },
>>>> };
>>>>
>>>> +static QemuOptsList runtime_json_opts = {
>>>> + .name = "gluster_json",
>>>> + .head = QTAILQ_HEAD_INITIALIZER(runtime_json_opts.head),
>>>> + .desc = {
>>>> + {
>>>> + .name = GLUSTER_OPT_VOLUME,
>>>> + .type = QEMU_OPT_STRING,
>>>> + .help = "name of gluster volume where VM image resides",
>>>> + },
>>>> + {
>>>> + .name = GLUSTER_OPT_PATH,
>>>> + .type = QEMU_OPT_STRING,
>>>> + .help = "absolute path to image file in gluster volume",
>>>> + },
>>>> + {
>>>> + .name = GLUSTER_OPT_DEBUG,
>>>> + .type = QEMU_OPT_NUMBER,
>>>> + .help = "Gluster log level, valid range is 0-9",
>>>> + },
>>>> + { /* end of list */ }
>>>> + },
>>>> +};
>>>> +
>>>> +static QemuOptsList runtime_type_opts = {
>>>> + .name = "gluster_type",
>>>> + .head = QTAILQ_HEAD_INITIALIZER(runtime_type_opts.head),
>>>> + .desc = {
>>>> + {
>>>> + .name = GLUSTER_OPT_TYPE,
>>>> + .type = QEMU_OPT_STRING,
>>>> + .help = "tcp|unix",
>>>> + },
>>>> + { /* end of list */ }
>>>> + },
>>>> +};
>>>> +
>>>> +static QemuOptsList runtime_unix_opts = {
>>>> + .name = "gluster_unix",
>>>> + .head = QTAILQ_HEAD_INITIALIZER(runtime_unix_opts.head),
>>>> + .desc = {
>>>> + {
>>>> + .name = GLUSTER_OPT_SOCKET,
>>>> + .type = QEMU_OPT_STRING,
>>>> + .help = "socket file path)",
>>>> + },
>>>> + { /* end of list */ }
>>>> + },
>>>> +};
>>>> +
>>>> +static QemuOptsList runtime_tcp_opts = {
>>>> + .name = "gluster_tcp",
>>>> + .head = QTAILQ_HEAD_INITIALIZER(runtime_tcp_opts.head),
>>>> + .desc = {
>>>> + {
>>>> + .name = GLUSTER_OPT_TYPE,
>>>> + .type = QEMU_OPT_STRING,
>>>> + .help = "tcp|unix",
>>>> + },
>>>> + {
>>>> + .name = GLUSTER_OPT_HOST,
>>>> + .type = QEMU_OPT_STRING,
>>>> + .help = "host address (hostname/ipv4/ipv6 addresses)",
>>>> + },
>>>> + {
>>>> + .name = GLUSTER_OPT_PORT,
>>>> + .type = QEMU_OPT_NUMBER,
>>>> + .help = "port number on which glusterd is listening (default
>>>> 24007)",
>>>> + },
>>>> + { /* end of list */ }
>>>> + },
>>>> +};
>>>>
>>>> static int parse_volume_options(BlockdevOptionsGluster *gconf, char *path)
>>>> {
>>>> @@ -157,7 +236,8 @@ static int
>>>> qemu_gluster_parse_uri(BlockdevOptionsGluster *gconf,
>>>> return -EINVAL;
>>>> }
>>>>
>>>> - gconf->server = gsconf = g_new0(GlusterServer, 1);
>>>> + gconf->server = g_new0(GlusterServerList, 1);
>>>> + gconf->server->value = gsconf = g_new0(GlusterServer, 1);
>>>>
>>>> /* transport */
>>>> if (!uri->scheme || !strcmp(uri->scheme, "gluster")) {
>>>> @@ -211,38 +291,34 @@ out:
>>>> return ret;
>>>> }
>>>>
>>>> -static struct glfs *qemu_gluster_init(BlockdevOptionsGluster *gconf,
>>>> - const char *filename, Error **errp)
>>>> +static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
>>>> + Error **errp)
>>>> {
>>>> - struct glfs *glfs = NULL;
>>>> + struct glfs *glfs;
>>>> int ret;
>>>> int old_errno;
>>>> -
>>>> - ret = qemu_gluster_parse_uri(gconf, filename);
>>>> - if (ret < 0) {
>>>> - error_setg(errp, "Usage:
>>>> file=gluster[+transport]://[host[:port]]/"
>>>> - "volume/path[?socket=...]");
>>>> - errno = -ret;
>>>> - goto out;
>>>> - }
>>>> + GlusterServerList *server;
>>>>
>>>> glfs = glfs_new(gconf->volume);
>>>> if (!glfs) {
>>>> goto out;
>>>> }
>>>>
>>>> - if (gconf->server->type == GLUSTER_TRANSPORT_UNIX) {
>>>> - ret = glfs_set_volfile_server(glfs,
>>>> -
>>>> GlusterTransport_lookup[gconf->server->type],
>>>> - gconf->server->u.q_unix.socket, 0);
>>>> - } else {
>>>> - ret = glfs_set_volfile_server(glfs,
>>>> -
>>>> GlusterTransport_lookup[gconf->server->type],
>>>> - gconf->server->u.tcp.host,
>>>> - gconf->server->u.tcp.port);
>>>> - }
>>>> - if (ret < 0) {
>>>> - goto out;
>>>> + for (server = gconf->server; server; server = server->next) {
>>>> + if (server->value->type == GLUSTER_TRANSPORT_UNIX) {
>>>> + ret = glfs_set_volfile_server(glfs,
>>>> +
>>>> GlusterTransport_lookup[server->value->type],
>>>> + server->value->u.q_unix.socket,
>>>> 0);
>>>> + } else {
>>>> + ret = glfs_set_volfile_server(glfs,
>>>> +
>>>> GlusterTransport_lookup[server->value->type],
>>>> + server->value->u.tcp.host,
>>>> + server->value->u.tcp.port);
>>>
>>> server->value.u.tcp.port is optional. Using it without checking
>>> server->value.u.tcp.has_port relies on the default value being zero. We
>>> don't actually document that. Perhaps we should.
>>
>> I have made sure to fill it in the code, also marked
>> gsconf->u.tcp.has_port = true;
>>
>>>
>>>> + }
>>>> +
>>>> + if (ret < 0) {
>>>> + goto out;
>>>> + }
>>>> }
>>>>
>>>> ret = glfs_set_logging(glfs, "-", gconf->debug_level);
>>>> @@ -252,19 +328,19 @@ static struct glfs
>>>> *qemu_gluster_init(BlockdevOptionsGluster *gconf,
>>>>
>>>> ret = glfs_init(glfs);
>>>> if (ret) {
>>>> - if (gconf->server->type == GLUSTER_TRANSPORT_UNIX) {
>>>> - error_setg(errp,
>>>> - "Gluster connection for volume: %s, path: %s "
>>>> - "failed to connect on socket %s ",
>>>> - gconf->volume, gconf->path,
>>>> - gconf->server->u.q_unix.socket);
>>>> - } else {
>>>> - error_setg(errp,
>>>> - "Gluster connection for volume: %s, path: %s "
>>>> - "failed to connect host %s and port %d ",
>>>> - gconf->volume, gconf->path,
>>>> - gconf->server->u.tcp.host,
>>>> gconf->server->u.tcp.port);
>>>> + error_setg(errp, "Gluster connection for volume: %s, path: %s ",
>>>> + gconf->volume, gconf->path);
>>>> + for (server = gconf->server; server; server = server->next) {
>>>> + if (server->value->type == GLUSTER_TRANSPORT_UNIX) {
>>>> + error_append_hint(errp, "failed to connect on socket %s ",
>>>> + server->value->u.q_unix.socket);
>>>> + } else {
>>>> + error_append_hint(errp, "failed to connect host %s and
>>>> port %d ",
>>>> + server->value->u.tcp.host,
>>>> + server->value->u.tcp.port);
>>>> + }
>>>> }
>>>> + error_append_hint(errp, "Please refer to gluster logs for more
>>>> info ");
>>>
>>> Your code produces the error message "Gluster connection for volume:
>>> VOLUME, path: PATH ", which makes no sense.
>>>
>>> It also produces a hint that is a concatenation of one or more "failed
>>> to connect on FOO", followed by "Please refer to ..." without any
>>> punctuation, but with a trailing space.
>>>
>>> The error message must make sense on its own, without the hint.
>>>
>>> A fixed error message could look like this:
>>>
>>> Gluster connection for volume VOLUME, path PATH failed to connect on
>>> FOO, on BAR, and on BAZ
>>>
>>> or with a little less effort
>>>
>>> Gluster connection for volume VOLUME, path PATH failed to connect on
>>> FOO, BAR, BAZ
>>>
>>> or simply
>>>
>>> Can't connect to Gluster volume VOLUME, path PATH at FOO, BAR, BAZ
>>>
>>> You can't build up the error message with error_append_hint(). Using it
>>> to append a hint pointing to Gluster logs is fine.
>>
>> okay.
>>
>>>
>>>>
>>>> /* glfs_init sometimes doesn't set errno although docs suggest
>>>> that */
>>>> if (errno == 0) {
>>>> @@ -284,6 +360,195 @@ out:
>>>> return NULL;
>>>> }
>>>>
>>>> +static int qapi_enum_parse(const char *opt)
>>>> +{
>>>> + int i;
>>>> +
>>>> + if (!opt) {
>>>> + return GLUSTER_TRANSPORT__MAX;
>>>> + }
>>>> +
>>>> + for (i = 0; i < GLUSTER_TRANSPORT__MAX; i++) {
>>>> + if (!strcmp(opt, GlusterTransport_lookup[i])) {
>>>> + return i;
>>>> + }
>>>> + }
>>>> +
>>>> + return i;
>>>> +}
>>>> +
>>>> +/*
>>>> + * Convert the json formatted command line into qapi.
>>>> +*/
>>>> +static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
>>>> + QDict *options, Error **errp)
>>>> +{
>>>> + QemuOpts *opts;
>>>> + GlusterServer *gsconf;
>>>> + GlusterServerList *curr = NULL;
>>>> + QDict *backing_options = NULL;
>>>> + Error *local_err = NULL;
>>>> + char *str = NULL;
>>>> + const char *ptr;
>>>> + size_t num_servers;
>>>> + int i;
>>>> +
>>>> + /* create opts info from runtime_json_opts list */
>>>> + opts = qemu_opts_create(&runtime_json_opts, NULL, 0, &error_abort);
>>>> + qemu_opts_absorb_qdict(opts, options, &local_err);
>>>> + if (local_err) {
>>>> + goto out;
>>>> + }
>>>> +
>>>> + num_servers = qdict_array_entries(options,
>>>> GLUSTER_OPT_SERVER_PATTERN);
>>>> + if (num_servers < 1) {
>>>> + error_setg(&local_err, "please provide 'server' option with valid
>>>> "
>>>> + "fields in array of hostinfo ");
>>>
>>> This isn't an error message, it's instructions what to do. Such
>>> instructions can be useful when they're correct, but they can't replace
>>> an error message. The error message should state what's wrong. No
>>> less, no more.
>>
>> Let me know, how to log the hint from a leaf function, where Error
>> object is not created yet.
>> I also feel its more like an error in the usage of the json command
>
> The error we diagnose here is that @options either lacks a 'server' key,
> or its value is invalid. Since I'm too lazy to come up with an error
> message for that, I grep for this kind of error (value of
> qdict_array_entries() negative), and find one in quorum.c:
>
> s->num_children = qdict_array_entries(options, "children.");
> if (s->num_children < 0) {
> error_setg(&local_err, "Option children is not a valid array");
> ret = -EINVAL;
> goto exit;
> }
>
>>> Moreover, avoid prefixes like "qemu_gluster:". Usually, the fact that
>>> this is about Gluster is obvious. When it isn't, providing context is
>>> the caller's job.
>>
>> I think I have removed almost all 'qemu_gluster:' in v19 by respecting
>> you comments in v18
>
> Pasto, sorry.
>
>>>> + goto out;
>>>> + }
>>>> +
>>>> + ptr = qemu_opt_get(opts, GLUSTER_OPT_VOLUME);
>>>> + if (!ptr) {
>>>> + error_setg(&local_err, "please provide 'volume' option ");
>>>
>>> Not an error message.
>>
>> Also same here ...
>
> The usual error message for a missing mandatory key 'volume' is
> "Parameter 'volume' is missing". For historical reasons, that's
> commonly written as
>
> error_setg(errp, QERR_MISSING_PARAMETER, "volume");
>
> If I didn't know that already, I'd try to find out the same way as for
> the previous one: grep for this kind of error (value of qemu_opt_get()
> null), stick to the most common way to report it.
>
> Feel free to use a message that's more tailored to this particular
> error. My point here isn't that you should reuse existing error
> messages (that's a complete non-requirement), only that an error message
> should first and foremost tell the user what's wrong. Telling him what
> he might have to do fix it is secondary. It might be helpful, but in
> practice it's often misleading, because users often screw up in more
> ways than you anticipated when writing the error message.
>
>>>> + goto out;
>>>> + }
>>>> + gconf->volume = g_strdup(ptr);
>>>> +
>>>> + ptr = qemu_opt_get(opts, GLUSTER_OPT_PATH);
>>>> + if (!ptr) {
>>>> + error_setg(&local_err, "please provide 'path' option ");
>>>
>>> Not an error message.
>>
>> here ...
>>
>>>
>>>> + goto out;
>>>> + }
>>>> + gconf->path = g_strdup(ptr);
>>>> + qemu_opts_del(opts);
>>>> +
>>>> + for (i = 0; i < num_servers; i++) {
>>>> + str = g_strdup_printf(GLUSTER_OPT_SERVER_PATTERN"%d.", i);
>>>> + qdict_extract_subqdict(options, &backing_options, str);
>>>> +
>>>> + /* create opts info from runtime_type_opts list */
>>>> + opts = qemu_opts_create(&runtime_type_opts, NULL, 0,
>>>> &error_abort);
>>>> + qemu_opts_absorb_qdict(opts, backing_options, &local_err);
>>>> + if (local_err) {
>>>> + goto out;
>>>> + }
>>>> +
>>>> + ptr = qemu_opt_get(opts, GLUSTER_OPT_TYPE);
>>>> + gsconf = g_new0(GlusterServer, 1);
>>>> + gsconf->type = qapi_enum_parse(ptr);
>>>> + if (!ptr) {
>>>> + error_setg(&local_err, "please provide 'type' in hostinfo.%d
>>>> as "
>>>> + "tcp|unix ", i);
>>>
>>> Not an error message.
>>
>> and here ...
>> How do I say whats really wrong in the command, which could be long
>> (if provides N servers in the list)
>
> The usual error message for a key 'type' having a value other than 'tcp'
> or 'unix' would be "Parameter 'type' expects 'tcp' or 'unix'". For
> historical reasons, that's commonly written as
>
> error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "type", "tcp or unix");
>
> If I didn't know that already, I'd again grep.
>
> Once again, feel free to improve on this message.
>
>>>> + goto out;
>>>> +
>>>> + }
>>>> + if (gsconf->type == GLUSTER_TRANSPORT__MAX) {
>>>> + error_setg(&local_err, "'type' in hostinfo.%d can be tcp|unix
>>>> ", i);
>>>> + goto out;
>>>> + }
>>>> + qemu_opts_del(opts);
>>>> +
>>>> + if (gsconf->type == GLUSTER_TRANSPORT_TCP) {
>>>> + /* create opts info from runtime_tcp_opts list */
>>>> + opts = qemu_opts_create(&runtime_tcp_opts, NULL, 0,
>>>> &error_abort);
>>>> + qemu_opts_absorb_qdict(opts, backing_options, &local_err);
>>>> + if (local_err) {
>>>> + goto out;
>>>> + }
>>>> +
>>>> + ptr = qemu_opt_get(opts, GLUSTER_OPT_HOST);
>>>> + if (!ptr) {
>>>> + error_setg(&local_err, "please provide 'host' in
>>>> hostinfo.%d ",
>>>> + i);
>>>> + goto out;
>>>> + }
>>>> + gsconf->u.tcp.host = g_strdup(ptr);
>>>> + ptr = qemu_opt_get(opts, GLUSTER_OPT_PORT);
>>>> + gsconf->u.tcp.port = qemu_opt_get_number(opts,
>>>> GLUSTER_OPT_PORT,
>>>> + GLUSTER_DEFAULT_PORT);
>>>> + gsconf->u.tcp.has_port = true;
>>>> + qemu_opts_del(opts);
>>>> + } else {
>>>> + /* create opts info from runtime_unix_opts list */
>>>> + opts = qemu_opts_create(&runtime_unix_opts, NULL, 0,
>>>> &error_abort);
>>>> + qemu_opts_absorb_qdict(opts, backing_options, &local_err);
>>>> + if (local_err) {
>>>> + goto out;
>>>> + }
>>>> +
>>>> + ptr = qemu_opt_get(opts, GLUSTER_OPT_SOCKET);
>>>> + if (!ptr) {
>>>> + error_setg(&local_err, "please provide 'socket' in
>>>> hostinfo.%d ",
>>>> + i);
>>>> + goto out;
>>>> + }
>>>> + gsconf->u.q_unix.socket = g_strdup(ptr);
>>>> + qemu_opts_del(opts);
>>>> + }
>>>> +
>>>> + if (gconf->server == NULL) {
>>>> + gconf->server = g_new0(GlusterServerList, 1);
>>>> + gconf->server->value = gsconf;
>>>> + curr = gconf->server;
>>>> + } else {
>>>> + curr->next = g_new0(GlusterServerList, 1);
>>>> + curr->next->value = gsconf;
>>>> + curr = curr->next;
>>>> + }
>>>> +
>>>> + qdict_del(backing_options, str);
>>>> + g_free(str);
>>>> + str = NULL;
>>>> + }
>>>> +
>>>> + return 0;
>>>> +
>>>> +out:
>>>> + if (local_err) {
>>>> + error_propagate(errp, local_err);
>>>> + }
>>>
>>> error_propagate() does the right thing when its second argument is
>>> null. Please drop the conditional.
>>
>> Sure
>>
>>>
>>>> + qemu_opts_del(opts);
>>>> + if (str) {
>>>> + qdict_del(backing_options, str);
>>>> + g_free(str);
>>>> + }
>>>> + errno = EINVAL;
>>>> + return -errno;
>>>> +}
>>>> +
>>>> +static struct glfs *qemu_gluster_init(BlockdevOptionsGluster *gconf,
>>>> + const char *filename,
>>>> + QDict *options, Error **errp)
>>>> +{
>>>> + int ret;
>>>> + if (filename) {
>>>> + ret = qemu_gluster_parse_uri(gconf, filename);
>>>> + if (ret < 0) {
>>>> + error_setg(errp, "Usage:
>>>> file=gluster[+transport]://[host[:port]]/"
>>>> + "volume/path[?socket=...]");
>>>
>>> Not an error message.
>>
>> Can you suggest the change required here for displaying hints from a
>> leaf function, I am worried for missing your intention here, since you
>> have most of your v18 comments here.
>>
>> IIRC, the leaf function which wants to display out a hint/msg/error
>> should have an Error object, which is created by error_setg() and
>> error_append_hind() appends to it.
>
> The best you can do here is a generic error message like "invalid URI".
> To be more specific, you have to create the error in
> qemu_gluster_parse_uri(), because only there you know what exactly is
> wrong. Requires giving it an Error **errp parameter. I'm *not* asking
> you for that. It's a separate improvement.
>
> "invalid URI" isn't as horrible as you might think, because the actual
> error message should come out like this:
>
> qemu-system-x86_64: -drive file=gluster+tcp://junk: invalid URI
>
> which I find clearer than
>
> qemu-system-x86_64: -drive file=gluster+tcp://junk: Usage:
> file=gluster[+transport]://[server[:port]]/volname/image[?socket=...]
>
> I reiterate my plea to *test* error messages.
>
> We generally don't print usage information hints after errors. If we
> did, then the code would look like this:
>
> error_setg(errp, "invalid URI");
> error_append_hint(errp, "Usage: file=gluster[+transport]:"
> "//[host[:port]]/volume/path[?socket=...]\n");
>
> Should look like this:
>
> qemu-system-x86_64: -drive file=gluster+tcp://junk: invalid URI
> Usage: file=gluster[+transport]://[host[:port]]/volume/path[?socket=...]
>9
Make sense to me; shall split them to have 'generic msg' (first) +
'what went wrong' (hint next, if required)
first part with error_setg() + error_append_hint() for hints
> By the way, not sure what [?socket...] means.
>
[?socket] is the url query argument, which is unix domain socket file
path, on which glusterd is listening for management connection.
example:
qemu-system-x86_64
gluster+unix:///volume/fedora23.qcow2?socket=/var/run/glusterd.socket
In case, if the volfile resides on local machine, it fetches it form
there without involving the tcp loopback;
--
Prasanna
> Sorry for disappointing you Markus.
>
> We'll get there :)
- Re: [Qemu-devel] [PATCH v19 3/5] block/gluster: remove rdma transport, (continued)
[Qemu-devel] [PATCH v19 5/5] block/gluster: add support for multiple gluster servers, Prasanna Kumar Kalever, 2016/07/15