| Message ID | 877g758pnj.fsf@abhimanyu.in.ibm.com |
|---|---|
| State | New |
| Headers | show |
Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> writes: > Markus Armbruster <armbru@redhat.com> writes: >> Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> writes: >> >> Have you considered extending QEMUMachineInitArgs instead of adding this >> function? > > Did not think of this option earlier. You mean doing something like > this? Yes. Looks nicer, doesn't it?
Il 04/04/2014 12:58, Markus Armbruster ha scritto: >>> >> >>> >> Have you considered extending QEMUMachineInitArgs instead of adding this >>> >> function? >> > >> > Did not think of this option earlier. You mean doing something like >> > this? > Yes. Looks nicer, doesn't it? I still think it's a libvirt bug. Mixing -nodefaults and -usb and -device is looking for trouble I think. "-usb" is a do-what-I-mean kind of option and it makes sense for it to add a keyboard and mouse, even with -nodefaults. Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > Il 04/04/2014 12:58, Markus Armbruster ha scritto: >>>> >> >>>> >> Have you considered extending QEMUMachineInitArgs instead of adding this >>>> >> function? >>> > >>> > Did not think of this option earlier. You mean doing something like >>> > this? >> Yes. Looks nicer, doesn't it? > > I still think it's a libvirt bug. Mixing -nodefaults and -usb and > -device is looking for trouble I think. "-usb" is a do-what-I-mean > kind of option and it makes sense for it to add a keyboard and mouse, > even with -nodefaults. I agree that management tools should not use -usb, except when they want to manage ancient versions of QEMU for some reason.
Am 04.04.2014 10:28, schrieb Nikunj A Dadhania: > diff --git a/vl.c b/vl.c > index 017f92d..0d6c36c 100644 > --- a/vl.c > +++ b/vl.c > @@ -4348,7 +4348,8 @@ int main(int argc, char **argv, char **envp) > .kernel_filename = kernel_filename, > .kernel_cmdline = kernel_cmdline, > .initrd_filename = initrd_filename, > - .cpu_model = cpu_model }; > + .cpu_model = cpu_model, > + .has_defaults = has_defaults, }; If we do this, please put }; on the next line so that the next person appending something doesn't need to touch it again. :) I do agree with the others that libvirt shouldn't be using the legacy -usb option. Cheers, Andreas > machine->init(&args); > > audio_init();
[adding libvir-list] On 04/04/2014 05:23 AM, Markus Armbruster wrote: > Paolo Bonzini <pbonzini@redhat.com> writes: > >> Il 04/04/2014 12:58, Markus Armbruster ha scritto: >>>>>>> >>>>>>> Have you considered extending QEMUMachineInitArgs instead of adding this >>>>>>> function? >>>>> >>>>> Did not think of this option earlier. You mean doing something like >>>>> this? >>> Yes. Looks nicer, doesn't it? >> >> I still think it's a libvirt bug. Mixing -nodefaults and -usb and >> -device is looking for trouble I think. "-usb" is a do-what-I-mean >> kind of option and it makes sense for it to add a keyboard and mouse, >> even with -nodefaults. > > I agree that management tools should not use -usb, except when they want > to manage ancient versions of QEMU for some reason. We've tried to make libvirt avoid -usb when it knows it is targetting a new enough qemu. But I'm not the one most familiar with that code. At this point, I'm just trying to facilitate discussions, and make sure the right people are looking at this - what versions of qemu vs. libvirt have you tested, and what does libvirt still need to patch to avoid -usb, and/or qemu patch so that libvirt can correctly probe that -usb is not needed?
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index 3a13231..936a17f 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -1410,6 +1410,7 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args) const char *kernel_cmdline = args->kernel_cmdline; const char *initrd_filename = args->initrd_filename; const char *boot_device = args->boot_device; + unsigned int has_defaults = args->has_defaults; PowerPCCPU *cpu; CPUPPCState *env; PCIHostState *phb; @@ -1605,7 +1606,11 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args) if (usb_enabled(spapr->has_graphics)) { pci_create_simple(phb->bus, -1, "pci-ohci"); - if (spapr->has_graphics) { + /* + * For VGA/VNC, by default add usb keyboard/mouse, if -nodefaults + * provided skip adding usb keyboard/mouse + */ + if (spapr->has_graphics && has_defaults) { usbdevice_create("keyboard"); usbdevice_create("mouse"); } diff --git a/include/hw/boards.h b/include/hw/boards.h index b1d4e9f..ee81ddf 100644 --- a/include/hw/boards.h +++ b/include/hw/boards.h @@ -16,6 +16,7 @@ typedef struct QEMUMachineInitArgs { const char *kernel_cmdline; const char *initrd_filename; const char *cpu_model; + unsigned int has_defaults; } QEMUMachineInitArgs; typedef void QEMUMachineInitFunc(QEMUMachineInitArgs *args); diff --git a/vl.c b/vl.c index 017f92d..0d6c36c 100644 --- a/vl.c +++ b/vl.c @@ -4348,7 +4348,8 @@ int main(int argc, char **argv, char **envp) .kernel_filename = kernel_filename, .kernel_cmdline = kernel_cmdline, .initrd_filename = initrd_filename, - .cpu_model = cpu_model }; + .cpu_model = cpu_model, + .has_defaults = has_defaults, }; machine->init(&args); audio_init();