<html>
  <head>

    <meta http-equiv="content-type" content="text/html; charset=ISO-8859-1">
  </head>
  <body bgcolor="#FFFFFF" text="#000000">
    <font size="-1"><font face="monospace">Hello gents,<br>
        <br>
        while getting rid of gettimeofday, I've spotted something I'd
        like<br>
        to discuss in implementation of socket polling in<br>
        driver/main.c::main and driver/dstate.c::dstate_poll_fds<br>
        <br>
        My observations follow:<br>
        <br>
        1/ I'm not very happy with the poll mechanism design as whole.<br>
        I mean, typically, you implement the poll as { poll();
        process(); }*<br>
        loop; in pseudo-code:<br>
        <br>
        shut_down = false;<br>
        <br>
        fd_set.init(...);<br>
        timeout.set(...);<br>
        <br>
        while (!shut_down) {<br>
            status = poll(fd_set, timeout);<br>
        <br>
            switch (status) {<br>
                case POLL_ERROR:<br>
                    // Handle error ...<br>
        <br>
                    break;<br>
        <br>
                case POLL_TIMEOUT:<br>
                    // Handle timeout ...<br>
        <br>
                    timeout.set(...);<br>
        <br>
                    break;<br>
        <br>
                case POLL_EVENT:<br>
                    // Handle data ...<br>
        <br>
                    break;<br>
        }<br>
        <br>
        <br>
        I.e. you typically have a poll function call (select is used in
        NUT),<br>
        and you process the result of it in a loop.<br>
        <br>
        In driver/main.c, this is done like this { poll_and_process();
        }*,<br>
        i.e. there's an empty loop around function dstate_poll_fds<br>
        which calls select, processes the result and returns.<br>
        This approach has the drawback that logic of the { poll();
        process(); }*<br>
        loop is cut by an interface; you need to propagate info about
        poll result<br>
        through that interface, which is problematic and ugly.<br>
        <br>
        <br>
        2/ There is actually possibility of busy loop in case the socket
        becomes<br>
        invalid (and any decent code should always count with that).<br>
        If this happen, select will return and the dstate_poll_fds<br>
        function will return almost immediately, too (after logging a
        message).<br>
        Almost all the time, the return code shall be 0 (timeout didn't
        occur).<br>
        This means that the dstate_poll_fds function will be immediately<br>
        re-executed, select will almost immediately return...  Busy
        loop.<br>
        <br>
        <br>
        3/ Setting of the timeout is just weird.<br>
        I actually believe that the intention was completely different<br>
        than what's implemented.<br>
        The dstate_poll_fds function 1st argument is the struct timeval<br>
        passed by value(!)  Ergo, each time it's called in the {
        poll_and_process() }*<br>
        loop, it's simply set to the same time (in future).<br>
        The function then computes a difference with the actual time<br>
        and uses the result as select timeout...<br>
        I.e, if the poll_interval was simply passed to the function<br>
        and directly used as select timeout, the result would be exactly<br>
        the same.<br>
        I think this definitely isn't the intended functionality;
        obviously,<br>
        what was ment is to poll at most poll_interval seconds, then<br>
        update data and poll again; i.e. the timeout should've been<br>
        propagated through the dstate_poll_fds interface while being<br>
        updated by select.  If done like this, the code would actually
        make<br>
        sense, although...<br>
        <br>
        4/ The propagated timeout MUST NOT be passed directly to select<br>
        ... although there is one small problem: in case of error,<br>
        select invalidates BOTH the FD set AND the timeout.<br>
        So, the timeout may actually get set to just weird very high<br>
        numbers, which will painfully mess the next (and further)
        polling.<br>
        No problem, though, we just need to copy the timeout for select.<br>
        However, this whole mechanism is just strange; hence my point
        1/.<br>
        Not to mention that it can't really work, now (because of the
        timeout<br>
        copying with every call).<br>
        <br>
        <br>
        I think this code should be reviewed.  For now, I'm going to<br>
        just fix it so that it works as probably intended, before,<br>
        but that won't fix neither the potential busy loop problem<br>
        nor the broken { poll(); process() }* polling logic.<br>
        I don't want to rewrite that in scope of "getting rid of<br>
        obsolete gettimeofdate" task; I think a feature req.<br>
        or something is due way.<br>
        <br>
        <br>
        Comments?  Ideas?  Recommendations?  All welcome...<br>
        <br>
        Thanks,<br>
        <br>
        Regards,<br>
        <br>
        vasek<br>
        <br>
        --<br>
        Vaclav Krpec<br>
        <br>
        Network UPS Tools Project<br>
        Eaton Opensource Team<br>
        <br>
      </font></font>
  </body>
</html>