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

[Frediano Ziglio]

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

I think it's quite hard (and IMO wrong) to define a generic rule.
In C89 was easy... it was a compiler error!
But many compilers started to add exceptions accepting more "C++"
styles (was similar to the "//" comment).
Reasons are different:
- compilers don't get fool with that syntax;
- you can avoid uninitialized data;
- changes (so git diff) are more localized;
- can be short (don't need 2 lines for declaration and
  initialisation).
There are situation where the "old" style could be better:
- goto-s (in this case can be mandatory, you cannot skip
  variable declaration);
- variable reused (like the classic "int i").
So defining a single rule looks wrong to me.

In this specific case I found it more readable, you avoid one line
and if you are just reading quickly the code you don't have to switch
to some variable which are not used to some checks and back to these
variable usage.

> > > +        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.
> > 
> 
> Thanks, I'll wait your reply before pushing.
> 
> Cheers,
> 

Code is fixing an issue (even if it's minor) so I would say ack either way

Frediano