RE: [boost] Re: Sockets!

From: Rob Lievaart (Rob.Lievaart_at_[hidden])
Date: 2003-10-27 04:55:32

Hi Jessie,

Some quick feedback, I haven't actually tried to use it.
In general I think a socket framework would be usefull,
but as you can see in the vault a lot of people have tried,
and non has succeeded so far.  

> I'd like to get feedback on the current state of the library 
> and see what
> can be improved (I'm mostly interested in the design aspect). 
Which is not documented, so hard to comment on. :-)

- I like the idea of having streams, that can wrap around sockets,
  they can make it easy to use sockets in a lot of places.

- ipv4 vs. ipv6 
   From what I understand, to talk to a host I need to do something like
   this:

      typedef ipv4_domain domain;
      domain::address_type addr( host<domain>( "www.boost.org"
).address().ip(), 80 );
      connector<domain> conn;
      basic_socketstream<domain, char> sockstream( "stream:tcp" );
      ...

   This means that my code is locked to ipv4,  should boost ever become
   reachable via ipv6 then I'll still use ipv4 and if that is ever
   dropped I'm out of luck.

   Sometimes you want to be able to specify exactly what protocol to use,
but
   usually, I don't care.  It would be nice if the library could handle this
   so in the default case I don't have to think about it and my program
   will work with both ipv6 and ipv4 hosts/clients. (if and when available)

   (In Unix Network Programming, W. Richard Stevens describes some C wrapper
   functions that lets you treat IPV4, IPV6, and Unix domain sockets, more
   or less transparently)

   Also I wonder if the domain really needs to be a parameter everywhere.
   IIRC, the only difference at the level of the socket api is
   size fo the address structure being passed around, and the handling
   of gethostbyname/getaddrinfo functions. And some extra coket options.

   I expect it is possible to make most of the library unaware of the
existence
   of the different domains.  Probably only the address_type needs to be
aware
   that differences exist, but it could hide it from most of the library 
   and from the user.  

-  I see a connector , I assume there will also be an acceptor
   to accompany the "server_socket"?  I am still not to comfortable with
   the connector/acceptor idea.  Acceptors have to do some work, listen
   on a socket, create a new one, etc.  so it makes some sense to create
   a seperate object for that.  And then a connector looks good with that,
   but a whole object just for a call to
      ::connect( ... ) 
   feels kind of overdoing it to me.
   (I know loads of people disagree with me, I just thought I'd mention it.)

-  Exceptions:
   You have a large exception hierarchy.  There has been some debate on the
use
   of large exception hierarchies versus a single exception with error
codes.
   I am not sure if there was a winner, but personnally I would prefer a
single
   socket exception with errorcodes. 
   You currently have 20 or so exception classes and they are members of
either
   the socket_base or the host_base _class_ instead of boost::net or where
ever
   the library would live.

-  Host class
   Here you also have the domain ipv4,ipv6 in the type, but a host could
   have both an ipv4 and ipv6 stack, and AFAIK most current host that have
   IPV6 also have an ipv4 stack.  So in this case, the address list of
   a host would contain ipv4 and ipv6 addresses.

-  I wonder what the use of the abstract base classes domain_base and
   address_base is.  It looks to me like you only use their subclasses
   in templates and never plymorphically, so why would you make a 
   baseclass with virtual functions?

-  Why the split into basic_socket and basic_socket implementation,
   what is the use of that?  All the members of basic_socket_impl
   are virtual, but I don't see any case of wanting to replace this
   with something else at runtime.
   It looks to me like you are using it to share implementation between
   basic_socket and server_socket,  but wouldn't it be easier to
   do this using a common baseclass.  I could have missed it but
   I see no use for the seperate implementation class. 

Some implementation issues:

-  You like to use underscores in variable names, names starting with
   double underscores, or starting with an underscore and all caps are
   reserved.  They could clash with compiler defined names, so you
   should avoid them.  Take a look at:
   
    http://oakroadsystems.com/tech/cppredef.htm

-  I see quite a few ifdefs related to the socklen_t type,
   e.g.:

    #if defined( WIN32 )
        int _NameLength = sizeof( address_type::native_t );
    #else
        socklen_t _NameLength = sizeof( address_type::native_t );
    #endif

   These could be replaced by a single typedef somewhere.  Makes
   the code a lot simpler.

-  The shutdown function taking an ios_base::openmode as parameter
   seems strange.

-  It would be nice if the connector also accepted socket streams, instead 
   of needing to pass stream->rdsocket(). Also I am not sure if you want to 
   expose the socket at this level, the socket streams wrap a buffer that
wraps
   a socket, so maybe rdbuf should be enough here, from there you can
   go to the socket, iff required.

Just my $0.02,

Rob Lievaart.