Re: [PATCH] Warn user if the vga flag is passed but no vga device is created
On Mon, 25 Apr 2022 at 06:47, Markus Armbruster wrote: > > Gautam Agrawal writes: > > > This patch is in regards to this > > issue:https://gitlab.com/qemu-project/qemu/-/issues/581#. > > A global boolean variable "vga_interface_created"(declared in > > softmmu/globals.c) > > has been used to track the creation of vga interface. If the vga flag is > > passed in the command > > line "default_vga"(declared in softmmu/vl.c) variable is set to 0. To warn > > user, the condition > > checks if vga_interface_created is false and default_vga is equal to 0. > > > > The warning "No vga device is created" is logged if vga flag is passed > > but no vga device is created. This patch has been tested for > > x86_64, i386, sparc, sparc64 and arm boards. > > Suggest to include a reproducer here, e.g. > > $ qemu-system-x86_64 -S -display none -M microvm -vga std > qemu-system-x86_64: warning: No vga device is created > > See below for my critique of the warning message. You're reviewing an old version of this patch -- the newer versions improve the message... -- PMM
Re: [PATCH] Warn user if the vga flag is passed but no vga device is created
Gautam Agrawal writes:
> This patch is in regards to this
> issue:https://gitlab.com/qemu-project/qemu/-/issues/581#.
> A global boolean variable "vga_interface_created"(declared in
> softmmu/globals.c)
> has been used to track the creation of vga interface. If the vga flag is
> passed in the command
> line "default_vga"(declared in softmmu/vl.c) variable is set to 0. To warn
> user, the condition
> checks if vga_interface_created is false and default_vga is equal to 0.
>
> The warning "No vga device is created" is logged if vga flag is passed
> but no vga device is created. This patch has been tested for
> x86_64, i386, sparc, sparc64 and arm boards.
Suggest to include a reproducer here, e.g.
$ qemu-system-x86_64 -S -display none -M microvm -vga std
qemu-system-x86_64: warning: No vga device is created
See below for my critique of the warning message.
>
> Signed-off-by: Gautam Agrawal
> ---
> hw/isa/isa-bus.c| 1 +
> hw/pci/pci.c| 1 +
> hw/sparc/sun4m.c| 2 ++
> hw/sparc64/sun4u.c | 1 +
> include/sysemu/sysemu.h | 1 +
> softmmu/globals.c | 1 +
> softmmu/vl.c| 3 +++
> 7 files changed, 10 insertions(+)
>
> diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
> index 0ad1c5fd65..cd5ad3687d 100644
> --- a/hw/isa/isa-bus.c
> +++ b/hw/isa/isa-bus.c
> @@ -166,6 +166,7 @@ bool isa_realize_and_unref(ISADevice *dev, ISABus *bus,
> Error **errp)
>
> ISADevice *isa_vga_init(ISABus *bus)
> {
> +vga_interface_created = true;
> switch (vga_interface_type) {
> case VGA_CIRRUS:
> return isa_create_simple(bus, "isa-cirrus-vga");
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index dae9119bfe..fab9c80f8d 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -2038,6 +2038,7 @@ PCIDevice *pci_nic_init_nofail(NICInfo *nd, PCIBus
> *rootbus,
>
> PCIDevice *pci_vga_init(PCIBus *bus)
> {
> +vga_interface_created = true;
> switch (vga_interface_type) {
> case VGA_CIRRUS:
> return pci_create_simple(bus, -1, "cirrus-vga");
> diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
> index 7f3a7c0027..f45e29acc8 100644
> --- a/hw/sparc/sun4m.c
> +++ b/hw/sparc/sun4m.c
> @@ -921,6 +921,7 @@ static void sun4m_hw_init(MachineState *machine)
> /* sbus irq 5 */
> cg3_init(hwdef->tcx_base, slavio_irq[11], 0x0010,
> graphic_width, graphic_height, graphic_depth);
> +vga_interface_created = true;
> } else {
> /* If no display specified, default to TCX */
> if (graphic_depth != 8 && graphic_depth != 24) {
> @@ -936,6 +937,7 @@ static void sun4m_hw_init(MachineState *machine)
>
> tcx_init(hwdef->tcx_base, slavio_irq[11], 0x0010,
> graphic_width, graphic_height, graphic_depth);
> +vga_interface_created = true;
> }
> }
>
> diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
> index cda7df36e3..75334dba71 100644
> --- a/hw/sparc64/sun4u.c
> +++ b/hw/sparc64/sun4u.c
> @@ -633,6 +633,7 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
> switch (vga_interface_type) {
> case VGA_STD:
> pci_create_simple(pci_busA, PCI_DEVFN(2, 0), "VGA");
> +vga_interface_created = true;
> break;
> case VGA_NONE:
> break;
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index b9421e03ff..a558b895e4 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -32,6 +32,7 @@ typedef enum {
> } VGAInterfaceType;
>
> extern int vga_interface_type;
> +extern bool vga_interface_created;
>
> extern int graphic_width;
> extern int graphic_height;
> diff --git a/softmmu/globals.c b/softmmu/globals.c
> index 3ebd718e35..1a5f8d42ad 100644
> --- a/softmmu/globals.c
> +++ b/softmmu/globals.c
> @@ -40,6 +40,7 @@ int nb_nics;
> NICInfo nd_table[MAX_NICS];
> int autostart = 1;
> int vga_interface_type = VGA_NONE;
> +bool vga_interface_created = false;
> Chardev *parallel_hds[MAX_PARALLEL_PORTS];
> int win2k_install_hack;
> int singlestep;
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index 6f646531a0..cb79fa1f42 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -2734,6 +2734,9 @@ static void qemu_machine_creation_done(void)
> if (foreach_device_config(DEV_GDB, gdbserver_start) < 0) {
> exit(1);
> }
> +if (!vga_interface_created && !default_vga) {
> +warn_report("No vga device is created");
True, but this leaves the user guessing why.
Pointing to the option would help:
qemu-system-x86_64: warning: -vga std: No vga device is created
To get this, use loc_save() to save the option's location along
@vga_model, then bracket the warn_report() with loc_push_restore() and
loc_pop().
The option to ask the board to create a video device is spelled -vga for
historical reasons. Some of its arguments aren't VGA devices, e.g. tcx.
-help is phrased accordingly:
Re: [PATCH] Warn user if the vga flag is passed but no vga device is created
Hi, On 12/04/2022 22.25, Gautam Agrawal wrote: [...]>> On 08/04/2022 12.45, Gautam Agrawal wrote: This patch is in regards to this issue:https://gitlab.com/qemu-project/qemu/-/issues/581#. Better write this right in front of your Signed-off-by line: Resolves: https://gitlab.com/qemu-project/qemu/-/issues/581 ... then the ticket will be automatically be closed once your patch gets merged. I apologize for this mistake No need to apologize, it was just a hint for a minor improvement - and you've got the other patch formatting very right already :-) vga_interface_type is also used in hw/mips/fuloong2e.c and hw/xenpv/xen_machine_pv.c ... do they need a change, too? I can definitely make similar changes in them too since they also specify the vga_interface_type, shall I proceed with this? This will trigger a warning from the scripts/checkpatch.pl script: ERROR: do not initialise globals to 0 or NULL #238: FILE: softmmu/globals.c:43: +bool vga_interface_created = false; Could you kindly suggest a better approach to this than creating a global variable. Global variables are fine, simply drop the "= false" at the end of the line. All global variables are always pre-initialized to 0, so global boolean variables are set to 0 (i.e. false) by default, too. I'm not a native speaker, and maybe it's just a matter of taste, but I'd rather say it in past tense: "No VGA device has been created" I will correct the warning message, as suggested by Peter Maydell. Great, thank you! Thomas
Re: [PATCH] Warn user if the vga flag is passed but no vga device is created
hi, > thanks for your patch, looks pretty good already, but there is a small > issue: Try for example: > > ./qemu-system-s390x -vga none > > ... and it will print the warning "qemu-system-s390x: warning: No vga device > is created", though the user only asked for no VGA device. This seems to > happen if a machine does not have any VGA device by default, but still > requests "-vga none" on the command line. This can be solved by adding this condition : (vga_interface_type != VGA_NONE) > On 08/04/2022 12.45, Gautam Agrawal wrote: > > This patch is in regards to this > > issue:https://gitlab.com/qemu-project/qemu/-/issues/581#. > > Better write this right in front of your Signed-off-by line: > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/581 > > ... then the ticket will be automatically be closed once your patch gets > merged. > I apologize for this mistake > vga_interface_type is also used in hw/mips/fuloong2e.c and > hw/xenpv/xen_machine_pv.c ... do they need a change, too? I can definitely make similar changes in them too since they also specify the vga_interface_type, shall I proceed with this? > This will trigger a warning from the scripts/checkpatch.pl script: > > ERROR: do not initialise globals to 0 or NULL > #238: FILE: softmmu/globals.c:43: > +bool vga_interface_created = false; Could you kindly suggest a better approach to this than creating a global variable. > I'm not a native speaker, and maybe it's just a matter of taste, but I'd > rather say it in past tense: "No VGA device has been created" I will correct the warning message, as suggested by Peter Maydell. Regards, Gautam Agrawal
Re: [PATCH] Warn user if the vga flag is passed but no vga device is created
On Tue, 12 Apr 2022 at 13:13, Thomas Huth wrote:
> On 08/04/2022 12.45, Gautam Agrawal wrote:
> > +if (!vga_interface_created && !default_vga) {
> > +warn_report("No vga device is created");
>
> I'm not a native speaker, and maybe it's just a matter of taste, but I'd
> rather say it in past tense: "No VGA device has been created"
I think we could phrase the warning to tell the user more
clearly what has happened:
"A -vga option was passed but this machine type does not use that
option; no VGA device has been created"
thanks
-- PMM
Re: [PATCH] Warn user if the vga flag is passed but no vga device is created
Hi,
thanks for your patch, looks pretty good already, but there is a small
issue: Try for example:
./qemu-system-s390x -vga none
... and it will print the warning "qemu-system-s390x: warning: No vga device
is created", though the user only asked for no VGA device. This seems to
happen if a machine does not have any VGA device by default, but still
requests "-vga none" on the command line.
Some more comments below...
On 08/04/2022 12.45, Gautam Agrawal wrote:
This patch is in regards to this
issue:https://gitlab.com/qemu-project/qemu/-/issues/581#.
Better write this right in front of your Signed-off-by line:
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/581
... then the ticket will be automatically be closed once your patch gets merged.
A global boolean variable "vga_interface_created"(declared in softmmu/globals.c)
has been used to track the creation of vga interface. If the vga flag is passed
in the command
line "default_vga"(declared in softmmu/vl.c) variable is set to 0. To warn
user, the condition
checks if vga_interface_created is false and default_vga is equal to 0.
The warning "No vga device is created" is logged if vga flag is passed
but no vga device is created. This patch has been tested for
x86_64, i386, sparc, sparc64 and arm boards.
Signed-off-by: Gautam Agrawal
---
hw/isa/isa-bus.c| 1 +
hw/pci/pci.c| 1 +
hw/sparc/sun4m.c| 2 ++
hw/sparc64/sun4u.c | 1 +
include/sysemu/sysemu.h | 1 +
softmmu/globals.c | 1 +
softmmu/vl.c| 3 +++
7 files changed, 10 insertions(+)
vga_interface_type is also used in hw/mips/fuloong2e.c and
hw/xenpv/xen_machine_pv.c ... do they need a change, too?
diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
index 0ad1c5fd65..cd5ad3687d 100644
--- a/hw/isa/isa-bus.c
+++ b/hw/isa/isa-bus.c
@@ -166,6 +166,7 @@ bool isa_realize_and_unref(ISADevice *dev, ISABus *bus,
Error **errp)
ISADevice *isa_vga_init(ISABus *bus)
{
+vga_interface_created = true;
switch (vga_interface_type) {
case VGA_CIRRUS:
return isa_create_simple(bus, "isa-cirrus-vga");
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index dae9119bfe..fab9c80f8d 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2038,6 +2038,7 @@ PCIDevice *pci_nic_init_nofail(NICInfo *nd, PCIBus
*rootbus,
PCIDevice *pci_vga_init(PCIBus *bus)
{
+vga_interface_created = true;
switch (vga_interface_type) {
case VGA_CIRRUS:
return pci_create_simple(bus, -1, "cirrus-vga");
diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
index 7f3a7c0027..f45e29acc8 100644
--- a/hw/sparc/sun4m.c
+++ b/hw/sparc/sun4m.c
@@ -921,6 +921,7 @@ static void sun4m_hw_init(MachineState *machine)
/* sbus irq 5 */
cg3_init(hwdef->tcx_base, slavio_irq[11], 0x0010,
graphic_width, graphic_height, graphic_depth);
+vga_interface_created = true;
} else {
/* If no display specified, default to TCX */
if (graphic_depth != 8 && graphic_depth != 24) {
@@ -936,6 +937,7 @@ static void sun4m_hw_init(MachineState *machine)
tcx_init(hwdef->tcx_base, slavio_irq[11], 0x0010,
graphic_width, graphic_height, graphic_depth);
+vga_interface_created = true;
}
}
diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
index cda7df36e3..75334dba71 100644
--- a/hw/sparc64/sun4u.c
+++ b/hw/sparc64/sun4u.c
@@ -633,6 +633,7 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
switch (vga_interface_type) {
case VGA_STD:
pci_create_simple(pci_busA, PCI_DEVFN(2, 0), "VGA");
+vga_interface_created = true;
break;
case VGA_NONE:
break;
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index b9421e03ff..a558b895e4 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -32,6 +32,7 @@ typedef enum {
} VGAInterfaceType;
extern int vga_interface_type;
+extern bool vga_interface_created;
extern int graphic_width;
extern int graphic_height;
diff --git a/softmmu/globals.c b/softmmu/globals.c
index 3ebd718e35..1a5f8d42ad 100644
--- a/softmmu/globals.c
+++ b/softmmu/globals.c
@@ -40,6 +40,7 @@ int nb_nics;
NICInfo nd_table[MAX_NICS];
int autostart = 1;
int vga_interface_type = VGA_NONE;
+bool vga_interface_created = false;
This will trigger a warning from the scripts/checkpatch.pl script:
ERROR: do not initialise globals to 0 or NULL
#238: FILE: softmmu/globals.c:43:
+bool vga_interface_created = false;
Chardev *parallel_hds[MAX_PARALLEL_PORTS];
int win2k_install_hack;
int singlestep;
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 6f646531a0..cb79fa1f42 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -2734,6 +2734,9 @@ static void qemu_machine_creation_done(void)
if (foreach_device_config(DEV_GDB, gdbserver_start) < 0) {
exit(1);
}
[PATCH] Warn user if the vga flag is passed but no vga device is created
This patch is in regards to this
issue:https://gitlab.com/qemu-project/qemu/-/issues/581#.
A global boolean variable "vga_interface_created"(declared in softmmu/globals.c)
has been used to track the creation of vga interface. If the vga flag is passed
in the command
line "default_vga"(declared in softmmu/vl.c) variable is set to 0. To warn
user, the condition
checks if vga_interface_created is false and default_vga is equal to 0.
The warning "No vga device is created" is logged if vga flag is passed
but no vga device is created. This patch has been tested for
x86_64, i386, sparc, sparc64 and arm boards.
Signed-off-by: Gautam Agrawal
---
hw/isa/isa-bus.c| 1 +
hw/pci/pci.c| 1 +
hw/sparc/sun4m.c| 2 ++
hw/sparc64/sun4u.c | 1 +
include/sysemu/sysemu.h | 1 +
softmmu/globals.c | 1 +
softmmu/vl.c| 3 +++
7 files changed, 10 insertions(+)
diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
index 0ad1c5fd65..cd5ad3687d 100644
--- a/hw/isa/isa-bus.c
+++ b/hw/isa/isa-bus.c
@@ -166,6 +166,7 @@ bool isa_realize_and_unref(ISADevice *dev, ISABus *bus,
Error **errp)
ISADevice *isa_vga_init(ISABus *bus)
{
+vga_interface_created = true;
switch (vga_interface_type) {
case VGA_CIRRUS:
return isa_create_simple(bus, "isa-cirrus-vga");
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index dae9119bfe..fab9c80f8d 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2038,6 +2038,7 @@ PCIDevice *pci_nic_init_nofail(NICInfo *nd, PCIBus
*rootbus,
PCIDevice *pci_vga_init(PCIBus *bus)
{
+vga_interface_created = true;
switch (vga_interface_type) {
case VGA_CIRRUS:
return pci_create_simple(bus, -1, "cirrus-vga");
diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
index 7f3a7c0027..f45e29acc8 100644
--- a/hw/sparc/sun4m.c
+++ b/hw/sparc/sun4m.c
@@ -921,6 +921,7 @@ static void sun4m_hw_init(MachineState *machine)
/* sbus irq 5 */
cg3_init(hwdef->tcx_base, slavio_irq[11], 0x0010,
graphic_width, graphic_height, graphic_depth);
+vga_interface_created = true;
} else {
/* If no display specified, default to TCX */
if (graphic_depth != 8 && graphic_depth != 24) {
@@ -936,6 +937,7 @@ static void sun4m_hw_init(MachineState *machine)
tcx_init(hwdef->tcx_base, slavio_irq[11], 0x0010,
graphic_width, graphic_height, graphic_depth);
+vga_interface_created = true;
}
}
diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
index cda7df36e3..75334dba71 100644
--- a/hw/sparc64/sun4u.c
+++ b/hw/sparc64/sun4u.c
@@ -633,6 +633,7 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
switch (vga_interface_type) {
case VGA_STD:
pci_create_simple(pci_busA, PCI_DEVFN(2, 0), "VGA");
+vga_interface_created = true;
break;
case VGA_NONE:
break;
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index b9421e03ff..a558b895e4 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -32,6 +32,7 @@ typedef enum {
} VGAInterfaceType;
extern int vga_interface_type;
+extern bool vga_interface_created;
extern int graphic_width;
extern int graphic_height;
diff --git a/softmmu/globals.c b/softmmu/globals.c
index 3ebd718e35..1a5f8d42ad 100644
--- a/softmmu/globals.c
+++ b/softmmu/globals.c
@@ -40,6 +40,7 @@ int nb_nics;
NICInfo nd_table[MAX_NICS];
int autostart = 1;
int vga_interface_type = VGA_NONE;
+bool vga_interface_created = false;
Chardev *parallel_hds[MAX_PARALLEL_PORTS];
int win2k_install_hack;
int singlestep;
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 6f646531a0..cb79fa1f42 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -2734,6 +2734,9 @@ static void qemu_machine_creation_done(void)
if (foreach_device_config(DEV_GDB, gdbserver_start) < 0) {
exit(1);
}
+if (!vga_interface_created && !default_vga) {
+warn_report("No vga device is created");
+}
}
void qmp_x_exit_preconfig(Error **errp)
--
2.34.1
