[PATCH] Making use of argp in GRUB utilities

[PATCH] Making use of argp in GRUB utilities

[Lubomir Kundrak]

Hi,

I've noticed, that GRUB 2 uses argp in grub-emu [1], whereas other
utilities use getopt_long [2]. Wouldn't it be nice to make this
consistent?

[1] http://www.gnu.org/software/libc/manual/html_node/Argp.html
[2]
http://www.gnu.org/software/libc/manual/html_node/Getopt-Long-Options.html

I find the "GNU way," argp, approach more elegant, as demonstrated by a
patch to util/i386/pc/grub-mkimage.c. Seeing a patch with more minuses
than pluses is a good sign, indeed.

The main argument against agrp framework could be, that non-GNU C
libraries do not contain with it. There's a standalone libargp package
[3] that is available for package systems of all major operating
systems, including NetBSD and FreeBSD, so the only disadvantage is
addding a dependency there.

[3] http://www.lysator.liu.se/~nisse/misc/

Regards,
-- 
Lubomir Kundrak (Red Hat Security Response Team)

Re: [PATCH] Making use of argp in GRUB utilities

[Lubomir Kundrak]

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

Forgot to attache the file.
Here is it.

On Št, 2007-01-25 at 12:25 +0100, Lubomir Kundrak wrote:
> I find the "GNU way," argp, approach more elegant, as demonstrated by a
> patch to util/i386/pc/grub-mkimage.c. Seeing a patch with more minuses
> than pluses is a good sign, indeed.

Regards,
-- 
Lubomir Kundrak (Red Hat Security Response Team)

