Currently, the functions return a pointer to the
destination buffer on success or NULL on failure.

Not only does this kind of error handling look quite
alien in the context of libvirt, where most functions
return zero on success and a negative int on failure,
but it's also somewhat pointless because unless there's
been a failure the returned pointer will be the same
one passed in by the user, thus offering no additional
value.

Change the functions so that they return an int
instead.
Signed-off-by: NAndrea Bolognani <abologna@redhat.com>

Replace instances where we previously called virGetLastError just to
either get the code or to check if an error exists with
virGetLastErrorCode to avoid a validity pre-check.
Signed-off-by: NRamy Elkest <ramyelkest@gmail.com>
Reviewed-by: NErik Skultety <eskultet@redhat.com>

When the agent code was first introduced back in

  commit c160ce33
  Author: Daniel P. Berrange <berrange@redhat.com>
  Date:   Wed Oct 5 18:31:54 2011 +0100

    QEMU guest agent support

there was code that would loop and retry the connection when opening
the agent socket. At this time, the only thing done in between the
opening of the monitor socket & opening of the agent socket was a
call to set the monitor capabilities. This was a no-op on non-QMP
versions, so in theory there could be a race which let us connect
to the monitor while the agent socket was still not created by QEMU.

In the modern world, however, we long ago mandated the use of QMP
for managing QEMU, so we're guaranteed to have a set capabilities
QMP call. Once we've seen a reply to this, we're guaranteed that
QEMU has fully initialized all backends and is in its event loop.

We can thus be sure the QEMU agent socket is present and don't need
to retry connections to it, even without having the chardev FD passing
feature.
Reviewed-by: NJohn Ferlan <jferlan@redhat.com>
Signed-off-by: NDaniel P. Berrangé <berrange@redhat.com>

Rather than have virJSONValueArraySize return a -1 when the input
is not an array and then splat an error message, let's check for
an array before calling and then change the return to be a size_t
instead of ssize_t.

That means using the helper virJSONValueIsArray as well as using a
more generic error message such as "Malformed <something> array".
In some cases we can remove stack variables and when we cannot,
those variables should be size_t not ssize_t. Alter a few references
of if (!value) to be if (value == 0) instead as well.

Some callers can already assume an array is being worked on based
on the previous call, so there's less to do.
Signed-off-by: NJohn Ferlan <jferlan@redhat.com>
Reviewed-by: NJán Tomko <jtomko@redhat.com>

Signed-off-by: NMartin Kletzander <mkletzan@redhat.com>
Reviewed-by: NJán Tomko <jtomko@redhat.com>

So far we are repeating the following lines over and over:

  if (!(virSomeObjectClass = virClassNew(virClassForObject(),
                             "virSomeObject",
                             sizeof(virSomeObject),
                             virSomeObjectDispose)))
      return -1;

While this works, it is impossible to do some checking. Firstly,
the class name (the 2nd argument) doesn't match the name in the
code in all cases (the 3rd argument). Secondly, the current style
is needlessly verbose. This commit turns example into following:

  if (!(VIR_CLASS_NEW(virSomeObject,
                      virClassForObject)))
      return -1;
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>
Reviewed-by: NDaniel P. Berrangé <berrange@redhat.com>

Signed-off-by: NPeter Krempa <pkrempa@redhat.com>

Use virJSONValueObjectGetArray instead of virJSONValueObjectGet so that
it's not necessary to check whether it's an array.
Signed-off-by: NPeter Krempa <pkrempa@redhat.com>

It was not possible to determine whether virJSONValueObjectAddVArgs and
the functions using it would consume a virJSONValue or not when used
with the 'a' or 'A' modifier depending on when the loop failed.

Fix this by passing in a pointer to the pointer so that it can be
cleared once it's successfully consumed and the callers don't have to
second-guess leaving a chance of leaking or double freeing the value
depending on the ordering.

Fix all callers to pass a double pointer too.
Signed-off-by: NPeter Krempa <pkrempa@redhat.com>

We read from the agent until seeing a \r\n pair to indicate a completed
reply or event. To avoid memory denial-of-service though, we must have a
size limit on amount of data we buffer. 10 MB is large enough that it
ought to cope with normal agent replies, and small enough that we're not
consuming unreasonable mem.

This is identical to the flaw we had reading from the QEMU monitor
as CVE-2018-5748, so rather embarrassing that we forgot to fix
the agent code at the same time.
Signed-off-by: NDaniel P. Berrangé <berrange@redhat.com>

We forgot to free alloced mem when failed to
dup ifname or macaddr.

