Re: [PATCH 2/3] forkpty, openpty: split functions into new modules

[Eric Blake]

On 03/18/2010 07:35 PM, Bruno Haible wrote:
> Hi Eric,
> 
> You requested a rapid review:

Well, supplying rapid comments saying 'please give me more time' are
also acceptable to slow me down; but I do appreciate the rapid review!

> 
> Eric Blake wrote:
>> +  AC_SEARCH_LIBS([forkpty], [util libutil],
>> +    [if test "$ac_cv_search_forkpty" != "none required"; then
>> +       PTY_LIB="$ac_cv_search_forkpty"
>> +     fi])
> 
> The second argument [util libutil] can be simplified to [util], no?
> Searching for liblibutil sounds useless.

Argh.  I was thinking <util.h> vs. <libutil.h>, but this is for -lutil
(vs. -lc).  You are correct; I'll simplify that.

>> +Files:
>> +m4/forkpty.m4
>> +
>> +Depends-on:
>> +pty
> 
> I don't understand two things here:
>   - Why does 'forkpty' depend on 'openpty'?

I guess it doesn't have to.  It's just that for now, test-forkpty.c
tests both interfaces.

>   - Since forkpty.m4 is used for both 'forkpty' and 'openpty', it is asymetric
>     and a bit misleading to call it 'forkpty.m4'. Why not call it 'pty.m4'?

Sure; I named it forkpty.m4 to avoid confusion with the fact that I just
renamed pty.m4 -> pty_h.m4 in the previous patch.  But pty.m4 for both
forkpty and openpty makes sense.  I'll post a respin of the patches,
incorporating your comments, and restart the review clock.

-- 
Eric Blake   address@hidden    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature