qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH v2] hw/pci: fixed crash when using rombar=0 with romfile=path for hotplugged devices
@ 2014-10-27 13:44 Marcel Apfelbaum
  2014-10-27 13:50 ` Marcel Apfelbaum
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Marcel Apfelbaum @ 2014-10-27 13:44 UTC (permalink / raw)
  To: qemu-devel; +Cc: pbonzini, alex.williamson, mst

Combining rombar=0 with romfile=<path> is an user error,
silently dropping the romfile is a reasonable thing to do.

Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
---
v1 -> v2:
 After a discussion with Michael, Paolo and Alex, this
 patch silent drops the romfile instead of not allowing
 the hotplug.
 
 An OK from libvirt will be nice.

 hw/pci/pci.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 6ce75aa..cd7a403 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -1776,7 +1776,12 @@ static int pci_qdev_init(DeviceState *qdev)
         pci_dev->romfile = g_strdup(pc->romfile);
         is_default_rom = true;
     }
-    pci_add_option_rom(pci_dev, is_default_rom);
+
+    rc = pci_add_option_rom(pci_dev, is_default_rom);
+    if (rc != 0) {
+        pci_unregister_device(DEVICE(pci_dev));
+        return rc;
+    }
 
     return 0;
 }
@@ -1937,6 +1942,15 @@ static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom)
          * for 0.11 compatibility.
          */
         int class = pci_get_word(pdev->config + PCI_CLASS_DEVICE);
+
+        /*
+         * For hot-plugged devices silently ignore the option ROM
+         * if the rom bar is disabled.
+         */
+        if (DEVICE(pdev)->hotplugged) {
+            return 0;
+        }
+
         if (class == 0x0300) {
             rom_add_vga(pdev->romfile);
         } else {
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH v2] hw/pci: fixed crash when using rombar=0 with romfile=path for hotplugged devices
  2014-10-27 13:44 [Qemu-devel] [PATCH v2] hw/pci: fixed crash when using rombar=0 with romfile=path for hotplugged devices Marcel Apfelbaum
@ 2014-10-27 13:50 ` Marcel Apfelbaum
  2014-10-27 14:52 ` Michael S. Tsirkin
  2014-10-27 14:59 ` Markus Armbruster
  2 siblings, 0 replies; 9+ messages in thread
From: Marcel Apfelbaum @ 2014-10-27 13:50 UTC (permalink / raw)
  To: Eric Blake; +Cc: pbonzini, alex.williamson, qemu-devel, mst

On Mon, 2014-10-27 at 15:44 +0200, Marcel Apfelbaum wrote:
> Combining rombar=0 with romfile=<path> is an user error,
> silently dropping the romfile is a reasonable thing to do.
> 
> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> ---
> v1 -> v2:
>  After a discussion with Michael, Paolo and Alex, this
>  patch silent drops the romfile instead of not allowing
>  the hotplug.
>  
>  An OK from libvirt will be nice.
CC: Erik Blake <eblake@redhat.com>

Thanks,
Marcel

> 
>  hw/pci/pci.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 6ce75aa..cd7a403 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -1776,7 +1776,12 @@ static int pci_qdev_init(DeviceState *qdev)
>          pci_dev->romfile = g_strdup(pc->romfile);
>          is_default_rom = true;
>      }
> -    pci_add_option_rom(pci_dev, is_default_rom);
> +
> +    rc = pci_add_option_rom(pci_dev, is_default_rom);
> +    if (rc != 0) {
> +        pci_unregister_device(DEVICE(pci_dev));
> +        return rc;
> +    }
>  
>      return 0;
>  }
> @@ -1937,6 +1942,15 @@ static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom)
>           * for 0.11 compatibility.
>           */
>          int class = pci_get_word(pdev->config + PCI_CLASS_DEVICE);
> +
> +        /*
> +         * For hot-plugged devices silently ignore the option ROM
> +         * if the rom bar is disabled.
> +         */
> +        if (DEVICE(pdev)->hotplugged) {
> +            return 0;
> +        }
> +
>          if (class == 0x0300) {
>              rom_add_vga(pdev->romfile);
>          } else {

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH v2] hw/pci: fixed crash when using rombar=0 with romfile=path for hotplugged devices
  2014-10-27 13:44 [Qemu-devel] [PATCH v2] hw/pci: fixed crash when using rombar=0 with romfile=path for hotplugged devices Marcel Apfelbaum
  2014-10-27 13:50 ` Marcel Apfelbaum
@ 2014-10-27 14:52 ` Michael S. Tsirkin
  2014-10-27 15:04   ` Marcel Apfelbaum
  2014-10-27 14:59 ` Markus Armbruster
  2 siblings, 1 reply; 9+ messages in thread
From: Michael S. Tsirkin @ 2014-10-27 14:52 UTC (permalink / raw)
  To: Marcel Apfelbaum; +Cc: pbonzini, alex.williamson, qemu-devel

On Mon, Oct 27, 2014 at 03:44:04PM +0200, Marcel Apfelbaum wrote:
> Combining rombar=0 with romfile=<path> is an user error,
> silently dropping the romfile is a reasonable thing to do.
> 
> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>

How about failing adding the device instead?
Return error from pci_add_option_rom, and check at
calling sites?

> ---
> v1 -> v2:
>  After a discussion with Michael, Paolo and Alex, this
>  patch silent drops the romfile instead of not allowing
>  the hotplug.
>  
>  An OK from libvirt will be nice.
> 
>  hw/pci/pci.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 6ce75aa..cd7a403 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -1776,7 +1776,12 @@ static int pci_qdev_init(DeviceState *qdev)
>          pci_dev->romfile = g_strdup(pc->romfile);
>          is_default_rom = true;
>      }
> -    pci_add_option_rom(pci_dev, is_default_rom);
> +
> +    rc = pci_add_option_rom(pci_dev, is_default_rom);
> +    if (rc != 0) {
> +        pci_unregister_device(DEVICE(pci_dev));
> +        return rc;
> +    }
>  
>      return 0;
>  }
> @@ -1937,6 +1942,15 @@ static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom)
>           * for 0.11 compatibility.
>           */
>          int class = pci_get_word(pdev->config + PCI_CLASS_DEVICE);
> +
> +        /*
> +         * For hot-plugged devices silently ignore the option ROM
> +         * if the rom bar is disabled.
> +         */
> +        if (DEVICE(pdev)->hotplugged) {
> +            return 0;
> +        }
> +
>          if (class == 0x0300) {
>              rom_add_vga(pdev->romfile);
>          } else {
> -- 
> 1.8.3.1

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH v2] hw/pci: fixed crash when using rombar=0 with romfile=path for hotplugged devices
  2014-10-27 13:44 [Qemu-devel] [PATCH v2] hw/pci: fixed crash when using rombar=0 with romfile=path for hotplugged devices Marcel Apfelbaum
  2014-10-27 13:50 ` Marcel Apfelbaum
  2014-10-27 14:52 ` Michael S. Tsirkin
@ 2014-10-27 14:59 ` Markus Armbruster
  2014-10-27 15:04   ` Marcel Apfelbaum
  2 siblings, 1 reply; 9+ messages in thread
From: Markus Armbruster @ 2014-10-27 14:59 UTC (permalink / raw)
  To: Marcel Apfelbaum; +Cc: pbonzini, alex.williamson, qemu-devel, mst

Marcel Apfelbaum <marcel.a@redhat.com> writes:

> Combining rombar=0 with romfile=<path> is an user error,
> silently dropping the romfile is a reasonable thing to do.
>
> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> ---
> v1 -> v2:
>  After a discussion with Michael, Paolo and Alex, this
>  patch silent drops the romfile instead of not allowing
>  the hotplug.
>  
>  An OK from libvirt will be nice.
>
>  hw/pci/pci.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 6ce75aa..cd7a403 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -1776,7 +1776,12 @@ static int pci_qdev_init(DeviceState *qdev)
>          pci_dev->romfile = g_strdup(pc->romfile);
>          is_default_rom = true;
>      }
> -    pci_add_option_rom(pci_dev, is_default_rom);
> +
> +    rc = pci_add_option_rom(pci_dev, is_default_rom);
> +    if (rc != 0) {
> +        pci_unregister_device(DEVICE(pci_dev));
> +        return rc;
> +    }
>  
>      return 0;
>  }

Looks like this part isn't covered by the commit message.

Before: errors in pci_add_option_rom() are reported, but the function
succeeds anyway.  After: it fails.  Is this a silent bug fix?

