apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jeff Trawick <traw...@attglobal.net>
Subject Re: [PATCH] some progress with apr_socket_data_get()/apr_socket_data_set()
Date Wed, 16 Apr 2003 12:14:07 GMT
Justin Erenkrantz wrote:
> --On Tuesday, April 15, 2003 7:39 PM -0400 Jeff Trawick 
> <trawick@attglobal.net> wrote:
> 
>> You may have known that socket data is implemented as pool userdata in a
>> manner that prevented multiple sockets from the same pool being able 
>> to use
>> the same key for socket data.  Another problem is that any cleanup 
>> specified
>> in the call to apr_socket_data_set() is run when the pool is cleaned 
>> up, not
>> when the socket is destroyed (in contradiction to the doc).
> 
> 
> Err, yeah, why is it pool_userdata at all?  I'm confused.  Isn't using 
> pool's userdata *way* overkill for this?  Can't it just store a void* or 
> something in its internal structure?  Why does it have to live longer 
> than the socket structure itself?
> 
> Are we really wanting to support multiple socket_data instances per 
> socket? If a program wants that level, it seems it should just use the 
> apr_pool_userdata directly rather than this unnecessary abstraction.  KISS.

The context where I see this as useful is with utility routines (e.g., 
support libraries) that perform operations using the socket but don't 
have any leeway in how the app uses pools.  (This is a natural for an 
iol implemented using a socket iol "enhancement" to APR, since there is 
no leeway on interface in order to work with all unmodified APR apps.)

Converting from socket userdata to pool userdata should be this "simple" 
(and it would work properly, unlike code depending on the current 
apr_socket_data_get/set that only works if there is one such socket in 
the pool):

old way to set:
   apr_socket_data_set(sock, "val", "mykey", NULL);

new way to set (once we add a pool accessor for sockets):
   apr_pool_t *sock_pool = apr_socket_pool_get(sock);
   char keybuf[22]; /* big enough for 123456789abdef0mykey\0 */

   /* make key unique among all sockets in pool */
   apr_snprintf(keybuf, sizeof keybuf, "%ppmykey", sock);
   apr_pool_userdata_set("val", keybuf, NULL, sock_pool);

old way to get:
   apr_socket_data_get(&val, "mykey", sock);

new way to get (needs pool accessor for sockets):
   apr_pool_t *sock_pool = apr_socket_pool_get(sock);
   char keybuf[22]; /* big enough for 123456789abdef0mykey\0 */

   apr_snprintf(keybuf, sizeof keybuf, "%ppmykey", sock);
   apr_pool_userdata_get(&val, keybuf, sock_pool);

The snprintf trick to make the key unique among all sockets in the pool 
could be done in apr_socket_data_get/set but without having a bound on 
the size of the keys it requires dynamic storage allocation.

The overhead of building a unique key for every get/set operation is a 
giant concern to me.

Maybe there is a better idea for how to make the key unique without 
performing any expensive operations.

> I can buy the use of having a pointer associated with the socket.  It's 
> common enough, but yeah, you can do the same with pool_userdata 
> directly.  I'd prefer tossing out the key part in apr_socket_data_*, but 
> if you want to be cute and backwards-compat, yeah, the hash works as you 
> described.  I just question how many apps really need that.  -- justin

shrug

Perhaps it would be preferable to have no socket user data at all rather 
than a limit of exactly one item...  unclear...


Mime
View raw message