A future patch will add two-phase block commit jobs; as the
mechanism for managing them is similar to managing a block copy
job, existing errors should be made generic enough to occur
for either job type.
* src/conf/domain_conf.c (virDomainHasDiskMirror): Update
comment.
* src/qemu/qemu_driver.c (qemuDomainDefineXML)
(qemuDomainSnapshotCreateXML, qemuDomainRevertToSnapshot)
(qemuDomainBlockJobImpl, qemuDomainBlockCopy): Update error
message.
* src/qemu/qemu_hotplug.c (qemuDomainDetachDiskDevice): Likewise.
Signed-off-by: Eric Blake <eblake@redhat.com>
Peter's review of an early version of my addition of active block
commit pointed out some issues that I was copying from the block
copy code; fix them up now before perpetuating them.
For virsh commands that manage a single API call, it's nice to have
a 1:1 mapping of options to flags, so that we can test that
lower-layer software handles flag combinations correctly. But where
virsh is introducing syntactic sugar to combine multiple API calls
into a single user interface, we might as well make that interface
compact. That is, we should allow the shorter command-line of
'blockcopy $dom $disk --pivot' without having to explicitly specify
--wait, because this isn't directly a flag passed to a single
underlying API call.
Also, my use of embedded ?: ternaries bordered on unreadable.
* tools/virsh-domain.c (cmdBlockCopy): Make --pivot, --finish,
and --timeout imply --wait. Drop excess ?: operators.
* tools/virsh.pod (blockcopy): Update documentation.
Signed-off-by: Eric Blake <eblake@redhat.com>
With one of my recent patches (1c70277) libvirt's capable of
reporting NUMA node locality for PCI devices. The node ID is
stored in pci_dev.numa_node variable. However, since zero is
valid NUMA node ID, the default is -1 as it is in kernel too.
So, if the PCI device is not tied to any specific NUMA node, the
default is then NOT printed into XML. Therefore, when parsing
node device XML, the <node/> element is optional. But currently,
if it's not there, we must set sane default, otherwise after
parsing in the memory representation doesn't match the XML. We
are already doing this in other place: udevProcessPCI().
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Commit f586965 accidentally changed the semantics of the
virDomainBlockCommit command; where it previously looked for
an explicit top argument from the top of the chain, it now
starts from the backing file of the top. Of course, until
we allow active commits, the only difference it makes is in
the quality of the error message, but with code for active
commit coming soon, we need to support an explicit mention
of the active layer.
* src/qemu/qemu_driver.c (qemuDomainBlockCommit): Start looking
from top of chain.
Signed-off-by: Eric Blake <eblake@redhat.com>
The vcpupin command allowed specifying a negative number for the --vcpu
argument. This would the overflow when the underlying virDomainPinVcpu
API was called.
$ virsh vcpupin r7 -1 0
error: numerical overflow: input too large: 4294967295
Switch the vCPU variable to a unsigned int and parse it using the
corresponding function.
Also improve the vcpupin test to cover all the defects.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1101059
Signed-off-by: Jincheng Miao <jmiao@redhat.com>
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
To follow the new semantics of the vshCommandOptToU* functions convert
this one to reject negative numbers too. To allow using -1 for "maximum"
semantics for the vol-*load two bandwidth functions that use this helper
introduce vshCommandOptULongLongWrap.
To follow the new semantics of the vshCommandOptToU* functions convert
this one to reject negative numbers too. To allow using -1 for "maximum"
semantics for the two bandwidth functions that use this helper introduce
vshCommandOptULWrap. Although currently the migrate-setspeed function
for the qemu driver will reject -1 as maximum.
Use virStrToLong_uip instead of virStrToLong_ui to reject negative
numbers in the helper. None of the callers expects the wraparound
"feature" for negative numbers.
Also add a function that allows wrapping of negative numbers as it might
be used in the future and be explicit about the new semantics in the
function docs.
A network disk might actually be backed by local storage. Also the path
iterator actually handles networked disks well now so remove the code
that skips the labelling in dac and selinux security driver.
Rework internal pool lookup code to avoid printing the raw UUID buffer
in the case a storage pool can't be found:
$ virsh pool-name e012ace0-0460-5810-39ef-1bce5fa5a4dd
error: failed to get pool 'e012ace0-0460-5810-39ef-1bce5fa5a4dd'
error: Storage pool not found: no storage pool with matching uuid à¬à`X9ï_¥¤Ý
The rework is mostly done by switching the lookup code to the newly
introduced helper virStoragePoolObjFromStoragePool
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1104993
This option only makes sense for -fstack-protector.
With -fstack-protector-all or -fstack-protector-strong,
functions are protected regardless of buffer size.
https://bugzilla.redhat.com/show_bug.cgi?id=1105456
Commit baafe668 introduced new leaseshelper with a crash of freeing
env string. Calling 'getenv()' inside 'virGetEnvAllowSUID()' may
return a static string and we definitely should not free it.
The author probably want to free the copy of that string.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Roman Bogorodskiy reported a syntax-check failure when using
FreeBSD; complaining that:
prohibit_empty_first_line
tools/libvirt_win_icon_16x16.ico:1:
tools/libvirt_win_icon_32x32.ico:1:
tools/libvirt_win_icon_48x48.ico:1:
tools/libvirt_win_icon_64x64.ico:1:
maint.mk: Prohibited empty first line
In reality, the first 'line' of that file is NOT empty; but since
it is a binary file, awk is not required to handle it gracefully.
The simplest solution is to exempt all image files from syntax
checks in the first place - after all, we only store them in git
because they are inconvenient to regenerate, but they are not our
preferred format for making modifications, and syntax check should
only cover files that we are likely to modify.
* cfg.mk (VC_LIST_ALWAYS_EXCLUDE_REGEX): Exempt images.
(exclude_file_name_regexp--sc_prohibit_empty_lines_at_EOF): Simplify.
(exclude_file_name_regexp--sc_trailing_blank): Likewise.
Signed-off-by: Eric Blake <eblake@redhat.com>
When saving domain with relabel=no, the file that gets created must have the
context set anyway. That way restore can be successful without the need of
relabelling the file.
Signed-off-by: Shivaprasad G Bhat <sbhat@linux.vnet.ibm.com>
Since commit d69415d4, vmware version is parsed from both stdout and
stderr. This patch makes version parsing work even if there is garbage
(libvirt debug messages for example) in the command output.
Add test data for this case.
If we're compiling on non-Linux platform, the virNetDevGetLinkInfo()
is a dummy function which barely logs debug message that getting link
info is not supported. However, while the debug message was prepared
for printing the interface name too, I actually forgot to pass the
variable which resulted in build error on platforms like mingw or
FreeBSD.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
While exposing the info under <interface/> in previous patch works, it
may work only in cases where interface is configured on the host.
However, orchestrating application may want to know the link state and
speed even in that case. That's why we ought to expose this in nodedev
XML too:
virsh # nodedev-dumpxml net_eth0_f0_de_f1_2b_1b_f3
<device>
<name>net_eth0_f0_de_f1_2b_1b_f3</name>
<path>/sys/devices/pci0000:00/0000:00:19.0/net/eth0</path>
<parent>pci_0000_00_19_0</parent>
<capability type='net'>
<interface>eth0</interface>
<address>f0🇩🇪f1:2b:1b:f3</address>
<link speed='1000' state='up'/>
<capability type='80203'/>
</capability>
</device>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
In the previous commit the helper function was prepared, so now
we can wire it up and benefit from it. The Makefile change is
required because we're including virnedev,h which includes
virnetlink.h which tries to include netlink/msg.h. However this
file is not under /usr/include directly but is dependent on libnl
used.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
The purpose of this function is to fetch link state
and link speed for given NIC name from the SYSFS.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Currently it is not possible to determine the speed of an interface
and whether a link is actually detected from the API. Orchestrating
platforms want to be able to determine when the link has failed and
where multiple speeds may be available which one the interface is
actually connected at. This commit introduces an extension to our
interface XML (without implementation to interface driver backends):
<interface type='ethernet' name='eth0'>
<start mode='none'/>
<mac address='aa:bb:cc:dd:ee:ff'/>
<link speed='1000' state='up'/>
<mtu size='1492'/>
...
</interface>
Where @speed is negotiated link speed in Mbits per second, and state
is the current NIC state (can be one of the following: "unknown",
"notpresent", "down", "lowerlayerdown","testing", "dormant", "up").
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Commit 8ba0a58 introduced a compiler warning that I hit during
a run of ./autobuild.sh:
../../src/nodeinfo.c: In function 'nodeCapsInitNUMA':
../../src/nodeinfo.c:1853:43: error: 'nsiblings' may be used uninitialized in this function [-Werror=maybe-uninitialized]
if (virCapabilitiesAddHostNUMACell(caps, n, memory,
^
Sure enough, nsiblings starts uninitialized, and is set by a call
to virNodeCapsGetSiblingInfo, but that function fails to assign
through the pointer if virNumaGetDistances fails.
* src/nodeinfo.c (nodeCapsInitNUMA): Initialize nsiblings.
Signed-off-by: Eric Blake <eblake@redhat.com>
Jim Fehlig reported a regression found by libvirt-TCK tests:
> ~ # perl /usr/share/libvirt-tck/tests/qemu/100-disk-encryption.t
...
> ok 4 - defined persistent domain config
> # Starting inactive domain config
> libvirt error code: 1, message: internal error: unable to execute QEMU command
> 'cont': 'drive-ide0-0-1'
> (/var/cache/libvirt-tck/300-disk-encryption/demo.qcow2) is encrypted
Commit 2279d560 converted a boolean into a pointer with the intent of
transferring that pointer out of a temporary object into the caller's
data structure. The temporary structure meant that meta->encryption
was always NULL on entry, so we could get away with blindly allocating
the pointer when the header said so. But later, commit 8823272d
tweaked things to do backing chain detection in-place, rather than via
a temporary object; this has the net result that meta->encryption can
be non-NULL on entry. Not only did this turn the latent behavior into
a memory leak, it is also a behavior regression: blindly allocating a
new pointer wipes out what secrets we already knew about the chain,
making it impossible to restart the domain.
Of course, no one in their right mind should be relying on qcow2
encryption - it is fundamentally flawed. And sadly, the TCK tests
don't get run often enough, and this shows that our virstoragetest
does not exercise encrypted images at all. Otherwise, we could
have avoided a release containing this regression.
* src/util/virstoragefile.c (virStorageFileGetMetadataInternal):
Don't nuke an already-existing encryption.
Signed-off-by: Eric Blake <eblake@redhat.com>
clang complains about possibly uninitialized variable:
vbox/vbox_snapshot_conf.c:1355:9: error: variable 'ret' is used uninitialized whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized]
if (!(xPathContext = xmlXPathNewContext(xml))) {
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
So init 'ret' with NULL.
the 'migration_host' description may be a bit difficult to
understand for some users, so enhance the manual
Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
Now that qemu 2.0 allows commit of the active layer, people are
attempting to use virsh blockcommit and getting into a stuck
state, because libvirt is unprepared to handle the two-phase
commit required by qemu.
Stepping back a bit, there are two valid semantics for a
commit operation:
1. Maintain a 'golden' base, and a transient overlay. Make
changes in the overlay, and if everything appears to work,
commit those changes into the base, but still keep the overlay
for the next round of changes; repeat the cycle as desired.
2. Create an external snapshot, then back up the stable state
in the backing file. Once the backup is complete, commit the
overlay back into the base, and delete the temporary snapshot.
Since qemu doesn't know up front which of the two styles is
preferred, a block commit of the active layer merely gets
the job into a synchronized state, and sends an event; then
the user must either cancel (case 1) or complete (case 2),
where qemu then sends a second event that actually ends the
job. However, until commit e6bcbcd, libvirt was blindly
assuming the semantics that apply to a commit of an
intermediate image, where there is only one sane conclusion
(the job automatically ends with fewer elements in the chain);
and getting stuck because it wasn't prepared for qemu to enter
a second phase of the job.
This patch adds a flag to the libvirt API that a user MUST
supply in order to acknowledge that they will be using two-phase
semantics. It might be possible to have a mode where if the
flag is omitted, we automatically do the case 2 semantics on
the user's behalf; but before that happens, I must do additional
patches to track the fact that we are doing an active commit
in the domain XML. Later patches will add support of the flag,
and once 2-phase semantics are working, we can then decide
whether to relax things to allow an omitted flag to cause an
automatic pivot.
* include/libvirt/libvirt.h.in (VIR_DOMAIN_BLOCK_COMMIT_ACTIVE)
(VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT): New enums.
* src/libvirt.c (virDomainBlockCommit): Document two-phase job
when committing active layer, through new flag.
(virDomainBlockJobAbort): Document that pivot also occurs after
active commit.
* tools/virsh-domain.c (vshDomainBlockJob): Cover new job.
* src/qemu/qemu_driver.c (qemuDomainBlockCommit): Explicitly
reject active copy; later patches will add it in.
Signed-off-by: Eric Blake <eblake@redhat.com>
The machine is unregistered and its vbox XML file is changed in order to
add snapshot information. The machine is then registered with the
snapshot to redefine.
This structure contains the data to be saved in the VirtualBox XML file
and can be manipulated with severals exposed functions.
The structure is created by vboxSnapshotLoadVboxFile taking the
machine XML file.
It also can rewrite the XML by using vboxSnapshotSaveVboxFile.
The qemu driver always adds these options to the qemu commandlines,
but the commandline parser didn't recognize them, so sending a
libvirt-generated qemu commandline to its own argvtoxml would always
result in a warning message and a qemu namespace added to the
xml. Since the options don't add any functionality to the domain, they
should just be ignored (similar to -S).
Note that we can't yet add a test for this to qemuargv2xmltest,
because we would have to add QEMU_CAPS_NODEFCONFIG and
QEMU_CAPS_DEVICE to the capabilities for any corresponding
xml2argvtest, and QEMU_CAPS_DEVICE would necessitate having support
for parsing a memballoon device in order for qemuargv2xmltest to
pass. So we wait to add a test for -nodefconfig and -nodefaults until
after adding support for parsing -device virtio-balloon-*.
qmeuargv2xmltest.c would fail any test that logged anything during
qemuParseCommandline(), but then discard the log message, even with
VIR_TEST_DEBUG=2. This patch outputs the log messages with
fprintf(stderr,...) when debug logging is on.
In the process of modifying that logic, the testInfo data was made
more similar to that of qemuxml2argvtest.c - rather than turning
info->extraFlags into a bool, an enum of flags is defined, the info
struct is given an "unsigned int flags", and FLAG_EXPECT_WARNING is
saved into info->flags, to be checked during the test; this will make
it easier to add other FLAG_EXPECT_* items in the future.
4d06af97d3 introduced a possible memory
leak of the memory allocated into the "cpu" pointer in
parallelsBuildCapabilities in the case "nodeGetInfo()" would fail right
after the allocation. Rearrange the code to avoid the possibility of the
leak.
Found by Coverity.
bhyveload and bhyvectl wouldn't be checked otherwise as the configure
script wouldn't execute one of the tests:
checking for bhyve... /usr/local/sbin/bhyve
checking for bhyvectl... /usr/local/sbin/bhyvectl
checking for bhyveload... /usr/local/sbin/bhyveload
./configure: line 62602: test: too many arguments
Fix the shell statement testing the 3 binaries.
The original implementation of the VMX config parser assumed that the
virtualHW.version would have more influence on the content of the VMX
file than it actually seems to have. It started with accepting only
version 4. Additonal versions were added later without any additional
changes in the parser itself. This suggests that the influence of the
virtualHW.version on the content and format of the VMX file is small
or non-existent.
The parser worked without any changes across several virtualHW and
vSphere versions. So instead of adding new virtualHW.version values to
the parser as they come along, or adding an extra flag to allow unknown
virtualHW.version values just relax the check to require version 4 or
later.
Now that we track a disk mirror as a virStorageSource, we might
as well update the XML to theoretically allow any type of
mirroring destination (not just a local file). A later patch
will also be reusing <mirror> to track the block commit of the
top layer of a chain, which is another case where libvirt needs
to update the backing chain after the job is finally pivoted,
and since backing chains can have network backing files as the
destination to commit into, it makes more sense to display that
in the XML.
This patch changes output-only XML; it was already documented
that <mirror> does not affect a domain definition at this point
(because qemu doesn't provide persistent bitmaps yet). Any
application that was starting a block copy job with older libvirt
and then relying on the domain XML to determine if it was
complete will no longer be able to access the file= and format=
attributes of mirror that were previously used. However, this is
not going to be a problem in practice: the only time a block copy
job works is on a transient domain, and any app that is managing
a transient domain probably already does enough of its own
bookkeeping to know which file it is mirroring into without
having to re-read it from the libvirt XML. The one thing that
was likely to be used in a mirroring job was the ready=
attribute, which is unchanged. Meanwhile, I made sure the schema
and parser still accept the old format, even if we no longer
output it, so that upgrading from an older version of libvirt is
seamless.
* docs/schemas/domaincommon.rng (diskMirror): Alter definition.
* src/conf/domain_conf.c (virDomainDiskDefParseXML): Parse two
styles of mirror elements.
(virDomainDiskDefFormat): Output new style.
* tests/qemuxml2argvdata/qemuxml2argv-disk-mirror-old.xml: New
file, copied from...
* tests/qemuxml2argvdata/qemuxml2argv-disk-mirror.xml: ...here
before modernizing.
* tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-mirror-old*: New
files.
* tests/qemuxml2xmltest.c (mymain): Test both styles.
Signed-off-by: Eric Blake <eblake@redhat.com>
The current implementation of 'virsh blockcopy' (virDomainBlockRebase)
is limited to copying to a local file name. But future patches want
to extend it to also copy to network disks. This patch converts over
to a virStorageSourcePtr, although it should have no semantic change
visible to the user, in anticipation of those future patches being
able to use more fields for non-file destinations.
* src/conf/domain_conf.h (_virDomainDiskDef): Change type of
mirror information.
* src/conf/domain_conf.c (virDomainDiskDefParseXML): Localize
mirror parsing into new object.
(virDomainDiskDefFormat): Adjust clients.
* src/qemu/qemu_domain.c (qemuDomainDeviceDefPostParse):
Likewise.
* src/qemu/qemu_driver.c (qemuDomainBlockPivot)
(qemuDomainBlockJobImpl, qemuDomainBlockCopy): Likewise.
Signed-off-by: Eric Blake <eblake@redhat.com>
As part of the work on backing chains, I'm finding that it would
be easier to directly manipulate chains of pointers (adding a
snapshot merely adjusts pointers to form the correct list) rather
than copy data from one struct to another. This patch converts
domain disk source to be a pointer.
In this patch, the pointer is ALWAYS allocated (thanks in part to
the previous patch forwarding all disk def allocation through a
common point), and all other changse are just mechanical fallout of
the new type; there should be no functional change. It is possible
that we may want to leave the pointer NULL for a cdrom with no
medium in a later patch, but as that requires a closer audit of the
source to ensure we don't fault on a null dereference, I didn't do
it here.
* src/conf/domain_conf.h (_virDomainDiskDef): Change type of src.
* src/conf/domain_conf.c: Adjust all clients.
* src/security/security_selinux.c: Likewise.
* src/qemu/qemu_domain.c: Likewise.
* src/qemu/qemu_command.c: Likewise.
* src/qemu/qemu_conf.c: Likewise.
* src/qemu/qemu_process.c: Likewise.
* src/qemu/qemu_migration.c: Likewise.
* src/qemu/qemu_driver.c: Likewise.
* src/lxc/lxc_driver.c: Likewise.
* src/lxc/lxc_controller.c: Likewise.
* tests/securityselinuxlabeltest.c: Likewise.
Signed-off-by: Eric Blake <eblake@redhat.com>
A future patch wants to create disk definitions with non-zero
default contents; to avoid crashes, all callers that allocate
a disk definition should go through a common point.
I found allocation points by looking for any code that increments
ndisks, as well as any matches for ALLOC.*disk. Most places that
modified ndisks were covered by the parse from XML to domain/device
definition by initial domain creation or device hotplug; I also
hand-checked all drivers that generate a device struct on the
fly during getXMLDesc.
* src/conf/domain_conf.h (virDomainDiskDefNew): New prototype.
* src/conf/domain_conf.c (virDomainDiskDefNew): New function.
(virDomainDiskDefParseXML): Use it.
* src/parallels/parallels_driver.c (parallelsAddHddInfo):
Likewise.
* src/qemu/qemu_command.c (qemuParseCommandLine): Likewise.
* src/vbox/vbox_tmpl.c (vboxDomainGetXMLDesc): Likewise.
* src/vmx/vmx.c (virVMXParseDisk): Likewise.
* src/xenxs/xen_sxpr.c (xenParseSxprDisks, xenParseSxpr):
Likewise.
* src/xenxs/xen_xm.c (xenParseXM): Likewise.
* src/libvirt_private.syms (domain_conf.h): Export it.
Signed-off-by: Eric Blake <eblake@redhat.com>
As part of the work on backing chains, I'm finding that it would
be easier to directly manipulate chains of pointers (adding a
snapshot merely adjusts pointers to form the correct list) rather
than copy data from one struct to another. This patch converts
snapshot source to be a pointer.
In this patch, the pointer is ALWAYS allocated (any code that
increases ndisks now also allocates a source pointer for each
new disk), and all other changes are just mechanical fallout of
the new type; there should be no functional change. It is
possible that we may want to leave the pointer NULL for internal
snapshots in a later patch, but as that requires a closer audit
of the source to ensure we don't fault on a null dereference, I
didn't do it here.
* src/conf/snapshot_conf.h (_virDomainSnapshotDiskDef): Change
type of src.
* src/conf/snapshot_conf.c: Adjust all clients.
* src/qemu/qemu_conf.c: Likewise.
* src/qemu/qemu_driver.c: Likewise.
Signed-off-by: Eric Blake <eblake@redhat.com>
