Rather than having an error path, let's rework the code to allocate
and fill into an @authdef variable and then steal that into @ret when
we are successful leaving just a cleanup: path.
Signed-off-by: NJohn Ferlan <jferlan@redhat.com>
Reviewed-by: NErik Skultety <eskultet@redhat.com>
Reviewed-by: NJán Tomko <jtomko@redhat.com>

Signed-off-by: NJohn Ferlan <jferlan@redhat.com>
Reviewed-by: NErik Skultety <eskultet@redhat.com>
Reviewed-by: NJán Tomko <jtomko@redhat.com>

Since commit a7424faf QMP is always used.

Also, commit 932534e8 removed the last use of this apart from:
* parsing/formatting this in the caps cache
* using it as a temporary variable to know when to report an error
Signed-off-by: NJán Tomko <jtomko@redhat.com>
Reviewed-by: NJiri Denemark <jdenemar@redhat.com>

Ever since the introduction of the guest-get-fsinfo command
in QEMU commit 46d4c572 qga/qapi-schema.json says that
the 'disks' array can possibly be empty. For example when getting
the target list is unsupported:
https://bugzilla.redhat.com/show_bug.cgi?id=1567041

Pass an empty string instead of NULL to vshTableRowAppend to prevent
a mismatched column number.
Signed-off-by: NJán Tomko <jtomko@redhat.com>

Add comma after every string and trim the final one.
Signed-off-by: NJán Tomko <jtomko@redhat.com>

Use vshPrintExtra to report this message. It is a human-readable
explanation rather than an error.

Also, it is a very special system that runs with no filesystems.
Signed-off-by: NJán Tomko <jtomko@redhat.com>

Failing to print the table is also a reason to return failure
and print the reported error.

Switch to the usual pattern where we fall through the cleanup
label right after setting ret to true instead of infering the
return value from the number of filesystems returned.
Signed-off-by: NJán Tomko <jtomko@redhat.com>

Initialize 'info' to prevent accessing random access memory.

Introduced by commit 3072ded3 released in 4.8.0.

https://bugzilla.redhat.com/show_bug.cgi?id=1676354Signed-off-by: NJán Tomko <jtomko@redhat.com>

Leave the 'ret' variable for the current function's return value.
Signed-off-by: NJán Tomko <jtomko@redhat.com>

Do not use 'ret' throughout the whole function to avoid confusion
and comparison of unsigned 'i' against signed 'ret'.
Signed-off-by: NJán Tomko <jtomko@redhat.com>

Trivially implement this by deleting the bogus check in
vshTableSafeEncode.

Now it returns an empty string for an empty string instead
of returning NULL without setting an error.
Signed-off-by: NJán Tomko <jtomko@redhat.com>

Indent them by four spaces from the previous line, instead of starting
at columnn zero.
Signed-off-by: NJán Tomko <jtomko@redhat.com>

The wireshark-2.4.0 is almost 2 years old now. Assuming anybody
interested in running latest libvirt doesn't run old wireshark,
it is safe to do this. It also simplifies the code.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>
Reviewed-by: NJán Tomko <jtomko@redhat.com>

Since wirshark-2.5.0 toplevel plugins are no longer loaded. Only
plugins from epan/, wiretap/ or codecs/ subdirs are. Update the
plugin dir we generate. This is safe to do even for older
wiresharks, since they load plugins from there too.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>
Reviewed-by: NJán Tomko <jtomko@redhat.com>

As advertised in previous commits, wireshark has changed the way
that plugins register. In fact, it has done so two times since
the last time we've touched our code (wireshark v2.5.0 and
v2.9.0). Use the wireshark script from respective releases to
generate newer registration callbacks and put them into our code.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>
Reviewed-by: NJán Tomko <jtomko@redhat.com>

In order to be able to dissect libvirt protocol the wireshark
plugin needs to be registered. So far this plugin registration
code was generated on every build using a script that was copied
over from wireshark's tools/ directory.

