[Qemu-devel] [PATCH 00/14] Block-related fixes and cleanups

[Qemu-devel] [PATCH 00/14] Block-related fixes and cleanups

[Markus Armbruster]

I'm working on cleanly separating block device host and guest parts.
I'd like to route all this work through Kevin's block tree.  This is
just preliminaries.

Markus Armbruster (14):
  blockdev: Belatedly remove MAX_DRIVES
  blockdev: Belatedly remove driveopts
  usb: Remove unused usb_device_add() parameter is_hotplug
  ide: Remove useless IDEDeviceInfo members unit, drive
  ide: Remove redundant IDEState member conf
  ide: Split ide_init1() off ide_init2()
  ide: Change ide_init_drive() to require valid dinfo argument
  ide: Split non-qdev code off ide_init2()
  qdev: New qdev_prop_set_string()
  qdev: Don't leak string property value on hot unplug
  ide: Turn drive serial into a qdev property ide-drive.serial
  ide: Fix info qtree for ide-drive.ver
  scsi: Turn drive serial into a qdev property scsi-disk.serial
  scsi: Fix info qtree for scsi-disk.ver

 hw/ide/cmd646.c      |    4 +-
 hw/ide/core.c        |  106 +++++++++++++++++++++++++++++---------------------
 hw/ide/internal.h    |   13 +++---
 hw/ide/isa.c         |    2 +-
 hw/ide/macio.c       |    2 +-
 hw/ide/microdrive.c  |    3 +-
 hw/ide/mmio.c        |    2 +-
 hw/ide/piix.c        |    4 +-
 hw/ide/qdev.c        |   20 ++++++++-
 hw/qdev-properties.c |   11 +++++
 hw/qdev.c            |    6 +++
 hw/qdev.h            |    2 +
 hw/scsi-disk.c       |   24 ++++++++---
 sysemu.h             |    2 -
 vl.c                 |    9 +---
 15 files changed, 135 insertions(+), 75 deletions(-)

[Qemu-devel] [PATCH 01/14] blockdev: Belatedly remove MAX_DRIVES

[Markus Armbruster]

Unused since commit 751c6a17.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 sysemu.h |    1 -
 vl.c     |    2 --
 2 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/sysemu.h b/sysemu.h
