[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/2] nbd: change option parsing scheme
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-devel] [PATCH 1/2] nbd: change option parsing scheme |
Date: |
Wed, 5 Oct 2016 11:57:47 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0 |
I reviewed this patch before noticing that the overall idea is not what
Kevin suggested, so I'm sending it out anyway. Further comments from
Kevin and Max might come since I am not familiar with the current
conventions on parsing block device options.
On 05/10/2016 11:33, Denis V. Lunev wrote:
> From: Denis Plotnikov <address@hidden>
>
> This is a preparatory commit to make code more generic. We are going to
> add more options in the next patch.
>
> Signed-off-by: Denis Plotnikov <address@hidden>
> Signed-off-by: Denis V. Lunev <address@hidden>
> CC: Paolo Bonzini <address@hidden>
> CC: Kevin Wolf <address@hidden>
> CC: Max Reitz <address@hidden>
> ---
> block/nbd.c | 143
> ++++++++++++++++++++++++++++++++++++++++++++++++++++--------
> 1 file changed, 124 insertions(+), 19 deletions(-)
>
> diff --git a/block/nbd.c b/block/nbd.c
> index 6bc06d6..3b133ed 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -38,7 +38,9 @@
> #include "qapi/qmp/qstring.h"
> #include "qemu/cutils.h"
>
> -#define EN_OPTSTR ":exportname="
> +#define EN_OPTSTR "exportname"
Please replace the #define with the "exportname" string; same for
ZI_OPTSTR in patch 2.
> +#define PATH_PARAM (1u << 0)
>
> typedef struct BDRVNBDState {
> NbdClientSession client;
> @@ -47,6 +49,46 @@ typedef struct BDRVNBDState {
> char *path, *host, *port, *export, *tlscredsid;
> } BDRVNBDState;
>
> +/*
> + * helpers for dealing with option parsing
> + * to ease futher params adding and managing
> + */
> +
> +/*
> + * @param_flags - bit flags defining a set of param names to be parsed
> + */
> +static bool parse_query_params(QueryParams *qp, QDict *options,
> + unsigned int param_flags)
> +{
> + int i;
> + for (i = 0; i < qp->n; i++) {
> + QueryParam *param = &qp->p[i];
> +
> + if ((PATH_PARAM & param_flags) &&
> + strcmp(param->name, "socket") == 0) {
> + qdict_put(options, "path", qstring_from_str(param->value));
> + continue;
> + }
> +
> + }
> + return true;
> +}
Please remove the param_flags argument. In patch 2 you do:
if (find_prohibited_params(qp, PATH_PARAM)) {
ret = -EINVAL;
goto out;
}
if (!parse_query_params(qp, options, ZERO_INIT_PARAM)) {
ret = -EINVAL;
goto out;
}
Because you've filtered out the socket parameter, it's okay if
parse_query_params parses it always.
Also, please change nbd_parse_uri to return errors via Error**. Then
parse_query_params can do the same, and we get better error messages.
> +
> +static bool find_prohibited_params(QueryParams *qp, unsigned int param_flags)
> +{
> + int i;
> + for (i = 0; i < qp->n; i++) {
> + QueryParam *param = &qp->p[i];
> +
> + if ((PATH_PARAM & param_flags) &&
> + strcmp(param->name, "socket") == 0) {
> + return true;
> + }
> + }
> + return false;
> +}
Please change this function to something like
static QueryParam *find_query_param(QueryParams *qp, const char *name)
> + if (!parse_query_params(qp, options, PATH_PARAM)) {
> ret = -EINVAL;
> goto out;
> }
> - qdict_put(options, "path", qstring_from_str(qp->p[0].value));
> } else {
> QString *host;
> /* nbd[+tcp]://host[:port]/export */
> @@ -113,6 +155,11 @@ static int nbd_parse_uri(const char *filename, QDict
> *options)
> qdict_put(options, "port", qstring_from_str(port_str));
> g_free(port_str);
> }
> +
> + if (find_prohibited_params(qp, PATH_PARAM)) {
> + ret = -EINVAL;
> + goto out;
> + }
As mentioned above, using Error** will let you return a better error
message, such as "The 'socket' parameter is only valid for NBD over Unix
domain sockets".
> }
>
> out:
> @@ -127,7 +174,7 @@ static void nbd_parse_filename(const char *filename,
> QDict *options,
> Error **errp)
> {
> char *file;
> - char *export_name;
> + char *opt_str;
> const char *host_spec;
> const char *unixpath;
>
> @@ -150,17 +197,6 @@ static void nbd_parse_filename(const char *filename,
> QDict *options,
>
> file = g_strdup(filename);
>
> - export_name = strstr(file, EN_OPTSTR);
> - if (export_name) {
> - if (export_name[strlen(EN_OPTSTR)] == 0) {
> - goto out;
> - }
> - export_name[0] = 0; /* truncate 'file' */
> - export_name += strlen(EN_OPTSTR);
> -
> - qdict_put(options, "export", qstring_from_str(export_name));
> - }
> -
> /* extract the host_spec - fail if it's not nbd:... */
> if (!strstart(file, "nbd:", &host_spec)) {
> error_setg(errp, "File name string for NBD must start with 'nbd:'");
> @@ -173,8 +209,40 @@ static void nbd_parse_filename(const char *filename,
> QDict *options,
>
> /* are we a UNIX or TCP socket? */
> if (strstart(host_spec, "unix:", &unixpath)) {
> + opt_str = (char *) unixpath;
> +
> + /* do we have any options? */
> + /* unixpath could be unix: or unix:something:options */
> + opt_str = strchr(opt_str, ':');
> +
> + /* if we have any options then "divide" */
> + /* the path and the options by replacing the last colon with "\0" */
> + if (opt_str != NULL) {
> + /* truncate 'unixpath' replacing the last ":" */
> + char *colon_pos = opt_str;
> + colon_pos[0] = '\0';
> + opt_str++;
Just this:
if (opt_str != NULL) {
*opt_str++ = 0;
}
> + }
> qdict_put(options, "path", qstring_from_str(unixpath));
> } else {
> + /* host_spec could be ip:port or ip:port:options */
> + int i;
> + opt_str = (char *)host_spec;
> + for (i = 0; i < 2; i++) {
> + opt_str = strchr(opt_str, ':');
> + if (opt_str == NULL) {
> + break;
> + }
> + opt_str++;
> + }
> +
> + /* the same idea with dividing as above */
> + if (opt_str != NULL) {
> + /* truncate 'host_name' replacing the last ":" */
> + char *second_colon_pos = opt_str - 1;
> + second_colon_pos[0] = '\0';
> + }
Again, simpler:
opt_str = strchr((char *)host_spec, ':');
if (opt_str != NULL) {
opt_str = strchr(opt_str + 1, ':');
if (opt_str != NULL) {
*opt_str++ = 0;
}
}
> InetSocketAddress *addr = NULL;
Please avoid declarations in the middle of statements.
>
> addr = inet_parse(host_spec, errp);
> @@ -187,6 +255,43 @@ static void nbd_parse_filename(const char *filename,
> QDict *options,
> qapi_free_InetSocketAddress(addr);
> }
>
> + /* opt_str == NULL means no options given */
> + if (opt_str != NULL) {
> + static QemuOptsList file_opts = {
Please put this declaration outside the function, and call it nbd_opts.
> + .name = "file_opts",
.name = "nbd",
> + .head = QTAILQ_HEAD_INITIALIZER(file_opts.head),
> + .desc = {
> + {
> + .name = EN_OPTSTR,
> + .type = QEMU_OPT_STRING,
> + .help = "Name of the NBD export to open",
> + },
> + },
> + };
> +
> + QemuOpts *opts = qemu_opts_create(&file_opts, NULL, 0, errp);
> + if (opts == NULL) {
> + error_setg(errp, "Can't parse file options");
> + goto out;
> + }
> +
> + Error *local_err = NULL;
> + qemu_opts_do_parse(opts, opt_str, NULL, &local_err);
> + if (local_err) {
> + error_setg(errp, "Can't parse file options");
> + qemu_opts_del(opts);
> + goto out;
> + }
> +
> + const char *value;
> + value = qemu_opt_get(opts, EN_OPTSTR);
> + if (value) {
> + qdict_put(options, "export", qstring_from_str(value));
"export" should be changed to "exportname" in the QDict. This would
probably simplify the code but I don't know the exact function to use.
> + }
> +
> + qemu_opts_del(opts);
> + }
> +
> out:
> g_free(file);
> }
>