[libvirt] [PATCH] virPerfEventIsEnabled: Accept NULL @perf

Michal Privoznik posted 1 patch 6 years, 10 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/5749368abd6fc72e2dd234631687f0dc58c3dc85.1493908370.git.mprivozn@redhat.com
src/util/virperf.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
[libvirt] [PATCH] virPerfEventIsEnabled: Accept NULL @perf
Posted by Michal Privoznik 6 years, 10 months ago
After bdcf6e481 there is a crasher in libvirt. The commit assumes
that priv->perf is always set. That is not true. For inactive
domains, the priv->perf is not allocated as it is set in
qemuProcessLaunch(). Now, usually we differentiate between
accesses to inactive and active definition and it works just
fine. Except for 'domstats'. There priv->perf is accessed without
prior check for domain inactivity. While we could check for that,
more robust solution is to make virPerfEventIsEnabled() accept
NULL.

How to reproduce:
1) ensure you have at least one inactive domain
2) virsh domstats

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/util/virperf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/util/virperf.c b/src/util/virperf.c
index fa5b6cc..2c832b3 100644
--- a/src/util/virperf.c
+++ b/src/util/virperf.c
@@ -297,7 +297,7 @@ virPerfEventDisable(virPerfPtr perf,
 bool virPerfEventIsEnabled(virPerfPtr perf,
                            virPerfEventType type)
 {
-    return perf->events[type].enabled;
+    return perf && perf->events[type].enabled;
 }
 
 int
-- 
2.10.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virPerfEventIsEnabled: Accept NULL @perf
Posted by Daniel P. Berrange 6 years, 10 months ago
On Thu, May 04, 2017 at 04:32:50PM +0200, Michal Privoznik wrote:
> After bdcf6e481 there is a crasher in libvirt. The commit assumes
> that priv->perf is always set. That is not true. For inactive
> domains, the priv->perf is not allocated as it is set in
> qemuProcessLaunch(). Now, usually we differentiate between
> accesses to inactive and active definition and it works just
> fine. Except for 'domstats'. There priv->perf is accessed without
> prior check for domain inactivity. While we could check for that,
> more robust solution is to make virPerfEventIsEnabled() accept
> NULL.
> 
> How to reproduce:
> 1) ensure you have at least one inactive domain
> 2) virsh domstats
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/util/virperf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/util/virperf.c b/src/util/virperf.c
> index fa5b6cc..2c832b3 100644
> --- a/src/util/virperf.c
> +++ b/src/util/virperf.c
> @@ -297,7 +297,7 @@ virPerfEventDisable(virPerfPtr perf,
>  bool virPerfEventIsEnabled(virPerfPtr perf,
>                             virPerfEventType type)
>  {
> -    return perf->events[type].enabled;
> +    return perf && perf->events[type].enabled;
>  }

ACK


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
Re: [libvirt] [PATCH] virPerfEventIsEnabled: Accept NULL @perf
Posted by Michal Privoznik 6 years, 10 months ago
On 05/04/2017 04:41 PM, Daniel P. Berrange wrote:
> On Thu, May 04, 2017 at 04:32:50PM +0200, Michal Privoznik wrote:
>> After bdcf6e481 there is a crasher in libvirt. The commit assumes
>> that priv->perf is always set. That is not true. For inactive
>> domains, the priv->perf is not allocated as it is set in
>> qemuProcessLaunch(). Now, usually we differentiate between
>> accesses to inactive and active definition and it works just
>> fine. Except for 'domstats'. There priv->perf is accessed without
>> prior check for domain inactivity. While we could check for that,
>> more robust solution is to make virPerfEventIsEnabled() accept
>> NULL.
>>
>> How to reproduce:
>> 1) ensure you have at least one inactive domain
>> 2) virsh domstats
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>  src/util/virperf.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/util/virperf.c b/src/util/virperf.c
>> index fa5b6cc..2c832b3 100644
>> --- a/src/util/virperf.c
>> +++ b/src/util/virperf.c
>> @@ -297,7 +297,7 @@ virPerfEventDisable(virPerfPtr perf,
>>  bool virPerfEventIsEnabled(virPerfPtr perf,
>>                             virPerfEventType type)
>>  {
>> -    return perf->events[type].enabled;
>> +    return perf && perf->events[type].enabled;
>>  }
> 
> ACK
> 
> 

Thanks, pushed.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list