[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 11:23:08 -0500 (EST) |
Hi
----- Original Message -----
> Everybody's favourite device model has "size" property. It's declared
> as *string*
>
> DEFINE_PROP_STRING("size", IVShmemState, sizearg),
>
> which gets converted to a size manually in the realize method:
>
> } else if (s->sizearg == NULL) {
> s->ivshmem_size = 4 << 20; /* 4 MB default */
> } else {
> char *end;
> int64_t size = qemu_strtosz(s->sizearg, &end);
> if (size < 0 || *end != '\0' || !is_power_of_2(size)) {
> error_setg(errp, "Invalid size %s", s->sizearg);
> return;
> }
> s->ivshmem_size = size;
> }
>
> This is *wrong*. Impact, as far as I can tell:
>
> * -device ivshmem,help shows the property as 'str' instead of 'size'.
>
> Unhelpful, but hardly show-stopper.
>
> * On the command line and in HMP, ivshmem's size is parsed differently
> than other size properties. In particular, a number without a suffix
> is normally interpreted as bytes, except for ivshmem, where it's
> Mebibytes.
>
> Ugly inconsistency, but hardly the only one.
>
> * 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.
>
> 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.
> If that's unacceptable, we'll have to provide a new, fixed property,
> and deprecate size.
Sounds a better alternative to me.
- [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, 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, 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, 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