[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] ivshmem property size should be a size, not a string
From: |
Marc-André Lureau |
Subject: |
Re: [Qemu-devel] ivshmem property size should be a size, not a string |
Date: |
Fri, 20 Nov 2015 13:20:57 -0500 (EST) |
----- Original Message -----
> Eric Blake <address@hidden> writes:
>
> > On 11/20/2015 09:23 AM, Marc-André Lureau wrote:
> >> Hi
> >>
> >> ----- Original Message -----
> >>> Everybody's favourite device model has "size" property. It's declared
> >>> as *string*
> >>>
> >>> DEFINE_PROP_STRING("size", IVShmemState, sizearg),
> >>>
> >>>
> >>> * In QMP, the size must be given as JSON string instead of JSON number,
> >>> and size suffixes are accepted. Example: "size": "512k" instead of
> >>> "size": 524288.
> >>>
> >>> Right now, this violation of QMP rules is tolerable (barely), because
> >>> device_add breaks some of these rules already. However, one goal of
> >>> the current work on QAPI is to support a QMP command to plug devices
> >>> that doesn't break QMP rules, and then this violation will stand out.
> >>>
> >>> Therefore, I want it fixed now, before ivshmem gets used in anger.
> >
> > Was ivshmem even usable in 2.4? I'd argue that if we are going to
> > change it, changing it for 2.5 is the ideal time.
>
> Technically, we already have a compatibility break:
>
> $ qemu-system-x86_64 -nodefaults -S -display none -chardev
> socket,path=/work/armbru/images/ivshmem-socket,id=ivshmem-chr -device
> ivshmem,size=4,chardev=ivshmem-chr,shm=foo
>
> In 2.3.0, this ignores shm with warning "do not specify both 'chardev'
> and 'shm' with ivshmem".
>
> In current upstream, it's rejected.
That's a regression, either we should:
- keep accepting it
- or keep rejection and remove the "WARNING:"
Imho in this case, it was an undefined behaviour, warned, so it's fine to
reject it now.
> I suspect we'd find more if we cared to look.
>
> Of course, the only thing that *really* matters is *actual* usage in
> anger: something of value that breaks.
>
> Hash ivshmem been used in anger? If yes, how?
>
> >>> A straight fix of size isn't fully backwards compatible: suffixes no
> >>> longer work in QMP, and you need to use an 'M' suffix to get Mebibytes
> >>> on command line and HMP.
> >>
> >> I don't know the rules to break properties in qemu, but I would
> >> prefer to avoid it if possible.
> >
> > It's possible to use an alternate to accept both a string and an
> > integer. But in general, QMP _wants_ to use byte-count integers without
> > suffix (the convenience of '512k' is only for the command line and HMP),
> > so letting QMP expose a string of "512k" instead of an integer 524288
> > feels wrong.
>
> Indeed.
>
> >>> If that's unacceptable, we'll have to provide a new, fixed property,
> >>> and deprecate size.
> >>
> >> Sounds a better alternative to me.
> >
> > I'd rather fix the interface rather than catering to ugliness,
> > particularly since 2.5 already fixes so much else that was ugly about
> > ivshmem.
> >
> > Markus, did you have a patch to propose?
>
> Working on one.
>
- [Qemu-devel] ivshmem property size should be a size, not a string, Markus Armbruster, 2015/11/20
- Re: [Qemu-devel] ivshmem property size should be a size, not a string, Marc-André Lureau, 2015/11/20
- Re: [Qemu-devel] ivshmem property size should be a size, not a string, Eric Blake, 2015/11/20
- Re: [Qemu-devel] ivshmem property size should be a size, not a string, Markus Armbruster, 2015/11/20
- Re: [Qemu-devel] ivshmem property size should be a size, not a string,
Marc-André Lureau <=
- Re: [Qemu-devel] ivshmem property size should be a size, not a string, Markus Armbruster, 2015/11/20
- Re: [Qemu-devel] ivshmem property size should be a size, not a string, Marc-André Lureau, 2015/11/20
- Re: [Qemu-devel] ivshmem property size should be a size, not a string, Markus Armbruster, 2015/11/23
- Re: [Qemu-devel] ivshmem property size should be a size, not a string, Marc-André Lureau, 2015/11/23
- Re: [Qemu-devel] ivshmem property size should be a size, not a string, Markus Armbruster, 2015/11/23
- Re: [Qemu-devel] ivshmem property size should be a size, not a string, Marc-André Lureau, 2015/11/23
- Re: [Qemu-devel] ivshmem property size should be a size, not a string, Markus Armbruster, 2015/11/23
- Re: [Qemu-devel] ivshmem property size should be a size, not a string, Marc-André Lureau, 2015/11/23
- Re: [Qemu-devel] ivshmem property size should be a size, not a string, Markus Armbruster, 2015/11/23
- Re: [Qemu-devel] ivshmem property size should be a size, not a string, Marc-André Lureau, 2015/11/23