On Sat, Mar 25, 2017 at 1:31 AM, Eric Sandeen <sandeen@xxxxxxxxxxx> 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@xxxxxxxxxx>
>> ---
>> 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(
>>
--
Jan Tulak
jtulak@xxxxxxxxxx / jan@xxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
