diff mbox

compiler warning: pointer type in OCTET_STRING_noalloc()

Message ID 1460637956-19900-1-git-send-email-nhofmeyr@sysmocom.de
State Not Applicable
Headers show

Commit Message

Neels Hofmeyr April 14, 2016, 12:45 p.m. UTC 
It's useless to declare the str arg as const when all the function does is
assign it to a non-const pointer.

I found one caller that wants to pass a const pointer, ranap_msg_factory.c:184.
Fixing that in a commit to osmo-iuh. All other callers pass non-const str
arguments anyway.
---
 include/asn1c/asn1helpers.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Harald Welte April 15, 2016, 7:49 p.m. UTC | #1 
Hi Neels,

On Thu, Apr 14, 2016 at 02:45:56PM +0200, Neels Hofmeyr wrote:
> It's useless to declare the str arg as const when all the function does is
> assign it to a non-const pointer.

The point of doing it this way is that we don't have to write our code
in a sub-standard way, just because asn1c is written in a way that you
cannot have 'const' buffers as input into encoding.

So 'const' here is used as a hint to the programmer, that a read-only
string is passed into this function, and hence it is input data.

So this was very intentional.  Now you can argue that my line of
thinking above is basically ignoring the reality of asn1c.  But then,
don't we have actual real issues to address, than to debate about thing
slike this?  Sorry...
Neels Hofmeyr April 18, 2016, 10:40 a.m. UTC | #2 
On Fri, Apr 15, 2016 at 09:49:55PM +0200, Harald Welte wrote:
> Hi Neels,
> 
> On Thu, Apr 14, 2016 at 02:45:56PM +0200, Neels Hofmeyr wrote:
> > It's useless to declare the str arg as const when all the function does is
> > assign it to a non-const pointer.
> 
> The point of doing it this way is that we don't have to write our code
> in a sub-standard way, just because asn1c is written in a way that you
> cannot have 'const' buffers as input into encoding.
> 
> So 'const' here is used as a hint to the programmer, that a read-only
> string is passed into this function, and hence it is input data.
>
> So this was very intentional.  Now you can argue that my line of
> thinking above is basically ignoring the reality of asn1c.  But then,
> don't we have actual real issues to address, than to debate about thing
> slike this?  Sorry...

I would prefer not to debate about this as extensively, but you guys won't stop
bickering ;)

To resolve, I would appreciate if you either apply my patch or add the cast
within the function.

I still disagree and think it is the caller's duty to be aware that the asn1c
struct entry is not const, and that passing such a buffer around possibly
results in overwriting the data. That ugly cast should be in plain sight
wherever it is necessary, not hidden away in a function call, invisible until
your code breaks. I still see only the single caller that employs const data,
and this caller can easily justify casting to non-const, as the use is well
contained.

Feel free to overrule, just please get rid of the warning...

~Neels
diff mbox

Patch

diff --git a/include/asn1c/asn1helpers.h b/include/asn1c/asn1helpers.h
index d6b5e18..90e78ae 100644
--- a/include/asn1c/asn1helpers.h
+++ b/include/asn1c/asn1helpers.h
@@ -21,7 +21,7 @@  uint32_t asn1bitstr_to_u32(const BIT_STRING_t *in);
 uint32_t asn1bitstr_to_u28(const BIT_STRING_t *in);
 uint32_t asn1bitstr_to_u24(const BIT_STRING_t *in);
 
-static inline void OCTET_STRING_noalloc(OCTET_STRING_t *s, const uint8_t *str, int size)
+static inline void OCTET_STRING_noalloc(OCTET_STRING_t *s, uint8_t *str, int size)
 {
 	s->buf = str;
 	s->size = size;