qpid-proton mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alan Conway <acon...@redhat.com>
Subject Re: codec changes
Date Tue, 14 Apr 2015 17:27:43 GMT
On Sun, 2015-04-12 at 13:12 -0400, Rafael Schloming wrote:
> On Fri, Apr 10, 2015 at 11:37 AM, Alan Conway <aconway@redhat.com> 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.



Mime
View raw message