[Bug 196678] x11-servers/xorg-server: make config/devd recognize /dev/input/eventX from multimedia/webcamd

[bugzilla-noreply at freebsd.org]

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=196678

--- Comment #56 from Matthew Rezny <rezny at freebsd.org> ---
(In reply to rozhuk.im from comment #55)

I know there are multiple authors of this revision, I did not look through the
different versions to attribute changes to anyone, and I certainly do not mean
to criticize anyone, rather I criticized the state of the resulting code which
was a mix of styles and had some errors that could be due to multiple authors
and/or following bad examples elsewhere. So, I passed through it and cleaned it
in such a way to try to minimize errors. I certainly must thank everyone who
has contributed to the devd config code and I hope everyone has added their
name to the list of copyrights.

The double loop with a re-used index variable is in get_dev_type_by_path. The
outer loop iterates i with an early continue until the correct device is found
in the table. Once it finds the right entry, two assignments are made, one of
which, path_offset, depends on the current value of i as its an index into
hw_type_path. Then, the inner loop re-uses i to compute the value to assign to
dev_name_size. Since we eventually return from within the outer loop, it
doesn't matter to the overall flow that the value of i has changed to an
unrelated meaning, but there are two more assignments before that return that
intend to use i as an index into hw_type_path[]. This variable re-use is only
safe if flags and xdriver are assigned before the inner loop, at the same time
path_offset was assigned. Perhaps the variable re-use once was safe and it
evolved over time such that it no longer is. A safer approach is to use a new
variable for the inner loop to keep the meaning clear so that future devd
hackers won't get tripped up on the ordering there.

Comparing input_option_new() to realloc() is a good analogy. The list building
doesn't realloc, but it does a calloc() every time. If that succeeds, the new
list entry is created with the old pointer stored in it. If that fails, it
returns NULL. Thus, safely building a list requires use of two pointers, one to
hold the list we are building, and one to hold the return value while we check
it before assigning to the list pointer. Without the use of a second pointer,
the goto circus to call input_option_free_list() was comical. There is of
course a much cleaner way to structure the code using two pointers, no gotos,
and far fewer lines of code. At least there was an attempt to do the right
thing here; the hald and udev config code on the other hand never checks the
return value of a call to input_option_new(), so they might call
NewInputDeviceRequest() with either NULL, if they lost the whole list, or a
partial list, if they lost the initial list after a large calloc failed but
then got a new list when a smaller calloc succeeded in a later call to
input_option_new(). The upstream code isn't exactly a good example in many
ways.

-- 
You are receiving this mail because:
You are the assignee for the bug.