| Message ID | 20170315160017.27805-7-jtulak@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On 3/15/17 11:00 AM, Jan Tulak wrote: > Checks are modified to work with cross-section conflicts (data, metada, log, ...). More information in the changelog please; this doesn't tell us a whole lot about what this is actually doing. > Signed-off-by: Jan Tulak <jtulak@redhat.com> > --- > mkfs/xfs_mkfs.c | 43 ++++++++++++++++++++++++++++++++++--------- > 1 file changed, 34 insertions(+), 9 deletions(-) > > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c > index 7e0a4159..0877c196 100644 > --- a/mkfs/xfs_mkfs.c > +++ b/mkfs/xfs_mkfs.c > @@ -744,6 +744,9 @@ struct opt_params { > }, > }; > > +static void conflict_struct(struct opt_params *opt, struct subopt_param *subopt, weird tabs there > + struct subopt_conflict *conflict); "conflict_struct" seems like a variable name, not a function name. :) What does this function actually do? > + > #define TERABYTES(count, blog) ((__uint64_t)(count) << (40 - (blog))) > #define GIGABYTES(count, blog) ((__uint64_t)(count) << (30 - (blog))) > #define MEGABYTES(count, blog) ((__uint64_t)(count) << (20 - (blog))) > @@ -1351,10 +1354,9 @@ check_opt( > break; > if (conflict_opt.test_values) > break; > - if (opt->subopt_params[conflict_opt.subopt].seen || > - opt->subopt_params[conflict_opt.subopt].str_seen) { > - conflict(opt->name, (char **)opt->subopts, > - conflict_opt.subopt, index); > + if (opts[conflict_opt.opt].subopt_params[conflict_opt.subopt].seen || > + opts[conflict_opt.opt].subopt_params[conflict_opt.subopt].str_seen) { > + conflict_struct(opt, sp, &conflict_opt); > } > } > } > @@ -1379,13 +1381,12 @@ check_opt_value( > break; > if (!conflict_opt.test_values) > break; > - if ((opt->subopt_params[conflict_opt.subopt].seen || > - opt->subopt_params[conflict_opt.subopt].str_seen) && > - opt->subopt_params[conflict_opt.subopt].value > + if ((opts[conflict_opt.opt].subopt_params[conflict_opt.subopt].seen || > + opts[conflict_opt.opt].subopt_params[conflict_opt.subopt].str_seen) && > + opts[conflict_opt.opt].subopt_params[conflict_opt.subopt].value > == conflict_opt.invalid_value && > value == conflict_opt.at_value) { > - conflict(opt->name, (char **)opt->subopts, > - conflict_opt.subopt, index); > + conflict_struct(opt, sp, &conflict_opt); > } > } > } > @@ -3586,12 +3587,36 @@ conflict( > char *tab[], > int oldidx, > int newidx) > + that newline should not be here ... > { > fprintf(stderr, _("Cannot specify both -%c %s and -%c %s\n"), > opt, tab[oldidx], opt, tab[newidx]); > usage(); > } A comment about what this function actually /does/ would be good. All it really does is printf, I guess - so a function name more like show_conflict() might make more sense? conflict_struct just isn't a good descriptive name. > +static void > +conflict_struct( > + struct opt_params *opt, > + struct subopt_param *subopt, > + struct subopt_conflict *conflict) > +{ > + if(conflict->message) { > + fprintf(stderr, _("Cannot specify both -%c %s and -%c %s: %s\n"), > + opt->name, > + opt->subopts[subopt->index], > + opts[conflict->opt].name, > + opts[conflict->opt].subopts[conflict->subopt], > + _(conflict->message)); > + } else { > + fprintf(stderr, _("Cannot specify both -%c %s and -%c %s\n"), > + opt->name, > + opt->subopts[subopt->index], > + opts[conflict->opt].name, > + opts[conflict->opt].subopts[conflict->subopt]); > + } I wonder if this can be done w/o the cut and paste? > + usage(); > +} > + > > static void > illegal( > -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Mar 25, 2017 at 1:31 AM, Eric Sandeen <sandeen@sandeen.net> wrote: > On 3/15/17 11:00 AM, Jan Tulak wrote: >> Checks are modified to work with cross-section conflicts (data, metada, log, ...). > > More information in the changelog please; this doesn't > tell us a whole lot about what this is actually doing. Like this? Checks are modified to work with cross-section conflicts (data, metada, log, ...). So, it is now possible to detect a conflict between -m foo and -d bar options. This patch only extends checks to test for conflicts across all options instead of only the current one, the opts struct already can declare it. > >> Signed-off-by: Jan Tulak <jtulak@redhat.com> >> --- >> mkfs/xfs_mkfs.c | 43 ++++++++++++++++++++++++++++++++++--------- >> 1 file changed, 34 insertions(+), 9 deletions(-) >> >> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c >> index 7e0a4159..0877c196 100644 >> --- a/mkfs/xfs_mkfs.c >> +++ b/mkfs/xfs_mkfs.c >> @@ -744,6 +744,9 @@ struct opt_params { >> }, >> }; >> >> +static void conflict_struct(struct opt_params *opt, struct subopt_param *subopt, > > weird tabs there > >> + struct subopt_conflict *conflict); > > "conflict_struct" seems like a variable name, not a function name. :) > > What does this function actually do? See bellow at the definition of this function (your other comment). > >> + >> #define TERABYTES(count, blog) ((__uint64_t)(count) << (40 - (blog))) >> #define GIGABYTES(count, blog) ((__uint64_t)(count) << (30 - (blog))) >> #define MEGABYTES(count, blog) ((__uint64_t)(count) << (20 - (blog))) >> @@ -1351,10 +1354,9 @@ check_opt( >> break; >> if (conflict_opt.test_values) >> break; >> - if (opt->subopt_params[conflict_opt.subopt].seen || >> - opt->subopt_params[conflict_opt.subopt].str_seen) { >> - conflict(opt->name, (char **)opt->subopts, >> - conflict_opt.subopt, index); >> + if (opts[conflict_opt.opt].subopt_params[conflict_opt.subopt].seen || >> + opts[conflict_opt.opt].subopt_params[conflict_opt.subopt].str_seen) { >> + conflict_struct(opt, sp, &conflict_opt); >> } >> } >> } >> @@ -1379,13 +1381,12 @@ check_opt_value( >> break; >> if (!conflict_opt.test_values) >> break; >> - if ((opt->subopt_params[conflict_opt.subopt].seen || >> - opt->subopt_params[conflict_opt.subopt].str_seen) && >> - opt->subopt_params[conflict_opt.subopt].value >> + if ((opts[conflict_opt.opt].subopt_params[conflict_opt.subopt].seen || >> + opts[conflict_opt.opt].subopt_params[conflict_opt.subopt].str_seen) && >> + opts[conflict_opt.opt].subopt_params[conflict_opt.subopt].value >> == conflict_opt.invalid_value && >> value == conflict_opt.at_value) { >> - conflict(opt->name, (char **)opt->subopts, >> - conflict_opt.subopt, index); >> + conflict_struct(opt, sp, &conflict_opt); >> } >> } >> } >> @@ -3586,12 +3587,36 @@ conflict( >> char *tab[], >> int oldidx, >> int newidx) >> + > > that newline should not be here ... > >> { >> fprintf(stderr, _("Cannot specify both -%c %s and -%c %s\n"), >> opt, tab[oldidx], opt, tab[newidx]); >> usage(); >> } > > A comment about what this function actually /does/ would be good. > > All it really does is printf, I guess - so a function name more > like show_conflict() might make more sense? conflict_struct just > isn't a good descriptive name. Agreed, renamed to print_conflict_struct. The _struct is to differentiate it from the conflict() function that has a different set of arguments and can't print out the optional message conflicts definitions can have. > >> +static void >> +conflict_struct( >> + struct opt_params *opt, >> + struct subopt_param *subopt, >> + struct subopt_conflict *conflict) >> +{ >> + if(conflict->message) { >> + fprintf(stderr, _("Cannot specify both -%c %s and -%c %s: %s\n"), >> + opt->name, >> + opt->subopts[subopt->index], >> + opts[conflict->opt].name, >> + opts[conflict->opt].subopts[conflict->subopt], >> + _(conflict->message)); >> + } else { >> + fprintf(stderr, _("Cannot specify both -%c %s and -%c %s\n"), >> + opt->name, >> + opt->subopts[subopt->index], >> + opts[conflict->opt].name, >> + opts[conflict->opt].subopts[conflict->subopt]); >> + } > > I wonder if this can be done w/o the cut and paste? Yes. After few minutes of thinking how to nicely concatenate strings while avoiding allocs, I realised I can just print out the basic error, conditionally print the conflict->message and then always print "\n". I guess I overcomplicate things sometimes... :-/ Anyway, changed for the next version of this patch. > >> + usage(); >> +} >> + >> >> static void >> illegal( >>
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c index 7e0a4159..0877c196 100644 --- a/mkfs/xfs_mkfs.c +++ b/mkfs/xfs_mkfs.c @@ -744,6 +744,9 @@ struct opt_params { }, }; +static void conflict_struct(struct opt_params *opt, struct subopt_param *subopt, + struct subopt_conflict *conflict); + #define TERABYTES(count, blog) ((__uint64_t)(count) << (40 - (blog))) #define GIGABYTES(count, blog) ((__uint64_t)(count) << (30 - (blog))) #define MEGABYTES(count, blog) ((__uint64_t)(count) << (20 - (blog))) @@ -1351,10 +1354,9 @@ check_opt( break; if (conflict_opt.test_values) break; - if (opt->subopt_params[conflict_opt.subopt].seen || - opt->subopt_params[conflict_opt.subopt].str_seen) { - conflict(opt->name, (char **)opt->subopts, - conflict_opt.subopt, index); + if (opts[conflict_opt.opt].subopt_params[conflict_opt.subopt].seen || + opts[conflict_opt.opt].subopt_params[conflict_opt.subopt].str_seen) { + conflict_struct(opt, sp, &conflict_opt); } } } @@ -1379,13 +1381,12 @@ check_opt_value( break; if (!conflict_opt.test_values) break; - if ((opt->subopt_params[conflict_opt.subopt].seen || - opt->subopt_params[conflict_opt.subopt].str_seen) && - opt->subopt_params[conflict_opt.subopt].value + if ((opts[conflict_opt.opt].subopt_params[conflict_opt.subopt].seen || + opts[conflict_opt.opt].subopt_params[conflict_opt.subopt].str_seen) && + opts[conflict_opt.opt].subopt_params[conflict_opt.subopt].value == conflict_opt.invalid_value && value == conflict_opt.at_value) { - conflict(opt->name, (char **)opt->subopts, - conflict_opt.subopt, index); + conflict_struct(opt, sp, &conflict_opt); } } } @@ -3586,12 +3587,36 @@ conflict( char *tab[], int oldidx, int newidx) + { fprintf(stderr, _("Cannot specify both -%c %s and -%c %s\n"), opt, tab[oldidx], opt, tab[newidx]); usage(); } +static void +conflict_struct( + struct opt_params *opt, + struct subopt_param *subopt, + struct subopt_conflict *conflict) +{ + if(conflict->message) { + fprintf(stderr, _("Cannot specify both -%c %s and -%c %s: %s\n"), + opt->name, + opt->subopts[subopt->index], + opts[conflict->opt].name, + opts[conflict->opt].subopts[conflict->subopt], + _(conflict->message)); + } else { + fprintf(stderr, _("Cannot specify both -%c %s and -%c %s\n"), + opt->name, + opt->subopts[subopt->index], + opts[conflict->opt].name, + opts[conflict->opt].subopts[conflict->subopt]); + } + usage(); +} + static void illegal(
Checks are modified to work with cross-section conflicts (data, metada, log, ...). Signed-off-by: Jan Tulak <jtulak@redhat.com> --- mkfs/xfs_mkfs.c | 43 ++++++++++++++++++++++++++++++++++--------- 1 file changed, 34 insertions(+), 9 deletions(-)