From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54638) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a0yBD-00064k-VL for qemu-devel@nongnu.org; Mon, 23 Nov 2015 15:57:44 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a0yB8-00016x-W9 for qemu-devel@nongnu.org; Mon, 23 Nov 2015 15:57:43 -0500 Received: from prv-mh.provo.novell.com ([137.65.248.74]:55545) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a0yB8-00014z-Oh for qemu-devel@nongnu.org; Mon, 23 Nov 2015 15:57:38 -0500 Message-Id: <56531B4F02000048001220CA@prv-mh.provo.novell.com> Date: Mon, 23 Nov 2015 13:57:35 -0700 From: "Bruce Rogers" References: <87si401wpf.fsf@blackfin.pond.sub.org> In-Reply-To: <87si401wpf.fsf@blackfin.pond.sub.org> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: quoted-printable Content-Disposition: inline Subject: Re: [Qemu-devel] ivshmem property size should be a size, not a string List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org, Markus Armbruster Cc: marcandre.lureau@redhat.com, Claudio Fontana , Luiz Capitulino >>> On 11/20/2015 at 09:07 AM, Markus Armbruster = wrote:=20 > Everybody's favourite device model has "size" property. It's declared > as *string* >=20 > DEFINE_PROP_STRING("size", IVShmemState, sizearg), >=20 > which gets converted to a size manually in the realize method: >=20 > } else if (s->sizearg =3D=3D NULL) { > s->ivshmem_size =3D 4 << 20; /* 4 MB default */ > } else { > char *end; > int64_t size =3D qemu_strtosz(s->sizearg, &end); > if (size < 0 || *end !=3D '\0' || !is_power_of_2(size)) { > error_setg(errp, "Invalid size %s", s->sizearg); > return; > } > s->ivshmem_size =3D size; > } >=20 > This is *wrong*. Impact, as far as I can tell: >=20 > * -device ivshmem,help shows the property as 'str' instead of 'size'. >=20 > Unhelpful, but hardly show-stopper. >=20 > * 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. >=20 > Ugly inconsistency, but hardly the only one. >=20 > * 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. >=20 > 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. >=20 > Therefore, I want it fixed now, before ivshmem gets used in anger. >=20 > 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. >=20 > If that's unacceptable, we'll have to provide a new, fixed property, > and deprecate size. >=20 > 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