[libvirt] [PATCH 2/2] qemu: don't relabel chardev source file if virtlogd is used

[Pavel Hrdina]

On Wed, May 24, 2017 at 12:25:51PM +0200, Martin Kletzander wrote:
> On Tue, May 23, 2017 at 05:18:01PM +0100, Daniel P. Berrange wrote:
> >On Tue, May 23, 2017 at 05:53:46PM +0200, Pavel Hrdina wrote:
> >> On Tue, May 23, 2017 at 04:24:13PM +0100, Daniel P. Berrange wrote:
> >> > On Tue, May 23, 2017 at 04:25:01PM +0200, Martin Kletzander wrote:
> >> > > On Mon, May 15, 2017 at 04:28:35PM +0200, Pavel Hrdina wrote:
> >> > > > If libvirt uses virtlogd instead of passing the file path directly
> >> > > > to QEMU we shouldn't relabel the chardev source file, otherwise
> >> > > > virtlogd will get a permission denied while reloading.
> >> > > >
> >> > > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=143098
> >> > > >
> >> > >
> >> > > You missed a digit and I'm too lazy to check 10 bugs for a reproducer ;)
> >> > >
> >> > > > Signed-off-by: Pavel Hrdina <phrdina at redhat.com>
> >> > > > ---
> >> > > > src/conf/domain_conf.c          | 20 ++++++++++++++++++++
> >> > > > src/conf/domain_conf.h          |  1 +
> >> > > > src/qemu/qemu_command.c         | 12 ++++++++----
> >> > > > src/security/security_dac.c     |  6 ++++++
> >> > > > src/security/security_selinux.c |  6 ++++++
> >> > > > 5 files changed, 41 insertions(+), 4 deletions(-)
> >> > > >
> >> > >
> >> > > I see you are changing the parser code, but you are not changing the
> >> > > Relax-NG schema, neither any test.
> >> > >
> >> > > > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> >> > > > index aa441fae3c..92f011d3a4 100644
> >> > > > --- a/src/conf/domain_conf.c
> >> > > > +++ b/src/conf/domain_conf.c
> >> > > > @@ -2064,6 +2064,7 @@ virDomainChrSourceDefCopy(virDomainChrSourceDefPtr dest,
> >> > > >     }
> >> > > >
> >> > > >     dest->type = src->type;
> >> > > > +    dest->skipRelabel = src->skipRelabel;
> >> > > >
> >> > > >     return 0;
> >> > > > }
> >> > > > @@ -10608,6 +10609,7 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
> >> > > >     char *append = NULL;
> >> > > >     char *haveTLS = NULL;
> >> > > >     char *tlsFromConfig = NULL;
> >> > > > +    char *skipRelabel = NULL;
> >> > > >     int remaining = 0;
> >> > > >
> >> > > >     while (cur != NULL) {
> >> > > > @@ -10628,6 +10630,8 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
> >> > > >                 case VIR_DOMAIN_CHR_TYPE_UNIX:
> >> > > >                     if (!append && def->type == VIR_DOMAIN_CHR_TYPE_FILE)
> >> > > >                         append = virXMLPropString(cur, "append");
> >> > > > +                    if (!skipRelabel && def->type == VIR_DOMAIN_CHR_TYPE_FILE)
> >> > > > +                        skipRelabel = virXMLPropString(cur, "skipRelabel");
> >> > >
> >> > > I'm guessing you want to keep this away from users, right?  You should
> >> > > not be parsing it from the XML then.  Or you should add a thing there
> >> > > that the XML supports.  Not just a random attribute.
> >> > >
> >> > > Either keep this data in private structure or even better, just add the
> >> > > same thing as you would do with:
> >> > >
> >> > >  <seclabel relabel="no"/>
> >> > >
> >> > > with all the details of course.  The user can see it, can supply it, old
> >> > > releases support it, all the stuff is there already.
> >> > >
> >> > > I'm open to suggestions, but NACK to random "wannabe private" attributes.
> >> >
> >> > The use of virtlogd affects many devices in guests. So we should just
> >> > record at the top level of the QEMU domain status, whether virtlogd
> >> > was used or not.
> >>
> >> That would be one possibility, however we need to decide what to do in
> >> one specific case:
> >>
> >> 1. stdio_handler is set to logd and QEMU_CAPS_CHARDEV_FILE_APPEND is
> >> supported
> >>
> >> 2. start a guest with one serial chardev
> >>
> >> 3. change the stdio_handler to file and restart libvirtd
> >>
> >> 4. hotplug another serial chardev (this is currently broken [1])
> >>
> >> There are to possible solution, we will store the usage of virtlogd
> >> domain-wide and virtlogd will be used for that domain even if the config
> >> option would change for the rest of that domain's life.  Or we need to
> >> store the usage of virtlogd per device and it will always depend on the
> >> config option.
> >
> >Having some devices use virtlogd and some devices not use virtlogd
> >is just a recipe for maximising confusion of developers, users, and
> >support people alike.
> >
> >We should record whether we used virtlogd when we first start the
> >guest, and honour that until QEMU is killed.
> >
> 
> This ^^ gets formatted in qemuDomainObjPrivateXMLFormat() which uses
> qemuDomainObjPrivatePtr; and that's one of the private structures you
> could keep it in.  Just to answer Pavel's question.

Now it makes sense what you've meant by private structure, I was
confused because we were talking about XML :).

> Also, just for info, is the related crasher in virtlogd fixed already or
> is someone working on that?

The related crasher is not fix to my knowledge and probably no-one is
working on it.

Pavel
-------------- 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/20170524/961f000e/attachment-0001.sig>