[libvirt] [PATCH v2 2/3] qemu: Create hugepage path on per domain basis

[Ján Tomko]

On Tue, Nov 29, 2016 at 10:31:12AM +0100, Michal Privoznik wrote:
>If you've ever tried running a huge page backed guest under
>different user than root, you probably failed. Problem is even

Should this be: different than the user in qemu.conf?

>though we have corresponding APIs in the security drivers,
>there's no implementation and thus we don't relabel the huge page
>path. But even if we did, so far all of the domains share the
>same path:
>
>   /hugepageMount/libvirt/qemu
>
>Our only option there would be to set 0777 mode on the qemu dir
>which is totally unsafe. Therefore, we can create dir on
>per-domain basis, i.e.:
>
>   /hugepageMount/libvirt/qemu/domainName
>
>and chown domainName dir to the user that domain is configured to
>run under.
>
>Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>---
> src/qemu/qemu_command.c                            |  4 +-
> src/qemu/qemu_conf.c                               | 44 +++++++++----
> src/qemu/qemu_conf.h                               | 16 +++--
> src/qemu/qemu_driver.c                             | 19 ++----
> src/qemu/qemu_process.c                            | 74 ++++++++++++++++------
> .../qemuxml2argv-hugepages-numa.args               |  6 +-
> .../qemuxml2argv-hugepages-pages.args              | 16 ++---
> .../qemuxml2argv-hugepages-pages2.args             |  2 +-
> .../qemuxml2argv-hugepages-pages3.args             |  2 +-
> .../qemuxml2argv-hugepages-pages5.args             |  2 +-
> .../qemuxml2argv-hugepages-shared.args             | 14 ++--
> tests/qemuxml2argvdata/qemuxml2argv-hugepages.args |  2 +-
> .../qemuxml2argv-memory-hotplug-dimm-addr.args     |  4 +-
> .../qemuxml2argv-memory-hotplug-dimm.args          |  4 +-
> 14 files changed, 131 insertions(+), 78 deletions(-)
>

>@@ -3221,6 +3221,55 @@ qemuProcessReconnectCheckMemAliasOrderMismatch(virDomainObjPtr vm)
> }
>
>
>+static int
>+qemuProcessBuildDestroydHugepagesPath(virQEMUDriverPtr driver,

Extra 'd' after Destroy.

>+                                      virDomainObjPtr vm,

Only vm->def is needed.

>+                                      bool build)

This would look nicer split in two functions. And the 'Destroy' one can
be void and best-effort.

>+{
>+    virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>+    char *hugepagePath = NULL;
>+    size_t i;
>+    int ret = -1;
>+
>+    if (vm->def->mem.nhugepages) {
>+        for (i = 0; i < cfg->nhugetlbfs; i++) {
>+            VIR_FREE(hugepagePath);
>+            hugepagePath = qemuGetDomainHugepagePath(vm->def, &cfg->hugetlbfs[i]);
>+
>+            if (!hugepagePath)
>+                goto cleanup;

In theory, this does not need to be an error for the Destroy path, we
can just continue to the next mount. In practice, this will never fail.

>+
>+            if (build) {
>+                if (virFileMakePathWithMode(hugepagePath, 0700) < 0) {
>+                    virReportSystemError(errno,
>+                                         _("Unable to create %s"),
>+                                         hugepagePath);
>+                    goto cleanup;
>+                }
>+
>+                if (virSecurityManagerSetHugepages(driver->securityManager,
>+                                                   vm->def, hugepagePath) < 0) {
>+                    virReportError(VIR_ERR_INTERNAL_ERROR,
>+                                   "%s", _("Unable to set huge path in security driver"));

Pre-existing error message oddness.

>+                    VIR_FREE(hugepagePath);

Already done in 'cleanup'.

>+                    goto cleanup;
>+                }
>+            } else {
>+                if (rmdir(hugepagePath) < 0)
>+                    VIR_WARN("Unable to remove hugepage path: %s (errno=%d)",
>+                             hugepagePath, errno);

Can be VIR_ERROR, this will never happen(TM).

>+            }
>+        }
>+    }
>+
>+    ret = 0;
>+ cleanup:
>+    VIR_FREE(hugepagePath);
>+    virObjectUnref(cfg);
>+    return ret;
>+}
>+
>+

ACK

Jan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20161207/9bf1a9fa/attachment-0001.sig>