On 09/20/2010 01:14 PM, Anthony Liguori wrote:
Here's what makes sense to me:
1) async device remove + poll device status/removal notification +
remove backend
The management tool needs to determine when the device is gone and
remove the backend.
2) sync device remove + remove backend
Command does not return until device is removed
3) async device and backend removal + poll device/backend removal +
removal notification
One command that removes the device and any associated backend. We
need to indicate to the management layer when this operation is complete.
I think (2) is the most elegant but also the most difficult to
implement today. I think (1) is the least invasive to implement but
has the most management tool complexity. (3) is probably the best
compromise in terms of complexity and ease of implementation.
Just for comparison, your patch does:
4) async device removal + remove backend
Whereas remove backend may or may not cause removal depending on
whether device removal has happened. So it's really async removal but
it doesn't happen deterministically on it's own. What happens if you
call remove backend before starting async device removal? What if the
guest never removes the device? What if a reset happens?
One advantage of (1) is that there is no tricky life cycle
considerations. If we did (3), we would have to think through what
happens if a guest doesn't respond to an unplug request.
BTW, maybe the real problem is that device_del and pci hot unplug
shouldn't be intimately related.
We could have a pci_unplug that requested the device be unplugged.
However, unplugging didn't result in the device being deleted. Instead,
device_del had to be explicitly called after pci_unplug succeeded.
IRL, if I recall correctly, there's a button that you press that sends
the ACPI event to the OS. There usually is an LED associated with the
slot too and once the device has been unplugged by the OS, the LED goes
from red to green (or something like that). At that point, the human
can physically remove the card from the slot.
You can also initiate the unplug from the OS without the ACPI event ever
happening. I suspect that in our current implementation, that means
that we'll automatically delete the device which may have strange
effects on management tools.
So it probably makes sense for our interface to present the same
procedure. What do you think?
Regards,
Anthony Liguori
Regards,
Anthony Liguori
IOW, if device_del returns and the device isn't actually deleted,
that's a bug and addressing it like this just means we'll trip over
it somewhere else.
We'll have the same problem with drive_del.
Let's fix it there as well then.
Regards,
Anthony Liguori
Signed-off-by: Michael S. Tsirkin<m...@redhat.com>
---
net.c | 21 ++++++++++++++++++++-
net.h | 1 +
2 files changed, 21 insertions(+), 1 deletions(-)
diff --git a/net.c b/net.c
index 3d0fde7..10855d1 100644
--- a/net.c
+++ b/net.c
@@ -286,12 +286,31 @@ void qemu_del_vlan_client(VLANClientState *vc)
if (vc->vlan) {
QTAILQ_REMOVE(&vc->vlan->clients, vc, next);
} else {
+ /* Even if client will not be deleted yet, remove it
from list so it
+ * does not appear in monitor. */
+ QTAILQ_REMOVE(&non_vlan_clients, vc, next);
+ /* Detect that guest-visible (NIC) peer is active, and
delay deletion.
+ * */
+ if (vc->peer&& vc->peer->info->type ==
NET_CLIENT_TYPE_NIC) {
+ NICState *nic = DO_UPCAST(NICState, nc, vc->peer);
+ assert(!nic->peer_deleted);
+ nic->peer_deleted = true;
+ return;
+ }
if (vc->send_queue) {
qemu_del_net_queue(vc->send_queue);
}
- QTAILQ_REMOVE(&non_vlan_clients, vc, next);
if (vc->peer) {
vc->peer->peer = NULL;
+ /* If this is a guest-visible (NIC) device,
+ * and peer has already been removed from monitor,
+ * delete it here. */
+ if (vc->info->type == NET_CLIENT_TYPE_NIC) {
+ NICState *nic = DO_UPCAST(NICState, nc, vc);
+ if (nic->peer_deleted) {
+ qemu_del_vlan_client(vc->peer);
+ }
+ }
}
}
diff --git a/net.h b/net.h
index 518cf9c..44c31a9 100644
--- a/net.h
+++ b/net.h
@@ -72,6 +72,7 @@ typedef struct NICState {
VLANClientState nc;
NICConf *conf;
void *opaque;
+ bool peer_deleted;
} NICState;
struct VLANState {
