About the CLI of both grub-mkrescue versions

About the CLI of both grub-mkrescue versions

[Thomas Schmitt]

Hi,

i recently advertised grub-mkrescue as replacement of
script snippets for creating ISO images with legacy GRUB.

But it is not very appealing to the audience if i have
to mention the different interpretation of argument "--"
by the shell script of GRUB-2.00 and by the C program
of the git master branch.

The C program demands "--" before any custom options for
xorriso -as mkisofs are accepted. Omitting it lets argp_parser()
return ARGP_ERR_UNKNOWN rather than adding the mkisofs option
to the xorriso argument list.
The shell script forwards "--" to xorriso -as mkisofs,
where it ends the mkisofs emulation. Afterwards, the additional
mkisofs options are not understood by xorriso.

We discussed this a while ago in the thread following
  http://lists.gnu.org/archive/html/grub-devel/2014-01/msg00074.html
There was no conclusion.


If this CLI change shall persist, then there needs to be
some indication for scripts, whether the "--" is mandatory or
harmful.
Further, the documentation of grub-mkrescue needs to be updated.
Including the help text in the C code, which currently describes
the behavior of the shell script.

In my personal view, it would be better to keep the behavior of
GRUB-2.00. E.g. by replacing in grub-mkrescue.c line 229
    return ARGP_ERR_UNKNOWN;
by a call of xorriso_push() ... i guess ...


Have a nice day :)

Thomas

Re: About the CLI of both grub-mkrescue versions

[Andrei Borzenkov]

В Tue, 23 Sep 2014 10:12:12 +0200
"Thomas Schmitt" <scdbackup@gmx.net> пишет:

> Hi,
> 
> i recently advertised grub-mkrescue as replacement of
> script snippets for creating ISO images with legacy GRUB.
> 
> But it is not very appealing to the audience if i have
> to mention the different interpretation of argument "--"
> by the shell script of GRUB-2.00 and by the C program
> of the git master branch.
> 
> The C program demands "--" before any custom options for
> xorriso -as mkisofs are accepted. Omitting it lets argp_parser()
> return ARGP_ERR_UNKNOWN rather than adding the mkisofs option
> to the xorriso argument list.
> The shell script forwards "--" to xorriso -as mkisofs,
> where it ends the mkisofs emulation. Afterwards, the additional
> mkisofs options are not understood by xorriso.
> 
> We discussed this a while ago in the thread following
>   http://lists.gnu.org/archive/html/grub-devel/2014-01/msg00074.html
> There was no conclusion.
> 
> 
> If this CLI change shall persist, then there needs to be
> some indication for scripts, whether the "--" is mandatory or
> harmful.
> Further, the documentation of grub-mkrescue needs to be updated.

Care to send a patch for both help output and documentation?

> Including the help text in the C code, which currently describes
> the behavior of the shell script.
> 
> In my personal view, it would be better to keep the behavior of
> GRUB-2.00. E.g. by replacing in grub-mkrescue.c line 229
>     return ARGP_ERR_UNKNOWN;
> by a call of xorriso_push() ... i guess ...
> 


It won't work. Unknown options are detected before user parser is
called; user parser never sees them. So either you reimplement argument
processing or you have to bite the bullet and accept that behavior is
now conforms to standard one ... 

Or you can raise this issue on gnulib list, whether they can consider
implementing this (passing unknown options through to user parser).

Re: About the CLI of both grub-mkrescue versions

[Thomas Schmitt]

Hi,

Andrei Borzenkov wrote:

> Care to send a patch for both help output and documentation?

First one should decide whether the incompatible CLI change
is really intentional.

If the change is intentional, there should be some assistance
to those people who based their scripts on the old behavior.
A decision would be needed about the desired grade of assistance.


me:
> > In my personal view, it would be better to keep the behavior of
> > GRUB-2.00. E.g. by replacing in grub-mkrescue.c line 229
> >     return ARGP_ERR_UNKNOWN;
> > by a call of xorriso_push() ... i guess ...

Andrei Borzenkov wrote:
> It won't work. Unknown options are detected before user parser is
> called; user parser never sees them.

I see.
If the change is not intentional, then an own parser would be
needed. More or less what the shell code of old grub-mkrescue
does. Not an overly demanding task.


Don't get me wrong. I will support what gets decided.
If new features are needed in xorriso: no problem, just ask
for them.

Nevertheless, the current state of the change is inconsistent.
Implementation and documentation contradict. The most recent
statement known to me from Vladimir indicated that he was still
undecided, back then.

