At 12:42 PM 8/9/2004, Tom Marshall wrote:
> > How does everything else look?
>
>It looks good but I have a couple minor nitpicks...
>
> > @@ -433,12 +435,17 @@
> >
> >  #define HX_SOCK_EVENT_ALL       (~0)
> >
> > -typedef enum
> > -{
> > -    HX_SOCKBUF_DEFAULT,         // IHXBuffer
> > -    HX_SOCKBUF_TIMESTAMPED      // IHXTimeStampedBuffer
> > +typedef enum {
> > +    HX_SOCKBUF_DEFAULT,         /* IHXBuffer */
> > +    HX_SOCKBUF_TIMESTAMPED      /* IHXTimeStampedBuffer */
> >  } HXSockBufType;
> >
> > +typedef enum {
> > +    HX_SOCK_READBUF_SIZE_DEFAULT,    /* read once into default sized read
> > buffer */
> > +    HX_SOCK_READBUF_SIZE_COPY,       /* same as default but reallocates
> > once size is known */
> > +    HX_SOCK_READBUF_SIZE_PEEK        /* use facility such as FIONBIO to
> > determine size of pending data in network buffers */
> > +} HXSockReadBufFlag;
> > +
> >  /* Socket options for IHXSocket::GetOption() and IHXSocket::SetOption() */
> >  /* XXXTDM: Separate these like the real getsockopt/setsockopt? */
> >  typedef enum {
>
>It would be nice to use either "Type" or "Flag" consistently.  "Flag"
>implies a boolean to me, so I think "Type" is probably a better choice.

I see your point about "Flag". I purposely chose not to use "Type" because 
the term as used in other options such as "HXSockBufType" corresponds to an 
object type as opposed to a behavior attribute.

HXSockReadBufAttr
HXSockReadBufCtrl
???


> > @@ -494,9 +501,11 @@
> >      HX_SOCKOPT_IDLETIMEOUT,
> >
> >      /* Helix only*/
> > -    HX_SOCKOPT_BUFFER_TYPE,
> > -    HX_SOCKOPT_APP_SNDBUF,
> > -    HX_SOCKOPT_APP_RCVBUF,
> > +    HX_SOCKOPT_BUFFER_TYPE,     /* IHXBuffer type returned from read
> > functions */
> > +    HX_SOCKOPT_APP_SNDBUF,      /* outbound app buffer size in bytes*/
> > +    HX_SOCKOPT_APP_RCVBUF,      /* inbound app buffer size in bytes*/
> > +    HX_SOCKOPT_READBUF_FLAG,    /* controls read buffer allocation and
> > sizing behavior */
> > +    HX_SOCKOPT_READBUF_MAX,     /* max read buffer size in bytes 
> (datagram
> > and stream) */
> >
> >      HX_SOCKOPT_LAST
> >  } HXSockOpt;
> >
>
>It appears that HX_SOCKOPT_APP_RCVBUF was added and never used. I imagine
>that it was intended fill the same role as HX_SOCKOPT_READBUF_MAX.


It is used on the client for specifying the application rcv buffer size in 
the threaded socket. It is similar to the HX_SOCKOPT_APP_SNDBUF but used 
for buffering reads rather than writes.


>I like the _APP_ qualifier for Helix specific application level options.
>
>So can you rename the options, perhaps to this?
>
>         /* Application specific */
>         HX_SOCKOPT_APP_IDLETIMEOUT,
>         HX_SOCKOPT_APP_SNDBUF,
>         HX_SOCKOPT_APP_RCVBUF,
>         HX_SOCKOPT_APP_READBUF_TYPE,
>
>         HX_SOCKOPT_LAST

Yes, sounds good, except I'm not sure about HX_SOCKOPT_APP_READBUF_TYPE in 
light of HXSockBufType.


> > @@ -1702,11 +1705,31 @@
> >          *pval = m_uIdleTimeout;
> >          return HXR_OK;
> >      }
> > -    if (name == HX_SOCKOPT_BUFFER_TYPE)
> > +    else if (name == HX_SOCKOPT_BUFFER_TYPE)
> >      {
> >          *pval = m_bufType;
> >          return HXR_OK;
> >      }
> > +    else if (name == HX_SOCKOPT_READBUF_FLAG)
> > +    {
> > +        *pval = m_readBufFlag;
> > +        return HXR_OK;
> > +    }
> > +    else if (name == HX_SOCKOPT_READBUF_MAX)
> > +    {
> > +        *pval = m_mss;
> > +        return HXR_OK;
> > +    }
> > +    else if (name == HX_SOCKOPT_APP_SNDBUF)
> > +    {
> > +        *pval = 0;
> > +        if (m_pSendBuffer)
> > +        {
> > +            *pval = m_pSendBuffer->GetSize();
> > +        }
> > +        return HXR_OK;
> > +    }
> > +
> >
> >      if (hx_getsockopt(&m_sock, name, pval) != 0)
> >      {
>
>It looks like we are collecting quite a large number of handlers for app
>level socket options.  A switch statement is generally more efficient than
>an if/else ladder in this situation.

I agree.

Thanks,
Liam


>--
>Not that I have anything much against redundancy.  But I said that already.
>         -- Larry Wall