On Thu, Sep 8, 2011 at 6:06 PM, Peter Hutterer <peter.hutte...@who-t.net> wrote:
> On Thu, Sep 08, 2011 at 03:42:40PM -0700, Jason Gerecke wrote:
>> On Wed, Sep 7, 2011 at 8:35 PM, Peter Hutterer <peter.hutte...@who-t.net>
>> wrote:
>> > On Wed, Sep 07, 2011 at 04:36:51PM -0700, Jason Gerecke wrote:
>> >> The 'set_output' function is now responsible for parsing the
>> >> command-line arguments to both determine the proper function
>> >> to call, as well as converting them into a form that can be
>> >> more easily used by the helper functions. This patch also
>> >> breaks the RandR compatibility check out into its own function
>> >> to improve readability.
>> >>
>> >> Signed-off-by: Jason Gerecke <killert...@gmail.com>
>> >> ---
>> >> Changes from v2:
>> >> * Helper functions no longer "intelligent" like earlier
>> >> patches tried to make them
>> >>
>> >> * Helper functions now take explicit arguments instead
>> >> of the remaining commandline string
>> >>
>> >> * Break-out Xinerama compatibility check into its own
>> >> function for readability
>> >>
>> >> tools/xsetwacom.c | 84
>> >> ++++++++++++++++++++++++++++++++++------------------
>> >> 1 files changed, 55 insertions(+), 29 deletions(-)
>> >>
>> >> diff --git a/tools/xsetwacom.c b/tools/xsetwacom.c
>> >> index 7bc5b54..2f2e922 100644
>> >> --- a/tools/xsetwacom.c
>> >> +++ b/tools/xsetwacom.c
>> >> @@ -1930,6 +1930,32 @@ static void get_map(Display *dpy, XDevice *dev,
>> >> param_t *param, int argc, char**
>> >> }
>> >> }
>> >>
>> >> +/**
>> >> + * Determine if we need to use fall back to Xinerama, or if the RandR
>> >> + * extension will work OK. We depend on RandR 1.3 or better in order
>> >> + * to work.
>> >> + *
>> >> + * A server bug causes the NVIDIA driver to report RandR 1.3 support
>> >> + * despite not exposing RandR CRTCs. We need to fall back to Xinerama
>> >> + * for this case as well.
>> >> + */
>> >> +static Bool need_xinerama(Display *dpy)
>> >> +{
>> >> + int opcode, event, error;
>> >> + int maj, min;
>> >> +
>> >> + if (!XQueryExtension(dpy, "RANDR", &opcode, &event, &error) ||
>> >> + !XRRQueryVersion(dpy, &maj, &min) || (maj * 1000 + min) < 1002
>> >> ||
>> >> + XQueryExtension(dpy, "NV-CONTROL", &opcode, &event, &error))
>> >> + {
>> >> + TRACE("RandR extension not found, too old, or NV-CONTROL "
>> >> + "extension is also present.\n");
>> >> + return True;
>> >> + }
>> >> +
>> >> + return False;
>> >> +}
>> >> +
>> >> static void _set_matrix_prop(Display *dpy, XDevice *dev, const float
>> >> fmatrix[9])
>> >> {
>> >> Atom matrix_prop = XInternAtom(dpy, "Coordinate Transformation
>> >> Matrix", True);
>> >> @@ -2003,17 +2029,19 @@ static void _set_matrix(Display *dpy, XDevice
>> >> *dev,
>> >> _set_matrix_prop(dpy, dev, matrix);
>> >> }
>> >>
>> >> -static void set_output_xrandr(Display *dpy, XDevice *dev, param_t
>> >> *param, int argc, char **argv)
>> >> +/**
>> >> + * Adjust the transformation matrix based on RandR settings. This
>> >> function
>> >> + * will attempt to map the given device to the output with the given
>> >> RandR
>> >> + * output name.
>> >> + */
>> >> +static void set_output_xrandr(Display *dpy, XDevice *dev, char
>> >> *output_name)
>> >> {
>> >> int i, found = 0;
>> >> int x, y, width, height;
>> >> - char *output_name;
>> >> XRRScreenResources *res;
>> >> XRROutputInfo *output_info;
>> >> XRRCrtcInfo *crtc_info;
>> >>
>> >> - output_name = argv[0];
>> >> -
>> >> res = XRRGetScreenResources(dpy, DefaultRootWindow(dpy));
>> >> for (i = 0; i < res->noutput && !found; i++)
>> >> {
>> >> @@ -2054,17 +2082,20 @@ static void set_output_xrandr(Display *dpy,
>> >> XDevice *dev, param_t *param, int ar
>> >> }
>> >>
>> >> /**
>> >> - * Adjust the transformation matrix based on the Xinerama settings. For
>> >> - * TwinView This would better be done with libXNVCtrl but until they
>> >> learn
>> >> - * to package it properly, rely on Xinerama. Besides, libXNVCtrl isn't
>> >> - * available on RHEL, so we'd have to do it through Xinerama there
>> >> anyway.
>> >> + * Adjust the transformation matrix based on the Xinerama settings. This
>> >> + * function will attempt to map the given device to the specified
>> >> Xinerama
>> >> + * head number.
>> >> + *
>> >> + * For TwinView This would better be done with libXNVCtrl but until they
>> >> + * learn to package it properly, we need to rely on Xinerama. Besides,
>> >> + * libXNVCtrl isn't available on RHEL, so we'd have to do it through
>> >> + * Xinerama there anyway.
>> >> */
>> >> -static void set_output_xinerama(Display *dpy, XDevice *dev, param_t
>> >> *param, int argc, char **argv)
>> >> +static void set_output_xinerama(Display *dpy, XDevice *dev, int head)
>> >> {
>> >> int event, error;
>> >> XineramaScreenInfo *screens;
>> >> int nscreens;
>> >> - int head;
>> >>
>> >> if (!XineramaQueryExtension(dpy, &event, &error))
>> >> {
>> >> @@ -2072,13 +2103,6 @@ static void set_output_xinerama(Display *dpy,
>> >> XDevice *dev, param_t *param, int
>> >> return;
>> >> }
>> >>
>> >> - if (!convert_value_from_user(param, argv[0], &head))
>> >> - {
>> >> - fprintf(stderr, "Please specify the output name as HEAD-X,"
>> >> - "where X is the screen number\n");
>> >> - return;
>> >> - }
>> >> -
>> >> screens = XineramaQueryScreens(dpy, &nscreens);
>> >>
>> >> if (nscreens == 0)
>> >> @@ -2087,8 +2111,8 @@ static void set_output_xinerama(Display *dpy,
>> >> XDevice *dev, param_t *param, int
>> >> goto out;
>> >> } else if (nscreens <= head)
>> >> {
>> >> - fprintf(stderr, "Found %d screens, but you requested %s.\n",
>> >> - nscreens, argv[0]);
>> >> + fprintf(stderr, "Found %d screens, but you requested number
>> >> %d.\n",
>> >> + nscreens, head);
>> >> goto out;
>> >> }
>> >>
>> >> @@ -2104,8 +2128,7 @@ out:
>> >>
>> >> static void set_output(Display *dpy, XDevice *dev, param_t *param, int
>> >> argc, char **argv)
>> >> {
>> >> - int opcode, event, error;
>> >> - int maj, min;
>> >> + int tmp_int;
>> >
>> > tmp_int suggests that this value isn't used. It is, afaict. Mabye change it
>> > to head_no, that seems to be what the value represents.
>> >
>> The value of the variables in this and the next patch only have
>> meaning if the associated else-block is entered. While you could in
>> theory use the variables again later by verifying the necessary
>> preconditions again, getting it right would be error-prone. Ideally
>> you'd *only* use the variable within the else-block where it has
>> meaning. Unfortunately I can't just declare the variables local to
>> that scope since the functions doing the checks
>> (convert_value_from_user here, and XParseGeometry in the next patch)
>> rely on the variables being present. Since I can't force the compiler
>> to complain when you start doing dangerous things, I'd like to ensure
>> the next person to touch this bit of code thinks twice before using
>> the variables.
>
> tbh, not quite sure I understand your argument here. anyone hacking on the
> code would need to make sure that the variable is valid anyway when they're
> trying to use it. having it named tmp_int doesn't change that. My argument
> is that if it is the head name, we should have it named as such. If
> convert_value_from_user fails then the variable is undefined - which is the
> case with anything that comes back from scanf or indeed most variables
> passed by reference into a function.
>
> Cheers,
> Peter
>
I understand your argument and while in the grand scheme of things I
agree with you, I was just trying to make it as obvious as possible to
J. Random Hacker that the variable will almost always be invalid.
I'll make the change for v4 as requested though.
Jason
---
Day xee-nee-svsh duu-'ushtlh-ts'it;
nuu-wee-ya' duu-xan' 'vm-nvshtlh-ts'it.
Huu-chan xuu naa~-gha.
>> >>
>> >> if (argc == 0)
>> >> {
>> >> @@ -2122,15 +2145,18 @@ static void set_output(Display *dpy, XDevice
>> >> *dev, param_t *param, int argc, cha
>> >> return;
>> >> }
>> >>
>> >> - /* Check for RandR 1.2. Server bug causes the NVIDIA driver to
>> >> - * report with RandR 1.3 support but it doesn't expose RandR CRTCs.
>> >> - * Force Xinerama if NV-CONTROL is present */
>> >> - if (XQueryExtension(dpy, "NV-CONTROL", &opcode, &event, &error) ||
>> >> - !XQueryExtension(dpy, "RANDR", &opcode, &event, &error) ||
>> >> - !XRRQueryVersion(dpy, &maj, &min) || (maj * 1000 + min) < 1002)
>> >> - set_output_xinerama(dpy, dev, param, argc, argv);
>> >> + if (!need_xinerama(dpy))
>> >> + {
>> >> + set_output_xrandr(dpy, dev, argv[0]);
>> >> + }
>> >> + else if (convert_value_from_user(param, argv[0], &tmp_int))
>> >> + {
>> >> + set_output_xinerama(dpy, dev, tmp_int);
>> >> + }
>> >> else
>> >> - set_output_xrandr(dpy, dev, param, argc, argv);
>> >> + {
>> >> + fprintf(stderr, "Unable to find an output '%s'.\n",
>> >> argv[0]);
>> >> + }
>> >
>> > we usually don't use {} for single-line blocks.
>> Things started to look cluttered to me without them, but I'll swap them in
>> v4.
>>
>> >
>> > anyway, with those two fixed: Reviewed-by: Peter Hutterer
>> > <peter.hutte...@who-t.net>
>> >
>> > Cheers,
>> > Peter
>> >
>>
>> Jason
>>
>> ---
>> Day xee-nee-svsh duu-'ushtlh-ts'it;
>> nuu-wee-ya' duu-xan' 'vm-nvshtlh-ts'it.
>> Huu-chan xuu naa~-gha.
>>
>
------------------------------------------------------------------------------
Why Cloud-Based Security and Archiving Make Sense
Osterman Research conducted this study that outlines how and why cloud
computing security and archiving is rapidly being adopted across the IT
space for its ease of implementation, lower cost, and increased
reliability. Learn more. http://www.accelacomm.com/jaw/sfnl/114/51425301/
_______________________________________________
Linuxwacom-devel mailing list
Linuxwacom-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
