diff mbox series

ui: drop VNC feature _MASK constants

Message ID 20240103122600.2399662-1-berrange@redhat.com
State New
Headers show
Series ui: drop VNC feature _MASK constants | expand

Commit Message

Daniel P. Berrangé Jan. 3, 2024, 12:26 p.m. UTC 
Each VNC feature enum entry has a corresponding _MASK constant
which is the bit-shifted value. It is very easy for contributors
to accidentally use the _MASK constant, instead of the non-_MASK
constant, or the reverse. No compiler warning is possible and
it'll just silently do the wrong thing at runtime.

By introducing the vnc_set_feature helper method, we can drop
all the _MASK constants and thus prevent any future accidents.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 ui/vnc.c | 34 +++++++++++++++++-----------------
 ui/vnc.h | 21 ++++-----------------
 2 files changed, 21 insertions(+), 34 deletions(-)

Comments

Philippe Mathieu-Daudé Jan. 3, 2024, 1:26 p.m. UTC | #1 
On 3/1/24 13:26, Daniel P. Berrangé wrote:
> Each VNC feature enum entry has a corresponding _MASK constant
> which is the bit-shifted value. It is very easy for contributors
> to accidentally use the _MASK constant, instead of the non-_MASK
> constant, or the reverse. No compiler warning is possible and
> it'll just silently do the wrong thing at runtime.
> 
> By introducing the vnc_set_feature helper method, we can drop
> all the _MASK constants and thus prevent any future accidents.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   ui/vnc.c | 34 +++++++++++++++++-----------------
>   ui/vnc.h | 21 ++++-----------------
>   2 files changed, 21 insertions(+), 34 deletions(-)


> @@ -599,6 +582,10 @@ static inline uint32_t vnc_has_feature(VncState *vs, int feature) {
>       return (vs->features & (1 << feature));
>   }
>   
> +static inline void vnc_set_feature(VncState *vs, int feature) {

Even stricter using s/int/VncFeatures/ enum type.

With that:
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> +    vs->features |= (1 << feature);
> +}
Daniel P. Berrangé Jan. 3, 2024, 1:57 p.m. UTC | #2 
On Wed, Jan 03, 2024 at 02:26:41PM +0100, Philippe Mathieu-Daudé wrote:
> On 3/1/24 13:26, Daniel P. Berrangé wrote:
> > Each VNC feature enum entry has a corresponding _MASK constant
> > which is the bit-shifted value. It is very easy for contributors
> > to accidentally use the _MASK constant, instead of the non-_MASK
> > constant, or the reverse. No compiler warning is possible and
> > it'll just silently do the wrong thing at runtime.
> > 
> > By introducing the vnc_set_feature helper method, we can drop
> > all the _MASK constants and thus prevent any future accidents.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >   ui/vnc.c | 34 +++++++++++++++++-----------------
> >   ui/vnc.h | 21 ++++-----------------
> >   2 files changed, 21 insertions(+), 34 deletions(-)
> 
> 
> > @@ -599,6 +582,10 @@ static inline uint32_t vnc_has_feature(VncState *vs, int feature) {
> >       return (vs->features & (1 << feature));
> >   }
> > +static inline void vnc_set_feature(VncState *vs, int feature) {
> 
> Even stricter using s/int/VncFeatures/ enum type.

Even with that, the compiler happily lets you pass arbitrary int values
even if they're not mapped to VncFeature enum constants, so it doesn't
make it any stricter.  None the less it is beneficial as it makes it
self-documenting, so I'll change that.

> 
> With that:
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
> > +    vs->features |= (1 << feature);
> > +}
> 

With regards,
Daniel
diff mbox series

Patch

diff --git a/ui/vnc.c b/ui/vnc.c
index 4f23a0fa79..3db87fd89c 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -2144,16 +2144,16 @@  static void set_encodings(VncState *vs, int32_t *encodings, size_t n_encodings)
             vs->vnc_encoding = enc;
             break;
         case VNC_ENCODING_HEXTILE:
-            vs->features |= VNC_FEATURE_HEXTILE_MASK;
+            vnc_set_feature(vs, VNC_FEATURE_HEXTILE);
             vs->vnc_encoding = enc;
             break;
         case VNC_ENCODING_TIGHT:
-            vs->features |= VNC_FEATURE_TIGHT_MASK;
+            vnc_set_feature(vs, VNC_FEATURE_TIGHT);
             vs->vnc_encoding = enc;
             break;
 #ifdef CONFIG_PNG
         case VNC_ENCODING_TIGHT_PNG:
-            vs->features |= VNC_FEATURE_TIGHT_PNG_MASK;
+            vnc_set_feature(vs, VNC_FEATURE_TIGHT_PNG);
             vs->vnc_encoding = enc;
             break;
 #endif
@@ -2163,57 +2163,57 @@  static void set_encodings(VncState *vs, int32_t *encodings, size_t n_encodings)
              * So prioritize ZRLE, even if the client hints that it prefers
              * ZLIB.
              */
