[PATCH v2 0/2] Fix broken build with readline
This is a v2 of: https://www.redhat.com/archives/libvir-list/2020-September/msg00107.html diff to v1: - Patch 1/2 is completely new, it removes include of readline from virsh.c and virt-admin.c, so that patch 2/2 can be unchanged. Michal Prívozník (2): vsh: Introduce wrapper for add_history() vsh: Define HAVE_STDARG_H before including readline tools/virsh.c | 10 ++-------- tools/virt-admin.c | 10 ++-------- tools/vsh.c | 16 ++++++++++++++++ tools/vsh.h | 2 ++ 4 files changed, 22 insertions(+), 16 deletions(-) -- 2.26.2
This allows us to drop include of readline header files from virsh.c and virt-admin.c because they needed it only because of the add_history() function. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/virsh.c | 10 ++-------- tools/virt-admin.c | 10 ++-------- tools/vsh.c | 12 ++++++++++++ tools/vsh.h | 2 ++ 4 files changed, 18 insertions(+), 16 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 06ff5e8336..ac9b7ad238 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -30,11 +30,6 @@ #include <sys/stat.h> #include <inttypes.h> -#if WITH_READLINE -# include <readline/readline.h> -# include <readline/history.h> -#endif - #include "internal.h" #include "virerror.h" #include "virbuffer.h" @@ -920,9 +915,8 @@ main(int argc, char **argv) if (ctl->cmdstr == NULL) break; /* EOF */ if (*ctl->cmdstr) { -#if WITH_READLINE - add_history(ctl->cmdstr); -#endif + vshReadlineHistoryAdd(ctl->cmdstr); + if (vshCommandStringParse(ctl, ctl->cmdstr, NULL)) vshCommandRun(ctl, ctl->cmd); } diff --git a/tools/virt-admin.c b/tools/virt-admin.c index 774661b2ca..72084a78e9 100644 --- a/tools/virt-admin.c +++ b/tools/virt-admin.c @@ -24,11 +24,6 @@ #include <unistd.h> #include <getopt.h> -#if WITH_READLINE -# include <readline/readline.h> -# include <readline/history.h> -#endif - #include "internal.h" #include "virt-admin.h" #include "viralloc.h" @@ -1597,9 +1592,8 @@ main(int argc, char **argv) if (ctl->cmdstr == NULL) break; /* EOF */ if (*ctl->cmdstr) { -#if WITH_READLINE - add_history(ctl->cmdstr); -#endif + vshReadlineHistoryAdd(ctl->cmdstr); + if (vshCommandStringParse(ctl, ctl->cmdstr, NULL)) vshCommandRun(ctl, ctl->cmd); } diff --git a/tools/vsh.c b/tools/vsh.c index 11f493f969..2511089e6e 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -2925,6 +2925,12 @@ vshReadline(vshControl *ctl G_GNUC_UNUSED, const char *prompt) return readline(prompt); } +void +vshReadlineHistoryAdd(const char *cmd) +{ + return add_history(cmd); +} + #else /* !WITH_READLINE */ static int @@ -2960,6 +2966,12 @@ vshReadline(vshControl *ctl G_GNUC_UNUSED, return g_strdup(r); } +void +vshReadlineHistoryAdd(const char *cmd) +{ + /* empty */ +} + #endif /* !WITH_READLINE */ /* diff --git a/tools/vsh.h b/tools/vsh.h index 290bb71f3e..0a40995bf5 100644 --- a/tools/vsh.h +++ b/tools/vsh.h @@ -463,6 +463,8 @@ bool cmdComplete(vshControl *ctl, const vshCmd *cmd); /* readline */ char * vshReadline(vshControl *ctl, const char *prompt); +void vshReadlineHistoryAdd(const char *cmd); + /* allocation wrappers */ void *_vshMalloc(vshControl *ctl, size_t sz, const char *filename, int line); #define vshMalloc(_ctl, _sz) _vshMalloc(_ctl, _sz, __FILE__, __LINE__) -- 2.26.2
On a Thursday in 2020, Michal Privoznik wrote:
This allows us to drop include of readline header files from virsh.c and virt-admin.c because they needed it only because of the add_history() function.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/virsh.c | 10 ++-------- tools/virt-admin.c | 10 ++-------- tools/vsh.c | 12 ++++++++++++ tools/vsh.h | 2 ++ 4 files changed, 18 insertions(+), 16 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
As it turned out my previous commits which switched from HAVE_ to WITH_ and dropped stdarg.h detection were a bit too aggressive. Because of reasons described in 9ea3424a178 we need to define HAVE_STDARG_H before including readline otherwise macos build fails. Honestly, I still don't fully understand the problem so I am not going to bother you with "explanation". Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/vsh.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tools/vsh.c b/tools/vsh.c index 2511089e6e..ef2a3f62d7 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -30,6 +30,10 @@ #include <signal.h> #if WITH_READLINE +/* In order to have proper rl_message declaration with older + * versions of readline, we have to declare this. See 9ea3424a178 + * for more info. */ +# define HAVE_STDARG_H # include <readline/readline.h> # include <readline/history.h> #endif -- 2.26.2
On a Thursday in 2020, Michal Privoznik wrote:
As it turned out my previous commits which switched from HAVE_ to WITH_ and dropped stdarg.h detection were a bit too aggressive. Because of reasons described in 9ea3424a178 we need to define HAVE_STDARG_H before including readline otherwise macos build fails. Honestly, I still don't fully understand the problem so I am not going to bother you with "explanation".
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/vsh.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/tools/vsh.c b/tools/vsh.c index 2511089e6e..ef2a3f62d7 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -30,6 +30,10 @@ #include <signal.h>
#if WITH_READLINE +/* In order to have proper rl_message declaration with older + * versions of readline, we have to declare this. See 9ea3424a178
I don't think it's because of the 'older version', I'm surprised it works for me on Linux without it.
+ * for more info. */ +# define HAVE_STDARG_H # include <readline/readline.h> # include <readline/history.h> #endif
Reviewed-by: Ján Tomko <jtomko@redhat.com> (just don't expect it to fix the pipeline on master, some other issues have creeped in in the meantime) Jano
On 9/3/20 4:24 AM, Michal Privoznik wrote:
As it turned out my previous commits which switched from HAVE_ to WITH_ and dropped stdarg.h detection were a bit too aggressive. Because of reasons described in 9ea3424a178 we need to define HAVE_STDARG_H before including readline otherwise macos build fails. Honestly, I still don't fully understand the problem so I am not going to bother you with "explanation".
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/vsh.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/tools/vsh.c b/tools/vsh.c index 2511089e6e..ef2a3f62d7 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -30,6 +30,10 @@ #include <signal.h>
#if WITH_READLINE +/* In order to have proper rl_message declaration with older + * versions of readline, we have to declare this. See 9ea3424a178 + * for more info. */ +# define HAVE_STDARG_H # include <readline/readline.h> # include <readline/history.h> #endif
How common is it for library .h files to assume autotools-style HAVE_* and WITH_* flags are defined? I'm just wondering if this particular failure is just one instance that's indicating a more general problem.
On Thu, 2020-09-03 at 11:24 -0400, Laine Stump wrote:
On 9/3/20 4:24 AM, Michal Privoznik wrote:
+/* In order to have proper rl_message declaration with older + * versions of readline, we have to declare this. See 9ea3424a178 + * for more info. */ +# define HAVE_STDARG_H # include <readline/readline.h> # include <readline/history.h> #endif
How common is it for library .h files to assume autotools-style HAVE_* and WITH_* flags are defined? I'm just wondering if this particular failure is just one instance that's indicating a more general problem.
It looks like Wireshark also assumes you're using autotools: https://www.redhat.com/archives/libvir-list/2020-September/msg00156.html I would consider it a bug in the library. -- Andrea Bolognani / Red Hat / Virtualization
participants (4)
-
Andrea Bolognani -
Ján Tomko -
Laine Stump -
Michal Privoznik