> @@ -1937,6 +1942,15 @@ static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom)
>           * for 0.11 compatibility.
>           */
>          int class = pci_get_word(pdev->config + PCI_CLASS_DEVICE);
> +
> +        /*
> +         * For hot-plugged devices silently ignore the option ROM
> +         * if the rom bar is disabled.
> +         */
> +        if (DEVICE(pdev)->hotplugged) {
> +            return 0;
> +        }
> +
>          if (class == 0x0300) {
>              rom_add_vga(pdev->romfile);
>          } else {

This part is covered by the commit message.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH v2] hw/pci: fixed crash when using rombar=0 with romfile=path for hotplugged devices
  2014-10-27 14:52 ` Michael S. Tsirkin
@ 2014-10-27 15:04   ` Marcel Apfelbaum
  2014-10-27 16:03     ` Michael S. Tsirkin
  0 siblings, 1 reply; 9+ messages in thread
From: Marcel Apfelbaum @ 2014-10-27 15:04 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: pbonzini, alex.williamson, qemu-devel

On Mon, 2014-10-27 at 16:52 +0200, Michael S. Tsirkin wrote:
> On Mon, Oct 27, 2014 at 03:44:04PM +0200, Marcel Apfelbaum wrote:
> > Combining rombar=0 with romfile=<path> is an user error,
> > silently dropping the romfile is a reasonable thing to do.
> > 
> > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> 
> How about failing adding the device instead?
> Return error from pci_add_option_rom, and check at
> calling sites?

This was was the prev version of this patch has done,
we have only one calling site:  pci_qdev_init.
I could tweak the prev version to return error on both
rom_add_vga/rom_add_option, but I was under the impression
that silently drop the romfile was discussion's decision.

I am fine both ways, as it is a user error and hopefully
used correctly by libvirt. I only want to avoid the crash.

Thanks,
Marcel

> 
> > ---
> > v1 -> v2:
> >  After a discussion with Michael, Paolo and Alex, this
> >  patch silent drops the romfile instead of not allowing
> >  the hotplug.
> >  
> >  An OK from libvirt will be nice.
> > 
> >  hw/pci/pci.c | 16 +++++++++++++++-
> >  1 file changed, 15 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index 6ce75aa..cd7a403 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -1776,7 +1776,12 @@ static int pci_qdev_init(DeviceState *qdev)
> >          pci_dev->romfile = g_strdup(pc->romfile);
> >          is_default_rom = true;
> >      }
> > -    pci_add_option_rom(pci_dev, is_default_rom);
> > +
> > +    rc = pci_add_option_rom(pci_dev, is_default_rom);
> > +    if (rc != 0) {
> > +        pci_unregister_device(DEVICE(pci_dev));
> > +        return rc;
> > +    }
> >  
> >      return 0;
> >  }
> > @@ -1937,6 +1942,15 @@ static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom)
> >           * for 0.11 compatibility.
> >           */
> >          int class = pci_get_word(pdev->config + PCI_CLASS_DEVICE);
> > +
> > +        /*
> > +         * For hot-plugged devices silently ignore the option ROM
> > +         * if the rom bar is disabled.
> > +         */
> > +        if (DEVICE(pdev)->hotplugged) {
> > +            return 0;
> > +        }
> > +
> >          if (class == 0x0300) {
> >              rom_add_vga(pdev->romfile);
> >          } else {
> > -- 
> > 1.8.3.1

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH v2] hw/pci: fixed crash when using rombar=0 with romfile=path for hotplugged devices
  2014-10-27 14:59 ` Markus Armbruster
@ 2014-10-27 15:04   ` Marcel Apfelbaum
  2014-10-27 15:54     ` Markus Armbruster
  0 siblings, 1 reply; 9+ messages in thread
From: Marcel Apfelbaum @ 2014-10-27 15:04 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: pbonzini, alex.williamson, qemu-devel, mst

On Mon, 2014-10-27 at 15:59 +0100, Markus Armbruster wrote:
> Marcel Apfelbaum <marcel.a@redhat.com> writes:
> 
> > Combining rombar=0 with romfile=<path> is an user error,
> > silently dropping the romfile is a reasonable thing to do.
> >
> > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > ---
> > v1 -> v2:
> >  After a discussion with Michael, Paolo and Alex, this
> >  patch silent drops the romfile instead of not allowing
> >  the hotplug.
> >  
> >  An OK from libvirt will be nice.
> >
> >  hw/pci/pci.c | 16 +++++++++++++++-
> >  1 file changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index 6ce75aa..cd7a403 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -1776,7 +1776,12 @@ static int pci_qdev_init(DeviceState *qdev)
> >          pci_dev->romfile = g_strdup(pc->romfile);
> >          is_default_rom = true;
> >      }
> > -    pci_add_option_rom(pci_dev, is_default_rom);
> > +
> > +    rc = pci_add_option_rom(pci_dev, is_default_rom);
> > +    if (rc != 0) {
> > +        pci_unregister_device(DEVICE(pci_dev));
> > +        return rc;
> > +    }
> >  
> >      return 0;
> >  }
> 
> Looks like this part isn't covered by the commit message.
> 
> Before: errors in pci_add_option_rom() are reported, but the function
> succeeds anyway.  After: it fails.  Is this a silent bug fix?
Yes, it made sense for V1, less for V2, but I wanted to keep it.
Should I split in 2 patches?

