Changeset
src/bhyve/bhyve_capabilities.c | 4 ----
src/qemu/qemu_capabilities.c   | 2 +-
2 files changed, 1 insertion(+), 5 deletions(-)
Git apply log
Switched to a new branch '20180413060145.10659-1-rstoyanov1@gmail.com'
Applying: Remove redundant virFileIsExecutable check
To https://github.com/patchew-project/libvirt
 * [new tag]         patchew/20180413060145.10659-1-rstoyanov1@gmail.com -> patchew/20180413060145.10659-1-rstoyanov1@gmail.com
Test passed: syntax-check

loading

[libvirt] [PATCH] Remove redundant virFileIsExecutable check
Posted by Radostin Stoyanov, 13 weeks ago
Remove unnecessary virFileIsExecutable check after virFindFileInPath.
Since the commit 9ae992f virFindFileInPath will reject non-executables.

9ae992f24353d6506f570fc9dd58355b165e4472
virFindFileInPath: only find executable non-directory

Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com>
---
 src/bhyve/bhyve_capabilities.c | 4 ----
 src/qemu/qemu_capabilities.c   | 2 +-
 2 files changed, 1 insertion(+), 5 deletions(-)

diff --git a/src/bhyve/bhyve_capabilities.c b/src/bhyve/bhyve_capabilities.c
index 381cc0de3..e13085b1d 100644
--- a/src/bhyve/bhyve_capabilities.c
+++ b/src/bhyve/bhyve_capabilities.c
@@ -179,8 +179,6 @@ virBhyveProbeGrubCaps(virBhyveGrubCapsFlags *caps)
     binary = virFindFileInPath("grub-bhyve");
     if (binary == NULL)
         goto out;
-    if (!virFileIsExecutable(binary))
-        goto out;
 
     cmd = virCommandNew(binary);
     virCommandAddArg(cmd, "--help");
@@ -315,8 +313,6 @@ virBhyveProbeCaps(unsigned int *caps)
     binary = virFindFileInPath("bhyve");
     if (binary == NULL)
         goto out;
