Re: [Qemu-devel] [PATCH v3] Support 'help' as a synonym for '?' in command line options

[Eduardo Habkost]

On Fri, Aug 03, 2012 at 03:42:39PM -0500, Anthony Liguori wrote:
> Peter Maydell <address@hidden> writes:
> 
> > For command line options which permit '?' meaning 'please list the
> > permitted values', add support for 'help' as a synonym, by abstracting
> > the check out into a helper function.
> >
> > This change means that in some cases where we were being lazy in
> > our string parsing, "?junk" will now be rejected as an invalid option
> > rather than being (undocumentedly) treated the same way as "?".
> >
> > Update the documentation to use 'help' rather than '?', since '?'
> > is a shell metacharacter and thus prone to fail confusingly if there
> > is a single character filename in the current working directory and
> > the '?' has not been escaped. It's therefore better to steer users
> > towards 'help', though '?' is retained for backwards compatibility.
> >
> > We do not, however, update the output of the system emulator's -help
> > (or any documentation autogenerated from the qemu-options.hx which
> > is the source of the -help text) because libvirt parses our -help
> > output and will break. At a later date when QEMU provides a better
> > interface so libvirt can avoid having to do this, we can update the
> > -help text too.
> >
> > Signed-off-by: Peter Maydell <address@hidden>
> 
> Applied. Thanks.

I just found out that this patch broke "-cpu ?dump", "-cpu ?cpuid", and
"-cpu ?model":

[qemu/(c8057f9...)|BISECTING]$ ./install/bin/qemu-system-x86_64 -cpu ?dump
Unable to find x86 CPU definition
[qemu/(c8057f9...)|BISECTING]$


