Conversely ...

If I set an environment variable that I want my session-
specific <emulator> to get, then it only works on the
first libvirt session.  Subsequent environment variable
settings are ignored (or rather, the old value is used).

This kind of behaviour is inherant to the launch-on-first-use approach to starting libvirtd. In general you should not expect the <emulator> to inherit any environment variables from libvirtd, since libvirt is generally aiming to scrub the environment before starting QEMU. Any per-VM data needs to be passed exclusively via the libvirt guest XML.

I think there's something more needed from libvirt here.
I'll start a thread on libvir-list.

Actually forget that.  I spotted the <qemu:env/> element
http://libvirt.org/drvqemu.html#qemucommand
which will probably let us pass the environment variables
down to qemu.

NB usage of that element will mark the guest as tainted, and thus unsupported on RHEL. That's only provided as way to implement a short-term workaround for a missing feature, while a supportable solution can be developed.

This commit sets <qemu:env name="TMPDIR" value="..."/>
when that is appropriate:

https://github.com/libguestfs/libguestfs/commit/f9f0767e20847734db3747c06b4ff11729a62a07

At some point, need to go through this function and
file bugs for everything that libvirt will need to
support for RHEL 7:

https://github.com/libguestfs/libguestfs/blob/f9f0767e20847734db3747c06b4ff11729a62a07/src/launch-libvirt.c#L1007

Rich, did your patches in libvirt.git resolve this to your liking, or is there still something missing that this bug should track?

It's more of a design flaw than anything else.  I worked around
this in libguestfs by always setting TMPDIR.  My libvirt patch
did NOT resolve the issue in libvirt alone however.

