1. Patchew
  2. Libvirt
  3. [libvirt PATCH 0/4] qemu: virtiofs fixes
  4. View patch

[libvirt PATCH 4/4] qemu: virtiofs: use a .log.fs suffix for logs

Ján Tomko posted 4 patches 5 years, 10 months ago
[libvirt PATCH 4/4] qemu: virtiofs: use a .log.fs suffix for logs
Posted by Ján Tomko 5 years, 10 months ago 
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@redhat.com>
Reported-by: Dr. David Alan Gilbert <dgilbert@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
Re: [libvirt PATCH 4/4] qemu: virtiofs: use a .log.fs suffix for logs
Posted by Michal Prívozník 5 years, 10 months ago 
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@redhat.com>
> Reported-by: Dr. David Alan Gilbert <dgilbert@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
Re: [libvirt PATCH 4/4] qemu: virtiofs: use a .log.fs suffix for logs
Posted by Ján Tomko 5 years, 10 months ago 
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@redhat.com>
>> Reported-by: Dr. David Alan Gilbert <dgilbert@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
>
Re: [libvirt PATCH 4/4] qemu: virtiofs: use a .log.fs suffix for logs
Posted by Michal Prívozník 5 years, 10 months ago 
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@redhat.com>
>>> Reported-by: Dr. David Alan Gilbert <dgilbert@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@redhat.com>

Michal
Re: [libvirt PATCH 4/4] qemu: virtiofs: use a .log.fs suffix for logs
Posted by Daniel P. Berrangé 5 years, 10 months ago 
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@redhat.com>
> Reported-by: Dr. David Alan Gilbert <dgilbert@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 :|
Re: [libvirt PATCH 4/4] qemu: virtiofs: use a .log.fs suffix for logs
Posted by Ján Tomko 5 years, 10 months ago 
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@redhat.com>
>> Reported-by: Dr. David Alan Gilbert <dgilbert@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 :|
>
Re: [libvirt PATCH 4/4] qemu: virtiofs: use a .log.fs suffix for logs
Posted by Daniel P. Berrangé 5 years, 10 months ago 
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@redhat.com>
> > > Reported-by: Dr. David Alan Gilbert <dgilbert@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 :|

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.