Re: [Qemu-devel] [PATCH trival] vl.c: clean up code

[Markus Armbruster]

Chen Gang <address@hidden> writes:

> in get_boot_device()
>
>  - remove 'res' to simplify code
>
> in main():
>
>  - remove useless 'continue'.
>
>  - in main switch():
>
>    - remove or adjust all useless 'break'.
>
>    - remove useless '{' and '}'.
>
>  - use assignment directly to replace useless 'args'
>    (which is defined in the middle of code block).

Suggest to have one patch per item in this list.

>
> Signed-off-by: Chen Gang <address@hidden>
> ---
>  vl.c | 36 +++++++++++++-----------------------
>  1 file changed, 13 insertions(+), 23 deletions(-)
>
> diff --git a/vl.c b/vl.c
> index 9975e5a..9c733cb 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1188,18 +1188,16 @@ DeviceState *get_boot_device(uint32_t position)
>  {
>      uint32_t counter = 0;
>      FWBootEntry *i = NULL;
> -    DeviceState *res = NULL;
>  
>      if (!QTAILQ_EMPTY(&fw_boot_order)) {
>          QTAILQ_FOREACH(i, &fw_boot_order, link) {
>              if (counter == position) {
> -                res = i->dev;
> -                break;
> +                return i->dev;
>              }
>              counter++;
>          }
>      }
> -    return res;
> +    return NULL;
>  }
>  
>  /*
> @@ -3034,7 +3032,6 @@ int main(int argc, char **argv, char **envp)
>          if (argv[optind][0] != '-') {
>              /* disk image */
>              optind++;
> -            continue;
>          } else {
>              const QEMUOption *popt;
>  
> @@ -3204,11 +3201,11 @@ int main(int argc, char **argv, char **envp)
>              case QEMU_OPTION_curses:
>  #ifdef CONFIG_CURSES
>                  display_type = DT_CURSES;
> +                break;
>  #else
>                  fprintf(stderr, "Curses support is disabled\n");
>                  exit(1);
>  #endif
> -                break;
>              case QEMU_OPTION_portrait:
>                  graphic_rotate = 90;
>                  break;

I'm not sure eliding the break after exit is worthwhile.

In theory, it could cause false positives with tools smart enough to
diagnose fall through without comment, but too stupid to see that exit()
never returns.  No idea whether such tools exist.

The missing break might give human readers slight pause, until they
recognize exit.  Especially with such ifdeffery.

Dunno.

> @@ -3286,7 +3283,6 @@ int main(int argc, char **argv, char **envp)
>              case QEMU_OPTION_audio_help:
>                  AUD_help ();
>                  exit (0);
> -                break;
>              case QEMU_OPTION_soundhw:
>                  select_soundhw (optarg);
>                  break;
> @@ -3296,7 +3292,6 @@ int main(int argc, char **argv, char **envp)
>              case QEMU_OPTION_version:
>                  version();
>                  exit(0);
> -                break;
>              case QEMU_OPTION_m: {
>                  int64_t value;
>                  uint64_t sz;
> @@ -3638,11 +3633,10 @@ int main(int argc, char **argv, char **envp)
>                  olist = qemu_find_opts("machine");
>                  qemu_opts_parse(olist, "accel=tcg", 0);
>                  break;
> -            case QEMU_OPTION_no_kvm_pit: {
> +            case QEMU_OPTION_no_kvm_pit:
>                  fprintf(stderr, "Warning: KVM PIT can no longer be disabled "
>                                  "separately.\n");
>                  break;
> -            }
>              case QEMU_OPTION_no_kvm_pit_reinjection: {
>                  static GlobalProperty kvm_pit_lost_tick_policy[] = {
>                      {
> @@ -3681,11 +3675,11 @@ int main(int argc, char **argv, char **envp)
>  #ifdef CONFIG_VNC
>                  display_remote++;
>                  vnc_display = optarg;
> +                break;
>  #else
>                  fprintf(stderr, "VNC support is disabled\n");
>                  exit(1);
>  #endif
> -                break;
>              case QEMU_OPTION_no_acpi:
>                  acpi_enabled = 0;
>                  break;
> @@ -3811,7 +3805,6 @@ int main(int argc, char **argv, char **envp)
>                  xen_mode = XEN_ATTACH;
>                  break;
>              case QEMU_OPTION_trace:
> -            {
>                  opts = qemu_opts_parse(qemu_find_opts("trace"), optarg, 0);
>                  if (!opts) {
>                      exit(1);
> @@ -3819,7 +3812,6 @@ int main(int argc, char **argv, char **envp)
>                  trace_events = qemu_opt_get(opts, "events");
>                  trace_file = qemu_opt_get(opts, "file");
>                  break;
> -            }
>              case QEMU_OPTION_readconfig:
>                  {
>                      int ret = qemu_read_config_file(optarg);
> @@ -3876,12 +3868,12 @@ int main(int argc, char **argv, char **envp)
>                  if (!opts) {
>                      exit(1);
>                  }
> +                break;
>  #else
>                  error_report("File descriptor passing is disabled on this "
>                               "platform");
>                  exit(1);
>  #endif
> -                break;
>              case QEMU_OPTION_object:
>                  opts = qemu_opts_parse(qemu_find_opts("object"), optarg, 1);
>                  if (!opts) {
> @@ -4371,15 +4363,13 @@ int main(int argc, char **argv, char **envp)
>  
>      qdev_machine_init();
>  
> -    QEMUMachineInitArgs args = { .machine = machine,
> -                                 .ram_size = ram_size,
> -                                 .boot_order = boot_order,
> -                                 .kernel_filename = kernel_filename,
> -                                 .kernel_cmdline = kernel_cmdline,
> -                                 .initrd_filename = initrd_filename,
> -                                 .cpu_model = cpu_model };
> -
> -    current_machine->init_args = args;
> +    current_machine->init_args.machine = machine;
> +    current_machine->init_args.ram_size = ram_size;
> +    current_machine->init_args.boot_order = boot_order;
> +    current_machine->init_args.kernel_filename = kernel_filename;
> +    current_machine->init_args.kernel_cmdline = kernel_cmdline;
> +    current_machine->init_args.initrd_filename = initrd_filename;
> +    current_machine->init_args.cpu_model = cpu_model;
>      machine->init(&current_machine->init_args);
>  
>      audio_init();

I agree with dropping useless variable args.  However, you don't have to
replace the assignment of a compound literal by multiple simple
assignments for that.  You could write

    current_machine->init_args = (QEMUMachineInitArgs){
        .machine = machine,
        .ram_size = ram_size,
        .boot_order = boot_order,
        .kernel_filename = kernel_filename,
        .kernel_cmdline = kernel_cmdline,
        .initrd_filename = initrd_filename,
        .cpu_model = cpu_model };

Not exactly the same; it this implicitly zeroes members not mentioned.
No such members exist now.

Matter of taste, I guess.