[Qemu-devel] [PATCH 0/3] hw/core: minor fixups

[Qemu-devel] [PATCH 0/3] hw/core: minor fixups

[Philippe Mathieu-Daudé]

Sorry to spam so many people, there is no entries in MAINTAINERS for
hw/core/loader.c and hw/core/qdev.c, any volunters?

Philippe Mathieu-Daudé (3):
  elf-loader: warn about invalid endianess
  hw/core: fix missing return value in load_image_targphys_as()
  hw/core: report an error if invalid gpio is used

 hw/core/loader.c | 5 ++++-
 hw/core/qdev.c   | 6 +++++-
 2 files changed, 9 insertions(+), 2 deletions(-)

-- 
2.13.1

[Qemu-devel] [PATCH 1/3] elf-loader: warn about invalid endianess

[Philippe Mathieu-Daudé]

fprintf(stderr) is how errors are reported in this file.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/core/loader.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/core/loader.c b/hw/core/loader.c
index f72930ca4a..094f24627f 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -478,6 +478,7 @@ int load_elf_ram(const char *filename,
     }
 
     if (target_data_order != e_ident[EI_DATA]) {
+        fprintf(stderr, "%s: wrong endianess\n", filename);
         ret = ELF_LOAD_WRONG_ENDIAN;
         goto fail;
     }
-- 
2.13.1

[Qemu-devel] [PATCH 2/3] hw/core: fix missing return value in load_image_targphys_as()

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/core/loader.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/core/loader.c b/hw/core/loader.c
index 094f24627f..e137e772ae 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -150,7 +150,9 @@ int load_image_targphys_as(const char *filename,
         return -1;
     }
     if (size > 0) {
-        rom_add_file_fixed_as(filename, addr, -1, as);
+        if (rom_add_file_fixed_as(filename, addr, -1, as) < 0) {
+            return -1;
+        }
     }
     return size;
 }
-- 
2.13.1

[Qemu-devel] [PATCH 3/3] hw/core: report an error if invalid gpio is used

[Philippe Mathieu-Daudé]

then abort calling error_setg()

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/core/qdev.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 849952a8d4..05aaa67cb8 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -448,7 +448,11 @@ qemu_irq qdev_get_gpio_in_named(DeviceState *dev, const char *name, int n)
 {
     NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name);
 
-    assert(n >= 0 && n < gpio_list->num_in);
+    assert(n >= 0);
+    if (n >= gpio_list->num_in) {
+        error_setg(&error_abort, "Invalid gpio #%d (of %d) for %s",
+                   n, gpio_list->num_in, name ? name : "device");
+    }
     return gpio_list->in[n];
 }
 
-- 
2.13.1

Re: [Qemu-devel] [PATCH 1/3] elf-loader: warn about invalid endianess

[Peter Maydell]

On 23 June 2017 at 17:45, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> fprintf(stderr) is how errors are reported in this file.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/core/loader.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index f72930ca4a..094f24627f 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -478,6 +478,7 @@ int load_elf_ram(const char *filename,
>      }
>
>      if (target_data_order != e_ident[EI_DATA]) {
> +        fprintf(stderr, "%s: wrong endianess\n", filename);

"endianness" (two 'n's).

>          ret = ELF_LOAD_WRONG_ENDIAN;
>          goto fail;
>      }
> --
> 2.13.1

thanks
-- PMM

Re: [Qemu-devel] [PATCH 3/3] hw/core: report an error if invalid gpio is used

[Eduardo Habkost]

On Fri, Jun 23, 2017 at 01:45:57PM -0300, Philippe Mathieu-Daudé wrote:
> then abort calling error_setg()

I don't understand the reasons for this.  This commit message says
"what" and "how", but not "why".

> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/core/qdev.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 849952a8d4..05aaa67cb8 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -448,7 +448,11 @@ qemu_irq qdev_get_gpio_in_named(DeviceState *dev, const char *name, int n)
>  {
>      NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name);
>  
> -    assert(n >= 0 && n < gpio_list->num_in);
> +    assert(n >= 0);
> +    if (n >= gpio_list->num_in) {
> +        error_setg(&error_abort, "Invalid gpio #%d (of %d) for %s",
> +                   n, gpio_list->num_in, name ? name : "device");

Why exactly assert() is ok for (n < 0), but not for
(n >= gpio_list->num_io)?

If you have reasons to believe (n >= gpio_list->num_in) can be triggered
by user input, then abort() isn't an appropriate way to handle it.

> +    }
>      return gpio_list->in[n];
>  }
>  
> -- 
> 2.13.1
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 0/3] hw/core: minor fixups

[Eduardo Habkost]

