Re: [Qemu-devel] [Qemu-trivial] [PATCH 2/2] Check value for invalid negative values
On Do, 2015-06-18 at 06:45 -0400, Frediano Ziglio wrote: > > On Do, 2015-06-18 at 05:58 -0400, Frediano Ziglio wrote: > > > For the same reason there is the v >= l test. > > > The v >= l test state that the value can be out of range so it not always > > > a > > > constant in the range. > > > Adding the v < 0 check for every invalid value. As these are executed only > > > for logging should not be a performance penalty. > > > I also hope the compiler is able to optimize > > > > > > if (v < 0 || v >= l) > > > > > > with > > > > > > if ((unsigned) v >= l) > > > > Just make v explicitly unsigned? > > > > cheers, > > Gerd > > > > Do you mean in the prototype? Yes. > Well, this could have side effect due to different conversions so is not a so > trivial patch. What side effects? I don't expect any for in-range values, and how exactly we catch out-of-range values doesn't really matter ... cheers, Gerd
Re: [Qemu-devel] [Qemu-trivial] [PATCH 2/2] Check value for invalid negative values
> On Do, 2015-06-18 at 05:58 -0400, Frediano Ziglio wrote: > > For the same reason there is the v >= l test. > > The v >= l test state that the value can be out of range so it not always a > > constant in the range. > > Adding the v < 0 check for every invalid value. As these are executed only > > for logging should not be a performance penalty. > > I also hope the compiler is able to optimize > > > > if (v < 0 || v >= l) > > > > with > > > > if ((unsigned) v >= l) > > Just make v explicitly unsigned? > > cheers, > Gerd > Do you mean in the prototype? Well, this could have side effect due to different conversions so is not a so trivial patch. Explicitly casting to unsigned would do but is IMHO less easy to read that an explicit check. Frediano
Re: [Qemu-devel] [Qemu-trivial] [PATCH 2/2] Check value for invalid negative values
On Do, 2015-06-18 at 05:58 -0400, Frediano Ziglio wrote: > For the same reason there is the v >= l test. > The v >= l test state that the value can be out of range so it not always a > constant in the range. > Adding the v < 0 check for every invalid value. As these are executed only > for logging should not be a performance penalty. > I also hope the compiler is able to optimize > > if (v < 0 || v >= l) > > with > > if ((unsigned) v >= l) Just make v explicitly unsigned? cheers, Gerd
Re: [Qemu-devel] [Qemu-trivial] [PATCH 2/2] Check value for invalid negative values
For the same reason there is the v >= l test. The v >= l test state that the value can be out of range so it not always a constant in the range. Adding the v < 0 check for every invalid value. As these are executed only for logging should not be a performance penalty. I also hope the compiler is able to optimize if (v < 0 || v >= l) with if ((unsigned) v >= l) Frediano > > 11.06.2015 16:17, Frediano Ziglio wrote: > > In qxl_v2n check that value is not negative. > > Why do you think it is necessary? > > Thanks, > > /mjt > > > Signed-off-by: Frediano Ziglio > > --- > > hw/display/qxl-logger.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/hw/display/qxl-logger.c b/hw/display/qxl-logger.c > > index d944d3f..faed869 100644 > > --- a/hw/display/qxl-logger.c > > +++ b/hw/display/qxl-logger.c > > @@ -93,7 +93,7 @@ static const char *const spice_cursor_type[] = { > > > > static const char *qxl_v2n(const char *const n[], size_t l, int v) > > { > > -if (v >= l || !n[v]) { > > +if (v < 0 || v >= l || !n[v]) { > > return "???"; > > } > > return n[v]; > > > >
Re: [Qemu-devel] [Qemu-trivial] [PATCH 2/2] Check value for invalid negative values
11.06.2015 16:17, Frediano Ziglio wrote: > In qxl_v2n check that value is not negative. Why do you think it is necessary? Thanks, /mjt > Signed-off-by: Frediano Ziglio > --- > hw/display/qxl-logger.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/display/qxl-logger.c b/hw/display/qxl-logger.c > index d944d3f..faed869 100644 > --- a/hw/display/qxl-logger.c > +++ b/hw/display/qxl-logger.c > @@ -93,7 +93,7 @@ static const char *const spice_cursor_type[] = { > > static const char *qxl_v2n(const char *const n[], size_t l, int v) > { > -if (v >= l || !n[v]) { > +if (v < 0 || v >= l || !n[v]) { > return "???"; > } > return n[v];