| Message ID | 1455905587-4347-1-git-send-email-wei@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <wei@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(...) Thanks Laszlo
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 <wei@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: commit a3a292c420d2fec3c07a7ca56fbb064cd57a298a Author: Amit Shah <amit.shah@redhat.com> Date: Thu Dec 11 13:17:42 2014 +0530 virtio-rng: fix check for period_ms validity Thanks Laszlo
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 <wei@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.shah@redhat.com> > Date: Thu Dec 11 13:17:42 2014 +0530 > > virtio-rng: fix check for period_ms validity > > Thanks > Laszlo >
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 condition checking on vrng->conf.period_ms appears to be wrong, conflicting with the error comment following it. Signed-off-by: Wei Huang <wei@redhat.com> --- hw/virtio/virtio-rng.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)