I posted here because i encountered a legacy script which
equips a Linux distro by either GRUB-legacy or by ISOLINUX 4.
The ISOLINUX part is easy to update to version 5 or 6. Just
one more file to copy into the /isolinux directory.

The legacy GRUB part needs a total re-implementation on base
of grub-mkrescue. This is already quite an obstacle.
But telling people that this work will have to be adapted
in the foreseeable future to an incomptible CLI change ...


Have a nice day :)

Thomas

Re: About the CLI of both grub-mkrescue versions

[Thomas Schmitt]

Hi,

how about this:

-----------------------------------------------------------------

The C program gets renamed to grub-mkiso.c. The binary gets
installed under two names: grub-mkiso and grub-mkrescue.

If started as "grub-mkiso" the program implements the change in
the CLI. I.e. "--" marks the start of xorriso -as mkisofs options.

If started as "grub-mkrescue", an own argument parser implements
the old behavior of the grub-mkrescue(.in) script. I.e. unknown
arguments are used as xorriso -as mkisofs options. The unknown
argument "--" then causes xorriso -as mkisofs emulation to end.

-----------------------------------------------------------------

This would avoid to bother scripting users of grub-mkrescue
or maintainers of things like
  http://www.unix.com/man-page/linux/1/GRUB-MKRESCUE/

The name grub-mkiso would be well appropriate because the program
produces GRUB2 bootable ISO images not only for rescue systems,
but also for system distributions and live CDs.

Needed would be:
- Legacy parser in the C program compatible to old grub-mkrescue.in
- New help text in C program to reflect grub-mkiso CLI.
- Old help text in C program gets triggered by the legacy parser.
  It should mention that there is also the more modern grub-mkiso.
- Update docs/grub.texi, describing grub-mkiso and mentioning that
  grub-mkrescue is outdated.

Except the legacy parser, all these tasks have to be addressed
anyway, in order to reflect the current change from grub-mkrescue.in
to grub-mkrescue.c.


Have a nice day :)

Thomas

Re: About the CLI of both grub-mkrescue versions

[Andrei Borzenkov]

On Sun, Sep 28, 2014 at 10:28 PM, Thomas Schmitt <scdbackup@gmx.net> wrote:
> Hi,
>
> how about this:
>
> -----------------------------------------------------------------
>
> The C program gets renamed to grub-mkiso.c. The binary gets
> installed under two names: grub-mkiso and grub-mkrescue.
>
> If started as "grub-mkiso" the program implements the change in
> the CLI. I.e. "--" marks the start of xorriso -as mkisofs options.
>
> If started as "grub-mkrescue", an own argument parser implements
> the old behavior of the grub-mkrescue(.in) script.

What's the point of having to maintain two versions instead of one?

>                                                                       I.e. unknown
> arguments are used as xorriso -as mkisofs options. The unknown
> argument "--" then causes xorriso -as mkisofs emulation to end.
>
> -----------------------------------------------------------------
>
> This would avoid to bother scripting users of grub-mkrescue
> or maintainers of things like
>   http://www.unix.com/man-page/linux/1/GRUB-MKRESCUE/
>
> The name grub-mkiso would be well appropriate because the program
> produces GRUB2 bootable ISO images not only for rescue systems,
> but also for system distributions and live CDs.
>
> Needed would be:
> - Legacy parser in the C program compatible to old grub-mkrescue.in
> - New help text in C program to reflect grub-mkiso CLI.
> - Old help text in C program gets triggered by the legacy parser.
>   It should mention that there is also the more modern grub-mkiso.
> - Update docs/grub.texi, describing grub-mkiso and mentioning that
>   grub-mkrescue is outdated.
>
> Except the legacy parser, all these tasks have to be addressed
> anyway, in order to reflect the current change from grub-mkrescue.in
> to grub-mkrescue.c.
>
>
> Have a nice day :)
>
> Thomas
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

Re: About the CLI of both grub-mkrescue versions

[Thomas Schmitt]

Hi,

me:
> > grub-mkiso.c

Andrei Borzenkov:
> What's the point of having to maintain two versions instead of one?

Well, as stated:

> > This would avoid to bother scripting users of grub-mkrescue
> > or maintainers of things like
> >   http://www.unix.com/man-page/linux/1/GRUB-MKRESCUE/

Besides sabotageing scripts which use old grub-mkrescue,
the current state of grub-mkrescue.c also makes it hard to
write scripts which work with both, GRUB 2.00 and 2.02.

The documentation of the current state of grub-mkrescue.c
would have to be something like:

"Find out what version of grub-mkrescue you are running.
 If it is the new one of GRUB 2.02 or later, then use
 argument "--" between its own arguments and the arguments
 by which you announce your payload files to xorriso.
 If it is the old one, then do not use argument "--" unless you
 want xorriso to end its mkisofs emulation and thus to demand
 native xorriso commands as further input."

With my proposal it would rather be:

"grub-mkrescue is frozen and will not be augmented by new
 features. The new program grub-mkiso takes the same options
 as grub-mkrescue plus some more.
 The main difference between both is that you have to
 separate the grub-mkiso options from the xorriso -as mkisofs
 options by a double dash "--", which will not be forwarded
 to xorriso."

The compatibility mode will not need permanent maintainance.
It will rather be an alternative way to override the defaults
of some of the global variables in grub-mkrescue.c. Once the
legacy parser function is written, it can stay as it is.
Only if the variable names or meanings change, this has to be
reflected in the legacy parser function.

The other tasks which i mentioned, have to be addressed anyway,
because currently the documentation of grub-mkrescue is
flatly wrong in the program and in the .texi file.


Have a nice day :)

Thomas

Re: About the CLI of both grub-mkrescue versions

[Thomas Schmitt]

Hi,

to substantiate my proposal of renaming young grub-mkrescue.c to
grub-mkiso.c and to add a built-in emulation of grub-mkrescue(.in),
here the necessary code which i tested standalone with valgrind.

The decision which parser to use would be made in main():

--------------------------------------------------------------

  char *cpt;
  ...
  /* Get leaf name of argv[0] */
  for (cpt = argv[0] + strlen (argv[0]) - 1; cpt >= argv[0]; cpt--)
    if (*cpt == '/')
      break;
  cpt++;
  if (strcmp (cpt, "grub-mkrescue") == 0)
    {
      arg_parser_mkrescue (argc, argv);
    }
  else
    {
      argp_parse (&argp, argc, argv, 0, 0, 0);
    }

--------------------------------------------------------------

The help text is derived from grub-mkrescue.in of GRUB 2.00.

--------------------------------------------------------------

static void
printc (char *line)
{
  printf ("%s\n", line);
}

static void
print_mkrescue_help (char *prog_name)
{
  printf ("%s %s %s\n", _("Usage:"), prog_name, _("[OPTION] SOURCE..."));
  printc (_("Make GRUB CD-ROM, disk, pendrive and floppy bootable image."));
  printc (_("-h, --help"));
  printc (_("        print this message and exit"));
  printc (_("-v, --version"));
  printc (_("        print the version information and exit"));
  printc (_("-o, --output=FILE"));
  printc (_("        save output in FILE [required]"));
  printc (_("--rom-directory=DIR"));
  printc (_("        save ROM images in DIR [optional]"));
  printc (_("--xorriso=FILE"));
  printc (_("        use FILE as xorriso [optional]"));
  printc (_("Not supported any more are:"));
  printc (_("        --modules , --grub-mkimage , --override-directory"));
  printc (_("Other arguments get forwarded to xorriso -as mkisofs"));
  printc (_("emulation."));
  printc ("");
  printf ("%s %s\n", prog_name, _("generates a bootable rescue image"));
  printc (_("with specified source files, source directories, or mkisofs"));
  printc (_("options listed by the output of `xorriso -as mkisofs -help'"));
  printc ("");
  printc (_("Note: Do not use option \"--\" unless you want to submit"));
  printc (_("      native xorriso commands instead of file paths or"));
  printc (_("      mkisofs options. See man xorrisofs and man xorriso."));
  printc ("");
  printc (_("Report bugs to <bug-grub@gnu.org>."));
  printc (_("Mail xorriso support requests to <bug-xorriso@gnu.org>."));
  printc ("");
  printc (_("This is program grub-mkiso emulating the option"));
  printc (_("interpretation of legacy program grub-mkrescue."));
  printc (_("grub-mkiso in its native mode has more advanced options."));
  printc (_("But that mode demands to separate grub-mkiso options"));
  printc (_("and xorriso options by a double dash \"--\", which xorriso"));
  printc (_("will not get to see."));
}

--------------------------------------------------------------

The parser function implements the promised options and collects
the xorriso -as mkisofs options into the same char pointer array
as does the existing parser in the C program.
In particular it sets the values of these variables:

static char *rom_directory;
static int xorriso_tail_argc;
static int xorriso_tail_arg_alloc;
static char **xorriso_tail_argv;
static char *output_image;
static char *xorriso;

--------------------------------------------------------------

