gsm0480.c: squelch compiler warning due to const (caused recently)
diff mbox

Message ID 1457995959-12157-1-git-send-email-nhofmeyr@sysmocom.de
State New
Headers show

Commit Message

Neels Hofmeyr March 14, 2016, 10:52 p.m. UTC
The introduction of gsm48_hdr_pdisc() caused a new compiler warning.
In gsm0480_decode_*(), the gsm48_hdr is const, which we can safely cast
away to avoid the warning.
---
 src/gsm/gsm0480.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Harald Welte March 16, 2016, 10:28 a.m. UTC | #1
Hi Neels,

On Mon, Mar 14, 2016 at 11:52:39PM +0100, Neels Hofmeyr wrote:
> The introduction of gsm48_hdr_pdisc() caused a new compiler warning.
> In gsm0480_decode_*(), the gsm48_hdr is const, which we can safely cast
> away to avoid the warning.

Why does gsm48_hdr_pdisc() not take a 'const struct gsm48_hdr *'
argument in the first place?  I don't think it modifies the contents of
the structure...
Neels Hofmeyr March 16, 2016, 10:22 p.m. UTC | #2
On Wed, Mar 16, 2016 at 11:28:30AM +0100, Harald Welte wrote:
> Why does gsm48_hdr_pdisc() not take a 'const struct gsm48_hdr *'
> argument in the first place?  I don't think it modifies the contents of
> the structure...

Yes, Holger has applied the patch with exactly that change.

Yet this has me confused. A while back I concluded that C doesn't allow
passing non-const instances to functions that expect a const arg. That was
discussed on this list in January:

http://lists.osmocom.org/pipermail/openbsc/2016-January/001051.html
http://lists.osmocom.org/pipermail/openbsc/2016-January/001055.html

How is this different from the conclusion that Jacob confirmed two months
ago? Is it int (primitives) vs. struct??

Thanks!

~Neels
Harald Welte March 17, 2016, 7:06 a.m. UTC | #3
Hi Neels,

On Wed, Mar 16, 2016 at 11:22:11PM +0100, Neels Hofmeyr wrote:
> On Wed, Mar 16, 2016 at 11:28:30AM +0100, Harald Welte wrote:
> > Why does gsm48_hdr_pdisc() not take a 'const struct gsm48_hdr *'
> > argument in the first place?  I don't think it modifies the contents of
> > the structure...
> 
> Yes, Holger has applied the patch with exactly that change.
> 
> Yet this has me confused. A while back I concluded that C doesn't allow
> passing non-const instances to functions that expect a const arg. 

We do that all the time in Osmocom projects.  Especially for functions
that have input pointers (should be "const foo *") and output pointers
(should be "foo *), you can easily safeguard agaist some common cases
where the caller swaps the arguments this way.

This is standard practise, see the definition of memcpy:

 void *memcpy(void *dest, const void *src, size_t n);

> http://lists.osmocom.org/pipermail/openbsc/2016-January/001051.html
> http://lists.osmocom.org/pipermail/openbsc/2016-January/001055.html
> 
> How is this different from the conclusion that Jacob confirmed two months
> ago? Is it int (primitives) vs. struct??

from a quick look, the above discussion was about passing the address of
a pointer, and of course if you pass that into a function, you expect
the function to be able to modify the pointer stored at that address?
Neels Hofmeyr March 17, 2016, 3:02 p.m. UTC | #4
On Thu, Mar 17, 2016 at 08:06:32AM +0100, Harald Welte wrote:
> This is standard practise, see the definition of memcpy:
> 
>  void *memcpy(void *dest, const void *src, size_t n);

I'm very familiar with that and want to use this everywhere and always,
which is why my conclusion that it apparently didn't work was quite a
disappointment. Glad to see that it does work after all...

What bugs me is that I don't understand the apparent corner case where it
doesn't work.

> > http://lists.osmocom.org/pipermail/openbsc/2016-January/001051.html
> > http://lists.osmocom.org/pipermail/openbsc/2016-January/001055.html
> > 
> > How is this different from the conclusion that Jacob confirmed two months
> > ago? Is it int (primitives) vs. struct??
> 
> from a quick look, the above discussion was about passing the address of
> a pointer, and of course if you pass that into a function, you expect
> the function to be able to modify the pointer stored at that address?

First, let's clarify:

const int * const x;
  the pointer x can't be modified, neither can the int at *x be modified.

const int *x;
  pointer x can be modified, but the int at *x can't be modified.

const int **x;
  x can be modified, *x can be modified, but the int at **x can't be modified.

right?

Let's look at a simple example that works without warning: I only want to
modify a pointer, not the ints.

  const int *func(const int *yy)
  {
          return yy + 1;
  }

  int main(void)
  {
          int y[2] = {23, 42};
          int *yy = y;
          return *func(yy); // returns 42
  }

This works perfectly. I pass a pointer to a mutable int, though the function
expects a pointer to a const int. No compiler warning.

And the failing example again; it does the exact same thing, only it returns
the modified pointer in an output-argument, using a pointer to a pointer to a
const int:

  void func(const int **yyy)
  {
          (*yyy) ++;
  }

  int main(void)
  {
          int y[2] = {23, 42};
          int *yy = y;
          func(&yy);

          return *yy; // returns 42
  }

And here I get the warning:

  cc     check.c   -o check
  check.c: In function ‘main’:
  check.c:11:14: warning: passing argument 1 of ‘func’ from incompatible pointer type
           func(yyy);
                ^
  check.c:1:6: note: expected ‘const int **’ but argument is of type ‘int **’
   void func(const int **yyy)
      ^

I'm quite happy that it seems to be only a corner case, and I'll be consting
aggressively and happily ever after :)

