Re: [Qemu-devel] [PATCH 1/1] Add usb option in machine options.

["Andreas Färber" <afaerber@suse.de>]

Am 14.06.2012 07:17, schrieb zhlcindy@gmail.com:
> From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
> 
> For pseries machine, it needs to enable usb
> to add kbd or usb mouse. -usb option won't
> be used in the future, and machine options
> is a better way to enable usb.
> 
> So this patch is to add usb option to machine
> options (-machine type=psereis,usb=on/off)
> to enable/disable usb controller.
> 
> In this patch, usb_opt is an global option
> which can be checked by machines. For example,
> on pseries, it will check if usb_opt is on, if
> it is on, it will create one usb ohci controller.
> As the following:
> if (usb_opts && strcmp(usb_opts, "on") == 0)
>      pci_create_simple(bus, -1, "pci-ohci");
> 
> In this patch, usb is on by default.
> So, for -nodefault, usb should be set off in the
> command line as the following:
>  -machine type=pseries,usb=off.
> 
> Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com>
> reviewed-by:   Anthony Liguori <aliguori@us.ibm.com>
> reviewed-by:   Benjamin Herrenschmidt <benh@au1.ibm.com>
> ---
>  qemu-config.c |    4 ++++
>  sysemu.h      |    1 +
>  vl.c          |   12 ++++++++++++
>  3 files changed, 17 insertions(+)
> 
> diff --git a/qemu-config.c b/qemu-config.c
> index bb3bff4..258712a 100644
> --- a/qemu-config.c
> +++ b/qemu-config.c
> @@ -583,6 +583,10 @@ static QemuOptsList qemu_machine_opts = {
>              .name = "dtb",
>              .type = QEMU_OPT_STRING,
>              .help = "Linux kernel device tree file",
> +        }, {
> +            .name = "usb",
> +            .type = QEMU_OPT_BOOL,
> +            .help = "Set on/off to enable/disable usb",
>          },
>          { /* End of list */ }
>      },
> diff --git a/sysemu.h b/sysemu.h
> index bc2c788..c5ea10d 100644
> --- a/sysemu.h
> +++ b/sysemu.h
> @@ -13,6 +13,7 @@
>  /* vl.c */
>  
>  extern const char *bios_name;
> +extern const char *usb_opt;
>  
>  extern const char *qemu_name;
>  extern uint8_t qemu_uuid[];
> diff --git a/vl.c b/vl.c
> index 204d85b..10f8e4c 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -171,6 +171,7 @@ int main(int argc, char **argv)
>  
>  static const char *data_dir;
>  const char *bios_name = NULL;
> +const char *usb_opt = NULL;
>  enum vga_retrace_method vga_retrace_method = VGA_RETRACE_DUMB;
>  DisplayType display_type = DT_DEFAULT;
>  int display_remote = 0;

I'd be very surprised if Anthony has actually reviewed this...

The point of using machine options is so that you can use the QemuOpts
infrastructure to inquire this value, not to save more global state.
Especially not a string when all you want is a boolean value.

Further, in this patch it's only being assigned, not used anywhere.

Andreas

> @@ -758,6 +759,15 @@ static int bt_parse(const char *opt)
>      return 1;
>  }
>  
> +static int default_enable_usb(QemuOpts *opts)
> +{
> +    if (NULL == qemu_opt_get(opts, "usb")) {
> +        qemu_opt_set(opts, "usb", "on");
> +    }
> +
> +    return 0;
> +}
> +
>  /***********************************************************/
>  /* QEMU Block devices */
>  
> @@ -3356,6 +3366,8 @@ int main(int argc, char **argv, char **envp)
>          kernel_filename = qemu_opt_get(machine_opts, "kernel");
>          initrd_filename = qemu_opt_get(machine_opts, "initrd");
>          kernel_cmdline = qemu_opt_get(machine_opts, "append");
> +        default_enable_usb(machine_opts);
> +        usb_opt = qemu_opt_get(machine_opts, "usb");
>      } else {
>          kernel_filename = initrd_filename = kernel_cmdline = NULL;
>      }


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg