Re: [PATCH 1/2] qemu: Check for existing file in namespace

[Jano Tomko <jtomko@xxxxxxxxxx>]

On a %A in %Y, Michal Prívozník wrote:
> On 7/7/21 12:46 PM, Kristina Hanicova wrote:
>> Signed-off-by: Kristina Hanicova <khanicov@xxxxxxxxxx>
> 
> The commit message is a bit sparse. Can you describe the intent?
> 
>> ---
>>  src/qemu/qemu_namespace.c | 24 ++++++++++++++----------
>>  src/util/virprocess.c     |  6 ++++--
>>  2 files changed, 18 insertions(+), 12 deletions(-)
>>
>> diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c
>> index 98495e8ef8..154968acbd 100644
>> --- a/src/qemu/qemu_namespace.c
>> +++ b/src/qemu/qemu_namespace.c
>> @@ -929,6 +929,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);
>> +    int file_exists = 0;
> 
> 
> Oh, the rest of the function uses camelCase ;-) However, what if this
> was a bool ..
> 

One-word variables are my favorite way to avoid that flamewar: exists or existed

>> +
>> +    if (virFileExists(data->file))
>> +        file_exists = 1;
>>  
>>      if (virFileMakeParentPath(data->file) < 0) {
>>          virReportSystemError(errno,
>> @@ -1039,7 +1043,7 @@ qemuNamespaceMknodOne(qemuNamespaceMknodItem *data)
>>          virFileMoveMount(data->target, data->file) < 0)
>>          goto cleanup;
>>  
>> -    ret = 0;
>> +    ret = file_exists;
>>   cleanup:
>>      if (ret < 0 && delDevice) {
>>          if (isDir)
>> @@ -1069,15 +1073,19 @@ qemuNamespaceMknodHelper(pid_t pid G_GNUC_UNUSED,
>>      qemuNamespaceMknodData *data = opaque;
>>      size_t i;
>>      int ret = -1;
>> +    int file_existed = 0;
>>  
>>      qemuSecurityPostFork(data->driver->securityManager);
>>  
>>      for (i = 0; i < data->nitems; i++) {
>> -        if (qemuNamespaceMknodOne(&data->items[i]) < 0)
>> +        int rc = 0;
>> +
>> +        if ((rc = qemuNamespaceMknodOne(&data->items[i])) < 0)
>>              goto cleanup;

>> +        file_existed = file_existed || rc;

Please use <>= operators for numeric types, see
https://libvirt.org/coding-style.html#conditional-expressions

Also, if you switch the variable to bool, there is no need to check its original value:

  if (rc > 0)
      file_existed = true;

Alternatively, we could learn from qemuDomainNamespaceTeardownDisk and make InputTeardown a no-op ;)

>>      }
>>  
>> -    ret = 0;
>> +    ret = file_existed;
> 
> And then this was:
> 
>   ret = file_existed ? 1 : 0;
> 
> 
> IMO it's easier to track return value.
> 

Agreed.

Jano

>>   cleanup:
>>      qemuNamespaceMknodDataClear(data);
>>      return ret;