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

[Victor Toso]

Hi,

On Mon, Jul 15, 2019 at 05:18:17AM -0400, Frediano Ziglio wrote:
> > 
> > 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?

I'm fine with that but as general rule I try to follow the
surrounding coding style. Would you prefer we stick with C99 for
new codebase even if surrounding code is not following that? We
discussed a few times irc/email but no rule was made, so I get a
bit confused sometimes with this.

> > +        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

Thanks, I'll wait your reply before pushing.

Cheers,
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20190715/71ec17bc/attachment.sig>