Skip to contentSkip to conversation

Use getopt_long() instead of argp_parse() in grub-emu

PATCH In review Christian Franke 3 participants 9 messages Nov 10, 2007 – Jan 15, 2008

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 Use getopt_long() instead of argp_parse() in grub-emu

Unlike the other GRUB2 utils, grub-emu uses the glibc extension argp_parse(). It is unavailable on Cygwin, which might also be the case for other platforms where glibc is not the native runtime.

This patch changes this back to the more traditional getopt_long().

It also fixes the syntax of the path prefix.

Christian

2007-11-10 Christian Franke <[email protected]>

* util/grub-emu.c: Replace argp.h by getopt.h. (parse_opt): Remove.
(usage): New function.
(main): Replace argp_parse() by getopt_long(). Rename argument variables accordingly. Add missing "(...)" for root_dev in prefix.

util/grub-emu.c +86 -75
@@ -18,7 +18,7 @@
 
 #include <stdlib.h>
 #include <sys/stat.h>
-#include <argp.h>
+#include <getopt.h>
 #include <string.h>
 #include <signal.h>
 #include <sys/types.h>
@@ -90,104 +90,115 @@ grub_machine_fini (void)
 }
 
 
-const char *argp_program_version = PACKAGE_STRING;
-const char *argp_program_bug_address = PACKAGE_BUGREPORT;
-static char doc[] = "GRUB emulator";
-
-static struct argp_option options[] = {
-  {"root-device", 'r', "DEV",  0, "use DEV as the root device [default=guessed]", 0},
-  {"device-map",  'm', "FILE", 0, "use FILE as the device map", 0},
-  {"directory",   'd', "DIR",  0, "use GRUB files in the directory DIR", 0},
-  {"verbose",     'v', 0     , 0, "print verbose messages", 0},
-  {"hold",        'H', "SECONDS", OPTION_ARG_OPTIONAL, "wait until a debugger will attach", 0},
-  { 0, 0, 0, 0, 0, 0 }
-};
-
-struct arguments
-{
-  char *root_dev;
-  char *dev_map;
-  char *dir;
-  int hold;
-};
-
-static error_t
-parse_opt (int key, char *arg, struct argp_state *state)
-{
-  struct arguments *args = state->input;
-  
-  switch (key)
-    {
-    case 'r':
-      args->root_dev = arg;
-      break;
-    case 'd':
-      args->dir = arg;
-      break;
-    case 'm':
-      args->dev_map = arg;
-      break;
-    case 'v':
-      verbosity++;
-      break;
-    case 'H':
-      args->hold = arg ? atoi (arg) : -1;
-      break;
-    case ARGP_KEY_END:
-      break;
-    default:
-      return ARGP_ERR_UNKNOWN;
-    }
-  return 0;
+static struct option options[] =
+  {
+    {"root-device", required_argument, 0, 'r'},
+    {"device-map", required_argument, 0, 'm'},
+    {"directory", required_argument, 0, 'd'},
+    {"hold", optional_argument, 0, 'H'},
+    {"help", no_argument, 0, 'h'},
+    {"version", no_argument, 0, 'V'},
+    {"verbose", no_argument, 0, 'v'},
+    { 0, 0, 0, 0 }
+  };
+
+static int 
+usage (int status)
+{
+  if (status)
+    fprintf (stderr,
+	     "Try ``grub-emu --help'' for more information.\n");
+  else
+    printf (
+      "Usage: grub-emu [OPTION]...\n"
+      "\n"
+      "GRUB emulator.\n"
+      "\n"
+      "  -r, --root-device=DEV     use DEV as the root device [default=guessed]\n"
+      "  -m, --device-map=FILE     use FILE as the device map [default=%s]\n"
+      "  -d, --directory=DIR       use GRUB files in the directory DIR [default=%s]\n"
+      "  -v, --verbose             print verbose messages\n"
+      "  -H, --hold[=SECONDS]      wait until a debugger will attach\n"
+      "  -h, --help                display this message and exit\n"
+      "  -V, --version             print version information and exit\n"
+      "\n"
+      "Report bugs to <%s>.\n", DEFAULT_DEVICE_MAP, DEFAULT_DIRECTORY, PACKAGE_BUGREPORT);
+  return status;
 }
-
-static struct argp argp = {options, parse_opt, 0, doc, 0, 0, 0};
 
 
 int
 main (int argc, char *argv[])
 {
-  char *dir;
-  
-  struct arguments args =
-    {
-      .dir = DEFAULT_DIRECTORY,
-      .dev_map = DEFAULT_DEVICE_MAP,
-      .hold = 0
-    };
+  char *root_dev = 0;
+  char *dir = DEFAULT_DIRECTORY;
+  char *dev_map = DEFAULT_DEVICE_MAP;
+  volatile int hold = 0;
+  int opt;
   
   progname = "grub-emu";
-  
-  argp_parse (&argp, argc, argv, 0, 0, &args);
+
+  while ((opt = getopt_long (argc, argv, "r:d:m:vH:hV", options, 0)) != -1)
+    switch (opt)
+      {
+      case 'r':
+        root_dev = optarg;
+        break;
+      case 'd':
+        dir = optarg;
+        break;
+      case 'm':
+        dev_map = optarg;
+        break;
+      case 'v':
+        verbosity++;
+        break;
+      case 'H':
+        hold = (optarg ? atoi (optarg) : -1);
+        break;
+      case 'h':
+        return usage (0);
+      case 'V':
+        printf ("%s (%s) %s\n", progname, PACKAGE_NAME, PACKAGE_VERSION);
+        return 0;
+      default:
+        return usage (1);
+      }
+
+  if (optind < argc)
+    {
+      fprintf (stderr, "Unknown extra argument `%s'.\n", argv[optind]);
+      return usage (1);
+    }
 
   /* Wait until the ARGS.HOLD variable is cleared by an attached debugger. */
-  if (args.hold && verbosity > 0)
+  if (hold && verbosity > 0)
     printf ("Run \"gdb %s %d\", and set ARGS.HOLD to zero.\n",
             progname, (int) getpid ());
-  while (args.hold)
+  while (hold)
     {
-      if (args.hold > 0)
-        args.hold--;
+      if (hold > 0)
+        hold--;
 
       sleep (1);
     }
   
   /* XXX: This is a bit unportable.  */
-  grub_util_biosdisk_init (args.dev_map);
+  grub_util_biosdisk_init (dev_map);
 
   grub_hostfs_init ();
 
   grub_init_all ();
 
   /* Make sure that there is a root device.  */
-  if (! args.root_dev)
+  if (! root_dev)
     {
-      char *device_name = grub_guess_root_device (args.dir ? : DEFAULT_DIRECTORY);
+      char *device_name = grub_guess_root_device (dir);
       if (! device_name)
-        grub_util_error ("cannot find a device for %s.\n", args.dir ? : DEFAULT_DIRECTORY);
+        grub_util_error ("cannot find a device for %s.\n", dir);
 
-      args.root_dev = grub_util_get_grub_dev (device_name);
-      if (! args.root_dev)
+      root_dev = grub_util_get_grub_dev (device_name);
+      if (! root_dev)
 	{
 	  grub_util_info ("guessing the root device failed, because of `%s'",
 			  grub_errmsg);
@@ -195,9 +206,9 @@ main (int argc, char *argv[])
 	}
     }
 
-  dir = grub_get_prefix (args.dir ? : DEFAULT_DIRECTORY);
-  prefix = xmalloc (strlen (args.root_dev) + strlen (dir) + 1);
-  sprintf (prefix, "%s%s", args.root_dev, dir);
+  dir = grub_get_prefix (dir);
+  prefix = xmalloc (strlen (root_dev) + 2 + strlen (dir) + 1);
+  sprintf (prefix, "(%s)%s", root_dev, dir);
   free (dir);
   
   /* Start GRUB!  */
#1 lore raw
Quoted 3 lines · Christian Franke

Unlike the other GRUB2 utils, grub-emu uses the glibc extension argp_parse(). It is unavailable on Cygwin, which might also be the case for other platforms where glibc is not the native runtime.

This has been brought up before, BSD has the same problem. The outcome of the discussion was (IIRC) that we will use argp. When argp is not available we can use gnulib or a standalone argp parser ("argp-standalone") to support this. In that case it will mean changing configure.ac.

--
Marco

#2 lore raw
Quoted 8 lines · Marco Gerards
Quoted 4 lines · Christian Franke

Unlike the other GRUB2 utils, grub-emu uses the glibc extension argp_parse(). It is unavailable on Cygwin, which might also be the case for other platforms where glibc is not the native runtime.

Quoted 7 lines

This has been brought up before, BSD has the same problem. The outcome of the discussion was (IIRC) that we will use argp. When argp is not available we can use gnulib or a standalone argp parser ("argp-standalone") to support this. In that case it will mean changing configure.ac.

Will argp_parse() be a pre-requisite for building GRUB2 or will argp-standalone be included (some other projects do) ?

If you really want argp, why is it used only for the few trivial options of grub-emu ? The other utils still use getopt_long(). grub-emu does not benefit much from argp, but introduces another portability issue.

Christian

#3 lore raw
Quoted 16 lines · Christian Franke
Quoted 8 lines · Marco Gerards
Quoted 4 lines · Christian Franke

Unlike the other GRUB2 utils, grub-emu uses the glibc extension argp_parse(). It is unavailable on Cygwin, which might also be the case for other platforms where glibc is not the native runtime.

Quoted 6 lines

This has been brought up before, BSD has the same problem. The outcome of the discussion was (IIRC) that we will use argp. When argp is not available we can use gnulib or a standalone argp parser ("argp-standalone") to support this. In that case it will mean changing configure.ac.

Quoted 2 lines

Will argp_parse() be a pre-requisite for building GRUB2 or will argp-standalone be included (some other projects do) ?

Personally, I would prefer it if it could be used as an external library. The more we include, the more we need to care about.

Quoted 4 lines

If you really want argp, why is it used only for the few trivial options of grub-emu ? The other utils still use getopt_long(). grub-emu does not benefit much from argp, but introduces another portability issue.

It's what happened :-)

Perhaps we should consider getopt_long, for now. It's one file to change... Does anyone else object? Otherwise I'll review this patch.

--
Marco

#4 lore raw
Quoted 13 lines · Marco Gerards
Quoted 5 lines · Christian Franke
Quoted 2 lines

...

Will argp_parse() be a pre-requisite for building GRUB2 or will argp-standalone be included (some other projects do) ?

Personally, I would prefer it if it could be used as an external library. The more we include, the more we need to care about.

Yes, but then future package maintainers will have fewer portability issues to care about :-)

Quoted 6 lines
Quoted 5 lines

If you really want argp, why is it used only for the few trivial options of grub-emu ? The other utils still use getopt_long(). grub-emu does not benefit much from argp, but introduces another portability issue.

Quoted 6 lines

It's what happened :-)

Perhaps we should consider getopt_long, for now. It's one file to change... Does anyone else object? Otherwise I'll review this patch.

I would appreciate this.

Christian

#5 lore raw
In reply to Christian Franke
Quoted 7 lines · Christian Franke

Unlike the other GRUB2 utils, grub-emu uses the glibc extension argp_parse(). It is unavailable on Cygwin, which might also be the case for other platforms where glibc is not the native runtime.

This patch changes this back to the more traditional getopt_long().

It also fixes the syntax of the path prefix.

There where no objections, so I'll review this patch as promised.

How did you change the path syntax?

Quoted 3 lines

2007-11-10 Christian Franke <[email protected]>

* util/grub-emu.c: Replace argp.h by getopt.h.

Please use <argp.h> and <getopt.h> to show where it comes from.

Quoted 1 line

(parse_opt): Remove.

You also modified options, removed struct arguments, argp and perhaps other things I missed. You should document them properly in the changelog entry.

Quoted 4 lines

(usage): New function.
(main): Replace argp_parse() by getopt_long(). Rename argument variables accordingly. Add missing "(...)" for root_dev in prefix.

--
Marco

#6 lore raw
Quoted 7 lines · Marco Gerards
Quoted 4 lines · Christian Franke

Unlike the other GRUB2 utils, grub-emu uses the glibc extension argp_parse(). It is unavailable on Cygwin, which might also be the case for other platforms where glibc is not the native runtime.

Quoted 8 lines
Quoted 4 lines

This patch changes this back to the more traditional getopt_long().

It also fixes the syntax of the path prefix.

There where no objections, so I'll review this patch as promised.

Thanks.

Quoted 3 lines

How did you change the path syntax?

grub_util_get_grub_dev () returns e.g. "hd0,1", the resulting prefix is "hd0,1/boot/grub" but should be "(hd0,1)/boot/grub".

Quoted 8 lines
Quoted 4 lines

2007-11-10 Christian Franke <[email protected]>

* util/grub-emu.c: Replace argp.h by getopt.h.

Please use <argp.h> and <getopt.h> to show where it comes from.

Quoted 8 lines
Quoted 2 lines

(parse_opt): Remove.

You also modified options, removed struct arguments, argp and perhaps other things I missed. You should document them properly in the changelog entry.

OK.

Christian

2008-01-14 Christian Franke <[email protected]>

* util/grub-emu.c: Replace <argp.h> by <getopt.h>. (argp_program_version): Remove variable. (argp_program_bug_address): Likewise.
(options): Convert from struct argp_option to struct option. (struct arguments): Remove.
(parse_opt): Remove.
(usage): New function.
(main): Replace struct args members by simple variables. Replace argp_parse() by getopt_long().
Add switch to evaluate options.
Add missing "(...)" around root_dev in prefix string.

#7 lore raw

Christian Franke <[email protected]> writes:

[...]

Quoted 13 lines

2008-01-14 Christian Franke <[email protected]>

* util/grub-emu.c: Replace <argp.h> by <getopt.h>. (argp_program_version): Remove variable. (argp_program_bug_address): Likewise.
(options): Convert from struct argp_option to struct option. (struct arguments): Remove.
(parse_opt): Remove.
(usage): New function.
(main): Replace struct args members by simple variables. Replace argp_parse() by getopt_long().
Add switch to evaluate options.
Add missing "(...)" around root_dev in prefix string.

This can be committed. If no one does this, I will do this when I have the time to do this.

--
Marco

#8 lore raw

Done.

Quoted 32 lines · Marco Gerards

Christian Franke <[email protected]> writes:

[...]

Quoted 13 lines

2008-01-14 Christian Franke <[email protected]>

* util/grub-emu.c: Replace <argp.h> by <getopt.h>. (argp_program_version): Remove variable. (argp_program_bug_address): Likewise.
(options): Convert from struct argp_option to struct option. (struct arguments): Remove.
(parse_opt): Remove.
(usage): New function.
(main): Replace struct args members by simple variables. Replace argp_parse() by getopt_long().
Add switch to evaluate options.
Add missing "(...)" around root_dev in prefix string.

This can be committed. If no one does this, I will do this when I have the time to do this.

--
Marco

_______________________________________________
Grub-devel mailing list
[email protected]
http://lists.gnu.org/mailman/listinfo/grub-devel

Signature · 5 lines
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call, if you are unable to speak?
(as seen on /.)