On Thu, 2018-06-28 at 12:39 -0700, Keith Packard wrote: > Lyude Paul <ly...@redhat.com> writes: > > > Looks good! One nitpick I'm not 100% sure about: > > > +#define CHECK_FOR_REQUIRED_ARGUMENT() \ > > > + if (((i + 1) >= argc) || (!argv[i + 1])) { > > > \ > > > + ErrorF("Required argument to %s not specified\n", argv[i]); > > > \ > > > + UseMsg(); > > > \ > > > + FatalError("Required argument to %s not specified\n", argv[i]); > > > > Is the double printing of "Required argument to %s not specified" here > > intentional? > > I copied CHECK_FOR_REQUIRED_ARGUMENT from xf86Init.c where it does the > same thing. I assume this is intended to make sure the user understands > what error caused the server to exit -- you can see it both before and > after the long usage message.
Makes sense! Reviewed-by: Lyude Paul <ly...@redhat.com> > > > > + if (!xf86PrivsElevated()) > > > + ErrorF("-masterfd <fd> use the specified fd as the DRM > > > master fd\n"); > > > > I think it would be a better idea for us to show this argument description > > unconditionally, along with adding a note about setuid/setgid not being > > allowed with it > > Sounds good. > > Here's an updated patch with the usage message change suggested. Thanks > for reviewing! > > _______________________________________________ > xorg-devel@lists.x.org: X.Org development > Archives: http://lists.x.org/archives/xorg-devel > Info: https://lists.x.org/mailman/listinfo/xorg-devel -- Cheers, Lyude Paul _______________________________________________ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel