From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39445) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X8Avj-0000Dy-97 for qemu-devel@nongnu.org; Fri, 18 Jul 2014 12:22:50 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1X8Ava-0001vc-35 for qemu-devel@nongnu.org; Fri, 18 Jul 2014 12:22:43 -0400 Received: from mx1.redhat.com ([209.132.183.28]:44351) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X8AvZ-0001vQ-Oa for qemu-devel@nongnu.org; Fri, 18 Jul 2014 12:22:34 -0400 Message-ID: <53C949C1.6090206@redhat.com> Date: Fri, 18 Jul 2014 12:22:25 -0400 From: John Snow MIME-Version: 1.0 References: <1405630053-15052-1-git-send-email-jsnow@redhat.com> <87zjg7yyds.fsf@blackfin.pond.sub.org> <20140718074623.GC6960@grmbl.mre> <87a986vry1.fsf@blackfin.pond.sub.org> <20140718112730.GA26614@grmbl.mre> <874myesx0m.fsf@blackfin.pond.sub.org> <20140718121451.GC26614@grmbl.mre> <8761iurenc.fsf@blackfin.pond.sub.org> In-Reply-To: <8761iurenc.fsf@blackfin.pond.sub.org> Content-Type: multipart/alternative; boundary="------------030500030102020502010506" Subject: Re: [Qemu-devel] [PATCH v2] virtio-rng: Add human-readable error message for negative max-bytes parameter List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster , Amit Shah Cc: peter.maydell@linaro.org, qemu-devel@nongnu.org This is a multi-part message in MIME format. --------------030500030102020502010506 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit On 07/18/2014 09:16 AM, Markus Armbruster wrote: > Amit Shah writes: > >> On (Fri) 18 Jul 2014 [13:54:01], Markus Armbruster wrote: >>> Amit Shah writes: >>> >>>> On (Fri) 18 Jul 2014 [13:15:18], Markus Armbruster wrote: >>>>> Amit Shah writes: >>>>> >>>>>> On (Fri) 18 Jul 2014 [08:27:59], Markus Armbruster wrote: >>>>>>> John Snow writes: >>>>>>> >>>>>>>> If a negative integer is used for the max_bytes parameter, QEMU currently >>>>>>>> calls abort() and leaves behind a core dump. This patch adds a simple >>>>>>>> error message to make the reason for the termination clearer. >>>>>>>> >>>>>>>> Signed-off-by: John Snow >>>>>>>> --- >>>>>>>> v2: Changed 0L constant to (uint64_t)0 constant to match PRId64 format code >>>>>>>> on both 32bit and 64bit systems. Tested via -m32 flag. >>>>>>>> >>>>>>>> hw/virtio/virtio-rng.c | 6 +++++- >>>>>>>> 1 file changed, 5 insertions(+), 1 deletion(-) >>>>>>>> >>>>>>>> diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c >>>>>>>> index 1356aca..64c7d23 100644 >>>>>>>> --- a/hw/virtio/virtio-rng.c >>>>>>>> +++ b/hw/virtio/virtio-rng.c >>>>>>>> @@ -181,7 +181,11 @@ static void virtio_rng_device_realize(DeviceState *dev, Error **errp) >>>>>>>> >>>>>>>> vrng->vq = virtio_add_queue(vdev, 8, handle_input); >>>>>>>> >>>>>>>> - assert(vrng->conf.max_bytes <= INT64_MAX); >>>>>>>> + if (vrng->conf.max_bytes > INT64_MAX) { >>>>>>>> + error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE, "virtio-rng", >>>>>>>> + "max_bytes", vrng->conf.max_bytes, (uint64_t)0, INT64_MAX); >>> Missed this initially: the property name is "max-bytes", not >>> "max_bytes". Please fix. >>> >>>>>>>> + return; >>>>>>>> + } >>>>>>>> vrng->quota_remaining = vrng->conf.max_bytes; >>>>>>>> >>>>>>>> vrng->rate_limit_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL, >>>>>>> Elsewhere in this function, we use >>>>>>> >>>>>>> error_set(errp, QERR_INVALID_PARAMETER_VALUE, "period", >>>>>>> "a positive number"); >>>>>>> >>>>>>> Existing uses of QERR_PROPERTY_VALUE_OUT_OF_RANGE are all for intervals >>>>>>> with small bounds. >>>>>> That's suggestion for a 2.2 patch, right? >>>>> This *is* a 2.2 patch, isn't it? >>>> This one I proposed for 2.1 (because a device hotplug could cause qemu >>>> to abort). >>> Okay. >>> >>>>>> Do you think the usage as in this patch is fine? >>>>> It's not wrong, just inconsistent with existing usage. I'd prefer >>>>> consistency. >>>> Right. Which one do you prefer -- both using >>>> QERR_INVALID_PARAMETER_VALUE, or QERR_PROPERTY_VALUE_OUT_OF_RANGE? I >>>> prefer the latter. >>> I prefer >>> >>> qemu: -device virtio-rng-pci,period=0: Parameter 'period' >>> expects a positive number >>> >>> over >>> >>> qemu: -device virtio-rng-pci,max-bytes=-1: Property >>> virtio-rng.max_bytes doesn't take value -1 (minimum: 0, maximum: >>> 9223372036854775807) >>> >>> because frankly, "maximum: 9223372036854775807", while precise, borders >>> on gibberish. Precise gibberish, I guess :) >>> >>> Taking a step back: the property is uint64_t. Why isn't the upper bound >>> simply UINT64_MAX? >> OK - let's take this for 2.2 and get this answered. The risk isn't >> too great for the abort, since the user cannot innocently provide >> negative values. > Mekse sense. > >> John, can you look at Markus's comments and address them in a series? > Yes, please. > > Understand that my opinion on the error messages is just an opinion :) 1) /should/ the property be uint64_t? Do we really want to allow people to specify 16EiB? Logically the property should be unsigned of some sort, but the range we wish to restrict it to is probably much smaller. Is there an existing codepath that enforces signed/unsigned integers earlier, at parsing? 2) If we don't constrict the range further, then I agree: the "positive" error message is more user-friendly. It was a semantic choice on my part, but I think the less technical error is better. --------------030500030102020502010506 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit
On 07/18/2014 09:16 AM, Markus Armbruster wrote:
Amit Shah <amit.shah@redhat.com> writes:


