On 02/19/2016 02:59 PM, Laszlo Ersek wrote: > On 02/19/16 21:57, Laszlo Ersek wrote: >> On 02/19/16 19:13, Wei Huang wrote: >>> The condition checking on vrng->conf.period_ms appears to be wrong, >>> conflicting with the error comment following it. >>> >>> Signed-off-by: Wei Huang <w...@redhat.com> >>> --- >>> hw/virtio/virtio-rng.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c >>> index 473c044..a06427c 100644 >>> --- a/hw/virtio/virtio-rng.c >>> +++ b/hw/virtio/virtio-rng.c >>> @@ -149,7 +149,7 @@ static void virtio_rng_device_realize(DeviceState *dev, >>> Error **errp) >>> VirtIORNG *vrng = VIRTIO_RNG(dev); >>> Error *local_err = NULL; >>> >>> - if (!vrng->conf.period_ms > 0) { >>> + if (!(vrng->conf.period_ms > 0)) { >>> error_setg(errp, "'period' parameter expects a positive integer"); >>> return; >>> } >>> >> >> The current condition is absolutely weird, but I think it happens to >> work correctly: >> >> Period_ms has type uint32_t. If it is positive, then !period_ms is zero. >> 0>0 is false, hence the error message is not printed. >> >> If period_ms is zero, then !period_ms is 1. 1>0 is true, hence the error >> message is printed. >> >> I would rewrite the check as >> >> if (vrng->conf.period_ms == 0) { >> error_setg(...) > > ... actually, what the heck are you looking at? :) This has been fixed > up in: >
Oops. It looks like I was looking at an older HEAD. :-( Please ignore it. Sorry for the noise and thanks for pointing it out. -Wei > commit a3a292c420d2fec3c07a7ca56fbb064cd57a298a > Author: Amit Shah <amit.s...@redhat.com> > Date: Thu Dec 11 13:17:42 2014 +0530 > > virtio-rng: fix check for period_ms validity > > Thanks > Laszlo >