There's a single user for it which takes an existing
virPCIDeviceAddress, passes its various bits to the
function which in turn constructs a virPCIDevice and
then copies the string representation for the caller
to use: we can use virPCIDeviceAddressAsString()
instead and avoid creating the virPCIDevice in the
first place. Since the function ends up having no
users after the change, we can just drop it.
Signed-off-by: NAndrea Bolognani <abologna@redhat.com>
Reviewed-by: NMartin Kletzander <mkletzan@redhat.com>

The struct is called virPCIDeviceAddress and the
functions operating on it should be named accordingly.
Signed-off-by: NAndrea Bolognani <abologna@redhat.com>
Reviewed-by: NMartin Kletzander <mkletzan@redhat.com>

It's a better fit than conf/domain_conf.
Signed-off-by: NAndrea Bolognani <abologna@redhat.com>
Reviewed-by: NMartin Kletzander <mkletzan@redhat.com>

This will help in future expansions of the code when it is be
harder to track if @newName and/or @newLockspace were already
allocated or not and thus whether it is safe to 'return' or we
need to 'goto error'. By using the 'cleanup' label those two
cases merge into a single one.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>
Reviewed-by: NJohn Ferlan <jferlan@redhat.com>

If drvNew callback fails, nobody calls drvFree and thus private
data of the driver might leak.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>
Reviewed-by: NJohn Ferlan <jferlan@redhat.com>

Currently, there are only two types of resource. So effectively
this is a dead code. However, that assumption can change and we
shouldn't just silently ignore the error.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>
Reviewed-by: NJohn Ferlan <jferlan@redhat.com>

The if() is completely useless since args.path is set to NULL in
the line just above.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>
Reviewed-by: NJohn Ferlan <jferlan@redhat.com>

On daemon deinit only fileLockSpaceDir is freed. The other two
(scsiLockSpaceDir and lvmLockSpaceDir) are missing even though
they are allocated in virLockManagerLockDaemonLoadConfig().
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>
Reviewed-by: NJohn Ferlan <jferlan@redhat.com>

28 bytes in 1 blocks are definitely lost in loss record 26 of 66
   at 0x4C2CF0F: malloc (vg_replace_malloc.c:299)
   by 0x7A02719: strdup (strdup.c:42)
   by 0x197DC1: virStrdup (virstring.c:961)
   by 0x12B478: virLockDaemonConfigFilePath (lock_daemon_config.c:44)
   by 0x12A759: main (lock_daemon.c:1270)

62 (32 direct, 30 indirect) bytes in 1 blocks are definitely lost in loss record 41 of 66
   at 0x4C2EF26: calloc (vg_replace_malloc.c:711)
   by 0x151B61: virAlloc (viralloc.c:144)
   by 0x12B56C: virLockDaemonConfigNew (lock_daemon_config.c:71)
   by 0x12A491: main (lock_daemon.c:1262)

13 bytes in 1 blocks are definitely lost in loss record 21 of 70
   at 0x4C2CF0F: malloc (vg_replace_malloc.c:299)
   by 0x7A02719: strdup (strdup.c:42)
   by 0x197E3F: virStrdup (virstring.c:961)
   by 0x12C86B: virLockSpaceProtocolDispatchRegister (lock_daemon_dispatch.c:291)
   by 0x12BB73: virLockSpaceProtocolDispatchRegisterHelper (lock_daemon_dispatch_stubs.h:152)
   by 0x1336AA: virNetServerProgramDispatchCall (virnetserverprogram.c:437)
   by 0x13320D: virNetServerProgramDispatch (virnetserverprogram.c:304)
   by 0x139E3E: virNetServerProcessMsg (virnetserver.c:144)
   by 0x13A1A2: virNetServerDispatchNewMessage (virnetserver.c:230)
   by 0x1350F5: virNetServerClientDispatchMessage (virnetserverclient.c:343)
   by 0x137680: virNetServerClientDispatchEvent (virnetserverclient.c:1498)
   by 0x147704: virNetSocketEventHandle (virnetsocket.c:2140)
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>
Reviewed-by: NJohn Ferlan <jferlan@redhat.com>

