Message ID | 1397828484-21771-4-git-send-email-batuzovk@ispras.ru |
---|---|
State | New |
Headers | show |
Am 18.04.2014 15:41, schrieb Kirill Batuzov: > Error set by error_set is dynamically allocated and needs to be cleared > properly later. graphic_console_init neither needs error descriptions nor frees > them. Pass NULL instead of actual pointers to avoid unnecessary memory > allocations. > > Signed-off-by: Kirill Batuzov <batuzovk@ispras.ru> > --- > ui/console.c | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/ui/console.c b/ui/console.c > index e057755..438b6bd 100644 > --- a/ui/console.c > +++ b/ui/console.c > @@ -1590,7 +1590,6 @@ QemuConsole *graphic_console_init(DeviceState *dev, uint32_t head, > const GraphicHwOps *hw_ops, > void *opaque) > { > - Error *local_err = NULL; > int width = 640; > int height = 480; > QemuConsole *s; > @@ -1602,10 +1601,8 @@ QemuConsole *graphic_console_init(DeviceState *dev, uint32_t head, > s->hw_ops = hw_ops; > s->hw = opaque; > if (dev) { > - object_property_set_link(OBJECT(s), OBJECT(dev), > - "device", &local_err); > - object_property_set_int(OBJECT(s), head, > - "head", &local_err); > + object_property_set_link(OBJECT(s), OBJECT(dev), "device", NULL); > + object_property_set_int(OBJECT(s), head, "head", NULL); I guess it would be better to use &error_abort rather than failing silently. CC'ing Gerd. Regards, Andreas > } > > s->surface = qemu_create_displaysurface(width, height);
> > if (dev) { > > - object_property_set_link(OBJECT(s), OBJECT(dev), > > - "device", &local_err); > > - object_property_set_int(OBJECT(s), head, > > - "head", &local_err); > > + object_property_set_link(OBJECT(s), OBJECT(dev), "device", NULL); > > + object_property_set_int(OBJECT(s), head, "head", NULL); > > I guess it would be better to use &error_abort rather than failing > silently. Agree. cheers, Gerd
On Tue, 22 Apr 2014, Gerd Hoffmann wrote: > > > if (dev) { > > > - object_property_set_link(OBJECT(s), OBJECT(dev), > > > - "device", &local_err); > > > - object_property_set_int(OBJECT(s), head, > > > - "head", &local_err); > > > + object_property_set_link(OBJECT(s), OBJECT(dev), "device", NULL); > > > + object_property_set_int(OBJECT(s), head, "head", NULL); > > > > I guess it would be better to use &error_abort rather than failing > > silently. > > Agree. > The result of this change was a bit unexpected. QEMU fails to start with the error "Insufficient permission to perform this operation". This happens because "head" is a read-only property. Fortunately it is usually (always?) zero at creation time and we always try to write zero also. So it works somehow. But non-zero head will not work. We need either to initialize it in new_console with right head value or to change it to simple struct field. It depends entirely on our future plans for it. In v2 I also plan to change all occurrences of &local_err to &error_abort in ui/console.c They all are unchecked and can cause memory leaks and/or weird silent failures like setting of "head" property had.
Hi, > We need either to initialize it in new_console with right head value or > to change it to simple struct field. It depends entirely on our future > plans for it. It'll never change after initialization, so keeping it read-only and initialize in new_console sounds good to me. cheers, Gerd
diff --git a/ui/console.c b/ui/console.c index e057755..438b6bd 100644 --- a/ui/console.c +++ b/ui/console.c @@ -1590,7 +1590,6 @@ QemuConsole *graphic_console_init(DeviceState *dev, uint32_t head, const GraphicHwOps *hw_ops, void *opaque) { - Error *local_err = NULL; int width = 640; int height = 480; QemuConsole *s; @@ -1602,10 +1601,8 @@ QemuConsole *graphic_console_init(DeviceState *dev, uint32_t head, s->hw_ops = hw_ops; s->hw = opaque; if (dev) { - object_property_set_link(OBJECT(s), OBJECT(dev), - "device", &local_err); - object_property_set_int(OBJECT(s), head, - "head", &local_err); + object_property_set_link(OBJECT(s), OBJECT(dev), "device", NULL); + object_property_set_int(OBJECT(s), head, "head", NULL); } s->surface = qemu_create_displaysurface(width, height);
Error set by error_set is dynamically allocated and needs to be cleared properly later. graphic_console_init neither needs error descriptions nor frees them. Pass NULL instead of actual pointers to avoid unnecessary memory allocations. Signed-off-by: Kirill Batuzov <batuzovk@ispras.ru> --- ui/console.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)