Yet it's a stupid corner case. Would be great if someone had an explanation to
this odd gcc behavior.

Maybe it's rather a question for another mailing list, too...

~Neels
Harald Welte March 17, 2016, 3:25 p.m. UTC | #5
Hi Neels,

On Thu, Mar 17, 2016 at 04:02:07PM +0100, Neels Hofmeyr wrote:

> What bugs me is that I don't understand the apparent corner case where it
> doesn't work.

I'm sorry, I cannot help you there. My knowledge about the corner cases
is limited, and I'm happy to accept it as it hasn't been an issue yet.
we rarely use pointers to pointers in the osmocom code, luckily ;)

Call me ignorant, if you'd like :)
Neels Hofmeyr March 18, 2016, 10:08 a.m. UTC | #6
On Thu, Mar 17, 2016 at 04:25:48PM +0100, Harald Welte wrote:
> Hi Neels,
> 
> On Thu, Mar 17, 2016 at 04:02:07PM +0100, Neels Hofmeyr wrote:
> 
> > What bugs me is that I don't understand the apparent corner case where it
> > doesn't work.
> 
> I'm sorry, I cannot help you there. My knowledge about the corner cases
> is limited, and I'm happy to accept it as it hasn't been an issue yet.
> we rarely use pointers to pointers in the osmocom code, luckily ;)
> 
> Call me ignorant, if you'd like :)

Heh, good answer :)

At least I know now that it's a corner case.

~Neels

Patch
diff mbox

diff --git a/src/gsm/gsm0480.c b/src/gsm/gsm0480.c
index 8963b78..bb47cc6 100644
--- a/src/gsm/gsm0480.c
+++ b/src/gsm/gsm0480.c
@@ -220,7 +220,7 @@  int gsm0480_decode_ussd_request(const struct gsm48_hdr *hdr, uint16_t len,
 		return 0;
 	}
 
-	if (gsm48_hdr_pdisc(hdr) == GSM48_PDISC_NC_SS) {
+	if (gsm48_hdr_pdisc((struct gsm48_hdr*)hdr) == GSM48_PDISC_NC_SS) {
 		req->transaction_id = hdr->proto_discr & 0x70;
 
 		ss.transaction_id = req->transaction_id;
@@ -254,7 +254,7 @@  int gsm0480_decode_ss_request(const struct gsm48_hdr *hdr, uint16_t len,
 		return 0;
 	}
 
-	if (gsm48_hdr_pdisc(hdr) == GSM48_PDISC_NC_SS) {
+	if (gsm48_hdr_pdisc((struct gsm48_hdr*)hdr) == GSM48_PDISC_NC_SS) {
 		req->transaction_id = hdr->proto_discr & 0x70;
 		rc = parse_ss(hdr, len, req);
 	}