lkml.org 
[lkml]   [2019]   [Dec]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] vfs: Don't reject unknown parameters
    On Thu, Dec 12, 2019 at 03:01:56PM -0500, Laura Abbott wrote:

    > +static const struct fs_parameter_spec no_opt_fs_param_specs[] = {
    > + fsparam_string ("source", NO_OPT_SOURCE),
    > + {}
    > +};
    > +
    > +const struct fs_parameter_description no_opt_fs_parameters = {
    > + .name = "no_opt_fs",
    > + .specs = no_opt_fs_param_specs,
    > +};
    > +
    > +int fs_no_opt_parse_param(struct fs_context *fc, struct fs_parameter *param)
    > +{
    > + struct fs_parse_result result;
    > + int opt;
    > +
    > + opt = fs_parse(fc, &no_opt_fs_parameters, param, &result);
    > + if (opt < 0) {
    > + /* Just log an error for backwards compatibility */
    > + errorf(fc, "%s: Unknown parameter '%s'",
    > + fc->fs_type->name, param->key);
    > + return 0;
    > + }
    > +
    > + switch (opt) {
    > + case NO_OPT_SOURCE:
    > + if (param->type != fs_value_is_string)
    > + return invalf(fc, "%s: Non-string source",
    > + fc->fs_type->name);
    > + if (fc->source)
    > + return invalf(fc, "%s: Multiple sources specified",
    > + fc->fs_type->name);
    > + fc->source = param->string;
    > + param->string = NULL;
    > + break;
    > + }
    > +
    > + return 0;
    > +}
    > +EXPORT_SYMBOL(fs_no_opt_parse_param);

    Yecchhhh... Could you explain why do you want to bother with fs_parse()
    here? Seriously, look at it.
    {
    const struct fs_parameter_spec *p;
    const struct fs_parameter_enum *e;
    int ret = -ENOPARAM, b;

    result->has_value = !!param->string;
    result->negated = false;
    result->uint_64 = 0;

    p = fs_lookup_key(desc, param->key);
    OK, that's
    if (strcmp(param->key, "source") == 0)
    p = no_opt_fs_param_specs;
    else
    p = NULL;

    if (!p) {
    not "source"
    /* If we didn't find something that looks like "noxxx", see if
    * "xxx" takes the "no"-form negative - but only if there
    * wasn't an value.
    */
    if (result->has_value)
    goto unknown_parameter;
    if param->string is non-NULL - piss off

    if (param->key[0] != 'n' || param->key[1] != 'o' || !param->key[2])
    goto unknown_parameter;
    if not "no"<something> - ditto

    p = fs_lookup_key(desc, param->key + 2);
    if (!p)
    goto unknown_parameter;
    if not "nosource" - ditto
    if (!(p->flags & fs_param_neg_with_no))
    goto unknown_parameter;
    ... and since ->flags doesn't have that bit, the same for "nosource" anyway.
    result->boolean = false;
    result->negated = true;
    won't get here
    }

    OK, so the above is simply 'piss off unless param->key is "source"'. And
    p is no_opt_fs_param_specs.

    if (p->flags & fs_param_deprecated)
    nope
    warnf(fc, "%s: Deprecated parameter '%s'",
    desc->name, param->key);

    if (result->negated)
    goto okay;
    nope - set to false, never changed
    /* Certain parameter types only take a string and convert it. */
    switch (p->type) {
    that'd be fs_param_is_string
    ...
    case fs_param_is_string:
    if (param->type != fs_value_is_string)
    goto bad_value;
    if (!result->has_value) {
    if param->string is NULL...
    if (p->flags & fs_param_v_optional)
    nope
    goto okay;
    goto bad_value;
    }
    /* Fall through */
    default:
    break;
    }

    /* Try to turn the type we were given into the type desired by the
    * parameter and give an error if we can't.
    */
    switch (p->type) {
    again, fs_param_is_string
    ...
    case fs_param_is_string:
    goto okay;
    ...
    okay:
    return p->opt;
    bad_value:
    return invalf(fc, "%s: Bad value for '%s'", desc->name, param->key);
    unknown_parameter:
    return -ENOPARAM;


    In other words, that thing is equivalent to
    if (strcmp(param->key, "source")) {
    errorf(fc, "%s: Unknown parameter '%s'",
    fc->fs_type->name, param->key);
    return 0;
    }
    if (param->type != fs_value_is_string || !param->value) {
    invalf(fc, "%s: Bad value for '%s'", fc->fs_type->name, param->key);
    errorf(fc, "%s: Unknown parameter '%s'",
    fc->fs_type->name, param->key);
    return 0; // almost certainly not what you intended for that case
    }
    if (fc->source)
    return invalf(fc, "%s: Multiple sources specified", fc->fs_type->name);
    fc->source = param->string;
    param->string = NULL;
    return 0;

    And that - without the boilerplate from hell. But if you look at the caller of
    that method, you'll see this:
    if (fc->ops->parse_param) {
    ret = fc->ops->parse_param(fc, param);
    if (ret != -ENOPARAM)
    return ret;
    }

    /* If the filesystem doesn't take any arguments, give it the
    * default handling of source.
    */
    if (strcmp(param->key, "source") == 0) {
    if (param->type != fs_value_is_string)
    return invalf(fc, "VFS: Non-string source");
    if (fc->source)
    return invalf(fc, "VFS: Multiple sources");
    fc->source = param->string;
    param->string = NULL;
    return 0;
    }

    return invalf(fc, "%s: Unknown parameter '%s'",
    fc->fs_type->name, param->key);
    }

    So you could bloody well just leave recognition (and handling) of "source"
    to the caller, leaving you with just this:

    if (strcmp(param->key, "source") == 0)
    return -ENOPARAM;
    /* Just log an error for backwards compatibility */
    errorf(fc, "%s: Unknown parameter '%s'", fc->fs_type->name, param->key);
    return 0;

    and that's it.

    \
     
     \ /
      Last update: 2019-12-12 22:37    [W:4.301 / U:0.200 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site