In many files there are header comments that contain an Author:
statement, supposedly reflecting who originally wrote the code.
In a large collaborative project like libvirt, any non-trivial
file will have been modified by a large number of different
contributors. IOW, the Author: comments are quickly out of date,
omitting people who have made significant contribitions.
In some places Author: lines have been added despite the person
merely being responsible for creating the file by moving existing
code out of another file. IOW, the Author: lines give an incorrect
record of authorship.
With this all in mind, the comments are useless as a means to identify
who to talk to about code in a particular file. Contributors will always
be better off using 'git log' and 'git blame' if they need to find the
author of a particular bit of code.
This commit thus deletes all Author: comments from the source and adds
a rule to prevent them reappearing.
The Copyright headers are similarly misleading and inaccurate, however,
we cannot delete these as they have legal meaning, despite being largely
inaccurate. In addition only the copyright holder is permitted to change
their respective copyright statement.
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
This patch is introducing cache monitor(CMT) to cache and
memory bandwidth monitor(MBM) for monitoring CPU memory
bandwidth.
The host capability of the two monitors is also introduced
in this patch.
For CMT, the host capability is shown like:
<host>
...
<cache>
<bank id='0' level='3' type='both' size='15' unit='MiB' cpus='0-5'>
<control granularity='768' min='1536' unit='KiB' type='both' maxAllocs='4'/>
</bank>
<monitor level='3' 'reuseThreshold'='270336' maxMonitors='176'>
<feature name='llc_occupancy'/>
</monitor>
</cache>
...
</host>
For MBM, the capability is shown like this:
<host>
...
<memory_bandwidth>
<node id='1' cpus='6-11'>
<control granularity='10' min ='10' maxAllocs='4'/>
</node>
<monitor maxMonitors='176'>
<feature name='mbm_total_bytes'/>
<feature name='mbm_local_bytes'/>
</monitor>
</memory_bandwidth>
...
</host>
Signed-off-by: Wang Huaqiang <huaqiang.wang@intel.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Move memory bandwidth capability nodes into one data structure,
this allows us to add a monitor for memory bandwidth.
Signed-off-by: Wang Huaqiang <huaqiang.wang@intel.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Move all cache banks into one data structure, this allows
us to add other cache component, such as cache monitor.
Signed-off-by: Wang Huaqiang <huaqiang.wang@intel.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
All of the ones being removed are pulled in by internal.h. The only
exception is sanlock which expects the application to include <stdint.h>
before sanlock's headers, because sanlock prototypes use fixed width
int, but they don't include stdint.h themselves, so we have to leave
that one in place.
Signed-off-by: Erik Skultety <eskultet@redhat.com>
Acked-by: Michal Privoznik <mprivozn@redhat.com>
It doesn't really make sense for us to have stdlib.h and string.h but
not stdio.h in the internal.h header.
Signed-off-by: Erik Skultety <eskultet@redhat.com>
Acked-by: Michal Privoznik <mprivozn@redhat.com>
To add CMT/MBM feature and let code be consistent in later patches,
renaming variable name from 'controlBuf' to 'childrenBuf', locates
in functions 'virCapabilitiesFormatCaches' and
'virCapabilitiesFormatMemoryBandwidth'.
Signed-off-by: Wang Huaqiang <huaqiang.wang@intel.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Add new XML section to report host's memory bandwidth allocation
capability. The format as below example:
<host>
.....
<memory_bandwidth>
<node id='0' cpus='0-19'>
<control granularity='10' min ='10' maxAllocs='8'/>
</node>
</memory_bandwidth>
</host>
granularity ---- granularity of memory bandwidth, unit percentage.
min ---- minimum memory bandwidth allowed, unit percentage.
maxAllocs ---- maximum memory bandwidth allocation group supported.
Signed-off-by: Bing Niu <bing.niu@intel.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
The virCapabilitiesDomainDataLookupInternal() is given a list of
parameters representing the desired domain characteristics. It then has
to look throught the capabilities to identify an acceptable match.
The virCapsDomainDataCompare() method is used for filtering out
candidates which don't match the desired criteria. It is called
primarily from the innermost loops and as such is doing many repeated
checks. For example if architcture and os type were checked at the top
level loop the two inner loops could be avoided entirely. If emulator
and domain type were checked in the 2nd level loop the 3rd level loop
can be avoided too.
This change thus removes the virCapsDomainDataCompare() method and puts
suitable checks at the start of each loop to ensure it executes the
minimal number of loop iterations possible. The code becomes clearer to
understand as a nice side-effect.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Currently, the functions return a pointer to the
destination buffer on success or NULL on failure.
Not only does this kind of error handling look quite
alien in the context of libvirt, where most functions
return zero on success and a negative int on failure,
but it's also somewhat pointless because unless there's
been a failure the returned pointer will be the same
one passed in by the user, thus offering no additional
value.
Change the functions so that they return an int
instead.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
It will be used in that file later on, plus it makes sense for all the
implementations to be in same place. Also comment each one of them nicely and
add a comment explaining why they all need to end with the same _LAST value.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
There is no need to have virResctrlGetInfo() when it must be called after
virResctrlInfoNew() anyway, otherwise it's just an unusable object. When we
wrap the logic inside the New() function we'll save some calls later as well.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
https://bugzilla.redhat.com/show_bug.cgi?id=1569678
On some large systems (with ~400GB of RAM) it is possible for
unsigned int to overflow in which case we report invalid number
of 4K pages pool size. Switch to unsigned long long.
We hit overflow in virNumaGetPages when doing:
huge_page_sum += 1024 * page_size * page_avail;
because although 'huge_page_sum' is an unsigned long long, the
page_size and page_avail are both unsigned int, so the promotion
to unsigned long long doesn't happen until the sum has been
calculated, by which time we've already overflowed.
Turning page_avail into a unsigned long long is not strictly
needed until we need ability to represent more than 2^32
4k pages, which equates to 16 TB of RAM. That's not
outside the realm of possibility, so makes sense that we
change it to unsigned long long to avoid future problems.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
So far we are repeating the following lines over and over:
if (!(virSomeObjectClass = virClassNew(virClassForObject(),
"virSomeObject",
sizeof(virSomeObject),
virSomeObjectDispose)))
return -1;
While this works, it is impossible to do some checking. Firstly,
the class name (the 2nd argument) doesn't match the name in the
code in all cases (the 3rd argument). Secondly, the current style
is needlessly verbose. This commit turns example into following:
if (!(VIR_CLASS_NEW(virSomeObject,
virClassForObject)))
return -1;
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
If a function is disposing virSomething it should be called
virSomethingDispose(). There are two offenders:
virCapabilitiesDispose(virCapsPtr) and
virDomainXMLOptionClassDispose(virDomainXMLOptionPtr).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Because the cache banks are initialized based on the order in which their
respective directories exist on the filesystem, they can appear in different
order. This is here mainly for tests because the cache directory might have
different order of children nodes and tests would fail otherwise. It should not
be the case with sysfs, but one can never be sure. And this does not take
almost any extra time, mainly because it gets initialized once per driver.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
In the past we updated host-model CPUs with host CPU data by adding a
model and features, but keeping the host-model mode. And since the CPU
model is not normally formatted for host-model CPU defs, we had to pass
the updateCPU flag to the formatting code to be able to properly output
updated host-model CPUs. Libvirt doesn't do this anymore, host-model
CPUs are turned into custom mode CPUs once updated with host CPU data
and thus there's no reason for keeping the hacks inside CPU XML
formatters.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
That means that returning negative values means error and non-negative
values differ in meaning, but are all successful.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
It doesn't access anything from conf/ and ti will be needed to use
from other util/ places. This split makes the separation clearer.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
After an OOM error, virBuffer* APIs set buf->use to zero.
Adding a buffer to the parent buffer only if use is non-zero
would quietly drop data on error.
Check the error beforehand to make sure buf->use is zero
because we have not attempted to add anything to it.
On some platforms the number of bits in the cbm_mask might not be
divisible by 4 (and not even by 2), so we need to properly count the
bits. Similar file, min_cbm_bits, is properly parsed and used, but if
the number is greater than one, we lose the information about
granularity when reporting the data in capabilities. For that matter
always report granularity, but if it is not the same as the minimum,
add that information in there as well.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
The @cpus is allocated by virFileReadValueBitmap() but never
freed:
==21274== 40 (32 direct, 8 indirect) bytes in 1 blocks are definitely lost in loss record 808 of 1,004
==21274== at 0x4C2E080: calloc (vg_replace_malloc.c:711)
==21274== by 0x54BA561: virAlloc (viralloc.c:144)
==21274== by 0x54BC604: virBitmapNewEmpty (virbitmap.c:126)
==21274== by 0x54BD059: virBitmapParseUnlimited (virbitmap.c:570)
==21274== by 0x54EECE9: virFileReadValueBitmap (virfile.c:4113)
==21274== by 0x5563132: virCapabilitiesInitCaches (capabilities.c:1548)
==21274== by 0x2BB86E59: virQEMUCapsInit (qemu_capabilities.c:1132)
==21274== by 0x2BBEC067: virQEMUDriverCreateCapabilities (qemu_conf.c:928)
==21274== by 0x2BC3DEAA: qemuStateInitialize (qemu_driver.c:845)
==21274== by 0x5625AAC: virStateInitialize (libvirt.c:770)
==21274== by 0x124519: daemonRunStateInit (libvirtd.c:881)
==21274== by 0x554C927: virThreadHelper (virthread.c:206)
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
To every virDirOpen we must have VIR_DIR_CLOSE otherwise FD is
leaked.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
The @type from virFileReadValueString needs to be VIR_FREE each time
through the loop since it's not saved and since cleanup can be reached
prior to decoding it for @kernel_type amd bank->type, the cleanup code
needs to also have a VIR_FREE
Found by Coverity
We're only adding only info about L3 caches, we can add more
later (just by changing one line), but for now that's more than enough
without overwhelming anyone.
XML snippet of how this should look like (also seen as part of the commit):
<cache>
<bank id='0' level='3' type='both' size='8192' unit='KiB' cpus='0-7'/>
</cache>
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
It is no longer needed thanks to the great virfilewrapper.c. And this
way we don't have to add a new set of functions for each prefixed
path.
While on that, add two functions that weren't there before, string and
scaled integer reading ones. Also increase the length of the string
being read by one to accompany for the optional newline at the
end (i.e. change INT_STRLEN_BOUND to INT_BUFSIZE_BOUND).
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
If formatting NUMA topology fails, the function returns immediatelly,
but the buffer structure allocated on the stack references lot of
heap-allocated memory and that would get lost in such case.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
This way more drivers can utilize the functionality without copying
the code. And we can therefore test it in one place for all of them.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
There is no "node driver" as there was before, drivers have to do
their own ACL checking anyway, so they all specify their functions and
nodeinfo is basically just extending conf/capablities. Hence moving
the code to src/conf/ is the right way to go.
Also that way we can de-duplicate some code that is in virsysfs and/or
virhostcpu that got duplicated during the virhostcpu.c split. And
Some cleanup is done throughout the changes, like adding the vir*
prefix etc.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Guests are handled in callers, but if something goes wrong (when it
cannot be added to virCapabilities, for example), there's no way for
them to free it properly.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
In the reverted commit d2e5538b1, the libxl driver was changed to copy
interface names autogenerated by libxl to the corresponding network def
in the domain's virDomainDef object. The copied name is freed when the
domain transitions to the shutoff state. But when migrating a domain,
the autogenerated name is included in the XML sent to the destination
host. It is possible an interface with the same name already exists on
the destination host, causing migration to fail.
This patch defines a new capability for setting the network device
prefix that will be used in the driver. Valid prefixes are
VIR_NET_GENERATED_PREFIX or the one announced by the driver.
Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
Introduce VIR_DOMAIN_VIRT_NONE to give domaintype the default value of zero.
This is specially helpful in constructing better error messages
when we don't want to look up the default emulator by virtType.
The test data in vircapstest.c is also modified to reflect this change.
During a review, I've noticed this error message that was eventually
produced when I was trying to define a domain:
error: invalid argument: could not find capabilities for arch=mips64el
domaintype=(null)
Look at the (null). Why is it there? Well, during XML parsing, we try
to look up the default emulator for given OS type and possibly virt
type too. And this is the problem, because if we don't want to look up
by virt type, a -1 is passed to note this fact. Later, the code
handles -1 just right. Except for error message. When it is
constructed (in a very fabulous way I must say), the value is compared
to zero, not -1. And since we don't have any translation from -1 to a
virt type string, we just print (null).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
There is a lot of places, were it's pretty easy for user to enter some
characters that we need to escape to create a valid XML description.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1197580
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
My commit 747761a79 (v1.2.15 only) dropped this bit of logic when filling
in a default arch in the XML:
- /* First try to find one matching host arch */
- for (i = 0; i < caps->nguests; i++) {
- if (caps->guests[i]->ostype == ostype) {
- for (j = 0; j < caps->guests[i]->arch.ndomains; j++) {
- if (caps->guests[i]->arch.domains[j]->type == domain &&
- caps->guests[i]->arch.id == caps->host.arch)
- return caps->guests[i]->arch.id;
- }
- }
- }
That attempt to match host.arch is important, otherwise we end up
defaulting to i686 on x86_64 host for KVM, which is not intended.
Duplicate it in the centralized CapsLookup function.
Additionally add some testcases that would have caught this.
https://bugzilla.redhat.com/show_bug.cgi?id=1219191
virBufferContentAndReset() doesn't free buf contents, we should use
virBufferFreeAndReset() to get buf freed.
Signed-off-by: Zhang Bo <oscar.zhangbo@huawei.com>
This revealed that GuestDefaultEmulator was a bit buggy, capable
of returning an emulator that didn't match the passed domain type. Fix
up the test suite input to continue to pass.
This is a helper function to look up all capabilities data for all
the OS bits that are relevant to <domain>. This is
- os type
- arch
- domain type
- emulator
- machine type
This will be used to replace several functions in later commits.