Also use VIR_STEAL_PTR to simplify codes.
Signed-off-by: NChen Hanxiao <chenhanxiao@gmail.com>
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>

qemuAgentNotifyEvent accesses monitor structure and is called on qemu
reset/shutdown/suspend events under domain lock. Other monitor
functions on the other hand take monitor lock and don't hold domain lock.
Thus it is possible to have risky simultaneous access to the structure
from 2 threads. Let's take monitor lock here to make access exclusive.

In case of 0 filesystems *info is not set while according
to virDomainGetFSInfo contract user should call free on it even
in case of 0 filesystems. Thus we need to properly set
it. NULL will be enough as free eats NULLs ok.

We have couple of functions that operate over NULL terminated
lits of strings. However, our naming sucks:

virStringJoin
virStringFreeList
virStringFreeListCount
virStringArrayHasString
virStringGetFirstWithPrefix

We can do better:

virStringListJoin
virStringListFree
virStringListFreeCount
virStringListHasString
virStringListGetFirstWithPrefix
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>

We can receive NULL as sync reply in two situations. First
is garbage sync reply and this situation is handled by
resending sync message. Second is different cases
of rebooting guest, destroing domain etc and we can
give more meaningful error message. Actually we have
this error message in qemuAgentCommand already which checks
for the same sitatuion. AFAIK case with mon->running
is just to be safe on adding some future(?) cases of
returning NULL reply.

We can easily handle receiving garbage on sync. We don't
have to make client deal with this situation. We just
need to resend sync command but this time garbage is
not be possible.

When we wait for sync reply we can receive delayed
reply to syncs or commands that were sent erlier. We can
safely skip them until we receive sync reply with correct id.
There is no much sense report this situation to client.
Actually with a bit of "luck" if we involve client into
this the play can go on forever: send sync 0, receive
sync reply -1, send sync 1, receive reply 0 ...

After sync is sent we can receive garbare and this is not error.
Consider next regular case:

1. libvirtd sends sync
2. qga sends partial sync reply and die
3. libvirtd sends sync
4. qga sends sync reply
5. libvirtd receives garbage
  (half of first reply and second reply together)

We should handle this situation as it is recoverable.
Next sync can succeed. Let's report reply is NULL,
it will be converted to the VIR_ERR_AGENT_UNSYNCED
which signals client to retry.

Errors in qemuAgentIOProcessLine stop agent IO processing just
like any regular IO error, however some of current errors
that this functions spawns are false positives. Consider
next case for example:

1. send sync (unsynced state)
2. receive sync reply (sync established)
3. command send, but timeout occured (unsynced state)
4. receive command reply

Last IO triggers error because current code ignores
only delayed syncs when unsynced

We should not treat any delayed reply as error in unsynced
state. Until client and qga are not in sync delayed reply to any
command is possible. msg == NULL is the exact criterion
that we are not in sync.

Both qemu monitor and agent print the same
log on HUANGUP event, which would be confusing
when reading libvirtd log.

This patch will give a different log message to them.
Signed-off-by: NChen Hanxiao <chenhanxiao@gmail.com>
Reviewed-by: NMichal Privoznik <mprivozn@redhat.com>

A separate error code will help recognize real failures from
necessity to try again
Signed-off-by: NMaxim Nestratov <mnestratov@virtuozzo.com>

Documentation for the "guest-set-vcpus" command describes a proper
algorithm how to set vcpus. This patch makes the following changes:

- state of cpus that has not changed is not updated
- if the command was partially successful the command is re-tried with
  the rest of the arguments to get a proper error message
- code is more robust against malicious guest agent
- fix testsuite to the new semantics

Add a new helper that sanitizes error semantics of base64_encode_alloc.

We had both and the only difference was that the latter also included
information about multifunction setting.  The problem with that was that
we couldn't use functions made for only one of the structs (e.g.
parsing).  To consolidate those two structs, use the one in virpci.h,
include that in domain_conf.h and add the multifunction member in it.
Signed-off-by: NMartin Kletzander <mkletzan@redhat.com>

In a few places in libvirt we busy-wait for events, for example qemu
creating a monitor socket.  This is problematic because:

 - We need to choose a sufficiently small polling period so that
   libvirt doesn't add unnecessary delays.

 - We need to choose a sufficiently large polling period so that
   the effect of busy-waiting doesn't affect the system.

The solution to this conflict is to use an exponential backoff.

This patch adds two functions to hide the details, and modifies a few
places where we currently busy-wait.
Signed-off-by: NRichard W.M. Jones <rjones@redhat.com>

Replace the nonsensical debug statement by adding the expected event
code into the existing debug statement.

