5:46 a.m.
If the attempt to attach an input device failed, we erased the
unattached device from the namespace. This resulted in erasing an
already attached device in case of a duplicate. We need to check for
existing file in the namespace in order to determine erasing it in case
of a failure.
Kristina Hanicova (2):
qemu: Check for existing file in namespace
qemu: Do not erase input device from namespace if duplicate
src/qemu/qemu_hotplug.c | 3 +--
src/qemu/qemu_namespace.c | 34 ++++++++++++++++++++++------------
src/qemu/qemu_namespace.h | 3 ++-
src/util/virprocess.c | 6 ++++--
4 files changed, 29 insertions(+), 17 deletions(-)
--
2.31.1
5:46 a.m.
New subject: [PATCH 1/2] qemu: Check for existing file in namespace
Signed-off-by: Kristina Hanicova <khanicov(a)redhat.com>
---
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;
+
+ 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;
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;
}
--
2.31.1
3:53 a.m.
New subject: [PATCH 1/2] qemu: Check for existing file in namespace
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
9:32 a.m.
New subject: [PATCH 1/2] qemu: Check for existing file in namespace
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(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 ..
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;
5:46 a.m.
New subject: [PATCH 2/2] qemu: Do not erase input device from namespace if duplicate
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1780508
Signed-off-by: Kristina Hanicova <khanicov(a)redhat.com>
---
src/qemu/qemu_hotplug.c | 3 +--
src/qemu/qemu_namespace.c | 10 ++++++++--
src/qemu/qemu_namespace.h | 3 ++-
3 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index d2a354d026..7002623924 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -3225,9 +3225,8 @@ qemuDomainAttachInputDevice(virQEMUDriver *driver,
if (qemuBuildInputDevStr(&devstr, vm->def, input, priv->qemuCaps) < 0)
goto cleanup;
- if (qemuDomainNamespaceSetupInput(vm, input) < 0)
+ if (qemuDomainNamespaceSetupInput(vm, input, &teardowndevice) < 0)
goto cleanup;
- teardowndevice = true;
if (qemuSetupInputCgroup(vm, input) < 0)
goto cleanup;
diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c
index 154968acbd..22bdda229a 100644
--- a/src/qemu/qemu_namespace.c
+++ b/src/qemu/qemu_namespace.c
@@ -1600,9 +1600,11 @@ qemuDomainNamespaceTeardownRNG(virDomainObj *vm,
int
qemuDomainNamespaceSetupInput(virDomainObj *vm,
- virDomainInputDef *input)
+ virDomainInputDef *input,
+ bool *created)
{
g_autoptr(virGSListString) paths = NULL;
+ int ret = 0;
if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
return 0;
@@ -1610,8 +1612,12 @@ qemuDomainNamespaceSetupInput(virDomainObj *vm,
if (qemuDomainSetupInput(input, &paths) < 0)
return -1;
- if (qemuNamespaceMknodPaths(vm, paths) < 0)
+ if ((ret = qemuNamespaceMknodPaths(vm, paths)) < 0)
return -1;
+
+ if (ret == 0)
+ *created = true;
+
return 0;
}
diff --git a/src/qemu/qemu_namespace.h b/src/qemu/qemu_namespace.h
index 771d7873ef..5d9af123a9 100644
--- a/src/qemu/qemu_namespace.h
+++ b/src/qemu/qemu_namespace.h
@@ -80,7 +80,8 @@ int qemuDomainNamespaceTeardownRNG(virDomainObj *vm,
virDomainRNGDef *rng);
int qemuDomainNamespaceSetupInput(virDomainObj *vm,
- virDomainInputDef *input);
+ virDomainInputDef *input,
+ bool *created);
int qemuDomainNamespaceTeardownInput(virDomainObj *vm,
virDomainInputDef *input);
--
2.31.1
3:53 a.m.
New subject: [PATCH 2/2] qemu: Do not erase input device from namespace if duplicate
On 7/7/21 12:46 PM, Kristina Hanicova wrote:
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1780508
Signed-off-by: Kristina Hanicova <khanicov(a)redhat.com>
---
I know writing commit messages is hard, but I think you can describe the
problem and the proposed solution briefly.
src/qemu/qemu_hotplug.c | 3 +--
src/qemu/qemu_namespace.c | 10 ++++++++--
src/qemu/qemu_namespace.h | 3 ++-
3 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index d2a354d026..7002623924 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -3225,9 +3225,8 @@ qemuDomainAttachInputDevice(virQEMUDriver *driver,
if (qemuBuildInputDevStr(&devstr, vm->def, input, priv->qemuCaps) < 0)
goto cleanup;
- if (qemuDomainNamespaceSetupInput(vm, input) < 0)
+ if (qemuDomainNamespaceSetupInput(vm, input, &teardowndevice) < 0)
goto cleanup;
- teardowndevice = true;
if (qemuSetupInputCgroup(vm, input) < 0)
goto cleanup;
diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c
index 154968acbd..22bdda229a 100644
--- a/src/qemu/qemu_namespace.c
+++ b/src/qemu/qemu_namespace.c
@@ -1600,9 +1600,11 @@ qemuDomainNamespaceTeardownRNG(virDomainObj *vm,
int
qemuDomainNamespaceSetupInput(virDomainObj *vm,
- virDomainInputDef *input)
+ virDomainInputDef *input,
+ bool *created)
{
g_autoptr(virGSListString) paths = NULL;
+ int ret = 0;
if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
return 0;
@@ -1610,8 +1612,12 @@ qemuDomainNamespaceSetupInput(virDomainObj *vm,
if (qemuDomainSetupInput(input, &paths) < 0)
return -1;
- if (qemuNamespaceMknodPaths(vm, paths) < 0)
+ if ((ret = qemuNamespaceMknodPaths(vm, paths)) < 0)
return -1;
+
+ if (ret == 0)
+ *created = true;
+
return 0;
}
diff --git a/src/qemu/qemu_namespace.h b/src/qemu/qemu_namespace.h
index 771d7873ef..5d9af123a9 100644
--- a/src/qemu/qemu_namespace.h
+++ b/src/qemu/qemu_namespace.h
@@ -80,7 +80,8 @@ int qemuDomainNamespaceTeardownRNG(virDomainObj *vm,
virDomainRNGDef *rng);
int qemuDomainNamespaceSetupInput(virDomainObj *vm,
- virDomainInputDef *input);
+ virDomainInputDef *input,
+ bool *created);
int qemuDomainNamespaceTeardownInput(virDomainObj *vm,
virDomainInputDef *input);
So this fixes just <input/>. I know that we talked about other types and
that QEMU can open them multiple times, but I still thought that you
were going to make the rest of qemuDomainNamespaceSetup*() follow this
Input(). I mean, even though we did not see it in our testing, but the
hotplug code can still experience an error (for various reasons) in
which case we shouldn't try to remove the device from the namespace.
For instance: when hotplugging an USB device
qemuDomainAttachHostUSBDevice() is called and if say
qemuSetupHostdevCgroup() failed then the device would be removed from
namespace even though we might not have created it.
Having said that, I wonder if we can save a few lines of code if this
@created argument was set in qemuNamespaceMknodPaths() instead of having
the same pattern (if ret==0 then *created=true;) copied into all
qemuDomainNamespaceSetup*() functions. But I don't mind.
Michal
1233
days inactive
1235
days old
5 comments
3 participants
participants (3)
-
Jano Tomko -
Kristina Hanicova -
Michal Prívozník