Change in libosmocore[master]: logging: use LOGL_NOTICE when no loglevel is set
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/17604 ) Change subject: logging: use LOGL_NOTICE when no loglevel is set .. Patch Set 8: Code-Review+2 -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/17604 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: Ib9e180261505062505fc4605a98023910f76cde6 Gerrit-Change-Number: 17604 Gerrit-PatchSet: 8 Gerrit-Owner: dexter Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: dexter Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: pespin Gerrit-Comment-Date: Tue, 12 May 2020 18:09:20 + Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Change in libosmocore[master]: logging: use LOGL_NOTICE when no loglevel is set
laforge has submitted this change. (
https://gerrit.osmocom.org/c/libosmocore/+/17604 )
Change subject: logging: use LOGL_NOTICE when no loglevel is set
..
logging: use LOGL_NOTICE when no loglevel is set
when the API user of libosmocores logging infrastructure does not set a
pre-defined logging level in struct log_info_cat, the result would be an
invalid logging level. In order to avoid problems with that and to spare
all the additional .loglevel = LOGL_NOTICE (API users are advised to use
LOGL_NOTICE as default) lines in the user code lets check the logging
level on startup and set LOGL_NOTICE if there is no logging level set.
Change-Id: Ib9e180261505062505fc4605a98023910f76cde6
Related: OS#2577
---
M src/logging.c
1 file changed, 17 insertions(+), 9 deletions(-)
Approvals:
Jenkins Builder: Verified
fixeria: Looks good to me, approved
laforge: Looks good to me, approved
diff --git a/src/logging.c b/src/logging.c
index 4d6224d..4aaf515 100644
--- a/src/logging.c
+++ b/src/logging.c
@@ -1026,6 +1026,7 @@
int log_init(const struct log_info *inf, void *ctx)
{
int i;
+ struct log_info_cat *cat_ptr;
tall_log_ctx = talloc_named_const(ctx, 1, "logging");
if (!tall_log_ctx)
@@ -1043,29 +1044,36 @@
osmo_log_info->num_cat += inf->num_cat;
}
- osmo_log_info->cat = talloc_zero_array(osmo_log_info,
- struct log_info_cat,
- osmo_log_info->num_cat);
- if (!osmo_log_info->cat) {
+ cat_ptr = talloc_zero_array(osmo_log_info, struct log_info_cat,
+ osmo_log_info->num_cat);
+ if (!cat_ptr) {
talloc_free(osmo_log_info);
osmo_log_info = NULL;
return -ENOMEM;
}
- if (inf) { /* copy over the user part */
+ /* copy over the user part and sanitize loglevel */
+ if (inf) {
for (i = 0; i < inf->num_cat; i++) {
- memcpy((struct log_info_cat *) _log_info->cat[i],
- >cat[i], sizeof(struct log_info_cat));
+ memcpy(_ptr[i], >cat[i],
+ sizeof(struct log_info_cat));
+
+ /* Make sure that the loglevel is set to NOTICE in case
+* no loglevel has been preset. */
+ if (!cat_ptr[i].loglevel) {
+ cat_ptr[i].loglevel = LOGL_NOTICE;
+ }
}
}
/* copy over the library part */
for (i = 0; i < ARRAY_SIZE(internal_cat); i++) {
unsigned int cn = osmo_log_info->num_cat_user + i;
- memcpy((struct log_info_cat *) _log_info->cat[cn],
- _cat[i], sizeof(struct log_info_cat));
+ memcpy(_ptr[cn], _cat[i], sizeof(struct
log_info_cat));
}
+ osmo_log_info->cat = cat_ptr;
+
return 0;
}
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/17604
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: Ib9e180261505062505fc4605a98023910f76cde6
Gerrit-Change-Number: 17604
Gerrit-PatchSet: 8
Gerrit-Owner: dexter
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: dexter
Gerrit-Reviewer: fixeria
Gerrit-Reviewer: laforge
Gerrit-Reviewer: pespin
Gerrit-MessageType: merged
Change in libosmocore[master]: logging: use LOGL_NOTICE when no loglevel is set
fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/17604 ) Change subject: logging: use LOGL_NOTICE when no loglevel is set .. Patch Set 8: Code-Review+2 -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/17604 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: Ib9e180261505062505fc4605a98023910f76cde6 Gerrit-Change-Number: 17604 Gerrit-PatchSet: 8 Gerrit-Owner: dexter Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: dexter Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: pespin Gerrit-Comment-Date: Tue, 12 May 2020 16:15:59 + Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Change in libosmocore[master]: logging: use LOGL_NOTICE when no loglevel is set
Hello fixeria, pespin, laforge, Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/libosmocore/+/17604 to look at the new patch set (#8). Change subject: logging: use LOGL_NOTICE when no loglevel is set .. logging: use LOGL_NOTICE when no loglevel is set when the API user of libosmocores logging infrastructure does not set a pre-defined logging level in struct log_info_cat, the result would be an invalid logging level. In order to avoid problems with that and to spare all the additional .loglevel = LOGL_NOTICE (API users are advised to use LOGL_NOTICE as default) lines in the user code lets check the logging level on startup and set LOGL_NOTICE if there is no logging level set. Change-Id: Ib9e180261505062505fc4605a98023910f76cde6 Related: OS#2577 --- M src/logging.c 1 file changed, 17 insertions(+), 9 deletions(-) git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/04/17604/8 -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/17604 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: Ib9e180261505062505fc4605a98023910f76cde6 Gerrit-Change-Number: 17604 Gerrit-PatchSet: 8 Gerrit-Owner: dexter Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: dexter Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: pespin Gerrit-MessageType: newpatchset
Change in libosmocore[master]: logging: use LOGL_NOTICE when no loglevel is set
fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/17604 ) Change subject: logging: use LOGL_NOTICE when no loglevel is set .. Patch Set 7: Code-Review+1 (2 comments) https://gerrit.osmocom.org/c/libosmocore/+/17604/7/src/logging.c File src/logging.c: https://gerrit.osmocom.org/c/libosmocore/+/17604/7/src/logging.c@1031 PS7, Line 1031: tall_log_ctx = talloc_named_const(ctx, 1, "logging"); > Regarding fixeria's comment on free(): Correct. […] Rather OSMO_ASSERT(tall_log_ctx == NULL)... https://gerrit.osmocom.org/c/libosmocore/+/17604/7/src/logging.c@1048 PS7, Line 1048: Alignment is broken :/ -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/17604 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: Ib9e180261505062505fc4605a98023910f76cde6 Gerrit-Change-Number: 17604 Gerrit-PatchSet: 7 Gerrit-Owner: dexter Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: dexter Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: pespin Gerrit-Comment-Date: Tue, 12 May 2020 14:29:03 + Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: pespin Gerrit-MessageType: comment
Change in libosmocore[master]: logging: use LOGL_NOTICE when no loglevel is set
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/17604 ) Change subject: logging: use LOGL_NOTICE when no loglevel is set .. Patch Set 7: Code-Review+1 (1 comment) https://gerrit.osmocom.org/c/libosmocore/+/17604/7/src/logging.c File src/logging.c: https://gerrit.osmocom.org/c/libosmocore/+/17604/7/src/logging.c@1031 PS7, Line 1031: tall_log_ctx = talloc_named_const(ctx, 1, "logging"); Regarding fixeria's comment on free(): Correct. But then please submit a new patch freeing tall_log_ctx here before assigning it, in case log_init is called more than once. -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/17604 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: Ib9e180261505062505fc4605a98023910f76cde6 Gerrit-Change-Number: 17604 Gerrit-PatchSet: 7 Gerrit-Owner: dexter Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: dexter Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: pespin Gerrit-Comment-Date: Tue, 12 May 2020 14:25:15 + Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Change in libosmocore[master]: logging: use LOGL_NOTICE when no loglevel is set
Hello fixeria, pespin, laforge, Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/libosmocore/+/17604 to look at the new patch set (#7). Change subject: logging: use LOGL_NOTICE when no loglevel is set .. logging: use LOGL_NOTICE when no loglevel is set when the API user of libosmocores logging infrastructure does not set a pre-defined logging level in struct log_info_cat, the result would be an invalid logging level. In order to avoid problems with that and to spare all the additional .loglevel = LOGL_NOTICE (API users are advised to use LOGL_NOTICE as default) lines in the user code lets check the logging level on startup and set LOGL_NOTICE if there is no logging level set. Change-Id: Ib9e180261505062505fc4605a98023910f76cde6 Related: OS#2577 --- M src/logging.c 1 file changed, 16 insertions(+), 7 deletions(-) git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/04/17604/7 -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/17604 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: Ib9e180261505062505fc4605a98023910f76cde6 Gerrit-Change-Number: 17604 Gerrit-PatchSet: 7 Gerrit-Owner: dexter Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: dexter Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: pespin Gerrit-MessageType: newpatchset
Change in libosmocore[master]: logging: use LOGL_NOTICE when not loglevel is set
fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/17604 ) Change subject: logging: use LOGL_NOTICE when not loglevel is set .. Patch Set 6: (2 comments) https://gerrit.osmocom.org/c/libosmocore/+/17604/6/src/logging.c File src/logging.c: https://gerrit.osmocom.org/c/libosmocore/+/17604/6/src/logging.c@1047 PS6, Line 1047: cat_temp_ptr = talloc_zero_array(osmo_log_info, > please change name, "temp" seems to say the variable is function-scoped and > should be freed before e […] I can be 'cat_ptr' or even just 'cat'. https://gerrit.osmocom.org/c/libosmocore/+/17604/6/src/logging.c@1077 PS6, Line 1077: osmo_log_info->cat = cat_temp_ptr; > if osmo_log_info->cat is not NULL, talloc_free it before assigning it, to > make sure we don't leak if […] osmo_log_info is allocated by this function using talloc_zero(), and this is the only place where it's actually initialized. Thus no need to check not free() anything. -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/17604 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: Ib9e180261505062505fc4605a98023910f76cde6 Gerrit-Change-Number: 17604 Gerrit-PatchSet: 6 Gerrit-Owner: dexter Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: dexter Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: pespin Gerrit-Comment-Date: Tue, 12 May 2020 13:30:43 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: pespin Gerrit-MessageType: comment
Change in libosmocore[master]: logging: use LOGL_NOTICE when not loglevel is set
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/17604 ) Change subject: logging: use LOGL_NOTICE when not loglevel is set .. Patch Set 6: Code-Review-1 (4 comments) https://gerrit.osmocom.org/c/libosmocore/+/17604/6//COMMIT_MSG Commit Message: https://gerrit.osmocom.org/c/libosmocore/+/17604/6//COMMIT_MSG@7 PS6, Line 7: logging: use LOGL_NOTICE when not loglevel is set "no loglevel" https://gerrit.osmocom.org/c/libosmocore/+/17604/6/src/logging.c File src/logging.c: https://gerrit.osmocom.org/c/libosmocore/+/17604/6/src/logging.c@1047 PS6, Line 1047: cat_temp_ptr = talloc_zero_array(osmo_log_info, please change name, "temp" seems to say the variable is function-scoped and should be freed before exiting, and this is not the case, since this struct is probably gonna be left there for the entire run of the process. https://gerrit.osmocom.org/c/libosmocore/+/17604/6/src/logging.c@1074 PS6, Line 1074:sizeof(struct log_info_cat)); you can move this to previous line, there's still space for it. https://gerrit.osmocom.org/c/libosmocore/+/17604/6/src/logging.c@1077 PS6, Line 1077: osmo_log_info->cat = cat_temp_ptr; if osmo_log_info->cat is not NULL, talloc_free it before assigning it, to make sure we don't leak if somebody calls log_init twice. -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/17604 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: Ib9e180261505062505fc4605a98023910f76cde6 Gerrit-Change-Number: 17604 Gerrit-PatchSet: 6 Gerrit-Owner: dexter Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: dexter Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: pespin Gerrit-Comment-Date: Tue, 12 May 2020 11:49:21 + Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Change in libosmocore[master]: logging: use LOGL_NOTICE when not loglevel is set
Hello fixeria, laforge, Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/libosmocore/+/17604 to look at the new patch set (#6). Change subject: logging: use LOGL_NOTICE when not loglevel is set .. logging: use LOGL_NOTICE when not loglevel is set when the API user of libosmocores logging infrastructure does not set a pre-defined logging level in struct log_info_cat, the result would be an invalid logging level. In order to avoid problems with that and to spare all the additional .loglevel = LOGL_NOTICE (API users are advised to use LOGL_NOTICE as default) lines in the user code lets check the logging level on startup and set LOGL_NOTICE if there is no logging level set. Change-Id: Ib9e180261505062505fc4605a98023910f76cde6 Related: OS#2577 --- M src/logging.c 1 file changed, 17 insertions(+), 7 deletions(-) git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/04/17604/6 -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/17604 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: Ib9e180261505062505fc4605a98023910f76cde6 Gerrit-Change-Number: 17604 Gerrit-PatchSet: 6 Gerrit-Owner: dexter Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: dexter Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-MessageType: newpatchset
Change in libosmocore[master]: logging: use LOGL_NOTICE when not loglevel is set
Hello fixeria, laforge, Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/libosmocore/+/17604 to look at the new patch set (#5). Change subject: logging: use LOGL_NOTICE when not loglevel is set .. logging: use LOGL_NOTICE when not loglevel is set when the API user of libosmocores logging infrastructure does not set a pre-defined logging level in struct log_info_cat, the result would be an invalid logging level. In order to avoid problems with that and to spare all the additional .loglevel = LOGL_NOTICE (API users are advised to use LOGL_NOTICE as default) lines in the user code lets check the logging level on startup and set LOGL_NOTICE if there is no logging level set. Change-Id: Ib9e180261505062505fc4605a98023910f76cde6 Related: OS#2577 --- M src/logging.c 1 file changed, 16 insertions(+), 6 deletions(-) git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/04/17604/5 -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/17604 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: Ib9e180261505062505fc4605a98023910f76cde6 Gerrit-Change-Number: 17604 Gerrit-PatchSet: 5 Gerrit-Owner: dexter Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: dexter Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-MessageType: newpatchset
Change in libosmocore[master]: logging: use LOGL_NOTICE when not loglevel is set
fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/17604 ) Change subject: logging: use LOGL_NOTICE when not loglevel is set .. Patch Set 4: Code-Review+1 (1 comment) https://gerrit.osmocom.org/c/libosmocore/+/17604/4/src/logging.c File src/logging.c: https://gerrit.osmocom.org/c/libosmocore/+/17604/4/src/logging.c@1073 PS4, Line 1073: (struct log_info_cat *) This type cast is not needed anymore (because the pointer is not const now). -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/17604 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: Ib9e180261505062505fc4605a98023910f76cde6 Gerrit-Change-Number: 17604 Gerrit-PatchSet: 4 Gerrit-Owner: dexter Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: dexter Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Comment-Date: Mon, 20 Apr 2020 13:27:28 + Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Change in libosmocore[master]: logging: use LOGL_NOTICE when not loglevel is set
Hello laforge, Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/libosmocore/+/17604 to look at the new patch set (#4). Change subject: logging: use LOGL_NOTICE when not loglevel is set .. logging: use LOGL_NOTICE when not loglevel is set when the API user of libosmocores logging infrastructure does not set a pre-defined logging level in struct log_info_cat, the result would be an invalid logging level. In order to avoid problems with that and to spare all the additional .loglevel = LOGL_NOTICE (API users are advised to use LOGL_NOTICE as default) lines in the user code lets check the logging level on startup and set LOGL_NOTICE if there is no logging level set. Change-Id: Ib9e180261505062505fc4605a98023910f76cde6 Related: OS#2577 --- M src/logging.c 1 file changed, 15 insertions(+), 5 deletions(-) git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/04/17604/4 -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/17604 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: Ib9e180261505062505fc4605a98023910f76cde6 Gerrit-Change-Number: 17604 Gerrit-PatchSet: 4 Gerrit-Owner: dexter Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: dexter Gerrit-Reviewer: laforge Gerrit-CC: fixeria Gerrit-MessageType: newpatchset
Change in libosmocore[master]: logging: use LOGL_NOTICE when not loglevel is set
fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/17604 ) Change subject: logging: use LOGL_NOTICE when not loglevel is set .. Patch Set 3: (1 comment) https://gerrit.osmocom.org/c/libosmocore/+/17604/2/src/logging.c File src/logging.c: https://gerrit.osmocom.org/c/libosmocore/+/17604/2/src/logging.c@1064 PS2, Line 1064: *(uint8_t*) (& > I think the only proper way t osolve it is to copy the entire 'const' array > of 'cat' into dynamically allocated memory, and then make modifications there. > we should dynamically allocate memory for 'cat' here in this function, then > copy over the contents of 'cat' from osmo_log_info [...] Right, that's exactly how we do this. > [...] and make the changes there. When done we assign osmo_log_info->cat with > the location of the copy we just created. Is this correct? Vice versa. In the current code (with this patch applied) we allocate memory and immediately assign it to osmo_log_info->cat (line 1046), and then we make changes by casting const pointers to non-const. I am fine with proposed patch, but ideally we should use a temporary non-const pointer to make changes, and then assign it to osmo_log_info->cat (as was proposed in a comment near line 1046). -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/17604 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: Ib9e180261505062505fc4605a98023910f76cde6 Gerrit-Change-Number: 17604 Gerrit-PatchSet: 3 Gerrit-Owner: dexter Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: dexter Gerrit-Reviewer: laforge Gerrit-CC: fixeria Gerrit-Comment-Date: Sun, 12 Apr 2020 09:10:45 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: dexter Comment-In-Reply-To: fixeria Comment-In-Reply-To: laforge Gerrit-MessageType: comment
Change in libosmocore[master]: logging: use LOGL_NOTICE when not loglevel is set
dexter has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/17604 ) Change subject: logging: use LOGL_NOTICE when not loglevel is set .. Patch Set 3: (1 comment) There are some things that I do not understand, see comment in src/logging.c, PS2, line 1064 https://gerrit.osmocom.org/c/libosmocore/+/17604/2/src/logging.c File src/logging.c: https://gerrit.osmocom.org/c/libosmocore/+/17604/2/src/logging.c@1064 PS2, Line 1064: *(uint8_t*) (& > oh, this is actually a problem. I think many of our programs allocate the > arrays as 'const', i.e. […] I have difficulties to understand this. To my understanding we should dynamically allocate memory for 'cat' here in this function, then copy over the contents of 'cat' from osmo_log_info and make the changes there. When done we assign osmo_log_info->cat with the location of the copy we just created. Is this correct? The function gets a const struct log_info *inf, in logging.h I find const struct log_info_cat *cat in struct log_info. Is it even possible to replace osmo_log_info->cat with a now memory location then? -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/17604 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: Ib9e180261505062505fc4605a98023910f76cde6 Gerrit-Change-Number: 17604 Gerrit-PatchSet: 3 Gerrit-Owner: dexter Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: dexter Gerrit-Reviewer: laforge Gerrit-CC: fixeria Gerrit-Comment-Date: Thu, 09 Apr 2020 09:46:30 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: fixeria Comment-In-Reply-To: laforge Gerrit-MessageType: comment
Change in libosmocore[master]: logging: use LOGL_NOTICE when not loglevel is set
dexter has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/17604 ) Change subject: logging: use LOGL_NOTICE when not loglevel is set .. Patch Set 3: No change so far, just rebased - please ignore. -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/17604 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: Ib9e180261505062505fc4605a98023910f76cde6 Gerrit-Change-Number: 17604 Gerrit-PatchSet: 3 Gerrit-Owner: dexter Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: dexter Gerrit-Reviewer: laforge Gerrit-CC: fixeria Gerrit-Comment-Date: Thu, 09 Apr 2020 09:29:24 + Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment
Change in libosmocore[master]: logging: use LOGL_NOTICE when not loglevel is set
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/17604 ) Change subject: logging: use LOGL_NOTICE when not loglevel is set .. Patch Set 2: Code-Review-1 (1 comment) https://gerrit.osmocom.org/c/libosmocore/+/17604/2/src/logging.c File src/logging.c: https://gerrit.osmocom.org/c/libosmocore/+/17604/2/src/logging.c@1064 PS2, Line 1064: *(uint8_t*) (& > AFAIU, this is needed because struct log_info_cat *cat is marked as const. […] oh, this is actually a problem. I think many of our programs allocate the arrays as 'const', i.e. they are in 'rodata' and hence memory that cannot be modified. We cannot change the function notation as it will result in breakage all over the place. I think the only proper way t osolve it is to copy the entire 'const' array of 'cat' into dynamically allocated memory, and then make modifications there. -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/17604 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: Ib9e180261505062505fc4605a98023910f76cde6 Gerrit-Change-Number: 17604 Gerrit-PatchSet: 2 Gerrit-Owner: dexter Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: dexter Gerrit-Reviewer: laforge Gerrit-CC: fixeria Gerrit-Comment-Date: Wed, 01 Apr 2020 09:48:35 + Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: fixeria Gerrit-MessageType: comment
Change in libosmocore[master]: logging: use LOGL_NOTICE when not loglevel is set
fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/17604 ) Change subject: logging: use LOGL_NOTICE when not loglevel is set .. Patch Set 2: (2 comments) https://gerrit.osmocom.org/c/libosmocore/+/17604/2/src/logging.c File src/logging.c: https://gerrit.osmocom.org/c/libosmocore/+/17604/2/src/logging.c@1046 PS2, Line 1046: osmo_log_info->cat As an alternative solution (preferred), we can use a temporary pointer here, and then assign its value to osmo_log_info->cat: struct log_info_cat *cat = talloc_zero_array(...); // Do whatever we need to do ... osmo_log_info->cat = cat; https://gerrit.osmocom.org/c/libosmocore/+/17604/2/src/logging.c@1064 PS2, Line 1064: *(uint8_t*) (& AFAIU, this is needed because struct log_info_cat *cat is marked as const. Can we just drop this qualifier? -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/17604 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: Ib9e180261505062505fc4605a98023910f76cde6 Gerrit-Change-Number: 17604 Gerrit-PatchSet: 2 Gerrit-Owner: dexter Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: dexter Gerrit-Reviewer: laforge Gerrit-CC: fixeria Gerrit-Comment-Date: Tue, 31 Mar 2020 08:02:28 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
Change in libosmocore[master]: logging: use LOGL_NOTICE when not loglevel is set
dexter has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/17604 ) Change subject: logging: use LOGL_NOTICE when not loglevel is set .. Patch Set 2: (1 comment) https://gerrit.osmocom.org/c/libosmocore/+/17604/1/src/logging.c File src/logging.c: https://gerrit.osmocom.org/c/libosmocore/+/17604/1/src/logging.c@1065 PS1, Line 1065: LOGL_NOTICE; > cosmetic: no need to break the line. […] Done -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/17604 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: Ib9e180261505062505fc4605a98023910f76cde6 Gerrit-Change-Number: 17604 Gerrit-PatchSet: 2 Gerrit-Owner: dexter Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: dexter Gerrit-Reviewer: laforge Gerrit-Comment-Date: Mon, 30 Mar 2020 20:02:56 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: laforge Gerrit-MessageType: comment
Change in libosmocore[master]: logging: use LOGL_NOTICE when not loglevel is set
Hello laforge, Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/libosmocore/+/17604 to look at the new patch set (#2). Change subject: logging: use LOGL_NOTICE when not loglevel is set .. logging: use LOGL_NOTICE when not loglevel is set when the API user of libosmocores logging infrastructure does not set a pre-defined logging level in struct log_info_cat, the result would be an invalid logging level. In order to avoid problems with that and to spare all the additional .loglevel = LOGL_NOTICE (API users are advised to use LOGL_NOTICE as default) lines in the user code lets check the logging level on startup and set LOGL_NOTICE if there is no logging level set. Change-Id: Ib9e180261505062505fc4605a98023910f76cde6 Related: OS#2577 --- M src/logging.c 1 file changed, 9 insertions(+), 1 deletion(-) git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/04/17604/2 -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/17604 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: Ib9e180261505062505fc4605a98023910f76cde6 Gerrit-Change-Number: 17604 Gerrit-PatchSet: 2 Gerrit-Owner: dexter Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge Gerrit-MessageType: newpatchset
Change in libosmocore[master]: logging: use LOGL_NOTICE when not loglevel is set
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/17604 ) Change subject: logging: use LOGL_NOTICE when not loglevel is set .. Patch Set 1: Code-Review+1 (1 comment) https://gerrit.osmocom.org/c/libosmocore/+/17604/1/src/logging.c File src/logging.c: https://gerrit.osmocom.org/c/libosmocore/+/17604/1/src/logging.c@1065 PS1, Line 1065: LOGL_NOTICE; cosmetic: no need to break the line. If you want to break it, please use curly braces around the block. -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/17604 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: Ib9e180261505062505fc4605a98023910f76cde6 Gerrit-Change-Number: 17604 Gerrit-PatchSet: 1 Gerrit-Owner: dexter Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge Gerrit-Comment-Date: Sun, 29 Mar 2020 14:29:50 + Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Change in libosmocore[master]: logging: use LOGL_NOTICE when not loglevel is set
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/17604 ) Change subject: logging: use LOGL_NOTICE when not loglevel is set .. Patch Set 1: isn't it possible to cover this in libosmocore instead of having to implement it in each and every application? -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/17604 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: Ib9e180261505062505fc4605a98023910f76cde6 Gerrit-Change-Number: 17604 Gerrit-PatchSet: 1 Gerrit-Owner: dexter Gerrit-Reviewer: Jenkins Builder Gerrit-CC: laforge Gerrit-Comment-Date: Fri, 27 Mar 2020 17:29:55 + Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment
Change in libosmocore[master]: logging: use LOGL_NOTICE when not loglevel is set
dexter has uploaded this change for review. (
https://gerrit.osmocom.org/c/libosmocore/+/17604 )
Change subject: logging: use LOGL_NOTICE when not loglevel is set
..
logging: use LOGL_NOTICE when not loglevel is set
when the API user of libosmocores logging infrastructure does not set a
pre-defined logging level in struct log_info_cat, the result would be an
invalid logging level. In order to avoid problems with that and to spare
all the additional .loglevel = LOGL_NOTICE (API users are advised to use
LOGL_NOTICE as default) lines in the user code lets check the logging
level on startup and set LOGL_NOTICE if there is no logging level set.
Change-Id: Ib9e180261505062505fc4605a98023910f76cde6
Related: OS#2577
---
M src/logging.c
1 file changed, 8 insertions(+), 1 deletion(-)
git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/04/17604/1
diff --git a/src/logging.c b/src/logging.c
index 4d6224d..d67181c 100644
--- a/src/logging.c
+++ b/src/logging.c
@@ -1052,10 +1052,17 @@
return -ENOMEM;
}
- if (inf) { /* copy over the user part */
+ /* copy over the user part and sanitize loglevel */
+ if (inf) {
for (i = 0; i < inf->num_cat; i++) {
memcpy((struct log_info_cat *) _log_info->cat[i],
>cat[i], sizeof(struct log_info_cat));
+
+ /* Make sure that the loglevel is set to NOTICE in case
+* no loglevel has been preset. */
+ if (!osmo_log_info->cat[i].loglevel)
+ *(uint8_t*) (_log_info->cat[i].loglevel) =
+ LOGL_NOTICE;
}
}
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/17604
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: Ib9e180261505062505fc4605a98023910f76cde6
Gerrit-Change-Number: 17604
Gerrit-PatchSet: 1
Gerrit-Owner: dexter
Gerrit-MessageType: newchange