> 
> > @@ -1937,6 +1942,15 @@ static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom)
> >           * for 0.11 compatibility.
> >           */
> >          int class = pci_get_word(pdev->config + PCI_CLASS_DEVICE);
> > +
> > +        /*
> > +         * For hot-plugged devices silently ignore the option ROM
> > +         * if the rom bar is disabled.
> > +         */
> > +        if (DEVICE(pdev)->hotplugged) {
> > +            return 0;
> > +        }
> > +
> >          if (class == 0x0300) {
> >              rom_add_vga(pdev->romfile);
> >          } else {
> 
> This part is covered by the commit message.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH v2] hw/pci: fixed crash when using rombar=0 with romfile=path for hotplugged devices
  2014-10-27 15:04   ` Marcel Apfelbaum
@ 2014-10-27 15:54     ` Markus Armbruster
  0 siblings, 0 replies; 9+ messages in thread
From: Markus Armbruster @ 2014-10-27 15:54 UTC (permalink / raw)
  To: Marcel Apfelbaum; +Cc: pbonzini, alex.williamson, qemu-devel, mst

Marcel Apfelbaum <marcel.a@redhat.com> writes:

> On Mon, 2014-10-27 at 15:59 +0100, Markus Armbruster wrote:
>> Marcel Apfelbaum <marcel.a@redhat.com> writes:
>> 
>> > Combining rombar=0 with romfile=<path> is an user error,
>> > silently dropping the romfile is a reasonable thing to do.
>> >
>> > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
>> > ---
>> > v1 -> v2:
>> >  After a discussion with Michael, Paolo and Alex, this
>> >  patch silent drops the romfile instead of not allowing
>> >  the hotplug.
>> >  
>> >  An OK from libvirt will be nice.
>> >
>> >  hw/pci/pci.c | 16 +++++++++++++++-
>> >  1 file changed, 15 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>> > index 6ce75aa..cd7a403 100644
>> > --- a/hw/pci/pci.c
>> > +++ b/hw/pci/pci.c
>> > @@ -1776,7 +1776,12 @@ static int pci_qdev_init(DeviceState *qdev)
>> >          pci_dev->romfile = g_strdup(pc->romfile);
>> >          is_default_rom = true;
>> >      }
>> > -    pci_add_option_rom(pci_dev, is_default_rom);
>> > +
>> > +    rc = pci_add_option_rom(pci_dev, is_default_rom);
>> > +    if (rc != 0) {
>> > +        pci_unregister_device(DEVICE(pci_dev));
>> > +        return rc;
>> > +    }
>> >  
>> >      return 0;
>> >  }
>> 
>> Looks like this part isn't covered by the commit message.
>> 
>> Before: errors in pci_add_option_rom() are reported, but the function
>> succeeds anyway.  After: it fails.  Is this a silent bug fix?
> Yes, it made sense for V1, less for V2, but I wanted to keep it.
> Should I split in 2 patches?

Yes, please.

