Presumably after this libosmocore commit, I get scores of new warnings (see below) when compiling openbsc:
commit ed9d6da5df98538adc70aa03cb569eb9505d04b6 Author: Max msuraev@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) ^
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@sysmocom.de AuthorDate: Tue Oct 11 15:20:28 2016 +0200
Constify ctrl_cmd struct fields where appropriateSome 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) ^
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
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:
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
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
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:
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
-
Max -
Neels Hofmeyr