Many calls of qemuMonitorDelObject don't actually check the return value
or report the error from the object deletion itself since they are on
cleanup paths. In some cases this can lead to reporting of spurious
errors e.g. when qemuMonitorDelObject is used to clean up a possibly
pre-existing objects.

Add a new argument for qemuMonitorDelObject which controls whether
the internals report errors from qemu and fix all callers accordingly.

Note that some of the cases on device unplug which check the error code
don't in fact propagate the error to the user, but in this case it is
important to add the log entry anyways for tracing that the device
deletion failed.

https://bugzilla.redhat.com/show_bug.cgi?id=1784040Signed-off-by: NPeter Krempa <pkrempa@redhat.com>
Reviewed-by: NMichal Privoznik <mprivozn@redhat.com>

When changing media we'd attempt to remove the managed pr daemon even if
neither of the images involved in the media change used it. This caused
libvirtd to log a spurious error:

2020-03-18 01:41:19.832+0000: 643207: error : qemuMonitorJSONCheckError:412 : internal error: unable to execute QEMU command 'object-del': object 'pr-helper0' not found

With this patch we completely avoid calling the deletion code.

https://bugzilla.redhat.com/show_bug.cgi?id=1814486Signed-off-by: NPeter Krempa <pkrempa@redhat.com>
Reviewed-by: NMichal Privoznik <mprivozn@redhat.com>

The loop which checks whether the vcpus are in proper configuration for
the requested hot(un)plug skips the first modified vcpu. This means
that 'firstvcpu' which is used to print the error message in case the
configuration is not suitable would never point to the first modified
vcpu.

In cases such as:

  <vcpu placement='auto' current='5'>8</vcpu>
  <vcpus>
    <vcpu id='0' enabled='yes' hotpluggable='no'/>
    <vcpu id='1' enabled='yes' hotpluggable='no'/>
    <vcpu id='2' enabled='yes' hotpluggable='no'/>
    <vcpu id='3' enabled='yes' hotpluggable='no'/>
    <vcpu id='4' enabled='yes' hotpluggable='no'/>
    <vcpu id='5' enabled='no' hotpluggable='yes'/>
    <vcpu id='6' enabled='no' hotpluggable='yes'/>
    <vcpu id='7' enabled='no' hotpluggable='yes'/>
  </vcpus>

 # virsh setvcpu --config --disable  upstream 1
 error: invalid argument: vcpu '-1' can't be modified as it is followed by non-hotpluggable online vcpus

After this fix the proper vcpu is reported in the error message:

 # virsh setvcpu --config --disable  upstream 1
 error: invalid argument: vcpu '1' can't be modified as it is followed by non-hotpluggable online vcpu

https://bugzilla.redhat.com/show_bug.cgi?id=1611061Signed-off-by: NPeter Krempa <pkrempa@redhat.com>
Reviewed-by: NDaniel Henrique Barboza <danielhb413@gmail.com>

There are two last callers of this function. Replace them by
qemuAliasForSecret and delete qemuDomainGetSecretAESAlias.
Signed-off-by: NPeter Krempa <pkrempa@redhat.com>
Reviewed-by: NJán Tomko <jtomko@redhat.com>

Currently we don't have infrastructure to remember the secret aliases
for hostdevs. Since an upcoming patch is going to change aliases for
the disks, initialize the iscsi hostdevs separately so that we can keep
the alias. At the same time let's use qemuAliasForSecret instead of
qemuDomainGetSecretAESAlias when unplugging the iscsi hostdev.
Signed-off-by: NPeter Krempa <pkrempa@redhat.com>
Reviewed-by: NJán Tomko <jtomko@redhat.com>

Signed-off-by: NJán Tomko <jtomko@redhat.com>
Reviewed-by: NPavel Mores <pmores@redhat.com>
Reviewed-by: NMichal Privoznik <mprivozn@redhat.com>

This setting can be updating very easily on an already active
interface by just changing it in sysfs. If the bridge used for
connection is also changed, there is no need to separately update it,
because the new setting isf done as a part of connecting to the bridge
anyway.
Signed-off-by: NLaine Stump <laine@redhat.com>
Reviewed-by: NJán Tomko <jtomko@redhat.com>

