[Nut-upsdev] [PATCH] al175: updated driver, please restore it
Arjen de Korte
nut+devel at de-korte.org
Tue Jan 27 18:38:32 UTC 2009
Citeren Kirill Smelkov <kirr at mns.spb.ru>:
> So what would we do about this patch?
It fails to address the concerns raised, so we have no other option
than to reject it.
> I've tried to address the issues you've raised, and to describe my
> rationale behind alarm().
The use of alarm() here is inappropriate (and possibly incorrect as
well, see below). As stated before, none of the other drivers require
the use of alarm(), you don't have demonstrated a reason why the AL175
does so it shouldn't use it either. The alarm() function has many side
effects on other functions, most particularly with timer functions on
some systems. Therefor, the use of this should be kept to a bare
minimum in drivers. We intend to use the timer functions in the
future, to prevent drivers from stalling the communication with the
server and timing out. This is a frequent problem and we want to solve
this in a generic way for all drivers and not on a driver-by-driver
basis (like you did in the AL175 driver now).
Your misconception is that you need to calculate the time spent during
the communication. This is not the case. The ser_get_*() functions
will return as soon they have read the number of bytes requested or
the end character was found, no matter how much time is remaining. The
timeout is only there to make sure that if characters are lost or the
UPS is disconnected, the driver can break from (usually)
upsdrv_updateinfo() and retry the next time it goes there. If you find
that during normal communication with the UPS you're already getting
close to the magic 5 seconds, using alarm() to bail out is incorrect.
You might never reach the last entries in the list of queries to the
UPS you would like to do. The right solution would be to break up the
queries to the UPS in smaller pieces and perform only a limited set
each time upsdrv_updateinfo() is called (see documentation). You could
check for the OL/OB status on each poll and round robin select a
couple of queries for other variables.
> If it can't go in -- that's ok with me, but could you please at least
> tell me what you objections (if any) are now.
My main objection is still the alarm() function (which I have told you
a couple of times already), although I also don't like what you did to
replace the alloca() functions. Dynamic memory allocation should be
avoided if possible (due to possible leaking/fragmentation of memory)
as it is hard to diagnose in a backgrounded driver. I absolutely fail
to understand why you want to use structures to deal with data going
in and out of the UPS. There is absolutely no reason/need to attempt
to read all data in one large chunk. It's really beyond me why you
don't just read the (fixed length) data header, determine how many
data characters will follow based on information in this header, read
exactly that number of characters and then read the closing characters:
parse header data ('n' bytes)
ser_get_buf(<read 'n' bytes>)
ser_get_buf(<checksum and closing characters>)
The reason for being so picky on the above issues, is that we already
have far too many drivers in the tree that are impossible to
understand after the original author has lost interest in maintaining
Also, use library calls as much as possible. Don't add new functions
with almost the same functionality. Pretty printing ASCII control
characters in addition to the existing upsdebug_hex may be useful for
someone very familiar with the protocol used, but usually it is much
more informative if you decode the meaning of a message. So instead of
pretty printing the header, break up the elements and list them. The
upsdebug_hex function should be used only to display raw data for
diagnosing protocol errors. You've received something, but are unable
to parse it. In that case, the raw data may help diagnosing the problem.
When looking at the sources, it should be immediately clear in which
debug level messages will be printed (in numbers, we don't want to
have to lookup the value of a macro first, to see that it is printed
in debug level 3). Therefor, although macro's may reduce the amount of
typing involved, they are generally a bad idea to make code more
readable. A special problematic case here is macro's that expand into
multiple lines. This makes debugging harder, since it is no longer
possible to pinpoint a problem to a specific line in the source code.
Don't do this, use a function call instead.
Last but not least, it would be helpful if you first discuss possible
changes/solutions/countering, before actually doing them. Now you've
spent considerable effort in making changes, which I feel are a waste
of your effort and I really feel bad about having to reject them now,
after spending all this effor. I don't want you to waste considerable
time on changing things, if I already know that we'll be forced to
reject them when done because they don't fix the concerns there are.
Best regards, Arjen
Please keep list traffic on the list
More information about the Nut-upsdev