* [PATCH] ui: drop VNC feature _MASK constants
@ 2024-01-03 12:26 Daniel P. Berrangé
2024-01-03 13:26 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 3+ messages in thread
From: Daniel P. Berrangé @ 2024-01-03 12:26 UTC (permalink / raw)
To: qemu-devel; +Cc: Gerd Hoffmann, Marc-André Lureau, Daniel P. Berrangé
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(-)
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);
--
2.43.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] ui: drop VNC feature _MASK constants
2024-01-03 12:26 [PATCH] ui: drop VNC feature _MASK constants Daniel P. Berrangé
@ 2024-01-03 13:26 ` Philippe Mathieu-Daudé
2024-01-03 13:57 ` Daniel P. Berrangé
0 siblings, 1 reply; 3+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-01-03 13:26 UTC (permalink / raw)
To: Daniel P. Berrangé, qemu-devel; +Cc: Gerd Hoffmann, Marc-André Lureau
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);
> +}
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] ui: drop VNC feature _MASK constants
2024-01-03 13:26 ` Philippe Mathieu-Daudé
@ 2024-01-03 13:57 ` Daniel P. Berrangé
0 siblings, 0 replies; 3+ messages in thread
From: Daniel P. Berrangé @ 2024-01-03 13:57 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: qemu-devel, Gerd Hoffmann, Marc-André Lureau
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
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-01-03 13:57 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-01-03 12:26 [PATCH] ui: drop VNC feature _MASK constants Daniel P. Berrangé
2024-01-03 13:26 ` Philippe Mathieu-Daudé
2024-01-03 13:57 ` Daniel P. Berrangé
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).