On Fri, Jun 23, 2017 at 01:45:54PM -0300, Philippe Mathieu-Daudé wrote:
> Sorry to spam so many people, there is no entries in MAINTAINERS for
> hw/core/loader.c and hw/core/qdev.c, any volunters?

I can apply them through my machine tree, if nobody else volunteers.

Do you have a simple way to trigger the error paths addressed by patches 1/3
and 2/3?

-- 
Eduardo

Re: [Qemu-devel] [PATCH 3/3] hw/core: report an error if invalid gpio is used

[Eduardo Habkost]

On Fri, Jun 23, 2017 at 01:45:57PM -0300, Philippe Mathieu-Daudé wrote:
> then abort calling error_setg()
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/core/qdev.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 849952a8d4..05aaa67cb8 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -448,7 +448,11 @@ qemu_irq qdev_get_gpio_in_named(DeviceState *dev, const char *name, int n)
>  {
>      NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name);
>  
> -    assert(n >= 0 && n < gpio_list->num_in);
> +    assert(n >= 0);
> +    if (n >= gpio_list->num_in) {
> +        error_setg(&error_abort, "Invalid gpio #%d (of %d) for %s",
> +                   n, gpio_list->num_in, name ? name : "device");

I just noticed error_setg() documentation explicitly discourages this.

"""
  Please don't error_setg(&error_fatal, ...), use error_report() and
  exit(), because that's more obvious.
  Likewise, don't error_setg(&error_abort, ...), use assert().
"""

-- 
Eduardo

Re: [Qemu-devel] [PATCH 0/3] hw/core: minor fixups

[Laszlo Ersek]

On 06/23/17 18:45, Philippe Mathieu-Daudé wrote:
> Sorry to spam so many people, there is no entries in MAINTAINERS for
> hw/core/loader.c and hw/core/qdev.c, any volunters?
> 
> Philippe Mathieu-Daudé (3):
>   elf-loader: warn about invalid endianess
>   hw/core: fix missing return value in load_image_targphys_as()
>   hw/core: report an error if invalid gpio is used
> 
>  hw/core/loader.c | 5 ++++-
>  hw/core/qdev.c   | 6 +++++-
>  2 files changed, 9 insertions(+), 2 deletions(-)
> 

With the typo that Peter pointed out fixed:

series
Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Re: [Qemu-devel] [PATCH 0/3] hw/core: minor fixups

[Alistair Francis]

On Fri, Jun 23, 2017 at 9:45 AM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Sorry to spam so many people, there is no entries in MAINTAINERS for
> hw/core/loader.c and hw/core/qdev.c, any volunters?

If no one else wants to I could maintain hw/core/loader.c. It's pretty
similar to the generic-loader which I already maintain.

Thanks,
Alistair

>
> Philippe Mathieu-Daudé (3):
>   elf-loader: warn about invalid endianess
>   hw/core: fix missing return value in load_image_targphys_as()
>   hw/core: report an error if invalid gpio is used
>
>  hw/core/loader.c | 5 ++++-
>  hw/core/qdev.c   | 6 +++++-
>  2 files changed, 9 insertions(+), 2 deletions(-)
>
> --
> 2.13.1
>
>

Re: [Qemu-devel] [PATCH 2/3] hw/core: fix missing return value in load_image_targphys_as()

[Alistair Francis]

On Fri, Jun 23, 2017 at 9:45 AM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>

Thanks,
Alistair

> ---
>  hw/core/loader.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 094f24627f..e137e772ae 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -150,7 +150,9 @@ int load_image_targphys_as(const char *filename,
>          return -1;
>      }
>      if (size > 0) {
> -        rom_add_file_fixed_as(filename, addr, -1, as);
> +        if (rom_add_file_fixed_as(filename, addr, -1, as) < 0) {
> +            return -1;
> +        }
>      }
>      return size;
>  }
> --
> 2.13.1
>
>

Re: [Qemu-devel] [PATCH 3/3] hw/core: report an error if invalid gpio is used

[Alistair Francis]

On Fri, Jun 23, 2017 at 9:45 AM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> then abort calling error_setg()

The commit message should be able to be read separately from the title.

Once you fix up the message:

Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>

Thanks,
Alistair

>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/core/qdev.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 849952a8d4..05aaa67cb8 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -448,7 +448,11 @@ qemu_irq qdev_get_gpio_in_named(DeviceState *dev, const char *name, int n)
>  {
>      NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name);
>
> -    assert(n >= 0 && n < gpio_list->num_in);
> +    assert(n >= 0);
> +    if (n >= gpio_list->num_in) {
> +        error_setg(&error_abort, "Invalid gpio #%d (of %d) for %s",
> +                   n, gpio_list->num_in, name ? name : "device");
> +    }
>      return gpio_list->in[n];
>  }
>
> --
> 2.13.1
>
>

