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;

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