> Subject: Re: [PATCH v6 02/27] bootindex: add del_boot_device_path function > > On Thu, Sep 04, 2014 at 03:01:41AM +0000, Gonglei (Arei) wrote: > > Hi, > > > > > From: Eduardo Habkost [mailto:ehabk...@redhat.com] > > > Sent: Thursday, September 04, 2014 2:13 AM > > > Subject: Re: [PATCH v6 02/27] bootindex: add del_boot_device_path > function > > > > > > On Wed, Sep 03, 2014 at 06:45:56AM +0000, Gonglei (Arei) wrote: > > > [...] > > > > > > 4. When we hotplug the virtio-net-pci device, only pass > > > > > > virtio-net-pci's > > > pointer > > > > > to > > > > > > del_boot_device_path(). But virtio-net-pci != virtio-net-device, so > > > > > > I add > a > > > > > function > > > > > > named is_same_fw_dev_path() to handle this situation. > > > > > > > > > > When hot-unplugging virtio-net-pci I'd expect we free both > > > > > virtio-net-pci and virtio-net-device (and therefore call > > > > > del_boot_device_path twice, once for each device). Can you check > that? > > > > > > > > > Yes, I can. > > > > > > > > The del_boot_device_path() is called only once, just for virtio-net-pci. > > > > For its child, virtio-net-devcie's resource is cleaned by qbus->child > unrealizing > > > > process, will not call device_finalize(). > > > > > > Then we need to fix this to make sure there's a corresponding > > > del_boot_device_path() call (with the same pointer) to every > > > add_boot_device_path() call, instead of adding a hack to > > > del_boot_device_path(). > > > > > Good idea. We can add functions named $device_finalize_bootindex(), such > as: > > > > static void virtio_net_set_bootindex(Object *obj, Visitor *v, void *opaque, > > const char *name, Error > **errp) > > { > > VirtIONet *n = VIRTIO_NET(obj); > > > > set_bootindex(&n->nic_conf, v, name, errp); > > add_boot_device_path(n->nic_conf.bootindex, > > DEVICE(obj), "/ethernet-phy@0"); > > } > > > > static void virtio_net_finalize_bootindex(Object *obj, const char *name, > > void *opaque) > > { > > del_boot_device_path(DEVICE(obj)); > > } > > ... > > object_property_add(obj, "bootindex", "int", > > virtio_net_get_bootindex, > > virtio_net_set_bootindex, > > virtio_net_finalize_bootindex, dev, NULL); > > > > as the previous email, we lay add_boot_device_path() in > $device_set_bootindex, > > and lay del_boot_device_path() in $device_finalize_bootindex is a good idea > IMO. > > Whatever the approach we use,
Hmm... we just call this function in device_finalize() as I have used. My above explanation is wrong because of a virtio-net's bug. Thanks. > can we have a wrapper to reduce code > duplication? e.g. a: > void device_add_bootindex_property(DeviceState *dev, int32_t *field, const > char *suffix) > function. > > Then instead of reimplementing set/get/finalize functions, device code > could just call something like: > device_add_bootindex_property(dev, &n->nic_conf.bootindex, > "/ethernet-phy@0"); > This way we cannot attach our target that changing bootindex and take effect during vm rebooting. > -- > Eduardo Best regards, -Gonglei