> 
> Regards,
> 
> Anthony Liguori
> 
> > ---
> > Changes v1->v2:
> >  * dropped the changes to -help text to avoid breaking libvirt
> > Changes v2->v3:
> >  * call out in the commit message that we've tightened up the
> >    parsing so '?junk' is now rejected rather than treated like '?'
> >  * add a TODO comment to qemu-options.hx about updating the help text
> >    when we are able to
> >  * drop a reindent of a comment we only did to placate checkpatch
> >  * add 'help' support to "-device devicename,help" (required the
> >    introduction of a new qemu_opt_has_help_opt() function)
> >  * note that '?' is deprecated in is_help_option documentation.
> >
> >  arch_init.c       |    4 ++--
> >  blockdev.c        |   10 +++++-----
> >  bsd-user/main.c   |    4 ++--
> >  hw/mips_jazz.c    |    2 +-
> >  hw/qdev-monitor.c |    4 ++--
> >  hw/watchdog.c     |    2 +-
> >  linux-user/main.c |    4 ++--
> >  net.c             |    3 ++-
> >  qemu-common.h     |   18 ++++++++++++++++++
> >  qemu-doc.texi     |    2 +-
> >  qemu-ga.c         |    2 +-
> >  qemu-img.c        |    4 ++--
> >  qemu-option.c     |   12 ++++++++++++
> >  qemu-option.h     |   12 ++++++++++++
> >  qemu-options.hx   |    4 ++++
> >  qemu-timer.c      |    2 +-
> >  vl.c              |    4 ++--
> >  17 files changed, 70 insertions(+), 23 deletions(-)
> >
> > diff --git a/arch_init.c b/arch_init.c
> > index 26f30ef..60823ba 100644
> > --- a/arch_init.c
> > +++ b/arch_init.c
> > @@ -680,7 +680,7 @@ void select_soundhw(const char *optarg)
> >  {
> >      struct soundhw *c;
> >  
> > -    if (*optarg == '?') {
> > +    if (is_help_option(optarg)) {
> >      show_valid_cards:
> >  
> >          printf("Valid sound card names (comma separated):\n");
> > @@ -688,7 +688,7 @@ void select_soundhw(const char *optarg)
> >              printf ("%-11s %s\n", c->name, c->descr);
> >          }
> >          printf("\n-soundhw all will enable all of the above\n");
> > -        exit(*optarg != '?');
> > +        exit(!is_help_option(optarg));
> >      }
> >      else {
> >          size_t l;
> > diff --git a/blockdev.c b/blockdev.c
> > index 3d75015..8669142 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -398,11 +398,11 @@ DriveInfo *drive_init(QemuOpts *opts, int 
> > default_to_scsi)
> >  #endif
> >  
> >      if ((buf = qemu_opt_get(opts, "format")) != NULL) {
> > -       if (strcmp(buf, "?") == 0) {
> > -           error_printf("Supported formats:");
> > -           bdrv_iterate_format(bdrv_format_print, NULL);
> > -           error_printf("\n");
> > -           return NULL;
> > +        if (is_help_option(buf)) {
> > +            error_printf("Supported formats:");
> > +            bdrv_iterate_format(bdrv_format_print, NULL);
> > +            error_printf("\n");
> > +            return NULL;
> >          }
> >          drv = bdrv_find_whitelisted_format(buf);
> >          if (!drv) {
> > diff --git a/bsd-user/main.c b/bsd-user/main.c
> > index cd33d65..095ae8e 100644
> > --- a/bsd-user/main.c
> > +++ b/bsd-user/main.c
> > @@ -681,7 +681,7 @@ static void usage(void)
> >             "-g port           wait gdb connection to port\n"
> >             "-L path           set the elf interpreter prefix 
> > (default=%s)\n"
> >             "-s size           set the stack size in bytes (default=%ld)\n"
> > -           "-cpu model        select CPU (-cpu ? for list)\n"
> > +           "-cpu model        select CPU (-cpu help for list)\n"
> >             "-drop-ld-preload  drop LD_PRELOAD for target process\n"
> >             "-E var=value      sets/modifies targets environment 
> > variable(s)\n"
> >             "-U var            unsets targets environment variable(s)\n"
> > @@ -825,7 +825,7 @@ int main(int argc, char **argv)
> >              qemu_uname_release = argv[optind++];
> >          } else if (!strcmp(r, "cpu")) {
> >              cpu_model = argv[optind++];
> > -            if (strcmp(cpu_model, "?") == 0) {
> > +            if (is_help_option(cpu_model)) {
> >  /* XXX: implement xxx_cpu_list for targets that still miss it */
> >  #if defined(cpu_list)
> >                      cpu_list(stdout, &fprintf);
> > diff --git a/hw/mips_jazz.c b/hw/mips_jazz.c
> > index bf1b799..db927f1 100644
> > --- a/hw/mips_jazz.c
> > +++ b/hw/mips_jazz.c
> > @@ -239,7 +239,7 @@ static void mips_jazz_init(MemoryRegion *address_space,
> >              dp83932_init(nd, 0x80001000, 2, get_system_memory(), rc4030[4],
> >                           rc4030_opaque, rc4030_dma_memory_rw);
> >              break;
> > -        } else if (strcmp(nd->model, "?") == 0) {
> > +        } else if (is_help_option(nd->model)) {
> >              fprintf(stderr, "qemu: Supported NICs: dp83932\n");
> >              exit(1);
> >          } else {
> > diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
> > index 7915b45..b22a37a 100644
> > --- a/hw/qdev-monitor.c
> > +++ b/hw/qdev-monitor.c
> > @@ -138,13 +138,13 @@ int qdev_device_help(QemuOpts *opts)
> >      ObjectClass *klass;
> >  
> >      driver = qemu_opt_get(opts, "driver");
> > -    if (driver && !strcmp(driver, "?")) {
> > +    if (driver && is_help_option(driver)) {
> >          bool show_no_user = false;
> >          object_class_foreach(qdev_print_devinfo, TYPE_DEVICE, false, 
> > &show_no_user);
> >          return 1;
> >      }
> >  
> > -    if (!driver || !qemu_opt_get(opts, "?")) {
> > +    if (!driver || !qemu_opt_has_help_opt(opts)) {
> >          return 0;
> >      }
> >  
> > diff --git a/hw/watchdog.c b/hw/watchdog.c
> > index a42124d..b52aced 100644
> > --- a/hw/watchdog.c
> > +++ b/hw/watchdog.c
> > @@ -55,7 +55,7 @@ int select_watchdog(const char *p)
> >      QemuOpts *opts;
> >  
> >      /* -watchdog ? lists available devices and exits cleanly. */
> > -    if (strcmp(p, "?") == 0) {
> > +    if (is_help_option(p)) {
> >          QLIST_FOREACH(model, &watchdog_list, entry) {
> >              fprintf(stderr, "\t%s\t%s\n",
> >                       model->wdt_name, model->wdt_description);
> > diff --git a/linux-user/main.c b/linux-user/main.c
> > index a0ab8e8..25eaa11 100644
> > --- a/linux-user/main.c
> > +++ b/linux-user/main.c
> > @@ -3140,7 +3140,7 @@ static void handle_arg_uname(const char *arg)
> >  static void handle_arg_cpu(const char *arg)
> >  {
> >      cpu_model = strdup(arg);
> > -    if (cpu_model == NULL || strcmp(cpu_model, "?") == 0) {
> > +    if (cpu_model == NULL || is_help_option(cpu_model)) {
> >          /* XXX: implement xxx_cpu_list for targets that still miss it */
> >  #if defined(cpu_list_id)
> >          cpu_list_id(stdout, &fprintf, "");
> > @@ -3231,7 +3231,7 @@ struct qemu_argument arg_table[] = {
> >      {"s",          "QEMU_STACK_SIZE",  true,  handle_arg_stack_size,
> >       "size",       "set the stack size to 'size' bytes"},
> >      {"cpu",        "QEMU_CPU",         true,  handle_arg_cpu,
> > -     "model",      "select CPU (-cpu ? for list)"},
> > +     "model",      "select CPU (-cpu help for list)"},
> >      {"E",          "QEMU_SET_ENV",     true,  handle_arg_set_env,
> >       "var=value",  "sets targets environment variable (see below)"},
> >      {"U",          "QEMU_UNSET_ENV",   true,  handle_arg_unset_env,
> > diff --git a/net.c b/net.c
> > index dbca77b..32ca50e 100644
> > --- a/net.c
> > +++ b/net.c
> > @@ -691,8 +691,9 @@ int qemu_show_nic_models(const char *arg, const char 
> > *const *models)
> >  {
> >      int i;
> >  
> > -    if (!arg || strcmp(arg, "?"))
> > +    if (!arg || !is_help_option(arg)) {
> >          return 0;
> > +    }
> >  
> >      fprintf(stderr, "qemu: Supported NIC models: ");
> >      for (i = 0 ; models[i]; i++)
> > diff --git a/qemu-common.h b/qemu-common.h
> > index d26ff39..dd91912 100644
> > --- a/qemu-common.h
> > +++ b/qemu-common.h
> > @@ -136,6 +136,24 @@ int qemu_main(int argc, char **argv, char **envp);
> >  void qemu_get_timedate(struct tm *tm, int offset);
> >  int qemu_timedate_diff(struct tm *tm);
> >  
> > +/**
> > + * is_help_option:
> > + * @s: string to test
> > + *
> > + * Check whether @s is one of the standard strings which indicate
> > + * that the user is asking for a list of the valid values for a
> > + * command option like -cpu or -M. The current accepted strings
> > + * are 'help' and '?'. '?' is deprecated (it is a shell wildcard
> > + * which makes it annoying to use in a reliable way) but provided
> > + * for backwards compatibility.
> > + *
> > + * Returns: true if @s is a request for a list.
> > + */
> > +static inline bool is_help_option(const char *s)
> > +{
> > +    return !strcmp(s, "?") || !strcmp(s, "help");
> > +}
> > +
> >  /* cutils.c */
> >  void pstrcpy(char *buf, int buf_size, const char *str);
> >  void strpadcpy(char *buf, int buf_size, const char *str, char pad);
> > diff --git a/qemu-doc.texi b/qemu-doc.texi
> > index 84dad19..a41448a 100644
> > --- a/qemu-doc.texi
> > +++ b/qemu-doc.texi
> > @@ -2390,7 +2390,7 @@ Set the x86 elf interpreter prefix 
> > (default=/usr/local/qemu-i386)
> >  @item -s size
> >  Set the x86 stack size in bytes (default=524288)
> >  @item -cpu model
> > -Select CPU model (-cpu ? for list and additional feature selection)
> > +Select CPU model (-cpu help for list and additional feature selection)
> >  @item -ignore-environment
> >  Start with an empty environment. Without this option,
> >  the initial environment is a copy of the caller's environment.
> > diff --git a/qemu-ga.c b/qemu-ga.c
> > index 8199da7..f1a39ec 100644
> > --- a/qemu-ga.c
> > +++ b/qemu-ga.c
> > @@ -736,7 +736,7 @@ int main(int argc, char **argv)
> >              break;
> >          case 'b': {
> >              char **list_head, **list;
> > -            if (*optarg == '?') {
> > +            if (is_help_option(optarg)) {
> >                  list_head = list = qmp_get_command_list();
> >                  while (*list != NULL) {
> >                      printf("%s\n", *list);
> > diff --git a/qemu-img.c b/qemu-img.c
> > index 80cfb9b..b866f80 100644
> > --- a/qemu-img.c
> > +++ b/qemu-img.c
> > @@ -350,7 +350,7 @@ static int img_create(int argc, char **argv)
> >          img_size = (uint64_t)sval;
> >      }
> >  
> > -    if (options && !strcmp(options, "?")) {
> > +    if (options && is_help_option(options)) {
> >          ret = print_block_option_help(filename, fmt);
> >          goto out;
> >      }
> > @@ -744,7 +744,7 @@ static int img_convert(int argc, char **argv)
> >      /* Initialize before goto out */
> >      qemu_progress_init(progress, 2.0);
> >  
> > -    if (options && !strcmp(options, "?")) {
> > +    if (options && is_help_option(options)) {
> >          ret = print_block_option_help(out_filename, out_fmt);
> >          goto out;
> >      }
> > diff --git a/qemu-option.c b/qemu-option.c
> > index 8334190..27891e7 100644
> > --- a/qemu-option.c
> > +++ b/qemu-option.c
> > @@ -529,6 +529,18 @@ const char *qemu_opt_get(QemuOpts *opts, const char 
> > *name)
> >      return opt ? opt->str : NULL;
> >  }
> >  
> > +bool qemu_opt_has_help_opt(QemuOpts *opts)
> > +{
> > +    QemuOpt *opt;
> > +
> > +    QTAILQ_FOREACH_REVERSE(opt, &opts->head, QemuOptHead, next) {
> > +        if (is_help_option(opt->name)) {
> > +            return true;
> > +        }
> > +    }
> > +    return false;
> > +}
> > +
> >  bool qemu_opt_get_bool(QemuOpts *opts, const char *name, bool defval)
> >  {
> >      QemuOpt *opt = qemu_opt_find(opts, name);
> > diff --git a/qemu-option.h b/qemu-option.h
> > index 951dec3..ca72986 100644
> > --- a/qemu-option.h
> > +++ b/qemu-option.h
> > @@ -107,6 +107,18 @@ struct QemuOptsList {
> >  };
> >  
> >  const char *qemu_opt_get(QemuOpts *opts, const char *name);
> > +/**
> > + * qemu_opt_has_help_opt:
> > + * @opts: options to search for a help request
> > + *
> > + * Check whether the options specified by @opts include one of the
> > + * standard strings which indicate that the user is asking for a
> > + * list of the valid values for a command line option (as defined
> > + * by is_help_option()).
> > + *
> > + * Returns: true if @opts includes 'help' or equivalent.
> > + */
> > +bool qemu_opt_has_help_opt(QemuOpts *opts);
> >  bool qemu_opt_get_bool(QemuOpts *opts, const char *name, bool defval);
> >  uint64_t qemu_opt_get_number(QemuOpts *opts, const char *name, uint64_t 
> > defval);
> >  uint64_t qemu_opt_get_size(QemuOpts *opts, const char *name, uint64_t 
> > defval);
> > diff --git a/qemu-options.hx b/qemu-options.hx
> > index dc68e15..9277414 100644
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -6,6 +6,10 @@ HXCOMM construct option structures, enums and help message 
> > for specified
> >  HXCOMM architectures.
> >  HXCOMM HXCOMM can be used for comments, discarded from both texi and C
> >  
> > +HXCOMM TODO : when we are able to change -help output without breaking
> > +HXCOMM libvirt we should update the help options which refer to -cpu ?,
> > +HXCOMM -driver ?, etc to use the preferred -cpu help etc instead.
> > +
> >  DEFHEADING(Standard options:)
> >  STEXI
> >  @table @option
> > diff --git a/qemu-timer.c b/qemu-timer.c
> > index de98977..062fdf2 100644
> > --- a/qemu-timer.c
> > +++ b/qemu-timer.c
> > @@ -183,7 +183,7 @@ void configure_alarms(char const *opt)
> >      char *name;
> >      struct qemu_alarm_timer tmp;
> >  
> > -    if (!strcmp(opt, "?")) {
> > +    if (is_help_option(opt)) {
> >          show_available_alarms();
> >          exit(0);
> >      }
> > diff --git a/vl.c b/vl.c
> > index 9fea320..1fd1114 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -2086,7 +2086,7 @@ static QEMUMachine *machine_parse(const char *name)
> >          printf("%-20s %s%s\n", m->name, m->desc,
> >                 m->is_default ? " (default)" : "");
> >      }
> > -    exit(!name || *name != '?');
> > +    exit(!name || !is_help_option(name));
> >  }
> >  
> >  static int tcg_init(void)
> > @@ -3216,7 +3216,7 @@ int main(int argc, char **argv, char **envp)
> >       */
> >      cpudef_init();
> >  
> > -    if (cpu_model && *cpu_model == '?') {
> > +    if (cpu_model && is_help_option(cpu_model)) {
> >          list_cpus(stdout, &fprintf, cpu_model);
> >          exit(0);
> >      }
> > -- 
> > 1.7.9.5
> 
> 

-- 
Eduardo