Re: [Qemu-devel] insmod virtio-blk is broken in qemu 1.0 (was: Re: git-bisect results
On 12/16/2011 06:53 PM, Max Filippov wrote: git bisect says this. I didn't believe it first time, so I ran it twice with a few modifications, and it pointed to the same commit both times ... Richard, could you please elaborate on your testcase and configuration (host/target architecture, command lines, etc). Ok, I've found most of details, what's not clear to me is how you decide whether the build is good or bad. I mean, you need to rebuild qemu on every bisection step, but neither this commit nor the previous or the next one change anything that would compile for x86 targets. Fairly certain this bisect is a red herring. tglx reported this the other day in IRC. He narrowed it down to virtio-serial. He was able to reproduce it both with kvm tools and QEMU. Regards, Anthony Liguori 67882fd177389527510eb36b3f7712011a835545 is the first bad commit commit 67882fd177389527510eb36b3f7712011a835545 Author: Max Filippov Date: Tue Sep 6 03:55:28 2011 +0400 target-xtensa: implement narrow instructions Instructions with op0>= 8 are 2 bytes long, others are 3 bytes long. Signed-off-by: Max Filippov Signed-off-by: Blue Swirl Rich.
Re: [Qemu-devel] insmod virtio-blk is broken in qemu 1.0 (was: Re: git-bisect results
On 12/16/2011 06:07 PM, Richard W.M. Jones wrote: git bisect says this. I didn't believe it first time, so I ran it twice with a few modifications, and it pointed to the same commit both times ... 67882fd177389527510eb36b3f7712011a835545 is the first bad commit commit 67882fd177389527510eb36b3f7712011a835545 Author: Max Filippov Date: Tue Sep 6 03:55:28 2011 +0400 target-xtensa: implement narrow instructions Instructions with op0>= 8 are 2 bytes long, others are 3 bytes long. Signed-off-by: Max Filippov Signed-off-by: Blue Swirl Rich. If you disable virtio-serial on the QEMU command line, does the guest boot successfully? I believe this is a guest kernel regression in virtio-serial. Regards, Anthony Liguori
Re: [Qemu-devel] insmod virtio-blk is broken in qemu 1.0 (was: Re: git-bisect results
On 12/16/2011 06:07 PM, Richard W.M. Jones wrote: git bisect says this. I didn't believe it first time, so I ran it twice with a few modifications, and it pointed to the same commit both times ... Need more details because it doesn't appear to be broken to me. What guest, what's your command line, what guest kernel version, how does it break, etc.. Regards, Anthony Liguori 67882fd177389527510eb36b3f7712011a835545 is the first bad commit commit 67882fd177389527510eb36b3f7712011a835545 Author: Max Filippov Date: Tue Sep 6 03:55:28 2011 +0400 target-xtensa: implement narrow instructions Instructions with op0>= 8 are 2 bytes long, others are 3 bytes long. Signed-off-by: Max Filippov Signed-off-by: Blue Swirl Rich.
Re: [Qemu-devel] insmod virtio-blk is broken in qemu 1.0 (was: Re: git-bisect results (was: Re: qemu.git hangs booting Linux after insmod virtio_blk.ko))
>> git bisect says this. I didn't believe it first time, so I ran it >> twice with a few modifications, and it pointed to the same commit both >> times ... > > Richard, > could you please elaborate on your testcase and configuration > (host/target architecture, command lines, etc). Ok, I've found most of details, what's not clear to me is how you decide whether the build is good or bad. I mean, you need to rebuild qemu on every bisection step, but neither this commit nor the previous or the next one change anything that would compile for x86 targets. >> 67882fd177389527510eb36b3f7712011a835545 is the first bad commit >> commit 67882fd177389527510eb36b3f7712011a835545 >> Author: Max Filippov >> Date: Tue Sep 6 03:55:28 2011 +0400 >> >> target-xtensa: implement narrow instructions >> >> Instructions with op0 >= 8 are 2 bytes long, others are 3 bytes long. >> >> Signed-off-by: Max Filippov >> Signed-off-by: Blue Swirl >> >> Rich. -- Thanks. -- Max
Re: [Qemu-devel] [PATCH] target-arm: Fixed ARMv7-M SHPR access
On 16 December 2011 18:50, Sebastian Huber wrote: > According to "ARMv7-M Architecture Reference Manual" issue D section > "B3.2.10 System Handler Prioriy Register 1, SHPR1", "B3.2.11 System > Handler Prioriy Register 2, SHPR2", and "B3.2.12 System Handler Prioriy > Register 3, SHPR3". This would fix the specific issue of not being able to do byte or halfword accesses to the SHPR registers, but it doesn't do anything about other byte-accessible registers like the CFSR. The right fix for this is for armv7m_nvic to expose a memory region for the system control space which implements byte and halfword accesses, and not to try to indirect everything through a single GIC region. -- PMM
Re: [Qemu-devel] insmod virtio-blk is broken in qemu 1.0 (was: Re: git-bisect results (was: Re: qemu.git hangs booting Linux after insmod virtio_blk.ko))
> git bisect says this. I didn't believe it first time, so I ran it > twice with a few modifications, and it pointed to the same commit both > times ... Richard, could you please elaborate on your testcase and configuration (host/target architecture, command lines, etc). > 67882fd177389527510eb36b3f7712011a835545 is the first bad commit > commit 67882fd177389527510eb36b3f7712011a835545 > Author: Max Filippov > Date: Tue Sep 6 03:55:28 2011 +0400 > >target-xtensa: implement narrow instructions > >Instructions with op0 >= 8 are 2 bytes long, others are 3 bytes long. > >Signed-off-by: Max Filippov >Signed-off-by: Blue Swirl > > Rich. -- Thanks. -- Max
[Qemu-devel] insmod virtio-blk is broken in qemu 1.0 (was: Re: git-bisect results (was: Re: qemu.git hangs booting Linux after insmod virtio_blk.ko))
git bisect says this. I didn't believe it first time, so I ran it twice with a few modifications, and it pointed to the same commit both times ... 67882fd177389527510eb36b3f7712011a835545 is the first bad commit commit 67882fd177389527510eb36b3f7712011a835545 Author: Max Filippov Date: Tue Sep 6 03:55:28 2011 +0400 target-xtensa: implement narrow instructions Instructions with op0 >= 8 are 2 bytes long, others are 3 bytes long. Signed-off-by: Max Filippov Signed-off-by: Blue Swirl Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://et.redhat.com/~rjones/virt-top
Re: [Qemu-devel] [PATCH] w32: QEMU applications with SDL are always GUI applications
Am 16.12.2011 04:24, schrieb TeLeMan: On Sun, Dec 4, 2011 at 05:32, Stefan Weil wrote: Since commit 1d14ffa97eacd3cb722271eaf6f093038396eac4 (in 2005), QEMU applications on W32 don't use the default SDL compiler flags: Instead of a GUI application, a console application is created. This has disadvantages (there is always an empty console window) and no obvious reason, so this patch removes the strange flag modification. The SDL GUI applications still can be run from a console window and even send stdout and stderr to that console by setting environment variable SDL_STDIO_REDIRECT=no. Did you test it? Windows GUI applications can not send stdout to the startup console window unless they create their own console window. I did, but obviously not good enough: in an msys rxvt console the QEMU executables work as I wrote in the commit message. So msys-rxvt and some other applications (SciTE for example) allow running GUI applications with stdio channels. The Windows command prompt (cmd.exe) is different, and so is the normal MSYS console. Here console output does not work, and I also noticed problems when running an emulation with latest QEMU (application hangs). It seems to be difficult to get a solution which works for several scenarios: * It should be possible to create a link which starts an emulation with parameters and only one window (SDL, no extra console window). This needs a GUI application (or is it possible for a console application to suppress or close the console window?). * It must be possible to see stdout and stderr output. Default today: both are written to files in the program directory. This is bad because normally users have no write access there. It also does not allow running more than one emulation with separated output. * It should be possible to get stdout and stderr directly to the console. This is needed for running with curses, and it is useful when asking for -help. * It must be possible to run QEMU executables from cmd.exe. * It should be possible to run QEMU executables from other shells (msys command line, msys rxvt, cygwin command line, ...). What would you suggest? Regards, Stefan Weil
[Qemu-devel] [PATCH v3 2/3] ppce500_pci: remove sysbus_init_mmio_cb2 usage
Expose only one container MemoryRegion to sysbus.
(Peter Maydell's idea)
Signed-off-by: Benoît Canet
---
hw/ppce500_pci.c | 27 ++-
1 files changed, 6 insertions(+), 21 deletions(-)
diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c
index 6232af1..b606206 100644
--- a/hw/ppce500_pci.c
+++ b/hw/ppce500_pci.c
@@ -79,6 +79,7 @@ struct PPCE500PCIState {
uint32_t gasket_time;
qemu_irq irq[4];
/* mmio maps */
+MemoryRegion container;
MemoryRegion iomem;
};
@@ -298,26 +299,6 @@ static const VMStateDescription vmstate_ppce500_pci = {
}
};
-static void e500_pci_map(SysBusDevice *dev, target_phys_addr_t base)
-{
-PCIHostState *h = FROM_SYSBUS(PCIHostState, sysbus_from_qdev(dev));
-PPCE500PCIState *s = DO_UPCAST(PPCE500PCIState, pci_state, h);
-
-sysbus_add_memory(dev, base + PCIE500_CFGADDR, &h->conf_mem);
-sysbus_add_memory(dev, base + PCIE500_CFGDATA, &h->data_mem);
-sysbus_add_memory(dev, base + PCIE500_REG_BASE, &s->iomem);
-}
-
-static void e500_pci_unmap(SysBusDevice *dev, target_phys_addr_t base)
-{
-PCIHostState *h = FROM_SYSBUS(PCIHostState, sysbus_from_qdev(dev));
-PPCE500PCIState *s = DO_UPCAST(PPCE500PCIState, pci_state, h);
-
-sysbus_del_memory(dev, &h->conf_mem);
-sysbus_del_memory(dev, &h->data_mem);
-sysbus_del_memory(dev, &s->iomem);
-}
-
#include "exec-memory.h"
static int e500_pcihost_initfn(SysBusDevice *dev)
@@ -343,13 +324,17 @@ static int e500_pcihost_initfn(SysBusDevice *dev)
pci_create_simple(b, 0, "e500-host-bridge");
+memory_region_init(&s->container, "pci-container", PCIE500_ALL_SIZE);
memory_region_init_io(&h->conf_mem, &pci_host_conf_be_ops, h,
"pci-conf-idx", 4);
memory_region_init_io(&h->data_mem, &pci_host_data_le_ops, h,
"pci-conf-data", 4);
memory_region_init_io(&s->iomem, &e500_pci_reg_ops, s,
"pci.reg", PCIE500_REG_SIZE);
-sysbus_init_mmio_cb2(dev, e500_pci_map, e500_pci_unmap);
+memory_region_add_subregion(&s->container, PCIE500_CFGADDR, &h->conf_mem);
+memory_region_add_subregion(&s->container, PCIE500_CFGDATA, &h->data_mem);
+memory_region_add_subregion(&s->container, PCIE500_REG_BASE, &s->iomem);
+sysbus_init_mmio(dev, &s->container);
return 0;
}
--
1.7.7.4
[Qemu-devel] [PATCH v3 1/3] sh_pci: remove sysbus_init_mmio_cb2 usage
The isa region is not exposed as a sysbus region because the iobr
register contains its address and use it to remap dynamically
the region. (Peter Maydell's idea)
Signed-off-by: Benoît Canet
---
hw/r2d.c| 14 --
hw/sh_pci.c | 29 -
2 files changed, 16 insertions(+), 27 deletions(-)
diff --git a/hw/r2d.c b/hw/r2d.c
index 9b6fcba..6e1f71c 100644
--- a/hw/r2d.c
+++ b/hw/r2d.c
@@ -231,6 +231,8 @@ static void r2d_init(ram_addr_t ram_size,
qemu_irq *irq;
DriveInfo *dinfo;
int i;
+DeviceState *dev;
+SysBusDevice *busdev;
MemoryRegion *address_space_mem = get_system_memory();
if (!cpu_model)
@@ -252,8 +254,16 @@ static void r2d_init(ram_addr_t ram_size,
/* Register peripherals */
s = sh7750_init(env, address_space_mem);
irq = r2d_fpga_init(address_space_mem, 0x0400, sh7750_irl(s));
-sysbus_create_varargs("sh_pci", 0x1e20, irq[PCI_INTA], irq[PCI_INTB],
- irq[PCI_INTC], irq[PCI_INTD], NULL);
+
+dev = qdev_create(NULL, "sh_pci");
+busdev = sysbus_from_qdev(dev);
+qdev_init_nofail(dev);
+sysbus_mmio_map(busdev, 0, P4ADDR(0x1e20));
+sysbus_mmio_map(busdev, 1, A7ADDR(0x1e20));
+sysbus_connect_irq(busdev, 0, irq[PCI_INTA]);
+sysbus_connect_irq(busdev, 1, irq[PCI_INTB]);
+sysbus_connect_irq(busdev, 2, irq[PCI_INTC]);
+sysbus_connect_irq(busdev, 3, irq[PCI_INTD]);
sm501_init(address_space_mem, 0x1000, SM501_VRAM_SIZE,
irq[SM501], serial_hds[2]);
diff --git a/hw/sh_pci.c b/hw/sh_pci.c
index 86f468e..d4d028d 100644
--- a/hw/sh_pci.c
+++ b/hw/sh_pci.c
@@ -110,29 +110,6 @@ static void sh_pci_set_irq(void *opaque, int irq_num, int
level)
qemu_set_irq(pic[irq_num], level);
}
-static void sh_pci_map(SysBusDevice *dev, target_phys_addr_t base)
-{
-SHPCIState *s = FROM_SYSBUS(SHPCIState, dev);
-
-memory_region_add_subregion(get_system_memory(),
-P4ADDR(base),
-&s->memconfig_p4);
-memory_region_add_subregion(get_system_memory(),
-A7ADDR(base),
-&s->memconfig_a7);
-s->iobr = 0xfe24;
-memory_region_add_subregion(get_system_memory(), s->iobr, &s->isa);
-}
-
-static void sh_pci_unmap(SysBusDevice *dev, target_phys_addr_t base)
-{
-SHPCIState *s = FROM_SYSBUS(SHPCIState, dev);
-
-memory_region_del_subregion(get_system_memory(), &s->memconfig_p4);
-memory_region_del_subregion(get_system_memory(), &s->memconfig_a7);
-memory_region_del_subregion(get_system_memory(), &s->isa);
-}
-
static int sh_pci_init_device(SysBusDevice *dev)
{
SHPCIState *s;
@@ -153,9 +130,11 @@ static int sh_pci_init_device(SysBusDevice *dev)
memory_region_init_alias(&s->memconfig_a7, "sh_pci.2", &s->memconfig_p4,
0, 0x224);
isa_mmio_setup(&s->isa, 0x4);
-sysbus_init_mmio_cb2(dev, sh_pci_map, sh_pci_unmap);
+sysbus_init_mmio(dev, &s->memconfig_p4);
sysbus_init_mmio(dev, &s->memconfig_a7);
-sysbus_init_mmio(dev, &s->isa);
+s->iobr = 0xfe24;
+memory_region_add_subregion(get_system_memory(), s->iobr, &s->isa);
+
s->dev = pci_create_simple(s->bus, PCI_DEVFN(0, 0), "sh_pci_host");
return 0;
}
--
1.7.7.4
[Qemu-devel] [PATCH v3 3/3] sysbus: remove sysbus_init_mmio_cb2
This function is not longer in use so remove it.
Signed-off-by: Benoît Canet
---
hw/sysbus.c | 16
hw/sysbus.h |5 -
2 files changed, 0 insertions(+), 21 deletions(-)
diff --git a/hw/sysbus.c b/hw/sysbus.c
index b315a8c..81a57bd 100644
--- a/hw/sysbus.c
+++ b/hw/sysbus.c
@@ -53,8 +53,6 @@ void sysbus_mmio_map(SysBusDevice *dev, int n,
target_phys_addr_t addr)
if (dev->mmio[n].memory) {
memory_region_del_subregion(get_system_memory(),
dev->mmio[n].memory);
-} else if (dev->mmio[n].unmap) {
-dev->mmio[n].unmap(dev, dev->mmio[n].addr);
}
}
dev->mmio[n].addr = addr;
@@ -62,8 +60,6 @@ void sysbus_mmio_map(SysBusDevice *dev, int n,
target_phys_addr_t addr)
memory_region_add_subregion(get_system_memory(),
addr,
dev->mmio[n].memory);
-} else if (dev->mmio[n].cb) {
-dev->mmio[n].cb(dev, addr);
}
}
@@ -89,18 +85,6 @@ void sysbus_pass_irq(SysBusDevice *dev, SysBusDevice *target)
}
}
-void sysbus_init_mmio_cb2(SysBusDevice *dev,
- mmio_mapfunc cb, mmio_mapfunc unmap)
-{
-int n;
-
-assert(dev->num_mmio < QDEV_MAX_MMIO);
-n = dev->num_mmio++;
-dev->mmio[n].addr = -1;
-dev->mmio[n].cb = cb;
-dev->mmio[n].unmap = unmap;
-}
-
void sysbus_init_mmio(SysBusDevice *dev, MemoryRegion *memory)
{
int n;
diff --git a/hw/sysbus.h b/hw/sysbus.h
index 9bac582..2f4025b 100644
--- a/hw/sysbus.h
+++ b/hw/sysbus.h
@@ -11,7 +11,6 @@
#define QDEV_MAX_IRQ 256
typedef struct SysBusDevice SysBusDevice;
-typedef void (*mmio_mapfunc)(SysBusDevice *dev, target_phys_addr_t addr);
struct SysBusDevice {
DeviceState qdev;
@@ -21,8 +20,6 @@ struct SysBusDevice {
int num_mmio;
struct {
target_phys_addr_t addr;
-mmio_mapfunc cb;
-mmio_mapfunc unmap;
MemoryRegion *memory;
} mmio[QDEV_MAX_MMIO];
int num_pio;
@@ -43,8 +40,6 @@ typedef struct {
void sysbus_register_dev(const char *name, size_t size, sysbus_initfn init);
void sysbus_register_withprop(SysBusDeviceInfo *info);
void *sysbus_new(void);
-void sysbus_init_mmio_cb2(SysBusDevice *dev,
- mmio_mapfunc cb, mmio_mapfunc unmap);
void sysbus_init_mmio(SysBusDevice *dev, MemoryRegion *memory);
MemoryRegion *sysbus_mmio_get_region(SysBusDevice *dev, int n);
void sysbus_init_irq(SysBusDevice *dev, qemu_irq *p);
--
1.7.7.4
[Qemu-devel] [PATCH v3 0/3] remove sysbus_init_mmio_cb2 usage
These patches remove sysbus_init_mmio_cb2 usage from the codebase. v2: dont't change ppce500 initialisation method (peter) v3: remove unused variable (anthony) Benoît Canet (3): sh_pci: remove sysbus_init_mmio_cb2 usage ppce500_pci: remove sysbus_init_mmio_cb2 usage sysbus: remove sysbus_init_mmio_cb2 hw/ppce500_pci.c | 27 ++- hw/r2d.c | 14 -- hw/sh_pci.c | 29 - hw/sysbus.c | 16 hw/sysbus.h |5 - 5 files changed, 22 insertions(+), 69 deletions(-) -- 1.7.7.4
Re: [Qemu-devel] [PATCH] [RFC] Remove ppc_newworld/ppc_oldworld
(something seems odd - I only got Andreas' reply, not the original mail) On 16.12.2011, at 21:06, Andreas Färber wrote: > Am 16.12.2011 19:08, schrieb Anthony Liguori: >> I notice that these two machines have seem to have never really been touched >> other than tree-wide refactoring since their introduction. Googling for the >> machine types doesn't hit any user questions or comments about the machine >> types. >> >> For the most part, the devices haven't been converted to qdev and are >> actually >> the only remaining PCI devices that haven't been. True point. I just find it frustrating to work on something so it works the same way as before again. Maybe someone a bit less spoiled could convert them over? >> >> To me this indicates that the code currently isn't being used by anyone. I >> can do the qdev conversions if it is, but as far as I can tell, it's just bit >> rotting right now. It's the default machine. Admittedly it's not the greatest and best maintained code in the world, but I guess that applies for multiple corners of QEMU. >> >> Is this accurate? Can we remove this code? If there is future interest >> here, >> it's easy enough to revert this, fix up the code, and resubmit. > > No. There's an easy explanation why you might not find much mentions of > the machine names: g3beige (ppc_oldworld.c) is the default for > qemu-system-ppc, and mac99 (ppc_newworld.c) is the default for > qemu-system-ppc64. Yeah. We talked about moving the default for ppc64 to pseries. At that point we could start to talk about dropping -M mac99 (which is in horrible shape). For -M g3beige, I don't see an alternative popping up atm. It's the only sane target we have for book3s_32 guests. Alex
Re: [Qemu-devel] git-bisect results (was: Re: qemu.git hangs booting Linux after insmod virtio_blk.ko)
On Fri, Sep 30, 2011 at 05:51:52PM +0100, Richard W.M. Jones wrote: > On Fri, Sep 30, 2011 at 02:11:48PM +0100, Richard W.M. Jones wrote: > > > > I've not looked into this at all, it's just a report that something > > seems to be "up". I will try to git bisect this later if no one spots > > anything obvious. > > > > The next operation after insmod virtio_blk would be insmod_virtio_net. > > > > Guest kernel is a Fedora kernel, version 3.1.0-0.rc6.git0.3.fc16.x86_64. > > > > The ImExPS/2 message may or may not be coincidental. > > git-bisect points to: > > d67c3f2cd92aed2247bfa8a9da61a902b7b2ff09 is the first bad commit > commit d67c3f2cd92aed2247bfa8a9da61a902b7b2ff09 > Author: Gerd Hoffmann > Date: Wed Aug 10 17:34:13 2011 +0200 > > seabios: update to master > > commit 8e301472e324b6d6496d8b4ffc66863e99d7a505 > > user visible changes in seabios: > * ahci is enabled by default (and thus in this build). > * bootorder support for ahci. > * two-pass pci allocator (orders bars by size for better packing). > > Signed-off-by: Gerd Hoffmann > > :04 04 76eb0c81b76563b55cb2bb5c484ccd48b8cfcded > 5ec0d65d3a763a5566fe1f4c86269cad6d671020 Mpc-bios > :04 04 a5a7ea6e297c1e7490b0a2c28a06ce56e5be9449 > 78adb664d3ea82f1a4dd5ec239887ac5b0168a7f Mroms > bisect run success > > Rich. > > [...] > > [0.248188] Initializing network drop monitor service > > [0.402221] Freeing unused kernel memory: 940k freed > > [0.403955] Write protecting the kernel read-only data: 10240k > > [0.415286] Freeing unused kernel memory: 1260k freed > > [0.421039] Freeing unused kernel memory: 1584k freed > > febootstrap: mounting /proc > > febootstrap: uptime: 0.42 0.16 > > febootstrap: ext2 mini initrd starting up > > febootstrap: mounting /sys > > febootstrap: internal insmod libcrc32c.ko > > febootstrap: internal insmod crc32c-intel.ko > > insmod: init_module: crc32c-intel.ko: No such device > > febootstrap: internal insmod crc-itu-t.ko > > febootstrap: internal insmod crc-ccitt.ko > > febootstrap: internal insmod crc7.ko > > febootstrap: internal insmod crc8.ko > > febootstrap: internal insmod scsi_transport_spi.ko > > febootstrap: internal insmod sym53c8xx.ko > > febootstrap: internal insmod rfkill.ko > > febootstrap: internal insmod sparse-keymap.ko > > febootstrap: internal insmod ideapad-laptop.ko > > insmod: init_module: ideapad-laptop.ko: No such device > > febootstrap: internal insmod virtio_balloon.ko > > febootstrap: internal insmod virtio-rng.ko > > febootstrap: internal insmod virtio_blk.ko > > [0.669479] input: ImExPS/2 Generic Explorer Mouse as > > /devices/platform/i8042/serio1/input/input1 This bug, or something that looks very much like it, seems to have turned up again in qemu 1.0. I've had several reports and have opened a bug: https://bugzilla.redhat.com/show_bug.cgi?id=768508 Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones New in Fedora 11: Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 70 libraries supprt'd http://fedoraproject.org/wiki/MinGW http://www.annexia.org/fedora_mingw
Re: [Qemu-devel] [Bug 902306] [NEW] qemu-user -static variants require shared libraries
On Fri, Dec 09, 2011 at 08:20:16PM -, Michael Roth wrote: > On 12/09/2011 01:39 PM, Vagrant Cascadian wrote: > > > > /usr/lib/gcc/i486-linux-gnu/4.6/../../../i386-linux-gnu/libglib-2.0.a(gutils.o): > > In function `g_get_any_init_do': > >(.text+0xbbb): warning: Using 'getpwuid_r' in statically linked > > applications requires at runtime the shared libraries from the g > >libc version used for linking > > > > for a full log, see: > > We introduced a glib2.0 dependency in QEMU 0.15. I think this just a > result of glib introducing a much larger static build chain dependency. it works fine with qemu 0.15.1, even when built with the same versions of glib2.0 as as qemu 1.x branches. so that would seem a bit odd... unless qemu 1.x is using more of glib2.0 than qemu 0.15. would that then essentially come down to tracking down all of glib2.0's build dependencies and installing those as well? > I'm not sure if glib can be decoupled for usermode emulation, those it > at least seems to have escaped the malloc()->g_malloc() conversion so > maybe there were plans for that... > > But currently at least it's considered a hard general dependency. hrm. would like to see it working again, though it's a bit over my head. live well, vagrant -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/902306 Title: qemu-user -static variants require shared libraries Status in QEMU: New Status in “qemu” package in Debian: New Bug description: somehwere in the qemu 1.0 series, the qemu-user static variants started issuing build warnings like so: /usr/lib/gcc/i486-linux-gnu/4.6/../../../i386-linux-gnu/libglib-2.0.a(gutils.o): In function `g_get_any_init_do': (.text+0xe37): warning: Using 'getpwuid' in statically linked applications requires at runtime the shared libraries from the gli bc version used for linking /usr/lib/gcc/i486-linux-gnu/4.6/../../../i386-linux-gnu/libglib-2.0.a(gutils.o): In function `g_get_any_init_do': (.text+0xe2a): warning: Using 'setpwent' in statically linked applications requires at runtime the shared libraries from the gli bc version used for linking /usr/lib/gcc/i486-linux-gnu/4.6/../../../i386-linux-gnu/libglib-2.0.a(gutils.o): In function `g_get_any_init_do': (.text+0xe40): warning: Using 'endpwent' in statically linked applications requires at runtime the shared libraries from the gli bc version used for linking /usr/lib/gcc/i486-linux-gnu/4.6/../../../i386-linux-gnu/libglib-2.0.a(gutils.o): In function `g_get_any_init_do': (.text+0xb7a): warning: Using 'getpwnam_r' in statically linked applications requires at runtime the shared libraries from the g libc version used for linking /usr/lib/gcc/i486-linux-gnu/4.6/../../../i386-linux-gnu/libglib-2.0.a(gutils.o): In function `g_get_any_init_do': (.text+0xbbb): warning: Using 'getpwuid_r' in statically linked applications requires at runtime the shared libraries from the g libc version used for linking for a full log, see: https://buildd.debian.org/status/fetch.php?pkg=qemu&arch=amd64&ver=1.0~rc4%2Bdfsg-1&stamp=1322591568 i've also tested with qemu/master from today (commit 217bfb445b54db618a30f3a39170bebd9fd9dbf2), and it has the same issue. This seems to cause adduser, addgroup, etc. to fail in cross- architecture chroots that use statically built qemu-user binaries to emulate the foreign architecture. Older versions (0.12-0.15, at least) didn't seem to have this issue. live well, vagrant To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/902306/+subscriptions
[Qemu-devel] proposta soluzioni web.
Salve, vorrei cogliere questa occasione per presentarmi. Il mio nome è Andrea e sono la responsabile di Migliorewebsites.com Lavoro al fianco di numerosi siti internet. Gestisco il loro SEO in base alle particolarità dei maggiori motori di ricerca on line, tra cui Google, Bing e Yahoo. Lavorando al sito web di uno dei miei partner lavorativi, ho incrociato your domain. Sono convinta di poter esserle utile per incrementare il traffico, la visibilità e il Page Rank del suo sito. Se interessato, sarei più che felice di elaborare meglio i dettagli della mia proposta di assistenza. Le auguro una buona giornata, Andrea Matera and...@migliorewebsites.com Migliorewebsites.com http://it.linkedin.com/in/andreabmatera
[Qemu-devel] [PATCH] vhost-net: Move asserts to after check for end < start
When migrating a vm using vhost-net we hit the following assertion:
qemu-kvm: /usr/src/packages/BUILD/qemu-kvm-0.15.1/hw/vhost.c:30:
vhost_dev_sync_region: Assertion `start / (0x1000 * (8 *
sizeof(vhost_log_chunk_t))) < dev->log_size' failed.
The cases which the end < start check is intended to catch, such as
for vga video memory, will also likely trigger the assertion.
Reorder the code to handle this correctly.
Signed-off-by: Bruce Rogers
---
hw/vhost.c |4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/vhost.c b/hw/vhost.c
index 0870cb7..7309f71 100644
--- a/hw/vhost.c
+++ b/hw/vhost.c
@@ -26,11 +26,11 @@ static void vhost_dev_sync_region(struct vhost_dev *dev,
vhost_log_chunk_t *to = dev->log + end / VHOST_LOG_CHUNK + 1;
uint64_t addr = (start / VHOST_LOG_CHUNK) * VHOST_LOG_CHUNK;
-assert(end / VHOST_LOG_CHUNK < dev->log_size);
-assert(start / VHOST_LOG_CHUNK < dev->log_size);
if (end < start) {
return;
}
+assert(end / VHOST_LOG_CHUNK < dev->log_size);
+assert(start / VHOST_LOG_CHUNK < dev->log_size);
for (;from < to; ++from) {
vhost_log_chunk_t log;
int bit;
--
1.7.7
Re: [Qemu-devel] [PATCH] [RFC] Remove ppc_newworld/ppc_oldworld
Am 16.12.2011 19:08, schrieb Anthony Liguori: > I notice that these two machines have seem to have never really been touched > other than tree-wide refactoring since their introduction. Googling for the > machine types doesn't hit any user questions or comments about the machine > types. > > For the most part, the devices haven't been converted to qdev and are actually > the only remaining PCI devices that haven't been. > > To me this indicates that the code currently isn't being used by anyone. I > can do the qdev conversions if it is, but as far as I can tell, it's just bit > rotting right now. > > Is this accurate? Can we remove this code? If there is future interest here, > it's easy enough to revert this, fix up the code, and resubmit. No. There's an easy explanation why you might not find much mentions of the machine names: g3beige (ppc_oldworld.c) is the default for qemu-system-ppc, and mac99 (ppc_newworld.c) is the default for qemu-system-ppc64. If there's something ppc you'd like to remove then I would be open about discussing the current ppc_prep.c machine but not its infrastructure (prep_pci etc. - there we started a qdev'ification) as that's needed for 40p and bebox machines. I was holding back any refactorings while MemoryRegion API was a moving target and now QOM didn't seem inviting either. ;) Andreas
[Qemu-devel] [PATCH] target-arm: Fixed ARMv7-M SHPR access
Hello,
this may help to fix Bug 696094.
--
Sebastian Huber, embedded brains GmbH
Address : Obere Lagerstr. 30, D-82178 Puchheim, Germany
Phone : +49 89 18 90 80 79-6
Fax : +49 89 18 90 80 79-9
E-Mail : sebastian.hu...@embedded-brains.de
PGP : Public key available on request.
Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.
>From 0c8e700376cec0c7b5a70f999b5e286efc321423 Mon Sep 17 00:00:00 2001
From: Sebastian Huber
Date: Fri, 16 Dec 2011 19:46:40 +0100
Subject: [PATCH] target-arm: Fixed ARMv7-M SHPR access
According to "ARMv7-M Architecture Reference Manual" issue D section
"B3.2.10 System Handler Prioriy Register 1, SHPR1", "B3.2.11 System
Handler Prioriy Register 2, SHPR2", and "B3.2.12 System Handler Prioriy
Register 3, SHPR3".
Signed-off-by: Sebastian Huber
---
hw/arm_gic.c | 16 ++--
hw/armv7m_nvic.c | 19 ---
2 files changed, 14 insertions(+), 21 deletions(-)
diff --git a/hw/arm_gic.c b/hw/arm_gic.c
index 9b52119..5139d95 100644
--- a/hw/arm_gic.c
+++ b/hw/arm_gic.c
@@ -356,6 +356,11 @@ static uint32_t gic_dist_readb(void *opaque, target_phys_addr_t offset)
if (GIC_TEST_TRIGGER(irq + i))
res |= (2 << (i * 2));
}
+#else
+} else if (0xd18 <= offset && offset < 0xd24) {
+/* System Handler Priority. */
+irq = offset - 0xd14;
+res = GIC_GET_PRIORITY(irq, cpu);
#endif
} else if (offset < 0xfe0) {
goto bad_reg;
@@ -387,7 +392,8 @@ static uint32_t gic_dist_readl(void *opaque, target_phys_addr_t offset)
gic_state *s = (gic_state *)opaque;
uint32_t addr;
addr = offset;
-if (addr < 0x100 || addr > 0xd00)
+if (addr < 0x100 || (addr > 0xd00 && addr != 0xd18 && addr != 0xd1c
+&& addr != 0xd20))
return nvic_readl(s, addr);
#endif
val = gic_dist_readw(opaque, offset);
@@ -528,6 +534,11 @@ static void gic_dist_writeb(void *opaque, target_phys_addr_t offset,
GIC_CLEAR_TRIGGER(irq + i);
}
}
+#else
+} else if (0xd18 <= offset && offset < 0xd24) {
+/* System Handler Priority. */
+irq = offset - 0xd14;
+s->priority1[irq][0] = value & 0xff;
#endif
} else {
/* 0xf00 is only handled for 32-bit writes. */
@@ -553,7 +564,8 @@ static void gic_dist_writel(void *opaque, target_phys_addr_t offset,
#ifdef NVIC
uint32_t addr;
addr = offset;
-if (addr < 0x100 || (addr > 0xd00 && addr != 0xf00)) {
+if (addr < 0x100 || (addr > 0xd00 && addr != 0xd18 && addr != 0xd1c
+&& addr != 0xd20 && addr != 0xf00)) {
nvic_writel(s, addr, value);
return;
}
diff --git a/hw/armv7m_nvic.c b/hw/armv7m_nvic.c
index bf8c3c5..65b575e 100644
--- a/hw/armv7m_nvic.c
+++ b/hw/armv7m_nvic.c
@@ -195,14 +195,6 @@ static uint32_t nvic_readl(void *opaque, uint32_t offset)
case 0xd14: /* Configuration Control. */
/* TODO: Implement Configuration Control bits. */
return 0;
-case 0xd18: case 0xd1c: case 0xd20: /* System Handler Priority. */
-irq = offset - 0xd14;
-val = 0;
-val |= s->gic.priority1[irq++][0];
-val |= s->gic.priority1[irq++][0] << 8;
-val |= s->gic.priority1[irq++][0] << 16;
-val |= s->gic.priority1[irq][0] << 24;
-return val;
case 0xd24: /* System Handler Status. */
val = 0;
if (s->gic.irq_state[ARMV7M_EXCP_MEM].active) val |= (1 << 0);
@@ -335,17 +327,6 @@ static void nvic_writel(void *opaque, uint32_t offset, uint32_t value)
case 0xd14: /* Configuration Control. */
/* TODO: Implement control registers. */
goto bad_reg;
-case 0xd18: case 0xd1c: case 0xd20: /* System Handler Priority. */
-{
-int irq;
-irq = offset - 0xd14;
-s->gic.priority1[irq++][0] = value & 0xff;
-s->gic.priority1[irq++][0] = (value >> 8) & 0xff;
-s->gic.priority1[irq++][0] = (value >> 16) & 0xff;
-s->gic.priority1[irq][0] = (value >> 24) & 0xff;
-gic_update(&s->gic);
-}
-break;
case 0xd24: /* System Handler Control. */
/* TODO: Real hardware allows you to set/clear the active bits
under some circumstances. We don't implement this. */
--
1.7.1
[Qemu-devel] [PATCH 2/4] target-arm: Disable priority_mask feature
This is unused for the ARMv7-M NVIC.
Signed-off-by: Sebastian Huber
---
hw/arm_gic.c |4
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/hw/arm_gic.c b/hw/arm_gic.c
index 5139d95..cafcc81 100644
--- a/hw/arm_gic.c
+++ b/hw/arm_gic.c
@@ -707,7 +707,11 @@ static void gic_reset(gic_state *s)
int i;
memset(s->irq_state, 0, GIC_NIRQ * sizeof(gic_irq_state));
for (i = 0 ; i < NUM_CPU(s); i++) {
+#ifdef NVIC
+s->priority_mask[i] = 0x100;
+#else
s->priority_mask[i] = 0xf0;
+#endif
s->current_pending[i] = 1023;
s->running_irq[i] = 1023;
s->running_priority[i] = 0x100;
--
1.7.1
Re: [Qemu-devel] [Bug 696094] Re: TI Stellaris lm3s811evb (ARM Cortex-M3) : Systick interrupt not working
Hello,
I am able to run the RTEMS real time system on the TI Stellaris LM3S6965
with a working system tick. I used the attached local hacks and patches
with the Qemu development branch from today.
Have a nice day!
--
Sebastian Huber, embedded brains GmbH
Address : Obere Lagerstr. 30, D-82178 Puchheim, Germany
Phone : +49 89 18 90 80 79-6
Fax : +49 89 18 90 80 79-9
E-Mail : sebastian.hu...@embedded-brains.de
PGP : Public key available on request.
Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.
>From 0c8e700376cec0c7b5a70f999b5e286efc321423 Mon Sep 17 00:00:00 2001
From: Sebastian Huber
Date: Fri, 16 Dec 2011 19:46:40 +0100
Subject: [PATCH 1/4] target-arm: Fixed ARMv7-M SHPR access
According to "ARMv7-M Architecture Reference Manual" issue D section
"B3.2.10 System Handler Prioriy Register 1, SHPR1", "B3.2.11 System
Handler Prioriy Register 2, SHPR2", and "B3.2.12 System Handler Prioriy
Register 3, SHPR3".
Signed-off-by: Sebastian Huber
---
hw/arm_gic.c | 16 ++--
hw/armv7m_nvic.c | 19 ---
2 files changed, 14 insertions(+), 21 deletions(-)
diff --git a/hw/arm_gic.c b/hw/arm_gic.c
index 9b52119..5139d95 100644
--- a/hw/arm_gic.c
+++ b/hw/arm_gic.c
@@ -356,6 +356,11 @@ static uint32_t gic_dist_readb(void *opaque, target_phys_addr_t offset)
if (GIC_TEST_TRIGGER(irq + i))
res |= (2 << (i * 2));
}
+#else
+} else if (0xd18 <= offset && offset < 0xd24) {
+/* System Handler Priority. */
+irq = offset - 0xd14;
+res = GIC_GET_PRIORITY(irq, cpu);
#endif
} else if (offset < 0xfe0) {
goto bad_reg;
@@ -387,7 +392,8 @@ static uint32_t gic_dist_readl(void *opaque, target_phys_addr_t offset)
gic_state *s = (gic_state *)opaque;
uint32_t addr;
addr = offset;
-if (addr < 0x100 || addr > 0xd00)
+if (addr < 0x100 || (addr > 0xd00 && addr != 0xd18 && addr != 0xd1c
+&& addr != 0xd20))
return nvic_readl(s, addr);
#endif
val = gic_dist_readw(opaque, offset);
@@ -528,6 +534,11 @@ static void gic_dist_writeb(void *opaque, target_phys_addr_t offset,
GIC_CLEAR_TRIGGER(irq + i);
}
}
+#else
+} else if (0xd18 <= offset && offset < 0xd24) {
+/* System Handler Priority. */
+irq = offset - 0xd14;
+s->priority1[irq][0] = value & 0xff;
#endif
} else {
/* 0xf00 is only handled for 32-bit writes. */
@@ -553,7 +564,8 @@ static void gic_dist_writel(void *opaque, target_phys_addr_t offset,
#ifdef NVIC
uint32_t addr;
addr = offset;
-if (addr < 0x100 || (addr > 0xd00 && addr != 0xf00)) {
+if (addr < 0x100 || (addr > 0xd00 && addr != 0xd18 && addr != 0xd1c
+&& addr != 0xd20 && addr != 0xf00)) {
nvic_writel(s, addr, value);
return;
}
diff --git a/hw/armv7m_nvic.c b/hw/armv7m_nvic.c
index bf8c3c5..65b575e 100644
--- a/hw/armv7m_nvic.c
+++ b/hw/armv7m_nvic.c
@@ -195,14 +195,6 @@ static uint32_t nvic_readl(void *opaque, uint32_t offset)
case 0xd14: /* Configuration Control. */
/* TODO: Implement Configuration Control bits. */
return 0;
-case 0xd18: case 0xd1c: case 0xd20: /* System Handler Priority. */
-irq = offset - 0xd14;
-val = 0;
-val |= s->gic.priority1[irq++][0];
-val |= s->gic.priority1[irq++][0] << 8;
-val |= s->gic.priority1[irq++][0] << 16;
-val |= s->gic.priority1[irq][0] << 24;
-return val;
case 0xd24: /* System Handler Status. */
val = 0;
if (s->gic.irq_state[ARMV7M_EXCP_MEM].active) val |= (1 << 0);
@@ -335,17 +327,6 @@ static void nvic_writel(void *opaque, uint32_t offset, uint32_t value)
case 0xd14: /* Configuration Control. */
/* TODO: Implement control registers. */
goto bad_reg;
-case 0xd18: case 0xd1c: case 0xd20: /* System Handler Priority. */
-{
-int irq;
-irq = offset - 0xd14;
-s->gic.priority1[irq++][0] = value & 0xff;
-s->gic.priority1[irq++][0] = (value >> 8) & 0xff;
-s->gic.priority1[irq++][0] = (value >> 16) & 0xff;
-s->gic.priority1[irq][0] = (value >> 24) & 0xff;
-gic_update(&s->gic);
-}
-break;
case 0xd24: /* System Handler Control. */
/* TODO: Real hardware allows you to set/clear the active bits
under some circumstances. We don't implement this. */
--
1.7.1
>From 5f562d098d84e12d4688272dcf68a2d0318721a7 Mon Sep 17 00:00:00 2001
From: Sebastian Huber
Date: Fri, 16 Dec 2011 20:00:59 +0100
Subject: [PATCH 2/4] target-arm: Disable priority_mask feature
This is unused for the ARMv7-M NVIC.
Signed-off-by: Sebastian Huber
---
hw/arm_gic.c |4
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/hw/arm_gic.c b/hw/arm_gic.c
index 5139d95..cafcc81 100644
--- a/hw/arm_gic.c
+++ b/hw/arm_gic.c
@@
[Qemu-devel] [PATCH] target-arm: Fixed ARMv7-M SHPR access
According to "ARMv7-M Architecture Reference Manual" issue D section
"B3.2.10 System Handler Prioriy Register 1, SHPR1", "B3.2.11 System
Handler Prioriy Register 2, SHPR2", and "B3.2.12 System Handler Prioriy
Register 3, SHPR3".
Signed-off-by: Sebastian Huber
---
hw/arm_gic.c | 16 ++--
hw/armv7m_nvic.c | 19 ---
2 files changed, 14 insertions(+), 21 deletions(-)
diff --git a/hw/arm_gic.c b/hw/arm_gic.c
index 9b52119..5139d95 100644
--- a/hw/arm_gic.c
+++ b/hw/arm_gic.c
@@ -356,6 +356,11 @@ static uint32_t gic_dist_readb(void *opaque,
target_phys_addr_t offset)
if (GIC_TEST_TRIGGER(irq + i))
res |= (2 << (i * 2));
}
+#else
+} else if (0xd18 <= offset && offset < 0xd24) {
+/* System Handler Priority. */
+irq = offset - 0xd14;
+res = GIC_GET_PRIORITY(irq, cpu);
#endif
} else if (offset < 0xfe0) {
goto bad_reg;
@@ -387,7 +392,8 @@ static uint32_t gic_dist_readl(void *opaque,
target_phys_addr_t offset)
gic_state *s = (gic_state *)opaque;
uint32_t addr;
addr = offset;
-if (addr < 0x100 || addr > 0xd00)
+if (addr < 0x100 || (addr > 0xd00 && addr != 0xd18 && addr != 0xd1c
+&& addr != 0xd20))
return nvic_readl(s, addr);
#endif
val = gic_dist_readw(opaque, offset);
@@ -528,6 +534,11 @@ static void gic_dist_writeb(void *opaque,
target_phys_addr_t offset,
GIC_CLEAR_TRIGGER(irq + i);
}
}
+#else
+} else if (0xd18 <= offset && offset < 0xd24) {
+/* System Handler Priority. */
+irq = offset - 0xd14;
+s->priority1[irq][0] = value & 0xff;
#endif
} else {
/* 0xf00 is only handled for 32-bit writes. */
@@ -553,7 +564,8 @@ static void gic_dist_writel(void *opaque,
target_phys_addr_t offset,
#ifdef NVIC
uint32_t addr;
addr = offset;
-if (addr < 0x100 || (addr > 0xd00 && addr != 0xf00)) {
+if (addr < 0x100 || (addr > 0xd00 && addr != 0xd18 && addr != 0xd1c
+&& addr != 0xd20 && addr != 0xf00)) {
nvic_writel(s, addr, value);
return;
}
diff --git a/hw/armv7m_nvic.c b/hw/armv7m_nvic.c
index bf8c3c5..65b575e 100644
--- a/hw/armv7m_nvic.c
+++ b/hw/armv7m_nvic.c
@@ -195,14 +195,6 @@ static uint32_t nvic_readl(void *opaque, uint32_t offset)
case 0xd14: /* Configuration Control. */
/* TODO: Implement Configuration Control bits. */
return 0;
-case 0xd18: case 0xd1c: case 0xd20: /* System Handler Priority. */
-irq = offset - 0xd14;
-val = 0;
-val |= s->gic.priority1[irq++][0];
-val |= s->gic.priority1[irq++][0] << 8;
-val |= s->gic.priority1[irq++][0] << 16;
-val |= s->gic.priority1[irq][0] << 24;
-return val;
case 0xd24: /* System Handler Status. */
val = 0;
if (s->gic.irq_state[ARMV7M_EXCP_MEM].active) val |= (1 << 0);
@@ -335,17 +327,6 @@ static void nvic_writel(void *opaque, uint32_t offset,
uint32_t value)
case 0xd14: /* Configuration Control. */
/* TODO: Implement control registers. */
goto bad_reg;
-case 0xd18: case 0xd1c: case 0xd20: /* System Handler Priority. */
-{
-int irq;
-irq = offset - 0xd14;
-s->gic.priority1[irq++][0] = value & 0xff;
-s->gic.priority1[irq++][0] = (value >> 8) & 0xff;
-s->gic.priority1[irq++][0] = (value >> 16) & 0xff;
-s->gic.priority1[irq][0] = (value >> 24) & 0xff;
-gic_update(&s->gic);
-}
-break;
case 0xd24: /* System Handler Control. */
/* TODO: Real hardware allows you to set/clear the active bits
under some circumstances. We don't implement this. */
--
1.7.1
[Qemu-devel] [PATCH v2 4/8] qom: interpret the return value when setting legacy properties
Signed-off-by: Paolo Bonzini
---
hw/qdev-properties.c | 39 ---
hw/qdev.c|3 +--
hw/qdev.h|2 ++
3 files changed, 27 insertions(+), 17 deletions(-)
diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index f0b811c..76ecb38 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -614,6 +614,26 @@ int qdev_prop_exists(DeviceState *dev, const char *name)
return qdev_prop_find(dev, name) ? true : false;
}
+void qdev_prop_error(Error **errp, int ret,
+ DeviceState *dev, Property *prop, const char *value)
+{
+switch (ret) {
+case -EEXIST:
+error_set(errp, QERR_PROPERTY_VALUE_IN_USE,
+ dev->info->name, prop->name, value);
+break;
+default:
+case -EINVAL:
+error_set(errp, QERR_PROPERTY_VALUE_BAD,
+ dev->info->name, prop->name, value);
+break;
+case -ENOENT:
+error_set(errp, QERR_PROPERTY_VALUE_NOT_FOUND,
+ dev->info->name, prop->name, value);
+break;
+}
+}
+
int qdev_prop_parse(DeviceState *dev, const char *name, const char *value)
{
Property *prop;
@@ -632,21 +652,10 @@ int qdev_prop_parse(DeviceState *dev, const char *name,
const char *value)
}
ret = prop->info->parse(dev, prop, value);
if (ret < 0) {
-switch (ret) {
-case -EEXIST:
-qerror_report(QERR_PROPERTY_VALUE_IN_USE,
- dev->info->name, name, value);
-break;
-default:
-case -EINVAL:
-qerror_report(QERR_PROPERTY_VALUE_BAD,
- dev->info->name, name, value);
-break;
-case -ENOENT:
-qerror_report(QERR_PROPERTY_VALUE_NOT_FOUND,
- dev->info->name, name, value);
-break;
-}
+Error *err;
+qdev_prop_error(&err, ret, dev, prop, value);
+qerror_report_err(err);
+error_free(err);
return -1;
}
return 0;
diff --git a/hw/qdev.c b/hw/qdev.c
index c020a6f..c8ab7b7 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -1163,8 +1163,7 @@ static void qdev_set_legacy_property(DeviceState *dev,
Visitor *v, void *opaque,
ret = prop->info->parse(dev, prop, ptr);
if (ret != 0) {
-error_set(errp, QERR_INVALID_PARAMETER_VALUE,
- name, prop->info->name);
+qdev_prop_error(errp, ret, dev, prop, ptr);
}
g_free(ptr);
}
diff --git a/hw/qdev.h b/hw/qdev.h
index 6e18427..828d811 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -370,6 +370,8 @@ void qdev_prop_set_defaults(DeviceState *dev, Property
*props);
void qdev_prop_register_global_list(GlobalProperty *props);
void qdev_prop_set_globals(DeviceState *dev);
+void qdev_prop_error(Error **errp, int ret, DeviceState *name,
+ Property *prop, const char *value);
static inline const char *qdev_fw_name(DeviceState *dev)
{
--
1.7.7.1
[Qemu-devel] [PATCH v2 7/8] qom: distinguish "legacy" property type name from QOM type name
Signed-off-by: Paolo Bonzini
---
hw/qdev-properties.c | 12
hw/qdev.c|9 ++---
hw/qdev.h|1 +
3 files changed, 15 insertions(+), 7 deletions(-)
diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index f7aa3cb..dd41e5a 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -86,7 +86,8 @@ static void set_bit(DeviceState *dev, Visitor *v, void
*opaque,
}
PropertyInfo qdev_prop_bit = {
-.name = "on/off",
+.name = "boolean",
+.legacy_name = "on/off",
.type = PROP_TYPE_BIT,
.size = sizeof(uint32_t),
.parse = parse_bit,
@@ -189,7 +190,8 @@ static int print_hex8(DeviceState *dev, Property *prop,
char *dest, size_t len)
}
PropertyInfo qdev_prop_hex8 = {
-.name = "hex8",
+.name = "uint8",
+.legacy_name = "hex8",
.type = PROP_TYPE_UINT8,
.size = sizeof(uint8_t),
.parse = parse_hex8,
@@ -397,7 +399,8 @@ static int print_hex32(DeviceState *dev, Property *prop,
char *dest, size_t len)
}
PropertyInfo qdev_prop_hex32 = {
-.name = "hex32",
+.name = "uint32",
+.legacy_name = "hex32",
.type = PROP_TYPE_UINT32,
.size = sizeof(uint32_t),
.parse = parse_hex32,
@@ -485,7 +488,8 @@ static int print_hex64(DeviceState *dev, Property *prop,
char *dest, size_t len)
}
PropertyInfo qdev_prop_hex64 = {
-.name = "hex64",
+.name = "uint64",
+.legacy_name = "hex64",
.type = PROP_TYPE_UINT64,
.size = sizeof(uint64_t),
.parse = parse_hex64,
diff --git a/hw/qdev.c b/hw/qdev.c
index c8ab7b7..426ea71 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -218,13 +218,15 @@ int qdev_device_help(QemuOpts *opts)
if (!prop->info->parse) {
continue; /* no way to set it, don't show */
}
-error_printf("%s.%s=%s\n", info->name, prop->name, prop->info->name);
+error_printf("%s.%s=%s\n", info->name, prop->name,
+ prop->info->legacy_name ?: prop->info->name);
}
for (prop = info->bus_info->props; prop && prop->name; prop++) {
if (!prop->info->parse) {
continue; /* no way to set it, don't show */
}
-error_printf("%s.%s=%s\n", info->name, prop->name, prop->info->name);
+error_printf("%s.%s=%s\n", info->name, prop->name,
+ prop->info->legacy_name ?: prop->info->name);
}
return 1;
}
@@ -1182,7 +1184,8 @@ void qdev_property_add_legacy(DeviceState *dev, Property
*prop,
{
gchar *type;
-type = g_strdup_printf("legacy<%s>", prop->info->name);
+type = g_strdup_printf("legacy<%s>",
+ prop->info->legacy_name ?: prop->info->name);
qdev_property_add(dev, prop->name, type,
prop->info->print ? qdev_get_legacy_property : NULL,
diff --git a/hw/qdev.h b/hw/qdev.h
index a1cce37..8002644 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -156,6 +156,7 @@ enum PropertyType {
struct PropertyInfo {
const char *name;
+const char *legacy_name;
size_t size;
enum PropertyType type;
int64_t min;
--
1.7.7.1
[Qemu-devel] [PATCH v2 6/8] qom: introduce get/set methods for Property
This patch adds a visitor interface to Property. This way, QOM will be
able to expose Properties that access a fixed field in a struct without
exposing also the everything-is-a-string "feature" of qdev properties.
Whenever the printed representation in both QOM and qdev (which is
typically the case for device backends), parse/print code can be reused
via get_generic/set_generic. Dually, whenever multiple PropertyInfos
have the same representation in both the struct and the visitors the
code can be reused (for example among all of int32/uint32/hex32).
Signed-off-by: Paolo Bonzini
---
hw/qdev-addr.c | 41 ++
hw/qdev-properties.c | 355 ++
hw/qdev.h|4 +
3 files changed, 400 insertions(+), 0 deletions(-)
diff --git a/hw/qdev-addr.c b/hw/qdev-addr.c
index 305c2d3..5ddda2d 100644
--- a/hw/qdev-addr.c
+++ b/hw/qdev-addr.c
@@ -18,12 +18,53 @@ static int print_taddr(DeviceState *dev, Property *prop,
char *dest, size_t len)
return snprintf(dest, len, "0x" TARGET_FMT_plx, *ptr);
}
+static void get_taddr(DeviceState *dev, Visitor *v, void *opaque,
+ const char *name, Error **errp)
+{
+Property *prop = opaque;
+target_phys_addr_t *ptr = qdev_get_prop_ptr(dev, prop);
+int64_t value;
+
+value = *ptr;
+visit_type_int(v, &value, name, errp);
+}
+
+static void set_taddr(DeviceState *dev, Visitor *v, void *opaque,
+ const char *name, Error **errp)
+{
+Property *prop = opaque;
+target_phys_addr_t *ptr = qdev_get_prop_ptr(dev, prop);
+Error *local_err = NULL;
+int64_t value;
+
+if (dev->state != DEV_STATE_CREATED) {
+error_set(errp, QERR_PERMISSION_DENIED);
+return;
+}
+
+visit_type_int(v, &value, name, &local_err);
+if (local_err) {
+error_propagate(errp, local_err);
+return;
+}
+if ((uint64_t)value <= (uint64_t) ~(target_phys_addr_t)0) {
+*ptr = value;
+} else {
+error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE,
+ dev->id?:"", name, value, (uint64_t) 0,
+ (uint64_t) ~(target_phys_addr_t)0);
+}
+}
+
+
PropertyInfo qdev_prop_taddr = {
.name = "taddr",
.type = PROP_TYPE_TADDR,
.size = sizeof(target_phys_addr_t),
.parse = parse_taddr,
.print = print_taddr,
+.get = get_taddr,
+.set = set_taddr,
};
void qdev_prop_set_taddr(DeviceState *dev, const char *name,
target_phys_addr_t value)
diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index 76ecb38..f7aa3cb 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -55,12 +55,44 @@ static int print_bit(DeviceState *dev, Property *prop, char
*dest, size_t len)
return snprintf(dest, len, (*p & qdev_get_prop_mask(prop)) ? "on" : "off");
}
+static void get_bit(DeviceState *dev, Visitor *v, void *opaque,
+const char *name, Error **errp)
+{
+Property *prop = opaque;
+uint32_t *p = qdev_get_prop_ptr(dev, prop);
+bool value = (*p & qdev_get_prop_mask(prop)) != 0;
+
+visit_type_bool(v, &value, name, errp);
+}
+
+static void set_bit(DeviceState *dev, Visitor *v, void *opaque,
+const char *name, Error **errp)
+{
+Property *prop = opaque;
+Error *local_err = NULL;
+bool value;
+
+if (dev->state != DEV_STATE_CREATED) {
+error_set(errp, QERR_PERMISSION_DENIED);
+return;
+}
+
+visit_type_bool(v, &value, name, &local_err);
+if (local_err) {
+error_propagate(errp, local_err);
+return;
+}
+bit_prop_set(dev, prop, value);
+}
+
PropertyInfo qdev_prop_bit = {
.name = "on/off",
.type = PROP_TYPE_BIT,
.size = sizeof(uint32_t),
.parse = parse_bit,
.print = print_bit,
+.get = get_bit,
+.set = set_bit,
};
/* --- 8bit integer --- */
@@ -85,12 +117,54 @@ static int print_uint8(DeviceState *dev, Property *prop,
char *dest, size_t len)
return snprintf(dest, len, "%" PRIu8, *ptr);
}
+static void get_int8(DeviceState *dev, Visitor *v, void *opaque,
+ const char *name, Error **errp)
+{
+Property *prop = opaque;
+int8_t *ptr = qdev_get_prop_ptr(dev, prop);
+int64_t value;
+
+value = *ptr;
+visit_type_int(v, &value, name, errp);
+}
+
+static void set_int8(DeviceState *dev, Visitor *v, void *opaque,
+ const char *name, Error **errp)
+{
+Property *prop = opaque;
+int8_t *ptr = qdev_get_prop_ptr(dev, prop);
+Error *local_err = NULL;
+int64_t value;
+
+if (dev->state != DEV_STATE_CREATED) {
+error_set(errp, QERR_PERMISSION_DENIED);
+return;
+}
+
+visit_type_int(v, &value, name, &local_err);
+if (local_err) {
+error_propagate(errp, local_err);
+return;
+}
+if (value > prop->info->min && value <= prop->info->max) {
+*ptr = value;
+} else {
+error_set(e
Re: [Qemu-devel] [PATCH v2 4/8] qom: interpret the return value when setting legacy properties
On 12/16/2011 06:00 PM, Anthony Liguori wrote:
And perhaps it would make more sense to return Error * and make the
function name be a constructor:
Error *error_from_qdev_prop_err(int ret, DeviceState *dev,
Property *prop, const char *value);
That doesn't work too well when calling it from setters. However,
error_set_from_qdev_prop_error is definitely an improvement (no matter
what a mouthful it is).
I need to rush now. I placed this at git://github.com/qemu/bonzini.git
qom-props (commit 263e8c5), so you can play on top of it or pull it. I
attach the interdiff from v2.
Paolo
diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index dd41e5a..80115b3 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -644,13 +644,11 @@ static void set_generic(DeviceState *dev, Visitor *v, void *opaque,
}
if (!*str) {
g_free(str);
-qdev_prop_error(errp, EINVAL, dev, prop, str);
+error_set_from_qdev_prop_error(errp, EINVAL, dev, prop, str);
return;
}
ret = prop->info->parse(dev, prop, str);
-if (ret != 0) {
-qdev_prop_error(errp, ret, dev, prop, str);
-}
+error_set_from_qdev_prop_error(errp, ret, dev, prop, str);
g_free(str);
}
@@ -973,8 +972,8 @@ int qdev_prop_exists(DeviceState *dev, const char *name)
return qdev_prop_find(dev, name) ? true : false;
}
-void qdev_prop_error(Error **errp, int ret,
- DeviceState *dev, Property *prop, const char *value)
+void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev,
+Property *prop, const char *value)
{
switch (ret) {
case -EEXIST:
@@ -990,6 +989,10 @@ void qdev_prop_error(Error **errp, int ret,
error_set(errp, QERR_PROPERTY_VALUE_NOT_FOUND,
dev->info->name, prop->name, value);
break;
+case 0:
+break;
+default:
+abort();
}
}
@@ -1012,7 +1015,7 @@ int qdev_prop_parse(DeviceState *dev, const char *name, const char *value)
ret = prop->info->parse(dev, prop, value);
if (ret < 0) {
Error *err;
-qdev_prop_error(&err, ret, dev, prop, value);
+error_set_from_qdev_prop_error(&err, ret, dev, prop, value);
qerror_report_err(err);
error_free(err);
return -1;
diff --git a/hw/qdev.c b/hw/qdev.c
index 2f7defc..0465632 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -1169,9 +1169,7 @@ static void qdev_set_legacy_property(DeviceState *dev, Visitor *v, void *opaque,
}
ret = prop->info->parse(dev, prop, ptr);
-if (ret != 0) {
-qdev_prop_error(errp, ret, dev, prop, ptr);
-}
+error_set_from_qdev_prop_error(errp, ret, dev, prop, ptr);
g_free(ptr);
}
@@ -1179,7 +1177,7 @@ static void qdev_set_legacy_property(DeviceState *dev, Visitor *v, void *opaque,
* @qdev_add_legacy_property - adds a legacy property
*
* Do not use this is new code! Properties added through this interface will
- * be given names and types in the "legacy<>" type namespace.
+ * be given names and types in the "legacy" namespace.
*
* Legacy properties are always processed as strings. The format of the string
* depends on the property type.
@@ -1189,7 +1187,7 @@ void qdev_property_add_legacy(DeviceState *dev, Property *prop,
{
gchar *name, *type;
-name = g_strdup_printf("legacy<%s>", prop->name);
+name = g_strdup_printf("legacy-%s", prop->name);
type = g_strdup_printf("legacy<%s>",
prop->info->legacy_name ?: prop->info->name);
diff --git a/hw/qdev.h b/hw/qdev.h
index 3410e77..d5896be 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -375,8 +375,8 @@ void qdev_prop_set_defaults(DeviceState *dev, Property *props);
void qdev_prop_register_global_list(GlobalProperty *props);
void qdev_prop_set_globals(DeviceState *dev);
-void qdev_prop_error(Error **errp, int ret, DeviceState *name,
- Property *prop, const char *value);
+void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev,
+Property *prop, const char *value);
static inline const char *qdev_fw_name(DeviceState *dev)
{
[Qemu-devel] [PATCH v2 5/8] qom: introduce QERR_PROPERTY_VALUE_OUT_OF_RANGE
This will be used when reject invalid values for integer fields that
are less than 64-bits wide.
Signed-off-by: Paolo Bonzini
---
qerror.c |5 +
qerror.h |3 +++
2 files changed, 8 insertions(+), 0 deletions(-)
diff --git a/qerror.c b/qerror.c
index adde8a5..9a75d06 100644
--- a/qerror.c
+++ b/qerror.c
@@ -206,6 +206,11 @@ static const QErrorStringTable qerror_table[] = {
.desc = "Property '%(device).%(property)' can't find value
'%(value)'",
},
{
+.error_fmt = QERR_PROPERTY_VALUE_OUT_OF_RANGE,
+.desc = "Property '%(device).%(property)' doesn't take "
+ "value %(value) (minimum: %(min), maximum: %(max)'",
+},
+{
.error_fmt = QERR_QMP_BAD_INPUT_OBJECT,
.desc = "Expected '%(expected)' in QMP input",
},
diff --git a/qerror.h b/qerror.h
index 9190b02..efda232 100644
--- a/qerror.h
+++ b/qerror.h
@@ -171,6 +171,9 @@ QError *qobject_to_qerror(const QObject *obj);
#define QERR_PROPERTY_VALUE_NOT_FOUND \
"{ 'class': 'PropertyValueNotFound', 'data': { 'device': %s, 'property':
%s, 'value': %s } }"
+#define QERR_PROPERTY_VALUE_OUT_OF_RANGE \
+"{ 'class': 'PropertyValueOutOfRange', 'data': { 'device': %s, 'property':
%s, 'value': %"PRId64", 'min': %"PRId64", 'max': %"PRId64" } }"
+
#define QERR_QMP_BAD_INPUT_OBJECT \
"{ 'class': 'QMPBadInputObject', 'data': { 'expected': %s } }"
--
1.7.7.1
[Qemu-devel] [PATCH v2 8/8] qom: register qdev properties also as non-legacy properties
Push legacy properties into a legacy<...> namespace, and make them
available with correct types too.
Signed-off-by: Paolo Bonzini
---
hw/qdev.c | 28 +---
hw/qdev.h |7 +++
2 files changed, 28 insertions(+), 7 deletions(-)
diff --git a/hw/qdev.c b/hw/qdev.c
index 426ea71..2f7defc 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -80,6 +80,9 @@ static DeviceInfo *qdev_find_info(BusInfo *bus_info, const
char *name)
return NULL;
}
+static void qdev_property_add_legacy(DeviceState *dev, Property *prop,
+ Error **errp);
+
static DeviceState *qdev_create_from_info(BusState *bus, DeviceInfo *info)
{
DeviceState *dev;
@@ -104,10 +107,12 @@ static DeviceState *qdev_create_from_info(BusState *bus,
DeviceInfo *info)
for (prop = dev->info->props; prop && prop->name; prop++) {
qdev_property_add_legacy(dev, prop, NULL);
+qdev_property_add_static(dev, prop, NULL);
}
for (prop = dev->info->bus_info->props; prop && prop->name; prop++) {
qdev_property_add_legacy(dev, prop, NULL);
+qdev_property_add_static(dev, prop, NULL);
}
qdev_property_add_str(dev, "type", qdev_get_type, NULL, NULL);
@@ -1174,7 +1179,7 @@ static void qdev_set_legacy_property(DeviceState *dev,
Visitor *v, void *opaque,
* @qdev_add_legacy_property - adds a legacy property
*
* Do not use this is new code! Properties added through this interface will
- * be given types in the "legacy<>" type namespace.
+ * be given names and types in the "legacy<>" type namespace.
*
* Legacy properties are always processed as strings. The format of the string
* depends on the property type.
@@ -1182,18 +1187,35 @@ static void qdev_set_legacy_property(DeviceState *dev,
Visitor *v, void *opaque,
void qdev_property_add_legacy(DeviceState *dev, Property *prop,
Error **errp)
{
-gchar *type;
+gchar *name, *type;
+name = g_strdup_printf("legacy<%s>", prop->name);
type = g_strdup_printf("legacy<%s>",
prop->info->legacy_name ?: prop->info->name);
-qdev_property_add(dev, prop->name, type,
+qdev_property_add(dev, name, type,
prop->info->print ? qdev_get_legacy_property : NULL,
prop->info->parse ? qdev_set_legacy_property : NULL,
NULL,
prop, errp);
g_free(type);
+g_free(name);
+}
+
+/**
+ * @qdev_property_add_static - add a @Property to a device.
+ *
+ * Static properties access data in a struct. The actual type of the
+ * property and the field depends on the property type.
+ */
+void qdev_property_add_static(DeviceState *dev, Property *prop,
+ Error **errp)
+{
+qdev_property_add(dev, prop->name, prop->info->name,
+ prop->info->get, prop->info->set,
+ NULL,
+ prop, errp);
}
DeviceState *qdev_get_root(void)
diff --git a/hw/qdev.h b/hw/qdev.h
index 8002644..3410e77 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -487,11 +487,10 @@ const char *qdev_property_get_type(DeviceState *dev,
const char *name,
Error **errp);
/**
- * @qdev_property_add_legacy - add a legacy @Property to a device
- *
- * DO NOT USE THIS IN NEW CODE!
+ * @qdev_property_add_static - add a @Property to a device referencing a
+ * field in a struct.
*/
-void qdev_property_add_legacy(DeviceState *dev, Property *prop, Error **errp);
+void qdev_property_add_static(DeviceState *dev, Property *prop, Error **errp);
/**
* @qdev_get_root - returns the root device of the composition tree
--
1.7.7.1
[Qemu-devel] [PATCH v2 0/8] qom: introduce non-legacy static properties
QOM right now does not have a way to communicate values for qdev
properties except as strings. This is bad.
This patch improves the Property implementation so that properties
export a visitor-based interface in addition to the string-based
interface. The new interface can then be registered as a "static"
property. It's called static because it uses a struct field for
storage and, as such, should be present in all objects of a given
class.
Patches 1-2 are bugfixes and patches 3-4 are cleanups.
Example using qmp-shell:
x86_64-softmmu/qemu-system-x86_64 \
-hda ~/test.img -snapshot -S \
-qmp unix:/tmp/server.sock,server,nowait \
-netdev type=user,id=net -device virtio-net-pci,netdev=net,id=net \
-net user,vlan=1 -device virtio-net-pci,id=net2,vlan=1 \
-chardev id=stdio,backend=stdio -device isa-serial,chardev=stdio,id=serial
Boolean properties:
(QEMU) qom-get path=/i440fx/piix3 property=command_serr_enable
{u'return': True}
(QEMU) qom-get path=/i440fx/piix3 property=legacy
{u'return': u'on'}
PCI address properties (perhaps will disappear later, but not yet):
(QEMU) qom-get path=/i440fx/piix3 property=addr
{u'return': u'01.0'}
(QEMU) qom-get path=/i440fx/piix3 property=legacy
{u'return': u'01.0'}
String properties (QObject does not have NULL):
(QEMU) qom-get path=/vga property=romfile
{u'return': u'vgabios-cirrus.bin'}
(QEMU) qom-get path=/vga property=legacy
{u'return': u'"vgabios-cirrus.bin"'}
(QEMU) qom-get path=/i440fx/piix3 property=romfile
{u'return': u''}
(QEMU) qom-get path=/i440fx/piix3 property=legacy
{u'return': u''}
MAC properties:
(QEMU) qom-get path=/peripheral/net property=mac
{u'return': u'52:54:00:12:34:56'}
(QEMU) qom-get path=/peripheral/net property=legacy
{u'return': u'52:54:00:12:34:56'}
(QEMU) qom-set path=/peripheral/net property=mac value=52-54-00-12-34-57
{u'error': {u'data': {}, u'class': u'PermissionDenied', u'desc': u'Insufficient
permission to perform this operation'}}
Network properties:
(QEMU) qom-get path=/peripheral/net property=netdev
{u'return': u'net'}
(QEMU) qom-get path=/peripheral/net property=legacy
{u'return': u'net'}
VLAN properties:
(QEMU) qom-get path=/peripheral/net property=vlan
{u'return': -1}
(QEMU) qom-get path=/peripheral/net property=legacy
{u'return': u''}
(QEMU) qom-get path=/peripheral/net2 property=vlan
{u'return': 1}
(QEMU) qom-get path=/peripheral/net2 property=legacy
{u'return': u'1'}
Chardev properties:
(QEMU) qom-get path=/peripheral/serial property=chardev
{u'return': u'stdio'}
(QEMU) qom-get path=/peripheral/serial property=legacy
{u'return': u'stdio'}
Legacy hex32 properties:
(QEMU) qom-get path=/peripheral/serial property=iobase
{u'return': 1016}
(QEMU) qom-get path=/peripheral/serial property=legacy
{u'return': u'0x3f8'}
Examples of setting properties (after disabling the DEV_STATE_CREATED
check for testing only):
(QEMU) qom-set path=/peripheral/net2 property=vlan value=-1
{u'return': {}}
(QEMU) qom-get path=/peripheral/net2 property=vlan
{u'return': -1}
(QEMU) qom-get path=/peripheral/net2 property=legacy
{u'return': u''}
(QEMU) qom-set path=/peripheral/serial property=iobase value=760
{u'return': {}}
(QEMU) qom-get path=/peripheral/serial property=iobase
{u'return': 760}
(QEMU) qom-get path=/peripheral/serial property=legacy
{u'return': u'0x2f8'}
v1->v2:
New "qom: interpret the return value when setting legacy
properties". Always pass a value to the visitor when there
is no error. Handle empty strings as NULL. Did not change
QERR_PROPERTY_VALUE_OUT_OF_RANGE because it is consistent
with other QERR_PROPERTY_* errors, now used by QOM too.
Paolo Bonzini (8):
qapi: protect against NULL QObject in qmp_input_get_object
qom: fix swapped parameters
qom: push permission checks up into qdev_property_add_legacy
qom: interpret the return value when setting legacy properties
qom: introduce QERR_PROPERTY_VALUE_OUT_OF_RANGE
qom: introduce get/set methods for Property
qom: distinguish "legacy" property type name from QOM type name
qom: register qdev properties also as non-legacy properties
hw/qdev-addr.c | 41 +
hw/qdev-properties.c | 406 +++---
hw/qdev.c| 84 ++
hw/qdev.h| 14 ++-
qapi/qmp-input-visitor.c | 10 +-
qerror.c |5 +
qerror.h |3 +
7 files changed, 502 insertions(+), 61 deletions(-)
--
1.7.7.1
Re: [Qemu-devel] [PATCH v2 8/8] qom: register qdev properties also as non-legacy properties
On 12/16/2011 10:47 AM, Paolo Bonzini wrote:
Push legacy properties into a legacy<...> namespace, and make them
available with correct types too.
Signed-off-by: Paolo Bonzini
---
hw/qdev.c | 28 +---
hw/qdev.h |7 +++
2 files changed, 28 insertions(+), 7 deletions(-)
diff --git a/hw/qdev.c b/hw/qdev.c
index 426ea71..2f7defc 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -80,6 +80,9 @@ static DeviceInfo *qdev_find_info(BusInfo *bus_info, const
char *name)
return NULL;
}
+static void qdev_property_add_legacy(DeviceState *dev, Property *prop,
+ Error **errp);
+
static DeviceState *qdev_create_from_info(BusState *bus, DeviceInfo *info)
{
DeviceState *dev;
@@ -104,10 +107,12 @@ static DeviceState *qdev_create_from_info(BusState *bus,
DeviceInfo *info)
for (prop = dev->info->props; prop&& prop->name; prop++) {
qdev_property_add_legacy(dev, prop, NULL);
+qdev_property_add_static(dev, prop, NULL);
}
for (prop = dev->info->bus_info->props; prop&& prop->name; prop++) {
qdev_property_add_legacy(dev, prop, NULL);
+qdev_property_add_static(dev, prop, NULL);
}
qdev_property_add_str(dev, "type", qdev_get_type, NULL, NULL);
@@ -1174,7 +1179,7 @@ static void qdev_set_legacy_property(DeviceState *dev,
Visitor *v, void *opaque,
* @qdev_add_legacy_property - adds a legacy property
*
* Do not use this is new code! Properties added through this interface will
- * be given types in the "legacy<>" type namespace.
+ * be given names and types in the "legacy<>" type namespace.
*
* Legacy properties are always processed as strings. The format of the
string
* depends on the property type.
@@ -1182,18 +1187,35 @@ static void qdev_set_legacy_property(DeviceState *dev,
Visitor *v, void *opaque,
void qdev_property_add_legacy(DeviceState *dev, Property *prop,
Error **errp)
{
-gchar *type;
+gchar *name, *type;
+name = g_strdup_printf("legacy<%s>", prop->name);
Okay, let's make this name legacy-%s to make it clear that it's not type.
Otherwise, the patch looks good.
Regards,
Anthony Liguori
type = g_strdup_printf("legacy<%s>",
prop->info->legacy_name ?: prop->info->name);
-qdev_property_add(dev, prop->name, type,
+qdev_property_add(dev, name, type,
prop->info->print ? qdev_get_legacy_property : NULL,
prop->info->parse ? qdev_set_legacy_property : NULL,
NULL,
prop, errp);
g_free(type);
+g_free(name);
+}
+
+/**
+ * @qdev_property_add_static - add a @Property to a device.
+ *
+ * Static properties access data in a struct. The actual type of the
+ * property and the field depends on the property type.
+ */
+void qdev_property_add_static(DeviceState *dev, Property *prop,
+ Error **errp)
+{
+qdev_property_add(dev, prop->name, prop->info->name,
+ prop->info->get, prop->info->set,
+ NULL,
+ prop, errp);
}
DeviceState *qdev_get_root(void)
diff --git a/hw/qdev.h b/hw/qdev.h
index 8002644..3410e77 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -487,11 +487,10 @@ const char *qdev_property_get_type(DeviceState *dev,
const char *name,
Error **errp);
/**
- * @qdev_property_add_legacy - add a legacy @Property to a device
- *
- * DO NOT USE THIS IN NEW CODE!
+ * @qdev_property_add_static - add a @Property to a device referencing a
+ * field in a struct.
*/
-void qdev_property_add_legacy(DeviceState *dev, Property *prop, Error **errp);
+void qdev_property_add_static(DeviceState *dev, Property *prop, Error **errp);
/**
* @qdev_get_root - returns the root device of the composition tree
Re: [Qemu-devel] [PATCH v2 7/8] qom: distinguish "legacy" property type name from QOM type name
On 12/16/2011 10:47 AM, Paolo Bonzini wrote:
Signed-off-by: Paolo Bonzini
Reviewed-by: Anthony Liguori
Regards,
Anthony Liguori
---
hw/qdev-properties.c | 12
hw/qdev.c|9 ++---
hw/qdev.h|1 +
3 files changed, 15 insertions(+), 7 deletions(-)
diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index f7aa3cb..dd41e5a 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -86,7 +86,8 @@ static void set_bit(DeviceState *dev, Visitor *v, void
*opaque,
}
PropertyInfo qdev_prop_bit = {
-.name = "on/off",
+.name = "boolean",
+.legacy_name = "on/off",
.type = PROP_TYPE_BIT,
.size = sizeof(uint32_t),
.parse = parse_bit,
@@ -189,7 +190,8 @@ static int print_hex8(DeviceState *dev, Property *prop,
char *dest, size_t len)
}
PropertyInfo qdev_prop_hex8 = {
-.name = "hex8",
+.name = "uint8",
+.legacy_name = "hex8",
.type = PROP_TYPE_UINT8,
.size = sizeof(uint8_t),
.parse = parse_hex8,
@@ -397,7 +399,8 @@ static int print_hex32(DeviceState *dev, Property *prop,
char *dest, size_t len)
}
PropertyInfo qdev_prop_hex32 = {
-.name = "hex32",
+.name = "uint32",
+.legacy_name = "hex32",
.type = PROP_TYPE_UINT32,
.size = sizeof(uint32_t),
.parse = parse_hex32,
@@ -485,7 +488,8 @@ static int print_hex64(DeviceState *dev, Property *prop,
char *dest, size_t len)
}
PropertyInfo qdev_prop_hex64 = {
-.name = "hex64",
+.name = "uint64",
+.legacy_name = "hex64",
.type = PROP_TYPE_UINT64,
.size = sizeof(uint64_t),
.parse = parse_hex64,
diff --git a/hw/qdev.c b/hw/qdev.c
index c8ab7b7..426ea71 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -218,13 +218,15 @@ int qdev_device_help(QemuOpts *opts)
if (!prop->info->parse) {
continue; /* no way to set it, don't show */
}
-error_printf("%s.%s=%s\n", info->name, prop->name, prop->info->name);
+error_printf("%s.%s=%s\n", info->name, prop->name,
+ prop->info->legacy_name ?: prop->info->name);
}
for (prop = info->bus_info->props; prop&& prop->name; prop++) {
if (!prop->info->parse) {
continue; /* no way to set it, don't show */
}
-error_printf("%s.%s=%s\n", info->name, prop->name, prop->info->name);
+error_printf("%s.%s=%s\n", info->name, prop->name,
+ prop->info->legacy_name ?: prop->info->name);
}
return 1;
}
@@ -1182,7 +1184,8 @@ void qdev_property_add_legacy(DeviceState *dev, Property
*prop,
{
gchar *type;
-type = g_strdup_printf("legacy<%s>", prop->info->name);
+type = g_strdup_printf("legacy<%s>",
+ prop->info->legacy_name ?: prop->info->name);
qdev_property_add(dev, prop->name, type,
prop->info->print ? qdev_get_legacy_property : NULL,
diff --git a/hw/qdev.h b/hw/qdev.h
index a1cce37..8002644 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -156,6 +156,7 @@ enum PropertyType {
struct PropertyInfo {
const char *name;
+const char *legacy_name;
size_t size;
enum PropertyType type;
int64_t min;
Re: [Qemu-devel] [PATCH v2 6/8] qom: introduce get/set methods for Property
On 12/16/2011 10:47 AM, Paolo Bonzini wrote:
This patch adds a visitor interface to Property. This way, QOM will be
able to expose Properties that access a fixed field in a struct without
exposing also the everything-is-a-string "feature" of qdev properties.
Whenever the printed representation in both QOM and qdev (which is
typically the case for device backends), parse/print code can be reused
via get_generic/set_generic. Dually, whenever multiple PropertyInfos
have the same representation in both the struct and the visitors the
code can be reused (for example among all of int32/uint32/hex32).
Signed-off-by: Paolo Bonzini
Reviewed-by: Anthony Liguori
Regards,
Anthony Liguori
---
hw/qdev-addr.c | 41 ++
hw/qdev-properties.c | 355 ++
hw/qdev.h|4 +
3 files changed, 400 insertions(+), 0 deletions(-)
diff --git a/hw/qdev-addr.c b/hw/qdev-addr.c
index 305c2d3..5ddda2d 100644
--- a/hw/qdev-addr.c
+++ b/hw/qdev-addr.c
@@ -18,12 +18,53 @@ static int print_taddr(DeviceState *dev, Property *prop,
char *dest, size_t len)
return snprintf(dest, len, "0x" TARGET_FMT_plx, *ptr);
}
+static void get_taddr(DeviceState *dev, Visitor *v, void *opaque,
+ const char *name, Error **errp)
+{
+Property *prop = opaque;
+target_phys_addr_t *ptr = qdev_get_prop_ptr(dev, prop);
+int64_t value;
+
+value = *ptr;
+visit_type_int(v,&value, name, errp);
+}
+
+static void set_taddr(DeviceState *dev, Visitor *v, void *opaque,
+ const char *name, Error **errp)
+{
+Property *prop = opaque;
+target_phys_addr_t *ptr = qdev_get_prop_ptr(dev, prop);
+Error *local_err = NULL;
+int64_t value;
+
+if (dev->state != DEV_STATE_CREATED) {
+error_set(errp, QERR_PERMISSION_DENIED);
+return;
+}
+
+visit_type_int(v,&value, name,&local_err);
+if (local_err) {
+error_propagate(errp, local_err);
+return;
+}
+if ((uint64_t)value<= (uint64_t) ~(target_phys_addr_t)0) {
+*ptr = value;
+} else {
+error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE,
+ dev->id?:"", name, value, (uint64_t) 0,
+ (uint64_t) ~(target_phys_addr_t)0);
+}
+}
+
+
PropertyInfo qdev_prop_taddr = {
.name = "taddr",
.type = PROP_TYPE_TADDR,
.size = sizeof(target_phys_addr_t),
.parse = parse_taddr,
.print = print_taddr,
+.get = get_taddr,
+.set = set_taddr,
};
void qdev_prop_set_taddr(DeviceState *dev, const char *name,
target_phys_addr_t value)
diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index 76ecb38..f7aa3cb 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -55,12 +55,44 @@ static int print_bit(DeviceState *dev, Property *prop, char
*dest, size_t len)
return snprintf(dest, len, (*p& qdev_get_prop_mask(prop)) ? "on" :
"off");
}
+static void get_bit(DeviceState *dev, Visitor *v, void *opaque,
+const char *name, Error **errp)
+{
+Property *prop = opaque;
+uint32_t *p = qdev_get_prop_ptr(dev, prop);
+bool value = (*p& qdev_get_prop_mask(prop)) != 0;
+
+visit_type_bool(v,&value, name, errp);
+}
+
+static void set_bit(DeviceState *dev, Visitor *v, void *opaque,
+const char *name, Error **errp)
+{
+Property *prop = opaque;
+Error *local_err = NULL;
+bool value;
+
+if (dev->state != DEV_STATE_CREATED) {
+error_set(errp, QERR_PERMISSION_DENIED);
+return;
+}
+
+visit_type_bool(v,&value, name,&local_err);
+if (local_err) {
+error_propagate(errp, local_err);
+return;
+}
+bit_prop_set(dev, prop, value);
+}
+
PropertyInfo qdev_prop_bit = {
.name = "on/off",
.type = PROP_TYPE_BIT,
.size = sizeof(uint32_t),
.parse = parse_bit,
.print = print_bit,
+.get = get_bit,
+.set = set_bit,
};
/* --- 8bit integer --- */
@@ -85,12 +117,54 @@ static int print_uint8(DeviceState *dev, Property *prop,
char *dest, size_t len)
return snprintf(dest, len, "%" PRIu8, *ptr);
}
+static void get_int8(DeviceState *dev, Visitor *v, void *opaque,
+ const char *name, Error **errp)
+{
+Property *prop = opaque;
+int8_t *ptr = qdev_get_prop_ptr(dev, prop);
+int64_t value;
+
+value = *ptr;
+visit_type_int(v,&value, name, errp);
+}
+
+static void set_int8(DeviceState *dev, Visitor *v, void *opaque,
+ const char *name, Error **errp)
+{
+Property *prop = opaque;
+int8_t *ptr = qdev_get_prop_ptr(dev, prop);
+Error *local_err = NULL;
+int64_t value;
+
+if (dev->state != DEV_STATE_CREATED) {
+error_set(errp, QERR_PERMISSION_DENIED);
+return;
+}
+
+visit_type_int(v,&value, name,&local_err);
+if (local_err) {
+error_propagate(errp, local_err);
+return;
+}
+
Re: [Qemu-devel] [PATCH 5/8] qom: introduce QERR_PROPERTY_VALUE_OUT_OF_RANGE
On 12/16/2011 11:00 AM, Paolo Bonzini wrote: On 12/16/2011 03:01 PM, Paolo Bonzini wrote: I'd rather use generic errors when possible. How about VALUE_OUT_OF_RANGE and we can make the message "'%(item)' doesn't take value..." and pass "%s.%s" % (device, property) for item. Ok. I didn't do this in the end for two reasons. First, that it is inconsistent with other errors from qdev properties. Current master does not raise them when properties are accessed via QOM, but my revised series does. Second, that it is actually provides less structured information. There's no reason why a client should be expected to "know" that %(item) is in that form. Ok, then Reviewed-by: Anthony Liguori Regards, Anthony Liguori Paolo
Re: [Qemu-devel] [PATCH v2 5/8] qom: introduce QERR_PROPERTY_VALUE_OUT_OF_RANGE
On 12/16/2011 10:47 AM, Paolo Bonzini wrote:
This will be used when reject invalid values for integer fields that
are less than 64-bits wide.
Signed-off-by: Paolo Bonzini
VALUE_OUT_OF_RANGE?
Regards,
Anthony Liguori
---
qerror.c |5 +
qerror.h |3 +++
2 files changed, 8 insertions(+), 0 deletions(-)
diff --git a/qerror.c b/qerror.c
index adde8a5..9a75d06 100644
--- a/qerror.c
+++ b/qerror.c
@@ -206,6 +206,11 @@ static const QErrorStringTable qerror_table[] = {
.desc = "Property '%(device).%(property)' can't find value
'%(value)'",
},
{
+.error_fmt = QERR_PROPERTY_VALUE_OUT_OF_RANGE,
+.desc = "Property '%(device).%(property)' doesn't take "
+ "value %(value) (minimum: %(min), maximum: %(max)'",
+},
+{
.error_fmt = QERR_QMP_BAD_INPUT_OBJECT,
.desc = "Expected '%(expected)' in QMP input",
},
diff --git a/qerror.h b/qerror.h
index 9190b02..efda232 100644
--- a/qerror.h
+++ b/qerror.h
@@ -171,6 +171,9 @@ QError *qobject_to_qerror(const QObject *obj);
#define QERR_PROPERTY_VALUE_NOT_FOUND \
"{ 'class': 'PropertyValueNotFound', 'data': { 'device': %s, 'property': %s,
'value': %s } }"
+#define QERR_PROPERTY_VALUE_OUT_OF_RANGE \
+"{ 'class': 'PropertyValueOutOfRange', 'data': { 'device': %s, 'property': %s, 'value': %"PRId64",
'min': %"PRId64", 'max': %"PRId64" } }"
+
#define QERR_QMP_BAD_INPUT_OBJECT \
"{ 'class': 'QMPBadInputObject', 'data': { 'expected': %s } }"
Re: [Qemu-devel] [PATCH 5/8] qom: introduce QERR_PROPERTY_VALUE_OUT_OF_RANGE
On 12/16/2011 03:01 PM, Paolo Bonzini wrote: I'd rather use generic errors when possible. How about VALUE_OUT_OF_RANGE and we can make the message "'%(item)' doesn't take value..." and pass "%s.%s" % (device, property) for item. Ok. I didn't do this in the end for two reasons. First, that it is inconsistent with other errors from qdev properties. Current master does not raise them when properties are accessed via QOM, but my revised series does. Second, that it is actually provides less structured information. There's no reason why a client should be expected to "know" that %(item) is in that form. Paolo
Re: [Qemu-devel] [PATCH v2 4/8] qom: interpret the return value when setting legacy properties
On 12/16/2011 10:47 AM, Paolo Bonzini wrote:
Signed-off-by: Paolo Bonzini
---
hw/qdev-properties.c | 39 ---
hw/qdev.c|3 +--
hw/qdev.h|2 ++
3 files changed, 27 insertions(+), 17 deletions(-)
diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index f0b811c..76ecb38 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -614,6 +614,26 @@ int qdev_prop_exists(DeviceState *dev, const char *name)
return qdev_prop_find(dev, name) ? true : false;
}
+void qdev_prop_error(Error **errp, int ret,
+ DeviceState *dev, Property *prop, const char *value)
+{
+switch (ret) {
+case -EEXIST:
+error_set(errp, QERR_PROPERTY_VALUE_IN_USE,
+ dev->info->name, prop->name, value);
+break;
+default:
+case -EINVAL:
+error_set(errp, QERR_PROPERTY_VALUE_BAD,
+ dev->info->name, prop->name, value);
+break;
+case -ENOENT:
+error_set(errp, QERR_PROPERTY_VALUE_NOT_FOUND,
+ dev->info->name, prop->name, value);
+break;
+}
+}
+
int qdev_prop_parse(DeviceState *dev, const char *name, const char *value)
{
Property *prop;
@@ -632,21 +652,10 @@ int qdev_prop_parse(DeviceState *dev, const char *name,
const char *value)
}
ret = prop->info->parse(dev, prop, value);
if (ret< 0) {
-switch (ret) {
-case -EEXIST:
-qerror_report(QERR_PROPERTY_VALUE_IN_USE,
- dev->info->name, name, value);
-break;
-default:
-case -EINVAL:
-qerror_report(QERR_PROPERTY_VALUE_BAD,
- dev->info->name, name, value);
-break;
-case -ENOENT:
-qerror_report(QERR_PROPERTY_VALUE_NOT_FOUND,
- dev->info->name, name, value);
-break;
-}
+Error *err;
+qdev_prop_error(&err, ret, dev, prop, value);
+qerror_report_err(err);
+error_free(err);
return -1;
}
return 0;
diff --git a/hw/qdev.c b/hw/qdev.c
index c020a6f..c8ab7b7 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -1163,8 +1163,7 @@ static void qdev_set_legacy_property(DeviceState *dev,
Visitor *v, void *opaque,
ret = prop->info->parse(dev, prop, ptr);
if (ret != 0) {
-error_set(errp, QERR_INVALID_PARAMETER_VALUE,
- name, prop->info->name);
+qdev_prop_error(errp, ret, dev, prop, ptr);
}
g_free(ptr);
}
diff --git a/hw/qdev.h b/hw/qdev.h
index 6e18427..828d811 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -370,6 +370,8 @@ void qdev_prop_set_defaults(DeviceState *dev, Property
*props);
void qdev_prop_register_global_list(GlobalProperty *props);
void qdev_prop_set_globals(DeviceState *dev);
+void qdev_prop_error(Error **errp, int ret, DeviceState *name,
+ Property *prop, const char *value);
s/name/dev
And perhaps it would make more sense to return Error * and make the function
name be a constructor:
Error *error_from_qdev_prop_err(int ret, DeviceState *dev,
Property *prop, const char *value);
I was fairly confused about what was going on here at first.
Reards,
Anthony Liguori
static inline const char *qdev_fw_name(DeviceState *dev)
{
[Qemu-devel] [PATCH v2 3/8] qom: push permission checks up into qdev_property_add_legacy
qdev_property_get and qdev_property_set can generate permission
denied errors themselves. Do not duplicate this functionality in
qdev_get/set_legacy_property, and clean up excessive indentation.
Reviewed-by: Anthony Liguori
Signed-off-by: Paolo Bonzini
---
hw/qdev.c | 46 +++---
1 files changed, 19 insertions(+), 27 deletions(-)
diff --git a/hw/qdev.c b/hw/qdev.c
index bda8d6c..c020a6f 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -1135,46 +1135,38 @@ static void qdev_get_legacy_property(DeviceState *dev,
Visitor *v, void *opaque,
{
Property *prop = opaque;
-if (prop->info->print) {
-char buffer[1024];
-char *ptr = buffer;
+char buffer[1024];
+char *ptr = buffer;
-prop->info->print(dev, prop, buffer, sizeof(buffer));
-visit_type_str(v, &ptr, name, errp);
-} else {
-error_set(errp, QERR_PERMISSION_DENIED);
-}
+prop->info->print(dev, prop, buffer, sizeof(buffer));
+visit_type_str(v, &ptr, name, errp);
}
static void qdev_set_legacy_property(DeviceState *dev, Visitor *v, void
*opaque,
const char *name, Error **errp)
{
Property *prop = opaque;
+Error *local_err = NULL;
+char *ptr = NULL;
+int ret;
if (dev->state != DEV_STATE_CREATED) {
error_set(errp, QERR_PERMISSION_DENIED);
return;
}
-if (prop->info->parse) {
-Error *local_err = NULL;
-char *ptr = NULL;
+visit_type_str(v, &ptr, name, &local_err);
+if (local_err) {
+error_propagate(errp, local_err);
+return;
+}
-visit_type_str(v, &ptr, name, &local_err);
-if (!local_err) {
-int ret;
-ret = prop->info->parse(dev, prop, ptr);
-if (ret != 0) {
-error_set(errp, QERR_INVALID_PARAMETER_VALUE,
- name, prop->info->name);
-}
-g_free(ptr);
-} else {
-error_propagate(errp, local_err);
-}
-} else {
-error_set(errp, QERR_PERMISSION_DENIED);
+ret = prop->info->parse(dev, prop, ptr);
+if (ret != 0) {
+error_set(errp, QERR_INVALID_PARAMETER_VALUE,
+ name, prop->info->name);
}
+g_free(ptr);
}
/**
@@ -1194,8 +1186,8 @@ void qdev_property_add_legacy(DeviceState *dev, Property
*prop,
type = g_strdup_printf("legacy<%s>", prop->info->name);
qdev_property_add(dev, prop->name, type,
- qdev_get_legacy_property,
- qdev_set_legacy_property,
+ prop->info->print ? qdev_get_legacy_property : NULL,
+ prop->info->parse ? qdev_set_legacy_property : NULL,
NULL,
prop, errp);
--
1.7.7.1
[Qemu-devel] [PATCH v2 1/8] qapi: protect against NULL QObject in qmp_input_get_object
A NULL qobj can occur when a parameter is fetched via qdict_get, but
the parameter is not in the command. By returning NULL, the caller can
choose whether to raise a missing parameter error, an invalid parameter
type error, or use a default value. For example, qom-set could can
use this to reset a property to its default value, though at this time
it will fail with "Invalid parameter type". In any case, anything is
better than crashing!
Reviewed-by: Anthony Liguori
Signed-off-by: Paolo Bonzini
---
qapi/qmp-input-visitor.c | 10 ++
1 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index 8cbc0ab..c78022b 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -49,10 +49,12 @@ static const QObject *qmp_input_get_object(QmpInputVisitor
*qiv,
qobj = qiv->stack[qiv->nb_stack - 1].obj;
}
-if (name && qobject_type(qobj) == QTYPE_QDICT) {
-return qdict_get(qobject_to_qdict(qobj), name);
-} else if (qiv->nb_stack > 0 && qobject_type(qobj) == QTYPE_QLIST) {
-return qlist_entry_obj(qiv->stack[qiv->nb_stack - 1].entry);
+if (qobj) {
+if (name && qobject_type(qobj) == QTYPE_QDICT) {
+return qdict_get(qobject_to_qdict(qobj), name);
+} else if (qiv->nb_stack > 0 && qobject_type(qobj) == QTYPE_QLIST) {
+return qlist_entry_obj(qiv->stack[qiv->nb_stack - 1].entry);
+}
}
return qobj;
--
1.7.7.1
[Qemu-devel] [PATCH v2 2/8] qom: fix swapped parameters
Reviewed-by: Anthony Liguori
Signed-off-by: Paolo Bonzini
---
hw/qdev.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/hw/qdev.c b/hw/qdev.c
index 83913c7..bda8d6c 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -1110,7 +1110,7 @@ void qdev_property_set(DeviceState *dev, Visitor *v,
const char *name,
if (!prop->set) {
error_set(errp, QERR_PERMISSION_DENIED);
} else {
-prop->set(dev, prop->opaque, v, name, errp);
+prop->set(dev, v, prop->opaque, name, errp);
}
}
--
1.7.7.1
Re: [Qemu-devel] [PATCH 1/8] qapi: fix NULL pointer dereference
Hi, >> What are the uses of null in qdev string properties? I know you can't >> set a string to null since parse() doesn't have a null syntax. So we're >> really just talking about an uninitialized state, right? > > Yes. No ROM BAR is an example of a NULL string property. Both NULL and zero-length string are valid for "empty" though, so you can use -device e1000,romfile=,mac=whatever to boot without pxe rom. Which underlines anthonys point ;) cheers, Gerd
Re: [Qemu-devel] [PATCH 1/8] qapi: fix NULL pointer dereference
On 12/16/2011 04:54 PM, Anthony Liguori wrote: On 12/16/2011 09:42 AM, Paolo Bonzini wrote: On 12/16/2011 04:23 PM, Anthony Liguori wrote: Ok. I think nullable strings are not a good idea simply because it means that a property can have a state that cannot be set. How is this different from NULL links? (Honest, not trick question :)). An empty string == NULL for links. If a pointer is NULL, an empty string is returned. So get/set is full symmetric. Ok, so we can do the same for "pointer" properties. I'm just a bit worried about serial numbers, where "-device virtio-blk-pci,...,serial=" is different from no serial at all. One shows "" in info qtree, the other shows . For JSON, but it doesn't map to a config file easily nor does it map to command line syntax well. You can force nullable properties to have a null default (which is now the case in qdev), but I agree that's not too nice. Paolo
Re: [Qemu-devel] [PATCH 1/8] qapi: fix NULL pointer dereference
On 12/16/2011 09:42 AM, Paolo Bonzini wrote: On 12/16/2011 04:23 PM, Anthony Liguori wrote: Ok. I think nullable strings are not a good idea simply because it means that a property can have a state that cannot be set. How is this different from NULL links? (Honest, not trick question :)). An empty string == NULL for links. If a pointer is NULL, an empty string is returned. So get/set is full symmetric. Long term, I want to be able to do something like dump the current device graph to a config file, and then use that config file to recreate the same machine again. A nullable property without a null representation would not allow this. JSON null is such a representation. For JSON, but it doesn't map to a config file easily nor does it map to command line syntax well. Regards, Anthony Liguori Paolo
Re: [Qemu-devel] [PATCH 1/8] qapi: fix NULL pointer dereference
On 12/16/2011 04:23 PM, Anthony Liguori wrote: Ok. I think nullable strings are not a good idea simply because it means that a property can have a state that cannot be set. How is this different from NULL links? (Honest, not trick question :)). Long term, I want to be able to do something like dump the current device graph to a config file, and then use that config file to recreate the same machine again. A nullable property without a null representation would not allow this. JSON null is such a representation. Paolo
Re: [Qemu-devel] [PATCH 1/8] qapi: fix NULL pointer dereference
On 12/16/2011 09:13 AM, Paolo Bonzini wrote: On 12/16/2011 04:05 PM, Anthony Liguori wrote: What are the uses of null in qdev string properties? I know you can't set a string to null since parse() doesn't have a null syntax. So we're really just talking about an uninitialized state, right? Yes. No ROM BAR is an example of a NULL string property. So it's really filenames and backend names, right? Can we just treat empty strings like NULL? I think all of the various bits handles both the same way. The only case I can think of when it differs is serial numbers. An empty serial number is different from no serial number (which means do not expose one at all). However, DEFINE_PROP_STRING does not allow setting a default, and some device models rely on a NULL value as a default. I see two ways out: 1) add nullable strings; 2) merge without this patch, knowing qom-get is kind-of broken, and proceed adding a default to all DEFINE_PROP_STRING; this would be a merge nightmare for you. I thrive on pain it seems :-) > 3) merge without this patch, knowing it's kind-of broken, and later decide what to do. Ok. I think nullable strings are not a good idea simply because it means that a property can have a state that cannot be set. Long term, I want to be able to do something like dump the current device graph to a config file, and then use that config file to recreate the same machine again. A nullable property without a null representation would not allow this. Regards, Anthony Liguori Let's do (3), and add nullable strings if the discussion stalls. Paolo
Re: [Qemu-devel] [PATCH 1/8] qapi: fix NULL pointer dereference
On 12/16/2011 04:05 PM, Anthony Liguori wrote: What are the uses of null in qdev string properties? I know you can't set a string to null since parse() doesn't have a null syntax. So we're really just talking about an uninitialized state, right? Yes. No ROM BAR is an example of a NULL string property. So it's really filenames and backend names, right? Can we just treat empty strings like NULL? I think all of the various bits handles both the same way. The only case I can think of when it differs is serial numbers. An empty serial number is different from no serial number (which means do not expose one at all). However, DEFINE_PROP_STRING does not allow setting a default, and some device models rely on a NULL value as a default. I see two ways out: 1) add nullable strings; 2) merge without this patch, knowing qom-get is kind-of broken, and proceed adding a default to all DEFINE_PROP_STRING; this would be a merge nightmare for you. 3) merge without this patch, knowing it's kind-of broken, and later decide what to do. Let's do (3), and add nullable strings if the discussion stalls. Paolo
Re: [Qemu-devel] [PATCH v3 00/20] qom: dynamic properties and composition tree
Am 16.12.2011 15:54, schrieb Anthony Liguori:
> On 12/16/2011 08:18 AM, Kevin Wolf wrote:
>> Am 16.12.2011 14:51, schrieb Anthony Liguori:
>>> What I would like to get to eventually is:
>>>
>>> struct ISASerial {
>>> Device parent;
>>>
>>> UART _child uart;
>>> ISABus _link *bus;
>>> };
>>>
>>> A child should be able to be part of the parent devices memory with its life
>>> cycle bound to the parents life cycle. This is why a child property
>>> shouldn't
>>> be nullable.
>>
>> I don't think being bound to the life cycle (that is, from realize on)
>> implies anything about being nullable.
>>
>> For example, imagine two different types of UARTs with a compatible
>> interface, and you could choose whether to have one or the other on the
>> board. Maybe you could even use none at all (probably doesn't make a lot
>> of sense in this example, but I figure it might in other contexts).
>
> What you're describing is what a link<> is. Whenever you want the ability to
> make a choice (including the choice of None), a link<> is the type of
> property
> to use.
>
> But too much choice can be a bad thing. In many cases, you just want to have
> a
> child device for the purposes code sharing. An ISA serial device embedding a
> UART is a good example of this.
>
> Yes, you could make a UARTInterface and have the ISA serial device expose a
> link but that's roughly equivalent to having every chip on
> your
> motherboard be connected with a DIP package instead of being soldered on the
> board. You could do it, but it would be very expensive and cumbersome.
Sure, I'm not saying it's a practical thing to do in this case, I just
wanted to understand the way things are supposed to be modelled. I think
I understand now when it should be a child and when a link.
>> So even though once the device is realized, the UART is bound to the
>> life cycle of your ISASerial, you wouldn't want to have the UART type
>> hard-coded, but leave the user a choice. Would this be modelled as a
>> link rather than a child?
>
> Yes. I'm not terribly sure how this would work yet. A link and a child
> property both acquire references to a device and release a reference to a
> device
> at destruction time.
>
> For a child property, the reference held by the parent is the only reference
> in
> existing. For non-child properties, the 'peripheral' container also holds a
> reference (since you want to be able to assign the device somewhere else in
> the
> device model).
>
> I'm not sure tying life cycles for a user created device makes sense. If a
> user
> creates a device, IMO, the user should be the one to destroy the device.
Yes, that might be the most consistent.
Kevin
Re: [Qemu-devel] [PATCH 1/8] qapi: fix NULL pointer dereference
On 12/16/2011 09:03 AM, Paolo Bonzini wrote: On 12/16/2011 03:56 PM, Anthony Liguori wrote: I'd really prefer to stick to non-nullable strings as there is no obvious way to specify NULL in command line options. We can leave it as the default. A property with a non-null default is implicitly not nullable, which actually makes some sense. We can model this in get_string/set_string too. What are the uses of null in qdev string properties? I know you can't set a string to null since parse() doesn't have a null syntax. So we're really just talking about an uninitialized state, right? Yes. No ROM BAR is an example of a NULL string property. So it's really filenames and backend names, right? Can we just treat empty strings like NULL? I think all of the various bits handles both the same way. Regards, Anthony Liguori Paolo
Re: [Qemu-devel] [PATCH 1/8] qapi: fix NULL pointer dereference
On 12/16/2011 03:56 PM, Anthony Liguori wrote: I'd really prefer to stick to non-nullable strings as there is no obvious way to specify NULL in command line options. We can leave it as the default. A property with a non-null default is implicitly not nullable, which actually makes some sense. We can model this in get_string/set_string too. What are the uses of null in qdev string properties? I know you can't set a string to null since parse() doesn't have a null syntax. So we're really just talking about an uninitialized state, right? Yes. No ROM BAR is an example of a NULL string property. Paolo
Re: [Qemu-devel] [PATCH 1/8] qapi: fix NULL pointer dereference
On 12/16/2011 08:49 AM, Paolo Bonzini wrote: On 12/16/2011 03:46 PM, Anthony Liguori wrote: Hmm, then we have to introduce NULL into QJson and visitors. Visitors assume that strings aren't nullable (which is actually true in JSON and in QString). I also think that string properties shouldn't be nullable. Unfortunately, qdev string properties are nullable and there might well be examples in which empty and NULL are different. I'd rather not risk. But JSON actually has NULL, so not all is lost. I can introduce a nullable_str type in visitors (restricting structs and arrays should be fine, though). I'd really prefer to stick to non-nullable strings as there is no obvious way to specify NULL in command line options. What are the uses of null in qdev string properties? I know you can't set a string to null since parse() doesn't have a null syntax. So we're really just talking about an uninitialized state, right? Regards, Anthony Liguori Paolo
Re: [Qemu-devel] [PATCH v3 00/20] qom: dynamic properties and composition tree
On 12/16/2011 03:54 PM, Anthony Liguori wrote: Yes. I'm not terribly sure how this would work yet. A link and a child property both acquire references to a device and release a reference to a device at destruction time. For a child property, the reference held by the parent is the only reference in existing. For non-child properties, the 'peripheral' container also holds a reference (since you want to be able to assign the device somewhere else in the device model). I'm not sure tying life cycles for a user created device makes sense. If a user creates a device, IMO, the user should be the one to destroy the device. I agree. Paolo
Re: [Qemu-devel] [PATCH v3 00/20] qom: dynamic properties and composition tree
On 12/16/2011 08:18 AM, Kevin Wolf wrote:
Am 16.12.2011 14:51, schrieb Anthony Liguori:
On 12/16/2011 06:24 AM, Paolo Bonzini wrote:
On 12/16/2011 11:36 AM, Kevin Wolf wrote:
I think actually this is not the biggest problem. child properties are
dynamic, and it's not a problem IMO if they are created like that.
That they are added in an init function is an indicator that they aren't
really dynamic.
That's true. However, another indicator is that anything that does not have a
struct field is also not really static. :)
So right now, child properties are all "dynamic" in this sense. This could
change when Anthony converts buses to QOM. The bus right now is embedded into
the HBA's struct, is not a pointer. This likely would change when buses are
QOM-ized, but then the bus would indeed be a 100% static child.
I think having a child property that can be NULL could be
reasonable.
I think Anthony convinced me this is not the case (unlike links). Even if buses
and similar objects are changed to pointers because the implementation needs
that, those pointers should never be NULL (or if they can, the child property
should not exist when they are NULL).
What I would like to get to eventually is:
struct ISASerial {
Device parent;
UART _child uart;
ISABus _link *bus;
};
A child should be able to be part of the parent devices memory with its life
cycle bound to the parents life cycle. This is why a child property shouldn't
be nullable.
I don't think being bound to the life cycle (that is, from realize on)
implies anything about being nullable.
For example, imagine two different types of UARTs with a compatible
interface, and you could choose whether to have one or the other on the
board. Maybe you could even use none at all (probably doesn't make a lot
of sense in this example, but I figure it might in other contexts).
What you're describing is what a link<> is. Whenever you want the ability to
make a choice (including the choice of None), a link<> is the type of property
to use.
But too much choice can be a bad thing. In many cases, you just want to have a
child device for the purposes code sharing. An ISA serial device embedding a
UART is a good example of this.
Yes, you could make a UARTInterface and have the ISA serial device expose a
link but that's roughly equivalent to having every chip on your
motherboard be connected with a DIP package instead of being soldered on the
board. You could do it, but it would be very expensive and cumbersome.
So even though once the device is realized, the UART is bound to the
life cycle of your ISASerial, you wouldn't want to have the UART type
hard-coded, but leave the user a choice. Would this be modelled as a
link rather than a child?
Yes. I'm not terribly sure how this would work yet. A link and a child
property both acquire references to a device and release a reference to a device
at destruction time.
For a child property, the reference held by the parent is the only reference in
existing. For non-child properties, the 'peripheral' container also holds a
reference (since you want to be able to assign the device somewhere else in the
device model).
I'm not sure tying life cycles for a user created device makes sense. If a user
creates a device, IMO, the user should be the one to destroy the device.
Regards,
Anthony Liguori
Kevin
Re: [Qemu-devel] [RFC] Device isolation infrastructure v2
On Thu, Dec 15, 2011 at 11:05:07AM -0700, Alex Williamson wrote: > Starting with it in the core and hand waving some future use that we > don't plan to implement right now seems like the wrong direction. I agree with Alex. First of all, I havn't seen any real vfio problem that can't be solved with the current approach, and it has the great advantage of simplicity. It doesn't require a re-implementation of the driver-core based on groups. I agree that we need some improvements to Alex' code for the dma-api layer to solve the problem with broken devices using the wrong requestor-id. But that can be done incrementally with the current (current == in the iommu-tree) approach implemented by Alex. I also think that all this does not belong into the driver core for two reasons: 1) The information for building the device groups is provided by the iommu-layer 2) The group information is provided to vfio by the iommu-api This makes the iommu-layer the logical point to place the grouping code. There are some sources outside of the iommu-layer that may influence grouping (like pci-quirks), but most of the job is done by the iommu-drivers. Thanks, Joerg -- AMD Operating System Research Center Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach General Managers: Alberto Bozzo, Andrew Bowd Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632
Re: [Qemu-devel] [PATCH 1/8] qapi: fix NULL pointer dereference
On 12/16/2011 03:46 PM, Anthony Liguori wrote: Hmm, then we have to introduce NULL into QJson and visitors. Visitors assume that strings aren't nullable (which is actually true in JSON and in QString). I also think that string properties shouldn't be nullable. Unfortunately, qdev string properties are nullable and there might well be examples in which empty and NULL are different. I'd rather not risk. But JSON actually has NULL, so not all is lost. I can introduce a nullable_str type in visitors (restricting structs and arrays should be fine, though). Paolo
Re: [Qemu-devel] [PATCH 1/8] qapi: fix NULL pointer dereference
On 12/16/2011 08:22 AM, Paolo Bonzini wrote: On 12/16/2011 03:10 PM, Anthony Liguori wrote: If a property function does not set the Error pointer, it must visit something. Hmm, then we have to introduce NULL into QJson and visitors. Visitors assume that strings aren't nullable (which is actually true in JSON and in QString). I also think that string properties shouldn't be nullable. Regards, Anthony Liguori Paolo
Re: [Qemu-devel] [PATCH 6/8] qom: introduce get/set methods for Property
On 12/16/2011 08:18 AM, Paolo Bonzini wrote: On 12/16/2011 03:05 PM, Anthony Liguori wrote: I thought the same initially. However, I noticed that the visitor interfaces for links is also a string. So, even if a block/char/netdev property later becomes a link<>, the interface would not change. The semantics change though. A "drive" link takes a flat block device name. When it's converted to a link, it will take a QOM path. Since block devices will exist in their own directory, it will certainly still be possible to use the flat block device name but since a paths will also be supported, I think it's best to clearly distinguish the link based property from the flat block device name property. But it's a superset, no? My concern is whether you'll get a graceful failure going new->old if you start making use of absolute paths. The type name would change, so I guess that's good enough. Regards, Anthony Liguori Paolo
Re: [Qemu-devel] [PATCH 7/8] qom: distinguish "legacy" property type name from QOM type name
On 12/16/2011 08:18 AM, Paolo Bonzini wrote:
On 12/16/2011 03:06 PM, Anthony Liguori wrote:
- type = g_strdup_printf("legacy<%s>", prop->info->name);
+ type = g_strdup_printf("legacy<%s>",
+ prop->info->legacy_name ?: prop->info->name);
I think this confuses the legacy type with the legacy property names.
I think it would be better to do 'legacy-%s' as then it's at least clear
when something is a type name vs. a property name.
That's only in 8/8. Here I'm not changing property names yet, note that
everything is in prop->info.
This patch prepares for when you'll have a legacy type for property
legacy, and uint32 for iobase. But I can surely rename legacy to
legacy-iobase, if that's what you meant.
No, I got confused. Ignore my comments.
Regards,
Anthony Liguori
paolo
Re: [Qemu-devel] [PATCH 1/8] qapi: fix NULL pointer dereference
On 12/16/2011 03:10 PM, Anthony Liguori wrote: If a property function does not set the Error pointer, it must visit something. Hmm, then we have to introduce NULL into QJson and visitors. Paolo
Re: [Qemu-devel] [PATCH 7/8] qom: distinguish "legacy" property type name from QOM type name
On 12/16/2011 03:06 PM, Anthony Liguori wrote:
-type = g_strdup_printf("legacy<%s>", prop->info->name);
+type = g_strdup_printf("legacy<%s>",
+ prop->info->legacy_name ?: prop->info->name);
I think this confuses the legacy type with the legacy property names.
I think it would be better to do 'legacy-%s' as then it's at least clear
when something is a type name vs. a property name.
That's only in 8/8. Here I'm not changing property names yet, note that
everything is in prop->info.
This patch prepares for when you'll have a legacy type for
property legacy, and uint32 for iobase. But I can surely rename
legacy to legacy-iobase, if that's what you meant.
paolo
Re: [Qemu-devel] [PATCH 6/8] qom: introduce get/set methods for Property
On 12/16/2011 03:05 PM, Anthony Liguori wrote: I thought the same initially. However, I noticed that the visitor interfaces for links is also a string. So, even if a block/char/netdev property later becomes a link<>, the interface would not change. The semantics change though. A "drive" link takes a flat block device name. When it's converted to a link, it will take a QOM path. Since block devices will exist in their own directory, it will certainly still be possible to use the flat block device name but since a paths will also be supported, I think it's best to clearly distinguish the link based property from the flat block device name property. But it's a superset, no? Paolo
Re: [Qemu-devel] [PATCH v3 00/20] qom: dynamic properties and composition tree
Am 16.12.2011 14:51, schrieb Anthony Liguori:
> On 12/16/2011 06:24 AM, Paolo Bonzini wrote:
>> On 12/16/2011 11:36 AM, Kevin Wolf wrote:
I think actually this is not the biggest problem. child properties are
dynamic, and it's not a problem IMO if they are created like that.
>>>
>>> That they are added in an init function is an indicator that they aren't
>>> really dynamic.
>>
>> That's true. However, another indicator is that anything that does not have a
>> struct field is also not really static. :)
>>
>> So right now, child properties are all "dynamic" in this sense. This could
>> change when Anthony converts buses to QOM. The bus right now is embedded into
>> the HBA's struct, is not a pointer. This likely would change when buses are
>> QOM-ized, but then the bus would indeed be a 100% static child.
>>
>>> I think having a child property that can be NULL could be
>>> reasonable.
>>
>> I think Anthony convinced me this is not the case (unlike links). Even if
>> buses
>> and similar objects are changed to pointers because the implementation needs
>> that, those pointers should never be NULL (or if they can, the child property
>> should not exist when they are NULL).
>
> What I would like to get to eventually is:
>
> struct ISASerial {
> Device parent;
>
> UART _child uart;
> ISABus _link *bus;
> };
>
> A child should be able to be part of the parent devices memory with its life
> cycle bound to the parents life cycle. This is why a child property
> shouldn't
> be nullable.
I don't think being bound to the life cycle (that is, from realize on)
implies anything about being nullable.
For example, imagine two different types of UARTs with a compatible
interface, and you could choose whether to have one or the other on the
board. Maybe you could even use none at all (probably doesn't make a lot
of sense in this example, but I figure it might in other contexts).
So even though once the device is realized, the UART is bound to the
life cycle of your ISASerial, you wouldn't want to have the UART type
hard-coded, but leave the user a choice. Would this be modelled as a
link rather than a child?
Kevin
Re: [Qemu-devel] [PATCH v3 00/20] qom: dynamic properties and composition tree
On 12/16/2011 07:52 AM, Paolo Bonzini wrote:
On 12/16/2011 02:51 PM, Anthony Liguori wrote:
struct ISASerial {
Device parent;
UART _child uart;
ISABus _link *bus;
};
A child should be able to be part of the parent devices memory with its
life cycle bound to the parents life cycle. This is why a child
property shouldn't be nullable.
100% agreed in principle.
But technically, if you do not make them pointers how do you handle children
that, for some reason (broken reference counting for example) outlive the
parent? Aborting could be a very good answer. :)
Yes, aborting would be the answer.
Regards,
Anthony Liguori
Paolo
Re: [Qemu-devel] [PATCH 1/8] qapi: fix NULL pointer dereference
On 12/16/2011 08:00 AM, Paolo Bonzini wrote:
On 12/16/2011 02:55 PM, Anthony Liguori wrote:
This is visible with
qom-get path=/i440fx/piix3 property=romfile
after static non-string properties are introduced.
I'm a bit confused about what's happening here. What's the significance
of non-string properties?
Should have been "static non-legacy properties".
When you don't have a value for a property, legacy properties are visited as
"", while the new static properties do not pass anything to the visitor.
I stole this from qdev_property_get_str:
value = prop->get(dev, errp);
if (value) {
visit_type_str(v, &value, name, errp);
g_free(value);
}
I should more clearly document it. NULL would be only return if errp was set.
I just didn't want to introduce a local_err in order to be able to check whether
the function failed.
If a property function does not set the Error pointer, it must visit something.
Regards,
Anthony Liguori
Paolo
Re: [Qemu-devel] [PATCH 7/8] qom: distinguish "legacy" property type name from QOM type name
On 12/16/2011 06:01 AM, Paolo Bonzini wrote:
For non-string properties, there is no reason to distinguish type names
such as "uint32" or "hex32". Restrict those to legacy properties.
Signed-off-by: Paolo Bonzini
---
hw/qdev-properties.c | 12
hw/qdev.c|9 ++---
hw/qdev.h|1 +
3 files changed, 15 insertions(+), 7 deletions(-)
diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index 5e8dd9a..6b6732e 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -86,7 +86,8 @@ static void set_bit(DeviceState *dev, Visitor *v, void
*opaque,
}
PropertyInfo qdev_prop_bit = {
-.name = "on/off",
+.name = "boolean",
+.legacy_name = "on/off",
.type = PROP_TYPE_BIT,
.size = sizeof(uint32_t),
.parse = parse_bit,
@@ -189,7 +190,8 @@ static int print_hex8(DeviceState *dev, Property *prop,
char *dest, size_t len)
}
PropertyInfo qdev_prop_hex8 = {
-.name = "hex8",
+.name = "uint8",
+.legacy_name = "hex8",
.type = PROP_TYPE_UINT8,
.size = sizeof(uint8_t),
.parse = parse_hex8,
@@ -397,7 +399,8 @@ static int print_hex32(DeviceState *dev, Property *prop,
char *dest, size_t len)
}
PropertyInfo qdev_prop_hex32 = {
-.name = "hex32",
+.name = "uint32",
+.legacy_name = "hex32",
.type = PROP_TYPE_UINT32,
.size = sizeof(uint32_t),
.parse = parse_hex32,
@@ -485,7 +488,8 @@ static int print_hex64(DeviceState *dev, Property *prop,
char *dest, size_t len)
}
PropertyInfo qdev_prop_hex64 = {
-.name = "hex64",
+.name = "uint64",
+.legacy_name = "hex64",
.type = PROP_TYPE_UINT64,
.size = sizeof(uint64_t),
.parse = parse_hex64,
diff --git a/hw/qdev.c b/hw/qdev.c
index c020a6f..d76861e 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -218,13 +218,15 @@ int qdev_device_help(QemuOpts *opts)
if (!prop->info->parse) {
continue; /* no way to set it, don't show */
}
-error_printf("%s.%s=%s\n", info->name, prop->name, prop->info->name);
+error_printf("%s.%s=%s\n", info->name, prop->name,
+ prop->info->legacy_name ?: prop->info->name);
}
for (prop = info->bus_info->props; prop&& prop->name; prop++) {
if (!prop->info->parse) {
continue; /* no way to set it, don't show */
}
-error_printf("%s.%s=%s\n", info->name, prop->name, prop->info->name);
+error_printf("%s.%s=%s\n", info->name, prop->name,
+ prop->info->legacy_name ?: prop->info->name);
}
return 1;
}
@@ -1183,7 +1185,8 @@ void qdev_property_add_legacy(DeviceState *dev, Property
*prop,
{
gchar *type;
-type = g_strdup_printf("legacy<%s>", prop->info->name);
+type = g_strdup_printf("legacy<%s>",
+ prop->info->legacy_name ?: prop->info->name);
I think this confuses the legacy type with the legacy property names.
I think it would be better to do 'legacy-%s' as then it's at least clear when
something is a type name vs. a property name.
Regards,
Anthony Liguori
qdev_property_add(dev, prop->name, type,
prop->info->print ? qdev_get_legacy_property : NULL,
diff --git a/hw/qdev.h b/hw/qdev.h
index 9778123..c7d9535 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -156,6 +156,7 @@ enum PropertyType {
struct PropertyInfo {
const char *name;
+const char *legacy_name;
size_t size;
enum PropertyType type;
int64_t min;
Re: [Qemu-devel] [PATCH 6/8] qom: introduce get/set methods for Property
On 12/16/2011 07:51 AM, Paolo Bonzini wrote: On 12/16/2011 02:11 PM, Gerd Hoffmann wrote: I think we should not touch these. First it doesn't buy us much as we are using the old parse/print functions for the visitor-based access, which doesn't look like a good idea to me. Also they will not stay but will be converted to child<> and link<>, so I think it is better to keep the old stuff in the legacy<> namespace. I thought the same initially. However, I noticed that the visitor interfaces for links is also a string. So, even if a block/char/netdev property later becomes a link<>, the interface would not change. The semantics change though. A "drive" link takes a flat block device name. When it's converted to a link, it will take a QOM path. Since block devices will exist in their own directory, it will certainly still be possible to use the flat block device name but since a paths will also be supported, I think it's best to clearly distinguish the link based property from the flat block device name property. Regards, Anthony Liguori Paolo
[Qemu-devel] [PATCH] i440fx: clean up
Add the piix3 child also in the Xen case. Drop the unused piix3
field from PCII440FXState.
Signed-off-by: Paolo Bonzini
---
hw/piix_pci.c |8 +++-
1 files changed, 3 insertions(+), 5 deletions(-)
diff --git a/hw/piix_pci.c b/hw/piix_pci.c
index d785d4b..8d106a8 100644
--- a/hw/piix_pci.c
+++ b/hw/piix_pci.c
@@ -82,7 +82,6 @@ struct PCII440FXState {
MemoryRegion smram_region;
uint8_t smm_enabled;
bool smram_enabled;
-PIIX3State *piix3;
};
@@ -324,12 +323,11 @@ static PCIBus *i440fx_common_init(const char *device_name,
pci_create_simple_multifunction(b, -1, true, "PIIX3"));
pci_bus_irqs(b, piix3_set_irq, pci_slot_get_pirq, piix3,
PIIX_NUM_PIRQS);
-
-qdev_property_add_child(dev, "piix3", &piix3->dev.qdev, NULL);
}
-piix3->pic = pic;
-(*pi440fx_state)->piix3 = piix3;
+qdev_property_add_child(dev, "piix3", &piix3->dev.qdev, NULL);
+
+piix3->pic = pic;
*piix3_devfn = piix3->dev.devfn;
--
1.7.7.1
Re: [Qemu-devel] [PATCH 5/8] qom: introduce QERR_PROPERTY_VALUE_OUT_OF_RANGE
On 12/16/2011 03:00 PM, Anthony Liguori wrote: This will be used when reject invalid values for integer fields that are less than 64-bits wide. Signed-off-by: Paolo Bonzini I'd rather use generic errors when possible. How about VALUE_OUT_OF_RANGE and we can make the message "'%(item)' doesn't take value..." and pass "%s.%s" % (device, property) for item. Ok. Paolo
Re: [Qemu-devel] [PATCH 1/8] qapi: fix NULL pointer dereference
On 12/16/2011 02:55 PM, Anthony Liguori wrote:
This is visible with
qom-get path=/i440fx/piix3 property=romfile
after static non-string properties are introduced.
I'm a bit confused about what's happening here. What's the significance
of non-string properties?
Should have been "static non-legacy properties".
When you don't have a value for a property, legacy properties are
visited as "", while the new static properties do not pass
anything to the visitor.
I stole this from qdev_property_get_str:
value = prop->get(dev, errp);
if (value) {
visit_type_str(v, &value, name, errp);
g_free(value);
}
Paolo
Re: [Qemu-devel] [PATCH 5/8] qom: introduce QERR_PROPERTY_VALUE_OUT_OF_RANGE
On 12/16/2011 06:01 AM, Paolo Bonzini wrote:
This will be used when reject invalid values for integer fields that
are less than 64-bits wide.
Signed-off-by: Paolo Bonzini
I'd rather use generic errors when possible. How about VALUE_OUT_OF_RANGE and
we can make the message "'%(item)' doesn't take value..." and pass "%s.%s" %
(device, property) for item.
Regards,
Anthony Liguori
---
qerror.c |5 +
qerror.h |3 +++
2 files changed, 8 insertions(+), 0 deletions(-)
diff --git a/qerror.c b/qerror.c
index adde8a5..9a75d06 100644
--- a/qerror.c
+++ b/qerror.c
@@ -206,6 +206,11 @@ static const QErrorStringTable qerror_table[] = {
.desc = "Property '%(device).%(property)' can't find value
'%(value)'",
},
{
+.error_fmt = QERR_PROPERTY_VALUE_OUT_OF_RANGE,
+.desc = "Property '%(device).%(property)' doesn't take "
+ "value %(value) (minimum: %(min), maximum: %(max)'",
+},
+{
.error_fmt = QERR_QMP_BAD_INPUT_OBJECT,
.desc = "Expected '%(expected)' in QMP input",
},
diff --git a/qerror.h b/qerror.h
index 9190b02..efda232 100644
--- a/qerror.h
+++ b/qerror.h
@@ -171,6 +171,9 @@ QError *qobject_to_qerror(const QObject *obj);
#define QERR_PROPERTY_VALUE_NOT_FOUND \
"{ 'class': 'PropertyValueNotFound', 'data': { 'device': %s, 'property': %s,
'value': %s } }"
+#define QERR_PROPERTY_VALUE_OUT_OF_RANGE \
+"{ 'class': 'PropertyValueOutOfRange', 'data': { 'device': %s, 'property': %s, 'value': %"PRId64",
'min': %"PRId64", 'max': %"PRId64" } }"
+
#define QERR_QMP_BAD_INPUT_OBJECT \
"{ 'class': 'QMPBadInputObject', 'data': { 'expected': %s } }"
Re: [Qemu-devel] [PATCH 4/8] qom: push permission checks up into qdev_property_add_legacy
On 12/16/2011 06:01 AM, Paolo Bonzini wrote:
qdev_property_get and qdev_property_set can generate permission
denied errors themselves. Do not duplicate this functionality in
qdev_get/set_legacy_property, and clean up excessive indentation.
Signed-off-by: Paolo Bonzini
Nice cleanup.
Reviewed-by: Anthony Liguori
Regards,
Anthony Liguori
---
hw/qdev.c | 46 +++---
1 files changed, 19 insertions(+), 27 deletions(-)
diff --git a/hw/qdev.c b/hw/qdev.c
index bda8d6c..c020a6f 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -1135,46 +1135,38 @@ static void qdev_get_legacy_property(DeviceState *dev,
Visitor *v, void *opaque,
{
Property *prop = opaque;
-if (prop->info->print) {
-char buffer[1024];
-char *ptr = buffer;
+char buffer[1024];
+char *ptr = buffer;
-prop->info->print(dev, prop, buffer, sizeof(buffer));
-visit_type_str(v,&ptr, name, errp);
-} else {
-error_set(errp, QERR_PERMISSION_DENIED);
-}
+prop->info->print(dev, prop, buffer, sizeof(buffer));
+visit_type_str(v,&ptr, name, errp);
}
static void qdev_set_legacy_property(DeviceState *dev, Visitor *v, void
*opaque,
const char *name, Error **errp)
{
Property *prop = opaque;
+Error *local_err = NULL;
+char *ptr = NULL;
+int ret;
if (dev->state != DEV_STATE_CREATED) {
error_set(errp, QERR_PERMISSION_DENIED);
return;
}
-if (prop->info->parse) {
-Error *local_err = NULL;
-char *ptr = NULL;
+visit_type_str(v,&ptr, name,&local_err);
+if (local_err) {
+error_propagate(errp, local_err);
+return;
+}
-visit_type_str(v,&ptr, name,&local_err);
-if (!local_err) {
-int ret;
-ret = prop->info->parse(dev, prop, ptr);
-if (ret != 0) {
-error_set(errp, QERR_INVALID_PARAMETER_VALUE,
- name, prop->info->name);
-}
-g_free(ptr);
-} else {
-error_propagate(errp, local_err);
-}
-} else {
-error_set(errp, QERR_PERMISSION_DENIED);
+ret = prop->info->parse(dev, prop, ptr);
+if (ret != 0) {
+error_set(errp, QERR_INVALID_PARAMETER_VALUE,
+ name, prop->info->name);
}
+g_free(ptr);
}
/**
@@ -1194,8 +1186,8 @@ void qdev_property_add_legacy(DeviceState *dev, Property
*prop,
type = g_strdup_printf("legacy<%s>", prop->info->name);
qdev_property_add(dev, prop->name, type,
- qdev_get_legacy_property,
- qdev_set_legacy_property,
+ prop->info->print ? qdev_get_legacy_property : NULL,
+ prop->info->parse ? qdev_set_legacy_property : NULL,
NULL,
prop, errp);
Re: [Qemu-devel] [PATCH 3/8] qom: fix swapped parameters
On 12/16/2011 06:01 AM, Paolo Bonzini wrote:
Signed-off-by: Paolo Bonzini
Yeah, this code path is currently dead as there is no way to test it until we
defer s/init/realize/ and defer it to guest launch.
In my next series when I introduce Object and move properties into it, we can
write some unit tests to cover this sort of thing.
Reviewed-by: Anthony Liguori
Regards,
Anthony Liguori
---
hw/qdev.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/hw/qdev.c b/hw/qdev.c
index 83913c7..bda8d6c 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -1110,7 +1110,7 @@ void qdev_property_set(DeviceState *dev, Visitor *v,
const char *name,
if (!prop->set) {
error_set(errp, QERR_PERMISSION_DENIED);
} else {
-prop->set(dev, prop->opaque, v, name, errp);
+prop->set(dev, v, prop->opaque, name, errp);
}
}
Re: [Qemu-devel] [PATCH 2/8] qapi: protect against NULL QObject in qmp_input_get_object
On 12/16/2011 06:01 AM, Paolo Bonzini wrote:
A NULL qobj can occur when a parameter is fetched via qdict_get, but
the parameter is not in the command. By returning NULL, the caller can
choose whether to raise a missing parameter error, an invalid parameter
type error, or use a default value. For example, qom-set could can
use this to reset a property to its default value, though at this time
it will fail with "Invalid parameter type". In any case, anything is
better than crashing!
Reviewed-by: Anthony Liguori
Regards,
Anthony Liguori
Signed-off-by: Paolo Bonzini
---
qapi/qmp-input-visitor.c | 10 ++
1 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index 8cbc0ab..c78022b 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -49,10 +49,12 @@ static const QObject *qmp_input_get_object(QmpInputVisitor
*qiv,
qobj = qiv->stack[qiv->nb_stack - 1].obj;
}
-if (name&& qobject_type(qobj) == QTYPE_QDICT) {
-return qdict_get(qobject_to_qdict(qobj), name);
-} else if (qiv->nb_stack> 0&& qobject_type(qobj) == QTYPE_QLIST) {
-return qlist_entry_obj(qiv->stack[qiv->nb_stack - 1].entry);
+if (qobj) {
+if (name&& qobject_type(qobj) == QTYPE_QDICT) {
+return qdict_get(qobject_to_qdict(qobj), name);
+} else if (qiv->nb_stack> 0&& qobject_type(qobj) == QTYPE_QLIST) {
+return qlist_entry_obj(qiv->stack[qiv->nb_stack - 1].entry);
+}
}
return qobj;
Re: [Qemu-devel] [PATCH 1/8] qapi: fix NULL pointer dereference
On 12/16/2011 06:01 AM, Paolo Bonzini wrote:
QAPI currently cannot deal with no object pushed to the stack,
and dereferences a NULL pointer. This is visible with
qom-get path=/i440fx/piix3 property=romfile
after static non-string properties are introduced.
I'm a bit confused about what's happening here. What's the significance of
non-string properties?
Regards,
Anthony Liguori
Signed-off-by: Paolo Bonzini
---
qapi/qmp-output-visitor.c |4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c
index f76d015..29575da 100644
--- a/qapi/qmp-output-visitor.c
+++ b/qapi/qmp-output-visitor.c
@@ -65,13 +65,13 @@ static QObject *qmp_output_pop(QmpOutputVisitor *qov)
static QObject *qmp_output_first(QmpOutputVisitor *qov)
{
QStackEntry *e = QTAILQ_LAST(&qov->stack, QStack);
-return e->value;
+return e ? e->value : NULL;
}
static QObject *qmp_output_last(QmpOutputVisitor *qov)
{
QStackEntry *e = QTAILQ_FIRST(&qov->stack);
-return e->value;
+return e ? e->value : NULL;
}
static void qmp_output_add_obj(QmpOutputVisitor *qov, const char *name,
Re: [Qemu-devel] [PATCH 0/8] qom: introduce non-legacy static properties
On 12/16/2011 06:01 AM, Paolo Bonzini wrote:
QOM right now does not have a way to communicate values for qdev
properties except as strings. This is bad.
This patch improves the Property implementation so that properties
export a visitor-based interface in addition to the string-based
interface. The new interface can then be registered as a "static"
property. It's called static because it uses a struct field for
storage and, as such, should be present in all objects of a given
class.
Excellent! See the patches for individual comments but this is definitely a
good thing to have.
Regards,
Anthony Liguori
Patches 1-3 are bugfixes and patch 4 is a cleanup, so please apply
those at least.
Example using qmp-shell:
x86_64-softmmu/qemu-system-x86_64 \
-hda ~/test.img -snapshot -S \
-qmp unix:/tmp/server.sock,server,nowait \
-netdev type=user,id=net -device virtio-net-pci,netdev=net,id=net \
-net user,vlan=1 -device virtio-net-pci,id=net2,vlan=1 \
-chardev id=stdio,backend=stdio -device isa-serial,chardev=stdio,id=serial
Boolean properties:
(QEMU) qom-get path=/i440fx/piix3 property=command_serr_enable
{u'return': True}
(QEMU) qom-get path=/i440fx/piix3 property=legacy
{u'return': u'on'}
PCI address properties (perhaps will disappear later, but not yet):
(QEMU) qom-get path=/i440fx/piix3 property=addr
{u'return': u'01.0'}
(QEMU) qom-get path=/i440fx/piix3 property=legacy
{u'return': u'01.0'}
String properties (QObject does not have NULL):
(QEMU) qom-get path=/vga property=romfile
{u'return': u'vgabios-cirrus.bin'}
(QEMU) qom-get path=/vga property=legacy
{u'return': u'"vgabios-cirrus.bin"'}
(QEMU) qom-get path=/i440fx/piix3 property=romfile
{u'return': {}}
(QEMU) qom-get path=/i440fx/piix3 property=legacy
{u'return': u''}
MAC properties:
(QEMU) qom-get path=/peripheral/net property=mac
{u'return': u'52:54:00:12:34:56'}
(QEMU) qom-get path=/peripheral/net property=legacy
{u'return': u'52:54:00:12:34:56'}
(QEMU) qom-set path=/peripheral/net property=mac value=52-54-00-12-34-57
{u'error': {u'data': {}, u'class': u'PermissionDenied', u'desc': u'Insufficient
permission to perform this operation'}}
Network properties:
(QEMU) qom-get path=/peripheral/net property=netdev
{u'return': u'net'}
(QEMU) qom-get path=/peripheral/net property=legacy
{u'return': u'net'}
VLAN properties:
(QEMU) qom-get path=/peripheral/net property=vlan
{u'return': {}}
(QEMU) qom-get path=/peripheral/net property=legacy
{u'return': u''}
(QEMU) qom-get path=/peripheral/net2 property=vlan
{u'return': 1}
(QEMU) qom-get path=/peripheral/net2 property=legacy
{u'return': u'1'}
Chardev properties:
(QEMU) qom-get path=/peripheral/serial property=chardev
{u'return': u'stdio'}
(QEMU) qom-get path=/peripheral/serial property=legacy
{u'return': u'stdio'}
Legacy hex32 properties:
(QEMU) qom-get path=/peripheral/serial property=iobase
{u'return': 1016}
(QEMU) qom-get path=/peripheral/serial property=legacy
{u'return': u'0x3f8'}
Examples of setting properties (after disabling the DEV_STATE_CREATED
check for testing only):
(QEMU) qom-set path=/peripheral/net2 property=vlan value=-1
{u'return': {}}
(QEMU) qom-get path=/peripheral/net2 property=vlan
{u'return': {}}
(QEMU) qom-get path=/peripheral/net2 property=legacy
{u'return': u''}
(QEMU) qom-set path=/peripheral/serial property=iobase value=760
{u'return': {}}
(QEMU) qom-get path=/peripheral/serial property=iobase
{u'return': 760}
(QEMU) qom-get path=/peripheral/serial property=legacy
{u'return': u'0x2f8'}
Paolo Bonzini (8):
qapi: fix NULL pointer dereference
qapi: protect against NULL QObject in qmp_input_get_object
qom: fix swapped parameters
qom: push permission checks up into qdev_property_add_legacy
qom: introduce QERR_PROPERTY_VALUE_OUT_OF_RANGE
qom: introduce get/set methods for Property
qom: distinguish "legacy" property type name from QOM type name
qom: register qdev properties also as non-legacy properties
hw/qdev-addr.c| 41 +
hw/qdev-properties.c | 360 -
hw/qdev.c | 85 +++-
hw/qdev.h | 12 +-
qapi/qmp-input-visitor.c | 10 +-
qapi/qmp-output-visitor.c |4 +-
qerror.c |5 +
qerror.h |3 +
8 files changed, 472 insertions(+), 48 deletions(-)
Re: [Qemu-devel] [PATCH v3 00/20] qom: dynamic properties and composition tree
On 12/16/2011 02:51 PM, Anthony Liguori wrote:
struct ISASerial {
Device parent;
UART _child uart;
ISABus _link *bus;
};
A child should be able to be part of the parent devices memory with its
life cycle bound to the parents life cycle. This is why a child
property shouldn't be nullable.
100% agreed in principle.
But technically, if you do not make them pointers how do you handle
children that, for some reason (broken reference counting for example)
outlive the parent? Aborting could be a very good answer. :)
Paolo
Re: [Qemu-devel] [PATCH v3 00/20] qom: dynamic properties and composition tree
On 12/16/2011 07:28 AM, Paolo Bonzini wrote:
On 12/16/2011 01:55 PM, Kevin Wolf wrote:
I don't think that not remembering the child device because you don't
need to reference it any more makes it any less static. You could easily
add the struct member, assign it once and then it matches your
definition of static.
I think in that case you would add a link property, not a child.
And it's not even true of all child devices, for example PCII440FXState
does have a pointer to its piix3 already.
It's unused, actually. You're right, even though I'm not sure whether a
child will be stored in the parent as a Pin or as a qemu_irq.
> I think Anthony convinced me this is not the case (unlike links). Even
> if buses and similar objects are changed to pointers because the
> implementation needs that, those pointers should never be NULL (or if
> they can, the child property should not exist when they are NULL).
So child properties are never configurable, and if a device is optional
or you can choose between multiple devices then it should be a link
instead. That is, device composition in term of "child devices" happens
only hard-coded and the user doesn't do it.
Is this a reasonably accurate description of the intention?
It does sound accurate. In addition, a child property should really model a
parent-child relationship and should create a tree.
As an example, say you want to join two devices ("A" and "B"), by adding a
connection from B to A. Assuming you cannot simply add A as a child to B, you
can do it in several ways:
1) directly add a link property to B.
Examples:
- a local APIC has a link
2) generalizing (1), you can add a link property to B, where X can be any
superclass of A or an interface implemented by A.
Examples:
- an object representing a PCI bus has a link for each
slot (PCIDevice is an abstract class)
- a SCSIBus can have a link to the parent (assuming
SCSIBus is a class and SCSIBusAdapter is an interface)
3) using composition ("has-a") instead of inheritance, you can add a child
property to A and a link property to B.
Example:
- a SCSI bus adapter is a device that has a child.
A SCSIDevice doesn't have a link to the adapter, but only
a link.
- instead of representing PCIDevice as a class, you could have a
child in the children, and a link in
the bus. (I would actually prefer this to an abstract PCIDevice
class, but we agreed that it would be too much churn for now).
- X can be simply a "Pin".
All of these examples match your definition, I tihnk.
For bidirectional links you can have any combination of the above.
Yes, exactly.
Regards,
Anthony Liguori
Paolo
Re: [Qemu-devel] [PATCH v3 00/20] qom: dynamic properties and composition tree
On 12/16/2011 06:24 AM, Paolo Bonzini wrote:
On 12/16/2011 11:36 AM, Kevin Wolf wrote:
> I think actually this is not the biggest problem. child properties are
> dynamic, and it's not a problem IMO if they are created like that.
That they are added in an init function is an indicator that they aren't
really dynamic.
That's true. However, another indicator is that anything that does not have a
struct field is also not really static. :)
So right now, child properties are all "dynamic" in this sense. This could
change when Anthony converts buses to QOM. The bus right now is embedded into
the HBA's struct, is not a pointer. This likely would change when buses are
QOM-ized, but then the bus would indeed be a 100% static child.
I think having a child property that can be NULL could be
reasonable.
I think Anthony convinced me this is not the case (unlike links). Even if buses
and similar objects are changed to pointers because the implementation needs
that, those pointers should never be NULL (or if they can, the child property
should not exist when they are NULL).
What I would like to get to eventually is:
struct ISASerial {
Device parent;
UART _child uart;
ISABus _link *bus;
};
A child should be able to be part of the parent devices memory with its life
cycle bound to the parents life cycle. This is why a child property shouldn't
be nullable.
Regards,
Anthony Liguori
Paolo
Re: [Qemu-devel] [PATCH 6/8] qom: introduce get/set methods for Property
On 12/16/2011 02:11 PM, Gerd Hoffmann wrote:
I think we should not touch these. First it doesn't buy us much as we
are using the old parse/print functions for the visitor-based access,
which doesn't look like a good idea to me. Also they will not stay but
will be converted to child<> and link<>, so I think it is better to keep
the old stuff in the legacy<> namespace.
I thought the same initially. However, I noticed that the visitor
interfaces for links is also a string. So, even if a block/char/netdev
property later becomes a link<>, the interface would not change.
Using the old parse/print functions and get_set/generic is only to avoid
code duplication, I can surely inline everything but it would be uglier.
And again, I found an example in the code of using a similar adapter
pattern (the string properties).
There is one case where I had doubts, namely the PCI address properties.
They will be replaced by links that you set in the parent. However,
in the end I decided to start this way because:
1) QOM properties can still come and go at this stage;
2) The PCI address property can still stay forever as a synthetic
property declared by PCIDevice, so the "qom-get" ABI won't change. The
"qom-set" ABI will, so it might be better to do:
PropertyInfo qdev_prop_pci_devfn = {
.name = "pci-devfn",
.type = PROP_TYPE_UINT32,
.size = sizeof(uint32_t),
.parse = parse_pci_devfn,
.print = print_pci_devfn,
+.get = get_pci_devfn,
+.set = NULL,
};
Advantages: it shows that setting the PCI address is (going to be) a
legacy feature;
Disadvantages: looks a little ad-hoc. See below for an alternative.
Agree on the bit/bool/int types. Although we maybe should apply some
care to integer bus properties, some of them are used for addressing and
will most likely replaced by child<> and link<> too.
Yes, these will also become synthetic and read-only. So an alternative
could be:
for (prop = dev->info->props; prop && prop->name; prop++) {
qdev_property_add_legacy(dev, prop, NULL);
/* Let the generic initializer register alternative definitions
* for qdev properties.
*/
if (!qdev_property_find(dev, prop->name) {
qdev_property_add_static(dev, prop, NULL);
}
}
for (prop = dev->info->bus_info->props; prop && prop->name; prop++) {
qdev_property_add_legacy(dev, prop, NULL);
if (!qdev_property_find(dev, prop->name) {
qdev_property_add_static(dev, prop, NULL);
}
}
For now the pci_devfn property remains read-write, but as soon as the
PCIDevice will be able to define it as synthetic, it will become read-only.
Paolo
Re: [Qemu-devel] [PATCH v3 00/20] qom: dynamic properties and composition tree
On 12/16/2011 04:17 AM, Paolo Bonzini wrote: On 12/16/2011 10:42 AM, Kevin Wolf wrote: > > Applied. > > Regards, > > Anthony Liguori So you pushed this with qdev_property_add_child() calls spread all over the place instead of being treated like other properties?:-( I think actually this is not the biggest problem. child properties are dynamic, and it's not a problem IMO if they are created like that. I don't like that _link_ properties are spread all over the place instead of being treated like other properties. Link properties are static, and PROP_PTR properties could often be converted to links. With your new series, we should be able to add a DEFINE_PROP_LINK() that does the right thing I guess. I've got no objection to that. Regards, Anthony Liguori I'm playing with separating the "legacy" and "static" property concepts so that we can head in that direction. I hope to send out an RFC in an hour or so. Paolo
Re: [Qemu-devel] [PATCH v3 00/20] qom: dynamic properties and composition tree
On 12/16/2011 03:42 AM, Kevin Wolf wrote:
Am 15.12.2011 19:10, schrieb Anthony Liguori:
On 12/12/2011 02:29 PM, Anthony Liguori wrote:
This is a follow up to my previous series to get us started in the QOM
direction. A few things are different this time around. Most notably:
1) Devices no longer have names. Instead, path names are always used to
identify devices.
2) In order to support (1), dynamic properties are now supported.
3) The concept of a "root device" has been introduced. The root device is
roughly modelling the motherboard of a machine. This type is a container
type and it's best to think of it as something like a PCB board I guess.
See my other mail for a description of my merge plan for QOM.
Applied.
Regards,
Anthony Liguori
So you pushed this with qdev_property_add_child() calls spread all over
the place instead of being treated like other properties? :-(
I would really like to be able to do something like:
struct ISASerial {
Device parent;
UART uart;
};
static Properties isa_serial_properties[] = {
DEFINE_PROP_CHILD(ISASerial, uart),
...
};
Actually, I'd really like to do:
struct ISASerial {
Device parent;
UART _child uart;
};
And use an IDL compiler to generate the property table.
But at any rate, to get there, we need to first refactor the device model so
that child devices are being created and registered in the device init
functions. We also need to a bunch more of the qom series so that we can do
in-place initialization of objects.
Right now, a lot of things that are "children" are created in deep corners of
the machine init often times long before the parent is ever created.
Regards,
Anthony Liguori
Kevin
Re: [Qemu-devel] [PATCH v3 00/20] qom: dynamic properties and composition tree
On 12/16/2011 01:55 PM, Kevin Wolf wrote:
I don't think that not remembering the child device because you don't
need to reference it any more makes it any less static. You could easily
add the struct member, assign it once and then it matches your
definition of static.
I think in that case you would add a link property, not a child.
And it's not even true of all child devices, for example PCII440FXState
does have a pointer to its piix3 already.
It's unused, actually. You're right, even though I'm not sure whether a
child will be stored in the parent as a Pin or as a qemu_irq.
> I think Anthony convinced me this is not the case (unlike links). Even
> if buses and similar objects are changed to pointers because the
> implementation needs that, those pointers should never be NULL (or if
> they can, the child property should not exist when they are NULL).
So child properties are never configurable, and if a device is optional
or you can choose between multiple devices then it should be a link
instead. That is, device composition in term of "child devices" happens
only hard-coded and the user doesn't do it.
Is this a reasonably accurate description of the intention?
It does sound accurate. In addition, a child property should really
model a parent-child relationship and should create a tree.
As an example, say you want to join two devices ("A" and "B"), by adding
a connection from B to A. Assuming you cannot simply add A as a
child to B, you can do it in several ways:
1) directly add a link property to B.
Examples:
- a local APIC has a link
2) generalizing (1), you can add a link property to B, where X can be
any superclass of A or an interface implemented by A.
Examples:
- an object representing a PCI bus has a link for each
slot (PCIDevice is an abstract class)
- a SCSIBus can have a link to the parent (assuming
SCSIBus is a class and SCSIBusAdapter is an interface)
3) using composition ("has-a") instead of inheritance, you can add a
child property to A and a link property to B.
Example:
- a SCSI bus adapter is a device that has a child.
A SCSIDevice doesn't have a link to the adapter, but only
a link.
- instead of representing PCIDevice as a class, you could have a
child in the children, and a link in
the bus. (I would actually prefer this to an abstract PCIDevice
class, but we agreed that it would be too much churn for now).
- X can be simply a "Pin".
All of these examples match your definition, I tihnk.
For bidirectional links you can have any combination of the above.
Paolo
Re: [Qemu-devel] [PATCH 6/8] qom: introduce get/set methods for Property
Hi,
> PropertyInfo qdev_prop_drive = {
> .name = "drive",
> .type = PROP_TYPE_DRIVE,
> .size = sizeof(BlockDriverState *),
> .parse = parse_drive,
> .print = print_drive,
> +.get = get_generic,
> +.set = set_generic,
> .free = free_drive,
> };
>
> @@ -407,6 +687,8 @@ PropertyInfo qdev_prop_chr = {
> .size = sizeof(CharDriverState*),
> .parse = parse_chr,
> .print = print_chr,
> +.get = get_generic,
> +.set = set_generic,
> };
>
> /* --- netdev device --- */
> @@ -441,6 +723,8 @@ PropertyInfo qdev_prop_netdev = {
> .size = sizeof(VLANClientState*),
> .parse = parse_netdev,
> .print = print_netdev,
> +.get = get_generic,
> +.set = set_generic,
> };
> PropertyInfo qdev_prop_vlan = {
> .name = "vlan",
> .type = PROP_TYPE_VLAN,
> .size = sizeof(VLANClientState*),
> .parse = parse_vlan,
> .print = print_vlan,
> +.get = get_vlan,
> +.set = set_vlan,
> };
>
> /* --- pointer --- */
> @@ -531,6 +860,8 @@ PropertyInfo qdev_prop_macaddr = {
> .size = sizeof(MACAddr),
> .parse = parse_mac,
> .print = print_mac,
> +.get = get_generic,
> +.set = set_generic,
> };
>
> PropertyInfo qdev_prop_pci_devfn = {
> .name = "pci-devfn",
> .type = PROP_TYPE_UINT32,
> .size = sizeof(uint32_t),
> .parse = parse_pci_devfn,
> .print = print_pci_devfn,
> +.get = get_pci_devfn,
> +.set = set_generic,
> };
I think we should not touch these. First it doesn't buy us much as we
are using the old parse/print functions for the visitor-based access,
which doesn't look like a good idea to me. Also they will not stay but
will be converted to child<> and link<>, so I think it is better to keep
the old stuff in the legacy<> namespace.
Agree on the bit/bool/int types. Although we maybe should apply some
care to integer bus properties, some of them are used for addressing and
will most likely replaced by child<> and link<> too.
cheers,
Gerd
Re: [Qemu-devel] [PATCH v3 00/20] qom: dynamic properties and composition tree
Am 16.12.2011 13:24, schrieb Paolo Bonzini: > On 12/16/2011 11:36 AM, Kevin Wolf wrote: >>> I think actually this is not the biggest problem. child properties are >>> dynamic, and it's not a problem IMO if they are created like that. >> >> That they are added in an init function is an indicator that they aren't >> really dynamic. > > That's true. However, another indicator is that anything that does not > have a struct field is also not really static. :) I don't think that not remembering the child device because you don't need to reference it any more makes it any less static. You could easily add the struct member, assign it once and then it matches your definition of static. And it's not even true of all child devices, for example PCII440FXState does have a pointer to its piix3 already. > So right now, child properties are all "dynamic" in this sense. This > could change when Anthony converts buses to QOM. The bus right now is > embedded into the HBA's struct, is not a pointer. This likely would > change when buses are QOM-ized, but then the bus would indeed be a 100% > static child. > >> I think having a child property that can be NULL could be >> reasonable. > > I think Anthony convinced me this is not the case (unlike links). Even > if buses and similar objects are changed to pointers because the > implementation needs that, those pointers should never be NULL (or if > they can, the child property should not exist when they are NULL). So child properties are never configurable, and if a device is optional or you can choose between multiple devices then it should be a link instead. That is, device composition in term of "child devices" happens only hard-coded and the user doesn't do it. Is this a reasonably accurate description of the intention? Kevin
[Qemu-devel] [PATCH 6/8] qom: introduce get/set methods for Property
This patch adds a visitor interface to Property. This way, QOM will be
able to expose Properties that access a fixed field in a struct without
exposing also the everything-is-a-string "feature" of qdev properties.
Whenever the printed representation in both QOM and qdev (which is
typically the case for device backends), parse/print code can be reused
via get_generic/set_generic. Dually, whenever multiple PropertyInfos
have the same representation in both the struct and the visitors the
code can be reused (for example among all of int32/uint32/hex32).
Signed-off-by: Paolo Bonzini
---
hw/qdev-addr.c | 41 ++
hw/qdev-properties.c | 348 ++
hw/qdev.h|4 +
3 files changed, 393 insertions(+), 0 deletions(-)
diff --git a/hw/qdev-addr.c b/hw/qdev-addr.c
index 305c2d3..5ddda2d 100644
--- a/hw/qdev-addr.c
+++ b/hw/qdev-addr.c
@@ -18,12 +18,53 @@ static int print_taddr(DeviceState *dev, Property *prop,
char *dest, size_t len)
return snprintf(dest, len, "0x" TARGET_FMT_plx, *ptr);
}
+static void get_taddr(DeviceState *dev, Visitor *v, void *opaque,
+ const char *name, Error **errp)
+{
+Property *prop = opaque;
+target_phys_addr_t *ptr = qdev_get_prop_ptr(dev, prop);
+int64_t value;
+
+value = *ptr;
+visit_type_int(v, &value, name, errp);
+}
+
+static void set_taddr(DeviceState *dev, Visitor *v, void *opaque,
+ const char *name, Error **errp)
+{
+Property *prop = opaque;
+target_phys_addr_t *ptr = qdev_get_prop_ptr(dev, prop);
+Error *local_err = NULL;
+int64_t value;
+
+if (dev->state != DEV_STATE_CREATED) {
+error_set(errp, QERR_PERMISSION_DENIED);
+return;
+}
+
+visit_type_int(v, &value, name, &local_err);
+if (local_err) {
+error_propagate(errp, local_err);
+return;
+}
+if ((uint64_t)value <= (uint64_t) ~(target_phys_addr_t)0) {
+*ptr = value;
+} else {
+error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE,
+ dev->id?:"", name, value, (uint64_t) 0,
+ (uint64_t) ~(target_phys_addr_t)0);
+}
+}
+
+
PropertyInfo qdev_prop_taddr = {
.name = "taddr",
.type = PROP_TYPE_TADDR,
.size = sizeof(target_phys_addr_t),
.parse = parse_taddr,
.print = print_taddr,
+.get = get_taddr,
+.set = set_taddr,
};
void qdev_prop_set_taddr(DeviceState *dev, const char *name,
target_phys_addr_t value)
diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index f0b811c..5e8dd9a 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -55,12 +55,44 @@ static int print_bit(DeviceState *dev, Property *prop, char
*dest, size_t len)
return snprintf(dest, len, (*p & qdev_get_prop_mask(prop)) ? "on" : "off");
}
+static void get_bit(DeviceState *dev, Visitor *v, void *opaque,
+const char *name, Error **errp)
+{
+Property *prop = opaque;
+uint32_t *p = qdev_get_prop_ptr(dev, prop);
+bool value = (*p & qdev_get_prop_mask(prop)) != 0;
+
+visit_type_bool(v, &value, name, errp);
+}
+
+static void set_bit(DeviceState *dev, Visitor *v, void *opaque,
+const char *name, Error **errp)
+{
+Property *prop = opaque;
+Error *local_err = NULL;
+bool value;
+
+if (dev->state != DEV_STATE_CREATED) {
+error_set(errp, QERR_PERMISSION_DENIED);
+return;
+}
+
+visit_type_bool(v, &value, name, &local_err);
+if (local_err) {
+error_propagate(errp, local_err);
+return;
+}
+bit_prop_set(dev, prop, value);
+}
+
PropertyInfo qdev_prop_bit = {
.name = "on/off",
.type = PROP_TYPE_BIT,
.size = sizeof(uint32_t),
.parse = parse_bit,
.print = print_bit,
+.get = get_bit,
+.set = set_bit,
};
/* --- 8bit integer --- */
@@ -85,12 +117,54 @@ static int print_uint8(DeviceState *dev, Property *prop,
char *dest, size_t len)
return snprintf(dest, len, "%" PRIu8, *ptr);
}
+static void get_int8(DeviceState *dev, Visitor *v, void *opaque,
+ const char *name, Error **errp)
+{
+Property *prop = opaque;
+int8_t *ptr = qdev_get_prop_ptr(dev, prop);
+int64_t value;
+
+value = *ptr;
+visit_type_int(v, &value, name, errp);
+}
+
+static void set_int8(DeviceState *dev, Visitor *v, void *opaque,
+ const char *name, Error **errp)
+{
+Property *prop = opaque;
+int8_t *ptr = qdev_get_prop_ptr(dev, prop);
+Error *local_err = NULL;
+int64_t value;
+
+if (dev->state != DEV_STATE_CREATED) {
+error_set(errp, QERR_PERMISSION_DENIED);
+return;
+}
+
+visit_type_int(v, &value, name, &local_err);
+if (local_err) {
+error_propagate(errp, local_err);
+return;
+}
+if (value > prop->info->min && value <= prop->info->max) {
+*ptr = value;
+} else {
+error_set(e
[Qemu-devel] [PATCH 5/8] qom: introduce QERR_PROPERTY_VALUE_OUT_OF_RANGE
This will be used when reject invalid values for integer fields that
are less than 64-bits wide.
Signed-off-by: Paolo Bonzini
---
qerror.c |5 +
qerror.h |3 +++
2 files changed, 8 insertions(+), 0 deletions(-)
diff --git a/qerror.c b/qerror.c
index adde8a5..9a75d06 100644
--- a/qerror.c
+++ b/qerror.c
@@ -206,6 +206,11 @@ static const QErrorStringTable qerror_table[] = {
.desc = "Property '%(device).%(property)' can't find value
'%(value)'",
},
{
+.error_fmt = QERR_PROPERTY_VALUE_OUT_OF_RANGE,
+.desc = "Property '%(device).%(property)' doesn't take "
+ "value %(value) (minimum: %(min), maximum: %(max)'",
+},
+{
.error_fmt = QERR_QMP_BAD_INPUT_OBJECT,
.desc = "Expected '%(expected)' in QMP input",
},
diff --git a/qerror.h b/qerror.h
index 9190b02..efda232 100644
--- a/qerror.h
+++ b/qerror.h
@@ -171,6 +171,9 @@ QError *qobject_to_qerror(const QObject *obj);
#define QERR_PROPERTY_VALUE_NOT_FOUND \
"{ 'class': 'PropertyValueNotFound', 'data': { 'device': %s, 'property':
%s, 'value': %s } }"
+#define QERR_PROPERTY_VALUE_OUT_OF_RANGE \
+"{ 'class': 'PropertyValueOutOfRange', 'data': { 'device': %s, 'property':
%s, 'value': %"PRId64", 'min': %"PRId64", 'max': %"PRId64" } }"
+
#define QERR_QMP_BAD_INPUT_OBJECT \
"{ 'class': 'QMPBadInputObject', 'data': { 'expected': %s } }"
--
1.7.7.1
[Qemu-devel] [PATCH 8/8] qom: register qdev properties also as non-legacy properties
Push legacy properties into a legacy<...> namespace, and make them
available with correct types too.
Signed-off-by: Paolo Bonzini
---
hw/qdev.c | 28 +---
hw/qdev.h |7 +++
2 files changed, 28 insertions(+), 7 deletions(-)
diff --git a/hw/qdev.c b/hw/qdev.c
index d76861e..e09f540 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -80,6 +80,9 @@ static DeviceInfo *qdev_find_info(BusInfo *bus_info, const
char *name)
return NULL;
}
+static void qdev_property_add_legacy(DeviceState *dev, Property *prop,
+ Error **errp);
+
static DeviceState *qdev_create_from_info(BusState *bus, DeviceInfo *info)
{
DeviceState *dev;
@@ -104,10 +107,12 @@ static DeviceState *qdev_create_from_info(BusState *bus,
DeviceInfo *info)
for (prop = dev->info->props; prop && prop->name; prop++) {
qdev_property_add_legacy(dev, prop, NULL);
+qdev_property_add_static(dev, prop, NULL);
}
for (prop = dev->info->bus_info->props; prop && prop->name; prop++) {
qdev_property_add_legacy(dev, prop, NULL);
+qdev_property_add_static(dev, prop, NULL);
}
qdev_property_add_str(dev, "type", qdev_get_type, NULL, NULL);
@@ -1175,7 +1180,7 @@ static void qdev_set_legacy_property(DeviceState *dev,
Visitor *v, void *opaque,
* @qdev_add_legacy_property - adds a legacy property
*
* Do not use this is new code! Properties added through this interface will
- * be given types in the "legacy<>" type namespace.
+ * be given names and types in the "legacy<>" type namespace.
*
* Legacy properties are always processed as strings. The format of the string
* depends on the property type.
@@ -1183,18 +1188,35 @@ static void qdev_set_legacy_property(DeviceState *dev,
Visitor *v, void *opaque,
void qdev_property_add_legacy(DeviceState *dev, Property *prop,
Error **errp)
{
-gchar *type;
+gchar *name, *type;
+name = g_strdup_printf("legacy<%s>", prop->name);
type = g_strdup_printf("legacy<%s>",
prop->info->legacy_name ?: prop->info->name);
-qdev_property_add(dev, prop->name, type,
+qdev_property_add(dev, name, type,
prop->info->print ? qdev_get_legacy_property : NULL,
prop->info->parse ? qdev_set_legacy_property : NULL,
NULL,
prop, errp);
g_free(type);
+g_free(name);
+}
+
+/**
+ * @qdev_property_add_static - add a @Property to a device.
+ *
+ * Static properties access data in a struct. The actual type of the
+ * property and the field depends on the property type.
+ */
+void qdev_property_add_static(DeviceState *dev, Property *prop,
+ Error **errp)
+{
+qdev_property_add(dev, prop->name, prop->info->name,
+ prop->info->get, prop->info->set,
+ NULL,
+ prop, errp);
}
DeviceState *qdev_get_root(void)
diff --git a/hw/qdev.h b/hw/qdev.h
index c7d9535..0ba4e3a 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -485,11 +485,10 @@ const char *qdev_property_get_type(DeviceState *dev,
const char *name,
Error **errp);
/**
- * @qdev_property_add_legacy - add a legacy @Property to a device
- *
- * DO NOT USE THIS IN NEW CODE!
+ * @qdev_property_add_static - add a @Property to a device referencing a
+ * field in a struct.
*/
-void qdev_property_add_legacy(DeviceState *dev, Property *prop, Error **errp);
+void qdev_property_add_static(DeviceState *dev, Property *prop, Error **errp);
/**
* @qdev_get_root - returns the root device of the composition tree
--
1.7.7.1
[Qemu-devel] [PATCH 2/8] qapi: protect against NULL QObject in qmp_input_get_object
A NULL qobj can occur when a parameter is fetched via qdict_get, but
the parameter is not in the command. By returning NULL, the caller can
choose whether to raise a missing parameter error, an invalid parameter
type error, or use a default value. For example, qom-set could can
use this to reset a property to its default value, though at this time
it will fail with "Invalid parameter type". In any case, anything is
better than crashing!
Signed-off-by: Paolo Bonzini
---
qapi/qmp-input-visitor.c | 10 ++
1 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index 8cbc0ab..c78022b 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -49,10 +49,12 @@ static const QObject *qmp_input_get_object(QmpInputVisitor
*qiv,
qobj = qiv->stack[qiv->nb_stack - 1].obj;
}
-if (name && qobject_type(qobj) == QTYPE_QDICT) {
-return qdict_get(qobject_to_qdict(qobj), name);
-} else if (qiv->nb_stack > 0 && qobject_type(qobj) == QTYPE_QLIST) {
-return qlist_entry_obj(qiv->stack[qiv->nb_stack - 1].entry);
+if (qobj) {
+if (name && qobject_type(qobj) == QTYPE_QDICT) {
+return qdict_get(qobject_to_qdict(qobj), name);
+} else if (qiv->nb_stack > 0 && qobject_type(qobj) == QTYPE_QLIST) {
+return qlist_entry_obj(qiv->stack[qiv->nb_stack - 1].entry);
+}
}
return qobj;
--
1.7.7.1
[Qemu-devel] [PATCH 4/8] qom: push permission checks up into qdev_property_add_legacy
qdev_property_get and qdev_property_set can generate permission
denied errors themselves. Do not duplicate this functionality in
qdev_get/set_legacy_property, and clean up excessive indentation.
Signed-off-by: Paolo Bonzini
---
hw/qdev.c | 46 +++---
1 files changed, 19 insertions(+), 27 deletions(-)
diff --git a/hw/qdev.c b/hw/qdev.c
index bda8d6c..c020a6f 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -1135,46 +1135,38 @@ static void qdev_get_legacy_property(DeviceState *dev,
Visitor *v, void *opaque,
{
Property *prop = opaque;
-if (prop->info->print) {
-char buffer[1024];
-char *ptr = buffer;
+char buffer[1024];
+char *ptr = buffer;
-prop->info->print(dev, prop, buffer, sizeof(buffer));
-visit_type_str(v, &ptr, name, errp);
-} else {
-error_set(errp, QERR_PERMISSION_DENIED);
-}
+prop->info->print(dev, prop, buffer, sizeof(buffer));
+visit_type_str(v, &ptr, name, errp);
}
static void qdev_set_legacy_property(DeviceState *dev, Visitor *v, void
*opaque,
const char *name, Error **errp)
{
Property *prop = opaque;
+Error *local_err = NULL;
+char *ptr = NULL;
+int ret;
if (dev->state != DEV_STATE_CREATED) {
error_set(errp, QERR_PERMISSION_DENIED);
return;
}
-if (prop->info->parse) {
-Error *local_err = NULL;
-char *ptr = NULL;
+visit_type_str(v, &ptr, name, &local_err);
+if (local_err) {
+error_propagate(errp, local_err);
+return;
+}
-visit_type_str(v, &ptr, name, &local_err);
-if (!local_err) {
-int ret;
-ret = prop->info->parse(dev, prop, ptr);
-if (ret != 0) {
-error_set(errp, QERR_INVALID_PARAMETER_VALUE,
- name, prop->info->name);
-}
-g_free(ptr);
-} else {
-error_propagate(errp, local_err);
-}
-} else {
-error_set(errp, QERR_PERMISSION_DENIED);
+ret = prop->info->parse(dev, prop, ptr);
+if (ret != 0) {
+error_set(errp, QERR_INVALID_PARAMETER_VALUE,
+ name, prop->info->name);
}
+g_free(ptr);
}
/**
@@ -1194,8 +1186,8 @@ void qdev_property_add_legacy(DeviceState *dev, Property
*prop,
type = g_strdup_printf("legacy<%s>", prop->info->name);
qdev_property_add(dev, prop->name, type,
- qdev_get_legacy_property,
- qdev_set_legacy_property,
+ prop->info->print ? qdev_get_legacy_property : NULL,
+ prop->info->parse ? qdev_set_legacy_property : NULL,
NULL,
prop, errp);
--
1.7.7.1
Re: [Qemu-devel] [PATCH v3 00/20] qom: dynamic properties and composition tree
On 12/16/2011 11:36 AM, Kevin Wolf wrote: > I think actually this is not the biggest problem. child properties are > dynamic, and it's not a problem IMO if they are created like that. That they are added in an init function is an indicator that they aren't really dynamic. That's true. However, another indicator is that anything that does not have a struct field is also not really static. :) So right now, child properties are all "dynamic" in this sense. This could change when Anthony converts buses to QOM. The bus right now is embedded into the HBA's struct, is not a pointer. This likely would change when buses are QOM-ized, but then the bus would indeed be a 100% static child. I think having a child property that can be NULL could be reasonable. I think Anthony convinced me this is not the case (unlike links). Even if buses and similar objects are changed to pointers because the implementation needs that, those pointers should never be NULL (or if they can, the child property should not exist when they are NULL). Paolo
[Qemu-devel] [PATCH 1/8] qapi: fix NULL pointer dereference
QAPI currently cannot deal with no object pushed to the stack,
and dereferences a NULL pointer. This is visible with
qom-get path=/i440fx/piix3 property=romfile
after static non-string properties are introduced.
Signed-off-by: Paolo Bonzini
---
qapi/qmp-output-visitor.c |4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c
index f76d015..29575da 100644
--- a/qapi/qmp-output-visitor.c
+++ b/qapi/qmp-output-visitor.c
@@ -65,13 +65,13 @@ static QObject *qmp_output_pop(QmpOutputVisitor *qov)
static QObject *qmp_output_first(QmpOutputVisitor *qov)
{
QStackEntry *e = QTAILQ_LAST(&qov->stack, QStack);
-return e->value;
+return e ? e->value : NULL;
}
static QObject *qmp_output_last(QmpOutputVisitor *qov)
{
QStackEntry *e = QTAILQ_FIRST(&qov->stack);
-return e->value;
+return e ? e->value : NULL;
}
static void qmp_output_add_obj(QmpOutputVisitor *qov, const char *name,
--
1.7.7.1
[Qemu-devel] [PATCH 0/8] qom: introduce non-legacy static properties
QOM right now does not have a way to communicate values for qdev
properties except as strings. This is bad.
This patch improves the Property implementation so that properties
export a visitor-based interface in addition to the string-based
interface. The new interface can then be registered as a "static"
property. It's called static because it uses a struct field for
storage and, as such, should be present in all objects of a given
class.
Patches 1-3 are bugfixes and patch 4 is a cleanup, so please apply
those at least.
Example using qmp-shell:
x86_64-softmmu/qemu-system-x86_64 \
-hda ~/test.img -snapshot -S \
-qmp unix:/tmp/server.sock,server,nowait \
-netdev type=user,id=net -device virtio-net-pci,netdev=net,id=net \
-net user,vlan=1 -device virtio-net-pci,id=net2,vlan=1 \
-chardev id=stdio,backend=stdio -device isa-serial,chardev=stdio,id=serial
Boolean properties:
(QEMU) qom-get path=/i440fx/piix3 property=command_serr_enable
{u'return': True}
(QEMU) qom-get path=/i440fx/piix3 property=legacy
{u'return': u'on'}
PCI address properties (perhaps will disappear later, but not yet):
(QEMU) qom-get path=/i440fx/piix3 property=addr
{u'return': u'01.0'}
(QEMU) qom-get path=/i440fx/piix3 property=legacy
{u'return': u'01.0'}
String properties (QObject does not have NULL):
(QEMU) qom-get path=/vga property=romfile
{u'return': u'vgabios-cirrus.bin'}
(QEMU) qom-get path=/vga property=legacy
{u'return': u'"vgabios-cirrus.bin"'}
(QEMU) qom-get path=/i440fx/piix3 property=romfile
{u'return': {}}
(QEMU) qom-get path=/i440fx/piix3 property=legacy
{u'return': u''}
MAC properties:
(QEMU) qom-get path=/peripheral/net property=mac
{u'return': u'52:54:00:12:34:56'}
(QEMU) qom-get path=/peripheral/net property=legacy
{u'return': u'52:54:00:12:34:56'}
(QEMU) qom-set path=/peripheral/net property=mac value=52-54-00-12-34-57
{u'error': {u'data': {}, u'class': u'PermissionDenied', u'desc': u'Insufficient
permission to perform this operation'}}
Network properties:
(QEMU) qom-get path=/peripheral/net property=netdev
{u'return': u'net'}
(QEMU) qom-get path=/peripheral/net property=legacy
{u'return': u'net'}
VLAN properties:
(QEMU) qom-get path=/peripheral/net property=vlan
{u'return': {}}
(QEMU) qom-get path=/peripheral/net property=legacy
{u'return': u''}
(QEMU) qom-get path=/peripheral/net2 property=vlan
{u'return': 1}
(QEMU) qom-get path=/peripheral/net2 property=legacy
{u'return': u'1'}
Chardev properties:
(QEMU) qom-get path=/peripheral/serial property=chardev
{u'return': u'stdio'}
(QEMU) qom-get path=/peripheral/serial property=legacy
{u'return': u'stdio'}
Legacy hex32 properties:
(QEMU) qom-get path=/peripheral/serial property=iobase
{u'return': 1016}
(QEMU) qom-get path=/peripheral/serial property=legacy
{u'return': u'0x3f8'}
Examples of setting properties (after disabling the DEV_STATE_CREATED
check for testing only):
(QEMU) qom-set path=/peripheral/net2 property=vlan value=-1
{u'return': {}}
(QEMU) qom-get path=/peripheral/net2 property=vlan
{u'return': {}}
(QEMU) qom-get path=/peripheral/net2 property=legacy
{u'return': u''}
(QEMU) qom-set path=/peripheral/serial property=iobase value=760
{u'return': {}}
(QEMU) qom-get path=/peripheral/serial property=iobase
{u'return': 760}
(QEMU) qom-get path=/peripheral/serial property=legacy
{u'return': u'0x2f8'}
Paolo Bonzini (8):
qapi: fix NULL pointer dereference
qapi: protect against NULL QObject in qmp_input_get_object
qom: fix swapped parameters
qom: push permission checks up into qdev_property_add_legacy
qom: introduce QERR_PROPERTY_VALUE_OUT_OF_RANGE
qom: introduce get/set methods for Property
qom: distinguish "legacy" property type name from QOM type name
qom: register qdev properties also as non-legacy properties
hw/qdev-addr.c| 41 +
hw/qdev-properties.c | 360 -
hw/qdev.c | 85 +++-
hw/qdev.h | 12 +-
qapi/qmp-input-visitor.c | 10 +-
qapi/qmp-output-visitor.c |4 +-
qerror.c |5 +
qerror.h |3 +
8 files changed, 472 insertions(+), 48 deletions(-)
--
1.7.7.1
[Qemu-devel] [PATCH 7/8] qom: distinguish "legacy" property type name from QOM type name
For non-string properties, there is no reason to distinguish type names
such as "uint32" or "hex32". Restrict those to legacy properties.
Signed-off-by: Paolo Bonzini
---
hw/qdev-properties.c | 12
hw/qdev.c|9 ++---
hw/qdev.h|1 +
3 files changed, 15 insertions(+), 7 deletions(-)
diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index 5e8dd9a..6b6732e 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -86,7 +86,8 @@ static void set_bit(DeviceState *dev, Visitor *v, void
*opaque,
}
PropertyInfo qdev_prop_bit = {
-.name = "on/off",
+.name = "boolean",
+.legacy_name = "on/off",
.type = PROP_TYPE_BIT,
.size = sizeof(uint32_t),
.parse = parse_bit,
@@ -189,7 +190,8 @@ static int print_hex8(DeviceState *dev, Property *prop,
char *dest, size_t len)
}
PropertyInfo qdev_prop_hex8 = {
-.name = "hex8",
+.name = "uint8",
+.legacy_name = "hex8",
.type = PROP_TYPE_UINT8,
.size = sizeof(uint8_t),
.parse = parse_hex8,
@@ -397,7 +399,8 @@ static int print_hex32(DeviceState *dev, Property *prop,
char *dest, size_t len)
}
PropertyInfo qdev_prop_hex32 = {
-.name = "hex32",
+.name = "uint32",
+.legacy_name = "hex32",
.type = PROP_TYPE_UINT32,
.size = sizeof(uint32_t),
.parse = parse_hex32,
@@ -485,7 +488,8 @@ static int print_hex64(DeviceState *dev, Property *prop,
char *dest, size_t len)
}
PropertyInfo qdev_prop_hex64 = {
-.name = "hex64",
+.name = "uint64",
+.legacy_name = "hex64",
.type = PROP_TYPE_UINT64,
.size = sizeof(uint64_t),
.parse = parse_hex64,
diff --git a/hw/qdev.c b/hw/qdev.c
index c020a6f..d76861e 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -218,13 +218,15 @@ int qdev_device_help(QemuOpts *opts)
if (!prop->info->parse) {
continue; /* no way to set it, don't show */
}
-error_printf("%s.%s=%s\n", info->name, prop->name, prop->info->name);
+error_printf("%s.%s=%s\n", info->name, prop->name,
+ prop->info->legacy_name ?: prop->info->name);
}
for (prop = info->bus_info->props; prop && prop->name; prop++) {
if (!prop->info->parse) {
continue; /* no way to set it, don't show */
}
-error_printf("%s.%s=%s\n", info->name, prop->name, prop->info->name);
+error_printf("%s.%s=%s\n", info->name, prop->name,
+ prop->info->legacy_name ?: prop->info->name);
}
return 1;
}
@@ -1183,7 +1185,8 @@ void qdev_property_add_legacy(DeviceState *dev, Property
*prop,
{
gchar *type;
-type = g_strdup_printf("legacy<%s>", prop->info->name);
+type = g_strdup_printf("legacy<%s>",
+ prop->info->legacy_name ?: prop->info->name);
qdev_property_add(dev, prop->name, type,
prop->info->print ? qdev_get_legacy_property : NULL,
diff --git a/hw/qdev.h b/hw/qdev.h
index 9778123..c7d9535 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -156,6 +156,7 @@ enum PropertyType {
struct PropertyInfo {
const char *name;
+const char *legacy_name;
size_t size;
enum PropertyType type;
int64_t min;
--
1.7.7.1
[Qemu-devel] [PATCH 3/8] qom: fix swapped parameters
Signed-off-by: Paolo Bonzini
---
hw/qdev.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/hw/qdev.c b/hw/qdev.c
index 83913c7..bda8d6c 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -1110,7 +1110,7 @@ void qdev_property_set(DeviceState *dev, Visitor *v,
const char *name,
if (!prop->set) {
error_set(errp, QERR_PERMISSION_DENIED);
} else {
-prop->set(dev, prop->opaque, v, name, errp);
+prop->set(dev, v, prop->opaque, name, errp);
}
}
--
1.7.7.1
[Qemu-devel] [PATCH] usb-host: rip out legacy procfs support
This patch removes support for parsing /proc/bus/usb/devices for device
discovery. The code lacks a few features compared to the sysfs code and
is also bitrotting as everybody has sysfs these days.
This implies having sysfs mounted is mandatory now to use the usb-host
driver. udev isn't required though. qemu will prefer the udev-managed
device nodes below /dev/bus/usb, but in case this directory isn't preset
qemu will use the device nodes below /proc/bus/usb (default usbfs mount
point).
Bottom line: make sure you have both sysfs and usbfs mounted properly,
and everything should continue to work as it did before.
Signed-off-by: Gerd Hoffmann
---
usb-linux.c | 327 ---
1 files changed, 42 insertions(+), 285 deletions(-)
diff --git a/usb-linux.c b/usb-linux.c
index ed14bb1..db41104 100644
--- a/usb-linux.c
+++ b/usb-linux.c
@@ -66,23 +66,9 @@ typedef int USBScanFunc(void *opaque, int bus_num, int addr,
const char *port,
#define DPRINTF(...)
#endif
-#define USBDBG_DEVOPENED "husb: opened %s/devices\n"
-
-#define USBPROCBUS_PATH "/proc/bus/usb"
#define PRODUCT_NAME_SZ 32
#define MAX_ENDPOINTS 15
#define MAX_PORTLEN 16
-#define USBDEVBUS_PATH "/dev/bus/usb"
-#define USBSYSBUS_PATH "/sys/bus/usb"
-
-static char *usb_host_device_path;
-
-#define USB_FS_NONE 0
-#define USB_FS_PROC 1
-#define USB_FS_DEV 2
-#define USB_FS_SYS 3
-
-static int usb_fs_type;
/* endpoint association data */
#define ISO_FRAME_DESC_PER_URB 32
@@ -430,6 +416,31 @@ static void usb_host_async_cancel(USBDevice *dev,
USBPacket *p)
}
}
+static int usb_host_open_device(int bus, int addr)
+{
+const char *usbfs = NULL;
+char filename[32];
+struct stat st;
+int fd, rc;
+
+rc = stat("/dev/bus/usb", &st);
+if (rc == 0 && S_ISDIR(st.st_mode)) {
+/* udev-created device nodes available */
+usbfs = "/dev/bus/usb";
+} else {
+/* fallback: usbfs mounted below /proc */
+usbfs = "/proc/bus/usb";
+}
+
+snprintf(filename, sizeof(filename), "%s/%03d/%03d",
+ usbfs, bus, addr);
+fd = open(filename, O_RDWR | O_NONBLOCK);
+if (fd < 0) {
+fprintf(stderr, "husb: open %s: %s\n", filename, strerror(errno));
+}
+return fd;
+}
+
static int usb_host_claim_port(USBHostDevice *s)
{
#ifdef USBDEVFS_CLAIM_PORT
@@ -459,12 +470,7 @@ static int usb_host_claim_port(USBHostDevice *s)
return -1;
}
-if (!usb_host_device_path) {
-return -1;
-}
-snprintf(line, sizeof(line), "%s/%03d/%03d",
- usb_host_device_path, s->match.bus_num, hub_addr);
-s->hub_fd = open(line, O_RDWR | O_NONBLOCK);
+s->hub_fd = usb_host_open_device(s->match.bus_num, hub_addr);
if (s->hub_fd < 0) {
return -1;
}
@@ -509,10 +515,6 @@ static int usb_linux_get_num_interfaces(USBHostDevice *s)
char device_name[64], line[1024];
int num_interfaces = 0;
-if (usb_fs_type != USB_FS_SYS) {
-return -1;
-}
-
sprintf(device_name, "%d-%s", s->bus_num, s->port);
if (!usb_host_read_file(line, sizeof(line), "bNumInterfaces",
device_name)) {
@@ -1079,41 +1081,21 @@ static int usb_host_handle_control(USBDevice *dev,
USBPacket *p,
static uint8_t usb_linux_get_alt_setting(USBHostDevice *s,
uint8_t configuration, uint8_t interface)
{
-uint8_t alt_setting;
-struct usb_ctrltransfer ct;
-int ret;
-
-if (usb_fs_type == USB_FS_SYS) {
-char device_name[64], line[1024];
-int alt_setting;
+char device_name[64], line[1024];
+int alt_setting;
-sprintf(device_name, "%d-%s:%d.%d", s->bus_num, s->port,
-(int)configuration, (int)interface);
+sprintf(device_name, "%d-%s:%d.%d", s->bus_num, s->port,
+(int)configuration, (int)interface);
-if (!usb_host_read_file(line, sizeof(line), "bAlternateSetting",
-device_name)) {
-goto usbdevfs;
-}
-if (sscanf(line, "%d", &alt_setting) != 1) {
-goto usbdevfs;
-}
-return alt_setting;
-}
-
-usbdevfs:
-ct.bRequestType = USB_DIR_IN | USB_RECIP_INTERFACE;
-ct.bRequest = USB_REQ_GET_INTERFACE;
-ct.wValue = 0;
-ct.wIndex = interface;
-ct.wLength = 1;
-ct.data = &alt_setting;
-ct.timeout = 50;
-ret = ioctl(s->fd, USBDEVFS_CONTROL, &ct);
-if (ret < 0) {
+if (!usb_host_read_file(line, sizeof(line), "bAlternateSetting",
+device_name)) {
+/* Assume alt 0 on error */
+return 0;
+}
+if (sscanf(line, "%d", &alt_setting) != 1) {
/* Assume alt 0 on error */
return 0;
}
-
return alt_setting;
}
@@ -1262,7 +1244,6 @@ static int usb_host_open(USBHostDevice *dev, int bus_num,
const char *prod_name, int speed)
{
int fd = -1, ret;
-char buf[1024];
trace_usb_
Re: [Qemu-devel] [PATCH] kvm: Print something before calling abort() if KVM_RUN fails
On Fri, Dec 16, 2011 at 11:20:20AM +1100, Michael Ellerman wrote: > It's a little unfriendly to call abort() without printing any sort of > error message. So turn the DPRINTK() into an fprintf(stderr, ...). > > Signed-off-by: Michael Ellerman > --- > kvm-all.c |3 ++- > 1 files changed, 2 insertions(+), 1 deletions(-) Thanks, applied to the trivial patches tree: http://repo.or.cz/w/qemu/stefanha.git/shortlog/refs/heads/trivial-patches Stefan
Re: [Qemu-devel] [PATCH] stellaris: Calculate system clock period on reset
On Thu, Dec 15, 2011 at 06:58:26PM +, Peter Maydell wrote: > Calculate the system clock period on reset; otherwise it remains > set to the default value of zero and attempting to use it provokes > a hang. This is one of the issues noted in LP:696094. > > Signed-off-by: Peter Maydell > --- > hw/stellaris.c |1 + > 1 files changed, 1 insertions(+), 0 deletions(-) Thanks, applied to the trivial patches tree: http://repo.or.cz/w/qemu/stefanha.git/shortlog/refs/heads/trivial-patches Stefan
Re: [Qemu-devel] [Qemu-trivial] [PATCH] Makefile.target: Remove unnecessary dependency rules
On Thu, Dec 15, 2011 at 06:28:52PM +, Peter Maydell wrote: > Remove some dependency rules which aren't necessary (the automatically > generated .d files cover all these). These were leftovers from dyngen > days, when the object files also had a dependency on some generated > files. > > Signed-off-by: Peter Maydell > --- > Makefile.target |6 -- > 1 files changed, 0 insertions(+), 6 deletions(-) Thanks, applied to the trivial patches tree: http://repo.or.cz/w/qemu/stefanha.git/shortlog/refs/heads/trivial-patches Stefan
Re: [Qemu-devel] [PATCH v3 00/20] qom: dynamic properties and composition tree
Am 16.12.2011 11:17, schrieb Paolo Bonzini: > On 12/16/2011 10:42 AM, Kevin Wolf wrote: Applied. Regards, Anthony Liguori >> So you pushed this with qdev_property_add_child() calls spread all over >> the place instead of being treated like other properties?:-( > > I think actually this is not the biggest problem. child properties are > dynamic, and it's not a problem IMO if they are created like that. That they are added in an init function is an indicator that they aren't really dynamic. Some of them are conditional, some aren't. For those which are, I think having a child property that can be NULL could be reasonable. In the end you want to have this kind of things configurable via QOM instead of hard-coded in the devices, right? > I don't like that _link_ properties are spread all over the place > instead of being treated like other properties. Link properties are > static, and PROP_PTR properties could often be converted to links. Yes, links too. Kevin
Re: [Qemu-devel] [Xen-devel] [PATCH] xen-qemu: register virtual iommu device on qemu pci bus
On Thursday 15 December 2011 18:33:33 Ian Jackson wrote:
> Wei Wang2 writes ("[Xen-devel] [PATCH] xen-qemu: register virtual iommu
device on qemu pci bus"):
> > Attached patch is for qemu to register iommu device on pci bus. Guest OS
> > requires this to access iommu pci config space in some cases.
>
> Thanks for this submission.
>
> However, we are trying to focus development on the new xen-supporting
> upstream qemu branches.
>
> This old qemu branch is basically dead. We will have to give it
> bugfixes and security fixes, it in parallel with the new trees, for a
> long time (because old guests may need it). So for that reason I
> would really prefer not to add new features to it.
>
> Would you be able to rebase your work on the upstream qemu ? You will
> need Anthony Perard's PCI passthrough series of course, and you will
> also need to liase with qemu upstream (so I've CC'd their list) since
> it will be their tree that you'll be targeting.
>
> If there is a particular reason why this work should be in
> qemu-xen-unstable then please do say. I'm open to being persuaded.
>
> Thanks,
> Ian.
Sure, I will do that. Thanks for the suggestion
Wei.
Re: [Qemu-devel] [patch] replace all strdup() with g_strdup()
On 12/16/2011 10:41 AM, Daniel P. Berrange wrote: Yes& no. In general you are correct that g_malloc/g_strdup needs to be matched with g_free, but in the context of the QEMU binary at least we don't strictly need that. The general issue is that GLib's memory allocators default to the system malloc/free, but withg_mem_set_vtable it is possible to override those allocators. So any libraries using GLib should definitely take care to match g_malloc/g_strdup/g_free, but if you are a self contained program that never calls g_mem_set_vtable, we don't technically have to worry about it. I think the keyword here is "technically". :) If we want to use the GLib profiling allocators or any other kind of statistic gathering, we do have to match the allocations. Right now, we're not getting it 100% right, but sweeping conversions make it harder to just grep-w for malloc/free/strdup. Paolo
Re: [Qemu-devel] [PATCH v3 00/20] qom: dynamic properties and composition tree
On 12/16/2011 10:42 AM, Kevin Wolf wrote: > > Applied. > > Regards, > > Anthony Liguori So you pushed this with qdev_property_add_child() calls spread all over the place instead of being treated like other properties?:-( I think actually this is not the biggest problem. child properties are dynamic, and it's not a problem IMO if they are created like that. I don't like that _link_ properties are spread all over the place instead of being treated like other properties. Link properties are static, and PROP_PTR properties could often be converted to links. I'm playing with separating the "legacy" and "static" property concepts so that we can head in that direction. I hope to send out an RFC in an hour or so. Paolo
