Patchwork [02/15] virtio-serial: save/load: Ensure nr_ports on src and dest are same.

login
register
mail settings
Submitter Amit Shah
Date March 24, 2010, 2:49 p.m.
Message ID <1269442173-18421-3-git-send-email-amit.shah@redhat.com>
Download mbox | patch
Permalink /patch/48419/
State New
Headers show

Comments

Amit Shah - March 24, 2010, 2:49 p.m.
The number of ports on the source as well as the destination machines
should match. If they don't, it means some ports that got hotplugged on
the source aren't instantiated on the destination. Or that ports that
were hot-unplugged on the source are created on the destination.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
Reported-by: Juan Quintela <quintela@redhat.com>
---
 hw/virtio-serial-bus.c |   18 ++++++++++++++++--
 1 files changed, 16 insertions(+), 2 deletions(-)
Jamie Lokier - March 26, 2010, 1:09 a.m.
Amit Shah wrote:
> The number of ports on the source as well as the destination machines
> should match. If they don't, it means some ports that got hotplugged on
> the source aren't instantiated on the destination. Or that ports that
> were hot-unplugged on the source are created on the destination.

Surely the set of guest-visible ids must match exactly and be checked
(maybe mapped, if they were given in a different order), in which case
counting the number of ports looks redundant.

I.e. is this check hiding the omission of the proper one?

-- Jamie
Amit Shah - March 26, 2010, 2:03 a.m.
On (Fri) Mar 26 2010 [01:09:23], Jamie Lokier wrote:
> Amit Shah wrote:
> > The number of ports on the source as well as the destination machines
> > should match. If they don't, it means some ports that got hotplugged on
> > the source aren't instantiated on the destination. Or that ports that
> > were hot-unplugged on the source are created on the destination.
> 
> Surely the set of guest-visible ids must match exactly and be checked
> (maybe mapped, if they were given in a different order), in which case
> counting the number of ports looks redundant.
> 
> I.e. is this check hiding the omission of the proper one?

Yes, That's added in a later patch (5). That patch changes the guest
abi, so it's separate.

Also, patch 3 checks if all the ports on the src are present on the
dest.

		Amit
Jamie Lokier - March 26, 2010, 4:08 a.m.
Amit Shah wrote:
> On (Fri) Mar 26 2010 [01:09:23], Jamie Lokier wrote:
> > Amit Shah wrote:
> > > The number of ports on the source as well as the destination machines
> > > should match. If they don't, it means some ports that got hotplugged on
> > > the source aren't instantiated on the destination. Or that ports that
> > > were hot-unplugged on the source are created on the destination.
> > 
> > Surely the set of guest-visible ids must match exactly and be checked
> > (maybe mapped, if they were given in a different order), in which case
> > counting the number of ports looks redundant.
> > 
> > I.e. is this check hiding the omission of the proper one?
> 
> Yes, That's added in a later patch (5). That patch changes the guest
> abi, so it's separate.
> 
> Also, patch 3 checks if all the ports on the src are present on the
> dest.

Great, thanks :-)

-- Jamie

Patch

diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index 9a7f0c1..d31e62d 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -402,7 +402,7 @@  static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
 {
     VirtIOSerial *s = opaque;
     VirtIOSerialPort *port;
-    uint32_t max_nr_ports, nr_active_ports;
+    uint32_t max_nr_ports, nr_active_ports, nr_ports;
     unsigned int i;
 
     if (version_id > 2) {
@@ -419,7 +419,21 @@  static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
     /* The config space */
     qemu_get_be16s(f, &s->config.cols);
     qemu_get_be16s(f, &s->config.rows);
-    s->config.nr_ports = qemu_get_be32(f);
+    nr_ports = qemu_get_be32(f);
+
+    if (nr_ports != s->config.nr_ports) {
+        /*
+         * Source hot-plugged/unplugged ports and we don't have all of
+         * them here.
+         *
+         * Note: This condition cannot check for all hotplug/unplug
+         * events: eg, if one port was hot-plugged and one was
+         * unplugged, the nr_ports remains the same but the port id's
+         * would have changed and we won't catch it here. A later
+         * check for !find_port_by_id() will confirm if this happened.
+         */
+        return -EINVAL;
+    }
 
     /* Items in struct VirtIOSerial */