[Common-dev] CR: part 3: sockimp connect failure fix plus related host byte-order fix, etc.
Tom Marshall tmarshall at helixcommunity.org> 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