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

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(a)redhat.com&gt; >> > > > --- >> > > > 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.

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