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@xxxxxxxxxx> > >> > > > --- > >> > > > 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
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
