Hi Kirill,<br><br><div class="gmail_quote">2009/1/27 Kirill Smelkov <span dir="ltr"><<a href="mailto:kirr@mns.spb.ru">kirr@mns.spb.ru</a>></span><br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div><div></div><div class="Wj3C7c">On Tue, Jan 13, 2009 at 05:58:23PM +0300, Kirill Smelkov wrote:<br>
> Arjen, Arnaud,<br>
> first of all, I'm sorry for my late reply.<br>
><br>
> If it's not too late, here is updated al175:<br>
><br>
><br>
> On Fri, Dec 26, 2008 at 11:37:04PM +0100, Arjen de Korte wrote:<br>
> > Citeren Kirill Smelkov <<a href="mailto:kirr@mns.spb.ru">kirr@mns.spb.ru</a>>:<br>
> ><br>
> > [...]<br>
> ><br>
> >> Yes, "All the world is not an x86 Linux box," and I've tried to make all the<br>
> >> world happy.<br>
> ><br>
> > I raised several other issues as well, of which the most important one<br>
> > isn't addressed. The driver still uses 'alloca' which isn't portable (it<br>
> > is not in POSIX). I also indicated how this could be solved by using<br>
> > automatic variables. In any case I'm not conviced there is no<br>
> > alternative here, so this is a show stopper for now.<br>
><br>
> I've reworked it not to use alloca and to use auto-variables + xmalloc<br>
> in one place.<br>
><br>
> Now it usually looks like<br>
><br>
> raw_data reply;<br>
> byte_t reply_buf[11];<br>
><br>
> ...<br>
><br>
> raw_alloc_onstack(&reply, reply_buf);<br>
><br>
> > Also something I really don't like in this driver is the use of alarm().<br>
> > Although this is presently used in the 'net-xml' driver (in case an older<br>
> > neon library is detected), there is no need for this here. We have<br>
> > timeouts on *all* ser_get* functions and these *must* be used instead.<br>
> > Note that with using timeouts I mean something different than setting<br>
> > them to '999' seconds and setting an alarm() to breakout early. Unless<br>
> > there is a really good reason to use an alarm() (ie, there is no<br>
> > alternative), this shouldn't be used.<br>
><br>
> I've used alarm in the first place for the following reason:<br>
><br>
> Let's look at e.g. upsdrv_updateinfo() -- it reads several al175<br>
> registers, and each read_register consists of a chat between host and<br>
> al175.<br>
><br>
> And I want the whole transaction to be less than say 5 seconds. What<br>
> should I do? manually calculate read/write time budget left for _each_<br>
> read or write?<br>
><br>
> I think this is not practical.<br>
><br>
> That's why I simply<br>
><br>
> 1. arm the timer via alarm(T) to fire after T seconds<br>
> 2. do all the I/O, logic, etc...<br>
> 3. if I/O would stale at *any* place, it will be *always* aborted right<br>
> after T seconds since the transaction start (thanks to alarm)<br>
> 4. after all the I/O transaction is done -- reset the timer.<br>
><br>
> And exactly for this reason 999 is used as infinity which means 'much<br>
> more longer than _any_ T we'll pass to alarm() ever'. If there is a way<br>
> to actually pass 'please never timeout on select', I'd be happy to<br>
> rework the code.<br>
><br>
> Also according to mans, alarm(2) is in SVr4, POSIX.1-2001 and 4.3BSD.<br>
> That's why I think using alarm() makes sense.<br>
><br>
> But if there is a similiar functionality in NUT to set whole transaction<br>
> timeout, I'll be happy to rework the code to use what NUT provides.<br>
><br>
> > Even in the updated driver, I see quite a couple of fprintf() lines.<br>
> > Although these are in most (all?) cases encased in a 'if<br>
> > (nut_debug_level > 0)', we have the upsdebug* functions for that. Do<br>
> > yourself (and your fellow developers) a favor and use those instead.<br>
><br>
> > Another thing related to this is the XTRACE macro. This will only use a<br>
> > single (1) debug level to show output. Please use a layered approach,<br>
> > where various levels of debugging information are used.<br>
><br>
> I've split debug/trace output into several levels:<br>
><br>
> 1 /* user-level trace (status, instcmd, etc */<br>
> 2 /* proto handling info */<br>
> 3 /* IO trace */<br>
><br>
><br>
> so with -D the user will see only high level stuff, with -DD --<br>
> high-level info + any information regarding COMLI proto handling, and<br>
> with -DDD -- everything includiong full IO trace.<br>
><br>
> Can't test it because I don't have the hardware now, but I hope this is<br>
> ok.<br>
><br>
> (one place still uses fprintf(stderr, ...) but that's because it is<br>
> difficult to construct one line through several calls to upsdebug,<br>
> although sure this is doable, like upsdebug_hex does. Mine has pretty<br>
> printing for control ascii characters, and I'd better leave it there,<br>
> because as I recall this was usefull for debugging. I hope this is not<br>
> a show-stopper.)<br>
><br>
><br>
> > It also helps understanding the driver if you keep the number of<br>
> > macro's limited.<br>
><br>
> The reason XTRACE was introduced in the first place is to automatically<br>
> put a function name as prefix into trace output through the use of<br>
> __FUNCTION__. if it is (again) a protability problem, we could always<br>
> workaround this as follows (taken from gcc manual):<br>
><br>
><br>
> #if __STDC_VERSION__ < 199901L<br>
> # if __GNUC__ >= 2<br>
> # define __func__ __FUNCTION__<br>
> # else<br>
> # define __func__ "<unknown>"<br>
> # endif<br>
> #endif<br>
><br>
><br>
><br>
> Here is the interdiff wrt recent version as posted to the list, and the<br>
> full patch:<br>
<br>
</div></div>[...]<br>
<br>
Arnaud, Arjen,<br>
<br>
So what would we do about this patch?<br>
<br>
I've tried to address the issues you've raised, and to describe my<br>
rationale behind alarm().<br>
<br>
If it can't go in -- that's ok with me, but could you please at least<br>
tell me what you objections (if any) are now.<br>
</blockquote></div><br>sorry, both Arjen and I were caught by real life!<br><br>we've had a discussion not long ago about your driver.<br>the conclusion is that there are still show stoppers for including it back in the tree.<br>
so it's a no-go for 2.4.0!<br>Remember that our aim is not to bother drivers developers, but to
ensure that their work will be maintainable and maintained even if they
disappear...<br>
<br>Arjen should get back to you soon and point/help you in doing what is needed.<br>Though he already stated most (if not all) the points in the previous mails.<br>In the meantime, 2.2.2 will still be available, and you can always use a patched 2.4.0 including al175<br>
<br>so, keep your motivation, we'll end up in succeeding ;-)<br><br>cheers,<br>Arnaud<br>-- <br>Linux / Unix Expert R&D - Eaton - <a href="http://www.eaton.com/mgeops">http://www.eaton.com/mgeops</a><br>Network UPS Tools (NUT) Project Leader - <a href="http://www.networkupstools.org/">http://www.networkupstools.org/</a><br>
Debian Developer - <a href="http://people.debian.org/~aquette/">http://people.debian.org/~aquette/</a><br>Free Software Developer - <a href="http://arnaud.quette.free.fr/">http://arnaud.quette.free.fr/</a><br><br>