util: fix a bug found in sorting cache monitor results

[libvirt] [PATCH] util: fix a bug found in sorting cache monitor results

Posted by Wang Huaqiang 4 years, 11 months ago

From: Huaqiang <huaqiang.wang@intel.com>

The qsort element is a pointer of virResctrlMonitorStats, and
the comparing function's arguments have a type of pointer of
virResctrlMonitorStatsPtr.

Signed-off-by: Huaqiang <huaqiang.wang@intel.com>
---
 src/util/virresctrl.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index b845f36..a190977 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -2659,8 +2659,8 @@ static int
 virResctrlMonitorStatsSorter(const void *a,
                              const void *b)
 {
-    return ((virResctrlMonitorStatsPtr)a)->id
-        - ((virResctrlMonitorStatsPtr)b)->id;
+    return (*(virResctrlMonitorStatsPtr *)a)->id
+        - (*(virResctrlMonitorStatsPtr *)b)->id;
 }
 
 
@@ -2758,7 +2758,7 @@ virResctrlMonitorGetStats(virResctrlMonitorPtr monitor,
 
     /* Sort in id's ascending order */
     if (*nstats)
-        qsort(*stats, *nstats, sizeof(*stat), virResctrlMonitorStatsSorter);
+        qsort(*stats, *nstats, sizeof(stat), virResctrlMonitorStatsSorter);
 
     ret = 0;
  cleanup:
-- 
2.7.4

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

Re: [libvirt] [PATCH] util: fix a bug found in sorting cache monitor results

Posted by Michal Privoznik 4 years, 11 months ago

On 5/10/19 5:21 PM, Wang Huaqiang wrote:
> From: Huaqiang <huaqiang.wang@intel.com>
> 
> The qsort element is a pointer of virResctrlMonitorStats, and
> the comparing function's arguments have a type of pointer of
> virResctrlMonitorStatsPtr.
> 
> Signed-off-by: Huaqiang <huaqiang.wang@intel.com>
> ---
>   src/util/virresctrl.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
> index b845f36..a190977 100644
> --- a/src/util/virresctrl.c
> +++ b/src/util/virresctrl.c
> @@ -2659,8 +2659,8 @@ static int
>   virResctrlMonitorStatsSorter(const void *a,
>                                const void *b)
>   {
> -    return ((virResctrlMonitorStatsPtr)a)->id
> -        - ((virResctrlMonitorStatsPtr)b)->id;
> +    return (*(virResctrlMonitorStatsPtr *)a)->id
> +        - (*(virResctrlMonitorStatsPtr *)b)->id;
>   }
>   
>   
> @@ -2758,7 +2758,7 @@ virResctrlMonitorGetStats(virResctrlMonitorPtr monitor,
>   
>       /* Sort in id's ascending order */
>       if (*nstats)
> -        qsort(*stats, *nstats, sizeof(*stat), virResctrlMonitorStatsSorter);
> +        qsort(*stats, *nstats, sizeof(stat), virResctrlMonitorStatsSorter);

Nice catch, but since we're touching this line, how about getting rid of 
@stat completely? I mean, I'd like to change it to sizeof(**stats) 
because that's how big one element is.

Any objections?

Also, I'll update the commit message a bit before pushing.

Michal

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

Re: [libvirt] [PATCH] util: fix a bug found in sorting cache monitor results

Posted by Daniel P. Berrangé 4 years, 11 months ago

On Mon, May 13, 2019 at 12:05:47PM +0200, Michal Privoznik wrote:
> On 5/10/19 5:21 PM, Wang Huaqiang wrote:
> > From: Huaqiang <huaqiang.wang@intel.com>
> > 
> > The qsort element is a pointer of virResctrlMonitorStats, and
> > the comparing function's arguments have a type of pointer of
> > virResctrlMonitorStatsPtr.
> > 
> > Signed-off-by: Huaqiang <huaqiang.wang@intel.com>
> > ---
> >   src/util/virresctrl.c | 6 +++---
> >   1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
> > index b845f36..a190977 100644
> > --- a/src/util/virresctrl.c
> > +++ b/src/util/virresctrl.c
> > @@ -2659,8 +2659,8 @@ static int
> >   virResctrlMonitorStatsSorter(const void *a,
> >                                const void *b)
> >   {
> > -    return ((virResctrlMonitorStatsPtr)a)->id
> > -        - ((virResctrlMonitorStatsPtr)b)->id;
> > +    return (*(virResctrlMonitorStatsPtr *)a)->id
> > +        - (*(virResctrlMonitorStatsPtr *)b)->id;
> >   }
> > @@ -2758,7 +2758,7 @@ virResctrlMonitorGetStats(virResctrlMonitorPtr monitor,
> >       /* Sort in id's ascending order */
> >       if (*nstats)
> > -        qsort(*stats, *nstats, sizeof(*stat), virResctrlMonitorStatsSorter);
> > +        qsort(*stats, *nstats, sizeof(stat), virResctrlMonitorStatsSorter);
> 
> Nice catch, but since we're touching this line, how about getting rid of
> @stat completely? I mean, I'd like to change it to sizeof(**stats) because
> that's how big one element is.

Yeah, I'd prefer using  sizeof(**stats) as that is more obviously
correct when comparing args.

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