When creating qemu capabilities, a dummy virDomainObj is created just
because our monitor code expects that. However, the object is created
locked already. Then, under cleanup label, we simply unref the object
which results in whole domain object to be disposed. The object lock
is destroyed subsequently, but hey - it's still locked:
==24845== Thread #14's call to pthread_mutex_destroy failed
==24845== with error code 16 (EBUSY: Device or resource busy)
==24845== at 0x4C3024E: pthread_mutex_destroy (in /usr/lib64/valgrind/vgpreload_helgrind-amd64-linux.so)
==24845== by 0x531F72E: virMutexDestroy (virthread.c:83)
==24845== by 0x5302977: virObjectLockableDispose (virobject.c:237)
==24845== by 0x5302A89: virObjectUnref (virobject.c:265)
==24845== by 0x1DD37866: virQEMUCapsInitQMP (qemu_capabilities.c:3397)
==24845== by 0x1DD37CC6: virQEMUCapsNewForBinary (qemu_capabilities.c:3481)
==24845== by 0x1DD381E2: virQEMUCapsCacheLookup (qemu_capabilities.c:3609)
==24845== by 0x1DD30F8A: virQEMUCapsInitGuest (qemu_capabilities.c:744)
==24845== by 0x1DD31889: virQEMUCapsInit (qemu_capabilities.c:1020)
==24845== by 0x1DD7DD36: virQEMUDriverCreateCapabilities (qemu_conf.c:888)
==24845== by 0x1DDC57C0: qemuStateInitialize (qemu_driver.c:803)
==24845== by 0x53DC743: virStateInitialize (libvirt.c:777)
==24845==
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
in virDomainFSInfoFree(), don't free the virDomainFSInfo data.
==10670== 80 bytes in 2 blocks are definitely lost in loss record 576 of 793
==10670== at 0x4A06BC3: calloc (vg_replace_malloc.c:618)
==10670== by 0x509DEBD: virAlloc (viralloc.c:144)
==10670== by 0x19FBD558: qemuAgentGetFSInfo (qemu_agent.c:1837)
==10670== by 0x1A03CF91: qemuDomainGetFSInfo (qemu_driver.c:19238)
Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
Commit 4bbe1029f fixed a problem in commit f7afeddc by moving the call
to virNetDevGetIndex() to a location common to all interface types (so
that the nicindex array would be filled in for macvtap as well as tap
interfaces), but the location was *too* common, as the original call
to virNetDevGetIndex() had been in a section qualified by "if
(cfg->privileged)". The result was that the "fixed" libvirtd would try
to call virNetDevGetIndex() even for session mode libvirtd, and end up
failing with the log message:
Unable to open control socket: Operation not permitted
To remedy that, this patch qualifies the call to virNetDevGetIndex()
in its new location with cfg->privileged.
This resolves https://bugzilla.redhat.com/show_bug.cgi?id=1198244
As of bba93d40 all of our RPC objects are derived from
virObjectLockable. However, during rewrite some errors sneaked
in. For instance, the dispose functions to virNetClient and
virNetServerClient objects were not only freeing allocated
memory, but unlocking themselves. This is wrong. Object should
never disappear while locked.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Well, one day this will be self-locking object, but not today.
But lets prepare the code for that! Moreover,
virNetworkObjListFree() is no longer needed, so turn it into
virNetworkObjListDispose().
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
This API will be used in the future to call passed callback over
each network object in the list. It's slightly different to its
virDomainObjListForEach counterpart, because virDomainObjList
uses a hash table to store domain object, while virNetworkObjList
uses an array.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Commit v1.2.4-52-gda879e5 fixed issues with domains started before
sanlock driver was enabled by checking whether a running domain is
registered with sanlock and if it's not, sanlock driver is basically
ignored for the domain.
However, it was checking this even for domain which has just been
started and no sanlock_* API was called for them yet. This results in
cmd 9 target pid 2135544 not found
error messages to appear in sanlock.log whenever we start a new domain.
This patch avoids this useless check for freshly started domains.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
virLockManager*New APIs are never called with
VIR_LOCK_MANAGER_USES_STATE. Moreover, lockd driver does not maintain
any state that would need to be transferred during migration and thus it
should not mention VIR_LOCK_MANAGER_USES_STATE at all.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
By adding a call and check of return of virBitmapToData to the
IOThreads code, my Coverity checker lets me know qemuDomainHelperGetVcpus
also needs to check the status...
We have something like pvpanic device. However, in some cases it does
not have any address assigned, in which case we produce this ugly XML
(still valid though):
<devices>
<emulator>/usr/bin/qemu</emulator>
...
<panic>
</panic>
</devices>
Lets format "<panic/>" instead.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Depending on the flags passed, either attempt to return the active/live
IOThread data for the domain or the config data.
The active/live path will call into the Monitor in order to get the
IOThread data and then correlate the thread_id's returned from the
monitor to the currently running system/threads in order to ascertain
the affinity for each iothread_id.
The config path will map each of the configured IOThreads and return
any configured iothreadspin data
Signed-off-by: John Ferlan <jferlan@redhat.com>
Add virDomainGetIOThreadInfo in order to return a list of
virDomainIOThreadInfoPtr structures which list the IOThread ID
and the CPU Affinity map for each IOThread for the domain.
For an active domain, the live data will be returned, while for
an inactive domain, the config data will be returned.
The API supports either the --live or --config flag, but not both.
Also added virDomainIOThreadsInfoFree in order to free the cpumap
and the IOThreadInfo structure.
Signed-off-by: John Ferlan <jferlan@redhat.com>
There was a mess in the way how we store unlimited value for memory
limits and how we handled values provided by user. Internally there
were two possible ways how to store unlimited value: as 0 value or as
VIR_DOMAIN_MEMORY_PARAM_UNLIMITED. Because we chose to store memory
limits as unsigned long long, we cannot use -1 to represent unlimited.
It's much easier for us to say that everything greater than
VIR_DOMAIN_MEMORY_PARAM_UNLIMITED means unlimited and leave 0 as valid
value despite that it makes no sense to set limit to 0.
Remove unnecessary function virCompareLimitUlong. The update of test
is to prevent the 0 to be miss-used as unlimited in future.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1146539
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
The first one is to truncate the memory limit to
VIR_DOMAIN_MEMORY_PARAM_UNLIMITED if the value is greater and the second
one is to decide whether the memory limit is set or not, unlimited means
that it's not set.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Pass the TPM file descriptor to QEMU via command line.
Instead of passing /dev/tpm0 we now pass /dev/fdset/10 and the additional
parameters -add-fd set=10,fd=20.
This addresses the use case when QEMU is started with non-root privileges
and QEMU cannot open /dev/tpm0 for example.
Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
Implement virCommandPassFDGetFDIndex to determine the index a given
file descriptor will have when passed to the child process.
Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
In the old days of a global driver lock, it was necessary to unlock
the driver after a domain restore operation. When the global lock
was removed from the driver, some remnants were left behind in
libxlDomainRestoreFlags. Remove this unneeded (and incorrect) code.
Signed-off-by: Jim Fehlig <jfehlig@suse.com>
Domain death watch is already disabled in libxlDomainCleanup. No
need to disable it a second and third time.
Signed-off-by: Jim Fehlig <jfehlig@suse.com>
Since the APIs support just one element per namespace and while
modifying an element all duplicates would be removed, let's do this
right away in the post parse callback.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1190590
Adding functionality to libvirt that will allow it
query the ethtool interface for the availability
of certain NIC HW offload features
Here is an example of the feature XML definition:
<device>
<name>net_eth4_90_e2_ba_5e_a5_45</name>
<path>/sys/devices/pci0000:00/0000:00:03.0/0000:08:00.1/net/eth4</path>
<parent>pci_0000_08_00_1</parent>
<capability type='net'>
<interface>eth4</interface>
<address>90:e2:ba:5e:a5:45</address>
<link speed='10000' state='up'/>
<feature name='rx'/>
<feature name='tx'/>
<feature name='sg'/>
<feature name='tso'/>
<feature name='gso'/>
<feature name='gro'/>
<feature name='rxvlan'/>
<feature name='txvlan'/>
<feature name='rxhash'/>
<capability type='80203'/>
</capability>
</device>
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Well, the parallelsConnectOpen() joins several sub-driver openings
into one big if condition. If any of sub-driver fails to open, the
whole API finishes immediately. The problem is, sub-drivers may have
left some memory allocated. Fortunately, we have a free function for
that: parallelsConnectClose(). This is, however, not prepared for
partially allocated driver structure. So, prepare the free function
for it and call it at the right place, in the if body.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
According to the POSIX standard, off_t (returned by lseek) is defined as
signed integral type no shorter than int. Because our offset variable is defined
as unsigned long long, the original check was passed successfully if UINT64_MAX had
been used as offset value, due to implicit conversion.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1177219
When the domain's source disk type is network, if source protocol is rbd
or sheepdog, the 'if().. break' will end the current case, which lead to
miss check the driver type is raw or qcow2. Libvirt will allow to create
internal snapshot for a running domain with raw format disk which based
on rbd storage.
While both protocols support internal snapshots of the disk qemu is not
able to use it as it requires some place to store the memory image. The
check if the disk is backed by a qcow2 image needs to be executed
always.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1179533
Signed-off-by: Shanzhi Yu <shyu@redhat.com>
Previously when a domain would get stuck in a domain job due to a
programming mistake we'd report the following control state:
$ virsh domcontrol domain
occupied (1424343406.150s)
The timestamp is invalid as the monitor was not entered for that domain.
We can use that to detect that the domain has an active job and report a
better error instead:
$ virsh domcontrol domain
error: internal (locking) error
In order to hide the object internals (and use just accessors
everywhere), lets store a pointer to the object, instead of object
itself.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
In order to hide the object internals (and use just accessors
everywhere), lets store a pointer to the object, instead of object
itself.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
In order to hide the object internals (and use just accessors
everywhere), lets store a pointer to the object, instead of object
itself.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Instead of copying the whole object onto stack when calling the
function, just pass the pointer to the object and save up some
space on the stack. Moreover, this prepares the code to hide the
virNetworkObjList structure into network_conf.c and use accessors
only.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
All of our vir*Free() functions should accept NULL, even though
that there's no way of actually passing NULL with current code.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
This is probably a copy-paste error from virDomainObj*
counterpart. But when speaking of virNetworkObj we should use
variable @nets for an array of networks, rather than @doms. It's
just confusing.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Moreover, there are two points within the function, where we're
missing 'goto cleanup'. Fix this too.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Silly this bug went unnoticed so long. At the beginning we try to
find the passed network in the list of network objects. If found,
it's locked and real work takes place. Then, in the end, the
network object is never unlocked.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Okay, this is mainly for educational purposes since is called
from single point only with all the possible locks held. So
there's no way for other thread to hop in and do something wrong.
Nevertheless, we should not give bad example.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
We have this function networkObjFromNetwork() which for given
virNetworkPtr tries to find corresponding virNetworkObjPtr. If no
object is found, a nice error message is printed out:
no network with matching uuid '$uuid' ($name)
Let's improve the error message produced by networkLookupByUUID to
follow that logic.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
https://bugzilla.redhat.com/show_bug.cgi?id=1197600
So, libvirt uses pid file to track pid of started qemus. Whenever
a domain is started, its pid is put into corresponding pid file.
The pid file path is generated based on domain name and stored
into domain object internals. However, it's not stored in the
status XML and therefore lost on daemon restarts. Hence, later,
when domain is being shut down, the daemon does not know which
pid file to unlink, and the correct pid file is left behind. To
avoid this, lets generate the pid file path again in
qemuProcessReconnect().
Reported-by: Luyao Huang <lhuang@redhat.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Instead of checking defaultMode for every channel that has no mode
configured, test it only once outside of channel loop. This fixes a bug
that in case all possible channels are fore example set to insecure, but
defaultMode is set to secure, we wouldn't auto-generate TLS port. This
results in failure while starting a guest.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1143832
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
We have two different places that needs to be updated while touching
code for allocation spice ports. Add a bool option to
'qemuProcessSPICEAllocatePorts' function to switch between true and fake
allocation so we can use this function also in qemu_driver to generate
native domain definition.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Since adding the support for scheduler policy settings in commit
8680ea97, there are two enums with the same information. That was
caused by rewriting the patch since first draft.
Find out thanks to clang, but there was no impact whatsoever.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
The problem here was that when opening a channel, we were checking
whether the channel given is alias (can't be NULL for running domain) or
it's name, which can be NULL (for example with spicevmc). In case of
such domain qemuDomainOpenChannel() made the daemon crash.
STREQ_NULLABLE() is safe to use since the code in question is wrapped in
"if (name)" and is more readable, so use that instead of checking for
non-NULL "vm->def->channels[i]->target.name".
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
The virStorageBackendISCSIFindPoolSources API only needs the 'host' name
in order to discover iSCSI pools, it returns the various device paths.
On input, it's also possible to further restrict a search by providing the
port attribute for the host element and the (undocumented) initiator element.
For example:
$ virsh find-storage-pool-sources-as iscsi
error: Failed to find any iscsi pool sources
error: invalid argument: hostname and device path must be specified for iscsi sources
$ virsh find-storage-pool-sources-as iscsi 192.168.122.1
<sources>
<source>
<host name='192.168.122.1' port='3260'/>
<device path='iqn.2013-12.com.example:iscsi-chap-lclpool'/>
</source>
</sources>
https://bugzilla.redhat.com/show_bug.cgi?id=1181062
According to the formatstorage.html description for <source> element
and "format" attribute: "All drivers are required to have a default
value for this, so it is optional."
As it turns out the disk backend did not choose a default value, so I
added a default of "msdos" if the source type is "unknown" as well as
updating the storage.html backend disk volume driver documentation to
indicate the default format is dos.
https://bugzilla.redhat.com/show_bug.cgi?id=1142631
This patch resolves a situation where the same "<target dev='$name'...>"
can be used for multiple disks in the domain.
While the $name is "mostly" advisory regarding the expected order that
the disk is added to the domain and not guaranteed to map to the device
name in the guest OS, it still should be unique enough such that other
domblk* type operations can be performed.
Without the patch, the domblklist will list the same Target twice:
$ virsh domblklist $dom
Target Source
------------------------------------------------
sda /var/lib/libvirt/images/file.qcow2
sda /var/lib/libvirt/images/file.img
Additionally, getting domblkstat, domblkerror, domblkinfo, and other block*
type calls will not be able to reference the second target.
Fortunately, hotplug disallows adding a "third" sda value:
$ qemu-img create -f raw /var/lib/libvirt/images/file2.img 10M
$ virsh attach-disk $dom /var/lib/libvirt/images/file2.img sda
error: Failed to attach disk
error: operation failed: target sda already exists
$
BUT, it since 'sdb' doesn't exist one would get the following on the same
hotplug attempt, but changing to use 'sdb' instead of 'sda'
$ virsh attach-disk $dom /var/lib/libvirt/images/file2.img sdb
error: Failed to attach disk
error: internal error: unable to execute QEMU command 'device_add': Duplicate ID 'scsi0-0-1' for device
$
Since we cannot fix this issue at parsing time, the best that can be done so
as to not "lose" a domain is to make the check prior to starting the guest
with the results as follows:
$ virsh start $dom
error: Failed to start domain $dom
error: XML error: target 'sda' duplicated for disk sources '/var/lib/libvirt/images/file.qcow2' and '/var/lib/libvirt/images/file.img'
$
Running 'make check' found a few more instances in the tests where this
duplicated target dev value was being used. These also exhibited some
duplicated 'id=' values (negating the uniqueness argument of aliases) in
the corresponding .args file and of course the *xmlout version of a few
input XML files.
NUMA enabled guest configuration explicitly specifies memory sizes for
individual nodes. Allowing the virDomainSetMemoryFlags API (and friends)
to change the total doesn't make sense as the individual node configs
are not updated in that case.
Forbid use of the API in case NUMA is specified.
Add VIR_VOL_XML_PARSE_OPT_CAPACITY flag to virStorageVolDefParseXML.
With this flag, no error is reported when the capacity is missing
if there is a backing store.
Instead of just looking at the output of fstat, call
virStorageFileGetMetadata to get the full capacity from
image headers.
Note that the capacity is probed unconditionally. The updateCapacity
bool parameter is ignored and will be removed in the following commit.
In virStorageVolCreateXML, add VIR_VOL_XML_PARSE_NO_CAPACITY
to the call parsing the XML of the new volume to make the capacity
optional.
If the capacity is omitted, use the capacity of the old volume.
We already do that for values that are less than the original
volume capacity.
If we combine the boot order on the command line with other
boot options, we prepend order= in front of it.
Instead of checking if the number of added arguments is between
0 and 2, separate the strings for boot order and options
and prepend boot order only if both strings are not empty.
Commit 6992994 started filling the listen attribute
of the parent <graphics> elements from type='network' listens.
When this XML is passed to UpdateDevice, parsing fails:
XML error: graphics listen attribute 10.20.30.40 must match
address attribute of first listen element (found none)
Ignore the address in the parent <graphics> attribute
when no type='address' listens are found,
the same we ignore the address for the <listen> subelements
when parsing inactive XML.
The gluster volume name extraction code was copied from the XML parser
without changing the VIR_ERR_XML_ERROR error code. Use
VIR_ERR_CONFIG_UNSUPPORTED instead.
Similar to commit fdb80ed4f6 libvirtd
would crash if a gluster URI without path would be used in the backing
chain of a volume. The crash happens in the gluster specific part of the
parser that extracts the gluster volume name from the path.
Fix the crash by checking that the PATH is NULL.
This patch does not contain a test case as it's not possible to test it
with the current infrastructure as the test suite would attempt to
contact the gluster server in the URI. I'm working on the test suite
addition but that will be post-release material.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1196528
In virNetworkDHCPHostDefParseXML an error is reported
when partialOkay == true, and none of ip, mac, name
were supplied.
Add the missing goto and error out in this case.
https://bugzilla.redhat.com/show_bug.cgi?id=1196503
We already check whether the host id is valid or not, add a jump
to forbid invalid host id.
Signed-off-by: Luyao Huang <lhuang@redhat.com>
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Commit f7afeddc added code to report to systemd an array of interface
indexes for all tap devices used by a guest. Unfortunately it not only
didn't add code to report the ifindexes for macvtap interfaces
(interface type='direct') or the tap devices used by type='ethernet',
it ended up sending "-1" as the ifindex for each macvtap or hostdev
interface. This resulted in a failure to start any domain that had a
macvtap or hostdev interface (or actually any type other than
"network" or "bridge").
This patch does the following with the nicindexes array:
1) Modify qemuBuildInterfaceCommandLine() to only fill in the
nicindexes array if given a non-NULL pointer to an array (and modifies
the test jig calls to the function to send NULL). This is because
there are tests in the test suite that have type='ethernet' and still
have an ifname specified, but that device of course doesn't actually
exist on the test system, so attempts to call virNetDevGetIndex() will
fail.
2) Even then, only add an entry to the nicindexes array for
appropriate types, and to do so for all appropriate types ("network",
"bridge", and "direct"), but only if the ifname is known (since that
is required to call virNetDevGetIndex().
Previously this function relied on having ATTRIBUTE_NONNULL(1) in its
prototype rather than explicitly checking for a null
ifname. Unfortunately, ATTRIBUTE_NONNULL is just a hint to the
optimizer and code analyzers like Coverity, it doesn't actually check
anything at execution time, so the result was possible warnings from
Coverity, along with the possibility of null dereferences when ifname
wasn't available.
This patch removes the ATTRIBUTE_NONNULL from the prototype, and
checks ifname inside the function, logging an error if it's NULL (once
we've determined that the user really is trying to set a bandwidth).
libvirt was unconditionally calling virNetDevBandwidthClear() for
every interface (and network bridge) of a type that supported
bandwidth, whether it actually had anything set or not. This doesn't
hurt anything (unless ifname == NULL!), but is wasteful.
This patch makes sure that all calls to virNetDevBandwidthClear() are
qualified by checking that the interface really had some bandwidth
setup done, and checks for a null ifname inside
virNetDevBandwidthClear(), silently returning success if it is null
(as well as removing the ATTRIBUTE_NONNULL from that function's
prototype, since we can't guarantee that it is never null,
e.g. sometimes a type='ethernet' interface has no ifname as it is
provided on the fly by qemu).
If the qemu binary on x86 does not support lsi SCSI controller,
but it supports virtio-scsi, we reject the virtio-specific attributes
for no reason.
Move the default controller assignment before the check.
https://bugzilla.redhat.com/show_bug.cgi?id=1168849
https://bugzilla.redhat.com/show_bug.cgi?id=1183869
Soo. you've successfully started yourself a domain. And since you want
to use it on your host exclusively you are confident enough to
passthrough the host CPU model, like this:
<cpu mode='host-passthrough'/>
Then, after a while, you want to save the domain into a file (e.g.
virsh save dom dom.save). And here comes the trouble. The file consist
of two parts: Libvirt header (containing domain XML among other
things), and qemu migration data. Now, the domain XML in the header is
formatted using special flags (VIR_DOMAIN_XML_SECURE |
VIR_DOMAIN_XML_UPDATE_CPU | VIR_DOMAIN_XML_INACTIVE |
VIR_DOMAIN_XML_MIGRATABLE).
Then, on your way back from the bar, you think of changing something
in the XML in the saved file (we have a command for it after all), say
listen address for graphics console. So you successfully type in the
command:
virsh save-image-edit dom.save
Change all the bits, and exit the editor. But instead of success
you're left with sad error message:
error: unsupported configuration: Target CPU model <null> does not
match source Pentium Pro
Sigh. Digging into the code you see lines, where we check for ABI
stability. The new XML you've produced is compared with the old one
from the saved file to see if qemu ABI will break or not. Wait, what?
We are using different flags to parse the XML you've provided so we
were just lucky it worked in some cases? Yep, that's right.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Well, not that we are not formatting invalid XML, rather than not as
beautiful as we can:
<cpu mode='host-passthrough'>
</cpu>
If there are no children, let's use the singleton element.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Well, so far there are no variables to free, no cleanup work needed on
an error, so bare 'return -1;' after each error is just okay. But this
will change in a while.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
This API joins the following two lines:
char *s = virBufferContentAndReset(buf1);
virBufferAdd(buf2, s, -1);
into one:
virBufferAddBuffer(buf2, buf1);
With one exception: there's no re-indentation applied to @buf1.
The idea is, that in general both can have different indentation
(like the test I'm adding proves)
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
In commit cc41c648 I've re-factored qemuMonitorFindBalloonObjectPath, but
missed that there is a memory leak. The "nextpath" variable is
overwritten while looping in for cycle and we have to free it before next
cycle.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
https://bugzilla.redhat.com/show_bug.cgi?id=1151942
While the restriction doesn't have origin in any RFC, it matters
to us while constructing the dnsmasq config file (or command line
previously). For better picture, this is how the corresponding
part of network XML look like:
<dns>
<forwarder addr='8.8.4.4'/>
<txt name='example' value='example value'/>
</dns>
And this is how the config file looks like then:
server=8.8.4.4
txt-record=example,example value
Now we can see why there can't be any commas in the TXT name.
They are used by dnsmasq to separate @name and @value.
Funny, we have it in the documentation, but the code (which was
pushed back in 2011) didn't reflect that.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Making use of the ARCH_IS_S390 macro introduced with
e808357528
Signed-off-by: Stefan Zimmermann <stzi@linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com>
Since s390 does not support usb the default creation of a usb controller
for a domain should not occur.
Also adjust s390 test cases by removing usb device instances since
usb devices are no longer created by default for s390 the s390
test cases need to be adjusted.
Signed-off-by: Stefan Zimmermann <stzi@linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com>
This implement handling of <backenddomain name=''/> parameter introduced
in previous patch.
Works on Xen >= 4.3, because only there libxl supports setting backend
domain by name. Specifying backend domain by ID or UUID is currently not
supported.
Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
At least Xen supports backend drivers in another domain (aka "driver
domain"). This patch introduces an XML config option for specifying the
backend domain name for <disk> and <interface> devices. E.g.
<disk>
<backenddomain name='diskvm'/>
...
</disk>
<interface type='bridge'>
<backenddomain name='netvm'/>
...
</interface>
In the future, same option will be needed for USB devices (hostdev
objects), but for now libxl doesn't have support for PVUSB.
Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
The function that parses the <forward> subelement of a network used to
fail/log an error if the network definition contained both a <pf>
element as well as at least one <interface> or <address> element. That
check was present because the configuration of a network should have
either one <pf>, one or more <interface>, or one or more <address>,
but never combinations of multiple kinds.
This caused a problem when libvirtd was restarted with a network
already active - when a network with a <pf> element is started, the
referenced PF (Physical Function of an SRIOV-capable network card) is
checked for VFs (Virtual Functions), and the <forward> is filled in
with a list of all VFs for that PF either in the form of their PCI
addresses (a list of <address>) or their netdev names (a list of
<interface>); the <pf> element is not removed though. When libvirtd is
restarted, it parses the network status and finds both the original
<pf> from the config, as well as the list of either <address> or
<interface>, fails the parse, and the network is not added to the
active list. This failure is often obscured because the network is
marked as autostart so libvirt immediately restarts it.
It seems odd to me that <interface> and <address> are stored in the
same array rather than keeping two separate arrays, and having
separate arrays would have made the check much simpler. However,
changing to use two separate arrays would have required changes in
more places, potentially creating more conflicts and (more
importantly) more possible regressions in the event of a backport, so
I chose to keep the existing data structure in order to localize the
change.
It appears that this problem has been in the code ever since support
for <pf> was added (0.9.10), but until commit
34cc3b2f10 (first in libvirt 1.2.4)
networks with interface pools were not properly marked as active on
restart anyway, so there is no point in backporting this patch any
further than that.
Later patches will need to access the full definition to do check the
memory size and thus the checking needs to be done after the whole
definition including devices is known.
For historical reasons data regarding NUMA configuration were split
between the CPU definition and numatune. We cannot do anything about the
XML still being split, but we certainly can at least store the relevant
data in one place.
This patch moves the NUMA stuff to the right place.
As virDomainNumatuneSet now doesn't allocate the virDomainNuma object
any longer it's not necessary to pass the pointer to a pointer to store
the object as it will not change any longer.
While touching the parameter definitions I've also changed the name of
the parameter to "numa".
Since our formatter now handles well if the config is allocated and not
filled we can safely always-allocate the NUMA config and remove the
ad-hoc allocation code.
This will help in later patches as the parser will be refactored to just
fill the data.
Move the existing virDomainDefNew to virDomainDefNewFull as it's setting
a few things in the conf and re-introduce virDomainDefNew as a function
without parameters for common use.
Do a content-aware check if formatting of the <numatune> element is
necessary. Later on the def->numa structure will be always present so we
cannot decide only on the basis whether it's allocated.
Shuffling around the logic will allow to simplify the code quite a bit.
As an additional bonus the change in the logic now reports an error if
automatic placement is selected and individual placement is configured.
Currently the code would exit without reporting an error as
virBitmapParse reports one only if it fails to parse the bitmap, whereas
the code was jumping to the error label even in case 0 cpus were
correctly parsed in the map.
It's easier to recalculate the number in the one place it's used as
having a separate variable to track it. It will also help with moving
the NUMA code to the separate module.
Name it virNumaMemAccess and add it to conf/numa_conf.[ch]
Note that to avoid a circular dependency the type of the NUMA cell
memAccess variable was changed to int. It will be turned back later
after the circular dependency will not exist.
The mask was stored both as a bitmap and as a string. The string is used
for XML output only. Remove the string, as it can be reconstructed from
the bitmap.
The test change is necessary as the bitmap formatter doesn't "optimize"
using the '^' operator.
Rewrite the function to save a few local variables and reorder the code
to make more sense.
Additionally the ncells_max member of the virCPUDef structure is used
only for tracking allocation when parsing the numa definition, which can
be avoided by switching to VIR_ALLOC_N as the array is not resized
after initial allocation.
For weird historical reasons NUMA cells are added as a subelement of
<cpu> while the actual configuration is done in <numatune>.
This patch splits out the cell parser code from cpu config to NUMA
config. Note that the changes to the code are minimal just to make it
work and the function will be refactored in the next patch.
For a while now there are two places that gather information about NUMA
related guest configuration. While the XML can't be changed we can at
least store the data in one place in the definition.
Rename the numatune_conf.[ch] files to numa_conf as later patches will
move the rest of the definitions from the cpu definition to this one.
Not all machine types support all devices, device properties, backends,
etc. So until we create a matrix of [machineType, qemuCaps], lets just
filter out some capabilities before we return them to the consumer
(which is going to make decisions based on them straight away).
Currently, as qemu is unable to tell which capabilities are (not)
enabled for given machine types, it's us who has to hardcode the matrix.
One day maybe the hardcoding will go away and we can create the matrix
dynamically on the fly based on a few monitor calls.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
1. Delete all boot devices for VM instance
2. Find the first HDD from XML and set it as bootable
Now we support only one boot device and it should be HDD.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Not all files we want to find using virFileFindResource{,Full} are
generated when libvirt is built, some of them (such as RNG schemas) are
distributed with sources. The current API was not able to find source
files if libvirt was built in VPATH.
Both RNG schemas and cpu_map.xml are distributed in source tarball.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
https://bugzilla.redhat.com/show_bug.cgi?id=1179678
When migrating with storage, libvirt iterates over domain disks and
instruct qemu to migrate the ones we are interested in (shared, RO and
source-less disks are skipped). The disks are migrated in series. No
new disk is transferred until the previous one hasn't been quiesced.
This is checked on the qemu monitor via 'query-jobs' command. If the
disk has been quiesced, it practically went from copying its content
to mirroring state, where all disk writes are mirrored to the other
side of migration too. Having said that, there's one inherent error in
the design. The monitor command we use reports only active jobs. So if
the job fails for whatever reason, we will not see it anymore in the
command output. And this can happen fairly simply: just try to migrate
a domain with storage. If the storage migration fails (e.g. due to
ENOSPC on the destination) we resume the host on the destination and
let it run on partly copied disk.
The proper fix is what even the comment in the code says: listen for
qemu events instead of polling. If storage migration changes state an
event is emitted and we can act accordingly: either consider disk
copied and continue the process, or consider disk mangled and abort
the migration.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Upon BLOCK_JOB_COMPLETED event delivery, we check if the job has
completed (in qemuMonitorJSONHandleBlockJobImpl()). For better image,
the event looks something like this:
"timestamp": {"seconds": 1423582694, "microseconds": 372666}, "event":
"BLOCK_JOB_COMPLETED", "data": {"device": "drive-virtio-disk0", "len":
8412790784, "offset": 409993216, "speed": 8796093022207, "type":
"mirror", "error": "No space left on device"}}
If "len" does not equal "offset" it's considered an error, and we can
clearly see "error" field filled in. However, later in the event
processing this case was handled no differently to case of job being
aborted via separate API. It's time that we start differentiate these
two because of the future work.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Currently, upon BLOCK_JOB_* event, disk->mirrorState is not updated
each time. The callback code handling the events checks if a blockjob
was started via our public APIs prior to setting the mirrorState.
However, some block jobs may be started internally (e.g. during
storage migration), in which case we don't bother with setting
disk->mirror (there's nothing we can set it to anyway), or other
fields. But it will come handy if we update the mirrorState in these
cases too. The event wasn't delivered just for fun - we've started the
job after all.
So, in this commit, the mirrorState is set to whatever job status
we've obtained. Of course, there are some actions on some statuses
that we want to perform. But instead of if {} else if {} else {} ...
enumeration, let's move to switch().
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
If 'virNumaGetHostNodeset()' fails then the error path will try to free
uninitialized pointer mem_mask. Introduced by commit af2a1f058.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
PowerPC : Forbid NULL CPU model with 'host-model' mode in qemu command line.
This ensures that an XML such as following:
...
<cpu mode='host-model'>
<model fallback='allow'/>
</cpu>
...
will not generate a '-cpu host,compat=(null)' command line with qemu-system-ppc64.
Signed-off-by: Prerna Saxena <prerna@linux.vnet.ibm.com>
PowerPC : Explicitly associate 'qemu-system-ppc64' as the
default emulator for all 64-bit PowerPC guests ( both Big & Little Endian )
Signed-off-by: Prerna Saxena <prerna@linux.vnet.ibm.com>
https://bugzilla.redhat.com/show_bug.cgi?id=1126762
Commit 43b67f introduced a deadlock issue when we use numatune
to change numa settings to a vm in session mode.
Jump to endjob instead of jump to cleanup.
Signed-off-by: Luyao Huang <lhuang@redhat.com>
So, when building the '-numa' command line, the
qemuBuildMemoryBackendStr() function does quite a lot of checks to
chose the best backend, or to check if one is in fact needed. However,
it returned that backend is needed even for this little fella:
<numatune>
<memory mode="strict" nodeset="0,2"/>
</numatune>
This can be guaranteed via CGroups entirely, there's no need to use
memory-backend-ram to let qemu know where to get memory from. Well, as
long as there's no <memnode/> element, which explicitly requires the
backend. Long story short, we wouldn't have to care, as qemu works
either way. However, the problem is migration (as always). Previously,
libvirt would have started qemu with:
-numa node,memory=X
in this case and restricted memory placement in CGroups. Today, libvirt
creates more complicated command line:
-object memory-backend-ram,id=ram-node0,size=X
-numa node,memdev=ram-node0
Again, one wouldn't find anything wrong with these two approaches.
Both work just fine. Unless you try to migrated from the older libvirt
into the newer one. These two approaches are, unfortunately, not
compatible. My suggestion is, in order to allow users to migrate, lets
use the older approach for as long as the newer one is not needed.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Periodically my Coverity scan will return a checked_return failure
for libxlDomainShutdownThread call to libxlDomainStart. Followed the
libxlAutostartDomain example in order to check the status, emit a
message, and continue on.
Jumping to the cleanup label prior to starting the container failed to
properly clean everything up that is handled by the virLXCProcessCleanup
which is called if virLXCProcessStop is called on failure after the
container properly starts. Most importantly is prior to this patch none
of the stop/release hooks, host device reattachment, and network cleanup
(that is reverse of virLXCProcessSetupInterfaces).
Signed-off-by: Luyao Huang <lhuang@redhat.com>
Modify the VIR_DEBUG message in virLXCProcessCleanup to make it clearer
about the path. Also add some more VIR_DEBUG messages in virLXCProcessStart
in order to help debug error flow.
https://bugzilla.redhat.com/show_bug.cgi?id=1176503
Move the two console checks - one for zero nconsoles present and the
other for an invalid console type to earlier in the processing rather than
getting after performing some setup that has to be undone for what amounts
to an invalid configuration.
This resolves the above bug since it's not not possible to have changed
the security labels when we cause the configuration check failure.
if (mgr == NULL || mgr->drv == NULL)
return ret;
This check isn't really necessary, security manager cannot be a NULL
pointer as it is either selinux (by default) or 'none', if no other driver is
set in the config. Even with no config file driver name yields 'none'.
The other hunk checks for domain's security model validity, but we should
also check devices' security model as well, therefore this hunk is moved into
a separate function which is called by virSecurityManagerCheckAllLabel that
checks both the domain's security model and devices' security model.
https://bugzilla.redhat.com/show_bug.cgi?id=1165485
Signed-off-by: Ján Tomko <jtomko@redhat.com>
We do have a check for valid per-domain security model, however we still
do permit an invalid security model for a domain's device (those which
are specified with <source> element).
This patch introduces a new function virSecurityManagerCheckAllLabel
which compares user specified security model against currently
registered security drivers. That being said, it also permits 'none'
being specified as a device security model.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1165485
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Add an XML attribute to allow disabling merge of rx buffers
on the host:
<interface ...>
...
<model type='virtio'/>
<driver ...>
<host mrg_rxbuf='off'/>
</driver>
</interface>
https://bugzilla.redhat.com/show_bug.cgi?id=1186886
The enum converters are defined in the domain_conf.h (so
accessible widely across the code), but on the symbol layer, only
virDomainNetTypeToString was exposed. However, FromString variant
is going to be needed shortly.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Commit b6a2828e introduced new functions to set process scheduler. There
is a small typo in ELSE path for systems where scheduler is not
available.
Also some of the definitions were introduced later in kernel. For
example RHEL-5 is running on kernel 2.6.18, but SCHED_IDLE was introduces
in 2.6.23 [1] and SCHED_BATCH in 2.6.16 [1]. We should not count only on
existence of function sched_setscheduler(), we must also check for
existence of used macros as they might not be defined.
[1] see 'man 7 sched'
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
While the main storage driver code allows the flag
VIR_STORAGE_VOL_RESIZE_SHRINK to be set, none of the backend
drivers are supporting it. At the very least this can work
for plain file based volumes since we just ftruncate() them
to the new size. It does not work with qcow2 volumes, but we
can arguably delegate to qemu-img for error reporting for that
instead of second guessing this for ourselves:
$ virsh vol-resize --shrink /home/berrange/VirtualMachines/demo.qcow2 2G
error: Failed to change size of volume 'demo.qcow2' to 2G
error: internal error: Child process (/usr/bin/qemu-img resize /home/berrange/VirtualMachines/demo.qcow2 2147483648) unexpected exit status 1: qemu-img: qcow2 doesn't support shrinking images yet
qemu-img: This image does not support resize
See also https://bugzilla.redhat.com/show_bug.cgi?id=1021802
The qemuDomainHelperGetVcpus attempted to report an error when the
vcpupids info was NULL. Unfortunately earlier code would clamp the
value of 'maxinfo' to 0 when nvcpupids was 0, so the error reporting
would end up being skipped.
This lead to 'virsh vcpuinfo <dom>' just returning an empty list
instead of giving the user a clear error.
If a previous commit I fixed the incorrect handling of vcpu pids
for TCG mode QEMU:
commit b07f3d821d
Author: Daniel P. Berrange <berrange@redhat.com>
Date: Thu Dec 18 16:34:39 2014 +0000
Don't setup fake CPU pids for old QEMU
The code assumes that def->vcpus == nvcpupids, so when we setup
fake CPU pids for old QEMU with nvcpupids == 1, we cause the
later code to read off the end of the array. This has fun results
like sche_setaffinity(0, ...) which changes libvirtd's own CPU
affinity, or even better sched_setaffinity($RANDOM, ...) which
changes the affinity of a random OS process.
The intent was that this would merely disable the ability to set
per-vCPU affinity. It should still have been possible to set VM
level host CPU affinity.
Unfortunately, when you set <vcpu cpuset='0-1'>4</vcpu>, the XML
parser will internally take this & initialize an entry in the
def->cputune.vcpupin array for every VCPU. IOW this is implicitly
being treated as
<cputune>
<vcpupin cpuset='0-1' vcpu='0'/>
<vcpupin cpuset='0-1' vcpu='1'/>
<vcpupin cpuset='0-1' vcpu='2'/>
<vcpupin cpuset='0-1' vcpu='3'/>
</cputune>
Even more fun, the faked cputune elements are hidden from view when
querying the live XML, because their cpuset mask is the same as the
VM default cpumask.
The upshot was that it was impossible to set VM level CPU affinity.
To fix this we must update qemuProcessSetVcpuAffinities so that it
only reports a fatal error if the per-VCPU cpu mask is different
from the VM level cpu mask.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
When initializing a libxl_domain_build_info struct with
libxl_domain_build_info_init(), VNC is enabled by default. As a
result, VMs configured with no graphics still have VNC enabled.
This behavior is a regression wrt to the legacy Xen driver.
Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
Do not silently ignore its value. LibXL support only one address, so
refuse multiple IPs.
Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
In order for QEMU vCPU (and other) threads to run with RT scheduler,
libvirt needs to take care of that so QEMU doesn't have to run privileged.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1178986
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
This function uses sched_setscheduler() function so it works with
processes and threads as well (even threads not created by us, which is
what we'll need in the future).
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Prior to commit 7d5bf48474 (first appearing in libvirt 1.2.2), the
status XML of a domain's interface was missing a lot of important
information; mainly it just output the config of the interface, plus
the name of the tap device and qemu device alias. Commit 7d5bf48474
changed the status XML to include many important bits of information
that were required to make network "hook" scripts useful - bandwidth
information, vlan tag, the name of the bridge (or physical device in
the case of macvtap) that the tap/macvtap device was attached to - the
commit log for 7d5bf48474 has a very detailed explanation of the
change. For quick reference - in the example given there, prior to the
change, status XML looked like figure [C]:
<interface type='network'>
<source network='testnet' portgroup='admin'/>
<target dev='macvtap0'/>
<alias name='net0'/>
<address type='pci' domain='0x0000' bus='0x00'
slot='0x03' function='0x0'/>
</interface>
and after the change, it looked like figure [E]:
<interface type='direct'>
<source dev='p4p1_0' mode='bridge'/>
<bandwidth>
<inbound average='1000' peak='5000' burst='1024'/>
<outbound average='128' peak='256' burst='256'/>
</bandwidth>
<target dev='macvtap0'/>
<alias name='net0'/>
<address type='pci' domain='0x0000' bus='0x00'
slot='0x03' function='0x0'/>
</interface>
You'll notice that bandwidth info, physdev, and macvtap mode have been
added, but the network and portgroup names are now missing - I didn't
think that this information was of any use once the needed
bandwidth/vlan/etc config had been pulled from the network/portgroup.
I was wrong.
A few months after that change a user on IRC asked what happened to
portgroup in the status XML and described how he used it (more or less
as a tag to decide what external information to use in a hook script
that was run at startup/migration time - see
http://wiki.libvirt.org/page/OVS_and_PVLANS ). At that time I planned
to make a patch to re-add portgroup, but life intervened as that was
just prior to a transatlantic move involving several weeks of
"vacation". During this time I somehow forgot to make the patch, and
also mistakenly remembered that I *had* made it.
Subsequent to this, as a part of mprivozn's work to add support for
network-specific hooks, I did re-add the output of the network name in
status XML, but once again completely forgot about portgroup. This was
in commit a3609121 (first appearing in libvirt 1.2.11). This made the
status XML from the above example look like this:
<interface type='direct'>
<source network='testnet' dev='p4p1_0' mode='bridge'/>
<bandwidth>
<inbound average='1000' peak='5000' burst='1024'/>
<outbound average='128' peak='256' burst='256'/>
</bandwidth>
<target dev='macvtap0'/>
<alias name='net0'/>
<address type='pci' domain='0x0000' bus='0x00'
slot='0x03' function='0x0'/>
</interface>
*This* patch just adds the portgroup back to the status XML, so the
same example interface will look like this:
<interface type='direct'>
<source network='testnet' portgroup='admin'
dev='p4p1_0' mode='bridge'/>
<bandwidth>
<inbound average='1000' peak='5000' burst='1024'/>
<outbound average='128' peak='256' burst='256'/>
</bandwidth>
<target dev='macvtap0'/>
<alias name='net0'/>
<address type='pci' domain='0x0000' bus='0x00'
slot='0x03' function='0x0'/>
</interface>
The result is that the status XML now contains all information about
how the interface is setup (bandwidth, physical device, tap device,
etc), in addition to pointers to its origin (the network and
portgroup).
virDomainGraphicsListenSetAddress() and
virDomainGraphicsListenSetNetwork() both set their respective char* to
NULL directly when asked to set it to NULL, which is okay as long as
it's already set to NULL. If these functions are ever called to clear
a listen object that has a valid string in address or network, it will
end up leaking the old value. Currently that doesn't happen, so this
is just a preemptive strike.
Prior to 0.9.4, libvirt only supported a single listen, and it had to
be an IP address:
<graphics listen='1.2.3.4' ..../>
Starting with 0.9.4, a graphics element could have a <listen>
subelement (actually the grammar supports multiples, but all of the
drivers only support a single <listen> per <graphics>), and that
listen element can be of type='address' or type='network'. For
type='address', <listen> also has an attribute called 'address' which
contains the IP address for listening:
<graphics ....>
<listen type='address' address='1.2.3.4' .../>
</graphics>
type can also be "network", and in that case listen will have a
"network" attribute which will contain the name of a libvirt
network:
<graphics ....>
<listen type='network' network='testnet' .../>
</graphics>
At domain start (or migrate) time, libvirt will attempt to
find an IP address associated with that network (e.g. the IP address
of the bridge device used by the network, or the physical device
listed in <forward dev='physdev'/>) and fill in that address in the
status XML:
<graphics ....>
<listen type='network' network='testnet' address='1.2.3.4' .../>
</graphics>
In the case that a <graphics> element has a <listen> subelement of
type='address', that listen subelement's "address" attribute is
backfilled into the parent graphics element's "listen" *attribute* for
backward compatibility (so that a management application unaware of
the separate <listen> element can still learn the listen
address). This backfill should be done with the IP learned from
type='network' as well, and that's what this patch does:
<graphics listen='1.2.3.4' ....>
<listen type='network' network='testnet' address='1.2.3.4' .../>
</graphics>
This is a continuation of the fix for:
https://bugzilla.redhat.com/show_bug.cgi?id=1191016
In the event we're falling into the code that tries to create the file
in a forked environment (VIR_FILE_OPEN_FORK) we pass different mode bits,
but those are never set because the virFileOpenForceOwnerMode has a check
if the OPEN_FORCE_MODE bit is set before attempting to change the mode.
Since this is a special case it seems reasonable to set u+rw,g+rw,o
Rather than have a dummy waitpid loop and return of the failure status
from recvfd, adjust the logic to save the recvfd error & fd and then
in priority order:
- if waitpid failed, use that errno value
- waitpid succeeded, but if the child exited abnormally, report failure
(use EACCES to report as return failure, since either EACCES or EPERM is
what caused us to fall into the fork+setuid path)
- waitpid succeeded, but if the child reported non-zero status, report
failure (use the errno value that the child encoded into exit status)
- waitpid succeeded, but if recvfd failed, report recvfd_errno
- waitpid and recvfd succeeded, use the fd
NOTE: Original logic to retry the open and force owner mode was
"documented" as only being attempted if we had already tried opening
with the fork+setuid, but checked flags vs. VIR_FILE_OPEN_NOFORK which
is counter to how we would get to that point. So that code was removed.
https://bugzilla.redhat.com/show_bug.cgi?id=1191355
When we attempt to migrate a vm with a migrateuri that has no scheme:
# virsh migrate test4 --live qemu+ssh://lhuang/system --migrateuri 127.0.0.1
target libvirtd will crash because uri->scheme is NULL in
qemuMigrationPrepareDirect on this line:
if (STRNEQ(uri->scheme, "tcp") &&
Add a value check before this line. Also fix a bug like this in
doNativeMigrate, that could only happen when destination libvirtd
returned an incorrect URI.
Signed-off-by: Luyao Huang <lhuang@redhat.com>
Signed-off-by: Ján Tomko <jtomko@redhat.com>
The function virDomainVcpuPinDel() used vcpupin_list to stand for
def->cputune.vcpupin, which made the codes more readable.
However, in this function, it will realloc vcpupin_list later.
As the definition of realloc(), it may free vcpupin_list and then
points it to a new-realloced address, but def->cputune.vcpupin doesn't
point to the new address(it's freed however).
Thus,
1) When we refer to the def->cputune.vcpupin afterwards, which was freed
by realloc(), an INVALID READ occurs, and libvirtd may crash.
2) As no one will use vcpupin_list any more, and no one frees it(it's just
alloced by realloc()), memory leak occurs.
Part of the valgrind logs are shown as below:
==1837== Thread 15:
==1837== Invalid read of size 8
==1837== at 0x5367337: virDomainDefFormatInternal (domain_conf.c:18392)
which is : virBufferAsprintf(buf, "<vcpupin vcpu='%u' ",
def->cputune.vcpupin[i]->vcpuid);
==1837== by 0x536966C: virDomainObjFormat (domain_conf.c:18970)
==1837== by 0x5369743: virDomainSaveStatus (domain_conf.c:19166)
==1837== by 0x117B26DC: qemuDomainPinVcpuFlags (qemu_driver.c:4586)
==1837== by 0x53EA313: virDomainPinVcpuFlags (libvirt.c:9803)
==1837== by 0x14CB7D: remoteDispatchDomainPinVcpuFlags (remote_dispatch.h:6762)
==1837== by 0x14CC81: remoteDispatchDomainPinVcpuFlagsHelper (remote_dispatch.h:6740)
==1837== by 0x5464C30: virNetServerProgramDispatchCall (virnetserverprogram.c:437)
==1837== by 0x546507A: virNetServerProgramDispatch (virnetserverprogram.c:307)
==1837== by 0x171B83: virNetServerProcessMsg (virnetserver.c:172)
==1837== by 0x171E6E: virNetServerHandleJob (virnetserver.c:193)
==1837== by 0x5318E78: virThreadPoolWorker (virthreadpool.c:145)
==1837== Address 0x12ea2870 is 0 bytes inside a block of size 16 free'd
==1837== at 0x4C291AC: realloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==1837== by 0x52A3D14: virReallocN (viralloc.c:245)
==1837== by 0x52A3DFB: virShrinkN (viralloc.c:372)
==1837== by 0x52A3F57: virDeleteElementsN (viralloc.c:503)
==1837== by 0x533939E: virDomainVcpuPinDel (domain_conf.c:15405) //doReset为true时才会进到。
==1837== by 0x117B2642: qemuDomainPinVcpuFlags (qemu_driver.c:4573)
==1837== by 0x53EA313: virDomainPinVcpuFlags (libvirt.c:9803)
==1837== by 0x14CB7D: remoteDispatchDomainPinVcpuFlags (remote_dispatch.h:6762)
==1837== by 0x14CC81: remoteDispatchDomainPinVcpuFlagsHelper (remote_dispatch.h:6740)
==1837== by 0x5464C30: virNetServerProgramDispatchCall (virnetserverprogram.c:437)
==1837== by 0x546507A: virNetServerProgramDispatch (virnetserverprogram.c:307)
==1837== by 0x171B83: virNetServerProcessMsg (virnetserver.c:172)
Steps to reproduce the problem:
1) use virDomainPinVcpuFlags() to pin a guest's vcpu to all the pcpus
of the host.
This patch uses def->cputune.vcpupin instead of vcpupin_list to do the
realloc() job, to avoid invalid read or memory leaking.
Signed-off-by: Zhang Bo <oscar.zhangbo@huawei.com>
Signed-off-by: Yue Wenyuan <yuewenyuan@huawei.com@huawei.com>
Export the required helpers and add backend code to hotplug RNG devices.
Signed-off-by: Luyao Huang <lhuang@redhat.com>
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
The helpers will be useful when implementing hotplug and coldplug of
random number generator devices.
Signed-off-by: Luyao Huang <lhuang@redhat.com>
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
As the RNG device is using an -object as backend refactor the code to
use the JSON to commandline generator so that we can reuse the code
later in hotplug.
Move the alias name right after the object type for rng-egd backend so
that we can later use the JSON to commandline generator to create the
command line.
Libvirt didn't prefix the random number generator backend object alias
with any string thus the device alias and object alias were identical.
To avoid possible problems, rename the alias for the backend object and
tweak tests to comply with the change.
Signed-off-by: Luyao Huang <lhuang@redhat.com>
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Rename qemuBuildRNGDeviceArgs to qemuBuildRNGDevStr and change the
return type so that it can be reused in the device hotplug code later.
Signed-off-by: Luyao Huang <lhuang@redhat.com>
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
This function is used to assign an alias for a RNG device. It will be
later reused when hotplugging RNGs.
Signed-off-by: Luyao Huang <lhuang@redhat.com>
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
When adding devices to the definition it's useful to check whether the
devices don't reside on a conflicting address. This patch adds a helper
that iterates all device info and compares the addresses with the given
info.
commit a58e1cb4 didn't fix the bug if the security_default_confined is
not set to 1. We now clean up even if there is no seclabel defined or
the default one.
When defining and creating networks, we have been checking to make
sure there is only a single "default" portgroup, but haven't verified
that no two portgroups have the same name. We *do* check for multiple
definitions when updating the portgroups in an existing network
though.
This patch adds a check to networkValidate(), which is called when a
network is defined or created, to disallow duplicate names. It would
actually make sense to do this in the network XML parser (since it's
not really "something that might make sense but isn't supported by
this driver", but is instead "something that should never be
allowed"), but doing that carries the danger of causing errors when
rereading the config of existing networks when libvirtd is restarted
after an upgrade, and that would result in networks disappearing from
libvirt's list. (I'm thinking I should change the error to "XML_ERROR"
instead of "UNSUPPORTED", even though that's not the type of error
that networkValidate is intended for)
This resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1115858
It is only usable for NETWORK and BRIDGE type interfaces.
Error out when trying to start a domain where the custom
tap device path is specified for interfaces of other types,
or when the daemon is not privileged.
Note that this cannot be checked at definition time, because
the comparison is against actual type.
https://bugzilla.redhat.com/show_bug.cgi?id=1147195
It is only supported for virtio adapters.
Silently drop it if it was specified for other models,
as is done for other virtio attributes.
Also mention this in the documentation.
https://bugzilla.redhat.com/show_bug.cgi?id=1147195
Return 0 instead of ERR_NO_SUPPORT in each driver
where we don't support managed save or -1 if
the domain does not exist.
This avoids spamming daemon logs when 'virsh dominfo' is run.
https://bugzilla.redhat.com/show_bug.cgi?id=1095637
It is often helpful to know which version of libvirt and QEMU
was present when a guest was first launched. Ensure this info
is written into the QEMU log file for each guest.
Commit id '652a2ec6' introduced two new node device capability flags
and the ability to use those flags as a way to search for a specific
subset of a 'scsi_host' device - namely a 'fc_host' and/or 'vports'.
The code modified the virNodeDeviceCapMatch whichs allows for searching
using the 'virsh nodedev-list [cap]' via virConnectListAllNodeDevices.
However, the original patches did not account for other searches for
the same capability key from virNodeDeviceNumOfCaps and virNodeDeviceListCaps
using nodeDeviceNumOfCaps and nodeDeviceListCaps. Since 'fc_host' and
'vports' are self defined bits of a 'scsi_host' device mere string
comparison against the basic/root type is not sufficient.
This patch adds the check for the 'fc_host' and 'vports' bits within
a 'scsi_host' device and allows the following python code to find the
capabilities for the device:
import libvirt
conn = libvirt.openReadOnly('qemu:///system')
devs = conn.listAllDevices()
for dev in devs:
if 'fc_host' in dev.listCaps() or 'vports' in dev.listCaps():
print dev.name(),dev.numOfCaps(),dev.listCaps()
Commit id '652a2ec6' introduced two new node device capability flags
and the ability to use those flags as a way to search for a specific
subset of a 'scsi_host' device - namely a 'fc_host' and/or 'vports'.
The code modified the virNodeDeviceCapMatch whichs allows for searching
using the 'virsh nodedev-list [cap]' via virConnectListAllNodeDevices.
However, the original patches did not account for other searches for
the same capability key from virNodeListDevices using virNodeDeviceHasCap.
Since 'fc_host' and 'vports' are self defined bits of a 'scsi_host'
device mere string comparison against the basic/root type is not
sufficient.
This patch adds the check for the 'fc_host' and 'vports' bits within
a 'scsi_host' device and allows the following python code to find the
capabilities for the device:
import libvirt
conn = libvirt.openReadOnly('qemu:///system')
fc = conn.listDevices('fc_host', 0)
print(fc)
fc = conn.listDevices('vports', 0)
print(fc)
Signed-off-by: Shivaprasad G Bhat <sbhat@linux.vnet.ibm.com>
Add the missing jump to the error label when the uuid in the
migration cookie XML does not match the uuid of the migrated
domain.
Signed-off-by: Luyao Huang <lhuang@redhat.com>
Signed-off-by: Ján Tomko <jtomko@redhat.com>
If a storage file would be backed with a NBD device without path
(nbd://localhost) libvirt would crash when parsing the backing path for
the disk as the URI structure's path element is NULL in such case but
the NBD parser would access it shamelessly.
Add the missing jump to thje error label. The error message shouldn't
ever be triggered though as it's called only on pre-selected nodes.
Signed-off-by: Luyao Huang <lhuang@redhat.com>
Commit b38da584 introduced two new functions to get a page size but it
won't work on Windows. We should take care of this.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Commit e562a61a introduced new function to get/set interface state but
there was misuse of ATTRIBUTE_NONNULL on non-pointer attributes and also
we need to wrap that functions by #ifdef to not break mingw build.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Some code paths have special logic depending on the page size
reported by sysconf, which in turn affects the test results.
We must mock this so tests always have a consistent page size.
e562a61a07 added these two new helper functions and only used them
within virnetdev.c, but declared them in the .h file. If some
currently unsupported interface flags need to be accessed in the
future, it will make more sense to write the appropriate higher level
function rather than require us to artificially define IFF_* on some
mythical platform that doesn't have SIOC[SG]IFFLAGS (and therefore
doesn't have IFF_*) just so we can call virNetDevSetIFFFlags() to
return an error.
To help someone in not going down the wrong road, this patch makes the
two helper functions static, hopefully making it less likely that
someone will want to use them outside of virnetdev.c.
https://bugzilla.redhat.com/show_bug.cgi?id=1176510
When storageDriverAutostart is called path virStateReload via a 'service
libvirtd reload', then because the volume list in the pool wasn't cleared
prior to the call, each volume would be listed multiple times (as many
times as we reload). I believe the issue would be introduced by commit
id '9e093f0b' at least for the libvirtd reload path, although I suppose
the introduction of virStateReload (commit id '70da0494') could be a
different cause.
Thus like other places prior to calling refreshPool, we need to call
virStoragePoolObjClearVols
Change done by commit f309db1f4d wrongly
assumes that qemu can start with a combination of NUMA nodes specified
with the "memdev" option and the appropriate backends, and the legacy
way by specifying only "mem" as a size argument. QEMU rejects such
commandline though:
$ /usr/bin/qemu-system-x86_64 -S -M pc -m 1024 -smp 2 \
-numa node,nodeid=0,cpus=0,mem=256 \
-object memory-backend-ram,id=ram-node1,size=12345 \
-numa node,nodeid=1,cpus=1,memdev=ram-node1
qemu-system-x86_64: -numa node,nodeid=1,cpus=1,memdev=ram-node1: qemu: memdev option must be specified for either all or no nodes
To fix this issue we need to check if any of the nodes requires the new
definition with the backend and if so, then all other nodes have to use
it too.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1182467
With the new JSON to argv formatter we are now able to represent the
memory backend definitions in the JSON object format that is reusable
for monitor use (hotplug) and then convert it into the shell string.
This will avoid having two separate instances of the same code that
would create the different formats.
Previous refactors now allow to make this step without changes to the
test suite.
QEMU's command line visitor as well as the JSON interface take bytes by
default for memory object sizes. Convert mebibytes to bytes so that we
can later refactor the existing code for hotplug purposes.
QEMU's qapi visitor code allows yes/on/y for true and no/off/n for false
value of boolean properities. Unify the used style so that we can
generate it later and fix test cases.
Extract the memory backend device code into a separate function so that
it can be later easily refactored and reused.
Few small changes for future reusability, namely:
- new (currently unused) parameter for user specified page size
- size of the memory is specified in kibibytes, divided up in the
function
- new (currently unused) parameter for user specifed source nodeset
- option to enforce capability check
Unlike -device, qemu uses a JSON object to add backend "objects" via the
monitor rather than the string that would be passed on the commandline.
To be able to reuse code parts that configure backends for various
devices, this patch adds a helper that will allow generating the command
line representations from the JSON property object.
This helper eases iterating all key=value pairs stored in a JSON
object. Usually we pick only certain known keys from a JSON object, but
this will allow to walk complete objects and have the callback act on
those.
To be able to easily represent nodesets and other data stored in
virBitmaps in libvirt, this patch introduces a set of helpers that allow
to convert the bitmap to and from JSON value objects.
Extract the logic to determine which nodeset has to be used for a domain
from the formatting step so that it can be reused separately when the
nodeset is used in a different way.
The function is called from all {Attach,Update,Detach}Device APIs to
create config strings that are later passed to the xend to perform the
desired action. The function is intended to handle all supported
devices. However, as of 5b05358aba we
are trying to get disk driver of the device without checking if the
device really is a disk. This leads to an segmentation fault:
#0 0x00007ffff7571815 in virDomainDiskGetDriver () from /usr/lib/libvirt.so.0
#1 0x00007fffeb9ad471 in ?? () from /usr/lib/libvirt/connection-driver/libvirt_driver_xen.so
#2 0x00007fffeb9b1062 in xenDaemonAttachDeviceFlags () from /usr/lib/libvirt/connection-driver/libvirt_driver_xen.so
#3 0x00007fffeb9a8a86 in ?? () from /usr/lib/libvirt/connection-driver/libvirt_driver_xen.so
#4 0x00007ffff7609266 in virDomainAttachDevice () from /usr/lib/libvirt.so.0
#5 0x0000555555593c9d in ?? ()
#6 0x00007ffff76743c9 in virNetServerProgramDispatch () from /usr/lib/libvirt.so.0
#7 0x00005555555a678d in ?? ()
#8 0x00007ffff755460e in ?? () from /usr/lib/libvirt.so.0
#9 0x00007ffff7553b06 in ?? () from /usr/lib/libvirt.so.0
#10 0x00007ffff4998b50 in start_thread () from /lib/x86_64-linux-gnu/libpthread.so.0
#11 0x00007ffff46e30ed in clone () from /lib/x86_64-linux-gnu/libc.so.6
#12 0x0000000000000000 in ?? ()
Reported-by: Xiaolin Su <linxxnil@126.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
https://bugzilla.redhat.com/show_bug.cgi?id=1170492
In one of our previous commits (dc8b7ce7) we've done a functional
change even though it was intended as pure refactor. The problem is,
that the following XML:
<vcpu placement='static' current='2'>6</vcpu>
<cputune>
<emulatorpin cpuset='1-3'/>
</cputune>
<numatune>
<memory mode='strict' placement='auto'/>
</numatune>
gets translated into this one:
<vcpu placement='auto' current='2'>6</vcpu>
<cputune>
<emulatorpin cpuset='1-3'/>
</cputune>
<numatune>
<memory mode='strict' placement='auto'/>
</numatune>
We should not change the vcpu placement mode. Moreover, we're doing
something similar in case of emulatorpin and iothreadpin. If they were
set, but vcpu placement was auto, we've mistakenly removed them from
the domain XML even though we are able to set them independently on
vcpus.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
This patch enables synchronization of the host macvtap
device options with the guest device's in response to the
NIC_RX_FILTER_CHANGED event.
The following device options will be synchronized:
* PROMISC
* MULTICAST
* ALLMULTI
Signed-off-by: Tony Krowiak <akrowiak@linux.vnet.ibm.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
This patch provides the utility functions needed to synchronize
the rxfilter changes made to a guest domain with the corresponding
macvtap devices on the host:
* Get/set PROMISC flag
* Get/set ALLMULTI, MULTICAST
Signed-off-by: Tony Krowiak <akrowiak@linux.vnet.ibm.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
https://bugzilla.redhat.com/show_bug.cgi?id=1158034
If we're expecting to create a file somewhere and that fails for some
reason during qemuOpenFileAs, then we unlink the path we're attempting
to create leaving no way to determine what the "existing" privileges,
protections, or labels are that caused the failure (open, change owner
and group, change mode, etc.).
Furthermore, if we fall into the path where we'll be opening / creating
the file using VIR_FILE_OPEN_FORK, we need to first unlink/delete the file
we created in the first path; otherwise, the attempt by the child process
to open as some specific user:group may fail because the file was already
created using nfsnobody:nfsnobody. Again, if we didn't create the file we
don't want to blindly delete what already exists. Thus, a second reason for
the original check to set need_unlink to false when we find the file with
CREAT set, but already existing.
Signed-off-by: John Ferlan <jferlan@redhat.com>
A gnulib change (commit id 'beae0bdc') causes ENOTCONN to be returned
from recvfd which causes us to fall into the throwaway waitpid() call
and return ENOTCONN to the caller, this then gets displayed during
a 'virsh save' when using a root squashed NFS environment that's trying
to save the file as something other than root:root.
This patch will add the additional check for ENOTCONN to force the code
into the waitpid loop looking for the actual status from the _exit()'d
child fork.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Commit id '540c339a' to fix issues with reference counting and transient
domains moved the qemuDomainObjEndAsyncJob call prior to the attempt to
restart the guest CPU's resulting in an error:
error: Failed to save domain rhel70 to /tmp/pl/rhel70.save
error: internal error: unexpected async job 3
when (ret != 0) - eg, the error path from qemuDomainSaveMemory.
This patch will adjust the logic to call the EndAsyncJob only after
we've tried to restart the guest CPUs. It also needs to adjust the
test for qemuDomainRemoveInactive to add the ret == 0 condition.
Additionally, if we get to endjob: because of some error earlier, then
we need to save that error in the event the CPU restart logic fails.
We don't want to return the error from CPU restart failure, rather we
want to return the error from the failed save that caused us to fall
into the retry to start the CPU logic.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Well, even though users can pass the list of UEFI:NVRAM pairs at the
configure time, we may maintain the list of widely available UEFI
ourselves too. And as arm64 begin to rises, OVMF was ported there too.
With a slight name change - it's called AAVMF, with AAVMF_CODE.fd
being the UEFI firmware and AAVMF_VARS.fd being the NVRAM store file.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Up until now there are just two ways how to specify UEFI paths to
libvirt. The first one is editing qemu.conf, the other is editing
qemu_conf.c and recompile which is not that fancy. So, new
configure option is introduced: --with-loader-nvram which takes a
list of pairs of UEFI firmware and NVRAM store. This way, the
compiled in defaults can be passed during compile time without
need to change the code itself.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
When compiling without WITH_MACVTAP, we can get:
'unsupported flags (0x1) in function
virNetDevMacVLanCreateWithVPortProfile'
on an attempt to start a domain.
Remove the flag check to reach the more helpful error:
Cannot create macvlan devices on this platform
https://bugzilla.redhat.com/show_bug.cgi?id=1186928
https://bugzilla.redhat.com/show_bug.cgi?id=1138516
If the provided volume name doesn't match what parted generated as the
partition name, then return a failure.
Update virsh.pod and formatstorage.html.in to describe the 'name' restriction
for disk pools as well as the usage of the <target>'s <format type='value'>.
When removing a volume that is the extended partition, all the logical
volume partitions that exist within the extended partition will also be
removed, so we need to refresh the pool to have the updated list
During virStorageBackendDiskMakeDataVol processing, if we find an extended
partition, then handle it specially when updating the capacity/allocation
rather than calling virStorageBackendUpdateVolInfo.
As it turns out, once a logical partition exists, any attempt to refresh
the pool or after libvirtd restart/reload will result in a failure to open
the extended partition device resulting in the inability to start the pool.
The downside to this is we will lose the <permissions> and <timestamps> for
the extended partition upon subsequent restart, refresh, reload since the
stat() in virStorageBackendUpdateVolTargetInfoFD will not be called. However,
since it's really only a container and shouldn't directly be used for
storage that seems reasonable.
Therefore, only use the existing code that already had a comment about
getting the allocation wrong for extended partitions for just the setting
of the extended partition data.
While checking the existing partitions in virStorageBackendDiskPartFormat,
the code would erroneously compare the volume target format type (eg, the
virStoragePartedFsType) rather than the source partition type (eg, the
virStorageVolTypeDisk) which is set during virStorageBackendDiskReadPartitions.
During virStorageBackendDiskCreateVol if virStorageBackendDiskReadPartitions
fails, then we were leaving with an error and a partition on the disk for
which there was no corresponding volume and used space on the disk which
could be reclaimable through direct parted activity. On a subsequent restart,
reload, or refresh the volume may magically appear too.
Move the API to before virStorageBackendDiskCreateVol in order to be
able to call the DeleteVol API when virStorageBackendDiskReadPartitions
fails so that we don't by chance leave a partition on the disk.
https://bugzilla.redhat.com/show_bug.cgi?id=1183890
When we try to update a xml to a image file, we will clear the
graphics passwd settings, because we do not pass VIR_DOMAIN_XML_SECURE
to qemuDomainDefCopy, qemuDomainDefFormatBuf won't format the passwd.
Add VIR_DOMAIN_XML_SECURE flag when we call qemuDomainDefCopy
in qemuDomainSaveImageUpdateDef.
Signed-off-by: Luyao Huang <lhuang@redhat.com>
https://bugzilla.redhat.com/show_bug.cgi?id=1161024
This way the device is in vmdef only if ret = 0 and the caller
(qemuDomainAttachDeviceFlags) does not free it.
Otherwise it might get double freed by qemuProcessStop
and qemuDomainAttachDeviceFlags if the domain crashed
in monitor after we've added it to vm->def.
Do the allocation first, then add the actual device.
The second part should never fail. This is good
for live hotplug where we don't want to remove the device
on OOM after the monitor command succeeded.
The only change in behavior is that on failure, the
vmdef->consoles array is freed, not just the first console.