static void
arg_parser_mkrescue (int argc, char *argv[])
{
  int i;

  for (i = 1; i < argc; i++)
    {
      if (strcmp (argv[i], "-h") == 0 || strcmp (argv[i], "--help") == 0)
        {
          print_mkrescue_help (argv[0]);
          exit (0);
        }
      else if (strcmp (argv[i], "-v") == 0
               || strcmp (argv[i], "--version") == 0)
        {
          printf ("%s %s %s\n", argv[0], PACKAGE_NAME, PACKAGE_VERSION);
          exit (0);
        }
      else if (strcmp (argv[i], "--modules") == 0)
        {
          grub_util_error (_("Option --modules is not supported any more"));
        }
      else if (strncmp (argv[i], "--modules=", 10) == 0)
        {
          grub_util_error (_("Option --modules= is not supported any more"));
        }
      else if (strcmp (argv[i], "-o") == 0
               || strcmp (argv[i], "--output") == 0)
        {
          if (i == argc - 1)
            grub_util_error (_("option requires an argument -- `%s'"),
                             argv[i]);
          i++;
          free (output_image);
          output_image = xstrdup (argv[i]);
        }
      else if (strncmp (argv[i], "--output=", 9) == 0)
        {
          free (output_image);
          output_image = xstrdup (argv[i] + 9);
        }
      else if (strcmp (argv[i], "--rom-directory") == 0)
        {
          if (i == argc - 1)
            grub_util_error (_("option requires an argument -- `%s'"),
                             argv[i]);
          i++;
          free (rom_directory);
          rom_directory = xstrdup (argv[i]);
        }
      else if (strncmp (argv[i], "--rom-directory=", 16) == 0)
        {
          free (rom_directory);
          rom_directory = xstrdup (argv[i] + 16);
        }
      else if (strcmp (argv[i], "--override-directory") == 0)
        {
          grub_util_error(
                   _("Option --override-directory is not supported any more"));
        }
      else if (strncmp (argv[i], "--override-directory=", 21) == 0)
        {
          grub_util_error(
                  _("Option --override-directory= is not supported any more"));
        }
      else if (strcmp (argv[i], "--xorriso") == 0)
        {
          if (i == argc - 1)
            grub_util_error (_("option requires an argument -- `%s'"),
                             argv[i]);
          i++;
          free (xorriso);
          xorriso = xstrdup (argv[i]);
        }
      else if (strncmp (argv[i], "--xorriso=", 10) == 0)
        {
          free (xorriso);
          xorriso = xstrdup (argv[i] + 10);
        }
      else
        {
          if (xorriso_tail_arg_alloc <= xorriso_tail_argc)
            {
              xorriso_tail_arg_alloc = 2 * (4 + xorriso_tail_argc);
              xorriso_tail_argv = xrealloc (xorriso_tail_argv,
                                      sizeof (xorriso_tail_argv[0])
                                      * xorriso_tail_arg_alloc);
            }
          xorriso_tail_argv[xorriso_tail_argc++] = xstrdup (argv[i]);
        }
    }
}

--------------------------------------------------------------

There remains the incompatibility that i could not find
equivalents of the following grub-mkrescue.in features:

  --modules=MODULES
          pre-load specified modules MODULES
  --grub-mkimage=FILE
          use FILE as grub-mkimage
  --override-directory
          "Intentionally undocumented"

--------------------------------------------------------------

The valgrind test was made with a mock-up of grub-mkiso.c
consisting of some copied util/grub-*.c functions and a main()
which prints the content of the variables after parsing.

  ln -s grub-mkiso grub-mkrescue
  valgrind ./grub-mkrescue -o output.iso -J --rom-directory=./ROMDIR -P YET_ANOTHER_OS.ORG --md5 --emul-toc ./my_os_payload --xorriso="$HOME"/xorriso-1.3.8/xorriso/xorriso

yielded:

  output_image=           'output.iso'
  rom_directory=          './ROMDIR'
  xorriso=                '/home/thomas/xorriso-1.3.8/xorriso/xorriso'
  xorriso_tail_arg_alloc= 8
  xorriso_tail_argc=      6
  xorriso_tail_argv[ 0]= '-J'
  xorriso_tail_argv[ 1]= '-P'
  xorriso_tail_argv[ 2]= 'YET_ANOTHER_OS.ORG'
  xorriso_tail_argv[ 3]= '--md5'
  xorriso_tail_argv[ 4]= '--emul-toc'
  xorriso_tail_argv[ 5]= './my_os_payload'
  ==9810== 
  ==9810== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 2 from 1)
  ...
  ==9810== LEAK SUMMARY:
  ==9810==    definitely lost: 0 bytes in 0 blocks.
  ==9810==      possibly lost: 0 bytes in 0 blocks.
  ==9810==    still reachable: 185 bytes in 10 blocks.
  ==9810==         suppressed: 0 bytes in 0 blocks.

