| Message ID | 20170109201340.16593-4-dgilbert@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 9 January 2017 at 20:13, Dr. David Alan Gilbert (git) <dgilbert@redhat.com> wrote: > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > Check qdev's call to vmstate_register_with_alias_id; that gets > most of the common uses; there's hundreds of calls via vmstate_register > which could get fixed over time. Not quite that bad, I think -- I make it just over 50 calls. thanks -- PMM
* Peter Maydell (peter.maydell@linaro.org) wrote: > On 9 January 2017 at 20:13, Dr. David Alan Gilbert (git) > <dgilbert@redhat.com> wrote: > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > > > Check qdev's call to vmstate_register_with_alias_id; that gets > > most of the common uses; there's hundreds of calls via vmstate_register > > which could get fixed over time. > > Not quite that bad, I think -- I make it just over 50 calls. Well kind of; it seems to be a bit more complicated than that. I'd grep'd for vmstate_register and that gives me ~180 (including stuff in headers). Only 56 of those are vmstate_register() calls though, 117 are vmstate_register_ram calls which I'd not previously looked at, those call qemu_ram_set_idstr which looks like it suffers from the same problem though. Dave > thanks > -- PMM -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 10 January 2017 at 09:26, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote: > * Peter Maydell (peter.maydell@linaro.org) wrote: >> On 9 January 2017 at 20:13, Dr. David Alan Gilbert (git) >> <dgilbert@redhat.com> wrote: >> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> >> > >> > Check qdev's call to vmstate_register_with_alias_id; that gets >> > most of the common uses; there's hundreds of calls via vmstate_register >> > which could get fixed over time. >> >> Not quite that bad, I think -- I make it just over 50 calls. > > Well kind of; it seems to be a bit more complicated than that. > I'd grep'd for vmstate_register and that gives me ~180 (including > stuff in headers). Yes, I was specifically looking at the vmstate_register and vmstate_register_with_alias_id ones. > Only 56 of those are vmstate_register() calls though, 117 are > vmstate_register_ram calls which I'd not previously looked at, > those call qemu_ram_set_idstr which looks like it suffers from > the same problem though. They call qemu_ram_set_idstr with the memory region name string, though, which is "used for debugging; not visible to the user or ABI", so we can just say it's a bug to use a silly name and assert if it's too big, right? thanks -- PMM
* Peter Maydell (peter.maydell@linaro.org) wrote: > On 10 January 2017 at 09:26, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote: > > * Peter Maydell (peter.maydell@linaro.org) wrote: > >> On 9 January 2017 at 20:13, Dr. David Alan Gilbert (git) > >> <dgilbert@redhat.com> wrote: > >> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > >> > > >> > Check qdev's call to vmstate_register_with_alias_id; that gets > >> > most of the common uses; there's hundreds of calls via vmstate_register > >> > which could get fixed over time. > >> > >> Not quite that bad, I think -- I make it just over 50 calls. > > > > Well kind of; it seems to be a bit more complicated than that. > > I'd grep'd for vmstate_register and that gives me ~180 (including > > stuff in headers). > > Yes, I was specifically looking at the vmstate_register and > vmstate_register_with_alias_id ones. > > > Only 56 of those are vmstate_register() calls though, 117 are > > vmstate_register_ram calls which I'd not previously looked at, > > those call qemu_ram_set_idstr which looks like it suffers from > > the same problem though. > > They call qemu_ram_set_idstr with the memory region name string, > though, which is "used for debugging; not visible to the user > or ABI", so we can just say it's a bug to use a silly name > and assert if it's too big, right? qemu_ram_set_idstr already abort's if it hits a dupe (which after making sure it doesn't overflow the buffer is what we end up with if we have long names); so yes we already abort in that case. However, it's a bit optimistic of the memory region to claim the name is just for debug; Migration/ram.c transmits the RAMBlock's idstr on the wire (as does postcopy) - so I think the memory.h comment is wrong. I don't think it's a big problem since you're unlikely to hit these big names in practice; but it would be better to return an error rather than assert/abort since then you wouldn't abort as part of a hot-add. So it's worth taking the common cases as this patch does; I don't think it's worth the hastle of changing 100+ calls though. Dave > thanks > -- PMM -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 10 January 2017 at 10:34, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote: > However, it's a bit optimistic of the memory region to claim the name > is just for debug; Migration/ram.c transmits the RAMBlock's idstr on > the wire (as does postcopy) - so I think the memory.h comment > is wrong. We'd better fix that, then, or we'll find ourselves breaking migration compat by accident... > I don't think it's a big problem since you're unlikely to hit these > big names in practice; but it would be better to return an error > rather than assert/abort since then you wouldn't abort as part > of a hot-add. Almost all of the calls aren't going to be hot-add, though. > So it's worth taking the common cases as this patch does; I don't > think it's worth the hastle of changing 100+ calls though. You also have the code paths via memory_region_allocate_system_memory which end up calling vmstate_register_ram_global which then calls qemu_ram_set_idstr -- none of that has support for returning an error. (Aside: at some point I want to introduce a memory_region_allocate_aux_memory() which wraps the common pattern memory_region_init_ram(mr, NULL, name, size, &error_fatal); vmstate_register_ram_global(mr); so we have a simple way to create RAM blocks which aren't the main system ram, by analogy with memory_region_allocate_system_memory().) thanks -- PMM
"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote: > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > Check qdev's call to vmstate_register_with_alias_id; that gets > most of the common uses; there's hundreds of calls via vmstate_register > which could get fixed over time. > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > --- > hw/core/qdev.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > index ea97b15..df633d0 100644 > --- a/hw/core/qdev.c > +++ b/hw/core/qdev.c > @@ -933,10 +933,12 @@ static void device_set_realized(Object *obj, bool value, Error **errp) > } > > if (qdev_get_vmsd(dev)) { > - vmstate_register_with_alias_id(dev, -1, qdev_get_vmsd(dev), dev, > + if (vmstate_register_with_alias_id(dev, -1, qdev_get_vmsd(dev), dev, > dev->instance_id_alias, Indent this correctly, please. > dev->alias_required_for_version, > - NULL); > + &local_err) < 0) { > + goto post_realize_fail; > + } > } > > QLIST_FOREACH(bus, &dev->child_bus, sibling) { Once that is fixed, I am ok with it. Reviewed-by: Juan Quintela <quintela@redhat.com>
diff --git a/hw/core/qdev.c b/hw/core/qdev.c index ea97b15..df633d0 100644 --- a/hw/core/qdev.c +++ b/hw/core/qdev.c @@ -933,10 +933,12 @@ static void device_set_realized(Object *obj, bool value, Error **errp) } if (qdev_get_vmsd(dev)) { - vmstate_register_with_alias_id(dev, -1, qdev_get_vmsd(dev), dev, + if (vmstate_register_with_alias_id(dev, -1, qdev_get_vmsd(dev), dev, dev->instance_id_alias, dev->alias_required_for_version, - NULL); + &local_err) < 0) { + goto post_realize_fail; + } } QLIST_FOREACH(bus, &dev->child_bus, sibling) {