Re: [Qemu-devel] [PATCH v2 4/7] bootindex: delete bootindex when device is removed

[Gonglei (Arei)]

Hi,

> > >
> > > > +    del_boot_device_path(dev);
> > >
> > > You can call this from device_finalize() instead of placing it into each
> > > individual device.
> > >
> > Maybe put this call in device_finalize is not a good idea.
> > I have three reasons:
> >
> > 1. the device's some memory have been freed before call device_finalize,
> >  such as device->id. It is too later.
> 
> I don't think you even need id. See my reply to v4 2/8.
> 
> But you have a point about being too late: some devices call
> add_boot_device_path() on realize, so those would need to revert the
> operation on unrealize; others do it on init, so they need to do it on
> finalize.
> 
> On either case, I believe an extra check inside device_finalize()
> wouldn't hurt, even if it becomes redundant on some devices.
> 
> 
OK. And I copy your review from v3 2/7, as follows:

> 
> What if the device doesn't have any ID set? I don't see anything on
> add_boot_device_path() ensuring that dev->id is always set.
> 
Yes, the id is not always set. So, I add a check in V4.

> Why you don't just check if i->dev == dev?
>
No, if we check directly i->dev == dev, we will not handle the virtio devices.

For example, the common user configure a virtio-net nic using command line
like " -device virtio-net-pci,netdev=net0,bootindex=3,id=nic1 ". Then the id 
property
will be added for virtio-net-pci device, not virtio-net device which stored in 
the global
fw_boot_order list. So, the i->dev

When common users want to change the bootindex of virtio-net. They only are 
concerned 
that they have configured an id for virtio-net nic card. So, they can pass the 
id to QEMU. But
we should handle those scenes, meanwhile the device object gained by id is 
virtio-net-pci device
not equals i->dev.

> > 2. not every kinds of device can configure bootindex property, such as usb
> >  host adapters. It is a waste and useless for those devices. This is the
> >  main reason.
> 
> I would prefer to waste a few cycles scanning the boot index list every
> time a device is removed, than risking crashing QEMU in case somebody
> forget to add a del_boot_device_path() call.
> 
OK, fine!

Maybe I should do this in device_finalize() as Gerd's previous suggestion, 
like yours. Thanks.

> 
> > 3. virtio-net device's parent is virtio-pci device, which configured id 
> > property,
> >  But the device saved in global fw_boot_order list is virtio-net device have
> not
> >  id property. If we put call del_boot_device_path(dev) in
> virtio_net_device_unrealize
> >  we can delete it from fw_boot_order directly.
> 
> Sorry, I don't understand what you mean here. If virtio-net doesn't have
> an id property, would the current version of del_boot_device_path() even
> work?
> 
Please see above comments.

Thanks for your review!

Best regards,
-Gonglei