The memory leaks are caused by allocated storage of global
variables.


Have a nice day :)

Thomas

Re: About the CLI of both grub-mkrescue versions

[Andrei Borzenkov]

В Wed, 01 Oct 2014 09:25:12 +0200
"Thomas Schmitt" <scdbackup@gmx.net> пишет:

> Hi,
> 
> to substantiate my proposal of renaming young grub-mkrescue.c to
> grub-mkiso.c and to add a built-in emulation of grub-mkrescue(.in),
> here the necessary code which i tested standalone with valgrind.

Well, below is much more simple patch which is reusing argp
infrastructure. The only problem - it needs small patch for gnulib
otherwise --help does not work. IMHO this is a bug in gnulib. If you
convince them to fix it ...

I do not say that I particular like it, but looks better than
reimplement parser from scratch.

---
 grub-core/gnulib/argp-parse.c |  7 +++++--
 util/grub-mkrescue.c          | 37 ++++++++++++++++++++++++++++---------
 2 files changed, 33 insertions(+), 11 deletions(-)

diff --git a/grub-core/gnulib/argp-parse.c b/grub-core/gnulib/argp-parse.c
index 67ea32c..8fb50e1 100644
--- a/grub-core/gnulib/argp-parse.c
+++ b/grub-core/gnulib/argp-parse.c
@@ -89,13 +89,16 @@ static const struct argp_option argp_default_options[] =
 static error_t
 argp_default_parser (int key, char *arg, struct argp_state *state)
 {
+  struct argp_state help_state = *state;
+
+  help_state.flags &= ~ARGP_NO_ERRS;
   switch (key)
     {
     case '?':
-      __argp_state_help (state, state->out_stream, ARGP_HELP_STD_HELP);
+      __argp_state_help (&help_state, state->out_stream, ARGP_HELP_STD_HELP);
       break;
     case OPT_USAGE:
-      __argp_state_help (state, state->out_stream,
+      __argp_state_help (&help_state, state->out_stream,
                          ARGP_HELP_USAGE | ARGP_HELP_EXIT_OK);
       break;
 
diff --git a/util/grub-mkrescue.c b/util/grub-mkrescue.c
index e719839..8decb4e 100644
--- a/util/grub-mkrescue.c
+++ b/util/grub-mkrescue.c
@@ -70,6 +70,19 @@ xorriso_push (const char *val)
 }
 
 static void
+xorriso_push_tail (const char *val)
+{
+  if (xorriso_tail_arg_alloc <= xorriso_tail_argc)
+    {
+      xorriso_tail_arg_alloc = 2 * (4 + xorriso_tail_argc);
+      xorriso_tail_argv = xrealloc (xorriso_tail_argv,
+			       sizeof (xorriso_tail_argv[0])
+			       * xorriso_tail_arg_alloc);
+    }
+  xorriso_tail_argv[xorriso_tail_argc++] = xstrdup (val);
+}
+
+static void
 xorriso_link (const char *from, const char *to)
 {
   char *tof = grub_util_path_concat (2, iso9660_dir, to);
@@ -156,6 +169,12 @@ enum {
 static error_t 
 argp_parser (int key, char *arg, struct argp_state *state)
 {
+  if (state->quoted && !(int *)state->input)
+    {
+      *(int *)state->input = 1;
+      xorriso_push_tail ("--");
+    }
+
   if (grub_install_parse (key, arg))
     return 0;
   switch (key)
@@ -216,14 +235,7 @@ argp_parser (int key, char *arg, struct argp_state *state)
       return 0;
 
     case ARGP_KEY_ARG:
-      if (xorriso_tail_arg_alloc <= xorriso_tail_argc)
-	{
-	  xorriso_tail_arg_alloc = 2 * (4 + xorriso_tail_argc);
-	  xorriso_tail_argv = xrealloc (xorriso_tail_argv,
-				   sizeof (xorriso_tail_argv[0])
-				   * xorriso_tail_arg_alloc);
-	}
-      xorriso_tail_argv[xorriso_tail_argc++] = xstrdup (arg);
+      xorriso_push_tail (arg);
       return 0;
     default:
       return ARGP_ERR_UNKNOWN;
@@ -380,6 +392,8 @@ main (int argc, char *argv[])
   char *romdir;
   char *sysarea_img = NULL;
   const char *pkgdatadir;
+  int quote_seen = 0;
+  int unknown;
 
   grub_util_host_init (&argc, &argv);
   grub_util_disable_fd_syncs ();
@@ -391,7 +405,12 @@ main (int argc, char *argv[])
   xorriso = xstrdup ("xorriso");
   label_font = grub_util_path_concat (2, pkgdatadir, "unicode.pf2");
 
-  argp_parse (&argp, argc, argv, 0, 0, 0);
+  while (argc > 0 && argp_parse (&argp, argc, argv, ARGP_NO_ERRS, &unknown, &quote_seen))
+    {
+      xorriso_push_tail (argv[unknown]);
+      argv += unknown;
+      argc -= unknown;
+    }
 
   if (!output_image)
     grub_util_error ("%s", _("output file must be specified"));
-- 
tg: (77063f4..) u/grub-mkrescue-options (depends on: master)

Re: About the CLI of both grub-mkrescue versions

[Thomas Schmitt]

Hi,

> Well, below is much more simple patch which is reusing argp
> infrastructure. The only problem - it needs small patch for gnulib
> otherwise --help does not work. IMHO this is a bug in gnulib. If you
> convince them to fix it ...

Me ? I cannot even convince you or Vladimir :))

I understand that you propose a single interpretation mode
which shall be compatible with grub-mkrescue(.in) from
GRUB 2.00.
In the first thread about this topic, the wish was issued to
make the ISO producer behave like other GRUB tools.
Does any of them use ARGP_NO_ERRS ?

My proposal tries to keep old scripts runnable, while not
hampering the transition to the common interpretation style.
It also complains about now unsupported options of grub-mkrescue(.in),
wheras your poposal forwards them to xorriso. (If i get it right.)


> I do not say that I particular like it, but looks better than
> reimplement parser from scratch.

At least it addresses my concerns about compatibility for
scripts.

If it gets implemented, then it should catch old options
  -v
  --version
  --modules
  --modules= 
  --override-directory
  --override-directory=

My proposal for -vi and --version was:
        {
          printf ("%s %s %s\n", argv[0], PACKAGE_NAME, PACKAGE_VERSION);
          exit (0);
        }
The others just issue an error message and exit, as i do not
know whether they would make sense in grub-mkrescue.c.


Have a nice day :)

Thomas

Re: About the CLI of both grub-mkrescue versions

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 9104 bytes --]

On 01.10.2014 10:25, Thomas Schmitt wrote:
> Hi,
> 
> to substantiate my proposal of renaming young grub-mkrescue.c to
> grub-mkiso.c and to add a built-in emulation of grub-mkrescue(.in),
> here the necessary code which i tested standalone with valgrind.
> 
> The decision which parser to use would be made in main():
> 
I think that old parser is better. The only reason the change happened
is that it's a bug that sneaked in during migration to C. It should be
fixed.
> --------------------------------------------------------------
> 
>   char *cpt;
>   ...
>   /* Get leaf name of argv[0] */
>   for (cpt = argv[0] + strlen (argv[0]) - 1; cpt >= argv[0]; cpt--)
>     if (*cpt == '/')
>       break;
>   cpt++;
>   if (strcmp (cpt, "grub-mkrescue") == 0)
>     {
>       arg_parser_mkrescue (argc, argv);
>     }
>   else
>     {
>       argp_parse (&argp, argc, argv, 0, 0, 0);
>     }
> 
> --------------------------------------------------------------
> 
> The help text is derived from grub-mkrescue.in of GRUB 2.00.
> 
> --------------------------------------------------------------
> 
> static void
> printc (char *line)
> {
>   printf ("%s\n", line);
> }
> 
> static void
> print_mkrescue_help (char *prog_name)
> {
>   printf ("%s %s %s\n", _("Usage:"), prog_name, _("[OPTION] SOURCE..."));
>   printc (_("Make GRUB CD-ROM, disk, pendrive and floppy bootable image."));
>   printc (_("-h, --help"));
>   printc (_("        print this message and exit"));
>   printc (_("-v, --version"));
>   printc (_("        print the version information and exit"));
>   printc (_("-o, --output=FILE"));
>   printc (_("        save output in FILE [required]"));
>   printc (_("--rom-directory=DIR"));
>   printc (_("        save ROM images in DIR [optional]"));
>   printc (_("--xorriso=FILE"));
>   printc (_("        use FILE as xorriso [optional]"));
>   printc (_("Not supported any more are:"));
>   printc (_("        --modules , --grub-mkimage , --override-directory"));
>   printc (_("Other arguments get forwarded to xorriso -as mkisofs"));
>   printc (_("emulation."));
>   printc ("");
>   printf ("%s %s\n", prog_name, _("generates a bootable rescue image"));
>   printc (_("with specified source files, source directories, or mkisofs"));
>   printc (_("options listed by the output of `xorriso -as mkisofs -help'"));
>   printc ("");
>   printc (_("Note: Do not use option \"--\" unless you want to submit"));
>   printc (_("      native xorriso commands instead of file paths or"));
>   printc (_("      mkisofs options. See man xorrisofs and man xorriso."));
>   printc ("");
>   printc (_("Report bugs to <bug-grub@gnu.org>."));
>   printc (_("Mail xorriso support requests to <bug-xorriso@gnu.org>."));
>   printc ("");
>   printc (_("This is program grub-mkiso emulating the option"));
>   printc (_("interpretation of legacy program grub-mkrescue."));
>   printc (_("grub-mkiso in its native mode has more advanced options."));
>   printc (_("But that mode demands to separate grub-mkiso options"));
>   printc (_("and xorriso options by a double dash \"--\", which xorriso"));
>   printc (_("will not get to see."));
> }
> 
> --------------------------------------------------------------
> 
> The parser function implements the promised options and collects
> the xorriso -as mkisofs options into the same char pointer array
> as does the existing parser in the C program.
> In particular it sets the values of these variables:
> 
> static char *rom_directory;
> static int xorriso_tail_argc;
> static int xorriso_tail_arg_alloc;
> static char **xorriso_tail_argv;
> static char *output_image;
> static char *xorriso;
> 
> --------------------------------------------------------------
> 
> static void
> arg_parser_mkrescue (int argc, char *argv[])
> {
>   int i;
> 
>   for (i = 1; i < argc; i++)
>     {
>       if (strcmp (argv[i], "-h") == 0 || strcmp (argv[i], "--help") == 0)
>         {
>           print_mkrescue_help (argv[0]);
>           exit (0);
>         }
>       else if (strcmp (argv[i], "-v") == 0
>                || strcmp (argv[i], "--version") == 0)
>         {
>           printf ("%s %s %s\n", argv[0], PACKAGE_NAME, PACKAGE_VERSION);
>           exit (0);
>         }
>       else if (strcmp (argv[i], "--modules") == 0)
>         {
>           grub_util_error (_("Option --modules is not supported any more"));
>         }
>       else if (strncmp (argv[i], "--modules=", 10) == 0)
>         {
>           grub_util_error (_("Option --modules= is not supported any more"));
>         }
>       else if (strcmp (argv[i], "-o") == 0
>                || strcmp (argv[i], "--output") == 0)
>         {
>           if (i == argc - 1)
>             grub_util_error (_("option requires an argument -- `%s'"),
>                              argv[i]);
>           i++;
>           free (output_image);
>           output_image = xstrdup (argv[i]);
>         }
>       else if (strncmp (argv[i], "--output=", 9) == 0)
>         {
>           free (output_image);
>           output_image = xstrdup (argv[i] + 9);
>         }
>       else if (strcmp (argv[i], "--rom-directory") == 0)
>         {
>           if (i == argc - 1)
>             grub_util_error (_("option requires an argument -- `%s'"),
>                              argv[i]);
>           i++;
>           free (rom_directory);
>           rom_directory = xstrdup (argv[i]);
>         }
>       else if (strncmp (argv[i], "--rom-directory=", 16) == 0)
>         {
>           free (rom_directory);
>           rom_directory = xstrdup (argv[i] + 16);
>         }
>       else if (strcmp (argv[i], "--override-directory") == 0)
>         {
>           grub_util_error(
>                    _("Option --override-directory is not supported any more"));
>         }
>       else if (strncmp (argv[i], "--override-directory=", 21) == 0)
>         {
>           grub_util_error(
>                   _("Option --override-directory= is not supported any more"));
>         }
>       else if (strcmp (argv[i], "--xorriso") == 0)
>         {
>           if (i == argc - 1)
>             grub_util_error (_("option requires an argument -- `%s'"),
>                              argv[i]);
>           i++;
>           free (xorriso);
>           xorriso = xstrdup (argv[i]);
>         }
>       else if (strncmp (argv[i], "--xorriso=", 10) == 0)
>         {
>           free (xorriso);
>           xorriso = xstrdup (argv[i] + 10);
>         }
>       else
>         {
>           if (xorriso_tail_arg_alloc <= xorriso_tail_argc)
>             {
>               xorriso_tail_arg_alloc = 2 * (4 + xorriso_tail_argc);
>               xorriso_tail_argv = xrealloc (xorriso_tail_argv,
>                                       sizeof (xorriso_tail_argv[0])
>                                       * xorriso_tail_arg_alloc);
>             }
>           xorriso_tail_argv[xorriso_tail_argc++] = xstrdup (argv[i]);
>         }
>     }
> }
> 
> --------------------------------------------------------------
> 
> There remains the incompatibility that i could not find
> equivalents of the following grub-mkrescue.in features:
> 
>   --modules=MODULES
>           pre-load specified modules MODULES
>   --grub-mkimage=FILE
>           use FILE as grub-mkimage
>   --override-directory
>           "Intentionally undocumented"
> 
> --------------------------------------------------------------
> 
> The valgrind test was made with a mock-up of grub-mkiso.c
> consisting of some copied util/grub-*.c functions and a main()
> which prints the content of the variables after parsing.
> 
>   ln -s grub-mkiso grub-mkrescue
>   valgrind ./grub-mkrescue -o output.iso -J --rom-directory=./ROMDIR -P YET_ANOTHER_OS.ORG --md5 --emul-toc ./my_os_payload --xorriso="$HOME"/xorriso-1.3.8/xorriso/xorriso
> 
> yielded:
> 
>   output_image=           'output.iso'
>   rom_directory=          './ROMDIR'
>   xorriso=                '/home/thomas/xorriso-1.3.8/xorriso/xorriso'
>   xorriso_tail_arg_alloc= 8
>   xorriso_tail_argc=      6
>   xorriso_tail_argv[ 0]= '-J'
>   xorriso_tail_argv[ 1]= '-P'
>   xorriso_tail_argv[ 2]= 'YET_ANOTHER_OS.ORG'
>   xorriso_tail_argv[ 3]= '--md5'
>   xorriso_tail_argv[ 4]= '--emul-toc'
>   xorriso_tail_argv[ 5]= './my_os_payload'
>   ==9810== 
>   ==9810== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 2 from 1)
>   ...
>   ==9810== LEAK SUMMARY:
>   ==9810==    definitely lost: 0 bytes in 0 blocks.
>   ==9810==      possibly lost: 0 bytes in 0 blocks.
>   ==9810==    still reachable: 185 bytes in 10 blocks.
>   ==9810==         suppressed: 0 bytes in 0 blocks.
> 
> The memory leaks are caused by allocated storage of global
> variables.
> 
> 
> Have a nice day :)
> 
> Thomas
> 
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 213 bytes --]

