Re: [Qemu-devel] ivshmem property size should be a size, not a string

[Marc-André Lureau]

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.