[mythtv] [mythtv-commits] Ticket #1945: DVB-S/diseqc patch
yeasah at schwide.com
Sun Jul 2 17:19:10 UTC 2006
Daniel Kristjansson wrote:
>I have not even attempted to run this code, this is just 3-4
>passes of fixing obvious mistakes with respect to MythTV coding
>standards and conformity with the rest of the code, and adding
>error checking where system libraries are likely to return errors.
>There may have also been some thread safety fixes, but I know
>the code is not really thread safe yet.
>One way for you go about this is to start with your patch and
>apply the fixes in the branch to your code one by one. Testing
>along the way. When anything breaks try to fix it or go back
>to the previous version and start applying other fixes. Repeat
>this process until you have something that looks like the code
>in SVN but doesn't have any of the problems the code in SVN has.
>Then just send me a patch with the differences. I'll apply any
>bits that are ok, and either fix other problems another way or
>punt them back to you with an explanation.
I can't easily do what you're suggesting here, because the branch has so
many changes all in one changeset, it would be more difficult to tease
apart the individually coherent changes than it is worth. I'll enumerate
the changes I know about here.
Things I agree with, or at least don't have a major problem with:
1) Changes to the names of many files, classes, types and variables -- I
certainly don't have a problem with that.
2) Changes to the method of adhering to USING_DVB -- I just copied the
method used in dvbchannel, but I thought that was extremely hokey and
I'm glad to see it changed.
3) Movement of the db upgrade code into dbcheck.cpp and the assignment
of a new db schema number -- great, I've been asking for that for as
long as there's been a patch.
4) Movement of some stuff which is technically private from the diseqc
headers to the implementation files -- sure, I agree with that. I don't
much care for interface documentation in the implementation file, but I
understand the desire to reduce the impact of changes on recompliations,
and thus keeping the headers as simple as possible.
5) Random syntax changes, no problem with that.
6) Changing from STL containers to Qt containers -- they are similar
enough that it's not a big deal, and there's plenty of STL in the
application already, so I figured it was ok. But I can understand the
desire to not use two different implementations of the same thing.
7) Adding the new GUI file to the translation qmake file -- great, I
didn't even know about that.
8) Additional db error checking everywhere, which is very good.
9) Changing the default source id on the tune methods to be zero instead
of -1 -- fine, I figured it would be better to use a value that was
entirely out of the range of the database field, but autoincrement won't
ever cough up a zero normally so I suppose it's just as good (and then
it can use a uint for the ID like it should) -- OTOH if somebody ever
makes myth multi-database something like that could be a problem.
10) Locking added to the signal monitor stuff. Great, I wasn't sure
exactly what was required in the signal monitor for thread safety.
11) In general checking if a pointer is NULL before deleting it. I
dislike that, just because it perpetuates the common misconception that
it is unsafe to delete a null pointer, and explicitly refuses to use a
feature of the language that was put there for that exact purpose. But
it's harmless, so I don't really have a problem with it.
Things that I don't really agree with -- I'm not particularly happy with
some of these changes, but I'm not about to argue them either, as it's
not my project:
12) Changing the database identifier lookup functions to use arrays of
QStrings instead of const ascii strings. That one baffles me -- it's
more complex and has more overhead with QStrings, and I don't see any
benefit there -- it's not like i18n'd or unicode db identifiers makes
any sense, and the underlying strings are still stored as const char*
entities in the library's constant data section in either case. Also,
the arrays are now initialized with an explicit size now instead of
allowing the compiler to determine the array count -- there is just no
benefit to that, and it can only cause problems.
13) Generally changing things to favor compactness. I can't say I
generally agree with all of these changes. For example, adding multiple
explicit return statements to a function can be problematic from a
maintenance perspective when done to even moderately complex functions,
as the chance that somebody will decide to add some code to that
function after an earlier conditional return that they didn't spot is
too great, and if that conditional is a rare case such a fault may not
be detected for a long time.
14) Similarly, eliminating interim temporary values is IMO generally a
loss, since the output of any half-assed optimizing compiler will be
indentical in virtually all cases (in fact, breaking subexpressions out
explicitly can help some crappy optimizers in some cases), but you lose
a piece of self-documentation in that the code no longer assigns a
meaningful name to a subexpression -- plus it just adds more work later
if/when the function becomes more complicated and needs to reference the
subexpression multiple times.
15) A comment indicating that the QTime object that is used to determine
the elapsed time since the last tuning request is broken at midnight
because of a wrap -- I don't think this is so. The QTime documentation
only states that the elapsed() method wraps after 24 hours of elapsed
time, which means it would only be a problem if 24 hours had passed
after an initial tune and then a retune was somehow required, which
sounds pretty absurd to me -- and even then, the only negative effect
would be that it would take an extra retune interval of time (since the
counter resets to zero) -- even if this can actually happen at all,
after 24 hours of watching one thing, I doubt waiting an extra retune
interval is going to be a problem.
16) Moving two of the diseqc ioctl methods that previously resided in
the main tree class into static module functions. I definitely don't
agree with that. Currently it is certainly possible to do that, since
those methods didn't currently use any state from the tree object
(actually they did, but you changed it to be passed in), but I can
easily expect that they might use more significant state in the future
as people report that their particular gear needs more tweaks, and one
of the methods had to stay anyway (so now two out of three of a
symmetric set of methods have been randomly moved to static non-class
functions) -- I can't see any good rationale here.
Things that are definitely bugs:
17) Deleting the diseqc tree object in dvbchannel. Not right, needs to
18) Changes to the device IDs to use unsigned ints. Ah, I see why some
of the code is broken now. In order to assign unique IDs to nodes that
haven't been created yet, the device tree temporarily assigns nodes with
negative number IDs as placeholders until the final store command, at
which point the nodes are inserted into the database and proper positive
number IDs are assigned for all the new nodes. By changing the IDs to be
unsigned, that totally breaks that. It's true that the original code
couldn't properly represent device IDs greater than ~2 billion, but
changing them all to be unsigned is not a solution to that. Avoiding the
negative number thing would result in a more complex solution (maybe
storing a unique id plus a bool that indicates if it's "real" or not,
and generating strings for the UI to index everything off of based on a
formatted combination of the bool and int), but it would work -- another
much more simple option would simply be to alter the table IDs to match
the range of a signed int.
19) The switch SetChild() method no longer checks the supplied ordinal
to see if it's in the range of the array, but instead tries to retrieve
a child at that ordinal and fails if there is no child object. Of course
this breaks the switch, since it means that you can't add child nodes
unless there are already child nodes present. I imagine this is the
source of the no-switch-children problem I noticed.
... That's about all I have the stamina for so far, which is just what I
can see by manual comparison about halfway through the main diseqc file,
but I hope you can see why I don't have a lot of confidence in the
changeset as a whole right now -- since some of the functional changes
are problematic in a way that indicates a failure to fully understand
the new code, coupled with the fact that it's not straightforward at all
for me to locate the actual functional changes, and it's virtually
impossible to know if I've reviewed them all. I expect there could
easily be more subtle problems that have been introduced that might not
show up in testing.
Regarding thread safety, I think the most important thing is that the
tree be locked for the lifetime of a retrieved rotor object (when the
signal monitor requests the rotor from the channel class and uses it to
update its values), which it currently isn't -- beyond that, the tree
itself could easily be locked when it is in use -- I think that's not
strictly necessary given the current usage patterns (if you have two
dvbchannel objects trying to use the same device at the same time you've
got other problems, and I assume that should never happen), but it would
be a trivial addition so it's probably a good idea whether it's strictly
needed or not -- to avoid failure cascades. As far as I remember, the
other objects are either definitively single-threaded or already locked.
More information about the mythtv-dev