On (Fri) 18 Jul 2014 [13:54:01], Markus Armbruster wrote:

Amit Shah <amit.shah@redhat.com> writes:


On (Fri) 18 Jul 2014 [13:15:18], Markus Armbruster wrote:

Amit Shah <amit.shah@redhat.com> writes:


On (Fri) 18 Jul 2014 [08:27:59], Markus Armbruster wrote:

John Snow <jsnow@redhat.com> writes:


If a negative integer is used for the max_bytes parameter, QEMU currently
calls abort() and leaves behind a core dump. This patch adds a simple
error message to make the reason for the termination clearer.

Signed-off-by: John Snow <jsnow@redhat.com>
---
v2: Changed 0L constant to (uint64_t)0 constant to match PRId64 format code
    on both 32bit and 64bit systems. Tested via -m32 flag.

 hw/virtio/virtio-rng.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c
index 1356aca..64c7d23 100644
--- a/hw/virtio/virtio-rng.c
+++ b/hw/virtio/virtio-rng.c
@@ -181,7 +181,11 @@ static void virtio_rng_device_realize(DeviceState *dev, Error **errp)
 
     vrng->vq = virtio_add_queue(vdev, 8, handle_input);
 
-    assert(vrng->conf.max_bytes <= INT64_MAX);
+    if (vrng->conf.max_bytes > INT64_MAX) {
+        error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE, "virtio-rng",
+                  "max_bytes", vrng->conf.max_bytes, (uint64_t)0, INT64_MAX);

Missed this initially: the property name is "max-bytes", not
"max_bytes".  Please fix.


+        return;
+    }
     vrng->quota_remaining = vrng->conf.max_bytes;
 
     vrng->rate_limit_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL,

Elsewhere in this function, we use

        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "period",
                  "a positive number");

Existing uses of QERR_PROPERTY_VALUE_OUT_OF_RANGE are all for intervals
with small bounds.

That's suggestion for a 2.2 patch, right?

This *is* a 2.2 patch, isn't it?

This one I proposed for 2.1 (because a device hotplug could cause qemu
to abort).

Okay.


Do you think the usage as in this patch is fine?

It's not wrong, just inconsistent with existing usage.  I'd prefer
consistency.

Right.  Which one do you prefer -- both using
QERR_INVALID_PARAMETER_VALUE, or QERR_PROPERTY_VALUE_OUT_OF_RANGE?  I
prefer the latter.

I prefer

    qemu: -device virtio-rng-pci,period=0: Parameter 'period'
expects a positive number

over

    qemu: -device virtio-rng-pci,max-bytes=-1: Property
virtio-rng.max_bytes doesn't take value -1 (minimum: 0, maximum:
9223372036854775807)

because frankly, "maximum: 9223372036854775807", while precise, borders
on gibberish.  Precise gibberish, I guess :)

Taking a step back: the property is uint64_t.  Why isn't the upper bound
simply UINT64_MAX?

OK - let's take this for 2.2 and get this answered.  The risk isn't
too great for the abort, since the user cannot innocently provide
negative values.

Mekse sense.


John, can you look at Markus's comments and address them in a series?

Yes, please.

Understand that my opinion on the error messages is just an opinion :)

1) should the property be uint64_t? Do we really want to allow people to specify 16EiB? Logically the property should be unsigned of some sort, but the range we wish to restrict it to is probably much smaller. Is there an existing codepath that enforces signed/unsigned integers earlier, at parsing?

2) If we don't constrict the range further, then I agree: the "positive" error message is more user-friendly. It was a semantic choice on my part, but I think the less technical error is better.
--------------030500030102020502010506--