[Nut-upsdev] [PATCH] al175: updated driver, please restore it
kirr at mns.spb.ru
Tue Jan 27 15:09:22 UTC 2009
On Tue, Jan 13, 2009 at 05:58:23PM +0300, Kirill Smelkov wrote:
> Arjen, Arnaud,
> first of all, I'm sorry for my late reply.
> If it's not too late, here is updated al175:
> On Fri, Dec 26, 2008 at 11:37:04PM +0100, Arjen de Korte wrote:
> > Citeren Kirill Smelkov <kirr at mns.spb.ru>:
> > [...]
> >> Yes, "All the world is not an x86 Linux box," and I've tried to make all the
> >> world happy.
> > I raised several other issues as well, of which the most important one
> > isn't addressed. The driver still uses 'alloca' which isn't portable (it
> > is not in POSIX). I also indicated how this could be solved by using
> > automatic variables. In any case I'm not conviced there is no
> > alternative here, so this is a show stopper for now.
> I've reworked it not to use alloca and to use auto-variables + xmalloc
> in one place.
> Now it usually looks like
> raw_data reply;
> byte_t reply_buf;
> raw_alloc_onstack(&reply, reply_buf);
> > Also something I really don't like in this driver is the use of alarm().
> > Although this is presently used in the 'net-xml' driver (in case an older
> > neon library is detected), there is no need for this here. We have
> > timeouts on *all* ser_get* functions and these *must* be used instead.
> > Note that with using timeouts I mean something different than setting
> > them to '999' seconds and setting an alarm() to breakout early. Unless
> > there is a really good reason to use an alarm() (ie, there is no
> > alternative), this shouldn't be used.
> I've used alarm in the first place for the following reason:
> Let's look at e.g. upsdrv_updateinfo() -- it reads several al175
> registers, and each read_register consists of a chat between host and
> And I want the whole transaction to be less than say 5 seconds. What
> should I do? manually calculate read/write time budget left for _each_
> read or write?
> I think this is not practical.
> That's why I simply
> 1. arm the timer via alarm(T) to fire after T seconds
> 2. do all the I/O, logic, etc...
> 3. if I/O would stale at *any* place, it will be *always* aborted right
> after T seconds since the transaction start (thanks to alarm)
> 4. after all the I/O transaction is done -- reset the timer.
> And exactly for this reason 999 is used as infinity which means 'much
> more longer than _any_ T we'll pass to alarm() ever'. If there is a way
> to actually pass 'please never timeout on select', I'd be happy to
> rework the code.
> Also according to mans, alarm(2) is in SVr4, POSIX.1-2001 and 4.3BSD.
> That's why I think using alarm() makes sense.
> But if there is a similiar functionality in NUT to set whole transaction
> timeout, I'll be happy to rework the code to use what NUT provides.
> > Even in the updated driver, I see quite a couple of fprintf() lines.
> > Although these are in most (all?) cases encased in a 'if
> > (nut_debug_level > 0)', we have the upsdebug* functions for that. Do
> > yourself (and your fellow developers) a favor and use those instead.
> > Another thing related to this is the XTRACE macro. This will only use a
> > single (1) debug level to show output. Please use a layered approach,
> > where various levels of debugging information are used.
> I've split debug/trace output into several levels:
> 1 /* user-level trace (status, instcmd, etc */
> 2 /* proto handling info */
> 3 /* IO trace */
> so with -D the user will see only high level stuff, with -DD --
> high-level info + any information regarding COMLI proto handling, and
> with -DDD -- everything includiong full IO trace.
> Can't test it because I don't have the hardware now, but I hope this is
> (one place still uses fprintf(stderr, ...) but that's because it is
> difficult to construct one line through several calls to upsdebug,
> although sure this is doable, like upsdebug_hex does. Mine has pretty
> printing for control ascii characters, and I'd better leave it there,
> because as I recall this was usefull for debugging. I hope this is not
> a show-stopper.)
> > It also helps understanding the driver if you keep the number of
> > macro's limited.
> The reason XTRACE was introduced in the first place is to automatically
> put a function name as prefix into trace output through the use of
> __FUNCTION__. if it is (again) a protability problem, we could always
> workaround this as follows (taken from gcc manual):
> #if __STDC_VERSION__ < 199901L
> # if __GNUC__ >= 2
> # define __func__ __FUNCTION__
> # else
> # define __func__ "<unknown>"
> # endif
> Here is the interdiff wrt recent version as posted to the list, and the
> full patch:
So what would we do about this patch?
I've tried to address the issues you've raised, and to describe my
rationale behind alarm().
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.
More information about the Nut-upsdev