Message ID | 1475659988-20500-2-git-send-email-den@openvz.org |
---|---|
State | New |
Headers | show |
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 <dplotnikov@virtuozzo.com> > > 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 <dplotnikov@virtuozzo.com> > Signed-off-by: Denis V. Lunev <den@openvz.org> > CC: Paolo Bonzini <pbonzini@redhat.com> > CC: Kevin Wolf <kwolf@redhat.com> > CC: Max Reitz <mreitz@redhat.com> > --- > 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); > } >
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" + +#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; +} + +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; +} + + static int nbd_parse_uri(const char *filename, QDict *options) { URI *uri; @@ -79,18 +121,18 @@ static int nbd_parse_uri(const char *filename, QDict *options) } qp = query_params_parse(uri->query); - if (qp->n > 1 || (is_unix && !qp->n) || (!is_unix && qp->n)) { - ret = -EINVAL; - goto out; - } if (is_unix) { /* nbd+unix:///export?socket=path */ - if (uri->server || uri->port || strcmp(qp->p[0].name, "socket")) { + if (uri->server || uri->port) { + ret = -EINVAL; + goto out; + } + + 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; + } } 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++; + } 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'; + } + InetSocketAddress *addr = NULL; 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 = { + .name = "file_opts", + .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)); + } + + qemu_opts_del(opts); + } + out: g_free(file); }