To avoid code duplication between snapshot configuration code that
parses the disk source too we need to split out this code that will be
reused later on.
This patch tries to be code movement, some aspects of this function will
be refactored later.
https://bugzilla.redhat.com/show_bug.cgi?id=1027096
If there's the following snippet in the domain XML, the domain will be
lost upon the daemon restart (if the domain is started prior restart):
<seclabel type='dynamic' relabel='yes'/>
The problem is, the 'label', 'imagelabel' and 'baselabel' are parsed
whenever the VIR_DOMAIN_XML_INACTIVE is *not* present or the label is
static. The latter is not our case, obviously. So, when libvirtd starts
up, it finds domain state xml and parse it. During parsing, many XML
flags are enabled but VIR_DOMAIN_XML_INACTIVE. Hence, our parser tries
to extract 'label', 'imagelabel' and 'baselabel' from the XML which
fails for model='none'. Err, this model - even though not specified in
XML - can be taken from qemu wide config file: /etc/libvirtd/qemu.conf.
However, in order to know we are dealing with model='none' the code in
question must be moved forward a bit. Then a new check must be
introduced. This is what the first two chunks are doing.
But this alone is not sufficient. The domain state XML won't contain the
model attribute without slight modification. The model should be
inserted into the XML even if equal to 'none' and the state XML is being
generated - what if the origin (the @security_driver variable in
qemu.conf) changes during libvirtd restarts?
At the end, a test to catch this scenario is introduced.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
The linux kernel recently added support for paravirtual spinlock
handling to avoid performance regressions on overcomitted hosts. This
feature needs to be turned in the hypervisor so that the guest OS is
notified about the possible support.
This patch adds a new feature "paravirt-spinlock" to the XML and
supporting code to enable the "kvm_pv_unhalt" pseudo CPU feature in
qemu.
https://bugzilla.redhat.com/show_bug.cgi?id=1008989
Currently we were storing domain feature flags in a bit field as the
they were either enabled or disabled. New features such as paravirtual
spinlocks however can be tri-state as the default option may depend on
hypervisor version.
To allow storing tri-state feature state in the same place instead of
having to declare dedicated variables for each feature this patch
refactors the bit field to an array.
For some strange reason virDomainDiskSourcePoolDefParse accessed def of
the disk and allocated the pool object in it. To avoid the need to carry
over the disk definition object, refactor this function to return the
allocated object instead.
When starting a transient VM the first thing done is to check
for duplicates. The check looks if there are any running VMs
with the matching name/uuid. It explicitly allows there to
be inactive VMs, so that a persistent VM can be temporarily
booted with a different config.
There is a race condition, however, where 2 or more clients
try to create the same transient VM. The first client will
cause a virDomainObjPtr to be added to the domain list, and
it is inactive at this stage. The second client may then
come along and see this inactive VM, and mistake it for a
persistent VM.
If the first VM fails to start its transient guest for any
reason, then it'll remove the virDomainObjPtr from the list.
The second client now has a virDomainObjPtr that it can try
to boot, which libvirt no longer has a record of. The result
can be a running QEMU process that is orphaned.
It was also, however, possible for the virDomainObjPtr to be
completely free'd which will cause libvirtd to crash in some
scenarios.
The fix is to only allow an existing inactive VM if it is
marked as persistent.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
virDomainObjListLoadAllConfigs sets dom->persistent after
having released its lock on the domain object. This exposes
a possible race condition.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Some ancient gcc fails to see the variables are initialized in a
separate function and a false positive is produced:
cc1: warnings being treated as errors
conf/domain_conf.c: In function 'virDomainChrGetDomainPtrs':
conf/domain_conf.c:10342: error: 'arrVar' may be used uninitialized in this function [-Wuninitialized]
conf/domain_conf.c:10343: error: 'cntVar' may be used uninitialized in this function [-Wuninitialized]
conf/domain_conf.c: In function 'virDomainChrInsert':
conf/domain_conf.c:10362: error: 'arrPtr' may be used uninitialized in this function [-Wuninitialized]
conf/domain_conf.c:10363: error: 'cntPtr' may be used uninitialized in this function [-Wuninitialized]
conf/domain_conf.c: In function 'virDomainChrRemove':
conf/domain_conf.c:10374: error: 'arrPtr' may be used uninitialized in this function [-Wuninitialized]
conf/domain_conf.c:10375: error: 'cntPtr' may be used uninitialized in this function [-Wuninitialized]
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
'const fooPtr' is the same as 'foo * const' (the pointer won't
change, but it's contents can). But in general, if an interface
is trying to be const-correct, it should be using 'const foo *'
(the pointer is to data that can't be changed).
Fix up offenders in src/conf/domain_conf, and their fallout.
Several things to note: virObjectLock() requires a non-const
argument; if this were C++, we could treat the locking field
as 'mutable' and allow locking an otherwise 'const' object, but
that is a more invasive change, so I instead dropped attempts
to be const-correct on domain lookup. virXMLPropString and
friends require a non-const xmlNodePtr - this is because libxml2
is not a const-correct library. We could make the src/util/virxml
wrappers cast away const, but I figured it was easier to not
try to mark xmlNodePtr as const. Finally, virDomainDeviceDefCopy
was a rather hard conversion - it calls virDomainDeviceDefPostParse,
which in turn in the xen driver was actually modifying the domain
outside of the current device being visited. We should not be
adding a device on the first per-device callback, but waiting until
after all per-device callbacks are complete.
* src/conf/domain_conf.h (virDomainObjListFindByID)
(virDomainObjListFindByUUID, virDomainObjListFindByName)
(virDomainObjAssignDef, virDomainObjListAdd): Drop attempt at
const.
(virDomainDeviceDefCopy): Use intended type.
(virDomainDeviceDefParse, virDomainDeviceDefPostParseCallback)
(virDomainVideoDefaultType, virDomainVideoDefaultRAM)
(virDomainChrGetDomainPtrs): Make const-correct.
* src/conf/domain_conf.c (virDomainObjListFindByID)
(virDomainObjListFindByUUID, virDomainObjListFindByName)
(virDomainDeviceDefCopy, virDomainObjListAdd)
(virDomainObjAssignDef, virDomainHostdevSubsysUsbDefParseXML)
(virDomainHostdevSubsysPciOrigStatesDefParseXML)
(virDomainHostdevSubsysPciDefParseXML)
(virDomainHostdevSubsysScsiDefParseXML)
(virDomainControllerModelTypeFromString)
(virDomainTPMDefParseXML, virDomainTimerDefParseXML)
(virDomainSoundCodecDefParseXML, virDomainSoundDefParseXML)
(virDomainWatchdogDefParseXML, virDomainRNGDefParseXML)
(virDomainMemballoonDefParseXML, virDomainNVRAMDefParseXML)
(virSysinfoParseXML, virDomainVideoAccelDefParseXML)
(virDomainVideoDefParseXML, virDomainHostdevDefParseXML)
(virDomainRedirdevDefParseXML)
(virDomainRedirFilterUsbDevDefParseXML)
(virDomainRedirFilterDefParseXML, virDomainIdMapEntrySort)
(virDomainIdmapDefParseXML, virDomainVcpuPinDefParseXML)
(virDiskNameToBusDeviceIndex, virDomainDeviceDefCopy)
(virDomainVideoDefaultType, virDomainHostdevAssignAddress)
(virDomainDeviceDefPostParseInternal, virDomainDeviceDefPostParse)
(virDomainChrGetDomainPtrs, virDomainControllerSCSINextUnit)
(virDomainSCSIDriveAddressIsUsed)
(virDomainDriveAddressIsUsedByDisk)
(virDomainDriveAddressIsUsedByHostdev): Fix fallout.
* src/openvz/openvz_driver.c (openvzDomainDeviceDefPostParse):
Likewise.
* src/libxl/libxl_domain.c (libxlDomainDeviceDefPostParse):
Likewise.
* src/qemu/qemu_domain.c (qemuDomainDeviceDefPostParse)
(qemuDomainDefaultNetModel): Likewise.
* src/lxc/lxc_domain.c (virLXCDomainDeviceDefPostParse):
Likewise.
* src/uml/uml_driver.c (umlDomainDeviceDefPostParse): Likewise.
* src/xen/xen_driver.c (xenDomainDeviceDefPostParse): Split...
(xenDomainDefPostParse): ...since per-device callback is not the
time to be adding a device.
Signed-off-by: Eric Blake <eblake@redhat.com>
virDomainChrGetDomainPtrs() required 4 levels of pointers (taking
a parameter that will be used as an output variable to return the
address of another variable that contains an array of pointers).
This is rather complex to reason about, especially when outside
of the domain_conf file, no other caller should be modifying
the resulting array of pointers directly. Changing the public
signature gives something is easier to reason with, and actually
make const-correct; which is important as it was the only function
that was blocking virDomainDeviceDefCopy from treating its source
as const.
* src/conf/domain_conf.h (virDomainChrGetDomainPtrs): Use simpler
types, and make const-correct for external users.
* src/conf/domain_conf.c (virDomainChrGetDomainPtrs): Split...
(virDomainChrGetDomainPtrsInternal): ...into an internal version
that lets us modify terms, vs. external form that is read-only.
(virDomainDeviceDefPostParseInternal, virDomainChrFind)
(virDomainChrInsert): Adjust callers.
* src/qemu/qemu_command.c (qemuGetNextChrDevIndex): Adjust caller.
(qemuDomainDeviceAliasIndex): Make const-correct.
Signed-off-by: Eric Blake <eblake@redhat.com>
Since 76b644c when the support for RAM filesystems was introduced,
libvirt accepted the following XML:
<source usage='1024' unit='KiB'/>
This was parsed correctly and internally stored in bytes, but it
was formatted as (with an extra 's'):
<source usage='1024' units='KiB'/>
When read again, this was treated as if the units were missing,
meaning libvirt was unable to parse its own XML correctly.
The usage attribute was documented as being in KiB, but it was not
scaled if the unit was missing. Transient domains still worked,
because this was balanced by an extra 'k' in the mount options.
This patch:
Changes the parser to use 'units' instead of 'unit', as the latter
was never documented (fixing persistent domains) and some programs
(libvirt-glib, libvirt-sandbox) already parse the 'units' attribute.
Removes the extra 'k' from the tmpfs mount options, which is needed
because now we parse our own XML correctly.
Changes the default input unit to KiB to match documentation, fixing:
https://bugzilla.redhat.com/show_bug.cgi?id=1015689
Again stolen from qemu_driver.c, but dropping all the unneeded bits.
This aims to copy all the current qemu validation checks since that's
the most commonly used real driver, but some of the checks are
completely artificial in the test driver.
This only supports creation of internal snapshots for initial
simplicity.
Since commit 297c99a5 an invalid source definition XML of a character
device that is used as backend for RNG devices, smartcards and redirdevs
causes crash of the daemon when parsing such a definition.
The device types mentioned above are not a part of a regular character
device but are backends for other types. Thus when parsing such device
NULL is passed as the argument @chr_def. Later when checking the
validity of the definition @chr_def was dereferenced when parsing a UNIX
socket backend with missing path of the socket and crashed the daemon.
Sample offending configuration:
<devices>
...
<rng model='virtio'>
<backend model='egd' type='unix'>
<source mode='bind' service='1024'/>
</backend>
</rng>
</devices>
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1012196
If virDomainSoundCodecDefParseXML returns an error (eg due
to OOM), then the xml nodeset codecNodes is leaked.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
If virDomainVcpuPinDefArrayFree is called with def != NULL,
but nvcpupin == 0, then it leaks memory for 'def'. This is
an unusual scenario, but it hits when cleaning up after an
OOM during parsing of XML.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
This resolves one of the issues in:
https://bugzilla.redhat.com/show_bug.cgi?id=1003983
This device is identical to qemu's "intel-hda" device (known as "ich6"
in libvirt), but has a different PCI device ID (which matches the ID
of the hda audio built into the ich9 chipset, of course). It's not
supported in earlier versions of qemu, so it requires a capability
bit.
The virDomainDefParseXML method did not check the return value
of the virBitmapNew API call for NULL. This lead to a crash on
OOM
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
If an OOM error occurs in virSecurityDeviceLabelDefParseXML the
cleanup code may free an uninitialized pointer, causing a crash
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
The ABI compatibility check for domain features didn't check the
expanded HyperV and APIC EOI values, thus possibly allowing change in
guest ABI.
Add the check and use typecasted switch statement to warn developers
when adding a new HyperV feature.
Currently the XML parser already allows the following syntax:
<disk type='block' device='cdrom'>
<source startupPolicy='optional'/>
<target dev='hda' bus='ide'/>
<address type='drive' controller='0' bus='0' target='0' unit='0'/>
</disk>
But it if the dev value is NULL then it would not have the leading
"<source ", resulting in invalid XML.
qemu/KVM also supports a tftp URL while specifying the cdrom ISO image.
The xml should be as following:
<disk type='network' device='cdrom'>
<source protocol='tftp' name='/url/path'>
<host name='host.name' port='69'/>
</source>
</disk>
Signed-off-by: Aline Manera <alinefm@br.ibm.com>
The ftps protocol is another protocol supported by qemu/KVM while specifying
the cdrom ISO image.
The xml should be as following:
<disk type='network' device='cdrom'>
<source protocol='ftps' name='/url/path'>
<host name='host.name' port='990'/>
</source>
</disk>
Signed-off-by: Aline Manera <alinefm@br.ibm.com>
The https protocol is also accepted by qemu/KVM when specifying the cdrom ISO
image.
The xml should be as following:
<disk type='network' device='cdrom'>
<source protocol='https' name='/url/path'>
<host name='host.name' port='443'/>
</source>
</disk>
Signed-off-by: Aline Manera <alinefm@br.ibm.com>
GCC 4.8.0+ whines about variable "new" being uninitialized since
commit 73bfac0e7182a3abd. This is a false positive as the
xmlFreeNode(new) statement can be only reached if new was actually
allocated successfully.
CC conf/libvirt_conf_la-domain_conf.lo
conf/domain_conf.c: In function 'virDomainDefSetMetadata':
conf/domain_conf.c:18650:24: error: 'new' may be used uninitialized in this function [-Werror=maybe-uninitialized]
xmlFreeNode(new);
Reported independently by John Ferlan and Michal Privoznik.
Eric Blake suggested that we could do a little better in case copying of
the metadata to be set fails. With this patch, the old metadata is
discarded after the new string is copied successfuly.
The virDomainGetMetadata function was designed to support also retrieval
of app specific metadata from the <metadata> element. This functionality
was never implemented originally.
The function implemented common behavior that can be reused for other
hypervisor drivers that use the virDomainObj data structures. Factor out
the core into a separate helper func.
The function implemented common behavior that can be reused for other
hypervisor drivers that use the virDomainObj data structures. Factor out
the core into a separate helper func.
CD-ROMs and Floppies are allowed to have no source to imply they are
empty or disconnected. Since the LUN type is used for raw CD-ROM access
with QEMU (and VMWare in the future), it also needs to allow an empty
source when the raw CD-ROM device is disconnected from the domain.
After freeing the bitmap pointer, it must set the pointer to NULL.
This will avoid any other use of the freed memory of the bitmap pointer.
https://bugzilla.redhat.com/show_bug.cgi?id=1006710
Signed-off-by: Liuji (Jeremy) <jeremy.liu@huawei.com>
'virsh domxml-from-native' and 'virsh qemu-attach' could misbehave
for an emulator installed in (a somewhat unlikely) location
such as /usr/local/qemu-1.6/qemu-system-x86_64 or (an even less
likely) /opt/notxen/qemu-system-x86_64. Limit the strstr seach
to just the basename of the file where we are assuming details
about the binary based on its name.
While testing, I accidentally triggered a core dump during strcmp
when I forgot to set os.type on one of my code paths; this patch
changes such a coding error to raise a nicer internal error instead.
* src/qemu/qemu_command.c (qemuParseCommandLine): Compute basename
earlier.
* src/conf/domain_conf.c (virDomainDefPostParseInternal): Avoid
NULL deref.
Signed-off-by: Eric Blake <eblake@redhat.com>
Delete the USB controller check from the USB Device checklist in
virDomainDeviceIsUSB as USB controller is a PCI device rather than
a USB one.
Signed-off-by: Liu Ji <jeremy.liu@huawei.com>
The VIR_FREE() macro will cast away any const-ness. This masked a
number of places where we passed a 'const char *' string to
VIR_FREE. Fortunately in all of these cases, the variable was not
in fact const data, but a heap allocated string. Fix all the
variable declarations to reflect this.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
In commit 991270db99690 I've used virDomainNetGetActualHostdev() to get
the actual hostdev from a network when removing the network from the
list to avoid leaving the hostdev in the list. I didn't notice that this
function doesn't check if the actual network is allocated and
dereferences it. This crashes the daemon when cleaning up a domain
object in early startup phases when the actual network definition isn't
allocated. When the actual definition isn't present, the hostdev that
might correspond to it won't be present anyways so it's safe to return
NULL.
Thanks to Cole Robinson for noticing this problem.
Commit 50348e6edfa reused the code to remove the hostdev portion of a
network definition on multiple places but forgot to take into account
that sometimes the "actual" network is passed and in some cases the
parent of that.
This patch uses the virDomainNetGetActualHostdev() helper to acquire the
correct pointer all the time while removing the hostdev portion from the
list.
Starting with qemu 1.6, the qemu-system-arm vexpress-a9 model has a
hardcoded virtio-mmio transport which enables attaching all virtio
devices.
On the command line, we have to use virtio-XXX-device rather than
virtio-XXX-pci, thankfully s390 already set the precedent here so
it's fairly straight forward.
At the XML level, this adds a new device address type virtio-mmio.
The controller and addressing don't have any subelements at the
moment because we they aren't needed for this usecase, but could
be added later if needed.
Add a test case for an ARM guest with one of every virtio device
enabled.
This corresponds to '-sd' and '-drive if=sd' on the qemu command line.
Needed for many ARM boards which don't provide any other way to
pass in storage.
Add an attribute named 'removable' to the 'target' element of disks,
which controls the removable flag. For instance, on a Linux guest it
controls the value of /sys/block/$dev/removable. This option is only
valid for USB disks (i.e. bus='usb'), and its default value is 'off',
which is the same behaviour as before.
To achieve this, 'removable=on' (or 'off') is appended to the '-device
usb-storage' parameter sent to qemu when adding a USB disk via
'-disk'. A capability flag QEMU_CAPS_USB_STORAGE_REMOVABLE was added
to keep track if this option is supported by the qemu version used.
Bug: https://bugzilla.redhat.com/show_bug.cgi?id=922495
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
When using a <interface type="network"> that points to a network with
hostdev forwarding mode a hostdev alias is created for the network. This
allias is inserted into the hostdev list, but is backed with a part of
the network object that it is connected to.
When a VM is being stopped qemuProcessStop() calls
networkReleaseActualDevice() which eventually frees the memory for the
hostdev object. Afterwards when the domain definition is being freed by
virDomainDefFree() an invalid pointer is accessed by
virDomainHostdevDefFree() and may cause a crash of the daemon.
This patch removes the entry in the hostdev list before freeing the
depending memory to avoid this issue.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1000973
<controller type='pci' index='0' model='pci-root'>
<pcihole64 unit='KiB'>1048576</pcihole64>
</controller>
It can be used to adjust (or disable) the size of the 64-bit
PCI hole. The size attribute is in kilobytes (different unit
can be specified on input), but it gets rounded up to
the nearest GB by QEMU.
Disabling it will be needed for guests that crash with the
64-bit PCI hole (like Windows XP), see:
https://bugzilla.redhat.com/show_bug.cgi?id=990418