-            if ((vs->features & VNC_FEATURE_ZRLE_MASK) == 0) {
-                vs->features |= VNC_FEATURE_ZLIB_MASK;
+            if (!vnc_has_feature(vs, VNC_FEATURE_ZRLE)) {
+                vnc_set_feature(vs, VNC_FEATURE_ZLIB);
                 vs->vnc_encoding = enc;
             }
             break;
         case VNC_ENCODING_ZRLE:
-            vs->features |= VNC_FEATURE_ZRLE_MASK;
+            vnc_set_feature(vs, VNC_FEATURE_ZRLE);
             vs->vnc_encoding = enc;
             break;
         case VNC_ENCODING_ZYWRLE:
-            vs->features |= VNC_FEATURE_ZYWRLE_MASK;
+            vnc_set_feature(vs, VNC_FEATURE_ZYWRLE);
             vs->vnc_encoding = enc;
             break;
         case VNC_ENCODING_DESKTOPRESIZE:
-            vs->features |= VNC_FEATURE_RESIZE_MASK;
+            vnc_set_feature(vs, VNC_FEATURE_RESIZE);
             break;
         case VNC_ENCODING_DESKTOP_RESIZE_EXT:
-            vs->features |= VNC_FEATURE_RESIZE_EXT_MASK;
+            vnc_set_feature(vs, VNC_FEATURE_RESIZE_EXT);
             break;
         case VNC_ENCODING_POINTER_TYPE_CHANGE:
-            vs->features |= VNC_FEATURE_POINTER_TYPE_CHANGE_MASK;
+            vnc_set_feature(vs, VNC_FEATURE_POINTER_TYPE_CHANGE);
             break;
         case VNC_ENCODING_RICH_CURSOR:
-            vs->features |= VNC_FEATURE_RICH_CURSOR_MASK;
+            vnc_set_feature(vs, VNC_FEATURE_RICH_CURSOR);
             break;
         case VNC_ENCODING_ALPHA_CURSOR:
-            vs->features |= VNC_FEATURE_ALPHA_CURSOR_MASK;
+            vnc_set_feature(vs, VNC_FEATURE_ALPHA_CURSOR);
             break;
         case VNC_ENCODING_EXT_KEY_EVENT:
             send_ext_key_event_ack(vs);
             break;
         case VNC_ENCODING_AUDIO:
             if (vs->vd->audio_state) {
-                vs->features |= VNC_FEATURE_AUDIO_MASK;
+                vnc_set_feature(vs, VNC_FEATURE_AUDIO);
                 send_ext_audio_ack(vs);
             }
             break;
         case VNC_ENCODING_WMVi:
-            vs->features |= VNC_FEATURE_WMVI_MASK;
+            vnc_set_feature(vs, VNC_FEATURE_WMVI);
             break;
         case VNC_ENCODING_LED_STATE:
-            vs->features |= VNC_FEATURE_LED_STATE_MASK;
+            vnc_set_feature(vs, VNC_FEATURE_LED_STATE);
             break;
         case VNC_ENCODING_XVP:
             if (vs->vd->power_control) {
-                vs->features |= VNC_FEATURE_XVP_MASK;
+                vnc_set_feature(vs, VNC_FEATURE_XVP);
                 send_xvp_message(vs, VNC_XVP_CODE_INIT);
             }
             break;
         case VNC_ENCODING_CLIPBOARD_EXT:
-            vs->features |= VNC_FEATURE_CLIPBOARD_EXT_MASK;
+            vnc_set_feature(vs, VNC_FEATURE_CLIPBOARD_EXT);
             vnc_server_cut_text_caps(vs);
             break;
         case VNC_ENCODING_COMPRESSLEVEL0 ... VNC_ENCODING_COMPRESSLEVEL0 + 9:
diff --git a/ui/vnc.h b/ui/vnc.h
index 96d19dce19..8b88555ff2 100644
--- a/ui/vnc.h
+++ b/ui/vnc.h
@@ -467,23 +467,6 @@  enum VncFeatures {
     VNC_FEATURE_AUDIO,
 };
 
-#define VNC_FEATURE_RESIZE_MASK              (1 << VNC_FEATURE_RESIZE)
-#define VNC_FEATURE_RESIZE_EXT_MASK          (1 << VNC_FEATURE_RESIZE_EXT)
-#define VNC_FEATURE_HEXTILE_MASK             (1 << VNC_FEATURE_HEXTILE)
-#define VNC_FEATURE_POINTER_TYPE_CHANGE_MASK (1 << VNC_FEATURE_POINTER_TYPE_CHANGE)
-#define VNC_FEATURE_WMVI_MASK                (1 << VNC_FEATURE_WMVI)
-#define VNC_FEATURE_TIGHT_MASK               (1 << VNC_FEATURE_TIGHT)
-#define VNC_FEATURE_ZLIB_MASK                (1 << VNC_FEATURE_ZLIB)
-#define VNC_FEATURE_RICH_CURSOR_MASK         (1 << VNC_FEATURE_RICH_CURSOR)
-#define VNC_FEATURE_ALPHA_CURSOR_MASK        (1 << VNC_FEATURE_ALPHA_CURSOR)
-#define VNC_FEATURE_TIGHT_PNG_MASK           (1 << VNC_FEATURE_TIGHT_PNG)
-#define VNC_FEATURE_ZRLE_MASK                (1 << VNC_FEATURE_ZRLE)
-#define VNC_FEATURE_ZYWRLE_MASK              (1 << VNC_FEATURE_ZYWRLE)
-#define VNC_FEATURE_LED_STATE_MASK           (1 << VNC_FEATURE_LED_STATE)
-#define VNC_FEATURE_XVP_MASK                 (1 << VNC_FEATURE_XVP)
-#define VNC_FEATURE_CLIPBOARD_EXT_MASK       (1 <<  VNC_FEATURE_CLIPBOARD_EXT)
-#define VNC_FEATURE_AUDIO_MASK               (1 <<  VNC_FEATURE_AUDIO)
-
 
 /* Client -> Server message IDs */
 #define VNC_MSG_CLIENT_SET_PIXEL_FORMAT           0
@@ -599,6 +582,10 @@  static inline uint32_t vnc_has_feature(VncState *vs, int feature) {
     return (vs->features & (1 << feature));
 }
 
+static inline void vnc_set_feature(VncState *vs, int feature) {
+    vs->features |= (1 << feature);
+}
+
 /* Framebuffer */
 void vnc_framebuffer_update(VncState *vs, int x, int y, int w, int h,
                             int32_t encoding);