1. Patchew
  2. Libvirt
  3. [libvirt] [PATCH 0/2] qemu: fix misc noisy logs for optional sources
  4. View patch

[libvirt] [PATCH 2/2] qemu: don't log error for missing optional sources on start

Nikolay Shirokovskiy posted 2 patches 7 years, 2 months ago
[libvirt] [PATCH 2/2] qemu: don't log error for missing optional sources on start
Posted by Nikolay Shirokovskiy 7 years, 2 months ago 
Because missing optional source is not error. The patch
address only local files. Fixing other cases is a bit ugly.

Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
---
 src/qemu/qemu_process.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 802274e..5e04bf9 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -6100,7 +6100,8 @@ qemuProcessPrepareHostStorage(virQEMUDriverPtr driver,
         if (!blockdev)
             virStorageSourceBackingStoreClear(disk->src);
 
-        if (qemuDomainDetermineDiskChain(driver, vm, disk, true) >= 0)
+        if (!qemuProcessMissingLocalOptionalDisk(disk) &&
+            qemuDomainDetermineDiskChain(driver, vm, disk, true) >= 0)
             continue;
 
         if (qemuDomainCheckDiskStartupPolicy(driver, vm, idx, cold_boot) >= 0)
-- 
1.8.3.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] qemu: don't log error for missing optional sources on start
Posted by John Ferlan 7 years, 2 months ago 
$SUBJ

"storage sources"

On 11/12/18 7:58 AM, Nikolay Shirokovskiy wrote:
> Because missing optional source is not error. The patch
> address only local files. Fixing other cases is a bit ugly.
> 
> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
> ---
>  src/qemu/qemu_process.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 802274e..5e04bf9 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -6100,7 +6100,8 @@ qemuProcessPrepareHostStorage(virQEMUDriverPtr driver,
>          if (!blockdev)
>              virStorageSourceBackingStoreClear(disk->src);
>  
> -        if (qemuDomainDetermineDiskChain(driver, vm, disk, true) >= 0)
> +        if (!qemuProcessMissingLocalOptionalDisk(disk) &&
> +            qemuDomainDetermineDiskChain(driver, vm, disk, true) >= 0)

Although it makes more sense for this path to use the startupPolicy, I
will point out that the first thing qemuDomainDetermineDiskChain does is
filter on virStorageSourceIsEmpty, so regardless of whether the
startupPolicy is optional or not, not much is happening for empty
sources.  So in essence unnecessary at least from my read.

John

>              continue;
>  
>          if (qemuDomainCheckDiskStartupPolicy(driver, vm, idx, cold_boot) >= 0)
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] qemu: don't log error for missing optional sources on start
Posted by Nikolay Shirokovskiy 7 years, 1 month ago 

On 11.12.2018 01:05, John Ferlan wrote:
> $SUBJ
> 
> "storage sources"
> 
> On 11/12/18 7:58 AM, Nikolay Shirokovskiy wrote:
>> Because missing optional source is not error. The patch
>> address only local files. Fixing other cases is a bit ugly.
>>
>> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
>> ---
>>  src/qemu/qemu_process.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>> index 802274e..5e04bf9 100644
>> --- a/src/qemu/qemu_process.c
>> +++ b/src/qemu/qemu_process.c
>> @@ -6100,7 +6100,8 @@ qemuProcessPrepareHostStorage(virQEMUDriverPtr driver,
>>          if (!blockdev)
>>              virStorageSourceBackingStoreClear(disk->src);
>>  
>> -        if (qemuDomainDetermineDiskChain(driver, vm, disk, true) >= 0)
>> +        if (!qemuProcessMissingLocalOptionalDisk(disk) &&
>> +            qemuDomainDetermineDiskChain(driver, vm, disk, true) >= 0)
> 
> Although it makes more sense for this path to use the startupPolicy, I
> will point out that the first thing qemuDomainDetermineDiskChain does is
> filter on virStorageSourceIsEmpty, so regardless of whether the
> startupPolicy is optional or not, not much is happening for empty
> sources.  So in essence unnecessary at least from my read.

The issue is not with empty (no source file is specified) sources but rather
with missing ones (source file is specified but file itself does not exist).
In this case virStorageSourceIsEmpty does not help and we get this log notice:

error: virStorageFileReportBrokenChain:427 : Cannot access storage file '/path/to/missing/optional/disk': No such file or directory

Which is not fatal for starting a domain because source is optional.

Sorry, I should put error messages in the original letters so you can
easier understand what's going on.

Nikolay

> 
> John
> 
>>              continue;
>>  
>>          if (qemuDomainCheckDiskStartupPolicy(driver, vm, idx, cold_boot) >= 0)
>>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] qemu: don't log error for missing optional sources on start
Posted by John Ferlan 7 years, 1 month ago 