The virSecurityStackAddNested() can fail in which case
virSecurityManagerNewStack() should fail too.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>
Reviewed-by: NJohn Ferlan <jferlan@redhat.com>

Use 'error' label to free allocated memory.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>
Reviewed-by: NJohn Ferlan <jferlan@redhat.com>

The corresponding structs are declared there.
Signed-off-by: NAndrea Bolognani <abologna@redhat.com>
Reviewed-by: NJán Tomko <jtomko@redhat.com>

These are simple predicates, which makes bool a more
appropriate return type than int.
Signed-off-by: NAndrea Bolognani <abologna@redhat.com>
Reviewed-by: NJán Tomko <jtomko@redhat.com>

The function is called on a virDomainDeviceInfo, so it
should be declared along with it.

Moving this function requires moving and making public
virDomainDeviceCCWAddressIsValid() as well, but that's
perfectly fine since the same reasoning above also
applies to it, due to virDomainDeviceCCWAddress being
(correctly) declared in device_conf.
Signed-off-by: NAndrea Bolognani <abologna@redhat.com>
Reviewed-by: NJán Tomko <jtomko@redhat.com>

It's used in virDomainDeviceInfo, which makes
domain_conf the wrong place to declare it.
Signed-off-by: NAndrea Bolognani <abologna@redhat.com>
Reviewed-by: NJán Tomko <jtomko@redhat.com>

https://bugzilla.redhat.com/show_bug.cgi?id=1621910

When users want to update a path to a CDROM they tend to
construct a very minimal XML and feed the API with it. This is
not a good practice as it breaks the assumptions the API is built
on. Most notably, leaving an element out should be treated as a
request for removal of the corresponding setting. Just like
leaving out <bandwidth/> clears out any QoS previously set.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>
Reviewed-by: NErik Skultety <eskultet@redhat.com>

Commit f7b55665 added 'save_error' even though the function
already has 'originalError' used in the 'try_remove' section.
Signed-off-by: NJán Tomko <jtomko@redhat.com>
Reviewed-by: NJohn Ferlan <jferlan@redhat.com>

As advertised in previous commit, there are three APIs that might
run for quite some time (because they read/write data from/to a
volume) and these three are: downloadVol, uploadVol, wipeVol.
Release pool object lock and reacquire it later to allow more
concurrency.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>
Reviewed-by: NJohn Ferlan <jferlan@redhat.com>

There are few operations in the storage driver that read/write
data onto volumes. Such operations can take very long time to
finish. During that time the storage pool object is locked which
has bad performance impacts (other threads can't fetch its XML
for instance). This commit prepares the storage driver for
releasing the lock during those operations (downloadVol,
uploadVol, wipeVol).
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>
Reviewed-by: NJohn Ferlan <jferlan@redhat.com>

Just like a few commits earlier, checking for pool source
duplicates and unlocking pools list afterwards is a buggy
pattern. The check must go into virStoragePoolObjAssignDef.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>
Reviewed-by: NJohn Ferlan <jferlan@redhat.com>

The @conn argument is needed only to do some source matching in
case of iSCSI source. Anyway, it's used just for node device
driver and as such can be replaced with virGetConnectNodeDev().

At the same time, the @conn struct member is dropped from
_virStoragePoolObjFindDuplicateData.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>
Reviewed-by: NJohn Ferlan <jferlan@redhat.com>

This function is going to be made static in used in
virStoragePoolObjAssignDef(). Therefore move it and all the
static functions it calls a few lines up.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>
Reviewed-by: NJohn Ferlan <jferlan@redhat.com>