This patch pushes the isolatedPort setting from the <interface> down
all the way to the callers of virNetDevBridgeAddPort(), and sets
BR_ISOLATED on the port (using virNetDevBridgePortSetIsolated()) after
the port has been successfully added to the bridge.
Signed-off-by: NLaine Stump <laine@redhat.com>
Signed-off-by: NLaine Stump <laine@redhat.com>
Reviewed-by: NJán Tomko <jtomko@redhat.com>

Not only was the original error code destroyed in the case of
encountering an error during recovery from a failed attach to the
bridge (and then *that* error was destroyed by logging a *second*
error about the failure to recover - virNetDevBridgeAddPort() already
logs an error, so the one about failing to recover was redundant), but
if the recovery was successful, the function would then return success
to the caller even though it had failed.

Fixes: 2711ac87
(overwritten errors were introduced along with this functionality)
Fixes: 6bde0a1a
(the wrong return value was introduced by a refactor)
Signed-off-by: NLaine Stump <laine@redhat.com>
Reviewed-by: NJán Tomko <jtomko@redhat.com>

My original implementation was completely broken because it attempted to
use object-add/del instead of blockdev-add/del.

https://bugzilla.redhat.com/show_bug.cgi?id=1798366Signed-off-by: NPeter Krempa <pkrempa@redhat.com>
Reviewed-by: NJán Tomko <jtomko@redhat.com>
Reviewed-by: NDaniel Henrique Barboza <danielhb413@gmail.com>

Starting on commit 1f433932, qemuDomainFillDeviceIsolationGroup()
returns 0 in all circunstances. Let's turn it to 'void' make it
clearer that the function will not fail. This also spares a
check for < 0 return in qemu_hotplug.c. The
qemuDomainFillDeviceIsolationGroupIter() callback now returns
0 at all times - which is already happening anyway.

Refer to 1f433932 commit message for more details on why
the function was changed to never return an error.
Signed-off-by: NDaniel Henrique Barboza <danielhb413@gmail.com>
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>
Reviewed-by: NMichal Privoznik <mprivozn@redhat.com>

Also add a space after the start in some of the cases.
Signed-off-by: NJán Tomko <jtomko@redhat.com>
Reviewed-by: NPeter Krempa <pkrempa@redhat.com>

Currently it is possible to start a domain which have disks
in same iotune group and at the same time having different iotune
params. Both params set are passed to qemu in command line and the one
that is passed later down command line is get actually set.
Let's prohibit such configurations.
Signed-off-by: NNikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>
Reviewed-by: NDaniel P. Berrangé <berrange@redhat.com>
Reviewed-by: NMichal Privoznik <mprivozn@redhat.com>

Remove unneeded, easy to remove goto labels (cleanup|error|done|...).
Signed-off-by: NDaniel Henrique Barboza <danielhb413@gmail.com>
Reviewed-by: NErik Skultety <eskultet@redhat.com>

With NVMe disks, one can start a blockjob with a NVMe disk
that is not visible in domain XML (at least right away). Usually,
it's fairly easy to override this limitation of
qemuDomainGetMemLockLimitBytes() - for instance for hostdevs we
temporarily add the device to domain def, let the function
calculate the limit and then remove the device. But it's not so
easy with virStorageSourcePtr - in some cases they don't
necessarily are attached to a disk. And even if they are it's
done later in the process and frankly, I find it too complicated
to be able to use the simple trick we use with hostdevs.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>
Reviewed-by: NCole Robinson <crobinso@redhat.com>

At the very beginning of the attach function the
qemuDomainStorageSourceChainAccessAllow() is called which
modifies CGroups, locks and seclabels for new disk and its
backing chain. This must be followed by a counterpart which
reverts back all the changes if something goes wrong. This boils
down to calling qemuDomainStorageSourceChainAccessRevoke() which
is done under 'error' label. But not all failure branches jump
there. They just jump onto 'cleanup' label where no revoke is
done. Such mistake is easy to do because 'cleanup' label does
exist. Therefore, dissolve 'error' block in 'cleanup' and have
everything jump onto 'cleanup' label.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>
Reviewed-by: NCole Robinson <crobinso@redhat.com>

Reviewed-by: NMichal Privoznik <mprivozn@redhat.com>
Signed-off-by: NDaniel P. Berrangé <berrange@redhat.com>

Our normal practice is for the object type to be the name prefix, and
the object instance be the first parameter passed in.

Rename these to virDomainObjSave and virDomainDefSave moving their
primary parameter to be the first one. Ensure that the xml options
are passed into both functions in prep for future work.

