[PATCH 1/3] qemu_namespace: Rename variable

Martin Kletzander posted 3 patches 8 months ago
[PATCH 1/3] qemu_namespace: Rename variable
Posted by Martin Kletzander 8 months ago
The boolean actually tells whether the file existed when the function
was called and using it in more places later on makes them
confusing (e.g. do something with a file if it does not exist).  To
better reflect the above and prepare for next patch rename this
variable.

Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
---
 src/qemu/qemu_namespace.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c
index bbe3d5a1f78a..71e29b4ba4f6 100644
--- a/src/qemu/qemu_namespace.c
+++ b/src/qemu/qemu_namespace.c
@@ -1002,10 +1002,10 @@ qemuNamespaceMknodOne(qemuNamespaceMknodItem *data)
     bool isDev = S_ISCHR(data->sb.st_mode) || S_ISBLK(data->sb.st_mode);
     bool isReg = S_ISREG(data->sb.st_mode) || S_ISFIFO(data->sb.st_mode) || S_ISSOCK(data->sb.st_mode);
     bool isDir = S_ISDIR(data->sb.st_mode);
-    bool exists = false;
+    bool existed = false;
 
     if (virFileExists(data->file))
-        exists = true;
+        existed = true;
 
     if (virFileMakeParentPath(data->file) < 0) {
         virReportSystemError(errno,
@@ -1131,7 +1131,7 @@ qemuNamespaceMknodOne(qemuNamespaceMknodItem *data)
         virFileMoveMount(data->target, data->file) < 0)
         goto cleanup;
 
-    ret = exists;
+    ret = existed;
  cleanup:
     if (ret < 0 && delDevice) {
         if (isDir)
-- 
2.47.0
Re: [PATCH 1/3] qemu_namespace: Rename variable
Posted by Peter Krempa 8 months ago
On Wed, Oct 16, 2024 at 09:23:05 +0200, Martin Kletzander wrote:
> The boolean actually tells whether the file existed when the function
> was called and using it in more places later on makes them
> confusing (e.g. do something with a file if it does not exist).  To
> better reflect the above and prepare for next patch rename this
> variable.
> 
> Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
> ---
>  src/qemu/qemu_namespace.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c
> index bbe3d5a1f78a..71e29b4ba4f6 100644
> --- a/src/qemu/qemu_namespace.c
> +++ b/src/qemu/qemu_namespace.c
> @@ -1002,10 +1002,10 @@ qemuNamespaceMknodOne(qemuNamespaceMknodItem *data)
>      bool isDev = S_ISCHR(data->sb.st_mode) || S_ISBLK(data->sb.st_mode);
>      bool isReg = S_ISREG(data->sb.st_mode) || S_ISFIFO(data->sb.st_mode) || S_ISSOCK(data->sb.st_mode);
>      bool isDir = S_ISDIR(data->sb.st_mode);
> -    bool exists = false;
> +    bool existed = false;

You can also add a comment explaining this further as the function
itself isn't documented to have these semantics.

Anyways ...

Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Re: [PATCH 1/3] qemu_namespace: Rename variable
Posted by Martin Kletzander 8 months ago
On Wed, Oct 16, 2024 at 09:32:54AM +0200, Peter Krempa wrote:
>On Wed, Oct 16, 2024 at 09:23:05 +0200, Martin Kletzander wrote:
>> The boolean actually tells whether the file existed when the function
>> was called and using it in more places later on makes them
>> confusing (e.g. do something with a file if it does not exist).  To
>> better reflect the above and prepare for next patch rename this
>> variable.
>>
>> Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
>> ---
>>  src/qemu/qemu_namespace.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c
>> index bbe3d5a1f78a..71e29b4ba4f6 100644
>> --- a/src/qemu/qemu_namespace.c
>> +++ b/src/qemu/qemu_namespace.c
>> @@ -1002,10 +1002,10 @@ qemuNamespaceMknodOne(qemuNamespaceMknodItem *data)
>>      bool isDev = S_ISCHR(data->sb.st_mode) || S_ISBLK(data->sb.st_mode);
>>      bool isReg = S_ISREG(data->sb.st_mode) || S_ISFIFO(data->sb.st_mode) || S_ISSOCK(data->sb.st_mode);
>>      bool isDir = S_ISDIR(data->sb.st_mode);
>> -    bool exists = false;
>> +    bool existed = false;
>
>You can also add a comment explaining this further as the function
>itself isn't documented to have these semantics.
>

I thought of that, started documenting a function called "MknodOne" by
saying it recreates all things, not just those needing a mknod.  Then I
read you said "can" and took the liberty of leaving this task to someone
who's a better wordsmith.

>Anyways ...
>
>Reviewed-by: Peter Krempa <pkrempa@redhat.com>
>