For consistency with other error messages, and the fact that
the object is always called a virDomainSnapshot rather than
a mere virSnapshot, include the word "domain" in the error
message.
Suggested-by: John Ferlan <jferlan@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Commit 28f8dfdc (1.0.0) added a flag to virDomainGetXMLDesc, but
failed to document its effects. And considering that the
MIGRATABLE flag has been the source of past bugs (CVE-2014-7823,
fixed in commit b1674ad5 (1.2.11), or even cf2d4c60 (1.2.13) where
flag mismatch broke virsh edit), make the wording wishy-washy
enough to discourage using the flag casually, by mentioning that
the resulting XML is more for internal use than for validation
against the schema.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Due to historical back-compat, bare 'virsh snapshot-create-as'
favors internal snapshots (but can't be used on domains with raw
storage), while 'virsh snapshot-create-as --disk-only' favors
external snapshots. What's more, snapshots created with
--disk-only while the domain was running are marked as snapshot
state 'disk-snapshot', while snapshots created while the domain
was offline are marked as snapshot state 'shutdown' (a
'disk-snapshot' image might not be quiescent, while a 'shutdown'
snapshot always is).
But this leads to some interesting problems: if we create a
--disk-only snapshot of an offline guest, and then immediately try
to 'virsh snapshot-create --redefine' using the resulting XML to
overwrite the existing snapashot in place, things silently succeed,
but 'virsh snapshot-create --redefine --disk-only' fails with an
error message that the snapshot state is not 'disk-only'. Worse,
if we delete the snapshot metadata first and then try to recreate
things, omitting --disk-only fails because the verification code
wants to force the default of an internal snapshot (which doesn't
work with raw disks), and using --disk-only still fails because the
snapshot XML is not 'disk-only' - making it impossible to recreate
the snapshot metadata (or to transfer it from one libvirtd host to
another). Ideally, the presence or absence of the --disk-only
flag, and the presence or absence of an existing snapshot being
overwritten, shouldn't matter; if the XML is valid for one
situation, it should always be valid to redefine the metadata for
that snapshot.
Fix things by uniformly using virDomainSnapshotDefIsExternal()
(caching the results up front, and eliminating other 'if' clauses
now rendered redundant) when deciding whether the XML being
requested for redefinition should permit external or force internal
state capture (we got it right in only one out of three places in
the function).
See also https://bugzilla.redhat.com/1680304; this fixes the
domain-agnostic problems mentioned there, but another patch is
needed to fix further oddities with the qemu driver. I did not
check for sure when the problems were introduced (git blame puts
some affected hunks as far back as 1.0.0), but it was definitely
been broken even before when commit 670e86bf (1.1.4) factored
redefine prep out of qemu code into the common snapshot_conf code.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Upcoming patches plan to introduce virDomainCheckpointPtr as a new
object for use in incremental backups, along with documentation on
how incremental backups differ from snapshots. But first, we need
to rename any existing mention of a 'system checkpoint' to instead
be a 'full system snapshot', so that we aren't overloading
the term checkpoint.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
The previous patch made it possible to split multiple commands by
adding newline, but not to split a long single command. The sequence
backslash-newline was being used as if it were a quoted newline
character, rather than completely elided the way the shell does.
Again, add more tests, although this time it seems more like I am
suffering from a leaning-toothpick syndrome with all the \.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
I wanted to do a demonstration with virsh batch mode, which
takes multiple commands all packed into a single argument:
$ virsh -c test:///default 'echo a; echo b;'
a
b
but that produced a really long line, so I tried to make it
more legible:
$ virsh -c test:///default '
echo a;
echo b;
'
error: unknown command: '
'
Let's be more like the shell, and treat unquoted newline as a
command separator just as we do for semicolon. In fact, with
that, I can even now mix styles:
$ virsh -c test:///default '
echo a; echo b
echo c
'
a
b
c
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
vcpupin will fail when maxvcpus is larger than current
vcpu:
virsh vcpupin win7 --vcpu 0 --cpulist 5-6
error: Requested operation is not valid: cpu affinity is not supported
win7 xml in the command above is like below:
...
<vcpu current="3" placement="static">8</vcpu>
...
The reason is vcpu[3] and vcpu[4] have zero tids and should not been
compared as valid situation in qemuDomainRefreshVcpuInfo().
This issue is introduced by commit 34f7743, which fix recording of vCPU
pids for MTTCG.
Signed-off-by: Yi Wang <wang.yi59@zte.com.cn>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Added GPFS as shared file system recognized during live migration
security checks.
GPFS is 'IBM General Parallel File System' also called
'IBM Spectrum Scale'
BUG: https://bugzilla.redhat.com/show_bug.cgi?id=1679528
Signed-off-by: Diego Michelotto <diego.michelotto@cnaf.infn.it>
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
The structure used to handle network entries was based on 'if,else'
conditions. This commit converts this ugly structure into a switch to
clearify each option of the handler.
Signed-off-by: Julio Faracco <jcfaracco@gmail.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Extract out the network "type" processing into it's own method
rather than inline within lxcNetworkParseDataSuffix.
Signed-off-by: Julio Faracco <jcfaracco@gmail.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
This commit removes the full network entry setting: "lxc.network.X" to
type only. Like "type", "name", "flags", etc. This will handle entries
regardless of whether they are prefixed by "lxc.network." (today) or
"lxc.net.X." (the future).
Signed-off-by: Julio Faracco <jcfaracco@gmail.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Refactor lxcNetworkWalkCallback to be a simple method to handle
both possible network settings with indexes or the simple one. It is
better the decouple the whole algorithm to parse data to only parse
which entry type libvirt is handling.
The new method is responsible to verify is the settings correspond to
network entry. Right now, it is only verifying "lxc.network.", but in
the future, it can be used to verify "lxc.net.X." too. Any other case
would be rejected.
On the other hand, the idea here is working only with types. If we know
that entry is part of network settings, after we just need to know which
type is. It keeps the handler simple.
Signed-off-by: Julio Faracco <jcfaracco@gmail.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
The new method called lxcNetworkParseDataIPs() is responsible to handle
IPv{4,6} settings now. The idea is let lxcNetworkWalkCallback() method
handle all entries related to network definition only.
Signed-off-by: Julio Faracco <jcfaracco@gmail.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
libvirt_iohelper is used internally by the virFileWrapperFd APIs;
more specifically, in the QEMU driver we have the doCoreDump() and
qemuDomainSaveMemory() helper functions as users, and those in turn
end up being called by the implementation of several driver APIs.
By calling virReportError() if libvirt_iohelper has failed, we
overwrite whatever generic error message QEMU might have raised
with the more useful one generated by the helper program.
After this commit, the user will be able to see the error directly
instead of having to dig in the journal or libvirtd log.
https://bugzilla.redhat.com/show_bug.cgi?id=1578741
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
virFileWrapperFdFree(), like all free functions, is supposed
to only release allocated resources, so error reporting is
better suited for virFileWrapperFdClose().
This reverts commit b0c3e93180.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Right now we're reporting errors in virFileWrapperFdFree(),
but that's hardly the appropriate place to do so, as free
functions are supposed to do nothing more than release
allocated resources.
We want to move that code back into virFileWrapperFdClose(),
but before we can do that we need to make sure the function
is actually called every time we're done processing the
wrapped file. The cleanup path is the obvious candidate.
In a couple of cases we can just move the call, but for the
remaining ones we need to duplicate it instead in order not
to alter the existing behavior. We do, however, make sure
that in all cases a failure to properly close the wrapper
results in the overall operation being reported as failed.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
We'll want to use this function in the cleanup path soon,
and in order to be able to do that we need to make sure we
can call it multiple times on the same virFileWrapperFd
without side effects.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Replace virDomainChrSourceDefFree with virObjectUnref.
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Use refcounting for priv->monConfig instead of asymmetric freeing.
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Change fb01e1a44 "virt-aa-helper: generate rules for gl enabled
graphics devices" implemented the detection for gl enabled
devices in virt-aa-helper. But further testing showed
that it will need much more access for the full gl stack
to work.
Upstream apparmor just recently split those things out and now
has two related abstractions at
https://gitlab.com/apparmor/apparmor/blob/master:
- dri-common at /profiles/apparmor.d/abstractions/dri-common
- mesa: at /profiles/apparmor.d/abstractions/mesa
If would be great to just include that for the majority of
rules, but they are not yet in any distribution so we need
to add rules inspired by them based on the testing that we
can do.
Furthermore qemu with opengl will also probe the backing device
of the rendernode for attributes which should be safe as
read-only wildcard rules.
Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1815452
Acked-by: Jamie Strandboge <jamie@canonical.com>
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
Change fb01e1a44 "virt-aa-helper: generate rules for gl enabled
graphics devices" implemented the detection for gl enabled
devices in virt-aa-helper. But it will in certain cases e.g. if
no rendernode was explicitly specified need to read /dev/dri
which it currently isn't allowed.
Add a rule to the apparmor profile of virt-aa-helper itself to
be able to do that.
Acked-by: Jamie Strandboge <jamie@canonical.com>
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
Add a bhyveDomainDefNeedsISAController() helper function
which by domain configuration determines whether LPC controller is
required or not.
Signed-off-by: Roman Bogorodskiy <bogorodskiy@gmail.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Implement the MSRs ignore unknown reads and writes feature
that's specified using:
<features>
...
<msrs unknown='ignore'>
...
</features>
in the domain XML.
In bhyve, it's just passing '-w' command line argument to the bhyve(8)
executable.
Signed-off-by: Roman Bogorodskiy <bogorodskiy@gmail.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
Introduce the 'msrs' feature element that controls Model Specific
Registers related behaviour. At this moment it allows only
single tunable attribute "unknown":
<msrs unknown='ignore|fault'/>
Which tells hypervisor to ignore accesses to unimplemented
Model Specific Registers. The only user of that for now is going
to be the bhyve driver.
Signed-off-by: Roman Bogorodskiy <bogorodskiy@gmail.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
The tests/cputestdata/cpu-parse.sh script has been broken since the
cpu_map.xml file was split into several XMLs.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Replace all uses where virBuffer would need clearing on the cleanup
path.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
virBuffer is almost always stack-allocated, but requires freeing of the
internals on error. Introduce a VIR_AUTOCLEAN function to deal with
this.
Along with the addition add a test which would leak the buffer contents
if it weren't autocleaned.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
The new utility macros are useful for variables we put on the stack but
require some cleanup. The most prominent of those is virBuffer which is
used almost exclusively in that way.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
The messages reference testBufEscapeN instead of testBufEscapeRegex.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
The conversion to VIR_AUTOFREE of 'escapeList' introduced memory leak of
the copied item to be escaped:
==17517== 2 bytes in 1 blocks are definitely lost in loss record 1 of 32
==17517== at 0x483880B: malloc (vg_replace_malloc.c:309)
==17517== by 0x54D666D: strdup (in /usr/lib64/libc-2.28.so)
==17517== by 0x497663E: virStrdup (virstring.c:956)
==17517== by 0x497663E: virStrdup (virstring.c:945)
==17517== by 0x48F8853: virBufferEscapeN (virbuffer.c:707)
==17517== by 0x403C9D: testBufEscapeN (virbuftest.c:383)
==17517== by 0x405FA8: virTestRun (testutils.c:174)
==17517== by 0x403A70: mymain (virbuftest.c:517)
==17517== by 0x406BC9: virTestMain (testutils.c:1097)
==17517== by 0x5470412: (below main) (in /usr/lib64/libc-2.28.so)
[...] (all other have same backtrace as it happens in a loop)
Fix it by reverting all the VIR_AUTO nonsense in this function as there
is exactly one place where it's handled.
This effectively reverts commits:
d0a92a037196fbf6df90d261ed2fb1
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
'virBufferFreeAndReset' does not free the top level structure itself.
Additionally we almost exclusively use stack'd buffers rather than
pointers.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
dnsmasq documentation says that the *IPv4* prefix/network
address/broadcast address sent to dhcp clients will be automatically
determined by dnsmasq by looking at the interface it's listening on,
so the original libvirt code did not add a netmask to the dnsmasq
commandline (or later, the dnsmasq conf file).
For *IPv6* however, dnsmasq apparently cannot automatically determine
the prefix (functionally the same as a netmask), and it must be
explicitly provided in the conf file (as a part of the dhcp-range
option). So many years after IPv4 DHCP support had been added, when
IPv6 dhcp support was added the prefix was included at the end of the
dhcp-range setting, but only for IPv6.
A user had reported a bug on a host where one of the interfaces was a
superset of the libvirt network where dhcp is needed (e.g., the host's
ethernet is 10.0.0.20/8, and the libvirt network is 10.10.0.1/24). For
some reason dnsmasq was supplying the netmask for the /8 network to
clients requesting an address on the /24 interface.
This seems like a bug in dnsmasq, but even if/when it gets fixed
there, it looks like there is no harm in just always adding the
netmask to all IPv4 dhcp-range options similar to how prefix is added
to all IPv6 dhcp-range options.
Signed-off-by: Laine Stump <laine@laine.org>
Reviewed-by: John Ferlan <jferlan@redhat.com>
This fixes a bug that has been present since the original version of
the function was pushed in commit 1ab80f3 on Nov. 26 2010 (by me). The
virSocketAddr::len was not being set.
Apparently until now we were always calling
virSocketAddrPrefixToNetmask with virSocketAddr object that was
already (coincidentally) initialized for the proper address family,
but the bug became apparent when trying to use it to fill in an
otherwise uninitialized object.
Signed-off-by: Laine Stump <laine@laine.org>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Most of the code base is fairly consistent about using the name
'uuidstr' when dealing with a formatted human-readable form, and
'uuid' when dealing with the smaller raw bytes form. Fix
snapshot_conf to comply, as well as reducing the scope of a human
string to only the error message that needs it.
Signed-off-by: Eric Blake <eblake@redhat.com>
Signal the udev thread the change of `priv->threadQuit` by using the
thread condition.
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
If the udev thread is stopped, it must be ensured that the watch
handle is also removed from the main loop.
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Currently, some arguments are called strcontent and strsrc, or
content and src or some other combination. This makes it
impossible to see at the first glance what argument is supposed
to represent 'expected' value and which one represents 'actual'
value. Rename the arguments to make it obvious.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The current naming makes it hard for me to see which holds the
expected value and which holds the actual value. Rename them to
make it obvious.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
In order to save a few lines of code, and also since it's hype
let's use VIR_AUTOFREE() for the two strings we allocate there.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The iohelper is an internal program that's only supposed to
be called by libvirt, and whatever output it might produce
will ultimately be passed to virReportError() or similar.
Since we do not want strings passed to those functions to
contain newlines, we can simply not output them in the first
place.
This is what happens in pretty much all cases already, but
in a couple instances newlines have managed to slip in.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Laine Stump <laine@laine.org>
Commit f609cb85 (0.9.5) introduced virDomainSnapshotGetXMLDesc()'s use
of @flags as a subset of virDomainXMLFlags, documenting that 2 of the
3 flags defined at the time would never be valid. Later, commit
28f8dfdc (1.0.0) introduced a new flag, VIR_DOMAIN_XML_MIGRATABLE, but
did not adjust the snapshot documentation to declare it as invalid.
However, since the flag is not accepted as valid by any of the
drivers (remote is just passthrough; esx and vbox don't support flags;
qemu, test, and vz only support VIR_DOMAIN_XML_SECURE), and it is
unlikely that the domain state saved off during a snapshot creation
needs to be migration-friendly (as the snapshot is not the source of
a migration), it is easier to just define an explicit set of supported
flags directly related to the snapshot API rather than trying to
borrow from domain API, and risking confusion if even more domain
flags are added later (in fact, I have an upcoming patch that plans to
add a new flag to virDomainGetXMLDesc that makes no sense for
snapshots).
There is no API or ABI impact (since we purposefully used unsigned int
rather than an enum type in public API, and since the new flag name
carries the same value as the reused name).
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Commit d2a929d4 (0.9.4) defined virDomainSaveImageGetXMLDesc()'s use
of @flags as a subset of virDomainXMLFlags, documenting that 2 of the
3 flags defined at the time would never be valid. Later, commit
28f8dfdc (1.0.0) introduced a new flag, VIR_DOMAIN_XML_MIGRATABLE, but
did not adjust the save image documentation to declare it as invalid.
Later, commit a67e3872 (3.7.0) blindly copied and pasted the same text
into virDomainManagedSaveGetXMLDesc.
However, since the flag is not accepted as valid by any of the
drivers (remote is just passthrough; and qemu is the only supporting
driver for either API, with support for just VIR_DOMAIN_XML_SECURE),
it is easier to just define an explicit set of supported flags
directly related to the save image API rather than trying to borrow
from live domain API, and risking confusion if even more domain flags
are added later (in fact, I have an upcoming patch that plans to add
a new flag to virDomainGetXMLDesc that makes no sense for saved
images). We may someday decide that saved images need to support the
_MIGRATABLE flag, as it is possible to load a saved image with a
different version of libvirt than the one that created it, but that
can be a separate patch if it is ever needed. Meanwhile, it DOES make
sense to reuse the same flags for SaveImage and for ManagedSave (since
ManagedSave is really just sugar for creating a normal SaveImage in a
location controlled by libvirt instead of by the user).
There is no API or ABI impact (since we purposefully used unsigned int
rather than an enum type in public API, and since the new flag name
carries the same value as the old reused name).
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Although VIR_DOMAIN_DEF_FORMAT_INACTIVE and VIR_DOMAIN_XML_INACTIVE
happen to have the same value (1<<1), they come from different enums;
and it is nicer to reason about a 'flags' variable if all uses of
that variable are compared against the same enum type. Messed up in
commit 06f75ff2 (3.8.0).
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Many drivers had a comment that they did not validate the incoming
'flags' to virDomainGetXMLDesc() because they were relying on
virDomainDefFormat() to do it instead. This used to be the case
(at least since 461e0f1a and friends in 0.9.4 added unknown flag
checking in general), but regressed in commit 0ecd6851 (1.2.12),
when all of the drivers were changed to pass 'flags' through the
new helper virDomainDefFormatConvertXMLFlags(). Since this helper
silently ignores unknown flags, we need to implement flag checking
in each driver instead.
Annoyingly, this means that any new flag values added will silently
be ignored when targeting an older libvirt, rather than our usual
practice of loudly diagnosing an unsupported flag. Add comments
in domain_conf.[ch] to remind us to be extra vigilant about the
impact when adding flags (a new flag to add data is safe if the
older server omitting the requested data doesn't break things in
the newer client; a new flag to suppress data rather than enhancing
the existing VIR_DOMAIN_XML_SECURE may form a data leak or even a
security hole).
In the qemu driver, there are multiple callers all funnelling to
qemuDomainDefFormatBufInternal(); many of them already validated
flags (and often only a subset of the full set of possible flags),
but for ease of maintenance, we can also check flags at the common
helper function.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
