[Spice-devel] [PATCH spice-server 1/2] Change ENABLE_EXTRA_CHECKS statements to #ifdef

[Eduardo Lima (Etrunko)]

On 15/03/18 08:19, Uri Lublin wrote:
> On 03/13/2018 03:37 PM, Eduardo Lima (Etrunko) wrote:
>> On 13/03/18 04:21, Frediano Ziglio wrote:
>>>>
>>>> This patch makes it clear that this is a configure switch and not a
>>>> variable defined somewhere else in the code.
>>>>
>>>
>>> The code is intended that way to make the compiler always parse
>>> these parts. Note that that define is always defined so your code
>>> is not doing what you are intending.
>>
>> I have sent this patch by mistake, but anyway, the fact of it always
>> being defined is true with autotools, but it is not with meson.
> 
> So with meson it's either 1 or undefined ?

Well, sort of, this can worked around of course, to have it defined to
either 1 or 0.

> 
> Note that in this case the current code would fail to build
> when ENABLE_EXTRA_CHECKS is undefined.
> 

That's exactly what happened, I got a build failure and then I started
to think of a better solution, which I proposed as a global static variable.

>> Do you think it would make sense to have this patch or is it better to
>> keep as is? If the latter, I think it would be better to keep a static
>> variable and change its value according to the define.
> 
> This patch makes sense to me.
> 
> Perhaps replace the #ifdef with
> #if defined(ENABLE_EXTRA_CHECKS) && ENABLE_EXTRA_CHECKS
> 
> Uri.
> 
>>
>> Regards, Eduardo.
>>
>>>
>>> Frediano
>>>
>>>> Signed-off-by: Eduardo Lima (Etrunko) <etrunko at redhat.com>
>>>> ---
>>>>   server/display-channel.c |  4 +++-
>>>>   server/reds.c            |  6 +++---
>>>>   server/stream-device.c   | 22 +++++++++++-----------
>>>>   3 files changed, 17 insertions(+), 15 deletions(-)
>>>>
>>>> diff --git a/server/display-channel.c b/server/display-channel.c
>>>> index 6dc10ee7..ae2af71d 100644
>>>> --- a/server/display-channel.c
>>>> +++ b/server/display-channel.c
>>>> @@ -89,7 +89,8 @@ display_channel_finalize(GObject *object)
>>>>       display_channel_destroy_surfaces(self);
>>>>       image_cache_reset(&self->priv->image_cache);
>>>>   -    if (ENABLE_EXTRA_CHECKS) {
>>>> +#ifdef ENABLE_EXTRA_CHECKS
>>>> +    {
>>>>           unsigned int count;
>>>>           _Drawable *drawable;
>>>>           VideoStream *stream;
>>>> @@ -111,6 +112,7 @@ display_channel_finalize(GObject *object)
>>>>              
>>>> spice_assert(self->priv->surfaces[count].context.canvas ==
>>>>               NULL);
>>>>           }
>>>>       }
>>>> +#endif
>>>>         monitors_config_unref(self->priv->monitors_config);
>>>>       g_array_unref(self->priv->video_codecs);
>>>> diff --git a/server/reds.c b/server/reds.c
>>>> index 73c9ec20..ca6fd44a 100644
>>>> --- a/server/reds.c
>>>> +++ b/server/reds.c
>>>> @@ -4434,9 +4434,9 @@ red_char_device_vdi_port_finalize(GObject
>>>> *object)
>>>>           dev->priv->current_read_buf = NULL;
>>>>       }
>>>>       g_free(dev->priv->mig_data);
>>>> -    if (ENABLE_EXTRA_CHECKS) {
>>>> -        spice_assert(dev->priv->num_read_buf == 0);
>>>> -    }
>>>> +#ifdef ENABLE_EXTRA_CHECKS
>>>> +    spice_assert(dev->priv->num_read_buf == 0);
>>>> +#endif
>>>>        
>>>> G_OBJECT_CLASS(red_char_device_vdi_port_parent_class)->finalize(object);
>>>>
>>>>   }
>>>> diff --git a/server/stream-device.c b/server/stream-device.c
>>>> index 6cf29d37..15048b82 100644
>>>> --- a/server/stream-device.c
>>>> +++ b/server/stream-device.c
>>>> @@ -197,9 +197,9 @@ handle_msg_invalid(StreamDevice *dev,
>>>> SpiceCharDeviceInstance *sin, const char *
>>>>   {
>>>>       static const char default_error_msg[] = "Protocol error";
>>>>   -    if (ENABLE_EXTRA_CHECKS) {
>>>> -        spice_assert(dev->hdr_pos >= sizeof(StreamDevHeader));
>>>> -    }
>>>> +#ifdef ENABLE_EXTRA_CHECKS
>>>> +    spice_assert(dev->hdr_pos >= sizeof(StreamDevHeader));
>>>> +#endif
>>>>         if (!error_msg) {
>>>>           error_msg = default_error_msg;
>>>> @@ -234,10 +234,10 @@ handle_msg_format(StreamDevice *dev,
>>>> SpiceCharDeviceInstance *sin)
>>>>   {
>>>>       SpiceCharDeviceInterface *sif =
>>>> spice_char_device_get_interface(sin);
>>>>   -    if (ENABLE_EXTRA_CHECKS) {
>>>> -        spice_assert(dev->hdr_pos >= sizeof(StreamDevHeader));
>>>> -        spice_assert(dev->hdr.type == STREAM_TYPE_FORMAT);
>>>> -    }
>>>> +#ifdef ENABLE_EXTRA_CHECKS
>>>> +    spice_assert(dev->hdr_pos >= sizeof(StreamDevHeader));
>>>> +    spice_assert(dev->hdr.type == STREAM_TYPE_FORMAT);
>>>> +#endif
>>>>         int n = sif->read(sin, dev->msg->buf + dev->msg_pos,
>>>>       sizeof(StreamMsgFormat) - dev->msg_pos);
>>>>       if (n < 0) {
>>>> @@ -262,10 +262,10 @@ handle_msg_data(StreamDevice *dev,
>>>> SpiceCharDeviceInstance *sin)
>>>>       SpiceCharDeviceInterface *sif =
>>>> spice_char_device_get_interface(sin);
>>>>       int n;
>>>>   -    if (ENABLE_EXTRA_CHECKS) {
>>>> -        spice_assert(dev->hdr_pos >= sizeof(StreamDevHeader));
>>>> -        spice_assert(dev->hdr.type == STREAM_TYPE_DATA);
>>>> -    }
>>>> +#ifdef ENABLE_EXTRA_CHECKS
>>>> +    spice_assert(dev->hdr_pos >= sizeof(StreamDevHeader));
>>>> +    spice_assert(dev->hdr.type == STREAM_TYPE_DATA);
>>>> +#endif
>>>>         while (1) {
>>>>           uint8_t buf[16 * 1024];
>>
>>
> 


-- 
Eduardo de Barros Lima (Etrunko)
Software Engineer - RedHat
etrunko at redhat.com