Re: [PATCH 05/10] nodeinfo: Add out parameter to nodeGetPresentCPUBitmap()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, 2015-07-20 at 16:18 +0200, Peter Krempa wrote:
> On Mon, Jul 20, 2015 at 16:07:42 +0200, Andrea Bolognani wrote:
> > On Mon, 2015-07-20 at 15:15 +0200, Peter Krempa wrote:
> > > 
> > > > -virBitmapPtr nodeGetPresentCPUBitmap(const char 
> > > > *sysfs_prefix);
> > > > -virBitmapPtr nodeGetCPUBitmap(const char *sysfs_prefix, int 
> > > > *max_id);
> > > > +virBitmapPtr nodeGetPresentCPUBitmap(const char *sysfs_prefix,
> > > > +                                     int *size);
> > > > +virBitmapPtr nodeGetCPUBitmap(const char *sysfs_prefix,
> > > > +                              int *size);
> > > 
> > > I'd prefer something like "ncpus" or maxcpu rather than size. For
> > > getting size virBitmapSize() is totally apropriate.
> > 
> > I've used "size" on purpose, because I didn't want people to
> > mistake that for a count of online or present CPUs: it's the
> > size of the returned bitmap, same value you'd get if you
> > called virBitmapSize() on it.
> 
> I thin the 'max_id' or perhaps 'max_cpu_id' were better. Otherwise 
> I'd
> stay with calling virBitmapSize. It doesn't then look like it's 
> adding
> any value on top of calling virBitmapSize directly and could actually 
> be
> optimized out.

Using "max_id" is wrong though, because the returned value is
the size of the bitmap: if you have 4 CPUs, it will return 4,
not 3 as the name "max_id" would suggest. 

Since virBitmapSize() does very little work anyway, I vote
for getting rid of the out parameter altogether.

Cheers.

-- 
Andrea Bolognani
Software Engineer - Virtualization Team

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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]