[PATCH 2/2] Fix one warning

[Jan Vesely]

On Mon, 2015-04-27 at 01:40 +0000, Zhou, Jammy wrote:
> Thanks for sharing the background. For [0], you mentioned that
> get_perms may return -1 in some cases for the group, can you help
> indicate which case it is?

The one i found is in xserver:
dri_drm_get_perms (hw/xfree86/dri/dri.c:759) copies values from
xf86ConfigDRI.

xf86configDRI is initialized in
configDRI(hw/xfree86/common/xf86Config.c:2166).
However, the default value if the DRI section is not present or does not
contain group setting is -1.

it looks like it relies on libdrm to fall back to default in that case,
and it looks like that path currently broken

I don't claim to fully understand what that old code is doing/supposed
to do, but scanning through it suggests that negative values are legal
way to report errors/undefined values.

there might be other users as well

jan

> 
> Since the drmSetServerInfo is seldom used, maybe we can just do the
> 'int' cast at this moment. I will send v2 for this.
> 
> Regards,
> Jammy
> 
> -----Original Message-----
> From: Jan Vesely [mailto:jv356 at scarletmail.rutgers.edu] On Behalf Of Jan Vesely
> Sent: Friday, April 24, 2015 10:30 PM
> To: Zhou, Jammy
> Cc: dri-devel at lists.freedesktop.org; Min, Frank
> Subject: Re: [PATCH 2/2] Fix one warning
> 
> On Fri, 2015-04-24 at 11:44 +0800, Jammy Zhou wrote:
> > xf86drm.c:356:2: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]
> >   group = (serv_group >= 0) ? serv_group : DRM_DEV_GID;
> >   ^
> > 
> > Signed-off-by: Jammy Zhou <Jammy.Zhou at amd.com>
> > ---
> >  xf86drm.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/xf86drm.c b/xf86drm.c
> > index 4d67861..fbda2aa 100644
> > --- a/xf86drm.c
> > +++ b/xf86drm.c
> > @@ -353,7 +353,7 @@ static int drmOpenDevice(dev_t dev, int minor, int type)
> >      }
> >  
> >      if (drm_server_info) {
> > -	group = (serv_group >= 0) ? serv_group : DRM_DEV_GID;
> > +	group = serv_group;
> 
> I don't think this change is correct. get_perms can return errors as negative values. I found that xserver does, see [0] for my take on fixing this, as well as Emil's response [1].
> 
> I think changing the condition to:
> ((int)serv_group >= 0)
> 
> should be ok(I don't think there are systems with GID_MAX greater than 2^31-1), but I never bothered sending v2.
> 
> jan
> 
> 
> [0]http://lists.freedesktop.org/archives/dri-devel/2015-February/077276.html
> [1]http://lists.freedesktop.org/archives/dri-devel/2015-February/078171.html
> 
> 
> 
> >  	chown_check_return(buf, user, group);
> >  	chmod(buf, devmode);
> >      }
> 
> 
> --
> Jan Vesely <jan.vesely at rutgers.edu>

-- 
Jan Vesely <jan.vesely at rutgers.edu>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20150428/7fae4206/attachment.sig>