11:10 a.m.
Shorten some paths and kill the whole process group instead
of just the parent.
Ján Tomko (4):
qemu: virtiofs: kill the whole process group when stopping
qemu: virtiofs: shorten pid filename
qemu: virtiofs: shorten socket filename
qemu: virtiofs: use a .log.fs suffix for logs
src/qemu/qemu_virtiofs.c | 14 +++++---------
1 file changed, 5 insertions(+), 9 deletions(-)
--
2.25.1
11:10 a.m.
New subject: [libvirt PATCH 1/4] qemu: virtiofs: kill the whole process group when stopping
After startup, virtiofds forks itself to drop its privileges.
Kill the whole process group instead of just the parent.
https://bugzilla.redhat.com/show_bug.cgi?id=1808697
Signed-off-by: Ján Tomko <jtomko(a)redhat.com>
Reported-by: Andrew Jones <drjones(a)redhat.com>
---
src/qemu/qemu_virtiofs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_virtiofs.c b/src/qemu/qemu_virtiofs.c
index d579ce1d33..d6159206eb 100644
--- a/src/qemu/qemu_virtiofs.c
+++ b/src/qemu/qemu_virtiofs.c
@@ -285,7 +285,7 @@ qemuVirtioFSStop(virQEMUDriverPtr driver G_GNUC_UNUSED,
rc = virPidFileReadPathIfAlive(pidfile, &pid, NULL);
if (rc >= 0 && pid != (pid_t) -1)
- virProcessKillPainfully(pid, true);
+ virProcessKillPainfully(-pid, true);
if (unlink(pidfile) < 0 &&
errno != ENOENT) {
--
2.25.1
11:41 a.m.
New subject: [libvirt PATCH 1/4] qemu: virtiofs: kill the whole process group when stopping
On 23. 3. 2020 17:10, Ján Tomko wrote:
After startup, virtiofds forks itself to drop its privileges.
Kill the whole process group instead of just the parent.
https://bugzilla.redhat.com/show_bug.cgi?id=1808697
Signed-off-by: Ján Tomko <jtomko(a)redhat.com>
Reported-by: Andrew Jones <drjones(a)redhat.com>
---
src/qemu/qemu_virtiofs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_virtiofs.c b/src/qemu/qemu_virtiofs.c
index d579ce1d33..d6159206eb 100644
--- a/src/qemu/qemu_virtiofs.c
+++ b/src/qemu/qemu_virtiofs.c
@@ -285,7 +285,7 @@ qemuVirtioFSStop(virQEMUDriverPtr driver G_GNUC_UNUSED,
rc = virPidFileReadPathIfAlive(pidfile, &pid, NULL);
if (rc >= 0 && pid != (pid_t) -1)
- virProcessKillPainfully(pid, true);
+ virProcessKillPainfully(-pid, true);
if (unlink(pidfile) < 0 &&
errno != ENOENT) {
Ah, this means that other places are affected too (e.g. qemu-pr-helper,
which shouldn't fork(), but libvirt can just use kill(-pid) to be sure).
But since I've posted a patch that will remove these lines and replace
them with a simple virPidFileForceCleanupPath(), we can do that in one
place for the benefit of others.
Michal
12:07 p.m.
New subject: [libvirt PATCH 1/4] qemu: virtiofs: kill the whole process group when stopping
On a Monday in 2020, Michal Prívozník wrote:
On 23. 3. 2020 17:10, Ján Tomko wrote:
> After startup, virtiofds forks itself to drop its privileges.
> Kill the whole process group instead of just the parent.
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1808697
>
> Signed-off-by: Ján Tomko <jtomko(a)redhat.com>
> Reported-by: Andrew Jones <drjones(a)redhat.com>
> ---
> src/qemu/qemu_virtiofs.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/qemu/qemu_virtiofs.c b/src/qemu/qemu_virtiofs.c
> index d579ce1d33..d6159206eb 100644
> --- a/src/qemu/qemu_virtiofs.c
> +++ b/src/qemu/qemu_virtiofs.c
> @@ -285,7 +285,7 @@ qemuVirtioFSStop(virQEMUDriverPtr driver G_GNUC_UNUSED,
>
> rc = virPidFileReadPathIfAlive(pidfile, &pid, NULL);
> if (rc >= 0 && pid != (pid_t) -1)
> - virProcessKillPainfully(pid, true);
> + virProcessKillPainfully(-pid, true);
>
> if (unlink(pidfile) < 0 &&
> errno != ENOENT) {
>
Ah, this means that other places are affected too (e.g. qemu-pr-helper,
which shouldn't fork(), but libvirt can just use kill(-pid) to be sure).
I just realized that virProcessKillPainfully goes down all the way to
virProcessKill which ignores pids <= 1, so this patch is not doing what
it should.
Jano
But since I've posted a patch that will remove these lines and
replace
them with a simple virPidFileForceCleanupPath(), we can do that in one
place for the benefit of others.
Michal
11:10 a.m.
New subject: [libvirt PATCH 2/4] qemu: virtiofs: shorten pid filename
There is no need to repeat the shortName, since it's
already present in the directory path.
Also use just 'fs' instead of 'virtiofsd'.
Signed-off-by: Ján Tomko <jtomko(a)redhat.com>
Suggested-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/qemu/qemu_virtiofs.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/src/qemu/qemu_virtiofs.c b/src/qemu/qemu_virtiofs.c
index d6159206eb..2e3535873d 100644
--- a/src/qemu/qemu_virtiofs.c
+++ b/src/qemu/qemu_virtiofs.c
@@ -42,13 +42,9 @@ qemuVirtioFSCreatePidFilename(virDomainObjPtr vm,
const char *alias)
{
qemuDomainObjPrivatePtr priv = vm->privateData;
- g_autofree char *shortName = NULL;
g_autofree char *name = NULL;
- if (!(shortName = virDomainDefGetShortName(vm->def)))
- return NULL;
-
- name = g_strdup_printf("%s-%s-virtiofsd", shortName, alias);
+ name = g_strdup_printf("%s-fs", alias);
return virPidFileBuildPath(priv->libDir, name);
}
--
2.25.1
11:10 a.m.
New subject: [libvirt PATCH 3/4] qemu: virtiofs: shorten socket filename
Use just 'fs' instead of 'virtiofsd'.
Signed-off-by: Ján Tomko <jtomko(a)redhat.com>
---
src/qemu/qemu_virtiofs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_virtiofs.c b/src/qemu/qemu_virtiofs.c
index 2e3535873d..aaa25bcbb8 100644
--- a/src/qemu/qemu_virtiofs.c
+++ b/src/qemu/qemu_virtiofs.c
@@ -56,7 +56,7 @@ qemuVirtioFSCreateSocketFilename(virDomainObjPtr vm,
{
qemuDomainObjPrivatePtr priv = vm->privateData;
- return virFileBuildPath(priv->libDir, alias, "-virtiofsd.sock");
+ return virFileBuildPath(priv->libDir, alias, "-fs.sock");
}
--
2.25.1
11:10 a.m.
New subject: [libvirt PATCH 4/4] qemu: virtiofs: use a .log.fs suffix for logs
As Dave pointed out, someone creative might name a domain
to make its logfile conflict with a logfile of another
domain's virtiofsd log.
Signed-off-by: Ján Tomko <jtomko(a)redhat.com>
Reported-by: Dr. David Alan Gilbert <dgilbert(a)redhat.com>
---
src/qemu/qemu_virtiofs.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_virtiofs.c b/src/qemu/qemu_virtiofs.c
index aaa25bcbb8..575b47fd74 100644
--- a/src/qemu/qemu_virtiofs.c
+++ b/src/qemu/qemu_virtiofs.c
@@ -67,9 +67,9 @@ qemuVirtioFSCreateLogFilename(virQEMUDriverConfigPtr cfg,
{
g_autofree char *name = NULL;
- name = g_strdup_printf("%s-%s", def->name, alias);
+ name = g_strdup_printf("%s-%s.log.fs", def->name, alias);
- return virFileBuildPath(cfg->logDir, name, "-virtiofsd.log");
+ return virFileBuildPath(cfg->logDir, name, NULL);
}
--
2.25.1
11:41 a.m.
New subject: [libvirt PATCH 4/4] qemu: virtiofs: use a .log.fs suffix for logs
On 23. 3. 2020 17:10, Ján Tomko wrote:
As Dave pointed out, someone creative might name a domain
to make its logfile conflict with a logfile of another
domain's virtiofsd log.
Signed-off-by: Ján Tomko <jtomko(a)redhat.com>
Reported-by: Dr. David Alan Gilbert <dgilbert(a)redhat.com>
---
src/qemu/qemu_virtiofs.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_virtiofs.c b/src/qemu/qemu_virtiofs.c
index aaa25bcbb8..575b47fd74 100644
--- a/src/qemu/qemu_virtiofs.c
+++ b/src/qemu/qemu_virtiofs.c
@@ -67,9 +67,9 @@ qemuVirtioFSCreateLogFilename(virQEMUDriverConfigPtr cfg,
{
g_autofree char *name = NULL;
- name = g_strdup_printf("%s-%s", def->name, alias);
+ name = g_strdup_printf("%s-%s.log.fs", def->name, alias);
- return virFileBuildPath(cfg->logDir, name, "-virtiofsd.log");
+ return virFileBuildPath(cfg->logDir, name, NULL);
}
Why not go with virDomainDefGetShortName() then? Using def->name to name
a file is dangerous anyways. Oh, is it because we want to keep the same
log name across domain cold reboots?
Michal
11:56 a.m.
New subject: [libvirt PATCH 4/4] qemu: virtiofs: use a .log.fs suffix for logs
On a Monday in 2020, Michal Prívozník wrote:
On 23. 3. 2020 17:10, Ján Tomko wrote:
> As Dave pointed out, someone creative might name a domain
> to make its logfile conflict with a logfile of another
> domain's virtiofsd log.
>
> Signed-off-by: Ján Tomko <jtomko(a)redhat.com>
> Reported-by: Dr. David Alan Gilbert <dgilbert(a)redhat.com>
> ---
> src/qemu/qemu_virtiofs.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/src/qemu/qemu_virtiofs.c b/src/qemu/qemu_virtiofs.c
> index aaa25bcbb8..575b47fd74 100644
> --- a/src/qemu/qemu_virtiofs.c
> +++ b/src/qemu/qemu_virtiofs.c
> @@ -67,9 +67,9 @@ qemuVirtioFSCreateLogFilename(virQEMUDriverConfigPtr cfg,
> {
> g_autofree char *name = NULL;
>
> - name = g_strdup_printf("%s-%s", def->name, alias);
> + name = g_strdup_printf("%s-%s.log.fs", def->name, alias);
>
> - return virFileBuildPath(cfg->logDir, name, "-virtiofsd.log");
> + return virFileBuildPath(cfg->logDir, name, NULL);
> }
>
>
>
Why not go with virDomainDefGetShortName() then? Using def->name to name
a file is dangerous anyways.
Other than path name limits (which are way higher than unix socket
limits), it should not be dangerous.
Oh, is it because we want to keep the same
log name across domain cold reboots?
That too.
Jano
Michal
11:57 a.m.
New subject: [libvirt PATCH 4/4] qemu: virtiofs: use a .log.fs suffix for logs
On 23. 3. 2020 17:56, Ján Tomko wrote:
On a Monday in 2020, Michal Prívozník wrote:
> On 23. 3. 2020 17:10, Ján Tomko wrote:
>> As Dave pointed out, someone creative might name a domain
>> to make its logfile conflict with a logfile of another
>> domain's virtiofsd log.
>>
>> Signed-off-by: Ján Tomko <jtomko(a)redhat.com>
>> Reported-by: Dr. David Alan Gilbert <dgilbert(a)redhat.com>
>> ---
>> src/qemu/qemu_virtiofs.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/qemu/qemu_virtiofs.c b/src/qemu/qemu_virtiofs.c
>> index aaa25bcbb8..575b47fd74 100644
>> --- a/src/qemu/qemu_virtiofs.c
>> +++ b/src/qemu/qemu_virtiofs.c
>> @@ -67,9 +67,9 @@
>> qemuVirtioFSCreateLogFilename(virQEMUDriverConfigPtr cfg,
>> {
>> g_autofree char *name = NULL;
>>
>> - name = g_strdup_printf("%s-%s", def->name, alias);
>> + name = g_strdup_printf("%s-%s.log.fs", def->name, alias);
>>
>> - return virFileBuildPath(cfg->logDir, name, "-virtiofsd.log");
>> + return virFileBuildPath(cfg->logDir, name, NULL);
>> }
>>
>>
>>
>
> Why not go with virDomainDefGetShortName() then? Using def->name to name
> a file is dangerous anyways.
Other than path name limits (which are way higher than unix socket
limits), it should not be dangerous.
> Oh, is it because we want to keep the same
> log name across domain cold reboots?
>
That too.
Okay, for whole series:
Reviewed-by: Michal Privoznik <mprivozn(a)redhat.com>
Michal
12:02 p.m.
New subject: [libvirt PATCH 4/4] qemu: virtiofs: use a .log.fs suffix for logs
On Mon, Mar 23, 2020 at 05:10:37PM +0100, Ján Tomko wrote:
As Dave pointed out, someone creative might name a domain
to make its logfile conflict with a logfile of another
domain's virtiofsd log.
Signed-off-by: Ján Tomko <jtomko(a)redhat.com>
Reported-by: Dr. David Alan Gilbert <dgilbert(a)redhat.com>
---
src/qemu/qemu_virtiofs.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_virtiofs.c b/src/qemu/qemu_virtiofs.c
index aaa25bcbb8..575b47fd74 100644
--- a/src/qemu/qemu_virtiofs.c
+++ b/src/qemu/qemu_virtiofs.c
@@ -67,9 +67,9 @@ qemuVirtioFSCreateLogFilename(virQEMUDriverConfigPtr cfg,
{
g_autofree char *name = NULL;
- name = g_strdup_printf("%s-%s", def->name, alias);
+ name = g_strdup_printf("%s-%s.log.fs", def->name, alias);
Having a logfile that doesn't end in ".log" is pretty gross IMHO
and will mean it isn't caught by logrotate matching *.log
Can't we deal with the clash in another way that preserves .log as a
suffix
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 :|
12:18 p.m.
New subject: [libvirt PATCH 4/4] qemu: virtiofs: use a .log.fs suffix for logs
On a Monday in 2020, Daniel P. Berrangé wrote:
On Mon, Mar 23, 2020 at 05:10:37PM +0100, Ján Tomko wrote:
> As Dave pointed out, someone creative might name a domain
> to make its logfile conflict with a logfile of another
> domain's virtiofsd log.
>
> Signed-off-by: Ján Tomko <jtomko(a)redhat.com>
> Reported-by: Dr. David Alan Gilbert <dgilbert(a)redhat.com>
> ---
> src/qemu/qemu_virtiofs.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/src/qemu/qemu_virtiofs.c b/src/qemu/qemu_virtiofs.c
> index aaa25bcbb8..575b47fd74 100644
> --- a/src/qemu/qemu_virtiofs.c
> +++ b/src/qemu/qemu_virtiofs.c
> @@ -67,9 +67,9 @@ qemuVirtioFSCreateLogFilename(virQEMUDriverConfigPtr cfg,
> {
> g_autofree char *name = NULL;
>
> - name = g_strdup_printf("%s-%s", def->name, alias);
> + name = g_strdup_printf("%s-%s.log.fs", def->name, alias);
Having a logfile that doesn't end in ".log" is pretty gross IMHO
Agreed.
and will mean it isn't caught by logrotate matching *.log
I did not realize that.
Can't we deal with the clash in another way that preserves .log as a
suffix
Not in the same directory, but I can create a separate directory,
something like:
/var/log/libvirt/qemu/vhost-user/%{name}-fs-%{alias}.log
/var/log/libvirt/qemu/devices/%{name}-fs-%{alias}.log
/var/log/libvirt/qemu/fs/%{name}-%{alias}.log
Or we can just pretend this is fine O:-)
Jano
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 :|
12:24 p.m.
New subject: [libvirt PATCH 4/4] qemu: virtiofs: use a .log.fs suffix for logs
On Mon, Mar 23, 2020 at 06:18:27PM +0100, Ján Tomko wrote:
On a Monday in 2020, Daniel P. Berrangé wrote:
> On Mon, Mar 23, 2020 at 05:10:37PM +0100, Ján Tomko wrote:
> > As Dave pointed out, someone creative might name a domain
> > to make its logfile conflict with a logfile of another
> > domain's virtiofsd log.
> >
> > Signed-off-by: Ján Tomko <jtomko(a)redhat.com>
> > Reported-by: Dr. David Alan Gilbert <dgilbert(a)redhat.com>
> > ---
> > src/qemu/qemu_virtiofs.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/qemu/qemu_virtiofs.c b/src/qemu/qemu_virtiofs.c
> > index aaa25bcbb8..575b47fd74 100644
> > --- a/src/qemu/qemu_virtiofs.c
> > +++ b/src/qemu/qemu_virtiofs.c
> > @@ -67,9 +67,9 @@ qemuVirtioFSCreateLogFilename(virQEMUDriverConfigPtr cfg,
> > {
> > g_autofree char *name = NULL;
> >
> > - name = g_strdup_printf("%s-%s", def->name, alias);
> > + name = g_strdup_printf("%s-%s.log.fs", def->name, alias);
>
> Having a logfile that doesn't end in ".log" is pretty gross IMHO
Agreed.
> and will mean it isn't caught by logrotate matching *.log
I did not realize that.
>
> Can't we deal with the clash in another way that preserves .log as a
> suffix
Not in the same directory, but I can create a separate directory,
something like:
/var/log/libvirt/qemu/vhost-user/%{name}-fs-%{alias}.log
/var/log/libvirt/qemu/devices/%{name}-fs-%{alias}.log
/var/log/libvirt/qemu/fs/%{name}-%{alias}.log
Yeah, a separate directory looks like the easy option unless we want
to get into a game of escaping names which is probably more trouble
than it is worth.
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 :|
1705
days inactive
1705
days old
12 comments
3 participants
participants (3)
-
Daniel P. Berrangé -
Ján Tomko -
Michal Prívozník