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

[Bruce Rogers]

>>> On 11/20/2015 at 09:07 AM, Markus Armbruster <address@hidden> wrote: 
> 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.
> 
>   If that's unacceptable, we'll have to provide a new, fixed property,
>   and deprecate size.
> 
>   Opinions?


At the request of a customer, SUSE began basic support of the ivshmem
interface as of our SLE 12 release, which included QEMU v2.0.0. (This is
around the same time frame that it was also wired up in libvirt in v1.2.10.)

As has been pointed out, there are issues with its usability, but lets not
break existing users thoughtlessly. I certainly support efforts to make
improvements, and to even deprecate the problematic interfaces as suitable
substitutes are added, but to do so in an order fashion allowing users time to
migrate to the new interface instead of breaking them all of a sudden.

Bruce