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
Signed-off-by: Pavel Hrdina <phrdina@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(-)
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");
/* PTY path is only parsed from live xml. */
if (!path &&
(def->type != VIR_DOMAIN_CHR_TYPE_PTY ||
@@ -10726,6 +10730,17 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
_("Invalid append attribute value '%s'"), append);
goto error;
}
+ if (skipRelabel && def->type == VIR_DOMAIN_CHR_TYPE_FILE &&
+ (flags & VIR_DOMAIN_DEF_PARSE_STATUS)) {
+ if (STREQ(skipRelabel, "yes")) {
+ def->skipRelabel = true;
+ } else {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("invalid 'skipRelabel' attribute value '%s'"),
+ skipRelabel);
+ goto error;
+ }
+ }
if (!path &&
def->type != VIR_DOMAIN_CHR_TYPE_PTY) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
@@ -10902,6 +10917,7 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
VIR_FREE(logfile);
VIR_FREE(haveTLS);
VIR_FREE(tlsFromConfig);
+ VIR_FREE(skipRelabel);
return remaining;
@@ -22324,6 +22340,10 @@ virDomainChrSourceDefFormat(virBufferPtr buf,
def->data.file.append != VIR_TRISTATE_SWITCH_ABSENT)
virBufferAsprintf(buf, " append='%s'",
virTristateSwitchTypeToString(def->data.file.append));
+ if ((flags & VIR_DOMAIN_DEF_FORMAT_STATUS) &&
+ def->type == VIR_DOMAIN_CHR_TYPE_FILE && def->skipRelabel) {
+ virBufferAddLit(buf, " skipRelabel='yes'");
+ }
virDomainSourceDefFormatSeclabel(buf, nseclabels, seclabels, flags);
}
break;
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 09fb7aada4..329eb90392 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1166,6 +1166,7 @@ struct _virDomainChrSourceDef {
} data;
char *logfile;
int logappend;
+ bool skipRelabel;
};
/* A complete character device, both host and domain views. */
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 813a8515c0..0625075bb2 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4998,6 +4998,7 @@ static int
qemuBuildChrChardevFileStr(virLogManagerPtr logManager,
virCommandPtr cmd,
const virDomainDef *def,
+ virDomainChrSourceDefPtr sourceDef,
virBufferPtr buf,
const char *filearg, const char *fileval,
const char *appendarg, int appendval)
@@ -5011,6 +5012,9 @@ qemuBuildChrChardevFileStr(virLogManagerPtr logManager,
appendval == VIR_TRISTATE_SWITCH_OFF)
flags |= VIR_LOG_MANAGER_PROTOCOL_DOMAIN_OPEN_LOG_FILE_TRUNCATE;
+ if (sourceDef)
+ sourceDef->skipRelabel = true;
+
if ((logfd = virLogManagerDomainOpenLogFile(logManager,
"qemu",
def->uuid,
@@ -5051,7 +5055,7 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager,
virCommandPtr cmd,
virQEMUDriverConfigPtr cfg,
const virDomainDef *def,
- const virDomainChrSourceDef *dev,
+ virDomainChrSourceDefPtr dev,
const char *alias,
virQEMUCapsPtr qemuCaps,
bool nowait)
@@ -5093,7 +5097,7 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager,
goto cleanup;
}
if (qemuBuildChrChardevFileStr(virQEMUCapsGet(qemuCaps, QEMU_CAPS_CHARDEV_FILE_APPEND) ?
- logManager : NULL, cmd, def, &buf,
+ logManager : NULL, cmd, def, dev, &buf,
"path", dev->data.file.path,
"append", dev->data.file.append) < 0)
goto cleanup;
@@ -5209,7 +5213,7 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager,
_("logfile not supported in this QEMU binary"));
goto cleanup;
}
- if (qemuBuildChrChardevFileStr(logManager, cmd, def, &buf,
+ if (qemuBuildChrChardevFileStr(logManager, cmd, def, NULL, &buf,
"logfile", dev->logfile,
"logappend", dev->logappend) < 0)
goto cleanup;
@@ -5573,7 +5577,7 @@ qemuBuildMonitorCommandLine(virLogManagerPtr logManager,
virQEMUDriverConfigPtr cfg,
virDomainDefPtr def,
virQEMUCapsPtr qemuCaps,
- const virDomainChrSourceDef *monitor_chr,
+ virDomainChrSourceDefPtr monitor_chr,
bool monitor_json)
{
char *chrdev;
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 922e484942..a4e02ca8bc 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -1196,6 +1196,9 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr,
if (chr_seclabel && !chr_seclabel->relabel)
return 0;
+ if (!chr_seclabel && dev_source->skipRelabel)
+ return 0;
+
if (chr_seclabel && chr_seclabel->label) {
if (virParseOwnershipIds(chr_seclabel->label, &user, &group) < 0)
return -1;
@@ -1276,6 +1279,9 @@ virSecurityDACRestoreChardevLabel(virSecurityManagerPtr mgr,
if (chr_seclabel && !chr_seclabel->relabel)
return 0;
+ if (!chr_seclabel && dev_source->skipRelabel)
+ return 0;
+
switch ((virDomainChrType) dev_source->type) {
case VIR_DOMAIN_CHR_TYPE_DEV:
case VIR_DOMAIN_CHR_TYPE_FILE:
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index 612dbc2a83..64ab2795d5 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -2216,6 +2216,9 @@ virSecuritySELinuxSetChardevLabel(virSecurityManagerPtr mgr,
if (chr_seclabel && !chr_seclabel->relabel)
return 0;
+ if (!chr_seclabel && dev_source->skipRelabel)
+ return 0;
+
if (chr_seclabel)
imagelabel = chr_seclabel->label;
if (!imagelabel)
@@ -2289,6 +2292,9 @@ virSecuritySELinuxRestoreChardevLabel(virSecurityManagerPtr mgr,
if (chr_seclabel && !chr_seclabel->relabel)
return 0;
+ if (!chr_seclabel && dev_source->skipRelabel)
+ return 0;
+
switch (dev_source->type) {
case VIR_DOMAIN_CHR_TYPE_DEV:
case VIR_DOMAIN_CHR_TYPE_FILE:
--
2.13.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
> > 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"/>
What sense would it make in this case? There is only one option when we're
running virtlogd and that is no label. If they supply relabel='yes', it won't
work with virtlogd unless they change it back to 'no'. So, you've got an option
the functionality of which depends on a service running, which is by default on
by the way.
Since I don't want to be someone who just criticizes other people's ideas
without adding anything to the discussion, I was thinking about re-using the
qemu config parameter stdioLogD. But that solution is also wrong as it turned
out. I haven't come up with anything better yet, though. So, having something in
the status XML seems like a plausible approach to me.
Erik
>
> 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.
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, May 23, 2017 at 05:13:17PM +0200, Erik Skultety wrote:
>> > 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"/>
>
>What sense would it make in this case? There is only one option when we're
>running virtlogd and that is no label. If they supply relabel='yes', it won't
>work with virtlogd unless they change it back to 'no'. So, you've got an option
>the functionality of which depends on a service running, which is by default on
>by the way.
>
If that's so clear, why put it in the XML? Why not just set it in the
private structure then? Either there is a reason for it to be variable
or not. If there is not, then we're effectively talking about something
like <domain doNotFailOnStart='yes'> attribute. There is no need for
such thing in that case.
Sure, I might be wrong. In that case, this commit needs more explanation.
>Since I don't want to be someone who just criticizes other people's ideas
>without adding anything to the discussion, I was thinking about re-using the
>qemu config parameter stdioLogD. But that solution is also wrong as it turned
>out. I haven't come up with anything better yet, though. So, having something in
>the status XML seems like a plausible approach to me.
>
>Erik
>
>>
>> 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.
>
>
>
>> --
>> libvir-list mailing list
>> libvir-list@redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list
>
>--
>libvir-list mailing list
>libvir-list@redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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.
[1] The hotplug of char devices doesn't use virtlogd at all and we don't
relabel the files to be accessible by QEMU.
Pavel
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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.
Also, just for info, is the related crasher in virtlogd fixed already or
is someone working on that?
>Regards,
>Daniel
>--
>|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
>|: https://libvirt.org -o- https://fstop138.berrange.com :|
>|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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 ;)
Yep, there should by additional 8 :)
> >Signed-off-by: Pavel Hrdina <phrdina@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.
We need to store it in the status XML so we don't lose the information
while restarting libvirtd. And I want to keep it hidden from users
because they shouldn't care about it. The decision whether we will use
virtlogd as stdio handler is made by checking the qemu.conf for
"stdio_handler" and whether QEMU supports appending to files.
> Either keep this data in private structure or even better, just add the
What do you mean by private structure?
> 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.
This for user to specify it, not to store our internal private
information that should survive restarting libvirtd.
> I'm open to suggestions, but NACK to random "wannabe private" attributes.
The only reason why I used the new private attribute is that we already
do it for graphics device (fromConfig and autoGenerated) and for
TCP character device (tlsFromConfig).
I can store it in the status part of the status XML outside of the
domain part, but that would require mapping it to the actual character
device.
Pavel
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.