Re: [libvirt] [PATCH 1/2] qemu: Create hugepage path on per domain basis

On 25.11.2016 09:35, Martin Kletzander wrote: > On Tue, Nov 22, 2016 at 01:45:42PM +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 > > Surely you mean different than the default user from 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(a)redhat.com&gt; >> --- >> src/qemu/qemu_command.c | 4 +- >> src/qemu/qemu_conf.c | 45 >> ++++++++++++++++------ >> src/qemu/qemu_conf.h | 16 +++++--- >> src/qemu/qemu_driver.c | 19 +++------ >> src/qemu/qemu_process.c | 25 +++++++++++- >> .../qemuxml2argv-hugepages-numa.args | 4 +- >> .../qemuxml2argv-hugepages-pages.args | 14 +++---- >> .../qemuxml2argv-hugepages-pages2.args | 2 +- >> .../qemuxml2argv-hugepages-pages3.args | 2 +- >> .../qemuxml2argv-hugepages-pages5.args | 2 +- >> .../qemuxml2argv-hugepages-shared.args | 12 +++--- >> tests/qemuxml2argvdata/qemuxml2argv-hugepages.args | 2 +- >> .../qemuxml2argv-memory-hotplug-dimm-addr.args | 4 +- >> .../qemuxml2argv-memory-hotplug-dimm.args | 4 +- >> 14 files changed, 97 insertions(+), 58 deletions(-) >> >> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c >> index 0ed88f5..942ad86 100644 >> --- a/src/qemu/qemu_conf.c >> +++ b/src/qemu/qemu_conf.c >> @@ -1468,8 +1468,26 @@ qemuGetHugepagePath(virHugeTLBFSPtr hugepage) >> } >> >> >> +char * >> +qemuGetDomainHugepagePath(const virDomainDef *def, >> + virHugeTLBFSPtr hugepage) >> +{ >> + char *base = qemuGetBaseHugepagePath(hugepage); >> + char *ret; >> + >> + if (!base || >> + virAsprintf(&ret, "%s/%s", base, def->name) < 0) { >> + VIR_FREE(base); >> + return NULL; >> + } >> + >> + return ret; >> +} >> + > > You can't simply user the name because our restrictions for the name are > too lax. You should get unique directory name usable for this using > virDomainObjGetShortName() to make sure the creation doesn't fail. I thought that when we are using plain domain name for storing domain status XML or pid file that I'm safe here too. But okay, I can change it. I jut hope that by the time the command line is built domain already has id allocated.

> > However, that reminds me that you might need to deal with similar thing > I had to deal with when adding per-domain subdirectories for private > domain paths. You should save the path (or at least the information > that the newer path is used) in the domain object and save/restore it > in/from the state XML. The way it's implemented now will break for > example hotplug of hugepage-backed memory after libvirt upgrade. Not really. We don't expose the path anywhere, and whenever it is needed we construct it. I've tested this and basically the only problem I ran into was that we don't build the path on domain hotplug (rather than on domain startup), but it is trivial to fix.

Michal