[PATCH 1/2] qemu: Check for existing file in namespace
Jano Tomko
jtomko at redhat.com
Fri Jul 9 14:32:30 UTC 2021
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 at redhat.com>
>
> 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;
More information about the libvir-list
mailing list