[libvirt] [PATCH 0/5] PATCH 0/8] Remove use of 'strsep' gnulib module (kill-a-gnulib-module-a-day initiative)
4:43 a.m.
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
4:43 a.m.
New subject: [libvirt] [PATCH 1/5] rpc: use virStringSplit instead of strsep
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(a)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
5:22 a.m.
New subject: [libvirt] [PATCH 1/5] rpc: use virStringSplit instead of strsep
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(a)redhat.com>
---
src/rpc/virnetsocket.c | 28 ++++++++++++----------------
1 file changed, 12 insertions(+), 16 deletions(-)
Reviewed-by: Pavel Hrdina <phrdina(a)redhat.com>
5:47 a.m.
New subject: [libvirt] [PATCH 1/5] rpc: use virStringSplit instead of strsep
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(a)redhat.com>
---
src/rpc/virnetsocket.c | 28 ++++++++++++----------------
1 file changed, 12 insertions(+), 16 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
4:43 a.m.
New subject: [libvirt] [PATCH 2/5] util: file: Use g_autofree in virFindFileInPath
Simplify the final lookup loop by freeing memory automatically and thus
being able to directly return the result.
Signed-off-by: Peter Krempa <pkrempa(a)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
5:24 a.m.
New subject: [libvirt] [PATCH 2/5] util: file: Use g_autofree in virFindFileInPath
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(a)redhat.com>
---
src/util/virfile.c | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
Reviewed-by: Pavel Hrdina <phrdina(a)redhat.com>
5:48 a.m.
New subject: [libvirt] [PATCH 2/5] util: file: Use g_autofree in virFindFileInPath
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(a)redhat.com>
---
src/util/virfile.c | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
4:43 a.m.
New subject: [libvirt] [PATCH 3/5] util: file: Simplify logic of shortuct cases in virFindFileInPath
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(a)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
5:31 a.m.
New subject: [libvirt] [PATCH 3/5] util: file: Simplify logic of shortuct cases in virFindFileInPath
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(a)redhat.com>
---
src/util/virfile.c | 16 ++++++----------
1 file changed, 6 insertions(+), 10 deletions(-)
Reviewed-by: Pavel Hrdina <phrdina(a)redhat.com>
5:51 a.m.
New subject: [libvirt] [PATCH 3/5] util: file: Simplify logic of shortuct cases in virFindFileInPath
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(a)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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
6:02 a.m.
New subject: [libvirt] [PATCH 3/5] util: file: Simplify logic of shortuct cases in virFindFileInPath
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(a)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.
7:29 a.m.
New subject: [libvirt] [PATCH v2 3/5] util: file: Use more obvious logic in virFindFileInPath
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(a)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
7:49 a.m.
New subject: [libvirt] [PATCH v2 3/5] util: file: Use more obvious logic in virFindFileInPath
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(a)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(a)redhat.com>
Jano
4:43 a.m.
New subject: [libvirt] [PATCH 4/5] util: file: Replace use of 'strsep' with virStringSplit
Use our helper instead of the gnulib one.
Signed-off-by: Peter Krempa <pkrempa(a)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
5:32 a.m.
New subject: [libvirt] [PATCH 4/5] util: file: Replace use of 'strsep' with virStringSplit
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(a)redhat.com>
---
src/util/virfile.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
Reviewed-by: Pavel Hrdina <phrdina(a)redhat.com>
5:52 a.m.
New subject: [libvirt] [PATCH 4/5] util: file: Replace use of 'strsep' with virStringSplit
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(a)redhat.com>
---
src/util/virfile.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
4:43 a.m.
New subject: [libvirt] [PATCH 5/5] gnulib: Remove use of 'strsep' module
We don't use strsep any more.
Signed-off-by: Peter Krempa <pkrempa(a)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
5:33 a.m.
New subject: [libvirt] [PATCH 5/5] gnulib: Remove use of 'strsep' module
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(a)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(a)redhat.com>
5:52 a.m.
New subject: [libvirt] [PATCH 5/5] gnulib: Remove use of 'strsep' module
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(a)redhat.com>
---
bootstrap.conf | 1 -
1 file changed, 1 deletion(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
1835
days inactive
1835
days old
18 comments
3 participants
participants (3)
-
Ján Tomko -
Pavel Hrdina -
Peter Krempa