On 12/11/18 2:39 AM, Nikolay Shirokovskiy wrote:
> 
> 
> On 11.12.2018 01:05, John Ferlan wrote:
>> $SUBJ
>>
>> "storage sources"
>>
>> On 11/12/18 7:58 AM, Nikolay Shirokovskiy wrote:
>>> Because missing optional source is not error. The patch
>>> address only local files. Fixing other cases is a bit ugly.
>>>
>>> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
>>> ---
>>>  src/qemu/qemu_process.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>>> index 802274e..5e04bf9 100644
>>> --- a/src/qemu/qemu_process.c
>>> +++ b/src/qemu/qemu_process.c
>>> @@ -6100,7 +6100,8 @@ qemuProcessPrepareHostStorage(virQEMUDriverPtr driver,
>>>          if (!blockdev)
>>>              virStorageSourceBackingStoreClear(disk->src);
>>>  
>>> -        if (qemuDomainDetermineDiskChain(driver, vm, disk, true) >= 0)
>>> +        if (!qemuProcessMissingLocalOptionalDisk(disk) &&
>>> +            qemuDomainDetermineDiskChain(driver, vm, disk, true) >= 0)
>>
>> Although it makes more sense for this path to use the startupPolicy, I
>> will point out that the first thing qemuDomainDetermineDiskChain does is
>> filter on virStorageSourceIsEmpty, so regardless of whether the
>> startupPolicy is optional or not, not much is happening for empty
>> sources.  So in essence unnecessary at least from my read.
> 
> The issue is not with empty (no source file is specified) sources but rather
> with missing ones (source file is specified but file itself does not exist).
> In this case virStorageSourceIsEmpty does not help and we get this log notice:
> 
> error: virStorageFileReportBrokenChain:427 : Cannot access storage file '/path/to/missing/optional/disk': No such file or directory
> 
> Which is not fatal for starting a domain because source is optional.

But, but not logging it how would someone know that it's missing
otherwise?  Maybe seeing that message in a log file draws someone's
attention to the fact that their source went missing and even though it
is optional, they may be surprised that the file isn't there. This is
one of those hard one's where it's difficult to read the minds of all
consumers. Damned by some if you do log and damned by others if you
don't log.

I'd prefer to err on the side of logging the message because it's easier
to ignore something that you know about than it is to not be told about
something and then wonder why you weren't told. Of course that's just
one person's opinion.

> 
> Sorry, I should put error messages in the original letters so you can
> easier understand what's going on.

Yes, that does help especially in edge cases.

John

> 
> Nikolay
> 
>>
>> John
>>
>>>              continue;
>>>  
>>>          if (qemuDomainCheckDiskStartupPolicy(driver, vm, idx, cold_boot) >= 0)
>>>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] qemu: don't log error for missing optional sources on start
Posted by Daniel P. Berrangé 7 years, 1 month ago 
On Tue, Dec 11, 2018 at 09:33:50AM -0500, John Ferlan wrote:
> 
> 
> On 12/11/18 2:39 AM, Nikolay Shirokovskiy wrote:
> > 
> > 
> > On 11.12.2018 01:05, John Ferlan wrote:
> >> $SUBJ
> >>
> >> "storage sources"
> >>
> >> On 11/12/18 7:58 AM, Nikolay Shirokovskiy wrote:
> >>> Because missing optional source is not error. The patch
> >>> address only local files. Fixing other cases is a bit ugly.
> >>>
> >>> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
> >>> ---
> >>>  src/qemu/qemu_process.c | 3 ++-
> >>>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> >>> index 802274e..5e04bf9 100644
> >>> --- a/src/qemu/qemu_process.c
> >>> +++ b/src/qemu/qemu_process.c
> >>> @@ -6100,7 +6100,8 @@ qemuProcessPrepareHostStorage(virQEMUDriverPtr driver,
> >>>          if (!blockdev)
> >>>              virStorageSourceBackingStoreClear(disk->src);
> >>>  
> >>> -        if (qemuDomainDetermineDiskChain(driver, vm, disk, true) >= 0)
> >>> +        if (!qemuProcessMissingLocalOptionalDisk(disk) &&
> >>> +            qemuDomainDetermineDiskChain(driver, vm, disk, true) >= 0)
> >>
> >> Although it makes more sense for this path to use the startupPolicy, I
> >> will point out that the first thing qemuDomainDetermineDiskChain does is
> >> filter on virStorageSourceIsEmpty, so regardless of whether the
> >> startupPolicy is optional or not, not much is happening for empty
> >> sources.  So in essence unnecessary at least from my read.
> > 
> > The issue is not with empty (no source file is specified) sources but rather
> > with missing ones (source file is specified but file itself does not exist).
> > In this case virStorageSourceIsEmpty does not help and we get this log notice:
> > 
> > error: virStorageFileReportBrokenChain:427 : Cannot access storage file '/path/to/missing/optional/disk': No such file or directory
> > 
> > Which is not fatal for starting a domain because source is optional.
> 
> But, but not logging it how would someone know that it's missing
> otherwise?  Maybe seeing that message in a log file draws someone's
> attention to the fact that their source went missing and even though it
> is optional, they may be surprised that the file isn't there. This is
> one of those hard one's where it's difficult to read the minds of all
> consumers. Damned by some if you do log and damned by others if you
> don't log.

IMHO if you have marked the source as optional, you have explicitly
stated that you don't care if it goes missing for *any* reason. As
such we certainly should not log anything at "error" or "warning"
levels. At the very most it would be an "info" level warning if at
all.

If apps are concerned about things going missing at the wrong time,
they should look at the XML once the guest is running to see if the
expected source is there, or validate it themselves before booting
the guest.

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

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.