Message ID | 1460638140-20008-1-git-send-email-nhofmeyr@sysmocom.de |
---|---|
State | Not Applicable |
Headers | show |
> On 14 Apr 2016, at 08:49, Neels Hofmeyr <nhofmeyr@sysmocom.de> wrote: > > This is after the patch to libasn1c, removing str arg constness. > --- > src/ranap_msg_factory.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/ranap_msg_factory.c b/src/ranap_msg_factory.c > index bdae92e..461df1a 100644 > --- a/src/ranap_msg_factory.c > +++ b/src/ranap_msg_factory.c > @@ -181,7 +181,7 @@ struct msgb *ranap_new_msg_dt(uint8_t sapi, const uint8_t *nas, unsigned int nas > ies.sapi = RANAP_SAPI_sapi_0; > > /* Avoid copying + later freeing of OCTET STRING */ > - OCTET_STRING_noalloc(&ies.nas_pdu, nas, nas_len); > + OCTET_STRING_noalloc(&ies.nas_pdu, (uint8_t*)nas, nas_len); doesn't seem the right way forward. Does OCTET_STRING_noalloc or its users need/want to modify the buffer? holger
On Thu, Apr 14, 2016 at 09:33:37AM -0400, Holger Freyther wrote: > > > On 14 Apr 2016, at 08:49, Neels Hofmeyr <nhofmeyr@sysmocom.de> wrote: > > > > This is after the patch to libasn1c, removing str arg constness. > > --- > > src/ranap_msg_factory.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/src/ranap_msg_factory.c b/src/ranap_msg_factory.c > > index bdae92e..461df1a 100644 > > --- a/src/ranap_msg_factory.c > > +++ b/src/ranap_msg_factory.c > > @@ -181,7 +181,7 @@ struct msgb *ranap_new_msg_dt(uint8_t sapi, const uint8_t *nas, unsigned int nas > > ies.sapi = RANAP_SAPI_sapi_0; > > > > /* Avoid copying + later freeing of OCTET STRING */ > > - OCTET_STRING_noalloc(&ies.nas_pdu, nas, nas_len); > > + OCTET_STRING_noalloc(&ies.nas_pdu, (uint8_t*)nas, nas_len); > > doesn't seem the right way forward. Does OCTET_STRING_noalloc or its users need/want to modify the buffer? So, I want to change this: static inline void OCTET_STRING_noalloc(OCTET_STRING_t *s, const uint8_t *str, int size) { s->buf = str; s->size = size; } because OCTET_STRING_t.s is not const, and must not be const, will never be const. OCTET_STRING_noalloc() has no way to tell whether callers will subsequently modify the buffer or not. So that function must not secretly unconst: static inline void OCTET_STRING_noalloc(OCTET_STRING_t *s, const uint8_t *str, int size) { s->buf = (uint8_t*)str; s->size = size; } ^ this is bad. To get rid of compiler warnings, I want to change to more sensible: static inline void OCTET_STRING_noalloc(OCTET_STRING_t *s, uint8_t *str, int size) { s->buf = str; s->size = size; } All callers of OCTET_STRING_noalloc() pass a non-const pointer, except that one above. I just want to unconst that single caller. OCTET_STRING_t.buf is simply not const. So if we're not going to do an ugly unconst like this somewhere, we have to change ranap_new_msg_dt() to take a non-const uint8_t* nas. I didn't want to change that though, because the function does rightfully signal outwards that *nas is never changed, because its use of the unconsted value is contained entirely in that function (to encode a message) and nas is in fact not modified (given that the encoders work correctly). Admitted, I should also add a comment. ~Neels
On Thu, Apr 14, 2016 at 04:57:11PM +0200, Neels Hofmeyr wrote: > All callers of OCTET_STRING_noalloc() pass a non-const pointer, except > that one above. I just want to unconst that single caller. I think the other callers should be fixed, rather than aligning the one correct part of the code. As indicated, I believe we shouldn't let bad impleentation or API design by asn1c contaminate our code. A pointer to input data into an encoding function should be const.
On Thu, Apr 14, 2016 at 04:57:11PM +0200, Neels Hofmeyr wrote: > So, I want to change this: > > static inline void OCTET_STRING_noalloc(OCTET_STRING_t *s, const uint8_t *str, int size) > { > s->buf = str; > s->size = size; > } > > because OCTET_STRING_t.s is not const, and must not be const, will never be const. I meant OCTET_STRING_t.buf, of course. ~Neels
diff --git a/src/ranap_msg_factory.c b/src/ranap_msg_factory.c index bdae92e..461df1a 100644 --- a/src/ranap_msg_factory.c +++ b/src/ranap_msg_factory.c @@ -181,7 +181,7 @@ struct msgb *ranap_new_msg_dt(uint8_t sapi, const uint8_t *nas, unsigned int nas ies.sapi = RANAP_SAPI_sapi_0; /* Avoid copying + later freeing of OCTET STRING */ - OCTET_STRING_noalloc(&ies.nas_pdu, nas, nas_len); + OCTET_STRING_noalloc(&ies.nas_pdu, (uint8_t*)nas, nas_len); /* ies -> dt */ rc = ranap_encode_directtransferies(&dt, &ies);