(In reply to comment #8)
> It's more of a design flaw than anything else.  I worked around
> this in libguestfs by always setting TMPDIR.  My libvirt patch
> did NOT resolve the issue in libvirt alone however.

Plus there is the fact that using <qemu:env>, the only
sensible workaround, marks the guest as tainted and
unsupported.

Okay, moving to the upstream tracker then, since this doesn't have particular bearing on fedora and likely isn't trivial to fix.

Rich, is this still relevant? If so, can you summarize what you want from libvirt here? Context is split around old mailing list postings

It's still a bug / design flaw in libvirt, but one which we have
worked around in libguestfs for a long time.

The problem is that session libvirtd is shared per user, so the
effect of setting an environment variable or rlimit is different
depending on whether libvirtd is currently running or not.

  libvirtd not running:
    call libvirt virConnectOpen
      libvirtd starts up, inherits caller's environment, rlimits

  libvirtd already running:
    call libvirt virConnect Open
      reuse existing libvirtd
      caller's environment, rlimits are ignored

The most important environment variable was $TMPDIR, since it
controls where qemu puts snapshots.  However we mitigate that
now using <qemu:env>, and in any case we no longer use qemu's
snapshot features (we create overlays manually in places we control).

rlimits have also been an issue, especially when you want to
control core dump behaviour.

Note that using <qemu:env> means all libguestfs domains are "tainted",
but we basically ignore that when it comes to support.

*** Bug 865464 has been marked as a duplicate of this bug. ***

I agree with dan's comment from

(In reply to Daniel Berrange from comment #9)
> I don't think this is a libvirt bug really, although the interaction here is
> certainly undesirable. If the user has set a custom TMPDIR env for their
> login session, applications (including libvirtd) should be honouring that
> variable.

Just ignoring parent process TMPDIR isn't an option IMO, that's definitely unexpected. We want 'virsh qemu:///session' or gnome-boxes to pick up a user's custom TMPDIR.

So what's the possible fix here?

Does libguestfs just specify TMPDIR to qemu override the potentially missing libvirtd TMPDIR? What qemu/libvirt usages of TMPDIR are required here anyways?

(In reply to Cole Robinson from comment #14)
> I agree with dan's comment from
> 
> (In reply to Daniel Berrange from comment #9)
> > I don't think this is a libvirt bug really, although the interaction here is
> > certainly undesirable. If the user has set a custom TMPDIR env for their
> > login session, applications (including libvirtd) should be honouring that
> > variable.
> 
> Just ignoring parent process TMPDIR isn't an option IMO, that's definitely
> unexpected.

I don't necessarily agree with this statement, but anyway ...

> We want 'virsh qemu:///session' or gnome-boxes to pick up a
> user's custom TMPDIR.

That's true.

> So what's the possible fix here?
> 
> Does libguestfs just specify TMPDIR to qemu override the potentially missing
> libvirtd TMPDIR?

Yes, libguestfs always sets the <qemu:env> for TMPDIR:

https://github.com/libguestfs/libguestfs/blob/master/src/launch-libvirt.c#L1712

The drawback is this taints the libvirt domain (although of course we
ignore this for the purposes of RHEL support cases).

A short term fix would be not to taint session domains where the caller
uses <qemu:env> (and perhaps rename <qemu:env> as something less
specific, since this affects all session libvirtd instances), since
using <qemu:env> is unavoidable.

> What qemu/libvirt usages of TMPDIR are required here
> anyways?

Any file that qemu may create in $TMPDIR.  It used to be that
qemu created all snapshot files there (for the '-drive snapshot=on'
option) and that was the most pressing problem for libguestfs since
those files can be massive.

I can't recall how we got libvirt to use snapshot=on, but in any
case for various reasons we never use that option now.  Instead we
create our own overlays.  However qemu still creates other temporary
files.

Of course the above discussion applies to any environment variable that
libvirtd doesn't cleanse from its environment and that qemu (or libvirtd)
may use.

FWIW I'm just trying to distill this to something actionable

> 
> > We want 'virsh qemu:///session' or gnome-boxes to pick up a
> > user's custom TMPDIR.
> 
> That's true.
> 
> > So what's the possible fix here?
> > 
> > Does libguestfs just specify TMPDIR to qemu override the potentially missing
> > libvirtd TMPDIR?
> 
> 
> A short term fix would be not to taint session domains where the caller
> uses <qemu:env> (and perhaps rename <qemu:env> as something less
> specific, since this affects all session libvirtd instances), since
> using <qemu:env> is unavoidable.
> 

Okay, that's a potential actionable item. Don't taint the VM if TMPDIR is passed, or possibly provide an official XML way to override TMPDIR

> > What qemu/libvirt usages of TMPDIR are required here
> > anyways?
> 
> Any file that qemu may create in $TMPDIR.  It used to be that
> qemu created all snapshot files there (for the '-drive snapshot=on'
> option) and that was the most pressing problem for libguestfs since
> those files can be massive.
> 
> I can't recall how we got libvirt to use snapshot=on, but in any
> case for various reasons we never use that option now.  Instead we
> create our own overlays.  However qemu still creates other temporary
> files.
> 

Libvirt doesn't have explicit snapshot=on support, so it may have predated libguestfs libvirt backend, or used qemu:commandline XML

I don't know see any other usages of TMPDIR qemu has that would effect typical libguests/libvirt usecases, and I wouldn't be surprised if selinux policy prevents qemu from creating files in /tmp . Dependent libraries might use it though .But in practice there may not be anything that should be using TMPDIR now

> Of course the above discussion applies to any environment variable that
> libvirtd doesn't cleanse from its environment and that qemu (or libvirtd)
> may use.

I'm confused then; above you agreed that 'virsh qemu:///session' or gnome-boxes should pick up a user's custom TMPDIR, but this advocates scrubbing TMPDIR. Do you mean scrubbing TMPDIR when we launch qemu? That also seems unexpected though... what are we supposed to scrub it to? Why shouldn't it abide TMPDIR from the user's session like other well behaving apps tend to do?

Or am I misunderstanding?

(In reply to Cole Robinson from comment #16)
> FWIW I'm just trying to distill this to something actionable
> 
> > 
> > > We want 'virsh qemu:///session' or gnome-boxes to pick up a
> > > user's custom TMPDIR.
> > 
> > That's true.
> > 
> > > So what's the possible fix here?
> > > 
> > > Does libguestfs just specify TMPDIR to qemu override the potentially missing
> > > libvirtd TMPDIR?
> > 
> > 
> > A short term fix would be not to taint session domains where the caller
> > uses <qemu:env> (and perhaps rename <qemu:env> as something less
> > specific, since this affects all session libvirtd instances), since
> > using <qemu:env> is unavoidable.
> > 
> 
> Okay, that's a potential actionable item. Don't taint the VM if TMPDIR is
> passed, or possibly provide an official XML way to override TMPDIR

I don't think we should really special case TMPDIR as I think its attacking a symptom rather than the cause.

> 
> > > What qemu/libvirt usages of TMPDIR are required here
> > > anyways?
> > 
> > Any file that qemu may create in $TMPDIR.  It used to be that
> > qemu created all snapshot files there (for the '-drive snapshot=on'
> > option) and that was the most pressing problem for libguestfs since
> > those files can be massive.
> > 
> > I can't recall how we got libvirt to use snapshot=on, but in any
> > case for various reasons we never use that option now.  Instead we
> > create our own overlays.  However qemu still creates other temporary
> > files.
> > 
> 
> Libvirt doesn't have explicit snapshot=on support, so it may have predated
> libguestfs libvirt backend, or used qemu:commandline XML
> 
> I don't know see any other usages of TMPDIR qemu has that would effect
> typical libguests/libvirt usecases, and I wouldn't be surprised if selinux
> policy prevents qemu from creating files in /tmp . Dependent libraries might
> use it though .But in practice there may not be anything that should be
> using TMPDIR now

Missing libvirt support for snapshot=on is really the main item we need to address - we have an open RFE for this already I believe. More generally we should ensure that *nothing* in QEMU uses TMPDIR for anything consequential. That QEMU was ever using /tmp for snapshots is a major design flaw in QEMU. We ought to have libvirt let the app set a filename for the snapshot so libvirt can label it and ensure its deleted on shutdown, if QEMU crashes without cleaning it up.

I'm fairly sure there's no other significant uses of TMPDIR, but if we find some, we should address them as they come up with new bugs.

In general the behaviour of libvirtd session whereby it inherits the env variables of the callers is intended so we inherit the right env for the user's login session.

TMPDIR is used whenever qemu calls mkstemp and related functions.
And libvirtd too, I guess.

Plus any library that qemu or libvirtd uses could be affected by
environment variables.  Ceph, as one example, uses TMPDIR and
CEPH_* environment variables.

The problem in this bug is that if you run, for example:

  CEPH_CONF=/my/ceph.conf guestfish -a rbd:///pool/disk

then it's pot luck whether or not you happen to run a new
libvirtd session instance (or there is one already running),
and so it's random whether the CEPH_CONF environment variable
is passed through to qemu or ignored.

This is IMO a design flaw.

(In reply to Richard W.M. Jones from comment #18)
> TMPDIR is used whenever qemu calls mkstemp and related functions.
> And libvirtd too, I guess.
> 
> Plus any library that qemu or libvirtd uses could be affected by
> environment variables.  Ceph, as one example, uses TMPDIR and
> CEPH_* environment variables.
> 
> The problem in this bug is that if you run, for example:
> 
>   CEPH_CONF=/my/ceph.conf guestfish -a rbd:///pool/disk
> 
> then it's pot luck whether or not you happen to run a new
> libvirtd session instance (or there is one already running),
> and so it's random whether the CEPH_CONF environment variable
> is passed through to qemu or ignored.
> 
> This is IMO a design flaw.

The design strategy for libvirt is that anything that can affect the guest configuration should be explicitly represented in the XML. Setting CEPH_CONF in particular is something that should not have any impact on the guests - we should be encoding any ceph config params that QEMU needs in the <disk> XML.

So this stalled again without any concrete suggestions...

As you said in comment 10:

> Okay, that's a potential actionable item. Don't taint the VM if
> TMPDIR is passed, or possibly provide an official XML way to
> override TMPDIR

It seems quite obvious (to me at least) that setting at least
a whitelist of environment variables shouldn't taint the domain.
Libguestfs only needs TMPDIR on that whitelist.

Libvirt should probably also clean the environment that it
passes to the first libvirtd, so that random environment
variables like CEPH_CONF don't affect other instances.

Then there's the question of checking every library that libvirtd
uses for environment usage and encoding anything that is useful into
the XML, which I appreciate is a huge amount of work.

IMHO the TMPDIR should be explicitly set to a private directory for the particular domain (which libvirt now creates) and for running the process with existing environment it would be best to use the embed way of running qemu.  Other than that there should be as much scrubbed as possible when launching the daemon as a sub-process, except things that we cannot revert (e.g. ulimit).

Partly related to this, we need to move to using systemd to spawn session libvirtd: https://gitlab.com/libvirt/libvirt/-/issues/21

This will prevent any inheritance/leaking of env variables from the calling proces into libvirtd, giving a more predictable env like we have for the system instance.

Thank you for reporting this issue to the libvirt project. Unfortunately we have been unable to resolve this issue due to insufficient maintainer capacity and it will now be closed. This is not a reflection on the possible validity of the issue, merely the lack of resources to investigate and address it, for which we apologise. If you none the less feel the issue is still important, you may choose to report it again at the new project issue tracker https://gitlab.com/libvirt/libvirt/-/issues The project also welcomes contribution from anyone who believes they can provide a solution.