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

Submitted by Victor Toso on July 12, 2019, 9:12 a.m.

Details

Message ID 20190712091242.13214-7-victortoso@redhat.com
State Accepted
Commit 7976dc31af511315fa7b83cfbb1e3bf4b613f84b
Headers show
Series "minor fixes" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Victor Toso July 12, 2019, 9:12 a.m.
From: Victor Toso <me@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@redhat.com>
---
 src/vdagent/x11-randr.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

Patch hide | download patch | download mbox

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;
+        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.",

Comments

> 
> From: Victor Toso <me@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@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

> 
> Hi,
> 
> On Mon, Jul 15, 2019 at 05:18:17AM -0400, Frediano Ziglio wrote:
> > > 
> > > From: Victor Toso <me@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@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