Re: [Qemu-devel] [PATCH v2] hw/pci: fixed crash when using rombar=0 with romfile=path for hotplugged devices

[Markus Armbruster]

Marcel Apfelbaum <address@hidden> writes:

> On Mon, 2014-10-27 at 15:59 +0100, Markus Armbruster wrote:
>> Marcel Apfelbaum <address@hidden> 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 <address@hidden>
>> > ---
>> > 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.

[...]