This is suboptimal, because the way that plugins register changes
across wireshark releases. Therefore, let's keep the generated
file in the git, put the command line used to generate the file
into a comment and remove the script.

This solution allows us to put different registration mechanism
into one file (under #ifdef-s) and thus compile with wider range
of wireshark releases.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>
Reviewed-by: NJán Tomko <jtomko@redhat.com>

Move the majority of the packet-libvirt.h content into
packet-libvirt.c and expose only register functions which are the
only ones that are not static.

The rationale behind is that packet-libvirt.h will be included
from packet.c and therefore the header file needs to be as clean
as possible.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>
Reviewed-by: NJán Tomko <jtomko@redhat.com>

Mock out libxlCapsHasPVUSB to always return true, so test results
aren't dependent on host libxl version
Reviewed-by: NJán Tomko <jtomko@redhat.com>
Signed-off-by: NCole Robinson <crobinso@redhat.com>

No functional change, but this will allow us to mock out the function
in the test suite
Reviewed-by: NJán Tomko <jtomko@redhat.com>
Signed-off-by: NCole Robinson <crobinso@redhat.com>

This allows us to mock functions in the libxl driver, like
is already possible for the qemu driver
Reviewed-by: NJán Tomko <jtomko@redhat.com>
Signed-off-by: NCole Robinson <crobinso@redhat.com>

Every other mock library is named ending in mock.c, move
virmocklibxl.c to follow that pattern
Reviewed-by: NJán Tomko <jtomko@redhat.com>
Signed-off-by: NCole Robinson <crobinso@redhat.com>

Commit fafcc818 changed the docs to say that when creating a
pool directory or file volume with no owner/group specified, they
will be inherited from the parent directory. This isn't correct
now and doesn't seem to have ever been correct

In reality default owner/group is whatever UID/GID libvirtd is
running as
Signed-off-by: NCole Robinson <crobinso@redhat.com>

If 2 threads call abort for example then one of them
will hang because client will send 2 abort messages and
server will reply only on first of them, the second will be
ignored. And on server reply client changes the state only
one of abort message to complete, the second will hang forever.
There are other similar issues.

We should complete all messages waiting reply if we got
error or expected abort/finish reply from server. Also if one
thread send finish and another abort one of them will win
the race and server will either abort or finish stream. If
stream is aborted then thread requested finishing should report
error. In order to archive this let's keep stream closing reason
in @closed field. If we receive VIR_NET_OK message for stream
then stream is finished if oldest (closest to queue end) message
in stream queue is finish message and stream is aborted if oldest
message is abort message. Otherwise it is protocol error.

By the way we need to fix case of receiving VIR_NET_CONTINUE
message. Now we take oldest message in queue and check if
this is dummy message. If one thread first sends abort and
second thread then receives data then oldest message is abort
message and second thread won't be notified when data arrives.
Let's find oldest dummy message instead.
Signed-off-by: NNikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>

If we call virStreamFinish and virStreamAbort from 2 distinct
threads for example we can have access to freed memory.
Because when virStreamFinish finishes for example virStreamAbort
yet to be finished and it access virNetClientStreamPtr object
in stream->privateData.

Also it does not make sense to clear @driver field. After
stream is finished/aborted it is better to have appropriate
error message instead of "unsupported error".

This commit reverts [1] or virNetClientStreamPtr and
virStreamPtr will never be unrefed due to cyclic dependency.
Before this patch we don't have leaks because all execution
paths we call virStreamFinish or virStreamAbort.

[1] 8b6ffe40 : virNetClientStreamNew: Track origin stream
Signed-off-by: NNikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>

This mixing errors and EOF condition in one flag is odd.
Instead let's check st->err.code where appropriate.
Signed-off-by: NNikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>

Checking virNetClientStreamRaiseError without client lock
is racy which is fixed in [1] for example. Thus let's remove such checks
when we are sending message to server. And in other cases
(like virNetClientStreamRecvHole for example) let's move the check
into client stream code.

virNetClientStreamRecvPacket already have stream lock so we could
introduce another error checking function like virNetClientStreamRaiseErrorLocked
but as error is set when both client and stream lock are hold we
can remove locking from virNetClientStreamRaiseError because all
callers hold either client or stream lock.

Also let's split virNetClientStreamRaiseErrorLocked into checking
state function and checking message send status function. They are
same yet.

[1] 1b6a29c21: rpc: fix race on stream abort/finish and server side abort
Signed-off-by: NNikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>

Signed-off-by: NNikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>

Stream server error is not propagated if thread does not have the buck.
In case we have the buck we are ok due to the code added in [1].

Let's check for stream error on all paths. Now we don't need
to raise error in virNetClientCallDispatchStream.

Old code reported error only if the first message in wait
queue awaits reply. It is odd as depends on wait queue
situation. For example if we have only TX
message in queue and in one iteration loop both send the
message and receive error then thread sending TX message did
not receive the error. Next if we have RX message (first)
and TX message (second) in queue and in one iteration
loop both send the TX message and receive error then
thread sending TX message received error. In short
it was inconsistent. Let's report error whenever
we received it and for every type of message as it makes
sense to report errors as early as possible.

[1] 16c6e2b4: Fix propagation of RPC errors from streams
Signed-off-by: NNikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>

Signed-off-by: NNikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>

In next patches we'll add stream state checks to this
function that applicable to all call paths. This is handy
place because we hold client lock here.
Signed-off-by: NNikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>

Stream abort/finish can hang because we can receive abort message
from server and yet sent abort/finish message to server. The latter
will not be answered ever because after server sends abort message
it forgets the stream and messages for unknown stream are simply ignored.

We check for stream error at the very beginning of remoteStreamFinish/remoteStreamAbort
but stream error can be set after the check in another thread operating
on stream. Let's check for stream error under client lock similar
to what's done in [1].

[1] 833b901c: stream: Check for stream EOF
Signed-off-by: NNikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>

These functions do mostly the same things, and it would be
preferrable if they did them in mostly the same ways. This
also fixes a few violations to our code style guidelines.
Signed-off-by: NAndrea Bolognani <abologna@redhat.com>

The function operates on a virDomainDef and is not tied to
device address assignment in any way, so it makes more sense
for it to live along with qemuDomainIs*() and the like.
Signed-off-by: NAndrea Bolognani <abologna@redhat.com>

Ideally we'd make all of them static, but there are a few
cases where we don't have a virDomainDef instance handy and
so they are the only option.

For the few ones we're forced to keep exporting, document
through comments that the alternative is preferred.
Signed-off-by: NAndrea Bolognani <abologna@redhat.com>

Now that we have added architecture checks to all
qemuDomainIs*() functions, we no longer need to perform the
same checks separately.
Signed-off-by: NAndrea Bolognani <abologna@redhat.com>

There is very little overlap in the machine types available
on different architectures, so broadly speaking checking the
machine type is usually enough; regardless, it's better to
check the architecture as well.
Signed-off-by: NAndrea Bolognani <abologna@redhat.com>

We want the signatures to be consistent, and also we're
going to start using the additional parameter next.
Signed-off-by: NAndrea Bolognani <abologna@redhat.com>

Make sure related functions, eg. all qemuDomainIs*(), are
close together instead of being sprinkled throughout both
the header and implementation file, and also that all
qemuDomainMachine*() functions are declared first since
we're going to make a bunch of them static later on.
Signed-off-by: NAndrea Bolognani <abologna@redhat.com>

While the chances of the current checks resulting in false
positives are basically zero, it's still nicer to check for
the full prefix instead of the prefix's prefix.
Signed-off-by: NAndrea Bolognani <abologna@redhat.com>

No need to check whether we're dealing with a pSeries guest
twice within just a few lines.
Signed-off-by: NAndrea Bolognani <abologna@redhat.com>