Finally enforce checking of the return type and mark all parameters
as non-NULL.
Reviewed-by: NMichal Privoznik <mprivozn@redhat.com>
Signed-off-by: NDaniel P. Berrangé <berrange@redhat.com>

This also isn't required (due to the vportprofile being stored in the
NetDef as a pointer rather than being directly contained), but it
seemed dishonest to not mark it as const (and thus permit users to
modify its contents)
Signed-off-by: NLaine Stump <laine@redhat.com>
Reviewed-by: NCole Robinson <crobinso@redhat.com>

In this case, the virNetDevBandwidthPtr that is returned is not to a
region within the virDomainNetDef arg, but points elsewhere (the
NetDef has the pointer, not the entire object), so technically it's
not necessary to make the return value a const, but it's a bit
disingenuous to *not* do it.
Signed-off-by: NLaine Stump <laine@redhat.com>
Reviewed-by: NCole Robinson <crobinso@redhat.com>

Signed-off-by: NDaniel Henrique Barboza <danielhb413@gmail.com>
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>
Reviewed-by: NMichal Privoznik <mprivozn@redhat.com>

Reviewed-by: NCole Robinson <crobinso@redhat.com>
Signed-off-by: NDaniel Henrique Barboza <danielhb413@gmail.com>

Reviewed-by: NCole Robinson <crobinso@redhat.com>
Signed-off-by: NDaniel Henrique Barboza <danielhb413@gmail.com>

The current 'setvcpus' timeout message requires a deeper
understanding of QEMU/Libvirt internals to proper react to it.
One who knows how setvcpus unplug work (it is an asynchronous
operation between QEMU and guest that Libvirt can't know for
sure if it failed, unless an explicit error happened during the
timeout period) will read the message and not assume a failed
operation. But the regular user, most often than not, will read
it and believe that the unplug operation failed.

This leads to situations where the user isn't exactly relieved
when accessing the guest and seeing that the unplug operation
worked. Instead, the user feel mislead by the timeout message
setvcpus threw.

Changing the timeout message to let the user know that the
unplug status is not known, and manual inspection in the guest
is required, is not a silver bullet. But it gives a more
realistic expectation of what happened, as best as we can tell
from Libvirt side anyways.
Reviewed-by: NCole Robinson <crobinso@redhat.com>
Signed-off-by: NDaniel Henrique Barboza <danielhb413@gmail.com>

qemu_hotplugpriv.h is a header file created to share a global variable
called 'qemuDomainRemoveDeviceWaitTime', declared in qemu_hotplug.c,
to other files that would want to change the timeout value
(currently, only tests/qemuhotplugtest.c).

Previous patch deprecated the variable, using qemu_driver->unplugTimeout
to set the timeout instead. This means that the header file is now
unused, and can be safely discarded.
Reviewed-by: NCole Robinson <crobinso@redhat.com>
Signed-off-by: NDaniel Henrique Barboza <danielhb413@gmail.com>

For some architectures and setups, device removal can take
longer than the default 5 seconds. This results in commands
such as 'virsh setvcpus' to fire timeout messages even if
the operation were successful in the guest, confusing the
user.

This patch sets a new 10 seconds unplug timeout for PPC64
guests. All other archs will keep the default 5 seconds
timeout.

Instead of putting 'if PPC64' conditionals inside qemu_hotplug.c
to set the new timeout value, a new function called
qemuDomainGetUnplugTimeout was added. The timeout value is then
retrieved when needed, by passing the correspondent DomainDef
object. This approach allows for different guest architectures
to have distint unplug timeout intervals, regardless of the
host architecture. This design also makes it easier to
modify/enhance the unplug timeout logic in the future
(allow for special timeouts for TCG domains, for example).

A new mock file was created to work with qemuhotplugtest.c,
given that the test timeout is significantly shorter than
the actual timeout value in qemu_hotplug.c.

The now unused 'qemuDomainRemoveDeviceWaitTime' global can't
be simply erased from qemu_hotplug.c though. Next patch will
remove it properly.
Reviewed-by: NCole Robinson <crobinso@redhat.com>
Suggested-by: NCole Robinson <crobinso@redhat.com>
Signed-off-by: NDaniel Henrique Barboza <danielhb413@gmail.com>

