[Qemu-devel] [PATCH 1/2] Constify some variable

[Qemu-devel] [PATCH 1/2] Constify some variable

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
---
 hw/display/qxl-logger.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/hw/display/qxl-logger.c b/hw/display/qxl-logger.c
index c900c2c..d944d3f 100644
--- a/hw/display/qxl-logger.c
+++ b/hw/display/qxl-logger.c
@@ -22,7 +22,7 @@
 #include "qemu/timer.h"
 #include "qxl.h"
 
-static const char *qxl_type[] = {
+static const char *const qxl_type[] = {
     [ QXL_CMD_NOP ]     = "nop",
     [ QXL_CMD_DRAW ]    = "draw",
     [ QXL_CMD_UPDATE ]  = "update",
@@ -31,7 +31,7 @@ static const char *qxl_type[] = {
     [ QXL_CMD_SURFACE ] = "surface",
 };
 
-static const char *qxl_draw_type[] = {
+static const char *const qxl_draw_type[] = {
     [ QXL_DRAW_NOP         ] = "nop",
     [ QXL_DRAW_FILL        ] = "fill",
     [ QXL_DRAW_OPAQUE      ] = "opaque",
@@ -48,7 +48,7 @@ static const char *qxl_draw_type[] = {
     [ QXL_DRAW_ALPHA_BLEND ] = "alpha-blend",
 };
 
-static const char *qxl_draw_effect[] = {
+static const char *const qxl_draw_effect[] = {
     [ QXL_EFFECT_BLEND            ] = "blend",
     [ QXL_EFFECT_OPAQUE           ] = "opaque",
     [ QXL_EFFECT_REVERT_ON_DUP    ] = "revert-on-dup",
@@ -59,12 +59,12 @@ static const char *qxl_draw_effect[] = {
     [ QXL_EFFECT_OPAQUE_BRUSH     ] = "opaque-brush",
 };
 
-static const char *qxl_surface_cmd[] = {
+static const char *const qxl_surface_cmd[] = {
    [ QXL_SURFACE_CMD_CREATE  ] = "create",
    [ QXL_SURFACE_CMD_DESTROY ] = "destroy",
 };
 
-static const char *spice_surface_fmt[] = {
+static const char *const spice_surface_fmt[] = {
    [ SPICE_SURFACE_FMT_INVALID  ] = "invalid",
    [ SPICE_SURFACE_FMT_1_A      ] = "alpha/1",
    [ SPICE_SURFACE_FMT_8_A      ] = "alpha/8",
@@ -74,14 +74,14 @@ static const char *spice_surface_fmt[] = {
    [ SPICE_SURFACE_FMT_32_ARGB  ] = "ARGB/32",
 };
 
-static const char *qxl_cursor_cmd[] = {
+static const char *const qxl_cursor_cmd[] = {
    [ QXL_CURSOR_SET   ] = "set",
    [ QXL_CURSOR_MOVE  ] = "move",
    [ QXL_CURSOR_HIDE  ] = "hide",
    [ QXL_CURSOR_TRAIL ] = "trail",
 };
 
-static const char *spice_cursor_type[] = {
+static const char *const spice_cursor_type[] = {
    [ SPICE_CURSOR_TYPE_ALPHA   ] = "alpha",
    [ SPICE_CURSOR_TYPE_MONO    ] = "mono",
    [ SPICE_CURSOR_TYPE_COLOR4  ] = "color4",
@@ -91,7 +91,7 @@ static const char *spice_cursor_type[] = {
    [ SPICE_CURSOR_TYPE_COLOR32 ] = "color32",
 };
 
-static const char *qxl_v2n(const char *n[], size_t l, int v)
+static const char *qxl_v2n(const char *const n[], size_t l, int v)
 {
     if (v >= l || !n[v]) {
         return "???";
-- 
2.1.0

[Qemu-devel] [PATCH 2/2] Check value for invalid negative values

[Frediano Ziglio]

In qxl_v2n check that value is not negative.

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
---
 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];
-- 
2.1.0

Re: [Qemu-devel] [Qemu-trivial] [PATCH 2/2] Check value for invalid negative values

[Michael Tokarev]

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 <fziglio@redhat.com>
> ---
>  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

[Frediano Ziglio]

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 <fziglio@redhat.com>
> > ---
> >  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

[Gerd Hoffmann]

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

[Frediano Ziglio]

 
> 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

[Gerd Hoffmann]

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] [PATCH 1/2] Constify some variable

[Michael Tokarev]

11.06.2015 16:17, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
> ---
>  hw/display/qxl-logger.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/display/qxl-logger.c b/hw/display/qxl-logger.c
> index c900c2c..d944d3f 100644
> --- a/hw/display/qxl-logger.c
> +++ b/hw/display/qxl-logger.c
> @@ -22,7 +22,7 @@
>  #include "qemu/timer.h"
>  #include "qxl.h"
>  
> -static const char *qxl_type[] = {
> +static const char *const qxl_type[] = {
...

Applied to trivial after adding subject prefix "hw/display/qxl-logger.c:".

Thanks,

/mjt