Since the monitor code always notifies the agent on guest
reboot/shutdown even if that was not initiated by the agent the warning
emitted later is bogus and pollutes the logs in such cases. Delete it
and keep just the original debug message where this info can be
inferred.

We have this function qemuAgentNotifyEvent() which is supposed to
be called from thread pool responsible for processing qemu
monitor events. The function then should wake up other thread
that is waiting for a guest to shutdown or reboot. However, if we
have received a different error a warning is printed out. This
warning lacks info on which event is expected.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>

While this is no functional change, whole channel definition is
going to be needed very soon. Moreover, while touching this obey
const correctness rule in qemuAgentOpen() - so far it was passed
regular pointer to channel config even though the function is
expected to not change pointee at all. Pass const pointer
instead.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>

The internal representation of a JSON array counts the items in
size_t. However, for some reason, when asking for the count it's
reported as int. Firstly, we need the function to return a signed
type as it's returning -1 on an error. But, not every system has
integer the same size as size_t. Therefore, lets return ssize_t.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>

While CPU0 was made unpluggable in Linux a while ago it's not desirable
to unplug it since some parts of the kernel (suspend-to-ram) still
depend on it.

This patch fixes the vCPU selection code in libvirt so that it will not
be disabled.

Base-64 encode the password and pass it to the guest agent
via the 'guest-set-user-password' command.

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

Most virDomainDiskIndexByName callers do not care about the index; what
they really want is a disk def pointer.
Signed-off-by: NJiri Denemark <jdenemar@redhat.com>

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

So, imagine you've issued an API that involves guest agent. For
instance, you want to query guest's IP addresses. So the API acquires
QUERY_JOB, locks the guest agent and issues the agent command.
However, for some reason, guest agent replies to initial ping
correctly, but then crashes tragically while executing real command
(in this case guest-network-get-interfaces). Since initial ping went
well, libvirt thinks guest agent is accessible and awaits reply to the
real command. But it will never come. What will is a monitor event.
Our handler (processSerialChangedEvent) will try to acquire
MODIFY_JOB, which will fail obviously because the other thread that's
executing the API already holds a job. So the event handler exits
early, and the QUERY_JOB is never released nor ended.

The way how to solve this is to put flag somewhere in the monitor
internals. The flag is called @running and agent commands are issued
iff the flag is set. The flag itself is set when we connect to the
agent socket. And unset whenever we see DISCONNECT event from the
agent. Moreover, we must wake up all the threads waiting for the
agent. This is done by signalizing the condition they're waiting on.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>

Since the code is now separated into the common helper, we can reuse it
instead of maintaining two copies.

When we shutdown/reboot a guest using agent-mode, if the guest itself blocks infinitely,
libvirt would block in qemuAgentShutdown() forever.
Thus, we set a timeout for shutdown/reboot, from our experience, 60 seconds would be fine.
Signed-off-by: NZhang Bo <oscar.zhangbo@huawei.com>
Signed-off-by: NWang Yufei <james.wangyufei@huawei.com>

Now that we allow HW address to be not present on our RPC layer,
don't error out if qemu-ga hasn't provided any.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>

By querying the qemu guest agent with the QMP command
"guest-network-get-interfaces" and converting the received JSON
output to structured objects.

Although "ifconfig" is deprecated, IP aliases created by "ifconfig"
are supported by this API. The legacy syntax of an IP alias is:
"<ifname>:<alias-name>". Since we want all aliases to be clubbed
under parent interface, simply stripping ":<alias-name>" suffices.
Note that IP aliases formed by "ip" aren't visible to "ifconfig",
and aliases created by "ip" do not have any specific name. But
we are lucky, as qemu guest agent detects aliases created by both.

src/qemu/qemu_agent.h:
  * Define qemuAgentGetInterfaces

src/qemu/qemu_agent.c:
  * Implement qemuAgentGetInterface

src/qemu/qemu_driver.c:
  * New function qemuGetDHCPInterfaces
  * New function qemuDomainInterfaceAddresses

src/remote_protocol-sructs:
  * Define new structs

tests/qemuagenttest.c:
  * Add new test: testQemuAgentGetInterfaces
    Test cases for IP aliases, 0 or multiple ipv4/ipv6 address(es)
Signed-off-by: NNehal J Wani <nehaljw.kkd1@gmail.com>

Get mounted filesystems list, which contains hardware info of disks and its
controllers, from QEMU guest agent 2.2+. Then, convert the hardware info
to corresponding device aliases for the disks.
Signed-off-by: NTomoki Sekiyama <tomoki.sekiyama@hds.com>

Signed-off-by: NMartin Kletzander <mkletzan@redhat.com>