On Sun, 2015-04-12 at 13:12 -0400, Rafael Schloming wrote: > On Fri, Apr 10, 2015 at 11:37 AM, Alan Conway <[email protected]> wrote: > > > > > On Wed, 2015-04-08 at 14:50 -0400, Andrew Stitcher wrote: > > > On Wed, 2015-04-08 at 10:48 -0400, Alan Conway wrote: > > > > ... > > > > The issue is that we need buffer growth AND control over allocation. > > > > pn_buffer_t forces use of malloc/free/realloc. That won't help if > > you're > > > > trying to get the data into a buffer allocated by Go for example. I > > > > agree a growable buffer type is a nicer API than raw function pointers, > > > > but the buffer type itself would need to use function pointers so we > > can > > > > replace the functions used for alloc/free/realloc. > > > > > > I think this is a pretty general issue actually that also crops up in > > > embedded systems, where you want some different control over memory > > > allocation. > > > > > > I think it might be better addressed by making malloc/free/realloc > > > replaceable for the whole of proton - would that solve your go issue? > > > > Sadly no. A proton realloc surrogate is just going to be passed void* > > and return void*. There's no memory safe way to implement that in Go, > > unless someone keeps a Go pointer to the returned buffer it gets garbage > > collected. There's no way to communicate a Go pointer thru a C proton > > interface. > > > > Given all that, I think I prefer the sprintf-style solution for growing > > buffers. It keeps the memory management strictly on the caller side of > > the pn_ interface which is simpler. > > > > If there is a big pn_data overhaul in the works then maybe we should not > > be talking about pn_data_encode and instead make sure we fix it in the > > overhaul. > > > > I notice we have these other APIs: > > > > codec.h:499:PN_EXTERN int pn_data_format(pn_data_t *data, char *bytes, > > size_t *size); > > message.h:739:PN_EXTERN int pn_message_encode(pn_message_t *msg, char > > *bytes, size_t *size); > > ssl.h:320:PN_EXTERN int pn_ssl_get_peer_hostname( pn_ssl_t *ssl, char > > *hostname, size_t *bufsize ); > > > > That is a better signature IMO, the new (backward compatible) syntax for > > such functions would be: > > > > return 0: *size is updated to be the actual size. > > return PN_OVERFLOW: *size is updated to be the required size. > > return < 0: *size is undefined. > > don't return > 0 > > > > Perhaps for the overhaul we should ensure all such functions (including > > the replacement for pn_data_encode) follow this pattern? > > > > I'm a big +1 on standardizing on a single snprintf style signature for all > encode functions. > > The signatures you mention above (similar to sprintf but using an IN/OUT > parameter) were my first attempt at a standard encode/decode signature. I > agree with you that at first it looks a bit more appealing for some reason, > however after using it for a while I actually found it quite cumbersome > relative to the traditional snprintf style. In the common case it takes > almost twice as much code to use it since you always have to declare, > initialize, and check an extra variable in separate steps due to the IN/OUT > parameter. It also doesn't map that nicely when you swig it since most > languages don't do the IN/OUT parameter thing well, so you have to have > more cumborsome/custom typemaps and/or wrapper code to deal with the > impedance mismatch. > > For those reasons I propose that we standardize on the traditional snprintf > rather than using the IN/OUT variant.
That works for me, now how do we manage the transition? I don't think we can afford to inflict "yum update proton; all proton apps crash" on our users. That means we cannot change the behavior of existing function names. I don't much like adding encode_foo2 for every encode_foo but I don't see what else to do.