index 879446a..063319c 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -176,7 +176,6 @@ typedef struct DriveInfo {
 
 #define MAX_IDE_DEVS	2
 #define MAX_SCSI_DEVS	7
-#define MAX_DRIVES 32
 
 extern QTAILQ_HEAD(drivelist, DriveInfo) drives;
 extern QTAILQ_HEAD(driveoptlist, DriveOpt) driveopts;
diff --git a/vl.c b/vl.c
index 7121cd0..f8d3034 100644
--- a/vl.c
+++ b/vl.c
@@ -172,8 +172,6 @@ int main(int argc, char **argv)
 
 static const char *data_dir;
 const char *bios_name = NULL;
-/* Note: drives_table[MAX_DRIVES] is a dummy block driver if none available
-   to store the VM snapshots */
 struct drivelist drives = QTAILQ_HEAD_INITIALIZER(drives);
 struct driveoptlist driveopts = QTAILQ_HEAD_INITIALIZER(driveopts);
 enum vga_retrace_method vga_retrace_method = VGA_RETRACE_DUMB;
-- 
1.6.6.1

[Qemu-devel] [PATCH 02/14] blockdev: Belatedly remove driveopts

[Markus Armbruster]

Unused since commit 9dfd7c7a.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 sysemu.h |    1 -
 vl.c     |    1 -
 2 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/sysemu.h b/sysemu.h
index 063319c..fd83b7d 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -178,7 +178,6 @@ typedef struct DriveInfo {
 #define MAX_SCSI_DEVS	7
 
 extern QTAILQ_HEAD(drivelist, DriveInfo) drives;
-extern QTAILQ_HEAD(driveoptlist, DriveOpt) driveopts;
 
 extern DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit);
 extern DriveInfo *drive_get_by_id(const char *id);
diff --git a/vl.c b/vl.c
index f8d3034..9283469 100644
--- a/vl.c
+++ b/vl.c
@@ -173,7 +173,6 @@ int main(int argc, char **argv)
 static const char *data_dir;
 const char *bios_name = NULL;
 struct drivelist drives = QTAILQ_HEAD_INITIALIZER(drives);
-struct driveoptlist driveopts = QTAILQ_HEAD_INITIALIZER(driveopts);
 enum vga_retrace_method vga_retrace_method = VGA_RETRACE_DUMB;
 DisplayType display_type = DT_DEFAULT;
 const char* keyboard_layout = NULL;
-- 
1.6.6.1

[Qemu-devel] [PATCH 03/14] usb: Remove unused usb_device_add() parameter is_hotplug

[Markus Armbruster]

Unused since commit b3e461d3.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 vl.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/vl.c b/vl.c
index 9283469..76a9b25 100644
--- a/vl.c
+++ b/vl.c
@@ -1315,7 +1315,7 @@ static void smp_parse(const char *optarg)
 /***********************************************************/
 /* USB devices */
 
-static int usb_device_add(const char *devname, int is_hotplug)
+static int usb_device_add(const char *devname)
 {
     const char *p;
     USBDevice *dev = NULL;
@@ -1367,7 +1367,7 @@ static int usb_device_del(const char *devname)
 static int usb_parse(const char *cmdline)
 {
     int r;
-    r = usb_device_add(cmdline, 0);
+    r = usb_device_add(cmdline);
     if (r < 0) {
         fprintf(stderr, "qemu: could not add USB device '%s'\n", cmdline);
     }
@@ -1377,7 +1377,7 @@ static int usb_parse(const char *cmdline)
 void do_usb_add(Monitor *mon, const QDict *qdict)
 {
     const char *devname = qdict_get_str(qdict, "devname");
-    if (usb_device_add(devname, 1) < 0) {
+    if (usb_device_add(devname) < 0) {
         error_report("could not add USB device '%s'", devname);
     }
 }
-- 
1.6.6.1

[Qemu-devel] [PATCH 04/14] ide: Remove useless IDEDeviceInfo members unit, drive

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/ide/internal.h |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 2efc784..b4554ce 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -464,8 +464,6 @@ typedef int (*ide_qdev_initfn)(IDEDevice *dev);
 struct IDEDeviceInfo {
     DeviceInfo qdev;
     ide_qdev_initfn init;
-    uint32_t unit;
-    DriveInfo *drive;
 };
 
 #define BM_STATUS_DMAING 0x01
-- 
1.6.6.1

[Qemu-devel] [PATCH 05/14] ide: Remove redundant IDEState member conf

[Markus Armbruster]

Commit 428c149b added IDEState member conf to let commit 0009baf1 find
the BlockConf from there.  It exists only for qdev drives, created via
ide_drive_initfn(), not for drives created via ide_init2().

But for a qdev drive, we can just as well reach its IDEDevice, which
contains the BlockConf.  Do that, and revert the parts of commit
428c149b that add IDEState member conf.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/ide/core.c     |   16 +++++++---------
 hw/ide/internal.h |    4 +---
 hw/ide/qdev.c     |    3 +--
 3 files changed, 9 insertions(+), 14 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 066fecb..c3334b1 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -98,6 +98,7 @@ static void ide_identify(IDEState *s)
 {
     uint16_t *p;
     unsigned int oldsize;
+    IDEDevice *dev;
 
     if (s->identify_set) {
 	memcpy(s->io_buffer, s->identify_data, sizeof(s->identify_data));
@@ -165,8 +166,9 @@ static void ide_identify(IDEState *s)
     put_le16(p + 101, s->nb_sectors >> 16);
     put_le16(p + 102, s->nb_sectors >> 32);
     put_le16(p + 103, s->nb_sectors >> 48);
-    if (s->conf && s->conf->physical_block_size)
-        put_le16(p + 106, 0x6000 | get_physical_block_exp(s->conf));
+    dev = s->unit ? s->bus->slave : s->bus->master;
+    if (dev && dev->conf.physical_block_size)
+        put_le16(p + 106, 0x6000 | get_physical_block_exp(&dev->conf));
 
     memcpy(s->identify_data, p, sizeof(s->identify_data));
     s->identify_set = 1;
@@ -2594,8 +2596,7 @@ void ide_bus_reset(IDEBus *bus)
     ide_clear_hob(bus);
 }
 
-void ide_init_drive(IDEState *s, DriveInfo *dinfo, BlockConf *conf,
-        const char *version)
+void ide_init_drive(IDEState *s, DriveInfo *dinfo, const char *version)
 {
     int cylinders, heads, secs;
     uint64_t nb_sectors;
@@ -2620,9 +2621,6 @@ void ide_init_drive(IDEState *s, DriveInfo *dinfo, BlockConf *conf,
         }
         strncpy(s->drive_serial_str, drive_get_serial(s->bs),
                 sizeof(s->drive_serial_str));
-        if (conf) {
-            s->conf = conf;
-        }
     }
     if (strlen(s->drive_serial_str) == 0)
         snprintf(s->drive_serial_str, sizeof(s->drive_serial_str),
@@ -2653,9 +2651,9 @@ void ide_init2(IDEBus *bus, DriveInfo *hd0, DriveInfo *hd1,
         s->sector_write_timer = qemu_new_timer(vm_clock,
                                                ide_sector_write_timer_cb, s);
         if (i == 0)
-            ide_init_drive(s, hd0, NULL, NULL);
+            ide_init_drive(s, hd0, NULL);
         if (i == 1)
-            ide_init_drive(s, hd1, NULL, NULL);
+            ide_init_drive(s, hd1, NULL);
     }
     bus->irq = irq;
 }
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index b4554ce..cf71019 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -398,7 +398,6 @@ struct IDEState {
     /* set for lba48 access */
     uint8_t lba48;
     BlockDriverState *bs;
-    BlockConf *conf;
     char version[9];
     /* ATAPI specific */
     uint8_t sense_key;
@@ -555,8 +554,7 @@ uint32_t ide_data_readw(void *opaque, uint32_t addr);
 void ide_data_writel(void *opaque, uint32_t addr, uint32_t val);
 uint32_t ide_data_readl(void *opaque, uint32_t addr);
 
-void ide_init_drive(IDEState *s, DriveInfo *dinfo, BlockConf *conf,
-    const char *version);
+void ide_init_drive(IDEState *s, DriveInfo *dinfo, const char *version);
 void ide_init2(IDEBus *bus, DriveInfo *hd0, DriveInfo *hd1,
                qemu_irq irq);
 void ide_init_ioport(IDEBus *bus, int iobase, int iobase2);
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index b18693d..9ebb906 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -99,8 +99,7 @@ typedef struct IDEDrive {
 static int ide_drive_initfn(IDEDevice *dev)
 {
     IDEBus *bus = DO_UPCAST(IDEBus, qbus, dev->qdev.parent_bus);
-    ide_init_drive(bus->ifs + dev->unit, dev->conf.dinfo, &dev->conf,
-                   dev->version);
+    ide_init_drive(bus->ifs + dev->unit, dev->conf.dinfo, dev->version);
     return 0;
 }
 
-- 
1.6.6.1

[Qemu-devel] [PATCH 06/14] ide: Split ide_init1() off ide_init2()

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/ide/core.c |   32 +++++++++++++++++---------------
 1 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index c3334b1..443ff10 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2633,27 +2633,29 @@ void ide_init_drive(IDEState *s, DriveInfo *dinfo, const char *version)
     ide_reset(s);
 }
 
+static void ide_init1(IDEBus *bus, int unit, DriveInfo *dinfo)
+{
+    static int drive_serial = 1;
+    IDEState *s = &bus->ifs[unit];
+
+    s->bus = bus;
+    s->unit = unit;
+    s->drive_serial = drive_serial++;
+    s->io_buffer = qemu_blockalign(s->bs, IDE_DMA_BUF_SECTORS*512 + 4);
+    s->io_buffer_total_len = IDE_DMA_BUF_SECTORS*512 + 4;
+    s->smart_selftest_data = qemu_blockalign(s->bs, 512);
+    s->sector_write_timer = qemu_new_timer(vm_clock,
+                                           ide_sector_write_timer_cb, s);
+    ide_init_drive(s, dinfo, NULL);
+}
+
 void ide_init2(IDEBus *bus, DriveInfo *hd0, DriveInfo *hd1,
                qemu_irq irq)
 {
-    IDEState *s;
-    static int drive_serial = 1;
     int i;
 
     for(i = 0; i < 2; i++) {
-        s = bus->ifs + i;
-        s->bus = bus;
-        s->unit = i;
-        s->drive_serial = drive_serial++;
-        s->io_buffer = qemu_blockalign(s->bs, IDE_DMA_BUF_SECTORS*512 + 4);
-        s->io_buffer_total_len = IDE_DMA_BUF_SECTORS*512 + 4;
-        s->smart_selftest_data = qemu_blockalign(s->bs, 512);
-        s->sector_write_timer = qemu_new_timer(vm_clock,
-                                               ide_sector_write_timer_cb, s);
-        if (i == 0)
-            ide_init_drive(s, hd0, NULL);
-        if (i == 1)
-            ide_init_drive(s, hd1, NULL);
+        ide_init1(bus, i, i == 0 ? hd0 : hd1);
     }
     bus->irq = irq;
 }
-- 
1.6.6.1

[Qemu-devel] [PATCH 07/14] ide: Change ide_init_drive() to require valid dinfo argument

[Markus Armbruster]

IDEState members drive_serial_str and version are now left empty until
an actual drive is connected.  Before, they got a default value that
was overwritten when a drive got connected.  Doesn't matter, because
they're used only while a drive is connected.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/ide/core.c |   47 +++++++++++++++++++++++++----------------------
 1 files changed, 25 insertions(+), 22 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 443ff10..f72d37f 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2601,30 +2601,29 @@ void ide_init_drive(IDEState *s, DriveInfo *dinfo, const char *version)
     int cylinders, heads, secs;
     uint64_t nb_sectors;
 
-    if (dinfo && dinfo->bdrv) {
-        s->bs = dinfo->bdrv;
-        bdrv_get_geometry(s->bs, &nb_sectors);
-        bdrv_guess_geometry(s->bs, &cylinders, &heads, &secs);
-        s->cylinders = cylinders;
-        s->heads = heads;
-        s->sectors = secs;
-        s->nb_sectors = nb_sectors;
-        /* The SMART values should be preserved across power cycles
-           but they aren't.  */
-        s->smart_enabled = 1;
-        s->smart_autosave = 1;
-        s->smart_errors = 0;
-        s->smart_selftest_count = 0;
-        if (bdrv_get_type_hint(s->bs) == BDRV_TYPE_CDROM) {
-            s->is_cdrom = 1;
-            bdrv_set_change_cb(s->bs, cdrom_change_cb, s);
-        }
-        strncpy(s->drive_serial_str, drive_get_serial(s->bs),
-                sizeof(s->drive_serial_str));
+    s->bs = dinfo->bdrv;
+    bdrv_get_geometry(s->bs, &nb_sectors);
+    bdrv_guess_geometry(s->bs, &cylinders, &heads, &secs);
+    s->cylinders = cylinders;
+    s->heads = heads;
+    s->sectors = secs;
+    s->nb_sectors = nb_sectors;
+    /* The SMART values should be preserved across power cycles
+       but they aren't.  */
+    s->smart_enabled = 1;
+    s->smart_autosave = 1;
+    s->smart_errors = 0;
+    s->smart_selftest_count = 0;
+    if (bdrv_get_type_hint(s->bs) == BDRV_TYPE_CDROM) {
+        s->is_cdrom = 1;
+        bdrv_set_change_cb(s->bs, cdrom_change_cb, s);
     }
-    if (strlen(s->drive_serial_str) == 0)
+    strncpy(s->drive_serial_str, drive_get_serial(s->bs),
+            sizeof(s->drive_serial_str));
+    if (*s->drive_serial_str) {
         snprintf(s->drive_serial_str, sizeof(s->drive_serial_str),
                  "QM%05d", s->drive_serial);
+    }
     if (version) {
         pstrcpy(s->version, sizeof(s->version), version);
     } else {
@@ -2646,7 +2645,11 @@ static void ide_init1(IDEBus *bus, int unit, DriveInfo *dinfo)
     s->smart_selftest_data = qemu_blockalign(s->bs, 512);
     s->sector_write_timer = qemu_new_timer(vm_clock,
                                            ide_sector_write_timer_cb, s);
-    ide_init_drive(s, dinfo, NULL);
+    if (dinfo) {
+        ide_init_drive(s, dinfo, NULL);
+    } else {
+        ide_reset(s);
+    }
 }
 
 void ide_init2(IDEBus *bus, DriveInfo *hd0, DriveInfo *hd1,
-- 
1.6.6.1

[Qemu-devel] [PATCH 08/14] ide: Split non-qdev code off ide_init2()

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/ide/cmd646.c     |    4 ++--
 hw/ide/core.c       |   30 ++++++++++++++++++++++--------
 hw/ide/internal.h   |    5 +++--
 hw/ide/isa.c        |    2 +-
 hw/ide/macio.c      |    2 +-
 hw/ide/microdrive.c |    3 ++-
 hw/ide/mmio.c       |    2 +-
 hw/ide/piix.c       |    4 ++--
 8 files changed, 34 insertions(+), 18 deletions(-)

diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
index cdcc9bf..559147f 100644
--- a/hw/ide/cmd646.c
+++ b/hw/ide/cmd646.c
@@ -260,8 +260,8 @@ static int pci_cmd646_ide_initfn(PCIDevice *dev)
     irq = qemu_allocate_irqs(cmd646_set_irq, d, 2);
     ide_bus_new(&d->bus[0], &d->dev.qdev);
     ide_bus_new(&d->bus[1], &d->dev.qdev);
-    ide_init2(&d->bus[0], NULL, NULL, irq[0]);
-    ide_init2(&d->bus[1], NULL, NULL, irq[1]);
+    ide_init2(&d->bus[0], irq[0]);
+    ide_init2(&d->bus[1], irq[1]);
 
     vmstate_register(0, &vmstate_ide_pci, d);
     qemu_register_reset(cmd646_reset, d);
diff --git a/hw/ide/core.c b/hw/ide/core.c
index f72d37f..d6381aa 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2632,7 +2632,7 @@ void ide_init_drive(IDEState *s, DriveInfo *dinfo, const char *version)
     ide_reset(s);
 }
 
-static void ide_init1(IDEBus *bus, int unit, DriveInfo *dinfo)
+static void ide_init1(IDEBus *bus, int unit)
 {
     static int drive_serial = 1;
     IDEState *s = &bus->ifs[unit];
@@ -2645,20 +2645,34 @@ static void ide_init1(IDEBus *bus, int unit, DriveInfo *dinfo)
     s->smart_selftest_data = qemu_blockalign(s->bs, 512);
     s->sector_write_timer = qemu_new_timer(vm_clock,
                                            ide_sector_write_timer_cb, s);
-    if (dinfo) {
-        ide_init_drive(s, dinfo, NULL);
-    } else {
-        ide_reset(s);
+}
+
+void ide_init2(IDEBus *bus, qemu_irq irq)
+{
+    int i;
+
+    for(i = 0; i < 2; i++) {
+        ide_init1(bus, i);
+        ide_reset(&bus->ifs[i]);
     }
+    bus->irq = irq;
 }
 
-void ide_init2(IDEBus *bus, DriveInfo *hd0, DriveInfo *hd1,
-               qemu_irq irq)
+/* TODO convert users to qdev and remove */
+void ide_init2_with_non_qdev_drives(IDEBus *bus, DriveInfo *hd0,
+                                    DriveInfo *hd1, qemu_irq irq)
 {
     int i;
+    DriveInfo *dinfo;
 
     for(i = 0; i < 2; i++) {
-        ide_init1(bus, i, i == 0 ? hd0 : hd1);
+        dinfo = i == 0 ? hd0 : hd1;
+        ide_init1(bus, i);
+        if (dinfo) {
+            ide_init_drive(&bus->ifs[i], dinfo, NULL);
+        } else {
+            ide_reset(&bus->ifs[i]);
+        }
     }
     bus->irq = irq;
 }
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index cf71019..6b0024d 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -555,8 +555,9 @@ void ide_data_writel(void *opaque, uint32_t addr, uint32_t val);
 uint32_t ide_data_readl(void *opaque, uint32_t addr);
 
 void ide_init_drive(IDEState *s, DriveInfo *dinfo, const char *version);
-void ide_init2(IDEBus *bus, DriveInfo *hd0, DriveInfo *hd1,
-               qemu_irq irq);
+void ide_init2(IDEBus *bus, qemu_irq irq);
+void ide_init2_with_non_qdev_drives(IDEBus *bus, DriveInfo *hd0,
+                                    DriveInfo *hd1, qemu_irq irq);
 void ide_init_ioport(IDEBus *bus, int iobase, int iobase2);
 
 /* hw/ide/qdev.c */
diff --git a/hw/ide/isa.c b/hw/ide/isa.c
index dff7c79..b6c6347 100644
--- a/hw/ide/isa.c
+++ b/hw/ide/isa.c
@@ -70,7 +70,7 @@ static int isa_ide_initfn(ISADevice *dev)
     ide_bus_new(&s->bus, &s->dev.qdev);
     ide_init_ioport(&s->bus, s->iobase, s->iobase2);
     isa_init_irq(dev, &s->irq, s->isairq);
-    ide_init2(&s->bus, NULL, NULL, s->irq);
+    ide_init2(&s->bus, s->irq);
     vmstate_register(0, &vmstate_ide_isa, s);
     return 0;
 };
diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index 639f3f6..f76c0fa 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -314,7 +314,7 @@ int pmac_ide_init (DriveInfo **hd_table, qemu_irq irq,
     int pmac_ide_memory;
 
     d = qemu_mallocz(sizeof(MACIOIDEState));
-    ide_init2(&d->bus, hd_table[0], hd_table[1], irq);
+    ide_init2_with_non_qdev_drives(&d->bus, hd_table[0], hd_table[1], irq);
 
     if (dbdma)
         DBDMA_register_channel(dbdma, channel, dma_irq, pmac_ide_transfer, pmac_ide_flush, d);
diff --git a/hw/ide/microdrive.c b/hw/ide/microdrive.c
index bfdb8c8..a7beac5 100644
--- a/hw/ide/microdrive.c
+++ b/hw/ide/microdrive.c
@@ -539,7 +539,8 @@ PCMCIACardState *dscm1xxxx_init(DriveInfo *bdrv)
     md->card.cis = dscm1xxxx_cis;
     md->card.cis_len = sizeof(dscm1xxxx_cis);
 
-    ide_init2(&md->bus, bdrv, NULL, qemu_allocate_irqs(md_set_irq, md, 1)[0]);
+    ide_init2_with_non_qdev_drives(&md->bus, bdrv, NULL,
+                                   qemu_allocate_irqs(md_set_irq, md, 1)[0]);
     md->bus.ifs[0].is_cf = 1;
     md->bus.ifs[0].mdata_size = METADATA_SIZE;
     md->bus.ifs[0].mdata_storage = (uint8_t *) qemu_mallocz(METADATA_SIZE);
diff --git a/hw/ide/mmio.c b/hw/ide/mmio.c
index cca883f..e75cccf 100644
--- a/hw/ide/mmio.c
+++ b/hw/ide/mmio.c
@@ -125,7 +125,7 @@ void mmio_ide_init (target_phys_addr_t membase, target_phys_addr_t membase2,
     MMIOState *s = qemu_mallocz(sizeof(MMIOState));
     int mem1, mem2;
 
-    ide_init2(&s->bus, hd0, hd1, irq);
+    ide_init2_with_non_qdev_drives(&s->bus, hd0, hd1, irq);
 
     s->shift = shift;
 
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index 4fa3851..dad6e86 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -135,8 +135,8 @@ static int pci_piix_ide_initfn(PCIIDEState *d)
     ide_init_ioport(&d->bus[0], 0x1f0, 0x3f6);
     ide_init_ioport(&d->bus[1], 0x170, 0x376);
 
-    ide_init2(&d->bus[0], NULL, NULL, isa_reserve_irq(14));
-    ide_init2(&d->bus[1], NULL, NULL, isa_reserve_irq(15));
+    ide_init2(&d->bus[0], isa_reserve_irq(14));
+    ide_init2(&d->bus[1], isa_reserve_irq(15));
     return 0;
 }
 
-- 
1.6.6.1

[Qemu-devel] [PATCH 09/14] qdev: New qdev_prop_set_string()

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/qdev-properties.c |    5 +++++
 hw/qdev.h            |    1 +
 2 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index 9ffdba7..b6ee50f 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -617,6 +617,11 @@ void qdev_prop_set_uint64(DeviceState *dev, const char *name, uint64_t value)
     qdev_prop_set(dev, name, &value, PROP_TYPE_UINT64);
 }
 
+void qdev_prop_set_string(DeviceState *dev, const char *name, char *value)
+{
+    qdev_prop_set(dev, name, &value, PROP_TYPE_STRING);
+}
+
 void qdev_prop_set_drive(DeviceState *dev, const char *name, DriveInfo *value)
 {
     qdev_prop_set(dev, name, &value, PROP_TYPE_DRIVE);
diff --git a/hw/qdev.h b/hw/qdev.h
index a44060e..7c25a94 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -268,6 +268,7 @@ void qdev_prop_set_uint16(DeviceState *dev, const char *name, uint16_t value);
 void qdev_prop_set_uint32(DeviceState *dev, const char *name, uint32_t value);
 void qdev_prop_set_int32(DeviceState *dev, const char *name, int32_t value);
 void qdev_prop_set_uint64(DeviceState *dev, const char *name, uint64_t value);
+void qdev_prop_set_string(DeviceState *dev, const char *name, char *value);
 void qdev_prop_set_chr(DeviceState *dev, const char *name, CharDriverState *value);
 void qdev_prop_set_netdev(DeviceState *dev, const char *name, VLANClientState *value);
 void qdev_prop_set_vlan(DeviceState *dev, const char *name, VLANState *value);
-- 
1.6.6.1

[Qemu-devel] [PATCH 10/14] qdev: Don't leak string property value on hot unplug

[Markus Armbruster]

parse_string() qemu_strdup()s the property value.  It is never freed.
It needs to be freed along with the device.  Otherwise, the value of
scsi-disk property "ver" gets leaked when hot-unplugging the disk, for
instance.

Call new PropertyInfo method free() from qdev_free().  Implement it
for qdev_prop_string.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/qdev-properties.c |    6 ++++++
 hw/qdev.c            |    6 ++++++
 hw/qdev.h            |    1 +
 3 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index b6ee50f..48a6b45 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -260,6 +260,11 @@ static int parse_string(DeviceState *dev, Property *prop, const char *str)
     return 0;
 }
 
+static void free_string(DeviceState *dev, Property *prop)
+{
+    qemu_free(*(char **)qdev_get_prop_ptr(dev, prop));
+}
+
 static int print_string(DeviceState *dev, Property *prop, char *dest, size_t len)
 {
     char **ptr = qdev_get_prop_ptr(dev, prop);
@@ -274,6 +279,7 @@ PropertyInfo qdev_prop_string = {
     .size  = sizeof(char*),
     .parse = parse_string,
     .print = print_string,
+    .free  = free_string,
 };
 
 /* --- drive --- */
diff --git a/hw/qdev.c b/hw/qdev.c
index af17486..09ff6fc 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -334,6 +334,7 @@ void qdev_init_nofail(DeviceState *dev)
 void qdev_free(DeviceState *dev)
 {
     BusState *bus;
+    Property *prop;
 
     if (dev->state == DEV_STATE_INITIALIZED) {
         while (dev->num_child_bus) {
@@ -349,6 +350,11 @@ void qdev_free(DeviceState *dev)
     }
     qemu_unregister_reset(qdev_reset, dev);
     QLIST_REMOVE(dev, sibling);
+    for (prop = dev->info->props; prop && prop->name; prop++) {
+        if (prop->info->free) {
+            prop->info->free(dev, prop);
+        }
+    }
     qemu_free(dev);
 }
 
diff --git a/hw/qdev.h b/hw/qdev.h
index 7c25a94..51a24e2 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -98,6 +98,7 @@ struct PropertyInfo {
     enum PropertyType type;
     int (*parse)(DeviceState *dev, Property *prop, const char *str);
     int (*print)(DeviceState *dev, Property *prop, char *dest, size_t len);
+    void (*free)(DeviceState *dev, Property *prop);
 };
 
 typedef struct GlobalProperty {
-- 
1.6.6.1

[Qemu-devel] [PATCH 11/14] ide: Turn drive serial into a qdev property ide-drive.serial

[Markus Armbruster]

It needs to be a qdev property, because it belongs to the drive's
guest part.

Bonus: info qtree now shows the serial number.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/ide/core.c     |   11 ++++++-----
 hw/ide/internal.h |    4 +++-
 hw/ide/qdev.c     |   16 +++++++++++++++-
 3 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index d6381aa..70af1b6 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2596,7 +2596,8 @@ void ide_bus_reset(IDEBus *bus)
     ide_clear_hob(bus);
 }
 
-void ide_init_drive(IDEState *s, DriveInfo *dinfo, const char *version)
+void ide_init_drive(IDEState *s, DriveInfo *dinfo,
+                    const char *version, const char *serial)
 {
     int cylinders, heads, secs;
     uint64_t nb_sectors;
@@ -2618,9 +2619,9 @@ void ide_init_drive(IDEState *s, DriveInfo *dinfo, const char *version)
         s->is_cdrom = 1;
         bdrv_set_change_cb(s->bs, cdrom_change_cb, s);
     }
-    strncpy(s->drive_serial_str, drive_get_serial(s->bs),
-            sizeof(s->drive_serial_str));
-    if (*s->drive_serial_str) {
+    if (serial && *serial) {
+        strncpy(s->drive_serial_str, serial, sizeof(s->drive_serial_str));
+    } else {
         snprintf(s->drive_serial_str, sizeof(s->drive_serial_str),
                  "QM%05d", s->drive_serial);
     }
@@ -2669,7 +2670,7 @@ void ide_init2_with_non_qdev_drives(IDEBus *bus, DriveInfo *hd0,
         dinfo = i == 0 ? hd0 : hd1;
         ide_init1(bus, i);
         if (dinfo) {
-            ide_init_drive(&bus->ifs[i], dinfo, NULL);
+            ide_init_drive(&bus->ifs[i], dinfo, NULL, dinfo->serial);
         } else {
             ide_reset(&bus->ifs[i]);
         }
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 6b0024d..eef1ee1 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -457,6 +457,7 @@ struct IDEDevice {
     uint32_t unit;
     BlockConf conf;
     char *version;
+    char *serial;
 };
 
 typedef int (*ide_qdev_initfn)(IDEDevice *dev);
@@ -554,7 +555,8 @@ uint32_t ide_data_readw(void *opaque, uint32_t addr);
 void ide_data_writel(void *opaque, uint32_t addr, uint32_t val);
 uint32_t ide_data_readl(void *opaque, uint32_t addr);
 
-void ide_init_drive(IDEState *s, DriveInfo *dinfo, const char *version);
+void ide_init_drive(IDEState *s, DriveInfo *dinfo,
+                    const char *version, const char *serial);
 void ide_init2(IDEBus *bus, qemu_irq irq);
 void ide_init2_with_non_qdev_drives(IDEBus *bus, DriveInfo *hd0,
                                     DriveInfo *hd1, qemu_irq irq);
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index 9ebb906..5f301bc 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -99,7 +99,20 @@ typedef struct IDEDrive {
 static int ide_drive_initfn(IDEDevice *dev)
 {
     IDEBus *bus = DO_UPCAST(IDEBus, qbus, dev->qdev.parent_bus);
-    ide_init_drive(bus->ifs + dev->unit, dev->conf.dinfo, dev->version);
+    IDEState *s = bus->ifs + dev->unit;
+    const char *serial;
+
+    serial = dev->serial;
+    if (!serial) {
+        /* try to fall back to value set with legacy -drive serial=... */
+        serial = drive_get_serial(s->bs);
+    }
+
+    ide_init_drive(s, dev->conf.dinfo, dev->version, serial);
+
+    if (!dev->serial) {
+        dev->serial = qemu_strdup(s->drive_serial_str);
+    }
     return 0;
 }
 
@@ -111,6 +124,7 @@ static IDEDeviceInfo ide_drive_info = {
         DEFINE_PROP_UINT32("unit", IDEDrive, dev.unit, -1),
         DEFINE_BLOCK_PROPERTIES(IDEDrive, dev.conf),
         DEFINE_PROP_STRING("ver",  IDEDrive, dev.version),
+        DEFINE_PROP_STRING("serial",  IDEDrive, dev.serial),
         DEFINE_PROP_END_OF_LIST(),
     }
 };
-- 
1.6.6.1

[Qemu-devel] [PATCH 12/14] ide: Fix info qtree for ide-drive.ver

[Markus Armbruster]

Show the actual default value instead of <null> when the property has
not been set.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/ide/qdev.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index 5f301bc..9b2aa21 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -110,6 +110,9 @@ static int ide_drive_initfn(IDEDevice *dev)
 
     ide_init_drive(s, dev->conf.dinfo, dev->version, serial);
 
+    if (!dev->version) {
+        dev->version = qemu_strdup(s->version);
+    }
     if (!dev->serial) {
         dev->serial = qemu_strdup(s->drive_serial_str);
     }
-- 
1.6.6.1

[Qemu-devel] [PATCH 13/14] scsi: Turn drive serial into a qdev property scsi-disk.serial

[Markus Armbruster]

It needs to be a qdev property, because it belongs to the drive's
guest part.

Bonus: info qtree now shows the serial number.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/scsi-disk.c |   17 +++++++++++++----
 1 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 4d20919..e8c066a 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -66,6 +66,7 @@ struct SCSIDiskState
     uint64_t max_lba;
     QEMUBH *bh;
     char *version;
+    char *serial;
 };
 
 static SCSIDiskReq *scsi_new_request(SCSIDevice *d, uint32_t tag, uint32_t lun)
@@ -359,9 +360,7 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
 
         case 0x80: /* Device serial number, optional */
         {
-            const char *serial = req->dev->conf.dinfo->serial ?
-                req->dev->conf.dinfo->serial : "0";
-            int l = strlen(serial);
+            int l = strlen(s->serial);
 
             if (l > req->cmd.xfer)
                 l = req->cmd.xfer;
@@ -371,7 +370,7 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
             DPRINTF("Inquiry EVPD[Serial number] "
                     "buffer size %zd\n", req->cmd.xfer);
             outbuf[buflen++] = l;
-            memcpy(outbuf+buflen, serial, l);
+            memcpy(outbuf+buflen, s->serial, l);
             buflen += l;
             break;
         }
@@ -1058,6 +1057,15 @@ static int scsi_disk_initfn(SCSIDevice *dev)
     }
     s->bs = s->qdev.conf.dinfo->bdrv;
 
+    if (!s->serial) {
+        if (*dev->conf.dinfo->serial) {
+            /* try to fall back to value set with legacy -drive serial=... */
+            s->serial = qemu_strdup(dev->conf.dinfo->serial);
+        } else {
+            s->serial = qemu_strdup("0");
+        }
+    }
+
     if (bdrv_is_sg(s->bs)) {
         error_report("scsi-disk: unwanted /dev/sg*");
         return -1;
@@ -1090,6 +1098,7 @@ static SCSIDeviceInfo scsi_disk_info = {
     .qdev.props   = (Property[]) {
         DEFINE_BLOCK_PROPERTIES(SCSIDiskState, qdev.conf),
         DEFINE_PROP_STRING("ver",  SCSIDiskState, version),
+        DEFINE_PROP_STRING("serial",  SCSIDiskState, serial),
         DEFINE_PROP_END_OF_LIST(),
     },
 };
-- 
1.6.6.1

[Qemu-devel] [PATCH 14/14] scsi: Fix info qtree for scsi-disk.ver

[Markus Armbruster]

Show the actual default value instead of <null> when the property has
not been set.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/scsi-disk.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index e8c066a..a3559d1 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -462,8 +462,7 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
     }
     memcpy(&outbuf[8], "QEMU    ", 8);
     memset(&outbuf[32], 0, 4);
-    memcpy(&outbuf[32], s->version ? s->version : QEMU_VERSION,
-           MIN(4, strlen(s->version ? s->version : QEMU_VERSION)));
+    memcpy(&outbuf[32], s->version, MIN(4, strlen(s->version)));
     /*
      * We claim conformance to SPC-3, which is required for guests
      * to ask for modern features like READ CAPACITY(16) or the
@@ -1066,6 +1065,10 @@ static int scsi_disk_initfn(SCSIDevice *dev)
         }
     }
 
+    if (!s->version) {
+        s->version = qemu_strdup(QEMU_VERSION);
+    }
+
     if (bdrv_is_sg(s->bs)) {
         error_report("scsi-disk: unwanted /dev/sg*");
         return -1;
-- 
1.6.6.1

[Qemu-devel] Re: [PATCH 00/14] Block-related fixes and cleanups

[Gerd Hoffmann]

On 05/28/10 15:38, Markus Armbruster wrote:
> I'm working on cleanly separating block device host and guest parts.
> I'd like to route all this work through Kevin's block tree.  This is
> just preliminaries.
>
> Markus Armbruster (14):
>    blockdev: Belatedly remove MAX_DRIVES
>    blockdev: Belatedly remove driveopts
>    usb: Remove unused usb_device_add() parameter is_hotplug
>    ide: Remove useless IDEDeviceInfo members unit, drive
>    ide: Remove redundant IDEState member conf
>    ide: Split ide_init1() off ide_init2()
>    ide: Change ide_init_drive() to require valid dinfo argument
>    ide: Split non-qdev code off ide_init2()
>    qdev: New qdev_prop_set_string()
>    qdev: Don't leak string property value on hot unplug
>    ide: Turn drive serial into a qdev property ide-drive.serial
>    ide: Fix info qtree for ide-drive.ver
>    scsi: Turn drive serial into a qdev property scsi-disk.serial
>    scsi: Fix info qtree for scsi-disk.ver

Nice cleanups.

Acked-by: Gerd Hoffmann <kraxel@redhat.com>

cheers,
   Gerd

[Qemu-devel] Re: [PATCH 05/14] ide: Remove redundant IDEState member conf

[Kevin Wolf]

Am 28.05.2010 15:38, schrieb Markus Armbruster:
> Commit 428c149b added IDEState member conf to let commit 0009baf1 find
> the BlockConf from there.  It exists only for qdev drives, created via
> ide_drive_initfn(), not for drives created via ide_init2().
> 
> But for a qdev drive, we can just as well reach its IDEDevice, which
> contains the BlockConf.  Do that, and revert the parts of commit
> 428c149b that add IDEState member conf.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/ide/core.c     |   16 +++++++---------
>  hw/ide/internal.h |    4 +---
>  hw/ide/qdev.c     |    3 +--
>  3 files changed, 9 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 066fecb..c3334b1 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -98,6 +98,7 @@ static void ide_identify(IDEState *s)
>  {
>      uint16_t *p;
>      unsigned int oldsize;
> +    IDEDevice *dev;
>  
>      if (s->identify_set) {
>  	memcpy(s->io_buffer, s->identify_data, sizeof(s->identify_data));
> @@ -165,8 +166,9 @@ static void ide_identify(IDEState *s)
>      put_le16(p + 101, s->nb_sectors >> 16);
>      put_le16(p + 102, s->nb_sectors >> 32);
>      put_le16(p + 103, s->nb_sectors >> 48);
> -    if (s->conf && s->conf->physical_block_size)
> -        put_le16(p + 106, 0x6000 | get_physical_block_exp(s->conf));
> +    dev = s->unit ? s->bus->slave : s->bus->master;
> +    if (dev && dev->conf.physical_block_size)

If this is meant to be the same condition it would be dev->conf && ...

I can't really tell right now if it can happen that either dev or
dev->conf can be NULL here. In a quick attempt I couldn't get either one.

So, which version is correct, or should the first part be dropped
completely?

Kevin

[Qemu-devel] Re: [PATCH 05/14] ide: Remove redundant IDEState member conf

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 28.05.2010 15:38, schrieb Markus Armbruster:
>> Commit 428c149b added IDEState member conf to let commit 0009baf1 find
>> the BlockConf from there.  It exists only for qdev drives, created via
>> ide_drive_initfn(), not for drives created via ide_init2().
>> 
>> But for a qdev drive, we can just as well reach its IDEDevice, which
>> contains the BlockConf.  Do that, and revert the parts of commit
>> 428c149b that add IDEState member conf.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  hw/ide/core.c     |   16 +++++++---------
>>  hw/ide/internal.h |    4 +---
>>  hw/ide/qdev.c     |    3 +--
>>  3 files changed, 9 insertions(+), 14 deletions(-)
>> 
>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>> index 066fecb..c3334b1 100644
>> --- a/hw/ide/core.c
>> +++ b/hw/ide/core.c
>> @@ -98,6 +98,7 @@ static void ide_identify(IDEState *s)
>>  {
>>      uint16_t *p;
>>      unsigned int oldsize;
>> +    IDEDevice *dev;
>>  
>>      if (s->identify_set) {
>>  	memcpy(s->io_buffer, s->identify_data, sizeof(s->identify_data));
>> @@ -165,8 +166,9 @@ static void ide_identify(IDEState *s)
>>      put_le16(p + 101, s->nb_sectors >> 16);
>>      put_le16(p + 102, s->nb_sectors >> 32);
>>      put_le16(p + 103, s->nb_sectors >> 48);
>> -    if (s->conf && s->conf->physical_block_size)
>> -        put_le16(p + 106, 0x6000 | get_physical_block_exp(s->conf));
>> +    dev = s->unit ? s->bus->slave : s->bus->master;
>> +    if (dev && dev->conf.physical_block_size)
>
> If this is meant to be the same condition it would be dev->conf && ...
>
> I can't really tell right now if it can happen that either dev or
> dev->conf can be NULL here. In a quick attempt I couldn't get either one.
>
> So, which version is correct, or should the first part be dropped
> completely?

dev->conf can't ever be null:

struct IDEDevice {
    DeviceState qdev;
    uint32_t unit;
    BlockConf conf;
    char *version;
    char *serial;
};

If s->bus was set up by ide_qdev_init(), then dev cannot be null.  I
*think* it can be null for non-qdev IDE.  Three places still use that.
Maybe it's time to accidentally break them and wait for the screams ;)

Re: [Qemu-devel] Re: [PATCH 00/14] Block-related fixes and cleanups

[Jes Sorensen]

On 05/31/10 11:19, Gerd Hoffmann wrote:
> On 05/28/10 15:38, Markus Armbruster wrote:
>> I'm working on cleanly separating block device host and guest parts.
>> I'd like to route all this work through Kevin's block tree.  This is
>> just preliminaries.
>>
>> Markus Armbruster (14):
>>    blockdev: Belatedly remove MAX_DRIVES
>>    blockdev: Belatedly remove driveopts
>>    usb: Remove unused usb_device_add() parameter is_hotplug
>>    ide: Remove useless IDEDeviceInfo members unit, drive
>>    ide: Remove redundant IDEState member conf
>>    ide: Split ide_init1() off ide_init2()
>>    ide: Change ide_init_drive() to require valid dinfo argument
>>    ide: Split non-qdev code off ide_init2()
>>    qdev: New qdev_prop_set_string()
>>    qdev: Don't leak string property value on hot unplug
>>    ide: Turn drive serial into a qdev property ide-drive.serial
>>    ide: Fix info qtree for ide-drive.ver
>>    scsi: Turn drive serial into a qdev property scsi-disk.serial
>>    scsi: Fix info qtree for scsi-disk.ver
> 
> Nice cleanups.
> 
> Acked-by: Gerd Hoffmann <kraxel@redhat.com>

Was just about to post the same message :)

Cheers,
Jes

[Qemu-devel] Re: [PATCH 07/14] ide: Change ide_init_drive() to require valid dinfo argument

[Kevin Wolf]

Am 28.05.2010 15:38, schrieb Markus Armbruster:
> IDEState members drive_serial_str and version are now left empty until
> an actual drive is connected.  Before, they got a default value that
> was overwritten when a drive got connected.  Doesn't matter, because
> they're used only while a drive is connected.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/ide/core.c |   47 +++++++++++++++++++++++++----------------------
>  1 files changed, 25 insertions(+), 22 deletions(-)
> 
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 443ff10..f72d37f 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -2601,30 +2601,29 @@ void ide_init_drive(IDEState *s, DriveInfo *dinfo, const char *version)
>      int cylinders, heads, secs;
>      uint64_t nb_sectors;
>  
> -    if (dinfo && dinfo->bdrv) {
> -        s->bs = dinfo->bdrv;
> -        bdrv_get_geometry(s->bs, &nb_sectors);
> -        bdrv_guess_geometry(s->bs, &cylinders, &heads, &secs);
> -        s->cylinders = cylinders;
> -        s->heads = heads;
> -        s->sectors = secs;
> -        s->nb_sectors = nb_sectors;
> -        /* The SMART values should be preserved across power cycles
> -           but they aren't.  */
> -        s->smart_enabled = 1;
> -        s->smart_autosave = 1;
> -        s->smart_errors = 0;
> -        s->smart_selftest_count = 0;
> -        if (bdrv_get_type_hint(s->bs) == BDRV_TYPE_CDROM) {
> -            s->is_cdrom = 1;
> -            bdrv_set_change_cb(s->bs, cdrom_change_cb, s);
> -        }
> -        strncpy(s->drive_serial_str, drive_get_serial(s->bs),
> -                sizeof(s->drive_serial_str));
> +    s->bs = dinfo->bdrv;
> +    bdrv_get_geometry(s->bs, &nb_sectors);
> +    bdrv_guess_geometry(s->bs, &cylinders, &heads, &secs);
> +    s->cylinders = cylinders;
> +    s->heads = heads;
> +    s->sectors = secs;
> +    s->nb_sectors = nb_sectors;
> +    /* The SMART values should be preserved across power cycles
> +       but they aren't.  */
> +    s->smart_enabled = 1;
> +    s->smart_autosave = 1;
> +    s->smart_errors = 0;
> +    s->smart_selftest_count = 0;
> +    if (bdrv_get_type_hint(s->bs) == BDRV_TYPE_CDROM) {
> +        s->is_cdrom = 1;
> +        bdrv_set_change_cb(s->bs, cdrom_change_cb, s);
>      }
> -    if (strlen(s->drive_serial_str) == 0)
> +    strncpy(s->drive_serial_str, drive_get_serial(s->bs),
> +            sizeof(s->drive_serial_str));
> +    if (*s->drive_serial_str) {

This should be the other way round. It breaks the serial number in both
cases.

Kevin

[Qemu-devel] Re: [PATCH 07/14] ide: Change ide_init_drive() to require valid dinfo argument

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 28.05.2010 15:38, schrieb Markus Armbruster:
>> IDEState members drive_serial_str and version are now left empty until
>> an actual drive is connected.  Before, they got a default value that
>> was overwritten when a drive got connected.  Doesn't matter, because
>> they're used only while a drive is connected.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  hw/ide/core.c |   47 +++++++++++++++++++++++++----------------------
>>  1 files changed, 25 insertions(+), 22 deletions(-)
>> 
>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>> index 443ff10..f72d37f 100644
>> --- a/hw/ide/core.c
>> +++ b/hw/ide/core.c
>> @@ -2601,30 +2601,29 @@ void ide_init_drive(IDEState *s, DriveInfo *dinfo, const char *version)
>>      int cylinders, heads, secs;
>>      uint64_t nb_sectors;
>>  
>> -    if (dinfo && dinfo->bdrv) {
>> -        s->bs = dinfo->bdrv;
>> -        bdrv_get_geometry(s->bs, &nb_sectors);
>> -        bdrv_guess_geometry(s->bs, &cylinders, &heads, &secs);
>> -        s->cylinders = cylinders;
>> -        s->heads = heads;
>> -        s->sectors = secs;
>> -        s->nb_sectors = nb_sectors;
>> -        /* The SMART values should be preserved across power cycles
>> -           but they aren't.  */
>> -        s->smart_enabled = 1;
>> -        s->smart_autosave = 1;
>> -        s->smart_errors = 0;
>> -        s->smart_selftest_count = 0;
>> -        if (bdrv_get_type_hint(s->bs) == BDRV_TYPE_CDROM) {
>> -            s->is_cdrom = 1;
>> -            bdrv_set_change_cb(s->bs, cdrom_change_cb, s);
>> -        }
>> -        strncpy(s->drive_serial_str, drive_get_serial(s->bs),
>> -                sizeof(s->drive_serial_str));
>> +    s->bs = dinfo->bdrv;
>> +    bdrv_get_geometry(s->bs, &nb_sectors);
>> +    bdrv_guess_geometry(s->bs, &cylinders, &heads, &secs);
>> +    s->cylinders = cylinders;
>> +    s->heads = heads;
>> +    s->sectors = secs;
>> +    s->nb_sectors = nb_sectors;
>> +    /* The SMART values should be preserved across power cycles
>> +       but they aren't.  */
>> +    s->smart_enabled = 1;
>> +    s->smart_autosave = 1;
>> +    s->smart_errors = 0;
>> +    s->smart_selftest_count = 0;
>> +    if (bdrv_get_type_hint(s->bs) == BDRV_TYPE_CDROM) {
>> +        s->is_cdrom = 1;
>> +        bdrv_set_change_cb(s->bs, cdrom_change_cb, s);
>>      }
>> -    if (strlen(s->drive_serial_str) == 0)
>> +    strncpy(s->drive_serial_str, drive_get_serial(s->bs),
>> +            sizeof(s->drive_serial_str));
>> +    if (*s->drive_serial_str) {
>
> This should be the other way round. It breaks the serial number in both
> cases.

I have no idea how that could get past my testing.  I'll respin.
Thanks!

[Qemu-devel] Re: [PATCH 00/14] Block-related fixes and cleanups

[Kevin Wolf]

Am 01.06.2010 20:32, schrieb Markus Armbruster:
> I'm working on cleanly separating block device host and guest parts.
> I'd like to route all this work through Kevin's block tree.  This is
> just preliminaries.
> 
> v2: Don't break IDE serial
> 
> Markus Armbruster (14):
>   blockdev: Belatedly remove MAX_DRIVES
>   blockdev: Belatedly remove driveopts
>   usb: Remove unused usb_device_add() parameter is_hotplug
>   ide: Remove useless IDEDeviceInfo members unit, drive
>   ide: Remove redundant IDEState member conf
>   ide: Split ide_init1() off ide_init2()
>   ide: Change ide_init_drive() to require valid dinfo argument
>   ide: Split non-qdev code off ide_init2()
>   qdev: New qdev_prop_set_string()
>   qdev: Don't leak string property value on hot unplug
>   ide: Turn drive serial into a qdev property ide-drive.serial
>   ide: Fix info qtree for ide-drive.ver
>   scsi: Turn drive serial into a qdev property scsi-disk.serial
>   scsi: Fix info qtree for scsi-disk.ver

Thanks, applied all to the block branch.

Kevin