[MPlayer-dev-eng] [PATCH] (v2) LIRC input: minor fix, cleanups, and proper select(2)

[Dennis Vshivkov]

On Wed, Jan 28, 2009 at 05:02:37PM +0100, Reimar Dцffinger wrote:

 > On Thu, Jan 29, 2009 at 04:02:21AM +1300, Dennis Vshivkov
 > wrote:
 >> Comments?

 > I think you have a weird way of combining fixes into a patch
 > that makes them look far more complicated than they are.

OK, I could have made a few parts out of the last patch in the
set.  That'd mean even more patch stages for individual careful
testing.  I finished at 4 AM already that day, and tonight
again.  The changes weren't that complicated, and they were
thoroughly commented, making for good SVN commit log entries.

Anyway, since that was too hard, here's v2!  A whopping five
parts.  Again:

    Legend: ! fix, + addition, - removal, * change. 

The first two as they were:

    lirc-input-1-no-delay-fix.diff:
      ! O_NONBLOCK part of mp_input_lirc_init() moved to before
        lirc_readconfig(): where it was, it missed
        lirc_freeconfig() upon error.
      * Renamed `mode' variable in mp_input_lirc_init() to
        `flags', which it really is.

    lirc-input-2-janitorial.diff:
      * Readability: renamed variables in mp_input_lirc_read() to
        better reflect their meaning: c -> cmd, cl -> cmd_len, 
        r -> ret, s -> dest_size.
      * Consistency: mp_input_lirc_read() is not a static, so
        should have its name at the beginning of a line.
      * Consistency: messages specific to LIRC in input/lirc.c
        should be MSGT_LIRC, not the generic MSGT_INPUT.
      * Consistency: report a LIRC error not only when it happens
        in lirc_nextcode(), but also when it happens in
        lirc_code2char(); also, for clarity, report where it
        happened.

The last part of v1 is now split three ways (a bit differently
than listed above, though):

    lirc-input-3-no-cmd_buf.diff:
      - Removed cmd_buf global in input/lirc.c:
        mp_input_lirc_read() writes directly to the passed
        buffer.

    lirc-input-4-read-all-codes.diff:
      - MP_INPUT_RETRY return code for mp_input_fd_t#read_func()
        method: long unfunctional and now entirely redundant,
        mp_input_lirc_read() ceases its only use.
      * mp_input_lirc_read() now processes all of input pending
        on LIRC socket and returns all resulting commands in one
        go.

    lirc-input-5-proper-select.diff:
      * LIRC socket is (once again) made part of top-level
        select(2) in input.c, thus mp_input_lirc_read() is only
        polled when there's new input on the socket.

Also, for ease of visually checking part 4, which indents some
code, a special diff (lirc-input-4-read-all-codes.bBw.diff) was
made ignoring all whitespace changes.

 > Also cleaning up code that will be removed anyway does not
 > make that much sense.

Of what I cleaned up (8 items) only one (`ret' variable) got
removed.  And, after all, I didn't know if all three patches
were going to make it.  So I wanted each part to be complete.

In the v2 set, none of the cleaned up items are removed.

 > I was thinking of something simple like attached patch as a
 > first step,

Using snprintf() to save on strlen() of new pieces is neat.
But repeated strlen() on growing string instead of using the
result of snprintf() is a bigger waste that the removed
strlen().

Also, if the buffer fills up with an incomplete command at the
end, like

    cmd1\ncmd2\ncm

then the next time commands come, like

    cmd4\ncmd5\n

the first of them (cmd4) will be ignored via the `drop' flag
codepath in mp_input_read_cmd().

The v2 uses snprintf(), but is free of the above troubles.  The
downside, compared to v1, is that it underuses the buffer by one
byte (for a trailing NUL).  IMHO that's negligible.

 > but I can't test it.

I always have my patches tested as much as I can, every separate
stage.

-- 
/Dennis Vshivkov <jaimor at orcon.net.nz>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: lirc-input-1-no-delay-fix.diff
Type: text/x-diff
Size: 1601 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20090131/e5772aa4/attachment.diff>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: lirc-input-2-janitorial.diff
Type: text/x-diff
Size: 2980 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20090131/e5772aa4/attachment-0001.diff>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: lirc-input-3-no-cmd_buf.diff
Type: text/x-diff
Size: 2484 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20090131/e5772aa4/attachment-0002.diff>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: lirc-input-4-read-all-codes.bBw.diff
Type: text/x-diff
Size: 1905 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20090131/e5772aa4/attachment-0003.diff>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: lirc-input-4-read-all-codes.diff
Type: text/x-diff
Size: 3054 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20090131/e5772aa4/attachment-0004.diff>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: lirc-input-5-proper-select.diff
Type: text/x-diff
Size: 735 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20090131/e5772aa4/attachment-0005.diff>