[-- Attachment #2: grub2-argp-grub-mkimage.patch --]
[-- Type: text/x-patch, Size: 4463 bytes --]

Index: util/i386/pc/grub-mkimage.c
===================================================================
RCS file: /sources/grub/grub2/util/i386/pc/grub-mkimage.c,v
retrieving revision 1.11
diff -u -p -r1.11 grub-mkimage.c
--- util/i386/pc/grub-mkimage.c	25 Nov 2006 03:21:29 -0000	1.11
+++ util/i386/pc/grub-mkimage.c	25 Jan 2007 11:18:41 -0000
@@ -1,7 +1,7 @@
 /* grub-mkimage.c - make a bootable image */
 /*
  *  GRUB  --  GRand Unified Bootloader
- *  Copyright (C) 2002,2003,2004,2005,2006  Free Software Foundation, Inc.
+ *  Copyright (C) 2002,2003,2004,2005,2006,2007  Free Software Foundation, Inc.
  *
  *  GRUB is free software; you can redistribute it and/or modify
  *  it under the terms of the GNU General Public License as published by
@@ -33,7 +33,7 @@
 #include <stdlib.h>
 
 #define _GNU_SOURCE	1
-#include <getopt.h>
+#include <argp.h>
 
 #if defined(HAVE_LZO_LZO1X_H)
 # include <lzo/lzo1x.h>
@@ -179,102 +179,79 @@ generate_image (const char *dir, FILE *o
 
 \f
 
-static struct option options[] =
-  {
-    {"directory", required_argument, 0, 'd'},
-    {"output", required_argument, 0, 'o'},
-    {"help", no_argument, 0, 'h'},
-    {"version", no_argument, 0, 'V'},
-    {"verbose", no_argument, 0, 'v'},
-    {0, 0, 0, 0}
-  };
+const char *argp_program_version = PACKAGE_STRING;
+const char *argp_program_bug_address = PACKAGE_BUGREPORT;
+static char doc[] = "Make a bootable image of GRUB";
+
+static struct argp_option options[] = { 
+  {"directory", 'd', "DIR",  0, "use images and modules under DIR [default="GRUB_DATADIR"]", 0},
+  {"output",    'o', "FILE", 0, "output a generated image to FILE [default=stdout]", 0},
+  {"verbose",   'v', 0,      0, "print verbose messages", 0},
+  { 0, 0, 0, 0, 0, 0 }
+};
 
-static void
-usage (int status)
+struct arguments
 {
-  if (status)
-    fprintf (stderr, "Try ``grub-mkimage --help'' for more information.\n");
-  else
-    printf ("\
-Usage: grub-mkimage [OPTION]... [MODULES]\n\
-\n\
-Make a bootable image of GRUB.\n\
-\n\
-  -d, --directory=DIR     use images and modules under DIR [default=%s]\n\
-  -o, --output=FILE       output a generated image to FILE [default=stdout]\n\
-  -h, --help              display this message and exit\n\
-  -V, --version           print version information and exit\n\
-  -v, --verbose           print verbose messages\n\
-\n\
-Report bugs to <%s>.\n\
-", GRUB_LIBDIR, PACKAGE_BUGREPORT);
+  char *dir;
+  char *output;
+  char **modules;
+};
 
-  exit (status);
-}
+static error_t
+parse_opt (int key, char *arg, struct argp_state *state)
+{
+  struct arguments *args = state->input;
+  
+  switch (key)
+    {
+    case 'd':
+      args->dir = arg;
+      break;
+    case 'o':
+      args->output = arg;
+      break; 
+    case 'v':
+      verbosity++;
+      break;
+    case ARGP_KEY_ARGS:
+      args->modules = state->argv + state->next;
+      break;
+    case ARGP_KEY_END:
+      if (! args->modules)
+        { args->modules = state->argv + state->next; }
+      break;
+    default:
+      return ARGP_ERR_UNKNOWN;
+    }
+  return 0;
+} 
+
+static struct argp argp = {options, parse_opt, "[MODULE...]", doc, 0, 0, 0};
+\f
 
 int
 main (int argc, char *argv[])
 {
-  char *output = 0;
-  char *dir = 0;
   FILE *fp = stdout;
+  struct arguments args =
+    {
+      .dir = GRUB_DATADIR,
+      .output = 0
+    };
 
   progname = "grub-mkimage";
-  
-  while (1)
-    {
-      int c = getopt_long (argc, argv, "d:o:hVv", options, 0);
 
-      if (c == -1)
-	break;
-      else
-	switch (c)
-	  {
-	  case 'o':
-	    if (output)
-	      free (output);
-	    
-	    output = xstrdup (optarg);
-	    break;
-
-	  case 'd':
-	    if (dir)
-	      free (dir);
-
-	    dir = xstrdup (optarg);
-	    break;
-
-	  case 'h':
-	    usage (0);
-	    break;
-
-	  case 'V':
-	    printf ("grub-mkimage (%s) %s\n", PACKAGE_NAME, PACKAGE_VERSION);
-	    return 0;
-
-	  case 'v':
-	    verbosity++;
-	    break;
-
-	  default:
-	    usage (1);
-	    break;
-	  }
-    }
+  argp_parse (&argp, argc, argv, 0, 0, &args);
 
-  if (output)
+  if (args.output)
     {
-      fp = fopen (output, "wb");
+      fp = fopen (args.output, "wb");
       if (! fp)
-	grub_util_error ("cannot open %s", output);
+	grub_util_error ("cannot open %s", args.output);
     }
 
-  generate_image (dir ? : GRUB_LIBDIR, fp, argv + optind);
+  generate_image (args.dir ? : GRUB_LIBDIR, fp, args.modules);
 
   fclose (fp);
-
-  if (dir)
-    free (dir);
-
   return 0;
 }

Re: [PATCH] Making use of argp in GRUB utilities

[Yoshinori K. Okuji]

On Thursday 25 January 2007 12:25, Lubomir Kundrak wrote:
> I've noticed, that GRUB 2 uses argp in grub-emu [1], whereas other
> utilities use getopt_long [2]. Wouldn't it be nice to make this
> consistent?

Maybe. 

> I find the "GNU way," argp, approach more elegant, as demonstrated by a
> patch to util/i386/pc/grub-mkimage.c. Seeing a patch with more minuses
> than pluses is a good sign, indeed.
>
> The main argument against agrp framework could be, that non-GNU C
> libraries do not contain with it. There's a standalone libargp package
> [3] that is available for package systems of all major operating
> systems, including NetBSD and FreeBSD, so the only disadvantage is
> addding a dependency there.

I preferred that libargp would be included in our source tree so that it would 
be used when argp is not found in a system, but I guess Marco hasn't done it 
yet. This depends on which is more convenient for users, using an external 
shared library, or using our own. In GRUB Legacy, I included getopt for *BSD, 
and I got positive answers. So I feel that it would be better to include.

Okuji

Re: [PATCH] Making use of argp in GRUB utilities

[Lubomir Kundrak]

On Pi, 2007-01-26 at 12:58 +0100, Yoshinori K. Okuji wrote:
> On Thursday 25 January 2007 12:25, Lubomir Kundrak wrote:
> > I've noticed, that GRUB 2 uses argp in grub-emu [1], whereas other
> > utilities use getopt_long [2]. Wouldn't it be nice to make this
> > consistent?
> 
> Maybe. 
> 
> > I find the "GNU way," argp, approach more elegant, as demonstrated by a
> > patch to util/i386/pc/grub-mkimage.c. Seeing a patch with more minuses
> > than pluses is a good sign, indeed.
> >
> > The main argument against agrp framework could be, that non-GNU C
> > libraries do not contain with it. There's a standalone libargp package
> > [3] that is available for package systems of all major operating
> > systems, including NetBSD and FreeBSD, so the only disadvantage is
> > addding a dependency there.
> 
> I preferred that libargp would be included in our source tree so that it would 
> be used when argp is not found in a system, but I guess Marco hasn't done it 
> yet. This depends on which is more convenient for users, using an external 
> shared library, or using our own. In GRUB Legacy, I included getopt for *BSD, 
> and I got positive answers. So I feel that it would be better to include.

Sound reasonable. Would it be of any use if I converted other utils to
make use of argp in my spare time?

> 
> Okuji

-- 
Lubomir Kundrak (Red Hat Security Response Team)

Re: [PATCH] Making use of argp in GRUB utilities

[Marco Gerards]

"Yoshinori K. Okuji" <okuji@enbug.org> writes:

Hi,

> On Thursday 25 January 2007 12:25, Lubomir Kundrak wrote:
>> I've noticed, that GRUB 2 uses argp in grub-emu [1], whereas other
>> utilities use getopt_long [2]. Wouldn't it be nice to make this
>> consistent?
>
> Maybe. 
>
>> I find the "GNU way," argp, approach more elegant, as demonstrated by a
>> patch to util/i386/pc/grub-mkimage.c. Seeing a patch with more minuses
>> than pluses is a good sign, indeed.
>>
>> The main argument against agrp framework could be, that non-GNU C
>> libraries do not contain with it. There's a standalone libargp package
>> [3] that is available for package systems of all major operating
>> systems, including NetBSD and FreeBSD, so the only disadvantage is
>> addding a dependency there.
>
> I preferred that libargp would be included in our source tree so that it would 
> be used when argp is not found in a system, but I guess Marco hasn't done it 
> yet. This depends on which is more convenient for users, using an external 
> shared library, or using our own. In GRUB Legacy, I included getopt for *BSD, 
> and I got positive answers. So I feel that it would be better to include.

There is the library you mentioned and the argp implementation from
gnulib.  I personally prefer gnulib, because we don't have to worry
about copyright at all in this case.  But I agree, including it so
things work on BSD is prefered.

I assume you noticed grub-emu isn't compiled by default.  It makes the
situation a bit easier when using BSD.  Although for a good port we
need some argp implementation on BSD.

--
Marco

Re: [PATCH] Making use of argp in GRUB utilities

[Marco Gerards]

Lubomir Kundrak <lkundrak@redhat.com> writes:

>> I preferred that libargp would be included in our source tree so that it would 
>> be used when argp is not found in a system, but I guess Marco hasn't done it 
>> yet. This depends on which is more convenient for users, using an external 
>> shared library, or using our own. In GRUB Legacy, I included getopt for *BSD, 
>> and I got positive answers. So I feel that it would be better to include.
>
> Sound reasonable. Would it be of any use if I converted other utils to
> make use of argp in my spare time?

It seems either that Okuji doesn't care or doesn't know yet.  I
personally prefer argp because it is easy to use, flexible and less
work to maintain.  And like you said, less code... less code normally
means less bugs.

So it depends on Okuji what to do.  If he doesn't care I would be
happy with such a patch.

--
Marco

Re: [PATCH] Making use of argp in GRUB utilities

[Yoshinori K. Okuji]

On Friday 26 January 2007 17:01, Marco Gerards wrote:
> There is the library you mentioned and the argp implementation from
> gnulib.  I personally prefer gnulib, because we don't have to worry
> about copyright at all in this case.  But I agree, including it so
> things work on BSD is prefered.

Yes, it is a good idea to steal code from gnulib. Gnulib exists for the very 
purpose.

Okuji

Re: [PATCH] Making use of argp in GRUB utilities

[Yoshinori K. Okuji]

On Friday 26 January 2007 17:03, Marco Gerards wrote:
> Lubomir Kundrak <lkundrak@redhat.com> writes:
> >> I preferred that libargp would be included in our source tree so that it
> >> would be used when argp is not found in a system, but I guess Marco
> >> hasn't done it yet. This depends on which is more convenient for users,
> >> using an external shared library, or using our own. In GRUB Legacy, I
> >> included getopt for *BSD, and I got positive answers. So I feel that it
> >> would be better to include.
> >
> > Sound reasonable. Would it be of any use if I converted other utils to
> > make use of argp in my spare time?
>
> It seems either that Okuji doesn't care or doesn't know yet.  I
> personally prefer argp because it is easy to use, flexible and less
> work to maintain.  And like you said, less code... less code normally
> means less bugs.
>
> So it depends on Okuji what to do.  If he doesn't care I would be
> happy with such a patch.

I don't know. I agree that using a single library for the same goal is more 
consistent. However, I am so used to getopt, and I have no problem with it. 
Thus I am not eager to migrate to argp in particular, although I do not 
object to using argp.

(In my whole life, I have used argp only once for Hurd.)

Okuji