Re: [Qemu-devel] [PATCH 3/3] vmstate registration: check return values

[Juan Quintela]

"Dr. David Alan Gilbert (git)"  wrote:
> From: "Dr. David Alan Gilbert" 
>
> 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 
> ---
>  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);
> +   _err) < 0) {
> +goto post_realize_fail;
> +}
>  }
>  
>  QLIST_FOREACH(bus, >child_bus, sibling) {

Once that is fixed, I am ok with it.

Reviewed-by: Juan Quintela 

Re: [Qemu-devel] [PATCH 3/3] vmstate registration: check return values

[Peter Maydell]

On 10 January 2017 at 10:34, Dr. David Alan Gilbert  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, _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

Re: [Qemu-devel] [PATCH 3/3] vmstate registration: check return values

[Dr. David Alan Gilbert]

* Peter Maydell (peter.mayd...@linaro.org) wrote:
> On 10 January 2017 at 09:26, Dr. David Alan Gilbert  
> wrote:
> > * Peter Maydell (peter.mayd...@linaro.org) wrote:
> >> On 9 January 2017 at 20:13, Dr. David Alan Gilbert (git)
> >>  wrote:
> >> > From: "Dr. David Alan Gilbert" 
> >> >
> >> > 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 / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 3/3] vmstate registration: check return values

[Peter Maydell]

On 10 January 2017 at 09:26, Dr. David Alan Gilbert  wrote:
> * Peter Maydell (peter.mayd...@linaro.org) wrote:
>> On 9 January 2017 at 20:13, Dr. David Alan Gilbert (git)
>>  wrote:
>> > From: "Dr. David Alan Gilbert" 
>> >
>> > 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

Re: [Qemu-devel] [PATCH 3/3] vmstate registration: check return values

[Dr. David Alan Gilbert]

* Peter Maydell (peter.mayd...@linaro.org) wrote:
> On 9 January 2017 at 20:13, Dr. David Alan Gilbert (git)
>  wrote:
> > From: "Dr. David Alan Gilbert" 
> >
> > 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 / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 3/3] vmstate registration: check return values

[Peter Maydell]

On 9 January 2017 at 20:13, Dr. David Alan Gilbert (git)
 wrote:
> From: "Dr. David Alan Gilbert" 
>
> 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

[Qemu-devel] [PATCH 3/3] vmstate registration: check return values

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" 

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 
---
 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,
dev->alias_required_for_version,
-   NULL);
+   _err) < 0) {
+goto post_realize_fail;
+}
 }
 
 QLIST_FOREACH(bus, >child_bus, sibling) {
-- 
2.9.3