Even though we do some checking it is not as thorough as it
should be. We already have virStoragePoolObjIsDuplicate but the
way we use it is a typical TOCTOU. Imagine two threads trying to
define two pools with the same name but different UUIDs. With the
current code neither of them finds a duplicate and thus proceed
to virStoragePoolObjAssignDef where only names are compared.
Therefore both threads succeed which is obviously wrong.

We should check for duplicates where we care for them.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>
Reviewed-by: NJohn Ferlan <jferlan@redhat.com>

This function is going to be made static in used in
virStoragePoolObjAssignDef(). Therefore move it a few lines up.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>
Reviewed-by: NJohn Ferlan <jferlan@redhat.com>

In two places the passed pool object argument is marked as
ATTRIBUTE_UNUSED even though it's used right away.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>
Reviewed-by: NJohn Ferlan <jferlan@redhat.com>

https://bugzilla.redhat.com/show_bug.cgi?id=1623157

Changing MTU on a running guest is not possible and trying to do
so made us face many problems. That's why we forbid it in
5f44d7e3. However, there is still one possible path where
users can sneak in change: migration XML.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>
Reviewed-by: NDaniel P. Berrangé <berrange@redhat.com>

https://bugzilla.redhat.com/show_bug.cgi?id=1621910

When introducing this check back in 4ad54a41 my mindset was
that if an element is missing in update XML then user is
requesting for removal of the corresponding setting. For
instance, if <bandwidth/> is not present in update XML any QoS
previously set on <interface/> is cleared out. Well this
assumption is correct but only to some extent.

Turns out, we have some users who when updating path to ISO
image construct very minimalistic disk XML and pass it to device
update API. Such XML is lacking a lot of information, and alias
is one of them. This triggers error in
virDomainDefCompatibleDevice() because we think that user is
requesting to remove the alias. Well, they are not.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>
Reviewed-by: NJohn Ferlan <jferlan@redhat.com>

The size/capacity stats gathered in qemuDomainBlocksStatsGather when
using -blockdev would be overwritten by assigning/copying the transfered
data statistics at the end. Fix it by moving the assignment prior to
fetching the capacity data.
Signed-off-by: NPeter Krempa <pkrempa@redhat.com>
Reviewed-by: NJán Tomko <jtomko@redhat.com>

S390 is aware of both vfio-pci and vfio-ccw devices, so
on S390 the capability QEMU_CAPS_VFIO_PCI_DISPLAY will be
available. Add an extra check to make sure we only set the
display to off for vfio-pci mediated devices. Otherwise we
add display for vfio-ccw device and this breaks vfio-ccw
device qemu command line.

Fixes: d54e45b6 conf: Introduce new <hostdev> attribute 'display'
Signed-off-by: NFarhan Ali <alifm@linux.ibm.com>
Reviewed-by: NMarc Hartmayer <mhartmay@linux.ibm.com>
Reviewed-by: NErik Skultety <eskultet@redhat.com>

Commit 6534b3c4 tried to raise an error when there is no numa
nodes by setting access='shared' in the domain config, but added
a helper called from qemuDomainDeviceDefValidate instead of a
helper called from qemuDomainDefValidate for XML:

  <memoryBacking>
    <hugepages/>
    <access mode='shared'/>
  </memoryBacking>

Since there are no memory devices in the test XML, there would
be no validation failure, but the test added was still failing.
Investigating that it turns out that unnecessary XML elements
were causing the failure (no need for <video>, <graphics>,
<pm>, usb controller model "piix3-uhci", disk attribute for
"discard='unmap'", <serial>, <console>, <channel> and a
memballoon model). Removing all those before moving the method
caused the test to succeed.

So this patch moves the validation to the right place and
removes all the unnecessary XML pieces that were causing
a false validation failure.

https://bugzilla.redhat.com/show_bug.cgi?id=1448149#c14Signed-off-by: NLuyao Huang <lhuang@redhat.com>
Reviewed-by: NJohn Ferlan <jferlan@redhat.com>