Re: [Qemu-devel] [PATCH 3/3] hw/core: report an error if invalid gpio is used

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1328 bytes --]

On 06/23/2017 02:25 PM, Eduardo Habkost wrote:
> On Fri, Jun 23, 2017 at 01:45:57PM -0300, Philippe Mathieu-Daudé wrote:
>> then abort calling error_setg()
> 
> I don't understand the reasons for this.  This commit message says
> "what" and "how", but not "why".
> 

>> -    assert(n >= 0 && n < gpio_list->num_in);
>> +    assert(n >= 0);
>> +    if (n >= gpio_list->num_in) {
>> +        error_setg(&error_abort, "Invalid gpio #%d (of %d) for %s",
>> +                   n, gpio_list->num_in, name ? name : "device");
> 
> Why exactly assert() is ok for (n < 0), but not for
> (n >= gpio_list->num_io)?
> 
> If you have reasons to believe (n >= gpio_list->num_in) can be triggered
> by user input, then abort() isn't an appropriate way to handle it.

What's more, error_setg(&error_abort) should not be used.  Yes, a quick
grep for 'error_setg.*error_abort' shows that we have a couple of bad
examples in the tree that should be patched, but it also finds that
include/qapi/error.h states:

 * Please don't error_setg(&error_fatal, ...), use error_report() and
 * exit(), because that's more obvious.
 * Likewise, don't error_setg(&error_abort, ...), use assert().

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 0/3] hw/core: minor fixups

[Philippe Mathieu-Daudé]

Hi Eduardo,

On 06/23/2017 04:45 PM, Eduardo Habkost wrote:
> Do you have a simple way to trigger the error paths addressed by patches 1/3
> and 2/3?

For 1/3 "elf-loader: warn about invalid endianess":

$ wget -q 
https://people.debian.org/~aurel32/qemu/mips/vmlinux-3.2.0-4-4kc-malta

$ file vmlinux-3.2.0-4-4kc-malta
vmlinux-3.2.0-4-4kc-malta: ELF 32-bit MSB executable, MIPS, MIPS32 
version 1 (SYSV), statically linked, 
BuildID[sha1]=66b8748075269e8aedb91d363050f74af8a0ebdd, not stripped

$ qemu-system-mipsel -version
QEMU emulator version 2.8.1(Debian 1:2.8+dfsg-6)
Copyright (c) 2003-2016 Fabrice Bellard and the QEMU Project developers

$ qemu-system-mipsel -kernel vmlinux-3.2.0-4-4kc-malta
qemu: could not load kernel 'vmlinux-3.2.0-4-4kc-malta'

Once applied:

$ mipsel-softmmu/qemu-system-mipsel -kernel vmlinux-3.2.0-4-4kc-malta
vmlinux-3.2.0-4-4kc-malta: wrong endianess
qemu: could not load kernel 'vmlinux-3.2.0-4-4kc-malta'

It could be more verbose/nicer.

I'm doing some dual endianness tests and sometimes it happened I only 
notice I'm stupid enough to load the wrong elf once stepping in gdb...


For 2/3 "fix missing return value in load_image_targphys_as()" I 
extracted it from a WiP branch "unify-arm-mips-loaders" think that if I 
never finish it, at least this one can still be useful for others.
No commits in this branch since 4months so I don't really remember how 
it happens, but looking at rom_add_file() I see:

         fprintf(stderr, "Could not open option rom '%s': %s\n",
                 rom->path, strerror(errno));
         goto err;
...
         fprintf(stderr, "rom: file %-20s: get size error: %s\n",
                 rom->name, strerror(errno));
         goto err;
...
         fprintf(stderr, "rom: file %-20s: read error: rc=%d (expected 
%zd)\n",
                 rom->name, rc, rom->datasize);
         goto err;

So my guess is again I missed something in the command line I used (used 
unfinished bash auto-complete which lead to a directory? use zipped 
rom?) and QEMU was still booting without using the specified rom.

I do remember single stepping there at least 2 times before realize 
again how stupid I was :)

Regards,

Phil.

Re: [Qemu-devel] [PATCH 1/3] elf-loader: warn about invalid endianess

[Michael Tokarev]

23.06.2017 19:45, Philippe Mathieu-Daudé wrote:
> fprintf(stderr) is how errors are reported in this file.

Applied to -trivial (with typo fix), thanks!

/mjt

Re: [Qemu-devel] [PATCH 2/3] hw/core: fix missing return value in load_image_targphys_as()

[Michael Tokarev]

Applied to -trivial, thanks!

/mjt