| Message ID | 20200920090929.8321-1-gert@greenie.muc.de |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [Openvpn-devel] Fix line number reporting on config file errors after <inline> segments | expand |
Hi, On 20/09/2020 11:09, Gert Doering wrote: > <inline> segments neglected to increment the "current line number > in config file" variable (line_num), so after the first <inline>, > errors reported have the wrong line number. > > Fix by introducing an extra argument to the check_inline*() / > read_inline_file() call chain: "so many lines in the inline block". > > Trac: #1325 > > Signed-off-by: Gert Doering <gert@greenie.muc.de> > --- > src/openvpn/options.c | 29 ++++++++++++++++++++--------- > 1 file changed, 20 insertions(+), 9 deletions(-) > > diff --git a/src/openvpn/options.c b/src/openvpn/options.c > index f370a2bb..af517bab 100644 > --- a/src/openvpn/options.c > +++ b/src/openvpn/options.c > @@ -4634,7 +4634,8 @@ in_src_get(const struct in_src *is, char *line, const int size) > } > > static char * > -read_inline_file(struct in_src *is, const char *close_tag, struct gc_arena *gc) > +read_inline_file(struct in_src *is, const char *close_tag, > + int *num_lines, struct gc_arena *gc) > { > char line[OPTION_LINE_SIZE]; > struct buffer buf = alloc_buf(8*OPTION_LINE_SIZE); > @@ -4643,6 +4644,7 @@ read_inline_file(struct in_src *is, const char *close_tag, struct gc_arena *gc) > > while (in_src_get(is, line, sizeof(line))) > { > + (*num_lines)++; > char *line_ptr = line; > /* Remove leading spaces */ > while (isspace(*line_ptr)) > @@ -4677,9 +4679,11 @@ read_inline_file(struct in_src *is, const char *close_tag, struct gc_arena *gc) > } > > static bool > -check_inline_file(struct in_src *is, char *p[], struct gc_arena *gc) > +check_inline_file(struct in_src *is, char *p[], > + int *num_lines, struct gc_arena *gc) Sorry for not chiming in earlier, but honestly I believe your other option would be "cleaner". The other option being "return int instead of bool, where the returned value is the number of lines of the inline'd material. If 0, we know there was no inline'd material, thus we can treat that result as "not inline'd". Anyway, this is just my opinion. If you believe this is a better approach, I am fine with it too. > { > bool is_inline = false; > + *num_lines = 0; > > if (p[0] && !p[1]) > { > @@ -4692,7 +4696,7 @@ check_inline_file(struct in_src *is, char *p[], struct gc_arena *gc) > p[0] = string_alloc(arg + 1, gc); > close_tag = alloc_buf(strlen(p[0]) + 4); > buf_printf(&close_tag, "</%s>", p[0]); > - p[1] = read_inline_file(is, BSTR(&close_tag), gc); > + p[1] = read_inline_file(is, BSTR(&close_tag), num_lines, gc); am I wrong or here we should add 1 to num_lines to include the opening tag that was parsed *before* entering read_inline_file()? Regards,
Hi, On Mon, Sep 21, 2020 at 09:22:38AM +0200, Antonio Quartulli wrote: > Sorry for not chiming in earlier, but honestly I believe your other > option would be "cleaner". The other option being "return int instead of > bool, where the returned value is the number of lines of the inline'd > material. > > If 0, we know there was no inline'd material, thus we can treat that > result as "not inline'd". We could do that, indeed. I was torn between both variants - this one is a bit more intrusive (because it adds a new argument to so many functions), but on the other hand, the bool stays a bool and is not magically overloaded - which would make the call to add_option() either "even longer", or if you rely on magic "we just pass the integer to a bool-expecting function" much harder to understand. > Anyway, this is just my opinion. If you believe this is a better > approach, I am fine with it too. I wasn't sure. In the end, this one didn't feel so bad, so this is what we have. Maybe I'll do the other one as well, for fun, and to compare :-) > > @@ -4692,7 +4696,7 @@ check_inline_file(struct in_src *is, char *p[], struct gc_arena *gc) > > p[0] = string_alloc(arg + 1, gc); > > close_tag = alloc_buf(strlen(p[0]) + 4); > > buf_printf(&close_tag, "</%s>", p[0]); > > - p[1] = read_inline_file(is, BSTR(&close_tag), gc); > > + p[1] = read_inline_file(is, BSTR(&close_tag), num_lines, gc); > > am I wrong or here we should add 1 to num_lines to include the opening > tag that was parsed *before* entering read_inline_file()? The opening tag is parsed as "regular" line in read_config_file(), and as such, the "++line_num" in there applies. (I have tested this :-) - nothing more embarrassing than a patch to fix line numbers actually still printing wrong line numbers) gert
Hi, On 21/09/2020 09:50, Gert Doering wrote: > Hi, > > On Mon, Sep 21, 2020 at 09:22:38AM +0200, Antonio Quartulli wrote: >> Sorry for not chiming in earlier, but honestly I believe your other >> option would be "cleaner". The other option being "return int instead of >> bool, where the returned value is the number of lines of the inline'd >> material. >> >> If 0, we know there was no inline'd material, thus we can treat that >> result as "not inline'd". > > We could do that, indeed. I was torn between both variants - this one > is a bit more intrusive (because it adds a new argument to so many > functions), but on the other hand, the bool stays a bool and is not > magically overloaded - which would make the call to add_option() > either "even longer", or if you rely on magic "we just pass the integer > to a bool-expecting function" much harder to understand. > >> Anyway, this is just my opinion. If you believe this is a better >> approach, I am fine with it too. > > I wasn't sure. In the end, this one didn't feel so bad, so this is > what we have. > > Maybe I'll do the other one as well, for fun, and to compare :-) > >>> @@ -4692,7 +4696,7 @@ check_inline_file(struct in_src *is, char *p[], struct gc_arena *gc) >>> p[0] = string_alloc(arg + 1, gc); >>> close_tag = alloc_buf(strlen(p[0]) + 4); >>> buf_printf(&close_tag, "</%s>", p[0]); >>> - p[1] = read_inline_file(is, BSTR(&close_tag), gc); >>> + p[1] = read_inline_file(is, BSTR(&close_tag), num_lines, gc); >> >> am I wrong or here we should add 1 to num_lines to include the opening >> tag that was parsed *before* entering read_inline_file()? > > The opening tag is parsed as "regular" line in read_config_file(), and as > such, the "++line_num" in there applies. > > (I have tested this :-) - nothing more embarrassing than a patch to fix > line numbers actually still printing wrong line numbers) > Ok, then for me this patch is good to go. FTR on IRC we discussed the possibility of using the "other approach". Gert may give it a try and decide if he wants to send that version to the ml as well. In any case, this patch gets my blessing: Acked-by: Antonio Quartulli <a@unstable.cc>
diff --git a/src/openvpn/options.c b/src/openvpn/options.c index f370a2bb..af517bab 100644 --- a/src/openvpn/options.c +++ b/src/openvpn/options.c @@ -4634,7 +4634,8 @@ in_src_get(const struct in_src *is, char *line, const int size) } static char * -read_inline_file(struct in_src *is, const char *close_tag, struct gc_arena *gc) +read_inline_file(struct in_src *is, const char *close_tag, + int *num_lines, struct gc_arena *gc) { char line[OPTION_LINE_SIZE]; struct buffer buf = alloc_buf(8*OPTION_LINE_SIZE); @@ -4643,6 +4644,7 @@ read_inline_file(struct in_src *is, const char *close_tag, struct gc_arena *gc) while (in_src_get(is, line, sizeof(line))) { + (*num_lines)++; char *line_ptr = line; /* Remove leading spaces */ while (isspace(*line_ptr)) @@ -4677,9 +4679,11 @@ read_inline_file(struct in_src *is, const char *close_tag, struct gc_arena *gc) } static bool -check_inline_file(struct in_src *is, char *p[], struct gc_arena *gc) +check_inline_file(struct in_src *is, char *p[], + int *num_lines, struct gc_arena *gc) { bool is_inline = false; + *num_lines = 0; if (p[0] && !p[1]) { @@ -4692,7 +4696,7 @@ check_inline_file(struct in_src *is, char *p[], struct gc_arena *gc) p[0] = string_alloc(arg + 1, gc); close_tag = alloc_buf(strlen(p[0]) + 4); buf_printf(&close_tag, "</%s>", p[0]); - p[1] = read_inline_file(is, BSTR(&close_tag), gc); + p[1] = read_inline_file(is, BSTR(&close_tag), num_lines, gc); p[2] = NULL; free_buf(&close_tag); is_inline = true; @@ -4702,22 +4706,23 @@ check_inline_file(struct in_src *is, char *p[], struct gc_arena *gc) } static bool -check_inline_file_via_fp(FILE *fp, char *p[], struct gc_arena *gc) +check_inline_file_via_fp(FILE *fp, char *p[], + int *num_lines, struct gc_arena *gc) { struct in_src is; is.type = IS_TYPE_FP; is.u.fp = fp; - return check_inline_file(&is, p, gc); + return check_inline_file(&is, p, num_lines, gc); } static bool check_inline_file_via_buf(struct buffer *multiline, char *p[], - struct gc_arena *gc) + int *num_lines, struct gc_arena *gc) { struct in_src is; is.type = IS_TYPE_BUF; is.u.multiline = multiline; - return check_inline_file(&is, p, gc); + return check_inline_file(&is, p, num_lines, gc); } static void @@ -4782,12 +4787,15 @@ read_config_file(struct options *options, if (parse_line(line + offset, p, SIZE(p)-1, file, line_num, msglevel, &options->gc)) { bool is_inline; + int lines; bypass_doubledash(&p[0]); - is_inline = check_inline_file_via_fp(fp, p, &options->gc); + is_inline = check_inline_file_via_fp(fp, p, + &lines, &options->gc); add_option(options, p, is_inline, file, line_num, level, msglevel, permission_mask, option_types_found, es); + line_num += lines; } } if (fp != stdin) @@ -4831,11 +4839,14 @@ read_config_string(const char *prefix, if (parse_line(line, p, SIZE(p)-1, prefix, line_num, msglevel, &options->gc)) { bool is_inline; + int lines; bypass_doubledash(&p[0]); - is_inline = check_inline_file_via_buf(&multiline, p, &options->gc); + is_inline = check_inline_file_via_buf(&multiline, p, + &lines, &options->gc); add_option(options, p, is_inline, prefix, line_num, 0, msglevel, permission_mask, option_types_found, es); + line_num += lines; } CLEAR(p); }
<inline> segments neglected to increment the "current line number in config file" variable (line_num), so after the first <inline>, errors reported have the wrong line number. Fix by introducing an extra argument to the check_inline*() / read_inline_file() call chain: "so many lines in the inline block". Trac: #1325 Signed-off-by: Gert Doering <gert@greenie.muc.de> --- src/openvpn/options.c | 29 ++++++++++++++++++++--------- 1 file changed, 20 insertions(+), 9 deletions(-)