Re: [PATCH 2/2] Fix one warning

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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?

in xserver:
function dr_drm_get_perms (hw/xfree86/dri/dri.c:759) copies previously
initialized values of xf86ConfigDRI.

Those values are set in configDRI (hw/xfree86/common/xf86Config.c:2166),
which is called from xf86HandleConfigFile
(hw/xfree86/common/xf86Config.c:2479)
and provided values parsed in
xf86readConfigFile(hw/xfree86/parser/read.c:89), group is only set in
dri section.

I did not really look into details. to me it looks like -1 is stored in
group unless there is a valid DRI section with group assigned name or
gid.

I did not look for other users of libdrm that might also use neg values
to indicate errors. You might want to ask someone who is more familiar
with the design than just reading the code (like I did)


regards,
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@xxxxxxxxxxxxxxxxxxxxxxx] On Behalf Of Jan Vesely
> Sent: Friday, April 24, 2015 10:30 PM
> To: Zhou, Jammy
> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx; 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@xxxxxxx>
> > ---
> >  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@xxxxxxxxxxx>

-- 
Jan Vesely <jan.vesely@xxxxxxxxxxx>

Attachment: signature.asc
Description: This is a digitally signed message part

_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux