[1/2] Fix undefined behaviour

Commit Message

Grzegorz Uriasz April 28, 2020, 6:28 a.m. UTC

Signed-off-by: Grzegorz Uriasz <[e-mail]>
---
 hw/xen/xen_pt_load_rom.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

Comments

Durrant, Paul April 28, 2020, 8:10 a.m. UTC | #1

> -----Original Message-----
> From: Grzegorz Uriasz <[e-mail]>
> Sent: 28 April 2020 07:29
> To: [e-mail]
> Cc: Grzegorz Uriasz <[e-mail]>; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; Stefano Stabellini <[e-mail]>; Anthony
> Perard <[e-mail]>; Paul Durrant <[e-mail]>; [e-mail]
> Subject: [PATCH 1/2] Fix undefined behaviour
> 
> Signed-off-by: Grzegorz Uriasz <[e-mail]>

I think we need more of a commit comment for both this and patch #2 to explain why you are making the changes.

  Paul

Peter Maydell April 28, 2020, 8:58 a.m. UTC | #2

On Tue, 28 Apr 2020 at 08:50, Grzegorz Uriasz <[e-mail]> wrote:
>
> Signed-off-by: Grzegorz Uriasz <[e-mail]>
> ---
>  hw/xen/xen_pt_load_rom.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)

The subject doesn't match the patch contents and there is
no long-form part of the commit message explaining why...

thanks
-- PMM

Artur Puzio April 28, 2020, 9:40 a.m. UTC | #3

On 28.04.2020 10:10, Paul Durrant wrote:
>> -----Original Message-----
>> From: Grzegorz Uriasz <[e-mail]>
>> Sent: 28 April 2020 07:29
>> To: [e-mail]
>> Cc: Grzegorz Uriasz <[e-mail]>; [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; Stefano Stabellini <[e-mail]>; Anthony
>> Perard <[e-mail]>; Paul Durrant <[e-mail]>; [e-mail]
>> Subject: [PATCH 1/2] Fix undefined behaviour
>>
>> Signed-off-by: Grzegorz Uriasz <[e-mail]>
> I think we need more of a commit comment for both this and patch #2 to explain why you are making the changes.
>
>   Paul 

I agree Grzegorz should improve the commit messages. In the mean time
see email with subject "[PATCH 0/2] Fix QEMU crashes when passing IGD to
a guest VM under XEN", it contains quite detailed explanation for both
"Fix undefined behaviour" and "Improve legacy vbios handling" patches.

Artur Puzio

Durrant, Paul April 28, 2020, 12:32 p.m. UTC | #4

> -----Original Message-----
> From: Artur Puzio <[e-mail]>
> Sent: 28 April 2020 10:41
> To: [e-mail]; 'Grzegorz Uriasz' <[e-mail]>; [e-mail]
> Cc: [e-mail]; [e-mail]; [e-mail]; 'Stefano
> Stabellini' <[e-mail]>; 'Anthony Perard' <[e-mail]>; xen-
> [e-mail]
> Subject: Re: [PATCH 1/2] Fix undefined behaviour
> 
> On 28.04.2020 10:10, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Grzegorz Uriasz <[e-mail]>
> >> Sent: 28 April 2020 07:29
> >> To: [e-mail]
> >> Cc: Grzegorz Uriasz <[e-mail]>; [e-mail]; [e-mail];
> >> [e-mail]; [e-mail]; Stefano Stabellini <[e-mail]>;
> Anthony
> >> Perard <[e-mail]>; Paul Durrant <[e-mail]>; [e-mail]
> >> Subject: [PATCH 1/2] Fix undefined behaviour
> >>
> >> Signed-off-by: Grzegorz Uriasz <[e-mail]>
> > I think we need more of a commit comment for both this and patch #2 to explain why you are making
> the changes.
> >
> >   Paul
> 
> I agree Grzegorz should improve the commit messages. In the mean time
> see email with subject "[PATCH 0/2] Fix QEMU crashes when passing IGD to
> a guest VM under XEN", it contains quite detailed explanation for both
> "Fix undefined behaviour" and "Improve legacy vbios handling" patches.
> 

Ok. Can you please make sure maintainers are cc-ed on patch #0 too.

  Paul

Durrant, Paul April 28, 2020, 12:33 p.m. UTC | #5

> -----Original Message-----
> From: Paul Durrant <[e-mail]>
> Sent: 28 April 2020 13:33
> To: 'Artur Puzio' <[e-mail]>; 'Grzegorz Uriasz' <[e-mail]>; [e-mail]
> Cc: [e-mail]; [e-mail]; [e-mail]; 'Stefano
> Stabellini' <[e-mail]>; 'Anthony Perard' <[e-mail]>; xen-
> [e-mail]
> Subject: RE: [PATCH 1/2] Fix undefined behaviour
> 
> > -----Original Message-----
> > From: Artur Puzio <[e-mail]>
> > Sent: 28 April 2020 10:41
> > To: [e-mail]; 'Grzegorz Uriasz' <[e-mail]>; [e-mail]
> > Cc: [e-mail]; [e-mail]; [e-mail]; 'Stefano
> > Stabellini' <[e-mail]>; 'Anthony Perard' <[e-mail]>; xen-
> > [e-mail]
> > Subject: Re: [PATCH 1/2] Fix undefined behaviour
> >
> > On 28.04.2020 10:10, Paul Durrant wrote:
> > >> -----Original Message-----
> > >> From: Grzegorz Uriasz <[e-mail]>
> > >> Sent: 28 April 2020 07:29
> > >> To: [e-mail]
> > >> Cc: Grzegorz Uriasz <[e-mail]>; [e-mail]; [e-mail];
> > >> [e-mail]; [e-mail]; Stefano Stabellini <[e-mail]>;
> > Anthony
> > >> Perard <[e-mail]>; Paul Durrant <[e-mail]>; [e-mail]
> > >> Subject: [PATCH 1/2] Fix undefined behaviour
> > >>
> > >> Signed-off-by: Grzegorz Uriasz <[e-mail]>
> > > I think we need more of a commit comment for both this and patch #2 to explain why you are making
> > the changes.
> > >
> > >   Paul
> >
> > I agree Grzegorz should improve the commit messages. In the mean time
> > see email with subject "[PATCH 0/2] Fix QEMU crashes when passing IGD to
> > a guest VM under XEN", it contains quite detailed explanation for both
> > "Fix undefined behaviour" and "Improve legacy vbios handling" patches.
> >
> 
> Ok. Can you please make sure maintainers are cc-ed on patch #0 too.
> 

Actually they are, sorry. My MUA is playing tricks on me.

  Paul

Patch

diff --git a/hw/xen/xen_pt_load_rom.c b/hw/xen/xen_pt_load_rom.c
index a50a80837e..9f100dc159 100644
--- a/hw/xen/xen_pt_load_rom.c
+++ b/hw/xen/xen_pt_load_rom.c
@@ -38,12 +38,12 @@  void *pci_assign_dev_load_option_rom(PCIDevice *dev,
     fp = fopen(rom_file, "r+");
     if (fp == NULL) {
         if (errno != ENOENT) {
-            error_report("pci-assign: Cannot open %s: %s", rom_file, strerror(errno));
+            warn_report("pci-assign: Cannot open %s: %s", rom_file, strerror(errno));
         }
         return NULL;
     }
     if (fstat(fileno(fp), &st) == -1) {
-        error_report("pci-assign: Cannot stat %s: %s", rom_file, strerror(errno));
+        warn_report("pci-assign: Cannot stat %s: %s", rom_file, strerror(errno));
         goto close_rom;
     }
 
@@ -59,10 +59,9 @@  void *pci_assign_dev_load_option_rom(PCIDevice *dev,
     memset(ptr, 0xff, st.st_size);
 
     if (!fread(ptr, 1, st.st_size, fp)) {
-        error_report("pci-assign: Cannot read from host %s", rom_file);
-        error_printf("Device option ROM contents are probably invalid "
-                     "(check dmesg).\nSkip option ROM probe with rombar=0, "
-                     "or load from file with romfile=\n");
+        warn_report("pci-assign: Cannot read from host %s", rom_file);
+        memory_region_unref(&dev->rom);
+        ptr = NULL;
         goto close_rom;
     }