[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 3/3] vmstate registration: check return values
From: |
Dr. David Alan Gilbert |
Subject: |
Re: [Qemu-devel] [PATCH 3/3] vmstate registration: check return values |
Date: |
Tue, 10 Jan 2017 10:34:00 +0000 |
User-agent: |
Mutt/1.7.1 (2016-10-04) |
* Peter Maydell (address@hidden) wrote:
> On 10 January 2017 at 09:26, Dr. David Alan Gilbert <address@hidden> wrote:
> > * Peter Maydell (address@hidden) wrote:
> >> On 9 January 2017 at 20:13, Dr. David Alan Gilbert (git)
> >> <address@hidden> wrote:
> >> > From: "Dr. David Alan Gilbert" <address@hidden>
> >> >
> >> > 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 / address@hidden / Manchester, UK