Re: About the CLI of both grub-mkrescue versions

[Andrei Borzenkov]

[-- Attachment #1: Type: text/plain, Size: 922 bytes --]

В Fri, 28 Nov 2014 21:41:03 +0200
Vladimir 'φ-coder/phcoder' Serbinenko <phcoder@gmail.com> пишет:

> On 01.10.2014 10:25, Thomas Schmitt wrote:
> > Hi,
> > 
> > to substantiate my proposal of renaming young grub-mkrescue.c to
> > grub-mkiso.c and to add a built-in emulation of grub-mkrescue(.in),
> > here the necessary code which i tested standalone with valgrind.
> > 
> > The decision which parser to use would be made in main():
> > 
> I think that old parser is better. The only reason the change happened
> is that it's a bug that sneaked in during migration to C. It should be
> fixed.

It is near to impossible to emulate old behavior using argp, at least
without some intrusive patch. Nor do I actually like "pass anything you
do not understand" - it has potential to break if grub-mkrescue gets
new options. 

Anyway - if it is OK to carry local patch to argp I try to revisit it.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: About the CLI of both grub-mkrescue versions

[Thomas Schmitt]

Hi,

Vladimir Serbinenko:
> I think that old parser is better. The only reason the change happened
> is that it's a bug that sneaked in during migration to C. It should be
> fixed.

Andrei Borzenkov:
> [...] Nor do I actually like "pass anything you
> do not understand" - it has potential to break if grub-mkrescue gets
> new options. 

I agree that it is a bug to make incompatible changes to the
grub-mkirescue CLI. But i also agree that this CLI is sub-optimal
in respect to future compatibility. It's just that i do not deem
sub-optimality a valid excuse for breaking interfaces.

Therefore my proposal to freeze grub-mkrescue CLI and to have
a new CLI with a new program name, while maintaining a common
backend for both CLI parsers.


> It is near to impossible to emulate old behavior using argp

What would be the advantage of using argp for the old CLI ?
The C version of the old CLI has the goal to be as compatible
as possible. It is straightforward to translate the shell code
of grub-mkrescue.in into equivalent C code.

-------------------------------------------------------------

Regardless which parser implementation gets chosen, there remains
the problem of the not yet implemented options from grub-mkrescue.in:

> >   printc (_("Not supported any more are:"));
> >   printc (_("        --modules , --grub-mkimage , --override-directory"));

Are their use cases obsolete ?
Shall their valid use cases be covered by other tools ?


Have a nice day :)

Thomas