-    if (!virFileIsExecutable(binary))
-        goto out;
 
     if ((ret = bhyveProbeCapsRTC_UTC(caps, binary)))
         goto out;
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 27180e850..5ebc72f6f 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -653,7 +653,7 @@ virQEMUCapsFindBinary(const char *format,
 
     ret = virFindFileInPath(binary);
     VIR_FREE(binary);
-    if (ret && virFileIsExecutable(ret))
+    if (ret == NULL)
         goto out;
 
     VIR_FREE(ret);
-- 
2.14.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Remove redundant virFileIsExecutable check
Posted by Michal Privoznik, 13 weeks ago
On 04/13/2018 08:01 AM, Radostin Stoyanov wrote:
> Remove unnecessary virFileIsExecutable check after virFindFileInPath.
> Since the commit 9ae992f virFindFileInPath will reject non-executables.
> 
> 9ae992f24353d6506f570fc9dd58355b165e4472
> virFindFileInPath: only find executable non-directory
> 
> Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com>
> ---
>  src/bhyve/bhyve_capabilities.c | 4 ----
>  src/qemu/qemu_capabilities.c   | 2 +-
>  2 files changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/src/bhyve/bhyve_capabilities.c b/src/bhyve/bhyve_capabilities.c
> index 381cc0de3..e13085b1d 100644
> --- a/src/bhyve/bhyve_capabilities.c
> +++ b/src/bhyve/bhyve_capabilities.c
> @@ -179,8 +179,6 @@ virBhyveProbeGrubCaps(virBhyveGrubCapsFlags *caps)
>      binary = virFindFileInPath("grub-bhyve");
>      if (binary == NULL)
>          goto out;
> -    if (!virFileIsExecutable(binary))
> -        goto out;
>  
>      cmd = virCommandNew(binary);
>      virCommandAddArg(cmd, "--help");
> @@ -315,8 +313,6 @@ virBhyveProbeCaps(unsigned int *caps)
>      binary = virFindFileInPath("bhyve");
>      if (binary == NULL)
>          goto out;
> -    if (!virFileIsExecutable(binary))
> -        goto out;

These twp ^^ make sense.

>  
>      if ((ret = bhyveProbeCapsRTC_UTC(caps, binary)))
>          goto out;
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 27180e850..5ebc72f6f 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -653,7 +653,7 @@ virQEMUCapsFindBinary(const char *format,
>  
>      ret = virFindFileInPath(binary);
>      VIR_FREE(binary);
> -    if (ret && virFileIsExecutable(ret))
> +    if (ret == NULL)
>          goto out;
>  
>      VIR_FREE(ret);
> 

However, this one does not. With this change, if virFindFileInPath()
returned something, VIR_FREE() is called over it and NULL is returned.
So the condition should be reversed. But looking at whole function, it's
insane code and the if() statement is not needed at all.

I'm squashing this in:

diff --git i/src/qemu/qemu_capabilities.c w/src/qemu/qemu_capabilities.c
index 5ebc72f6f4..c8488f875d 100644
--- i/src/qemu/qemu_capabilities.c
+++ w/src/qemu/qemu_capabilities.c
@@ -649,16 +649,10 @@ virQEMUCapsFindBinary(const char *format,
     char *binary = NULL;
 
     if (virAsprintf(&binary, format, archstr) < 0)
-        goto out;
+        return NULL;
 
     ret = virFindFileInPath(binary);
     VIR_FREE(binary);
-    if (ret == NULL)
-        goto out;
-
-    VIR_FREE(ret);
-
- out:
     return ret;
 }


ACKing and pushing.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Remove redundant virFileIsExecutable check
Posted by Radostin Stoyanov, 13 weeks ago
On 13/04/18 08:35, Michal Privoznik wrote:
> On 04/13/2018 08:01 AM, Radostin Stoyanov wrote:
>> Remove unnecessary virFileIsExecutable check after virFindFileInPath.
>> Since the commit 9ae992f virFindFileInPath will reject non-executables.
>>
>> 9ae992f24353d6506f570fc9dd58355b165e4472
>> virFindFileInPath: only find executable non-directory
>>
>> Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com>
>> ---
>>  src/bhyve/bhyve_capabilities.c | 4 ----
>>  src/qemu/qemu_capabilities.c   | 2 +-
>>  2 files changed, 1 insertion(+), 5 deletions(-)
>>
>> diff --git a/src/bhyve/bhyve_capabilities.c b/src/bhyve/bhyve_capabilities.c
>> index 381cc0de3..e13085b1d 100644
>> --- a/src/bhyve/bhyve_capabilities.c
>> +++ b/src/bhyve/bhyve_capabilities.c
>> @@ -179,8 +179,6 @@ virBhyveProbeGrubCaps(virBhyveGrubCapsFlags *caps)
>>      binary = virFindFileInPath("grub-bhyve");
>>      if (binary == NULL)
>>          goto out;
>> -    if (!virFileIsExecutable(binary))
>> -        goto out;
>>  
>>      cmd = virCommandNew(binary);
>>      virCommandAddArg(cmd, "--help");
>> @@ -315,8 +313,6 @@ virBhyveProbeCaps(unsigned int *caps)
>>      binary = virFindFileInPath("bhyve");
>>      if (binary == NULL)
>>          goto out;
>> -    if (!virFileIsExecutable(binary))
>> -        goto out;
> These twp ^^ make sense.
>
>>  
>>      if ((ret = bhyveProbeCapsRTC_UTC(caps, binary)))
>>          goto out;
>> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
>> index 27180e850..5ebc72f6f 100644
>> --- a/src/qemu/qemu_capabilities.c
>> +++ b/src/qemu/qemu_capabilities.c
>> @@ -653,7 +653,7 @@ virQEMUCapsFindBinary(const char *format,
>>  
>>      ret = virFindFileInPath(binary);
>>      VIR_FREE(binary);
>> -    if (ret && virFileIsExecutable(ret))
>> +    if (ret == NULL)
>>          goto out;
>>  
>>      VIR_FREE(ret);
>>
> However, this one does not. With this change, if virFindFileInPath()
> returned something, VIR_FREE() is called over it and NULL is returned.
> So the condition should be reversed. But looking at whole function, it's
> insane code and the if() statement is not needed at all.
>
> I'm squashing this in:
>
> diff --git i/src/qemu/qemu_capabilities.c w/src/qemu/qemu_capabilities.c
> index 5ebc72f6f4..c8488f875d 100644
> --- i/src/qemu/qemu_capabilities.c
> +++ w/src/qemu/qemu_capabilities.c
> @@ -649,16 +649,10 @@ virQEMUCapsFindBinary(const char *format,
>      char *binary = NULL;
>  
>      if (virAsprintf(&binary, format, archstr) < 0)
> -        goto out;
> +        return NULL;
>  
>      ret = virFindFileInPath(binary);
>      VIR_FREE(binary);
> -    if (ret == NULL)
> -        goto out;
> -
> -    VIR_FREE(ret);
> -
> - out:
>      return ret;
>  }
>
>
> ACKing and pushing.
Thanks :)

Radostin
> Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list