If @vm has flagged as "to be removed" virDomainObjListFindByNameLocked
returns NULL (although the definition actually exists). Therefore, the
possibility exits that "virHashAddEntry" will raise the error
"Duplicate key" => virDomainObjListAddObjLocked fails =>
virDomainObjEndAPI(&vm) is called and this leads to a freeing of @def
since @def is already assigned to vm->def. But actually this leads to
a double free since the common usage pattern is that the caller of
virDomainObjListAdd(Locked) is responsible for freeing @def in case of
an error.

Let's fix this by setting vm->def to NULL in case of an error.

Backtrace:

   ➤  bt
   #0  virFree (ptrptr=0x7575757575757575)
   #1  0x000003ffb5b25b3e in virDomainResourceDefFree
   #2  0x000003ffb5b37c34 in virDomainDefFree
   #3  0x000003ff9123f734 in qemuDomainDefineXMLFlags
   #4  0x000003ff9123f7f4 in qemuDomainDefineXML
   #5  0x000003ffb5cd2c84 in virDomainDefineXML
   #6  0x000000011745aa82 in remoteDispatchDomainDefineXML
   ...
Reviewed-by: NBjoern Walk <bwalk@linux.ibm.com>
Signed-off-by: NMarc Hartmayer <mhartmay@linux.ibm.com>

We would have used virtio for networking anyway, but it's
better to be explicit; for graphics, none of the existing
models work right now but virtio is the only one which
has a non-PCI variant, so it's as good a default as any
Spotted-by: NJán Tomko <jtomko@redhat.com>
Signed-off-by: NAndrea Bolognani <abologna@redhat.com>
Reviewed-by: NJán Tomko <jtomko@redhat.com>

None of the existing models is suitable for use with
RISC-V virt guests, and we don't want information about
the serial console to be missing from the XML.

The name is based on comments in qemu/hw/riscv/virt.c:

  RISC-V machine with 16550a UART and VirtIO MMIO

and in qemu/hw/char/serial.c:

  QEMU 16550A UART emulation

along with the output of dmesg in the guest:

  Serial: 8250/16550 driver, 4 ports, IRQ sharing disabled
  10000000.uart: ttyS0 at MMIO 0x10000000 (irq = 13,
    base_baud= 230400) is a 16550A
Signed-off-by: NAndrea Bolognani <abologna@redhat.com>
Reviewed-by: NJán Tomko <jtomko@redhat.com>

The architecture is new enough that we don't need to
concern ourselves with backwards compatibility in any
capacity.
Signed-off-by: NAndrea Bolognani <abologna@redhat.com>
Reviewed-by: NJán Tomko <jtomko@redhat.com>

Signed-off-by: NLubomir Rintel <lkundrak@v3.sk>
Reviewed-by: NAndrea Bolognani <abologna@redhat.com>

Add a new modifier letter for virJSONValueObjectAddVArgs which will add
a boolean value with our tristate semantics. The value is omitted when
the _ABSENT value is used.
Signed-off-by: NPeter Krempa <pkrempa@redhat.com>
Reviewed-by: NErik Skultety <eskultet@redhat.com>

A generic "failed to parse xml document" message without telling us
which XML file failed is quite unhelpful.
Reviewed-by: NJiri Denemark <jdenemar@redhat.com>
Signed-off-by: NDaniel P. Berrangé <berrange@redhat.com>

Reviewed-by: NJiri Denemark <jdenemar@redhat.com>
Signed-off-by: NDaniel P. Berrangé <berrange@redhat.com>

Reviewed-by: NJiri Denemark <jdenemar@redhat.com>
Signed-off-by: NDaniel P. Berrangé <berrange@redhat.com>

In preparation for splitting up the CPU map data file, move it into a
dedicated directory of its own.
Reviewed-by: NJiri Denemark <jdenemar@redhat.com>
Signed-off-by: NDaniel P. Berrangé <berrange@redhat.com>