If the mirror destination is not a file but a NVMe disk, then
call qemuHostdevReAttachOneNVMeDisk() to reattach the NVMe back
to the host.
This would be done by blockjob code when the job finishes, but in
this case the job won't finish - QEMU is killed meanwhile.
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1825785
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
In v5.10.0-rc1~42 (which was later fixed in v6.0.0-rc1~487) I am
removing XATTRs for a file that QEMU is mirroring a disk into but
it is killed meanwhile. Well, we can call
qemuSecurityRestoreImageLabel() which will not only remove XATTRs
but also use them to restore the original owner of the file.
This would be done by blockjob code when the job finishes, but in
this case the job won't finish - QEMU is killed meanwhile
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
In previous commit we started tracking whether QEMU supports
'-numa mem='. This is tied to the machine type because migration
from '-numa mem=' to '-numa memdev' is impossible (or vice
versa). But since it's tied to a machine type (where migration
from one to another is also unsupported) we can allow QEMU to get
rid of the deprecated command line.
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1783355
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
When building -numa command line there is a for() loop that
builds '-numa memdev=' for each guest NUMA node. And also
records in a local variable whether any of memory-object-*
backends must be used to satisfy desired config. Well, instead of
checking in each iteration whether corresponding capabilities are
set, we can do swap if() and for() and check only once.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
There is 'numa-mem-supported' machine attribute which specifies
whether '-numa mem=' is supported. Store it in our capabilities
as it will be used in later commits when building the command
line.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The AppArmor secdriver does not use labels to grant access to
resources. Therefore, it doesn't use XATTRs and hence it lacks
implementation of .domainMoveImageMetadata callback. This leads
to a harmless but needless error message appearing in the logs:
virSecurityManagerMoveImageMetadata:476 : this function is not
supported by the connection driver: virSecurityManagerMoveImageMetadata
Closes: https://gitlab.com/libvirt/libvirt/-/issues/25
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
==179663== 35 (24 direct, 11 indirect) bytes in 1 blocks are definitely lost in loss record 205 of 461
==179663== at 0x4839EC6: calloc (vg_replace_malloc.c:762)
==179663== by 0x5791AC0: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.6400.1)
==179663== by 0x190C79: qemuDomainObjPrivateXMLParseBlockjobDataCommit (qemu_domain.c:3295)
==179663== by 0x190DF7: qemuDomainObjPrivateXMLParseBlockjobDataSpecific (qemu_domain.c:3331)
==179663== by 0x19157D: qemuDomainObjPrivateXMLParseBlockjobData (qemu_domain.c:3469)
==179663== by 0x1918E8: qemuDomainObjPrivateXMLParseBlockjobs (qemu_domain.c:3498)
==179663== by 0x193841: qemuDomainObjPrivateXMLParse (qemu_domain.c:3944)
==179663== by 0x4A1BA9D: virDomainObjParseXML (domain_conf.c:22306)
==179663== by 0x4A1BFE9: virDomainObjParseNode (domain_conf.c:22429)
==179663== by 0x4A1C0B4: virDomainObjParseFile (domain_conf.c:22443)
==179663== by 0x1431E1: testCompareStatusXMLToXMLFiles (qemuxml2xmltest.c:61)
==179663== by 0x177722: virTestRun (testutils.c:142)
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
==156803== 58 (40 direct, 18 indirect) bytes in 1 blocks are definitely lost in loss record 306 of 463
==156803== at 0x4839EC6: calloc (vg_replace_malloc.c:762)
==156803== by 0x5791AC0: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.6400.1)
==156803== by 0x48F60DC: virAlloc (viralloc.c:48)
==156803== by 0x18DD74: qemuStorageSourcePrivateDataAssignSecinfo (qemu_domain.c:2384)
==156803== by 0x18DFD5: qemuStorageSourcePrivateDataParse (qemu_domain.c:2433)
==156803== by 0x49EC884: virDomainStorageSourceParse (domain_conf.c:9857)
==156803== by 0x49ECBA3: virDomainDiskBackingStoreParse (domain_conf.c:9909)
==156803== by 0x49F129D: virDomainDiskDefParseXML (domain_conf.c:10785)
==156803== by 0x4A1804E: virDomainDefParseXML (domain_conf.c:21543)
==156803== by 0x4A1B60C: virDomainObjParseXML (domain_conf.c:22254)
==156803== by 0x4A1BFE9: virDomainObjParseNode (domain_conf.c:22429)
==156803== by 0x4A1C0B4: virDomainObjParseFile (domain_conf.c:22443
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
The @tmpIfname is a pointer into a const string. To avoid
mistakenly changing the const string via the pointer, make the
pointer const too.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
As suggested in the linked bug, libvirt should firstly check
whether the major number of the device is device mapper major.
Because if it isn't subsequent DM_DEVICE_DEPS task may not only
fail, but also yield different results. In the bugzilla this is
demonstrated by creating a devmapper target named 'loop0' and
then creating loop target /dev/loop0. When the latter is then
passed to a domain, our virDevMapperGetTargetsImpl() function
blindly asks devmapper to provide target dependencies for
/dev/loop0 and because of the way devmapper APIs work, it will
'sanitize' the input by using the last component only which is
'loop0' and thus return different results than expected.
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1823976
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
We have a framework to register cleanup callbacks that are run
when a domain is shut down. The idea is to run callbacks in
reverse order than they were registered. However, looking at the
code this is not the case. Fortunately, this framework is used to
register a single callback and a single callback only -
qemuMigrationDstPrepareCleanup() - therefore there was no problem
just yet.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
This partially reverts fe65e9c8b5.
In the referenced commit I removed @ret from
virHostValidateBhyve() thinking it wasn't used when in fact it is
- it's usage is hidden under MODULE_STATUS_WARN(). Reintroduce
the variable back.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
In a few places we use 0 and false, or 1 and true interchangeably
even though the variable or return type in question is boolean.
Fix those places.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
There are few places where a return variable is introduced (ret
or retval), but then is never changed and is then passed to
return. Well, we can return the value that the variable is
initialized to directly.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
There are few functions that currently return an integer but in
fact they always return the same integer (zero). Make them void.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Since it's introduction in v0.9.7-147-gf4324e3292 the
virNetServerClientInitKeepAlive() function returned nothing than
a negative one. Fortunately, this did not pose any problem
because we ignored the retval happily. Well, it's time to check
for the retval because the function might fail regularly.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Instead of the following pattern:
type ret;
...
ret = func();
return ret;
we can use:
return func()
directly.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
From time to time we are asked which PCI addresses are reserved
in QEMU. Let's document them in one place, it's easier than
reconstructing the list from the code each time.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Laine Stump <laine@redhat.com>
In 97a0aa2467 the @actual variable was renamed to @confactual.
However, the commit missed non-Linux case resulting in a broken
build.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
When a device is "move"-d (this basically means it was renamed),
we add the new device onto our list but keep the old there too.
Fortunately, udev sets this DEVPATH_OLD property which points to
the old device path. We can use it to remove the old instance.
To test this try renaming an interface, for instance:
# ip link set tunl0 name tunl1
# ip link set tunl1 name tunl0
One problem with udev is that it sends old ifname in INTERFACE
property, which creates a problem for us, the property is where
we get the ifname from and use it then to query all kind of info
about the interface. Well, if it is non-existent then we can't
query anything. This happens if ifname rename is suppressed
(net.ifnames=0 on kernel cmd line for instance). Fortunately, we
can use "kernel" source for udev events which has always the
fresh info.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
Move internals of udevRemoveOneDevice() into a separate function
which accepts sysfs path as an argument and actually removes the
device from the internal list. It will be reused later.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
When removing a node device object from the internal list the
udevRemoveOneDevice() function does plain unref over the object.
This is not sufficient. If there is another thread that's waiting
for the object lock it will wait forever.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
While I'm at it, use more g_autofree and g_autoptr() in this
file. This also fixes a possible mem-leak in
virNetDevGetVirtualFunctions().
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
I've just got a new machine and I'm still converging on the
kernel config. Anyway, since I don't have enabled any of SRIO-V
drivers, my kernel doesn't have NET_DEVLINK enabled (i.e.
virNetDevGetFamilyId() returns 0). But this makes nodedev driver
ignore all interfaces, because when enumerating all devices via
udev, the control reaches virNetDevSwitchdevFeature() eventually
and subsequently virNetDevGetFamilyId() which 'fails'. Well, it's
not really a failure - the virNetDevSwitchdevFeature() stub
simply returns 0.
Also, move the call a few lines below, just around the place
where it's needed.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Introduced in v3.8.0-rc1~96, the virNetDevGetFamilyId() gets
netlink family ID for passed family name (even though it's used
only for getting "devlink" ID). Nevertheless, the function
returns 0 on an error or if no family ID was found. This makes it
harder for a caller to distinguish these two. Change the retval
so that a negative value is returned upon error, zero is no ID
found (but no error encountered) and a positive value is returned
on successful translation.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
As explained in the previous commit, we need to relabel the file
we are restoring the domain from. That is the FD that is passed
to QEMU. If the file is not under /dev then the file inside the
namespace is the very same as the one in the host. And regardless
of using transactions, the file will be relabeled. But, if the
file is under /dev then when using transactions only the copy
inside the namespace is relabeled and the one in the host is not.
But QEMU is reading from the one in the host, actually.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1772838
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
This API allows drivers to separate out handling of @stdin_path
of virSecurityManagerSetAllLabel(). The thing is, the QEMU driver
uses transactions for virSecurityManagerSetAllLabel() which
relabels devices from inside of domain's namespace. This is what
we usually want. Except when resuming domain from a file. The
file is opened before any namespace is set up and the FD is
passed to QEMU to read the migration stream from. Because of
this, the file lives outside of the namespace and if it so
happens that the file is a block device (i.e. it lives under
/dev) its copy will be created in the namespace. But the FD that
is passed to QEMU points to the original living in the host and
not in the namespace. So relabeling the file inside the namespace
helps nothing.
But if we have a separate API for relabeling the restore file
then the QEMU driver can continue calling
virSecurityManagerSetAllLabel() with transactions enabled and
call this new API without transactions.
We already have an API for relabeling a single file
(virSecurityManagerDomainSetPathLabel()) but in case of SELinux
it uses @imagelabel (which allows RW access) and we want to use
@content_context (which allows RO access).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Previously, we used virCapabilitiesDomainDataLookup() to fill
machine type in post parse callback if none was provided in the
domain XML. If machine type couldn't be filled in an error was
reported. After 4a4132b462 we've changed it to
virQEMUCapsGetPreferredMachine() which returns NULL, but we no
longer report an error and proceed with the post parse callbacks
processing. This may lead to a crash because the code later on
assumes def->os.machine is not NULL.
Fixes: 4a4132b462
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Pavel Mores <pmores@redhat.com>
When preparing to do a blockcopy, the mirror image is modified so
that QEMU can access it. For instance, the mirror has seclabels
set, if it is a NVMe disk it is detached from the host and so on.
And usually, the restore is done upon successful finish of the
blockcopy operation. But, if something fails then we need to
explicitly revoke the access to the mirror image (and thus
reattach NVMe disk back to the host).
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1822538
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Pavel Mores <pmores@redhat.com>
So far, libvirt generates the following path for automatic dumps:
$autoDumpPath/$id-$shortName-$timestamp
where $autoDumpPath is where libvirt stores dumps of guests (e.g.
/var/lib/libvirt/qemu/dump), $id is domain ID and $shortName is
shortened version of domain name. So for instance, the generated
path may look something like this:
/var/lib/libvirt/qemu/dump/1-QEMUGuest-2020-03-25-10:40:50
While in case of embed driver the following path would be
generated by default:
$root/lib/libvirt/qemu/dump/1-QEMUGuest-2020-03-25-10:40:50
which is not clashing with other embed drivers, we allow users to
override the default and have all embed drivers use the same
prefix. This can create clashing paths. Fortunately, we can reuse
the approach for machined name generation
(v6.1.0-178-gc9bd08ee35) and include part of hash of the root in
the generated path.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
So far, libvirt generates the following path for memory:
$memoryBackingDir/$id-$shortName/ram-nodeN
where $memoryBackingDir is the path where QEMU mmaps() memory for
the guest (e.g. /var/lib/libvirt/qemu/ram), $id is domain ID
and $shortName is shortened version of domain name. So for
instance, the generated path may look something like this:
/var/lib/libvirt/qemu/ram/1-QEMUGuest/ram-node0
While in case of embed driver the following path would be
generated by default:
$root/lib/qemu/ram/1-QEMUGuest/ram-node0
which is not clashing with other embed drivers, we allow users to
override the default and have all embed drivers use the same
prefix. This can create clashing paths. Fortunately, we can reuse
the approach for machined name generation
(v6.1.0-178-gc9bd08ee35) and include part of hash of the root in
the generated path.
Note, the important change is in qemuGetMemoryBackingBasePath().
The rest is needed to pass driver around.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
So far, libvirt generates the following path for hugepages:
$mnt/libvirt/qemu/$id-$shortName
where $mnt is the mount point of hugetlbfs corresponding to
hugepages of desired size (e.g. /dev/hugepages), $id is domain ID
and $shortName is shortened version of domain name. So for
instance, the generated path may look something like this:
/dev/hugepages/libvirt/qemu/1-QEMUGuest
But this won't work with embed driver really, because if there
are two instances of embed driver, and they both want to start a
domain with the same name and with hugepages, both drivers will
generate the same path which is not desired. Fortunately, we can
reuse the approach for machined name generation
(v6.1.0-178-gc9bd08ee35) and include part of hash of the root in
the generated path.
Note, the important change is in qemuGetBaseHugepagePath(). The
rest is needed to pass driver around.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
This reverts commit 06a19921b6.
What I haven't realized when writing this ^^ commit is that the
virQEMUDriver structure already stores the root directory path.
And since the pointer is immutable it can be accessed right from
the structure and thus there is no need to duplicate it in the
driver config.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
The code that generates "qemu-embed-$hash" is going to be useful
in more places. Separate it out into a function.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
The virDomainGenerateMachineName() function doesn't belong in
src/conf/ really, because it has nothing to do with domain XML
parsing. It landed there because of lack of better place in the
past. But now that we have src/hypervisor/ the function should
live there. At the same time, the function name is changed to
match new location.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Initially introduced in v3.10.0-rc1~172.
When generating a path for memory-backend-file or -mem-path, qemu
driver will use the following pattern:
$memoryBackingDir/libvirt/qemu/$id-$shortName
where $memoryBackingDir defaults to /var/lib/libvirt/qemu/ram but
can be overridden in qemu.conf. Anyway, the "/libvirt/qemu/" part
looks redundant, because it's already contained in the default,
or creates unnecessary nesting if overridden in qemu.conf.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Introduced in v1.2.17-rc1~121, the assumption was that the
driver->privileged is immutable at the time but it might change
in the future. Well, it did not ever since. It is still immutable
variable. Drop the needless accessor then.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
The virQEMUDriverConfigNew() accepts path to root directory for
embed mode as an argument. If the argument is not NULL it uses
the passed value as prefix for some internal paths (e.g.
cfg->libDir). If it is NULL though, it looks if the other
argument, @privileged is true or false and generates internal
paths accordingly. But when calling the function from the test
suite, instead of passing NULL for @root, an empty string is
passed. Fortunately, this doesn't create a problem because in
both problematic cases the generated paths are "fixed" to point
somewhere into build dir or the code which is tested doesn't
access them.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
While it is impossible for VIR_ALLOC() to return an error, we
should be consistent with the rest of the code and not continue
initializing the virSecurityDeviceLabelDef structure.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Even with GLib it is still possible for virQEMUCapsNew() to
return NULL because it calls virQEMUCapsInitialize() which is a
wrapper over pthread_once() which may fail. At least, we still
check for its retval. If it so happens that the virQEMUCapsNew()
fails and returns NULL, we should not dereference it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Unfortunately, yajl_free() is not NOP on NULL. It really does
expect a valid pointer. Therefore, check whether the pointer we
want to pass to it is NULL or not.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
The last usage of plain fork() was removed in v0.9.7-rc1~50, but
we forgot to update the syntax-check exemption list accordingly.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
In the network driver code there's networkKillDaemon() which is
the same as virProcessKillPainfully(). Replace the former with
the later and drop what becomes unused function.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Now, that we know that the virtiofsd will have the pidfile open
and locked we can use virPidFileForceCleanupPath() to kill it and
unlink the pidfile.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Now, that we know that the slirp helper will have the pidfile
open and locked we can use virPidFileForceCleanupPath() to kill
it and unlink the pidfile.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Now, that our virCommandSetPidFile() is more intelligent we don't
need to rely on the daemon to create and lock the pidfile and use
virCommandSetPidFile() at the same time.
NOTE that as advertised in the previous commit, this was
temporarily broken, because both virCommand and
qemuProcessStartManagedPRDaemon() would try to lock the pidfile.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Our virCommand module allows us to set a pidfile for commands we
want to spawn. The caller constructs the string of pidfile path
and then uses virCommandSetPidFile() to tell the module to write
the pidfile once the command is ran. This usually works, but has
two flaws:
1) the child process does not hold the pidfile open & locked.
Therefore, the caller (or anybody else) can't use our fancy
virPidFileForceCleanupPath() function to kill the command
afterwards. Also, for everybody else on the system it's
needlessly harder to check if the pid from the pidfile is still
alive or not.
2) if the caller ever makes a mistake and passes the same pidfile
path for two different commands, the start of the second command
will overwrite the pidfile even though the first command might
still be running.
NOTE that this temporarily renders some command spawning
unusable, specifically those code patterns where both
virCommandSetPidFile() is used together with instructing spawned
command to acquire pidfile itself. Fortunately, there is only one
occurrence of such pattern and it is in
qemuProcessStartManagedPRDaemon(). This is fixed in next commit.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
As a part of c799d150d5 I've introduced a test case that
tests whether passing error object between processes works. The
test spawns a child which reports a system error, parent process
then reads the error and compares with expected output. Problem
with this approach is that error message contains stringified
errno which is not portable. FreeBSD has generally different
messages than Linux. Therefore, use g_strerror() to do the errno
to string translation for us.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
In previous patches virKeyFile was replaced with its GLib
counterpart which created an incompatible change: comments can
now begin only with a number sign (#). While this won't probably
affect anyone, mention it in the release notes.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
When a QEMU process dies in the middle of a hotplug, then we fail
to restore the seclabels on the device. The problem is that if
the thread doing hotplug locks the domain object first and thus
blocks the thread that wants to do qemuProcessStop(), the
seclabel cleanup code will see vm->pid still set and mount
namespace used and therefore try to enter the namespace
represented by the PID. But the PID is gone really and thus
entering will fail and no restore is done. What we can do is to
try enter the namespace (if requested to do so) but if entering
fails, fall back to no NS mode.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1814481
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Pavel Mores <pmores@redhat.com>
When running a function in a forked child, so far the only thing
we could report is exit status of the child and the error
message. However, it may be beneficial to the caller to know the
actual error that happened in the child.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Pavel Mores <pmores@redhat.com>
The @src is not always a file. It may also be a directory (for
instance qemuDomainCreateDeviceRecursive() assumes that) - even
though it doesn't happen usually. Anyway, mount() can mount only
a dir onto a dir and a file onto a file.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Pavel Mores <pmores@redhat.com>
The @devPath variable is not modifiable. It merely just points to
string containing path where private devtmpfs is being
constructed. Make it const so it doesn't look weird that it's not
freed.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Pavel Mores <pmores@redhat.com>
If building namespace fails somewhere in the middle (that is some
files exists under devMountsSavePath[i]), then plain rmdir() is
not enough to remove dir. Umount the temp location and use
virFileDeleteTree() to remove the directory.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Pavel Mores <pmores@redhat.com>
The virFileMakePathWithMode() which is our recursive version of
mkdir() fails, it simply just returns a negative value with errno
set. No error is reported (as compared to virFileTouch() for
instance).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Pavel Mores <pmores@redhat.com>
In one of my previous commits I've introduced code that creates
all devices for given (possible) multipath target. But I've made
a mistake there - the code accesses 'next->path' without checking
if the disk source is local. Note that the 'next->path' is
NULL/doesn't make sense for VIR_STORAGE_TYPE_NVME.
Fixes: a30078cb83
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1814947
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
So far, when using the qemu:///embed driver, management
applications can't chose whether they want to register their
domains in machined or not. While having that option is certainly
desired, it will require more work. What we can do meanwhile is
to generate names that include part of hash of the root
directory. This is to ensure that if two applications using
different roots but the same domain name (and ID) start the
domain no clashing name for machined is generated.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
When initializing virQEMUDriverConfig structure we are given the
root directory for possible embed connection. Save it for future
use. While we could get it later from @uri member, it's not as
easy as dereferencing a pointer (virURIParse() +
virURIGetParam() + error reporting).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
According to the linked BZ, machined expects either valid
hostname or valid FQDN (see systemd commit
v239-3092-gd65652f1f2). While in case of multiple dots, a
trailing one doesn't violate FQDN, it does violate the rule in
case of something simple, like "domain.". But it's safe to remove
it in both cases.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1808499
Fixes: 45464db8ba
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The virQEMUCaps structure has usedQMP member which in the past
used to tell if qemu we are dealing with is capable of QMP. Well,
we don't support HMP anymore (minus a few HMP passthrough
commands, which are wrapped into QMP anyways) and the member is
not used really.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
If a disk has persistent reservations enabled, qemu-pr-helper
might open not only /dev/mapper/control but also individual
targets of the multipath device. We are already querying for them
in CGroups, but now we have to create them in the namespace too.
This was brought up in [1].
1: https://bugzilla.redhat.com/show_bug.cgi?id=1711045#c61
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Tested-by: Lin Ma <LMa@suse.com>
Reviewed-by: Jim Fehlig <jfehlig@suse.com>
The virbpf module wraps syscalls to BPF. However, if the kernel
headers used at the compile time don't have support for BPF the
module offers stubs which return a negative one to signal error
to the caller. But there is a slight discrepancy between real
functions and these stubs. While the former set errno and return
-1 the latter report an error (without setting the errno) and
return -1. This is not optimal because the caller might see stale
errno and overwrite the error message with a less accurate one.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
In the virCgroupV2DevicesAvailable() function we try to determine
whether CGroups version 2 are available. We do this by opening
what we believe is the CGroup mount point and issuing a BPF call.
When the call fails, a debug message is printed. However, the BPF
call sets errno too. Include it in the debug message to help us
with debugging.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
When rewriting the virDomainDiskTranslateSourcePool() function in
v6.1.0-rc1~184 a typo was introduced. Previously, we allowed
startup policy only for those volumes which translated to
VIR_STORAGE_TYPE_FILE. But starting with the referenced commit,
the value we checked for was changed to VIR_STORAGE_VOL_FILE
which comes from a different enum and has a different value too.
This is wrong, because virStorageSourceGetActualType() returns a
value from the original enum.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1811728
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
When preparing images for block jobs we modify their seclabels so
that QEMU can open them. However, as mentioned in the previous
commit, secdrivers base some it their decisions whether the image
they are working on is top of of the backing chain. Fortunately,
in places where we call secdrivers we know this and the
information can be passed to secdrivers.
The problem is the following: after the first blockcommit from
the base to one of the parents the XATTRs on the base image are
not cleared and therefore the second attempt to do another
blockcommit fails. This is caused by blockcommit code calling
qemuSecuritySetImageLabel() over the base image, possibly
multiple times (to ensure RW/RO access). A naive fix would be to
call the restore function. But this is not possible, because that
would deny QEMU the access to the base image. Fortunately, we
can use the fact that seclabels are remembered only for the top
of the backing chain and not for the rest of the backing chain.
And thanks to the previous commit we can tell secdrivers which
images are top of the backing chain.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1803551
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Our decision whether to remember seclabel for a disk image
depends on a few factors. If the image is readonly or shared or
not the chain top the remembering is suppressed for the image.
However, the virSecurityManagerSetImageLabel() is too low level
to determine whether passed @src is chain top or not. Even though
the function has the @parent argument it does not necessarily
reflect the chain top - it only points to the top level image in
the chain we want to relabel and not to the topmost image of the
whole chain. And this can't be derived from the passed domain
definition reliably neither - in some cases (like snapshots or
block copy) the @src is added to the definition only after the
operation succeeded. Therefore, introduce a flag which callers
can use to help us with the decision.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
When spawning a thread via our virThread APIs we let pthread
spawn this helper thread which sets couple of thread local
variables (e.g. thread job name or thread worker name) and as of
v6.1.0-40-gc85256b31b it also sets pthread name (which is then
visible in `ps' output for instance). Only after these steps the
intended function is called. However, just before calling it we
free the buffer that holds the thread name which results in
invalid memory reads:
==47027== Invalid read of size 1
==47027== at 0x48389C2: strlen (vg_replace_strmem.c:459)
==47027== by 0x58BB3D6: __vfprintf_internal (vfprintf-internal.c:1645)
==47027== by 0x58CE6E0: __vasprintf_internal (vasprintf.c:57)
==47027== by 0x574BA28: g_vasprintf (in /usr/lib64/libglib-2.0.so.0.6000.7)
==47027== by 0x57240CC: g_strdup_vprintf (in /usr/lib64/libglib-2.0.so.0.6000.7)
==47027== by 0x48E0EFA: vir_g_strdup_vprintf (glibcompat.c:209)
==47027== by 0x493AA05: virLogVMessage (virlog.c:573)
==47027== by 0x493A8FE: virLogMessage (virlog.c:513)
==47027== by 0x4992FC7: virThreadJobClear (virthreadjob.c:121)
==47027== by 0x4992844: virThreadHelper (virthread.c:237)
==47027== by 0x5817496: start_thread (pthread_create.c:486)
==47027== by 0x59563CE: clone (clone.S:95)
The problem is that neither virThreadJobSetWorker() nor
virThreadJobSet() create a copy of passed name. They just set a
thread local variable to point to the buffer which is then
freed. Moving the free towards the end of the wrapper function
solves the issue.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
I've found that if my virtlogd is socket activated but the daemon
doesn't run yet, then the virt-qemu-run is killed right after it
tries to start the domain. The problem is that because the default
setting is to use virtlogd, the domain create code tries to
connect to virtlogd socket, which in turn tries to detect who is
connecting (virNetSocketGetUNIXIdentity()) and as a part of it,
it will try to open /proc/${PID_OF_SHIM}/stat which is denied by
SELinux:
type=AVC msg=audit(1582903501.927:323): avc: denied { search } for \
pid=1210 comm="virtlogd" name="1843" dev="proc" ino=37224 \
scontext=system_u:system_r:virtlogd_t:s0-s0:c0.c1023 \
tcontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 tclass=dir \
permissive=0
Virtlogd reacts by closing the connection which the shim sees as
SIGPIPE. Since the default response to the signal is Term, we
don't even get to reporting any error nor to removing the
temporary directory.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
When virt-qemu-run is ran without any root directory specified on
the command line, a temporary directory is made and used instead.
But since we are using g_dir_make_tmp() to create the directory
it is going to have 0700 mode. So even though we create the whole
directory structure under it and label everything, QEMU is very
likely to not have the access. This is because in this case there
is no qemu.conf and thus distro default UID:GID is used to run
QEMU (e.g. qemu:kvm on Fedora). Change the mode of the temporary
directory so that everybody has eXecute permission.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Libvirt tries to forbid migration onto the same host and it does
that by checking if local and remote hostnames are the same and
whether local and remote UUIDs are the same. Well, the latter
makes sense but the former doesn't really because libvirtd can be
running inside an UTS namespace and hostnames can appear the same
on both sides of migration. On the other hand, host UUIDs are
unique, so rely on them when trying to prevent migration onto the
same host.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1639596
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
The way that our file locking works is that we open() the file we
want to lock and then use fcntl(fd, F_SETLKW, ...) to lock it.
The problem is, we are doing all of these as root which doesn't
work if the file lives on root squashed NFS, because if it does
then the open() fails. The way to resolve this is to make this a
non fatal error and leave callers deal with this (i.e. disable
remembering) - implemented in the previous commit.
https://bugzilla.redhat.com/show_bug.cgi?id=1804672
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
There are some cases where we want to remember the original owner
of a file but we fail to lock it for XATTR change (e.g. root
squashed NFS). If that is the case we error out and refuse to
start a domain. Well, we can do better if we disable remembering
for paths we haven't locked successfully.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
So far, in the lock state we are storing only the file
descriptors of the files we've locked. Therefore, when unlocking
them and something does wrong the only thing we can report is FD
number, which is not user friendly at all. But if we store paths
among with FDs we can do better error reporting.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
The variable is allocated, but never freed.
==119642== 29 bytes in 1 blocks are definitely lost in loss record 409 of 671
==119642== at 0x483579F: malloc (vg_replace_malloc.c:309)
==119642== by 0x5AB075F: __vasprintf_internal (in /lib64/libc-2.29.so)
==119642== by 0x57C1A28: g_vasprintf (in /usr/lib64/libglib-2.0.so.0.6000.7)
==119642== by 0x579A0CC: g_strdup_vprintf (in /usr/lib64/libglib-2.0.so.0.6000.7)
==119642== by 0x4AE6D58: vir_g_strdup_printf (glibcompat.c:197)
==119642== by 0x136EEE: qemuTestParseCapabilitiesArch (testutilsqemu.c:291)
==119642== by 0x138506: testQemuInfoSetArgs (testutilsqemu.c:763)
==119642== by 0x135FFF: mymain (qemuxml2argvtest.c:3093)
==119642== by 0x13A60E: virTestMain (testutils.c:839)
==119642== by 0x1368C2: main (qemuxml2argvtest.c:3121)
Fixes: 42b3e5b9e4
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The persistent alias name @persistent is allocated in
virDomainNetDefParseXML() but never freed.
==119642== 22 bytes in 2 blocks are definitely lost in loss record 178 of 671
==119642== at 0x483579F: malloc (vg_replace_malloc.c:309)
==119642== by 0x58F89F1: xmlStrndup (in /usr/lib64/libxml2.so.2.9.9)
==119642== by 0x4BA3B74: virXMLPropString (virxml.c:520)
==119642== by 0x4BDB0C5: virDomainNetDefParseXML (domain_conf.c:11876)
==119642== by 0x4BF9EF4: virDomainDefParseXML (domain_conf.c:21196)
==119642== by 0x4BFCD5B: virDomainDefParseNode (domain_conf.c:21943)
==119642== by 0x4BFCC36: virDomainDefParse (domain_conf.c:21901)
==119642== by 0x4BFCCCB: virDomainDefParseFile (domain_conf.c:21924)
==119642== by 0x114A9D: testCompareXMLToArgv (qemuxml2argvtest.c:452)
==119642== by 0x13894F: virTestRun (testutils.c:143)
==119642== by 0x11F46E: mymain (qemuxml2argvtest.c:1316)
==119642== by 0x13A60E: virTestMain (testutils.c:839
Fixes: fb0509d06a
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The privateData object is allocated in virDomainFSDefNew() but
never unref'd.
==119642== 480 bytes in 20 blocks are definitely lost in loss record 656 of 671
==119642== at 0x4837B86: calloc (vg_replace_malloc.c:762)
==119642== by 0x57806A0: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.6000.7)
==119642== by 0x4AE7392: virAllocVar (viralloc.c:331)
==119642== by 0x4B64395: virObjectNew (virobject.c:241)
==119642== by 0x48F1464: qemuDomainFSPrivateNew (qemu_domain.c:1427)
==119642== by 0x4BBF004: virDomainFSDefNew (domain_conf.c:2307)
==119642== by 0x4BD859A: virDomainFSDefParseXML (domain_conf.c:11217)
==119642== by 0x4BF9DD1: virDomainDefParseXML (domain_conf.c:21179)
==119642== by 0x4BFCD5B: virDomainDefParseNode (domain_conf.c:21943)
==119642== by 0x4BFCC36: virDomainDefParse (domain_conf.c:21901)
==119642== by 0x4BFCCCB: virDomainDefParseFile (domain_conf.c:21924)
==119642== by 0x114A9D: testCompareXMLToArgv (qemuxml2argvtest.c:452)
Fixes: 5120577ed7
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Firstly, the check for disk I/O error can be moved into 'if
(!offline)' section a few lines below.
Secondly, checks for vmstate and slirp should be moved under the
same section because they reflect live state of a domain. For
offline migration no QEMU is involved and thus these restrictions
are not valid.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
In two places where virPidFileForceCleanupPath() is called, we
try to unlink() the pidfile again. This is needless because
virPidFileForceCleanupPath() has done just that.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The virPidFileReadPath() function is supposed to return 0 on
success or a negative value on failure. But the negative value
has a special meaning - it's negated errno. Therefore, when
converting string to int we shouldn't return -1 which translates
to EPERM. Returning EINVAL looks closer to the truth.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
If a domain has a NVMe disk it already has the access configured.
Trying to configure it again on a commit or some other operation
is wrong and condemned to failure.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The location of virt-aa-helper shown in the docs is incorrect.
The helper binary is installed under libexec dir.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
The configure script allows users to specify different paths for
/etc/, /usr/sbin/, /var/run/ and /usr/libexec/. Instead of
assuming user will pass expected value, generate the apparmor
profiles using the actual values.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Now, that every use of virAtomic was replaced with its g_atomic
equivalent, let's remove the module.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
In future commits our virAtomic* APIs will be replaced with their
GLib variants. Instead of trying to update the test after each
commit and eventually removing the test anyway, remove it upfront
and save the hassle.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Michal Privoznik