[Common-dev] CR: part 3: sockimp connect failure fix plus related host byte-order fix, etc.

[Common-dev] CR: part 3: sockimp connect failure fix plus related host byte-order fix, etc.

Tom Marshall tmarshall at helixcommunity.org
Mon Aug 9 12:42:49 PDT 2004


> 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.

> @@ -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.

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

> @@ -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.

-- 
Not that I have anything much against redundancy.  But I said that already.
        -- Larry Wall
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
Url : http://lists.helixcommunity.org/pipermail/common-dev/attachments/20040809/894df6b1/attachment.bin


More information about the Common-dev mailing list
 

Site Map   |   Terms of Use   |   Privacy Policy   |   Contact Us

Copyright © 1995-2007 RealNetworks, Inc. All rights reserved. RealNetworks and Helix are trademarks of RealNetworks.
All other trademarks or registered trademarks are the property of their respective holders.