1. Patchew
  2. Libvirt
  3. [libvirt] [PATCH 0/5] PATCH 0/8] Remove use of 'strsep' gnulib module (kill-a-gnulib-module-a-day initiative)
  4. View patch

[libvirt] [PATCH 3/5] util: file: Simplify logic of shortuct cases in virFindFileInPath

Peter Krempa posted 5 patches 6 years, 2 months ago
[libvirt] [PATCH 3/5] util: file: Simplify logic of shortuct cases in virFindFileInPath
Posted by Peter Krempa 6 years, 2 months ago 
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/5] util: file: Simplify logic of shortuct cases in virFindFileInPath
Posted by Pavel Hrdina 6 years, 2 months ago 
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>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/5] util: file: Simplify logic of shortuct cases in virFindFileInPath
Posted by Ján Tomko 6 years, 2 months ago 
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/5] util: file: Simplify logic of shortuct cases in virFindFileInPath
Posted by Peter Krempa 6 years, 2 months ago 
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.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 3/5] util: file: Use more obvious logic in virFindFileInPath
Posted by Peter Krempa 6 years, 2 months ago 
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 3/5] util: file: Use more obvious logic in virFindFileInPath
Posted by Ján Tomko 6 years, 2 months ago 
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.