3:53 a.m.
On 7/7/21 12:46 PM, Kristina Hanicova wrote:
Signed-off-by: Kristina Hanicova <khanicov(a)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 ..
+
+ 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;
}
- ret = 0;
+ ret = file_existed;
And then this was:
ret = file_existed ? 1 : 0;
IMO it's easier to track return value.
cleanup:
qemuNamespaceMknodDataClear(data);
return ret;
@@ -1270,15 +1278,11 @@ qemuNamespaceMknodPaths(virDomainObj *vm,
if (qemuSecurityPreFork(driver->securityManager) < 0)
goto cleanup;
- if (virProcessRunInMountNamespace(vm->pid,
- qemuNamespaceMknodHelper,
- &data) < 0) {
- qemuSecurityPostFork(driver->securityManager);
- goto cleanup;
- }
+ ret = virProcessRunInMountNamespace(vm->pid, qemuNamespaceMknodHelper,
+ &data);
+
qemuSecurityPostFork(driver->securityManager);
- ret = 0;
cleanup:
for (i = 0; i < data.nitems; i++) {
if (data.items[i].bindmounted &&
diff --git a/src/util/virprocess.c b/src/util/virprocess.c
index 01d5d01d02..49aef75779 100644
--- a/src/util/virprocess.c
+++ b/src/util/virprocess.c
@@ -1298,7 +1298,9 @@ virProcessRunInForkHelper(int errfd,
virProcessForkCallback cb,
void *opaque)
{
- if (cb(ppid, opaque) < 0) {
+ int ret = 0;
+
+ if ((ret = cb(ppid, opaque)) < 0) {
virErrorPtr err = virGetLastError();
if (err) {
@@ -1323,7 +1325,7 @@ virProcessRunInForkHelper(int errfd,
return -1;
}
- return 0;
+ return ret;
}
This hunk should be a separate patch IMO because it affects more than
just QEMU driver. Also, I think the documentation to
virProcessRunInFork() should be changed, slightly. It currently says:
* On return, the returned value is either -1 with error message
* reported if something went bad in the parent, if child has
* died from a signal or if the child returned EXIT_CANCELED.
* Otherwise the returned value is the exit status of the child.
And the last bit is technically true (the best kind of true :-D), but I
think this would be better:
* Otherwise the returned value is the retval of the callback.
Because now we are passing the positive part of callback's retval. The
same description is used for virProcessRunInMountNamespace(), so it
might be worth fixing it too.
And one more thing, later in the parent we have the following code:
VIR_FORCE_CLOSE(errfd[1]);
nread = virFileReadHeaderFD(errfd[0], sizeof(*bin), &buf);
ret = virProcessWait(child, &status, false);
if (ret == 0) {
ret = status == EXIT_CANCELED ? -1 : status;
if (ret) {
/* machinery to copy error from the child */
} else {
/* generic error */
The second check for @ret value should be changed to ret < 0; because
now @ret can be a positive number which is not an error state.
Michal