18 Oct
2016
18 Oct
'16
2:15 a.m.
Presumably after this libosmocore commit, I get scores of new warnings (see
below) when compiling openbsc:
commit ed9d6da5df98538adc70aa03cb569eb9505d04b6
Author: Max <msuraev(a)sysmocom.de>
AuthorDate: Tue Oct 11 15:20:28 2016 +0200
Constify ctrl_cmd struct fields where appropriate
Some of these can be fixed by constifying the functions as well, but
talloc_free() of a const char * looks really wrong?
Please follow up on this or revert the commit... constifying is good, but
having twice the amount of warnings means twice the reading effort during
development cycles.
I would most prefer a revert now and come back as soon as no new warnings are
introduced; ymmv...
In file included from /usr/local/include/osmocom/core/talloc.h:4:0,
from ../../../src/osmo-bsc_nat/bsc_nat_ctrl.c:22:
../../../src/osmo-bsc_nat/bsc_nat_ctrl.c: In function ‘bsc_nat_handle_ctrlif_msg’:
../../../src/osmo-bsc_nat/bsc_nat_ctrl.c:131:16: warning: passing argument 1 of
‘_talloc_free’ discards ‘const’ qualifier from pointer target type
talloc_free(cmd->variable);
^
/usr/include/talloc.h:227:5: note: expected ‘void *’ but argument is of type ‘const char
*’
int _talloc_free(void *ptr, const char *location);
^
../../../src/osmo-bsc_nat/bsc_nat_ctrl.c:143:34: warning: passing argument 2 of
‘bsc_get_pending’ discards ‘const’ qualifier from pointer target type
pending = bsc_get_pending(bsc, cmd->id);
^
../../../src/osmo-bsc_nat/bsc_nat_ctrl.c:86:29: note: expected ‘char *’ but argument is of
type ‘const char *’
static struct bsc_cmd_list *bsc_get_pending(struct bsc_connection *bsc, char *id_str)
^
../../../src/osmo-bsc_nat/bsc_ussd.c: In function ‘bsc_ussd_check’:
../../../src/osmo-bsc_nat/bsc_nat_ctrl.c: In function ‘forward_to_bsc’:
../../../src/osmo-bsc_nat/bsc_nat_ctrl.c:228:31: warning: passing argument 1 of
‘extract_bsc_nr_variable’ discards ‘const’ qualifier from pointer target type
if (!extract_bsc_nr_variable(cmd->variable, &nr, &bsc_variable)) {
^
../../../src/osmo-bsc_nat/bsc_nat_ctrl.c:198:12: note: expected ‘char *’ but argument is
of type ‘const char *’
static int extract_bsc_nr_variable(char *variable, unsigned int *nr, char
**bsc_variable)
^
In file included from /usr/local/include/osmocom/core/talloc.h:4:0,
from ../../../src/osmo-bsc_nat/bsc_nat_ctrl.c:22:
../../../src/osmo-bsc_nat/bsc_nat_ctrl.c:259:16: warning: passing argument 1 of
‘_talloc_free’ discards ‘const’ qualifier from pointer target type
talloc_free(bsc_cmd->id);
^
/usr/include/talloc.h:227:5: note: expected ‘void *’ but argument is of type ‘const char
*’
int _talloc_free(void *ptr, const char *location);
^
../../../src/osmo-bsc_nat/bsc_nat_ctrl.c:266:16: warning: passing argument 1 of
‘_talloc_free’ discards ‘const’ qualifier from pointer target type
talloc_free(bsc_cmd->variable);
^
/usr/include/talloc.h:227:5: note: expected ‘void *’ but argument is of type ‘const char
*’
int _talloc_free(void *ptr, const char *location);
^
../../../src/osmo-bsc_nat/bsc_nat_ctrl.c: In function ‘extract_bsc_cfg_variable’:
../../../src/osmo-bsc_nat/bsc_nat_ctrl.c:323:31: warning: passing argument 1 of
‘extract_bsc_nr_variable’ discards ‘const’ qualifier from pointer target type
if (!extract_bsc_nr_variable(cmd->variable, &nr, bsc_variable)) {
^
../../../src/osmo-bsc_nat/bsc_nat_ctrl.c:198:12: note: expected ‘char *’ but argument is
of type ‘const char *’
static int extract_bsc_nr_variable(char *variable, unsigned int *nr, char
**bsc_variable)
^
--
- Neels Hofmeyr <nhofmeyr(a)sysmocom.de> http://www.sysmocom.de/
=======================================================================
* sysmocom - systems for mobile communications GmbH
* Alt-Moabit 93
* 10559 Berlin, Germany
* Sitz / Registered office: Berlin, HRB 134158 B
* Geschäftsführer / Managing Directors: Harald Welte
18 Oct
18 Oct
12:46 p.m.
Hi.
I think it's better to fix those warnings instead of rolling back
commit. It was added because without it there were plenty of warnings in
other places so overall we won't decrease number of warnings - we'll
just shift them from one place to another. Besides if we follow that
logic we have to rollback f45334be29016a36594aacc07c90e262e4994525 for
example.
Regarding the potential fix - it pretty-much boild down to interaction
between strtok_r() and talloc(). The obvious way would be to
talloc_strdup() string before passing it to 1st call to strtok_r(). The
potential problem is that strtok_r() changes it's argument. So the
questions are:
- do we expect that strtok_r() changes to 1st argument? If so - what
kind of changes are expected?
- what does strtok_r() do with memory? When it is safe to remove
duplicated string passed to it?
On 10/18/2016 02:15 AM, Neels Hofmeyr wrote:
Presumably after this libosmocore commit, I get scores
of new warnings (see
below) when compiling openbsc:
commit ed9d6da5df98538adc70aa03cb569eb9505d04b6
Author: Max <msuraev(a)sysmocom.de>
AuthorDate: Tue Oct 11 15:20:28 2016 +0200
Constify ctrl_cmd struct fields where appropriate
Some of these can be fixed by constifying the functions as well, but
talloc_free() of a const char * looks really wrong?
Please follow up on this or revert the commit... constifying is good, but
having twice the amount of warnings means twice the reading effort during
development cycles.
I would most prefer a revert now and come back as soon as no new warnings are
introduced; ymmv...
In file included from /usr/local/include/osmocom/core/talloc.h:4:0,
from ../../../src/osmo-bsc_nat/bsc_nat_ctrl.c:22:
../../../src/osmo-bsc_nat/bsc_nat_ctrl.c: In function ‘bsc_nat_handle_ctrlif_msg’:
../../../src/osmo-bsc_nat/bsc_nat_ctrl.c:131:16: warning: passing argument 1 of
‘_talloc_free’ discards ‘const’ qualifier from pointer target type
talloc_free(cmd->variable);
^
/usr/include/talloc.h:227:5: note: expected ‘void *’ but argument is of type ‘const char
*’
int _talloc_free(void *ptr, const char *location);
^
../../../src/osmo-bsc_nat/bsc_nat_ctrl.c:143:34: warning: passing argument 2 of
‘bsc_get_pending’ discards ‘const’ qualifier from pointer target type
pending = bsc_get_pending(bsc, cmd->id);
^
../../../src/osmo-bsc_nat/bsc_nat_ctrl.c:86:29: note: expected ‘char *’ but argument is
of type ‘const char *’
static struct bsc_cmd_list *bsc_get_pending(struct bsc_connection *bsc, char *id_str)
^
../../../src/osmo-bsc_nat/bsc_ussd.c: In function ‘bsc_ussd_check’:
../../../src/osmo-bsc_nat/bsc_nat_ctrl.c: In function ‘forward_to_bsc’:
../../../src/osmo-bsc_nat/bsc_nat_ctrl.c:228:31: warning: passing argument 1 of
‘extract_bsc_nr_variable’ discards ‘const’ qualifier from pointer target type
if (!extract_bsc_nr_variable(cmd->variable, &nr, &bsc_variable)) {
^
../../../src/osmo-bsc_nat/bsc_nat_ctrl.c:198:12: note: expected ‘char *’ but argument is
of type ‘const char *’
static int extract_bsc_nr_variable(char *variable, unsigned int *nr, char
**bsc_variable)
^
In file included from /usr/local/include/osmocom/core/talloc.h:4:0,
from ../../../src/osmo-bsc_nat/bsc_nat_ctrl.c:22:
../../../src/osmo-bsc_nat/bsc_nat_ctrl.c:259:16: warning: passing argument 1 of
‘_talloc_free’ discards ‘const’ qualifier from pointer target type
talloc_free(bsc_cmd->id);
^
/usr/include/talloc.h:227:5: note: expected ‘void *’ but argument is of type ‘const char
*’
int _talloc_free(void *ptr, const char *location);
^
../../../src/osmo-bsc_nat/bsc_nat_ctrl.c:266:16: warning: passing argument 1 of
‘_talloc_free’ discards ‘const’ qualifier from pointer target type
talloc_free(bsc_cmd->variable);
^
/usr/include/talloc.h:227:5: note: expected ‘void *’ but argument is of type ‘const char
*’
int _talloc_free(void *ptr, const char *location);
^
../../../src/osmo-bsc_nat/bsc_nat_ctrl.c: In function ‘extract_bsc_cfg_variable’:
../../../src/osmo-bsc_nat/bsc_nat_ctrl.c:323:31: warning: passing argument 1 of
‘extract_bsc_nr_variable’ discards ‘const’ qualifier from pointer target type
if (!extract_bsc_nr_variable(cmd->variable, &nr, bsc_variable)) {
^
../../../src/osmo-bsc_nat/bsc_nat_ctrl.c:198:12: note: expected ‘char *’ but argument is
of type ‘const char *’
static int extract_bsc_nr_variable(char *variable, unsigned int *nr, char
**bsc_variable)
^
--
Max Suraev <msuraev(a)sysmocom.de> http://www.sysmocom.de/
=======================================================================
* sysmocom - systems for mobile communications GmbH
* Alt-Moabit 93
* 10559 Berlin, Germany
* Sitz / Registered office: Berlin, HRB 134158 B
* Geschaeftsfuehrer / Managing Director: Harald Welte
19 Oct
19 Oct
2:02 p.m.
On Tue, Oct 18, 2016 at 12:46:01PM +0200, Max wrote:
Hi.
I think it's better to fix those warnings instead of rolling back
commit.
I totally agree in principle. The point is, this commit added a *huge* amount
of warnings = a large amount of time loss to every compilation cycle. I would
prefer if you *first* fix the fallout and *then* apply things like this.
This is such that I would invest time in somehow recording what warnings I have
already seen and only showing the changes in warnings; but the time is better
spent by not having such a large amount of warnings, and not forget about them.
So, where is your warning squelching patch right now, please?
Thanks for your understanding...
~Neels
3:03 p.m.
I've made ticket for this: https://projects.osmocom.org/issues/1829
On 10/19/2016 02:02 PM, Neels Hofmeyr wrote:
On Tue, Oct 18, 2016 at 12:46:01PM +0200, Max wrote:
--
Max Suraev <msuraev(a)sysmocom.de> http://www.sysmocom.de/
=======================================================================
* sysmocom - systems for mobile communications GmbH
* Alt-Moabit 93
* 10559 Berlin, Germany
* Sitz / Registered office: Berlin, HRB 134158 B
* Geschaeftsfuehrer / Managing Director: Harald Welte
Hi.
I think it's better to fix those warnings instead of rolling back
commit.
I totally agree in principle. The point is, this commit added a *huge*
amount
of warnings = a large amount of time loss to every compilation cycle. I would
prefer if you *first* fix the fallout and *then* apply things like this.
This is such that I would invest time in somehow recording what warnings I have
already seen and only showing the changes in warnings; but the time is better
spent by not having such a large amount of warnings, and not forget about them.
So, where is your warning squelching patch right now, please?
Thanks for your understanding...
~Neels
20 Oct
20 Oct
1:35 p.m.
On Wed, Oct 19, 2016 at 03:03:04PM +0200, Max wrote:
I've made ticket for this:
https://projects.osmocom.org/issues/1829
So you would like to first write unit tests and so on until these warnings will
go away. How urgently will you pursue this? Are you actively working on it?
I will locally revert the constify patch to avoid the warnings flood. Which
adds the need for rebase for every pull in libosmocore. I have some of those
already related to the 3G branches: it's cumbersome.
Can we please commit the revert on master and re-commit the constify when
OS#1829 is resolved?
~Neels
1:41 p.m.
There are other things like DTX and PTCCH which prevent me from working
on this right now. As I've explained in the ticket I don't think that
reverting this commit is urgent but feel free to add your comments to
the ticket, send revert patch to gerrit, raise ticket urgency with
Harald etc. etc.
On 10/20/2016 01:35 PM, Neels Hofmeyr wrote:
On Wed, Oct 19, 2016 at 03:03:04PM +0200, Max wrote:
--
Max Suraev <msuraev(a)sysmocom.de> http://www.sysmocom.de/
=======================================================================
* sysmocom - systems for mobile communications GmbH
* Alt-Moabit 93
* 10559 Berlin, Germany
* Sitz / Registered office: Berlin, HRB 134158 B
* Geschaeftsfuehrer / Managing Director: Harald Welte
I've made ticket for this:
https://projects.osmocom.org/issues/1829
So you would like to first write unit
tests and so on until these warnings will
go away. How urgently will you pursue this? Are you actively working on it?
I will locally revert the constify patch to avoid the warnings flood. Which
adds the need for rebase for every pull in libosmocore. I have some of those
already related to the 3G branches: it's cumbersome.
Can we please commit the revert on master and re-commit the constify when
OS#1829 is resolved?
~Neels
3167
days inactive
3169
days old
5 comments
2 participants
participants (2)
-
Max -
Neels Hofmeyr