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@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;