When attaching a mediated host device of model vfio-ccw without
specifying a guest-address, none is generated by libvirt. Let's fix this
and make sure to generate a device address during live-hotplug.
Reviewed-by: NBoris Fiuczynski <fiuczy@linux.ibm.com>
Signed-off-by: NBjoern Walk <bwalk@linux.ibm.com>
Signed-off-by: NJán Tomko <jtomko@redhat.com>
Reviewed-by: NJán Tomko <jtomko@redhat.com>

Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>
Reviewed-by: NDaniel Henrique Barboza <danielhb413@gmail.com>

Commit 01ca4010 (libvirt v5.1.0) moved address reservation for
hotplugged interface devices up to an earlier point in
qemuDomainAttachNetDevice(), because that function calls
qemuDomainSupportsNicdev() (in the case of
VIR_DOMAIN_NET_TYPE_VHOSTUSER), and qemuDomainSupportsNicdev() needs
to know the address type (for ARM machinetypes) and returns incorrect
results when the address type is "none".

This bugfix unfortunately caused a regression, because it also made PCI
address reservation happen before we noticed that the device was a
*hostdev* interface. Those interfaces are hotplugged by just calling
out to qemuDomainAttachHostdevDevice() - that function would then also
attempt to reserve the *same PCI address* that had just been reserved
in qemuDomainAttachNetDevice().

The solution is to move the bit of code that short-circuits out to
virDomainHostdevAttach() up *even earlier* so that no PCI address has
been allocated by the time it's called.

https://bugzilla.redhat.com/show_bug.cgi?id=1744523Signed-off-by: NLaine Stump <laine@redhat.com>
Reviewed-by: NAndrea Bolognani <abologna@redhat.com>

In both replaced cases we have other code that verifies that the bus
can't be changed or that the target is unique, so limiting the search to
disks with same bus makes no sense.
Signed-off-by: NPeter Krempa <pkrempa@redhat.com>
Reviewed-by: NDaniel Henrique Barboza <danielhb413@gmail.com>

In some places we need to check if a hostdev has VFIO backend.
Because of how complicated virDomainHostdevDef structure is, the
check consists of three lines. Move them to a function and
replace all checks with the function call.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>
Reviewed-by: NCole Robinson <crobinso@redhat.com>

Replace all occurrences of
  if (VIR_STRDUP(a, b) < 0)
     /* effectively dead code */
with:
  a = g_strdup(b);
Signed-off-by: NJán Tomko <jtomko@redhat.com>
Reviewed-by: NMichal Privoznik <mprivozn@redhat.com>

Replace:
  if (!s && VIR_STRDUP(s, str) < 0)
    goto;
with:
  if (!s)
    s = g_strdup(str);
Signed-off-by: NJán Tomko <jtomko@redhat.com>
Reviewed-by: NMichal Privoznik <mprivozn@redhat.com>

Signed-off-by: NJán Tomko <jtomko@redhat.com>
Reviewed-by: NMichal Privoznik <mprivozn@redhat.com>

Now that all the types using VIR_AUTOUNREF have a cleanup func defined
to virObjectUnref, use g_autoptr instead of VIR_AUTOUNREF.
Signed-off-by: NJán Tomko <jtomko@redhat.com>
Reviewed-by: NMichal Privoznik <mprivozn@redhat.com>

Since commit 44e7f029
    util: rewrite auto cleanup macros to use glib's equivalent

VIR_AUTOPTR aliases to g_autoptr. Replace all of its use by the GLib
macro version.
Signed-off-by: NJán Tomko <jtomko@redhat.com>
Reviewed-by: NMichal Privoznik <mprivozn@redhat.com>

Since commit 44e7f029
    util: rewrite auto cleanup macros to use glib's equivalent

VIR_AUTOFREE is just an alias for g_autofree. Use the GLib macros
directly instead of our custom aliases.
Signed-off-by: NJán Tomko <jtomko@redhat.com>
Reviewed-by: NMichal Privoznik <mprivozn@redhat.com>

Also define the macro for building with GLib older than 2.60
Signed-off-by: NJán Tomko <jtomko@redhat.com>
Reviewed-by: NMichal Privoznik <mprivozn@redhat.com>

Use G_GNUC_UNUSED from GLib instead of ATTRIBUTE_UNUSED.
Signed-off-by: NJán Tomko <jtomko@redhat.com>
Reviewed-by: NMichal Privoznik <mprivozn@redhat.com>

Cleanup labels are also dropped where possible.
Signed-off-by: NDaniel Henrique Barboza <danielhb413@gmail.com>
Reviewed-by: NErik Skultety <eskultet@redhat.com>