[...]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH v2] hw/pci: fixed crash when using rombar=0 with romfile=path for hotplugged devices
  2014-10-27 15:04   ` Marcel Apfelbaum
@ 2014-10-27 16:03     ` Michael S. Tsirkin
  2014-10-27 17:06       ` Eric Blake
  0 siblings, 1 reply; 9+ messages in thread
From: Michael S. Tsirkin @ 2014-10-27 16:03 UTC (permalink / raw)
  To: Marcel Apfelbaum; +Cc: pbonzini, alex.williamson, qemu-devel

On Mon, Oct 27, 2014 at 05:04:03PM +0200, Marcel Apfelbaum wrote:
> On Mon, 2014-10-27 at 16:52 +0200, Michael S. Tsirkin wrote:
> > On Mon, Oct 27, 2014 at 03:44:04PM +0200, Marcel Apfelbaum wrote:
> > > Combining rombar=0 with romfile=<path> is an user error,
> > > silently dropping the romfile is a reasonable thing to do.
> > > 
> > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > 
> > How about failing adding the device instead?
> > Return error from pci_add_option_rom, and check at
> > calling sites?
> 
> This was was the prev version of this patch has done,
> we have only one calling site:  pci_qdev_init.
> I could tweak the prev version to return error on both
> rom_add_vga/rom_add_option, but I was under the impression
> that silently drop the romfile was discussion's decision.
> 
> I am fine both ways, as it is a user error and hopefully
> used correctly by libvirt. I only want to avoid the crash.
> 
> Thanks,
> Marcel

Let's try the non silent error.
If someone is unhappy we can make it silent again, but
if it succeeds we'll have to keep it working forever.


> > > ---
> > > v1 -> v2:
> > >  After a discussion with Michael, Paolo and Alex, this
> > >  patch silent drops the romfile instead of not allowing
> > >  the hotplug.
> > >  
> > >  An OK from libvirt will be nice.
> > > 
> > >  hw/pci/pci.c | 16 +++++++++++++++-
> > >  1 file changed, 15 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > > index 6ce75aa..cd7a403 100644
> > > --- a/hw/pci/pci.c
> > > +++ b/hw/pci/pci.c
> > > @@ -1776,7 +1776,12 @@ static int pci_qdev_init(DeviceState *qdev)
> > >          pci_dev->romfile = g_strdup(pc->romfile);
> > >          is_default_rom = true;
> > >      }
> > > -    pci_add_option_rom(pci_dev, is_default_rom);
> > > +
> > > +    rc = pci_add_option_rom(pci_dev, is_default_rom);
> > > +    if (rc != 0) {
> > > +        pci_unregister_device(DEVICE(pci_dev));
> > > +        return rc;
> > > +    }
> > >  
> > >      return 0;
> > >  }
> > > @@ -1937,6 +1942,15 @@ static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom)
> > >           * for 0.11 compatibility.
> > >           */
> > >          int class = pci_get_word(pdev->config + PCI_CLASS_DEVICE);
> > > +
> > > +        /*
> > > +         * For hot-plugged devices silently ignore the option ROM
> > > +         * if the rom bar is disabled.
> > > +         */
> > > +        if (DEVICE(pdev)->hotplugged) {
> > > +            return 0;
> > > +        }
> > > +
> > >          if (class == 0x0300) {
> > >              rom_add_vga(pdev->romfile);
> > >          } else {
> > > -- 
> > > 1.8.3.1
> 
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH v2] hw/pci: fixed crash when using rombar=0 with romfile=path for hotplugged devices
  2014-10-27 16:03     ` Michael S. Tsirkin
@ 2014-10-27 17:06       ` Eric Blake
  0 siblings, 0 replies; 9+ messages in thread
From: Eric Blake @ 2014-10-27 17:06 UTC (permalink / raw)
  To: Michael S. Tsirkin, Marcel Apfelbaum
  Cc: pbonzini, alex.williamson, qemu-devel

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

On 10/27/2014 10:03 AM, Michael S. Tsirkin wrote:
> On Mon, Oct 27, 2014 at 05:04:03PM +0200, Marcel Apfelbaum wrote:
>> On Mon, 2014-10-27 at 16:52 +0200, Michael S. Tsirkin wrote:
>>> On Mon, Oct 27, 2014 at 03:44:04PM +0200, Marcel Apfelbaum wrote:
>>>> Combining rombar=0 with romfile=<path> is an user error,
>>>> silently dropping the romfile is a reasonable thing to do.
>>>>
>>>> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
>>>
>>> How about failing adding the device instead?
>>> Return error from pci_add_option_rom, and check at
>>> calling sites?
>>
>> This was was the prev version of this patch has done,
>> we have only one calling site:  pci_qdev_init.
>> I could tweak the prev version to return error on both
>> rom_add_vga/rom_add_option, but I was under the impression
>> that silently drop the romfile was discussion's decision.
>>
>> I am fine both ways, as it is a user error and hopefully
>> used correctly by libvirt. I only want to avoid the crash.
>>
>> Thanks,
>> Marcel
> 
> Let's try the non silent error.
> If someone is unhappy we can make it silent again, but
> if it succeeds we'll have to keep it working forever.

I agree - from the libvirt perspective, it is better to be conservative
and have a noisy failure on an incompatible combination, to prove no one
is using it (or give us reason to relax it later), than to accidentally
silently accept it and change from what was requested, and then find out
people were relying on the silent change in semantics.


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2014-10-27 17:06 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-10-27 13:44 [Qemu-devel] [PATCH v2] hw/pci: fixed crash when using rombar=0 with romfile=path for hotplugged devices Marcel Apfelbaum
2014-10-27 13:50 ` Marcel Apfelbaum
2014-10-27 14:52 ` Michael S. Tsirkin
2014-10-27 15:04   ` Marcel Apfelbaum
2014-10-27 16:03     ` Michael S. Tsirkin
2014-10-27 17:06       ` Eric Blake
2014-10-27 14:59 ` Markus Armbruster
2014-10-27 15:04   ` Marcel Apfelbaum
2014-10-27 15:54     ` Markus Armbruster

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).