error_exit() uses low-level error_printf() to report errors.
Modernize it to use error_vreport().
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Max Reitz <mreitz@redhat.com>
Cc: qemu-block@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Message-Id: <20190417190641.26814-2-armbru@redhat.com>
It would be nice to have Error object not freed away when debugging a
coredump.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Message-Id: <20190415142519.73060-1-vsementsov@virtuozzo.com>
[error_printf_unless_qmp() replaced by error_printf()]
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Before the from qerror_report() to error_setg(), hints looked like
this:
qerror_report(QERR_MACRO, ... arguments ...);
error_printf_unless_qmp(... hint ...);
error_printf_unless_qmp() made perfect sense: it printed exactly when
qerror_report() did.
After the conversion to error_setg():
error_setg(errp, QERR_MACRO, ... arguments ...);
error_printf_unless_qmp(... hint ...);
The "unless QMP part" still made some sense; in QMP context, the
caller generally uses the error as QMP response instead of printing
it.
However, everything else is wrong. If the caller handles the error,
the hint gets printed anyway (unless QMP). If the caller reports the
error, the hint gets printed *before* the report (unless QMP) or not
at all (if QMP).
Commit 50b7b000c9 fixed this by making hints a member of Error. It
kept printing hints with error_printf_unless_qmp():
void error_report_err(Error *err)
{
error_report("%s", error_get_pretty(err));
+ if (err->hint) {
+ error_printf_unless_qmp("%s\n", err->hint->str);
+ }
error_free(err);
}
This is wrong. We should (and now can) print the hint exactly when we
print the error.
The mistake has since been copied to warn_report_err() in commit
e43ead1d0b.
Fix both to use error_printf().
Reported-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Cc: Eric Blake <eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-Id: <20190416153850.5186-1-armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
[Commit message tweaked]
This commit adds a error_init() helper which calls
g_log_set_default_handler() so that glib logs (g_log, g_warning, ...)
are handled similarly to other QEMU logs. This means they will get a
timestamp if timestamps are enabled, and they will go through the
HMP monitor if one is configured.
This commit also adds a call to error_init() to the binaries
installed by QEMU. Since error_init() also calls error_set_progname(),
this means that *-linux-user, *-bsd-user and qemu-pr-helper messages
output with error_report, info_report, ... will slightly change: they
will be prefixed by the binary name.
glib debug messages are enabled through G_MESSAGES_DEBUG similarly to
the glib default log handler.
At the moment, this change will mostly impact SPICE logging if your
spice version is >= 0.14.1. With older spice versions, this is not going
to work as expected, but will not have any ill effect, so this call is
not conditional on the SPICE version.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-Id: <20190131164614.19209-3-cfergeau@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
qemu-io reimplements itself what
error_get_progname()/error_set_progname() already does.
This commit switches it to use this API from qemu-error.h
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-Id: <20190131164614.19209-2-cfergeau@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
The ObjectInfo struct has a variable length array containing the UTF-16
encoded filename. The number of characters of trailing data is given by
the 'length' field in the struct and this must be validated against the
size of the data packet received from the guest.
Since the data is UTF-16, we must convert the byte count we have to a
character count before validating. This must take care to truncate if
a malicious guest sent an odd number of bytes.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Bandan Das <bsd@redhat.com>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
With an external data file, preallocate_co() must write the final byte
to the external data file, not to the qcow2 image file.
This is harmless for preallocation of newly created images (only the
qcow2 file size is increased to the virtual disk size while it should be
much smaller), but with preallocated resize, it could in theory cause
visible corruption if the metadata of the image is larger than the data
(e.g. lots of bitmaps).
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Commit 767abe7 ("chardev: forbid 'wait' option with client sockets")
is a bit too strict. Current libvirt always set wait=false, and will
thus fail to add client chardev.
Make the code more permissive, allowing wait=false with client socket
chardevs. Deprecate usage of 'wait' with client sockets.
Fixes: 767abe7f49
Cc: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Message-id: 20190415163337.2795-1-marcandre.lureau@redhat.com
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Gcc 9 needs some convincing that sopreprbuf really is going to fill
in iov in the call from soreadbuf, even though the failure case
shouldn't happen.
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Message-Id: <20190415121740.9881-1-dgilbert@redhat.com>
Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
Filter the qemu-nbd server output to get rid of a direct reference
to my build directory.
Fixes: e9dce9cb
Reported-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
tmpfs does not support O_DIRECT. Detect this case, and skip flipping
@direct if the filesystem does not support it.
Fixes: bf3e50f623
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
It turns out that having options listed in three places continues to be
a bad idea. I'm still toying with the idea of an improved infrastructure
here, but in the meantime, another bandaid.
There are three locations:
(1) .hx file, formatted as texi
(2) .hx file, formatted as human readable.
(3) .texi file, as section headers, formatted as texi.
You can compare the two summaries within the .hx file like so:
Human-readable command summaries:
`./qemu-img --help | grep 'Command syntax' -A14`
Detokenized texi command summaries:
`grep "@item" qemu-img-cmds.hx | sed -E 's|@var\{([^\}]*?)\}|\1|g'`
You can compare the two separate texi summaries like so:
Texi command summaries:
`grep "@item" qemu-img-cmds.hx"`
Texi command headers:
grep -E "@item.*@var" qemu-img.texi | tail -14
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Message-id: 20190409210655.777-1-jsnow@redhat.com
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
On some systems wchar_t is "long int", on others just "int".
So go cast to "long int" and adjust the printf format accordingly.
Reported-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Message-id: 20190402073018.17747-1-kraxel@redhat.com
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Here's a last minute pull request for 4.0. Turns out my last pull
request, to fix a regression in extended config space access for the
pseries machine didn't fix things hard enough. This PR has a single
patch which improves the fix to work in more cases.
It's a ghastly, ghastly hack, but it's simple and localized. I
already have patches almost ready to go in 4.1 that provides a simpler
and cleaner solution to all this.
-----BEGIN PGP SIGNATURE-----
iQIzBAABCAAdFiEEdfRlhq5hpmzETofcbDjKyiDZs5IFAlywI1gACgkQbDjKyiDZ
s5JDchAAy5LBsVDPH3W/X3Btbrfmk8aY92pWbfZZBfCKVTRYD2HrQ9bqUgK8rDXe
TIPhVLhZsZdxJX6aAMRgymeO8vDdoksSmldGSQe9nBrnywgwoWBMEC4P5anVNGQv
8SSZBuhBaea3lBk8mF2vgJsP9QiLtQvHy/l9lTDaUSVNgws3fqGf5/YBjx7h6E+R
aUdgTvlDvPkm14vZN7W1bkpGYk3J74rVo6qu91zRD9hKzHTuFnRxZE4EQpOKvaNa
Tq8we05kfKdct5JoJXj+NvsT77lgynyt0AXT/TGtRdS2cgZ3JtHq2ZD/93dBMS/d
A5fNClEQ45XW/0dLMzzTN4xP3yII5N4mY78kB58L5PNezxCa+7MvxW/2QEHFskyi
MaWXN/2Dchx9cBvQuueulpaQHUiAYpFVIG9S0OPxG/SDdyqo929F2Z/P152scp3a
ChG9JKVgXtp3JHMWlBPFdOPJhNhtUN9HiqTrydbVka1rMJyAU+8hXpo96BvPZGdf
oH+nEqoDiJLGHp930esQ0F7wTQbSaIF+nEVkF8Q71NLDELnpTBMOc02TLhLocuqJ
cwlDHkw+LN+h+gdzxKtOMMVmL3MG47zsgvY8pgqKItP5i7k3gqEq4lUc75P6Athe
ULVHYQukowD4ns5ngBPZXZyv0zq7EAa8xoT/FAMx92UcNdshqKg=
=sN2O
-----END PGP SIGNATURE-----
Merge remote-tracking branch 'remotes/dgibson/tags/ppc-for-4.0-20190412' into staging
ppc patch queue for 2018-04-12
Here's a last minute pull request for 4.0. Turns out my last pull
request, to fix a regression in extended config space access for the
pseries machine didn't fix things hard enough. This PR has a single
patch which improves the fix to work in more cases.
It's a ghastly, ghastly hack, but it's simple and localized. I
already have patches almost ready to go in 4.1 that provides a simpler
and cleaner solution to all this.
# gpg: Signature made Fri 12 Apr 2019 06:34:16 BST
# gpg: using RSA key 75F46586AE61A66CC44E87DC6C38CACA20D9B392
# gpg: Good signature from "David Gibson <david@gibson.dropbear.id.au>" [full]
# gpg: aka "David Gibson (Red Hat) <dgibson@redhat.com>" [full]
# gpg: aka "David Gibson (ozlabs.org) <dgibson@ozlabs.org>" [full]
# gpg: aka "David Gibson (kernel.org) <dwg@kernel.org>" [unknown]
# Primary key fingerprint: 75F4 6586 AE61 A66C C44E 87DC 6C38 CACA 20D9 B392
* remotes/dgibson/tags/ppc-for-4.0-20190412:
spapr_pci: Fix broken naming of PCI bus
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Recent commit 5cf0d326a0 fixed a regression which was preventing the
guest to access the extended config space of a PCIe device. This was
done by introducing a new PCI bus subtype for PAPR. The original fix
was causing PCI busses to be named "spapr-pci-host-bridge-root-bus.N"
instead of "pci.N", which was making upper layers unhappy of course.
This got worked around by hardcoding the PCI bus name to "pci.0", but
this only works for the default PHB. And we're now hitting:
# qemu-system-ppc64 \
-device spapr-pci-host-bridge,index=1 \
-device e1000e,bus=pci.0 \
-device e1000e,bus=pci.1
qemu-system-ppc64: -device e1000e,bus=pci.1: Bus 'pci.1' not found
David already posted some patches [1] to control PCI extended config
space accesses with a new flag in the base PCI bus class instead of
subtyping. These patches are a bit more intrusive though, and
are targetted for 4.1.
When no name is passed to pci_register_bus(), the core device code
generates a lowercase name based on the QOM typename. The typename
for the base PCI bus class is "PCI", hence the "pci.0", "pci.1"
bus names. Rename the type of the PAPR PCI bus to "pci", so that
the QOM code can generate proper names. This is a hack but it is
enough to fix the regression. And all this will be reworked properly
in 4.1.
[1] https://patchwork.ozlabs.org/project/qemu-devel/list/?series=100486
Fixes: 5cf0d326a0
Signed-off-by: Greg Kurz <groug@kaod.org>
Message-Id: <155500034416.646888.1307366522340665522.stgit@bahia.lab.toulouse-stg.fr.ibm.com>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
A single patch to avoid an overflow when loading device trees.
-----BEGIN PGP SIGNATURE-----
iQEzBAABCAAdFiEE9sSsRtSTSGjTuM6PIeENKd+XcFQFAlytMDAACgkQIeENKd+X
cFRsGwf/ZdJ+HKAClxK2oFb2Z6bk1kqb1zbeJ1rQlSHD7r9CHnVCs3DBqnY7Oilw
4VGhZiMdS6zCDRb+g+rKBFpfRU3XTygKYTr9na+ADpLcSGeLZlwbbDDFvABvKPpx
4MDVemNRCyaHHdJeZpRxTlALUFBeNCRQWGtUcCp1BP/xjRQc1IbMj/6gEi7rSD13
y7zFVfwLM7QiJpQHTw9VBAgCtLVfRPP7S27Ey+CnKwf97kqLsLspZw0nXfNgm9sk
vrZ1XWcYDO/5BusYo9Kcdie8C1ykiSGMtJax5DFnOhNjK9A8tIx70v24dL5mQMJg
HKzHuXeX4wdL0L12+fBRsL1wAZDtfg==
=6l+a
-----END PGP SIGNATURE-----
Merge remote-tracking branch 'remotes/alistair/tags/pull-device-tree-20190409-1' into staging
Single device tree fix for 4.0
A single patch to avoid an overflow when loading device trees.
# gpg: Signature made Wed 10 Apr 2019 00:52:16 BST
# gpg: using RSA key F6C4AC46D4934868D3B8CE8F21E10D29DF977054
# gpg: Good signature from "Alistair Francis <alistair@alistair23.me>" [full]
# Primary key fingerprint: F6C4 AC46 D493 4868 D3B8 CE8F 21E1 0D29 DF97 7054
* remotes/alistair/tags/pull-device-tree-20190409-1:
device_tree: Fix integer overflowing in load_device_tree()
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
If the value of get_image_size() exceeds INT_MAX / 2 - 10000, the
computation of @dt_size overflows to a negative number, which then
gets converted to a very large size_t for g_malloc0() and
load_image_size(). In the (fortunately improbable) case g_malloc0()
succeeds and load_image_size() survives, we'd assign the negative
number to *sizep. What that would do to the callers I can't say, but
it's unlikely to be good.
Fix by rejecting images whose size would overflow.
Reported-by: Kurtis Miller <kurtis.miller@nccgroup.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
Message-Id: <20190409174018.25798-1-armbru@redhat.com>
Coverity points out (CID 1400442) that in this code:
if (packet->pages_alloc > p->pages->allocated) {
multifd_pages_clear(p->pages);
multifd_pages_init(packet->pages_alloc);
}
we free p->pages in multifd_pages_clear() but continue to
use it in the following code. We also leak memory, because
multifd_pages_init() returns the pointer to a new MultiFDPages_t
struct but we are ignoring its return value.
Fix both of these bugs by adding the missing assignment of
the newly created struct to p->pages.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Juan Quintela <quintela@redhat.com>
Message-id: 20190409151830.6024-1-peter.maydell@linaro.org
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
QEMU currently crashes when you try to hot-plug an "nvdimm" device
on older machine types:
$ qemu-system-x86_64 -monitor stdio -M pc-1.1
QEMU 3.1.92 monitor - type 'help' for more information
(qemu) device_add nvdimm,id=nvdimmn1
qemu-system-x86_64: /home/thuth/devel/qemu/util/error.c:57: error_setv:
Assertion `*errp == ((void *)0)' failed.
Aborted (core dumped)
The call to hotplug_handler_pre_plug() in pc_memory_pre_plug() has been
added recently before the check whether nvdimm is enabled. It should
be done after the check. And while we're at it, also check the errp
after the hotplug_handler_pre_plug(), otherwise errors are silently
ignored here.
Fixes: 9040e6dfa8
Signed-off-by: Thomas Huth <thuth@redhat.com>
Message-Id: <20190407092314.11066-1-thuth@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
In the accessor functions ld*_he_p() and st*_he_p() we use memcpy()
to perform a load or store to a pointer which might not be aligned
for the size of the type. We rely on the compiler to optimize this
memcpy() into an efficient load or store instruction where possible.
This is required for good performance, but at the moment it is also
required for correct operation, because some users of these functions
require that the access is atomic if the pointer is aligned, which
will only be the case if the compiler has optimized out the memcpy().
(The particular example where we discovered this is the virtio
vring_avail_idx() which calls virtio_lduw_phys_cached() which
eventually ends up calling lduw_he_p().)
Unfortunately some compile environments, such as the fortify-source
setup used in Alpine Linux, define memcpy() to a wrapper function
in a way that inhibits this compiler optimization.
The correct long-term fix here is to add a set of functions for
doing atomic accesses into AddressSpaces (and to other relevant
families of accessor functions like the virtio_*_phys_cached()
ones), and make sure that callsites which want atomic behaviour
use the correct functions.
In the meantime, switch to using __builtin_memcpy() in the
bswap.h accessor functions. This will make us robust against things
like this fortify library in the short term. In the longer term
it will mean that we don't end up with these functions being really
badly-performing even if the semantics of the out-of-line memcpy()
are correct.
Reported-by: Fernando Casas Schössow <casasfernando@outlook.com>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Message-Id: <20190318112938.8298-1-peter.maydell@linaro.org>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Since commit f590a812c2 we build the EDK2 EfiRom utility
unconditionally.
Some distributions require to use extra compiler/linker flags,
i.e. SUSE which enforces the PIE protection (see [*]).
EDK2 build tools already provide a set of variables for that,
use them to allow the caller to easily inject compiler/linker
options..
Now build scripts can pass extra options, example:
$ make -C roms \
EDK2_BASETOOLS_OPTFLAGS='-fPIE' \
efirom
[*] https://lists.opensuse.org/opensuse-factory/2017-06/msg00403.html
Reported-by: Olaf Hering <olaf@aepfle.de>
Suggested-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Message-Id: <20190409134536.15548-3-philmd@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
The iPXE's 'veryclean' recipe removes $(EFIROM) even if the EFIROM
macro originates from elsewhere:
$ git checkout f590a812c21~
$ make -C roms clean EFIROM=$(type -P EfiRom)
make: Entering directory '/source/qemu/roms'
[...]
make -C ipxe/src veryclean
make[1]: Entering directory '/source/qemu/roms/ipxe/src'
rm -f bin{,-*}/*.* bin{,-*}/.certificate.* bin{,-*}/.certificates.* bin{,-*}/.private_key.* bin{,-*}/errors bin{,-*}/NIC ./util/zbin ./util/elf2efi32 ./util/elf2efi64 /usr/bin/EfiRom ./util/efifatbin ./util/iccfix ./util/einfo TAGS bin{,-*}/symtab
rm: cannot remove '/usr/bin/EfiRom': Permission denied
make[1]: *** [Makefile.housekeeping:1564: clean] Error 1
make[1]: Leaving directory '/source/qemu/roms/ipxe/src'
make: *** [Makefile:152: clean] Error 2
make: Leaving directory '/source/qemu/roms'
Before f590a812c2 this variable could be overridden or unset,
and the 'veryclean' Makefile rule would not complain.
Commit f590a812c2 enforces this variable to the Intel EfiRom
tool provided by the EDK2 project.
To avoid the name clash and make the difference between the
projects obvious, rename the variable used by the EDK2 project
as EDK2_EFIROM.
Fixes: f590a812c2
Reported-by: Olaf Hering <olaf@aepfle.de>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Message-Id: <20190409134536.15548-2-philmd@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Fix a TCG crash due to attempting an atomic increment
operation without having set up the address first.
This is a similar case to that dealt with in commit
e84fcd7f66, and we fix it in the same way.
Fixes: https://bugs.launchpad.net/qemu/+bug/1807675
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
Message-id: 20190328104750.25046-1-peter.maydell@linaro.org
The PAPR PHB acts as a legacy PCI bus but it allows PCIe extended
config space accesses anyway (for pseries-2.9 and newer machine
types).
Introduce a specific PCI bus subtype to inform the common PCI code
about that.
Fixes: c2077e2ca0
Signed-off-by: Greg Kurz <groug@kaod.org>
Message-Id: <155414130834.574858.16502276132110219890.stgit@bahia.lan>
[dwg: Apply fix so we don't rename the default pci bus, breaking everything]
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Some PHB implementations, eg. PAPR used on pseries machine, act like
a regular PCI bus rather than a PCIe bus, but allow access to the
PCIe extended config space anyway.
Introduce a new PCI bus class method to modelize this behaviour and
use it when adjusting the config space size limit during accesses.
No behaviour change for existing PCI bus types.
Signed-off-by: Greg Kurz <groug@kaod.org>
Message-Id: <155414130271.574858.4253514266378127489.stgit@bahia.lan>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Add a missing space to the error message used when giving up on a
server that insists on an alignment which renders the last few bytes
of the export unreadable.
Fixes: 3add3ab78
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <20190404145226.32649-1-eblake@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
In commit 0c1d50bd, I added a couple of TODO comments about whether we
consult bl.request_alignment when responding to NBD_OPT_INFO. At the
time, qemu as server was hard-coding an advertised alignment of 512 to
clients that promised to obey constraints, and there was no function
for getting at a device's preferred alignment. But in hindsight,
advertising 512 when the block device prefers 1 caused other
compliance problems, and commit b0245d64 changed one of the two TODO
comments to advertise a more accurate alignment. Time to fix the other
TODO. Doesn't really impact qemu as client (our normal client doesn't
use NBD_OPT_INFO, and qemu-nbd --list promises to obey block sizes),
but it might prove useful to other clients.
Fixes: b0245d64
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <20190403030526.12258-4-eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
We've recently added traces for clients to flag server non-compliance;
let's do the same for servers to flag client non-compliance. According
to the spec, if the client requests NBD_INFO_BLOCK_SIZE, it is
promising to send all requests aligned to those boundaries. Of
course, if the client does not request NBD_INFO_BLOCK_SIZE, then it
made no promises so we shouldn't flag anything; and because we are
willing to handle clients that made no promises (the spec allows us to
use NBD_REP_ERR_BLOCK_SIZE_REQD if we had been unwilling), we already
have to handle unaligned requests (which the block layer already does
on our behalf). So even though the spec allows us to return EINVAL
for clients that promised to behave, it's easier to always answer
unaligned requests. Still, flagging non-compliance can be useful in
debugging a client that is trying to be maximally portable.
Qemu as client used to have one spot where it sent non-compliant
requests: if the server sends an unaligned reply to
NBD_CMD_BLOCK_STATUS, and the client was iterating over the entire
disk, the next request would start at that unaligned point; this was
fixed in commit a39286dd when the client was taught to work around
server non-compliance; but is equally fixed if the server is patched
to not send unaligned replies in the first place (yes, qemu 4.0 as
server still has few such bugs, although they will be patched in
4.1). Fortunately, I did not find any more spots where qemu as client
was non-compliant. I was able to test the patch by using the following
hack to convince qemu-io to run various unaligned commands, coupled
with serving 512-byte alignment by intentionally omitting '-f raw' on
the server while viewing server traces.
| diff --git i/nbd/client.c w/nbd/client.c
| index 427980bdd22..1858b2aac35 100644
| --- i/nbd/client.c
| +++ w/nbd/client.c
| @@ -449,6 +449,7 @@ static int nbd_opt_info_or_go(QIOChannel *ioc, uint32_t opt,
| nbd_send_opt_abort(ioc);
| return -1;
| }
| + info->min_block = 1;//hack
| if (!is_power_of_2(info->min_block)) {
| error_setg(errp, "server minimum block size %" PRIu32
| " is not a power of two", info->min_block);
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <20190403030526.12258-3-eblake@redhat.com>
[eblake: address minor review nits]
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Don't increment remaining_bytes until we know that we will actually be
including the current block status extent in the reply; otherwise, the
value traced will include a bytes value that is oversized by the
length of the next block status extent which did not get sent because
it instead ended the loop.
Fixes: fb7afc79
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <20190403030526.12258-2-eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
drive_new() returns null without setting an error when it provided
help. add_init_drive() assumes null means failure, and crashes trying
to report a null error.
Fixes: c4f26c9f37
Cc: qemu-stable@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
The glibc-2.29.9000-6.fc31.x86_64 package finally includes the gettid()
function as part of unistd.h when __USE_GNU is defined. This clashes
with linux-user code which unconditionally defines this function name
itself.
/home/berrange/src/virt/qemu/linux-user/syscall.c:253:16: error: static declaration of ‘gettid’ follows non-static declaration
253 | _syscall0(int, gettid)
| ^~~~~~
/home/berrange/src/virt/qemu/linux-user/syscall.c:184:13: note: in definition of macro ‘_syscall0’
184 | static type name (void) \
| ^~~~
In file included from /usr/include/unistd.h:1170,
from /home/berrange/src/virt/qemu/include/qemu/osdep.h:107,
from /home/berrange/src/virt/qemu/linux-user/syscall.c:20:
/usr/include/bits/unistd_ext.h:34:16: note: previous declaration of ‘gettid’ was here
34 | extern __pid_t gettid (void) __THROW;
| ^~~~~~
CC aarch64-linux-user/linux-user/signal.o
make[1]: *** [/home/berrange/src/virt/qemu/rules.mak:69: linux-user/syscall.o] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:449: subdir-aarch64-linux-user] Error 2
While we could make our definition conditional and rely on glibc's impl,
this patch simply renames our definition to sys_gettid() which is a
common pattern in this file.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Laurent Vivier <laurent@vivier.eu>
Message-Id: <20190320161842.13908-3-berrange@redhat.com>
Signed-off-by: Laurent Vivier <laurent@vivier.eu>
The gettid syscall was introduced in Linux 2.4.11. This is old enough
that we can assume it always exists and thus not bother with the
conditional backcompat logic.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Laurent Vivier <laurent@vivier.eu>
Message-Id: <20190320161842.13908-2-berrange@redhat.com>
Signed-off-by: Laurent Vivier <laurent@vivier.eu>
When bdrv_temp_snapshot_options() is called for snapshot=on, the
'discard' option in the options QDict hasn't been parsed and merged into
the flags yet. So copy the dict entry to make sure that the temporary
overlay enables discard when it was requested for the drive.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
The test uses the trick:
if (!opts) {
opts = &(QOSGraph...Options) { };
}
in a couple of places, however the temporary created
by the &() {} goes out of scope at the bottom of the if,
and results in a seg or assert when opts-> fields are
used (on fedora 30's gcc 9).
Fixes: fc281c8020
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Message-Id: <20190405184037.16799-1-dgilbert@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
Clean up wrong usage of FALSE and TRUE in places that use "bool" from stdbool.h.
FALSE and TRUE (with capital letters) are the constants defined by glib for
being used with the "gboolean" type of glib. But some parts of the code also use
TRUE and FALSE for variables that are declared as "bool" (the type from <stdbool.h>).
Signed-off-by: Jafar Abdi <cafer.abdi@gmail.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Acked-by: David Gibson <david@gibson.dropbear.id.au>
Message-Id: <1553351197-14581-4-git-send-email-cafer.abdi@gmail.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
Clean up wrong usage of FALSE and TRUE in places that use "bool" from stdbool.h.
FALSE and TRUE (with capital letters) are the constants defined by glib for
being used with the "gboolean" type of glib. But some parts of the code also use
TRUE and FALSE for variables that are declared as "bool" (the type from <stdbool.h>).
Signed-off-by: Jafar Abdi <cafer.abdi@gmail.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Message-Id: <1553351197-14581-3-git-send-email-cafer.abdi@gmail.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
