On 8/25/20 8:56 PM, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
>
> Now we have the config file parsing hooked up and feeding in
> parameters to mkfs, wire the parameters up to the existing CLI
> option parsing functions. THis gives the config file exactly the
> same capabilities and constraints as the command line option
> specification.
And as such, as you already mentioned, respecifications on the command
line will fail. That can be documented in the man page :)
The section names will need to be documented too.
(In a very much not-bikeshedding way, we could consider [b] rather than
[block] so you can cover it with "the section names match the option
characters, i.e. for -b one would use section name [b]" but I really don't
care and will not mention this again because as long as it's documented
it's fine.)
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> ---
> include/linux.h | 2 +-
> mkfs/xfs_mkfs.c | 121 +++++++++++++++++++++++++++++++++++++-----------
> 2 files changed, 95 insertions(+), 28 deletions(-)
>
> diff --git a/include/linux.h b/include/linux.h
> index 57726bb12b74..03b3278bb895 100644
> --- a/include/linux.h
> +++ b/include/linux.h
> @@ -92,7 +92,7 @@ static __inline__ void platform_uuid_unparse(uuid_t *uu, char *buffer)
> uuid_unparse(*uu, buffer);
> }
>
> -static __inline__ int platform_uuid_parse(char *buffer, uuid_t *uu)
> +static __inline__ int platform_uuid_parse(const char *buffer, uuid_t *uu)
> {
> return uuid_parse(buffer, *uu);
> }
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 6a373d614a56..deaed551b6d1 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -142,6 +142,13 @@ enum {
> * name MANDATORY
> * Name is a single char, e.g., for '-d file', name is 'd'.
> *
> + * ini_name MANDATORY
> + * Name is a string, not longer than MAX_INI_NAME_LEN, that is used as the
> + * section name for this option set in INI format config files. The only
> + * option set this is not required for is the command line config file
> + * specification options, everything else must be configurable via config
> + * files.
Nothing actually enforces this MANDATORY, right, this is just documentation.
So the fact that struct opt_params copts doesn't have it, is fine.
> @@ -967,13 +984,24 @@ respec(
>
> static void
> unknown(
> - char opt,
> - char *s)
> + const char opt,
> + const char *s)
(can all of the constification could maybe go in its own patch just to cut
down on the patch doomscrolling or does it have to go with the other changes?)
> {
> fprintf(stderr, _("unknown option -%c %s\n"), opt, s);
> usage();
> }
>
> +static void
> +unknown_cfgfile_opt(
> + const char *section,
> + const char *name,
> + const char *value)
> +{
> + fprintf(stderr, _("unknown config file option: [%s]:%s=%s\n"),
> + section, name, value);
If we allow more than one -c subopt in the future we might want to print
the filename. Wouldn't /hurt/ to do so now, or just remember to do it if
we ever add a 2nd -c subopt.
> + usage();
> +}
> +
> static void
> check_device_type(
> const char *name,
> @@ -1379,7 +1407,7 @@ getnum(
> */
> static char *
> getstr(
> - char *str,
> + const char *str,
> struct opt_params *opts,
> int index)
> {
> @@ -1388,14 +1416,14 @@ getstr(
> /* empty strings for string options are not valid */
> if (!str || *str == '\0')
> reqval(opts->name, opts->subopts, index);
> - return str;
> + return (char *)str;
(what's the cast for?)
> @@ -1682,23 +1710,22 @@ sector_opts_parser(
> }
>
> static struct subopts {
> - char opt;
> struct opt_params *opts;
> int (*parser)(struct opt_params *opts,
> int subopt,
> - char *value,
> + const char *value,
> struct cli_params *cli);
> } subopt_tab[] = {
> - { 'b', &bopts, block_opts_parser },
> - { 'c', &copts, cfgfile_opts_parser },
> - { 'd', &dopts, data_opts_parser },
> - { 'i', &iopts, inode_opts_parser },
> - { 'l', &lopts, log_opts_parser },
> - { 'm', &mopts, meta_opts_parser },
> - { 'n', &nopts, naming_opts_parser },
> - { 'r', &ropts, rtdev_opts_parser },
> - { 's', &sopts, sector_opts_parser },
> - { '\0', NULL, NULL },
> + { &bopts, block_opts_parser },
> + { &copts, cfgfile_opts_parser },
> + { &dopts, data_opts_parser },
> + { &iopts, inode_opts_parser },
> + { &lopts, log_opts_parser },
> + { &mopts, meta_opts_parser },
> + { &nopts, naming_opts_parser },
> + { &ropts, rtdev_opts_parser },
> + { &sopts, sector_opts_parser },
> + { NULL, NULL },
> };
>
> static void
> @@ -1712,12 +1739,12 @@ parse_subopts(
> int ret = 0;
>
> while (sop->opts) {
> - if (sop->opt == opt)
> + if (opt && sop->opts->name == opt)
> break;
> sop++;
> }
>
> - /* should never happen */
> + /* Should not happen */
ok? :)
Overall this seems remarkably tidy, thanks.
-Eric
