[libvirt] [PATCH 0/5] PATCH 0/8] Remove use of 'strsep' gnulib module (kill-a-gnulib-module-a-day initiative)
Peter Krempa (5): rpc: use virStringSplit instead of strsep util: file: Use g_autofree in virFindFileInPath util: file: Simplify logic of shortuct cases in virFindFileInPath util: file: Replace use of 'strsep' with virStringSplit gnulib: Remove use of 'strsep' module bootstrap.conf | 1 - src/rpc/virnetsocket.c | 28 ++++++++++++---------------- src/util/virfile.c | 38 ++++++++++++++++---------------------- 3 files changed, 28 insertions(+), 39 deletions(-) -- 2.23.0
When parsing allowed authentication methods for the native ssh lib trasnports we used strsep. Since we have virStringSplit helper let's use that one. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/rpc/virnetsocket.c | 28 ++++++++++++---------------- 1 file changed, 12 insertions(+), 16 deletions(-) diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index c9b2a16fe5..21ef7a8034 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -944,9 +944,8 @@ virNetSocketNewConnectLibSSH2(const char *host, int ret = -1; int portN; - char *authMethodNext = NULL; - char *authMethodsCopy = NULL; - char *authMethod; + VIR_AUTOSTRINGLIST authMethodList = NULL; + char **authMethodNext; /* port number will be verified while opening the socket */ if (virStrToLong_i(port, NULL, 10, &portN) < 0) { @@ -987,11 +986,12 @@ virNetSocketNewConnectLibSSH2(const char *host, if (virNetSSHSessionSetChannelCommand(sess, command) != 0) goto error; - authMethodsCopy = g_strdup(authMethods); + if (!(authMethodList = virStringSplit(authMethods, ",", 0))) + goto error; - authMethodNext = authMethodsCopy; + for (authMethodNext = authMethodList; *authMethodNext; authMethodNext++) { + const char *authMethod = *authMethodNext; - while ((authMethod = strsep(&authMethodNext, ","))) { if (STRCASEEQ(authMethod, "keyboard-interactive")) { ret = virNetSSHSessionAuthAddKeyboardAuth(sess, username, -1); } else if (STRCASEEQ(authMethod, "password")) { @@ -1028,13 +1028,11 @@ virNetSocketNewConnectLibSSH2(const char *host, sock->sshSession = sess; *retsock = sock; - VIR_FREE(authMethodsCopy); return 0; error: virObjectUnref(sock); virObjectUnref(sess); - VIR_FREE(authMethodsCopy); return ret; } #else @@ -1079,9 +1077,8 @@ virNetSocketNewConnectLibssh(const char *host, int ret = -1; int portN; - char *authMethodNext = NULL; - char *authMethodsCopy = NULL; - char *authMethod; + VIR_AUTOSTRINGLIST authMethodList = NULL; + char **authMethodNext; /* port number will be verified while opening the socket */ if (virStrToLong_i(port, NULL, 10, &portN) < 0) { @@ -1121,11 +1118,12 @@ virNetSocketNewConnectLibssh(const char *host, if (virNetLibsshSessionSetChannelCommand(sess, command) != 0) goto error; - authMethodsCopy = g_strdup(authMethods); + if (!(authMethodList = virStringSplit(authMethods, ",", 0))) + goto error; - authMethodNext = authMethodsCopy; + for (authMethodNext = authMethodList; *authMethodNext; authMethodNext++) { + const char *authMethod = *authMethodNext; - while ((authMethod = strsep(&authMethodNext, ","))) { if (STRCASEEQ(authMethod, "keyboard-interactive")) { ret = virNetLibsshSessionAuthAddKeyboardAuth(sess, -1); } else if (STRCASEEQ(authMethod, "password")) { @@ -1164,13 +1162,11 @@ virNetSocketNewConnectLibssh(const char *host, sock->ownsFd = false; *retsock = sock; - VIR_FREE(authMethodsCopy); return 0; error: virObjectUnref(sock); virObjectUnref(sess); - VIR_FREE(authMethodsCopy); return ret; } #else -- 2.23.0
On Thu, Nov 14, 2019 at 11:43:29AM +0100, Peter Krempa wrote:
When parsing allowed authentication methods for the native ssh lib trasnports we used strsep. Since we have virStringSplit helper let's use
s/trasnports/transports/
that one.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/rpc/virnetsocket.c | 28 ++++++++++++---------------- 1 file changed, 12 insertions(+), 16 deletions(-)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
On Thu, Nov 14, 2019 at 11:43:29AM +0100, Peter Krempa wrote:
When parsing allowed authentication methods for the native ssh lib trasnports we used strsep. Since we have virStringSplit helper let's use that one.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/rpc/virnetsocket.c | 28 ++++++++++++---------------- 1 file changed, 12 insertions(+), 16 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Simplify the final lookup loop by freeing memory automatically and thus being able to directly return the result. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virfile.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index b1aeaa5851..072a299b39 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -1634,10 +1634,9 @@ char * virFindFileInPath(const char *file) { const char *origpath = NULL; - char *path = NULL; + g_autofree char *path = NULL; char *pathiter; char *pathseg; - char *fullpath = NULL; if (file == NULL) return NULL; @@ -1672,14 +1671,12 @@ virFindFileInPath(const char *file) */ pathiter = path; while ((pathseg = strsep(&pathiter, ":")) != NULL) { - fullpath = g_strdup_printf("%s/%s", pathseg, file); + g_autofree char *fullpath = g_strdup_printf("%s/%s", pathseg, file); if (virFileIsExecutable(fullpath)) - break; - VIR_FREE(fullpath); + return g_steal_pointer(&fullpath); } - VIR_FREE(path); - return fullpath; + return NULL; } -- 2.23.0
On Thu, Nov 14, 2019 at 11:43:30AM +0100, Peter Krempa wrote:
Simplify the final lookup loop by freeing memory automatically and thus being able to directly return the result.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virfile.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
On Thu, Nov 14, 2019 at 11:43:30AM +0100, Peter Krempa wrote:
Simplify the final lookup loop by freeing memory automatically and thus being able to directly return the result.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virfile.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Combine the terms in the two sets of nested conditionals an don't reuse a variable for a more obvious control flow. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virfile.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index 072a299b39..c077639c1c 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -1644,20 +1644,16 @@ virFindFileInPath(const char *file) /* if we are passed an absolute path (starting with /), return a * copy of that path, after validating that it is executable */ - if (IS_ABSOLUTE_FILE_NAME(file)) { - char *ret = NULL; - if (virFileIsExecutable(file)) - ret = g_strdup(file); - return ret; - } + if (IS_ABSOLUTE_FILE_NAME(file) && virFileIsExecutable(file)) + return g_strdup(file); /* If we are passed an anchored path (containing a /), then there * is no path search - it must exist in the current directory */ - if (strchr(file, '/')) { - if (virFileIsExecutable(file)) - ignore_value(virFileAbsPath(file, &path)); - return path; + if (strchr(file, '/') && virFileIsExecutable(file)) { + char *abspath = NULL; + ignore_value(virFileAbsPath(file, &abspath)); + return abspath; } /* copy PATH env so we can tweak it */ -- 2.23.0
On Thu, Nov 14, 2019 at 11:43:31AM +0100, Peter Krempa wrote:
Combine the terms in the two sets of nested conditionals an don't reuse a variable for a more obvious control flow.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virfile.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
On Thu, Nov 14, 2019 at 11:43:31AM +0100, Peter Krempa wrote:
Combine the terms in the two sets of nested conditionals an don't reuse a variable for a more obvious control flow.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virfile.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/src/util/virfile.c b/src/util/virfile.c index 072a299b39..c077639c1c 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -1644,20 +1644,16 @@ virFindFileInPath(const char *file) /* if we are passed an absolute path (starting with /), return a * copy of that path, after validating that it is executable */ - if (IS_ABSOLUTE_FILE_NAME(file)) { - char *ret = NULL; - if (virFileIsExecutable(file)) - ret = g_strdup(file); - return ret; - } + if (IS_ABSOLUTE_FILE_NAME(file) && virFileIsExecutable(file)) + return g_strdup(file);
/* If we are passed an anchored path (containing a /), then there * is no path search - it must exist in the current directory */ - if (strchr(file, '/')) { - if (virFileIsExecutable(file)) - ignore_value(virFileAbsPath(file, &path)); - return path; + if (strchr(file, '/') && virFileIsExecutable(file)) { + char *abspath = NULL; + ignore_value(virFileAbsPath(file, &abspath)); + return abspath;
Before, if either the filename was absolute, or it contained a slash, NULL would be returned right away if the file was not executable. After this patch, those cases will fall through to the iteration over $PATH. Jano
}
/* copy PATH env so we can tweak it */ -- 2.23.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Nov 14, 2019 at 12:51:57 +0100, Ján Tomko wrote:
On Thu, Nov 14, 2019 at 11:43:31AM +0100, Peter Krempa wrote:
Combine the terms in the two sets of nested conditionals an don't reuse a variable for a more obvious control flow.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virfile.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/src/util/virfile.c b/src/util/virfile.c index 072a299b39..c077639c1c 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -1644,20 +1644,16 @@ virFindFileInPath(const char *file) /* if we are passed an absolute path (starting with /), return a * copy of that path, after validating that it is executable */ - if (IS_ABSOLUTE_FILE_NAME(file)) { - char *ret = NULL; - if (virFileIsExecutable(file)) - ret = g_strdup(file); - return ret; - } + if (IS_ABSOLUTE_FILE_NAME(file) && virFileIsExecutable(file)) + return g_strdup(file);
/* If we are passed an anchored path (containing a /), then there * is no path search - it must exist in the current directory */ - if (strchr(file, '/')) { - if (virFileIsExecutable(file)) - ignore_value(virFileAbsPath(file, &path)); - return path; + if (strchr(file, '/') && virFileIsExecutable(file)) { + char *abspath = NULL; + ignore_value(virFileAbsPath(file, &abspath)); + return abspath;
Before, if either the filename was absolute, or it contained a slash, NULL would be returned right away if the file was not executable.
After this patch, those cases will fall through to the iteration over $PATH.
Hmm, right. I'll send a v2.
Make it more obvious that the function will return NULL if the file is not executable and stop reusing variables. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- v2: - fixed logic to do the same as it did before - rewrote commit message to accomodate change src/util/virfile.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index 072a299b39..c7620e49d5 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -1645,19 +1645,23 @@ virFindFileInPath(const char *file) * copy of that path, after validating that it is executable */ if (IS_ABSOLUTE_FILE_NAME(file)) { - char *ret = NULL; - if (virFileIsExecutable(file)) - ret = g_strdup(file); - return ret; + if (!virFileIsExecutable(file)) + return NULL; + + return g_strdup(file); } /* If we are passed an anchored path (containing a /), then there * is no path search - it must exist in the current directory */ if (strchr(file, '/')) { - if (virFileIsExecutable(file)) - ignore_value(virFileAbsPath(file, &path)); - return path; + char *abspath = NULL; + + if (!virFileIsExecutable(file)) + return NULL; + + ignore_value(virFileAbsPath(file, &abspath)); + return abspath; } /* copy PATH env so we can tweak it */ -- 2.23.0
On Thu, Nov 14, 2019 at 02:29:20PM +0100, Peter Krempa wrote:
Make it more obvious that the function will return NULL if the file is not executable and stop reusing variables.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- v2: - fixed logic to do the same as it did before - rewrote commit message to accomodate change
src/util/virfile.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Use our helper instead of the gnulib one. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virfile.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index c077639c1c..55705c8463 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -1634,9 +1634,8 @@ char * virFindFileInPath(const char *file) { const char *origpath = NULL; - g_autofree char *path = NULL; - char *pathiter; - char *pathseg; + VIR_AUTOSTRINGLIST paths = NULL; + char **pathiter; if (file == NULL) return NULL; @@ -1660,14 +1659,16 @@ virFindFileInPath(const char *file) origpath = getenv("PATH"); if (!origpath) origpath = "/bin:/usr/bin"; - path = g_strdup(origpath); /* for each path segment, append the file to search for and test for * it. return it if found. */ - pathiter = path; - while ((pathseg = strsep(&pathiter, ":")) != NULL) { - g_autofree char *fullpath = g_strdup_printf("%s/%s", pathseg, file); + + if (!(paths = virStringSplit(origpath, ":", 0))) + return NULL; + + for (pathiter = paths; *pathiter; pathiter++) { + g_autofree char *fullpath = g_strdup_printf("%s/%s", *pathiter, file); if (virFileIsExecutable(fullpath)) return g_steal_pointer(&fullpath); } -- 2.23.0
On Thu, Nov 14, 2019 at 11:43:32AM +0100, Peter Krempa wrote:
Use our helper instead of the gnulib one.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virfile.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
On Thu, Nov 14, 2019 at 11:43:32AM +0100, Peter Krempa wrote:
Use our helper instead of the gnulib one.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virfile.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
We don't use strsep any more. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- bootstrap.conf | 1 - 1 file changed, 1 deletion(-) diff --git a/bootstrap.conf b/bootstrap.conf index f13fcae11a..e519c69d30 100644 --- a/bootstrap.conf +++ b/bootstrap.conf @@ -75,7 +75,6 @@ socket stat-time strchrnul strptime -strsep strtok_r sys_stat sys_wait -- 2.23.0
On Thu, Nov 14, 2019 at 11:43:33AM +0100, Peter Krempa wrote:
We don't use strsep any more.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- bootstrap.conf | 1 - 1 file changed, 1 deletion(-)
We should probably introduce syntax-check rule to forbid strsep. Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
On Thu, Nov 14, 2019 at 11:43:33AM +0100, Peter Krempa wrote:
We don't use strsep any more.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- bootstrap.conf | 1 - 1 file changed, 1 deletion(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (3)
-
Ján Tomko -
Pavel Hrdina -
Peter Krempa