[libvirt] [PATCH] vsh: Fix warnings in command line completer
GCC complained that vsh.c: In function 'vshReadlineOptionsGenerator': vsh.c:2622:29: warning: unused variable 'opt' [-Wunused-variable] const vshCmdOptDef *opt = &cmd->opts[list_index]; ^ vsh.c: In function 'vshReadlineParse': vsh.c:2830:44: warning: 'opt' may be used uninitialized in this function [-Wmaybe-uninitialized] completed_list = opt->completer(autoCompleteOpaque, Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- Notes: I'm not very fond of the second hunk, but the completer is such a horrible piece of code I couldn't believe it was pushed. I just made the easiest fix and ran away from the code screaming. tools/vsh.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tools/vsh.c b/tools/vsh.c index cdd1cba..420eb79 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -2619,7 +2619,6 @@ vshReadlineOptionsGenerator(const char *text, int state, const vshCmdDef *cmd_pa return NULL; while ((name = cmd->opts[list_index].name)) { - const vshCmdOptDef *opt = &cmd->opts[list_index]; char *res; list_index++; @@ -2648,7 +2647,7 @@ vshReadlineParse(const char *text, int state) static vshCommandParser parser, sanitizer; vshCommandToken tk; static const vshCmdDef *cmd; - const vshCmdOptDef *opt; + const vshCmdOptDef *opt = NULL; char *tkdata, *optstr, *const_tkdata, *completed_name; char *res = NULL; static char *ctext, *sanitized_text; -- 2.10.0
On Wed, Oct 05, 2016 at 09:26:43AM +0200, Jiri Denemark wrote:
GCC complained that
vsh.c: In function 'vshReadlineOptionsGenerator': vsh.c:2622:29: warning: unused variable 'opt' [-Wunused-variable] const vshCmdOptDef *opt = &cmd->opts[list_index]; ^ vsh.c: In function 'vshReadlineParse': vsh.c:2830:44: warning: 'opt' may be used uninitialized in this function [-Wmaybe-uninitialized] completed_list = opt->completer(autoCompleteOpaque,
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> ---
Notes: I'm not very fond of the second hunk, but the completer is such a horrible piece of code I couldn't believe it was pushed. I just made the easiest fix and ran away from the code screaming.
tools/vsh.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/tools/vsh.c b/tools/vsh.c index cdd1cba..420eb79 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -2619,7 +2619,6 @@ vshReadlineOptionsGenerator(const char *text, int state, const vshCmdDef *cmd_pa return NULL;
while ((name = cmd->opts[list_index].name)) { - const vshCmdOptDef *opt = &cmd->opts[list_index]; char *res;
list_index++; @@ -2648,7 +2647,7 @@ vshReadlineParse(const char *text, int state) static vshCommandParser parser, sanitizer; vshCommandToken tk; static const vshCmdDef *cmd; - const vshCmdOptDef *opt; + const vshCmdOptDef *opt = NULL;
Always a good thing to do, you can't broke something that worked by initialization. And it's nicer. Even though Peter^Wsome people would argue that it's pointless, especially for ancient and weird compilers (i.e. even slightly different compiler version that they're running or with one different setting). But looking at the function it's easy to dereference that opt lines below in that function. Same way cmd_exists can be used without initialization and I believe half of those hundred variables too. So the thing you're fixing with this hunk still needs a proper fix, and I don't know whether we want to hide that. But looking at the code, there's already so much stuff hidden... Basically, I'm trying to say "ACK, you were right, I'm running away too now!", which translates to "ACK" in tl;dr language.
char *tkdata, *optstr, *const_tkdata, *completed_name; char *res = NULL; static char *ctext, *sanitized_text; -- 2.10.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Oct 05, 2016 at 16:07:09 +0200, Martin Kletzander wrote:
On Wed, Oct 05, 2016 at 09:26:43AM +0200, Jiri Denemark wrote:
@@ -2648,7 +2647,7 @@ vshReadlineParse(const char *text, int state) static vshCommandParser parser, sanitizer; vshCommandToken tk; static const vshCmdDef *cmd; - const vshCmdOptDef *opt; + const vshCmdOptDef *opt = NULL;
Always a good thing to do, you can't broke something that worked by initialization. And it's nicer. Even though Peter^Wsome people would argue that it's pointless, especially for ancient and weird compilers (i.e. even slightly different compiler version that they're running or with one different setting). But looking at the function it's easy to dereference that opt lines below in that function. Same way cmd_exists can be used without initialization and I believe half of those hundred variables too.
Well, about half of the variables in there are *static*, which means they are initialized. Sure, there are legit uses of static variables in functions and it may even be this case, I don't know, but I know having so many of them in a single function is just not OK.
Basically, I'm trying to say "ACK, you were right, I'm running away too now!", which translates to "ACK" in tl;dr language.
Thanks, I pushed the patch. Jirka
On 05.10.2016 09:26, Jiri Denemark wrote:
GCC complained that
vsh.c: In function 'vshReadlineOptionsGenerator': vsh.c:2622:29: warning: unused variable 'opt' [-Wunused-variable] const vshCmdOptDef *opt = &cmd->opts[list_index]; ^ vsh.c: In function 'vshReadlineParse': vsh.c:2830:44: warning: 'opt' may be used uninitialized in this function [-Wmaybe-uninitialized] completed_list = opt->completer(autoCompleteOpaque,
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> ---
Notes: I'm not very fond of the second hunk, but the completer is such a horrible piece of code I couldn't believe it was pushed. I just made the easiest fix and ran away from the code screaming.
Well, this area has always been a mess. And I think it is mostly because of how readline API works. The completer is called multiple times and we have to do a lot in it in order to provide all the completions. Having said that, I'm not sure what can be done here to improve the code apart from rewriting it completely from scratch (or switching to a different library, e.g. libedit). Michal
On 06/10/16 09:57, Michal Privoznik wrote:
On 05.10.2016 09:26, Jiri Denemark wrote:
GCC complained that
vsh.c: In function 'vshReadlineOptionsGenerator': vsh.c:2622:29: warning: unused variable 'opt' [-Wunused-variable] const vshCmdOptDef *opt = &cmd->opts[list_index]; ^ vsh.c: In function 'vshReadlineParse': vsh.c:2830:44: warning: 'opt' may be used uninitialized in this function [-Wmaybe-uninitialized] completed_list = opt->completer(autoCompleteOpaque,
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> ---
Notes: I'm not very fond of the second hunk, but the completer is such a horrible piece of code I couldn't believe it was pushed. I just made the easiest fix and ran away from the code screaming.
Well, this area has always been a mess. And I think it is mostly because of how readline API works. The completer is called multiple times and we have to do a lot in it in order to provide all the completions. Having said that, I'm not sure what can be done here to improve the code apart from rewriting it completely from scratch (or switching to a different library, e.g. libedit).
Well, depending on which parts exactly you meant need rewriting (if of course you didn't refer to the whole virsh code then all of the following is just ambiguous) because even if you tried to rewrite just parts of it, there's imho very little chance you'd end up with a less mess that it is now and that is because there are parts of the code that would prevent you from doing so like the fact that we allow multiple commands delimited by ';' in the interactive shell. I've looked into that at least 3 times already and still think that the biggest mess is the parser which we should start from. It is very unfortunate that we allow multiple commands simultaneously, since that is preventing us from using an external library to do the parsing for us in a transparent way. The code would look sooo much cooler then...sigh... Erik
Michal
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (4)
-
Erik Skultety -
Jiri Denemark -
Martin Kletzander -
Michal Privoznik