[Spice-devel] [linux/vd-agent v1 6/7] x11-randr: simplest fix for address-of-packed-member

[Frediano Ziglio]

> 
> From: Victor Toso <me at victortoso.com>
> 
> The struct type for width/height is uint32_t while we are trying to
> access and change it with int* - code can be improved a bit in following
> patches but this one fixes the warning by copying the value from the
> struct and copying back new value afterwards.
> 
> Also:
> - Moved variables to internal scope;
> - Added braces to inner if;
> 
>  > src/vdagent/x11-randr.c: In function ‘zero_base_monitors’:
>  >     src/vdagent/x11-randr.c:621:28: error: taking address of packed member
>  >     of
>  >     ‘struct VDAgentMonConfig’ may result in an unaligned pointer value
>  > [-Werror=address-of-packed-member]
>  >   621 |         mon_width = (int *)&mon_config->monitors[i].width;
>  >       |                            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>  > src/vdagent/x11-randr.c:622:29: error: taking address of packed member of
>  >     ‘struct VDAgentMonConfig’ may result in an unaligned pointer value
>  >     [-Werror=address-of-packed-member]
>  >   622 |         mon_height = (int *)&mon_config->monitors[i].height;
>  >       |                             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Signed-off-by: Victor Toso <victortoso at redhat.com>
> ---
>  src/vdagent/x11-randr.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/src/vdagent/x11-randr.c b/src/vdagent/x11-randr.c
> index 924f5ec..7585031 100644
> --- a/src/vdagent/x11-randr.c
> +++ b/src/vdagent/x11-randr.c
> @@ -611,20 +611,24 @@ static void zero_base_monitors(struct vdagent_x11 *x11,
>                                 int *width, int *height)
>  {
>      int i, min_x = INT_MAX, min_y = INT_MAX, max_x = INT_MIN, max_y =
>      INT_MIN;
> -    int *mon_height, *mon_width;
>  
>      for (i = 0; i < mon_config->num_of_monitors; i++) {
> -        if (!monitor_enabled(&mon_config->monitors[i]))
> +        int mon_height, mon_width;
> +
> +        if (!monitor_enabled(&mon_config->monitors[i])) {
>              continue;
> +        }
>          mon_config->monitors[i].x &= ~7;
>          mon_config->monitors[i].width &= ~7;
> -        mon_width = (int *)&mon_config->monitors[i].width;
> -        mon_height = (int *)&mon_config->monitors[i].height;
> -        constrain_to_screen(x11, mon_width, mon_height);
> +        mon_width = mon_config->monitors[i].width;
> +        mon_height = mon_config->monitors[i].height;

Why not following C99 and define and initialize in the same
line?

> +        constrain_to_screen(x11, &mon_width, &mon_height);
>          min_x = MIN(mon_config->monitors[i].x, min_x);
>          min_y = MIN(mon_config->monitors[i].y, min_y);
> -        max_x = MAX(mon_config->monitors[i].x + *mon_width, max_x);
> -        max_y = MAX(mon_config->monitors[i].y + *mon_height, max_y);
> +        max_x = MAX(mon_config->monitors[i].x + mon_width, max_x);
> +        max_y = MAX(mon_config->monitors[i].y + mon_height, max_y);
> +        mon_config->monitors[i].width = mon_width;
> +        mon_config->monitors[i].height = mon_height;
>      }
>      if (min_x != 0 || min_y != 0) {
>          syslog(LOG_ERR, "%s: agent config %d,%d rooted, adjusting to 0,0.",

Otherwise patch looks good, I think the code generated could also
be better than before.

Frediano