Re: [PATCH] Add missing pixman dependecy
On Wed, Nov 9, 2022 at 5:14 PM Philippe Mathieu-Daudé wrote: > > Hi Mirek, > > On 9/11/22 16:34, mreza...@redhat.com wrote: > > From: Miroslav Rezanina > > > > Commit cfead31326 'acpi: pc: vga: use AcpiDevAmlIf interface to build VGA > > device descriptors' added > > a new file - acpi-vga.c. This file (indirectly) includes pixman.h file so > > we need to ensure pixman > > is available when file is compiled. > > We don't need this dependency if we move the build_vga_aml() declaration > out of hw/display/vga_int.h, i.e. hw/display/acpi-vga.h. > I'm not confident enough to change header files, especially during RC phase. I followed the pattern used in the hw/display/meson.build as we need this to be fixed before GA. Mirek > Regards, > > Phil. > -- Miroslav Řezanina Virtualization Team - Maintainer
[PATCH v2] Use long endian options for ppc64
GCC options pairs -mlittle/-mlittle-endian and -mbig/-mbig-endian are equivalent on ppc64 architecture. However, Clang supports only long version of the options. Use longer form in configure to properly support both GCC and Clang compiler. In addition, fix this issue in tcg test configure. Signed-off-by: Miroslav Rezanina --- This is v2 of configure: Use -mlittle-endian instead of -mlittle for ppc64. v2: - handle both -mlittle and -mbig usage - fix tests/tcg/configure.sh --- configure | 4 ++-- tests/tcg/configure.sh | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/configure b/configure index e6cfc0e4be..066fa29b70 100755 --- a/configure +++ b/configure @@ -655,10 +655,10 @@ case "$cpu" in ppc) CPU_CFLAGS="-m32" ;; ppc64) -CPU_CFLAGS="-m64 -mbig" ;; +CPU_CFLAGS="-m64 -mbig-endian" ;; ppc64le) cpu="ppc64" -CPU_CFLAGS="-m64 -mlittle" ;; +CPU_CFLAGS="-m64 -mlittle-endian" ;; s390) CPU_CFLAGS="-m31" ;; diff --git a/tests/tcg/configure.sh b/tests/tcg/configure.sh index 309335a2bd..21959e1fde 100755 --- a/tests/tcg/configure.sh +++ b/tests/tcg/configure.sh @@ -64,9 +64,9 @@ fi : ${cross_cc_ppc="powerpc-linux-gnu-gcc"} : ${cross_cc_cflags_ppc="-m32"} : ${cross_cc_ppc64="powerpc64-linux-gnu-gcc"} -: ${cross_cc_cflags_ppc64="-m64 -mbig"} +: ${cross_cc_cflags_ppc64="-m64 -mbig-endian"} : ${cross_cc_ppc64le="$cross_cc_ppc64"} -: ${cross_cc_cflags_ppc64le="-m64 -mlittle"} +: ${cross_cc_cflags_ppc64le="-m64 -mlittle-endian"} : ${cross_cc_riscv64="riscv64-linux-gnu-gcc"} : ${cross_cc_s390x="s390x-linux-gnu-gcc"} : ${cross_cc_sh4="sh4-linux-gnu-gcc"} -- 2.34.1
[PATCH] Fix tcg_out_vec_op argument type
Newly defined tcg_out_vec_op (34ef767609 tcg/s390x: Add host vector framework) for s390x uses pointer argument definition. This fails on gcc 11 as original declaration uses array argument: In file included from ../tcg/tcg.c:430: /builddir/build/BUILD/qemu-6.1.50/tcg/s390x/tcg-target.c.inc:2702:42: error: argument 5 of type 'const TCGArg *' {aka 'const long unsigned int *'} declared as a pointer [-Werror=array-parameter=] 2702 |const TCGArg *args, const int *const_args) |~~^~~~ ../tcg/tcg.c:121:41: note: previously declared as an array 'const TCGArg[16]' {aka 'const long unsigned int[16]'} 121 |const TCGArg args[TCG_MAX_OP_ARGS], |~^ In file included from ../tcg/tcg.c:430: /builddir/build/BUILD/qemu-6.1.50/tcg/s390x/tcg-target.c.inc:2702:59: error: argument 6 of type 'const int *' declared as a pointer [-Werror=array-parameter=] 2702 |const TCGArg *args, const int *const_args) |~~~^~ ../tcg/tcg.c:122:38: note: previously declared as an array 'const int[16]' 122 |const int const_args[TCG_MAX_OP_ARGS]); |~~^~~ Fixing argument type to pass build. Signed-off-by: Miroslav Rezanina --- tcg/s390x/tcg-target.c.inc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tcg/s390x/tcg-target.c.inc b/tcg/s390x/tcg-target.c.inc index 8938c446c8..57e803e339 100644 --- a/tcg/s390x/tcg-target.c.inc +++ b/tcg/s390x/tcg-target.c.inc @@ -2699,7 +2699,8 @@ static void tcg_out_dupi_vec(TCGContext *s, TCGType type, unsigned vece, static void tcg_out_vec_op(TCGContext *s, TCGOpcode opc, unsigned vecl, unsigned vece, - const TCGArg *args, const int *const_args) + const TCGArg args[TCG_MAX_OP_ARGS], + const int const_args[TCG_MAX_OP_ARGS]) { TCGType type = vecl + TCG_TYPE_V64; TCGArg a0 = args[0], a1 = args[1], a2 = args[2]; -- 2.27.0
[PATCH] Fix libpmem configuration option
For some reason, libpmem option setting was set to work in an opposite way (--enable-libpmem disabled it and vice versa). Fixing this so configuration works properly. Signed-off-by: Miroslav Rezanina --- configure | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/configure b/configure index 7994bdee92..ffa93cc5fd 100755 --- a/configure +++ b/configure @@ -1501,9 +1501,9 @@ for opt do ;; --disable-debug-mutex) debug_mutex=no ;; - --enable-libpmem) libpmem=disabled + --enable-libpmem) libpmem="enabled" ;; - --disable-libpmem) libpmem=enabled + --disable-libpmem) libpmem="disabled" ;; --enable-xkbcommon) xkbcommon="enabled" ;; -- 2.27.0
[PATCH] Fix libdaxctl option
For some reason, libdaxctl option setting was set to work in an opposite way (--enable-libdaxctl disabled it and vice versa). Fixing this so configuration works properly. Signed-off-by: Miroslav Rezanina --- configure | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/configure b/configure index 650d9c0735..4f51528a77 100755 --- a/configure +++ b/configure @@ -1531,9 +1531,9 @@ for opt do ;; --disable-keyring) secret_keyring="no" ;; - --enable-libdaxctl) libdaxctl=disabled + --enable-libdaxctl) libdaxctl="enabled" ;; - --disable-libdaxctl) libdaxctl=enabled + --disable-libdaxctl) libdaxctl="disabled" ;; --enable-fuse) fuse="enabled" ;; -- 2.27.0
Re: Prevent compiler warning on block.c
- Original Message - > From: "Peter Maydell" > To: "Miroslav Rezanina" > Cc: "QEMU Developers" , "Vladimir Sementsov-Ogievskiy" > , > "Qemu-block" > Sent: Wednesday, May 5, 2021 12:43:44 PM > Subject: Re: Prevent compiler warning on block.c > > On Wed, 5 May 2021 at 09:06, Miroslav Rezanina wrote: > > > > Commit 3108a15cf (block: introduce bdrv_drop_filter()) introduced > > uninitialized > > variable to_cow_parent in bdrv_replace_node_common function that is used > > only when > > detach_subchain is true. It is used in two places. First if block properly > > initialize > > the variable and second block use it. > > > > However, compiler treats this two blocks as two independent cases so it > > thinks first > > block can fail test and second one pass (although both use same condition). > > This cause > > warning that variable can be uninitialized in second block. > > > > To prevent this warning, initialize the variable with NULL. > > If fixing compiler warnings, please quote the compiler name/version > in the commit message. (This helps with understanding whether the issue > is because of an older and not-smart-enough compiler or a new bleeding-edge > compiler with extra checking.) Hi Peter, sorry for missing version. I was going to put the version in but got distracted when checking versions. This warning occurs using GCC 8.4.1 and 11.0.1. Mirek > > thanks > -- PMM > > -- Miroslav Rezanina Software Engineer - Virtualization Team Maintainer
Prevent compiler warning on block.c
Commit 3108a15cf (block: introduce bdrv_drop_filter()) introduced uninitialized variable to_cow_parent in bdrv_replace_node_common function that is used only when detach_subchain is true. It is used in two places. First if block properly initialize the variable and second block use it. However, compiler treats this two blocks as two independent cases so it thinks first block can fail test and second one pass (although both use same condition). This cause warning that variable can be uninitialized in second block. To prevent this warning, initialize the variable with NULL. Signed-off-by: Miroslav Rezanina --- diff --git a/block.c b/block.c index 874c22c43e..3ca27bd2d9 100644 --- a/block.c +++ b/block.c @@ -4851,7 +4851,7 @@ static int bdrv_replace_node_common(BlockDriverState *from, Transaction *tran = tran_new(); g_autoptr(GHashTable) found = NULL; g_autoptr(GSList) refresh_list = NULL; -BlockDriverState *to_cow_parent; +BlockDriverState *to_cow_parent = NULL; int ret; if (detach_subchain) {
Re: [PATCH 1/2] disas/arm-a64.cc: Fix build error
On Sat, Mar 20, 2021 at 12:18:54PM +0800, Gavin Shan wrote: > This fixes the following build error with gcc v11.0.0: > > # gcc --version > gcc (GCC) 11.0.0 20210210 (Red Hat 11.0.0-0) > > [969/2604] Compiling C++ object libcommon.fa.p/disas_arm-a64.cc.o > FAILED: libcommon.fa.p/disas_arm-a64.cc.o > : > In file included from /usr/include/glib-2.0/glib/gmacros.h:241, >from /usr/lib64/glib-2.0/include/glibconfig.h:9, >from /usr/include/glib-2.0/glib/gtypes.h:32, >from /usr/include/glib-2.0/glib/galloca.h:32, >from /usr/include/glib-2.0/glib.h:30, >from > /home/gavin/sandbox/qemu.main/include/glib-compat.h:32, >from > /home/gavin/sandbox/qemu.main/include/qemu/osdep.h:126, >from ../disas/arm-a64.cc:21: > /usr/include/c++/11/type_traits:56:3: error: template with C linkage > 56 | template > | ^~~~ > ../disas/arm-a64.cc:20:1: note: ‘extern "C"’ linkage started here > 20 | extern "C" { > | ^~ > > Signed-off-by: Gavin Shan > --- > disas/arm-a64.cc | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/disas/arm-a64.cc b/disas/arm-a64.cc > index 9fa779e175..8545c04038 100644 > --- a/disas/arm-a64.cc > +++ b/disas/arm-a64.cc > @@ -17,13 +17,13 @@ > * along with this program. If not, see <http://www.gnu.org/licenses/>. > */ > > +#include "vixl/a64/disasm-a64.h" > + > extern "C" { > #include "qemu/osdep.h" > #include "disas/dis-asm.h" > } > > -#include "vixl/a64/disasm-a64.h" > - > using namespace vixl; > > static Decoder *vixl_decoder = NULL; > -- > 2.23.0 > > This error occurs when c++ compiler is used. With the fix, build pass. Reviewed-by: Miroslav Rezanina
Re: [PATCH v2] Add missing initialization for g_autofree variables
On Mon, Mar 15, 2021 at 09:08:01AM +0100, Thomas Huth wrote: > On 15/03/2021 09.00, mreza...@redhat.com wrote: > > From: Miroslav Rezanina > > > > When declaring g_autofree variable without inicialization, compiler > > will raise "may be used uninitialized in this function" warning due > > to automatic free handling. > > > > This is mentioned in docs/devel/style.rst (quote from section > > "Automatic memory deallocation"): > > > >* Variables declared with g_auto* MUST always be initialized, > > otherwise the cleanup function will use uninitialized stack memory > > > > Add inicialization to NULL for these declaration to prevent this > > warning. > > > > Signed-off-by: Miroslav Rezanina > > > > --- > > * From v1: > >-- Removed fixes in hw/remote/memory.c and hw/remote/proxy.c > > fixed by patch sent by Zenghui Yu (multi-process: Initialize > > variables declared with g_auto*) > > --- > > hw/s390x/s390-pci-vfio.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/hw/s390x/s390-pci-vfio.c b/hw/s390x/s390-pci-vfio.c > > index ead4f222d5..0ee7dc21f2 100644 > > --- a/hw/s390x/s390-pci-vfio.c > > +++ b/hw/s390x/s390-pci-vfio.c > > @@ -29,7 +29,7 @@ > >*/ > > bool s390_pci_update_dma_avail(int fd, unsigned int *avail) > > { > > -g_autofree struct vfio_iommu_type1_info *info; > > +g_autofree struct vfio_iommu_type1_info *info = NULL; > > uint32_t argsz; > > assert(avail); > > I'd maybe rather rework the functions like this: > > diff --git a/hw/s390x/s390-pci-vfio.c b/hw/s390x/s390-pci-vfio.c > index ead4f222d5..1fe71fd93f 100644 > --- a/hw/s390x/s390-pci-vfio.c > +++ b/hw/s390x/s390-pci-vfio.c > @@ -29,14 +29,11 @@ > */ > bool s390_pci_update_dma_avail(int fd, unsigned int *avail) > { > -g_autofree struct vfio_iommu_type1_info *info; > -uint32_t argsz; > +uint32_t argsz = sizeof(struct vfio_iommu_type1_info); > +g_autofree struct vfio_iommu_type1_info *info = g_malloc0(argsz); > assert(avail); > -argsz = sizeof(struct vfio_iommu_type1_info); > -info = g_malloc0(argsz); > - Hi Thomas, I thought about it but for some reason I miss-read the code and though that avail is used for calculating argsz and didn't want to use it before assert. I'll send new version with this change. > /* > * If the specified argsz is not large enough to contain all capabilities > * it will be updated upon return from the ioctl. Retry until we have > > > @@ -230,7 +230,7 @@ static void s390_pci_read_pfip(S390PCIBusDevice *pbdev, > >*/ > > void s390_pci_get_clp_info(S390PCIBusDevice *pbdev) > > { > > -g_autofree struct vfio_device_info *info; > > +g_autofree struct vfio_device_info *info = NULL; > > VFIOPCIDevice *vfio_pci; > > uint32_t argsz; > > int fd; > > > > Anyway, > Reviewed-by: Thomas Huth > >
Re: [PATCH v3] multi-process: Initialize variables declared with g_auto*
On Mon, Mar 15, 2021 at 02:20:10PM +0800, Zenghui Yu wrote: > On 2021/3/15 13:48, Miroslav Rezanina wrote: > > Missing declaration without initialization in hw/s390x/s390-pci-vfio.c > > othwerwise correct. Will you send v4 with missing initialization or > > should I send then as another patch? > > I'd prefer the latter so that subsystem maintainers can take the > separate patch into their own tree ('Multi-process QEMU' and 'S390 PCI' > in this case). Please go ahead for the s390 fix. > Ok, I'll handle remaining g_autofree. Reviewed-by: Miroslav Rezanina > > Thanks, > Zenghui >
Re: [PATCH v3] multi-process: Initialize variables declared with g_auto*
On Fri, Mar 12, 2021 at 07:21:43PM +0800, Zenghui Yu wrote: > Quote docs/devel/style.rst (section "Automatic memory deallocation"): > > * Variables declared with g_auto* MUST always be initialized, > otherwise the cleanup function will use uninitialized stack memory > > Initialize @name properly to get rid of the compilation error (using > gcc-7.3.0 on CentOS): > > ../hw/remote/proxy.c: In function 'pci_proxy_dev_realize': > /usr/include/glib-2.0/glib/glib-autocleanups.h:28:3: error: 'name' may be > used uninitialized in this function [-Werror=maybe-uninitialized] >g_free (*pp); >^~~~ > ../hw/remote/proxy.c:350:30: note: 'name' was declared here > g_autofree char *name; > ^~~~ > > Signed-off-by: Zenghui Yu > Reviewed-by: Jagannathan Raman > Reviewed-by: Philippe Mathieu-Daudé > --- > * From v2: > - Add OS distro and compiler version into commit message > - Add Philippe's R-b > - Cc: qemu-triv...@nongnu.org > > hw/remote/memory.c | 5 ++--- > hw/remote/proxy.c | 3 +-- > 2 files changed, 3 insertions(+), 5 deletions(-) > > diff --git a/hw/remote/memory.c b/hw/remote/memory.c > index 32085b1e05..d97947d4b8 100644 > --- a/hw/remote/memory.c > +++ b/hw/remote/memory.c > @@ -42,10 +42,9 @@ void remote_sysmem_reconfig(MPQemuMsg *msg, Error **errp) > > remote_sysmem_reset(); > > -for (region = 0; region < msg->num_fds; region++) { > -g_autofree char *name; > +for (region = 0; region < msg->num_fds; region++, suffix++) { > +g_autofree char *name = g_strdup_printf("remote-mem-%u", suffix); > subregion = g_new(MemoryRegion, 1); > -name = g_strdup_printf("remote-mem-%u", suffix++); > memory_region_init_ram_from_fd(subregion, NULL, > name, sysmem_info->sizes[region], > true, msg->fds[region], > diff --git a/hw/remote/proxy.c b/hw/remote/proxy.c > index 4fa4be079d..6dda705fc2 100644 > --- a/hw/remote/proxy.c > +++ b/hw/remote/proxy.c > @@ -347,13 +347,12 @@ static void probe_pci_info(PCIDevice *dev, Error **errp) > PCI_BASE_ADDRESS_SPACE_IO : PCI_BASE_ADDRESS_SPACE_MEMORY; > > if (size) { > -g_autofree char *name; > +g_autofree char *name = g_strdup_printf("bar-region-%d", i); > pdev->region[i].dev = pdev; > pdev->region[i].present = true; > if (type == PCI_BASE_ADDRESS_SPACE_MEMORY) { > pdev->region[i].memory = true; > } > -name = g_strdup_printf("bar-region-%d", i); > memory_region_init_io(>region[i].mr, OBJECT(pdev), >_mr_ops, >region[i], >name, size); > -- > 2.19.1 > > Missing declaration without initialization in hw/s390x/s390-pci-vfio.c othwerwise correct. Will you send v4 with missing initialization or should I send then as another patch? Mirek
Re: [PATCH v6 0/7] net/eth: Fix stack-buffer-overflow in _eth_get_rss_ex_dst_addr()
- Original Message - > From: "Philippe Mathieu-Daudé" > To: qemu-devel@nongnu.org > Cc: "Jason Wang" , "Stefano Garzarella" > , "Thomas Huth" , > "Miroslav Rezanina" , "Dmitry Fleytman" > , "Paolo Bonzini" > , "Laurent Vivier" , "Philippe > Mathieu-Daudé" > Sent: Wednesday, March 10, 2021 7:31:16 PM > Subject: [PATCH v6 0/7] net/eth: Fix stack-buffer-overflow in > _eth_get_rss_ex_dst_addr() > > I had a look at the patch from Miroslav trying to silence a > compiler warning which in fact is a nasty bug. Here is a fix. > https://www.mail-archive.com/qemu-devel@nongnu.org/msg772735.html > Reviewed-by: Miroslav Rezanina > Since v5: > - addressed Stefano's review comments: > - add now patch fixing in6_address offset > > Since v4: > - reworked again, tested it with Fedora Raw Hide > > Philippe Mathieu-Daudé (7): > net/eth: Use correct in6_address offset in _eth_get_rss_ex_dst_addr() > net/eth: Simplify _eth_get_rss_ex_dst_addr() > net/eth: Better describe _eth_get_rss_ex_dst_addr's offset argument > net/eth: Check size earlier in _eth_get_rss_ex_dst_addr() > net/eth: Check iovec has enough data earlier > net/eth: Read ip6_ext_hdr_routing buffer before accessing it > net/eth: Add an assert() and invert if() statement to simplify code > > net/eth.c | 46 ++--- > tests/qtest/fuzz-e1000e-test.c | 53 ++ > MAINTAINERS | 1 + > tests/qtest/meson.build| 1 + > 4 files changed, 78 insertions(+), 23 deletions(-) > create mode 100644 tests/qtest/fuzz-e1000e-test.c > > -- > 2.26.2 > > > -- Miroslav Rezanina Software Engineer - Virtualization Team Maintainer
Re: [PATCH v4 4/6] net/eth: Check rt_hdr size before casting to ip6_ext_hdr
- Original Message - > From: "Philippe Mathieu-Daudé" > To: qemu-devel@nongnu.org > Cc: "Laurent Vivier" , "Dmitry Fleytman" > , "Miroslav Rezanina" > , "Li Qiang" , "Paolo Bonzini" > , "Jason Wang" > , "Thomas Huth" , "Alexander Bulekov" > , "Stefano Garzarella" > , "Philippe Mathieu-Daudé" , > qemu-sta...@nongnu.org > Sent: Tuesday, March 9, 2021 7:27:07 PM > Subject: [PATCH v4 4/6] net/eth: Check rt_hdr size before casting to > ip6_ext_hdr > > Do not cast our ip6_ext_hdr pointer to ip6_ext_hdr_routing if there > isn't enough data in the buffer for a such structure. > > This fix a 2 bytes buffer overrun in eth_parse_ipv6_hdr() reported > by QEMU fuzzer: > > $ cat << EOF | ./qemu-system-i386 -M pc-q35-5.0 \ > -accel qtest -monitor none \ > -serial none -nographic -qtest stdio > outl 0xcf8 0x80001010 > outl 0xcfc 0xe102 > outl 0xcf8 0x80001004 > outw 0xcfc 0x7 > write 0x25 0x1 0x86 > write 0x26 0x1 0xdd > write 0x4f 0x1 0x2b > write 0xe1020030 0x4 0x190002e1 > write 0xe102003a 0x2 0x0807 > write 0xe1020048 0x4 0x12077cdd > write 0xe1020400 0x4 0xba077cdd > write 0xe1020420 0x4 0x190002e1 > write 0xe1020428 0x4 0x3509d807 > write 0xe1020438 0x1 0xe2 > EOF > = > ==2859770==ERROR: AddressSanitizer: stack-buffer-overflow on address > 0x7ffdef904902 at pc 0x561ceefa78de bp 0x7ffdef904820 sp 0x7ffdef904818 > READ of size 1 at 0x7ffdef904902 thread T0 > #0 0x561ceefa78dd in _eth_get_rss_ex_dst_addr net/eth.c:410:17 > #1 0x561ceefa41fb in eth_parse_ipv6_hdr net/eth.c:532:17 > #2 0x561cef7de639 in net_tx_pkt_parse_headers > hw/net/net_tx_pkt.c:228:14 > #3 0x561cef7dbef4 in net_tx_pkt_parse hw/net/net_tx_pkt.c:273:9 > #4 0x561ceec29f22 in e1000e_process_tx_desc hw/net/e1000e_core.c:730:29 > #5 0x561ceec28eac in e1000e_start_xmit hw/net/e1000e_core.c:927:9 > #6 0x561ceec1baab in e1000e_set_tdt hw/net/e1000e_core.c:2444:9 > #7 0x561ceebf300e in e1000e_core_write hw/net/e1000e_core.c:3256:9 > #8 0x561cef3cd4cd in e1000e_mmio_write hw/net/e1000e.c:110:5 > > Address 0x7ffdef904902 is located in stack of thread T0 at offset 34 in > frame > #0 0x561ceefa320f in eth_parse_ipv6_hdr net/eth.c:486 > > This frame has 1 object(s): > [32, 34) 'ext_hdr' (line 487) <== Memory access at offset 34 overflows > this variable > HINT: this may be a false positive if your program uses some custom stack > unwind mechanism, swapcontext or vfork > (longjmp and C++ exceptions *are* supported) > SUMMARY: AddressSanitizer: stack-buffer-overflow net/eth.c:410:17 in > _eth_get_rss_ex_dst_addr > Shadow bytes around the buggy address: > 0x10003df188d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > 0x10003df188e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > 0x10003df188f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > 0x10003df18900: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > 0x10003df18910: 00 00 00 00 00 00 00 00 00 00 00 00 f1 f1 f1 f1 > =>0x10003df18920:[02]f3 f3 f3 00 00 00 00 00 00 00 00 00 00 00 00 > 0x10003df18930: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > 0x10003df18940: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > 0x10003df18950: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > 0x10003df18960: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > 0x10003df18970: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > Shadow byte legend (one shadow byte represents 8 application bytes): > Addressable: 00 > Partially addressable: 01 02 03 04 05 06 07 > Stack left redzone: f1 > Stack right redzone: f3 > ==2859770==ABORTING > > Add the corresponding qtest case with the fuzzer reproducer. > > FWIW GCC 11 similarly reported: > > net/eth.c: In function 'eth_parse_ipv6_hdr': > net/eth.c:410:15: error: array subscript 'struct ip6_ext_hdr_routing[0]' is > partly outside array bounds of 'struct ip6_ext_hdr[1]' > [-Werror=array-bounds] > 410 | if ((rthdr->rtype == 2) && (rthdr->segleft == 1)) { > | ~^~~ > net/eth.c:485:24: note: while referencing 'ext_hdr' > 485 | struct ip6_ext_hdr ext_hdr; > |^~~ > net/eth.c:410:38: error: array subscript 'struct ip6_ext_hdr_routing[0]' is > partly outside array bounds of 'struct ip6_ext_hdr[1]' > [-Werror=array-bounds] > 410 | if ((rthd
Re: [PATCH v2 0/2] net/eth: Fix stack-buffer-overflow in _eth_get_rss_ex_dst_addr()
On Fri, Jan 15, 2021 at 04:11:24PM +0100, Philippe Mathieu-Daudé wrote: > I had a look at the patch from Miroslav trying to silence a > compiler warning which in fact is a nasty bug. Here is a fix. > https://www.mail-archive.com/qemu-devel@nongnu.org/msg772735.html > > v2: Restrict tests so they don't fail when device aren't available > > Based-on: <20210115150936.282-1-phi...@redhat.com> > "tests/qtest: Fixes fuzz-tests" > > Philippe Mathieu-Daudé (2): > net/eth: Simplify _eth_get_rss_ex_dst_addr() > net/eth: Fix stack-buffer-overflow in _eth_get_rss_ex_dst_addr() > > net/eth.c | 37 +++- > tests/qtest/fuzz-e1000e-test.c | 53 ++ > MAINTAINERS| 1 + > tests/qtest/meson.build| 1 + > 4 files changed, 72 insertions(+), 20 deletions(-) > create mode 100644 tests/qtest/fuzz-e1000e-test.c > > -- > 2.26.2 > > > Reviewed-by: Miroslav Rezanina
Re: [PATCH 0/2] net/eth: Fix stack-buffer-overflow in _eth_get_rss_ex_dst_addr()
On Thu, Jan 14, 2021 at 03:50:39PM +0100, Philippe Mathieu-Daudé wrote: > I had a look at the patch from Miroslav trying to silence a > compiler warning which in fact is a nasty bug. Here is a fix. > https://www.mail-archive.com/qemu-devel@nongnu.org/msg772735.html > > Philippe Mathieu-Daudé (2): > net/eth: Simplify _eth_get_rss_ex_dst_addr() > net/eth: Fix stack-buffer-overflow in _eth_get_rss_ex_dst_addr() > > net/eth.c | 37 + > 1 file changed, 17 insertions(+), 20 deletions(-) > > -- > 2.26.2 > > > With qtest chunk included: Reviewed-by: Miroslav Rezanina
Re: [PATCH v3 1/2] Fix net.c warning on GCC 11
On Thu, Jan 14, 2021 at 02:15:59PM +0100, Philippe Mathieu-Daudé wrote: > +Jason +Dmitry > > On 1/14/21 8:07 AM, Miroslav Rezanina wrote: > > When building qemu with GCC 11, compiling eth.c file produce following > > warning: > > > >warning: array subscript 'struct ip6_ext_hdr_routing[0]' is partly > > outside array bounds of 'struct ip6_ext_hdr[1]' [-Warray-bounds] > > > > This is caused by retyping from ip6_ext_hdr to ip6_ext_hdr_routing that has > > more > > attributes. > > > > As this usage is expected, suppress the warning temporarily through the > > function > > using this retyping. > > This is not expected, this is a bug... Thanks for confirmation, my initial idea was the same but then I got impression (do not remember where) that ip6_ext_hdr is not type where data are initially written to so the overflow here is expected. Mirek > > > > > Signed-off-by: Miroslav Rezanina > > --- > > net/eth.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/net/eth.c b/net/eth.c > > index 1e0821c5f8..b9bdd0435c 100644 > > --- a/net/eth.c > > +++ b/net/eth.c > > @@ -405,6 +405,8 @@ _eth_get_rss_ex_dst_addr(const struct iovec *pkt, int > > pkt_frags, > > struct ip6_ext_hdr *ext_hdr, > > struct in6_address *dst_addr) > > { > > +#pragma GCC diagnostic push > > +#pragma GCC diagnostic ignored "-Warray-bounds" > > struct ip6_ext_hdr_routing *rthdr = (struct ip6_ext_hdr_routing *) > > ext_hdr; > > eth_parse_ipv6_hdr() called iov_to_buf() to fill the 2 bytes of ext_hdr. > > > if ((rthdr->rtype == 2) && > > Here we access after the 2 bytes filled... rthdr->rtype is somewhere on > eth_parse_ipv6_hdr's stack, its content is unknown. > > > @@ -426,6 +428,7 @@ _eth_get_rss_ex_dst_addr(const struct iovec *pkt, int > > pkt_frags, > > } > > > > return false; > > +#pragma GCC diagnostic pop > > Nacked-by: Philippe Mathieu-Daudé > >
Re: [PATCH v3 1/2] Fix net.c warning on GCC 11
On Thu, Jan 14, 2021 at 09:19:20AM -0500, Alexander Bulekov wrote: > On 210114 1415, Philippe Mathieu-Daudé wrote: > > +Jason +Dmitry > > > > On 1/14/21 8:07 AM, Miroslav Rezanina wrote: > > > When building qemu with GCC 11, compiling eth.c file produce following > > > warning: > > > > > >warning: array subscript 'struct ip6_ext_hdr_routing[0]' is partly > > > outside array bounds of 'struct ip6_ext_hdr[1]' [-Warray-bounds] > > > > > > This is caused by retyping from ip6_ext_hdr to ip6_ext_hdr_routing that > > > has more > > > attributes. > > > > > > As this usage is expected, suppress the warning temporarily through the > > > function > > > using this retyping. > > > > This is not expected, this is a bug... > > > > Seems related: https://bugs.launchpad.net/qemu/+bug/1879531 > -Alex > Yes, it is caused by the issue triggering the warning. Do you know whether the patch mentioned in bug was already sent? Mirek > > > > > > Signed-off-by: Miroslav Rezanina > > > --- > > > net/eth.c | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/net/eth.c b/net/eth.c > > > index 1e0821c5f8..b9bdd0435c 100644 > > > --- a/net/eth.c > > > +++ b/net/eth.c > > > @@ -405,6 +405,8 @@ _eth_get_rss_ex_dst_addr(const struct iovec *pkt, int > > > pkt_frags, > > > struct ip6_ext_hdr *ext_hdr, > > > struct in6_address *dst_addr) > > > { > > > +#pragma GCC diagnostic push > > > +#pragma GCC diagnostic ignored "-Warray-bounds" > > > struct ip6_ext_hdr_routing *rthdr = (struct ip6_ext_hdr_routing *) > > > ext_hdr; > > > > eth_parse_ipv6_hdr() called iov_to_buf() to fill the 2 bytes of ext_hdr. > > > > > if ((rthdr->rtype == 2) && > > > > Here we access after the 2 bytes filled... rthdr->rtype is somewhere on > > eth_parse_ipv6_hdr's stack, its content is unknown. > > > > > @@ -426,6 +428,7 @@ _eth_get_rss_ex_dst_addr(const struct iovec *pkt, int > > > pkt_frags, > > > } > > > > > > return false; > > > +#pragma GCC diagnostic pop > > > > Nacked-by: Philippe Mathieu-Daudé > > > > >
[PATCH v3 1/2] Fix net.c warning on GCC 11
When building qemu with GCC 11, compiling eth.c file produce following warning: warning: array subscript 'struct ip6_ext_hdr_routing[0]' is partly outside array bounds of 'struct ip6_ext_hdr[1]' [-Warray-bounds] This is caused by retyping from ip6_ext_hdr to ip6_ext_hdr_routing that has more attributes. As this usage is expected, suppress the warning temporarily through the function using this retyping. Signed-off-by: Miroslav Rezanina --- net/eth.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/net/eth.c b/net/eth.c index 1e0821c5f8..b9bdd0435c 100644 --- a/net/eth.c +++ b/net/eth.c @@ -405,6 +405,8 @@ _eth_get_rss_ex_dst_addr(const struct iovec *pkt, int pkt_frags, struct ip6_ext_hdr *ext_hdr, struct in6_address *dst_addr) { +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Warray-bounds" struct ip6_ext_hdr_routing *rthdr = (struct ip6_ext_hdr_routing *) ext_hdr; if ((rthdr->rtype == 2) && @@ -426,6 +428,7 @@ _eth_get_rss_ex_dst_addr(const struct iovec *pkt, int pkt_frags, } return false; +#pragma GCC diagnostic pop } static bool -- 2.18.4
[PATCH v3 0/2] Fixing several GCC 11 warnings
Compiling qemu using GCC 11 we got several new warnings. To allow build with --enable-werror, we need to solve issues generating these warnings. Signed-of-by: Miroslav Rezanina v2: - Patch 2 rewrite to use strpadcpy - removed Patch 3 (different version sent by Philippe Mathieu-Daudé) v3: - Fixed patch commit logs (no cod changes) Miroslav Rezanina (2): Fix net.c warning on GCC 11 s390x: Use strpadcpy for copying vm name net/eth.c | 3 +++ target/s390x/kvm.c | 12 +--- target/s390x/misc_helper.c | 7 +-- 3 files changed, 13 insertions(+), 9 deletions(-) -- 2.18.4
[PATCH v3 2/2] s390x: Use strpadcpy for copying vm name
Using strncpy with length equal to the size of target array, GCC 11 reports following warning: warning: '__builtin_strncpy' specified bound 256 equals destination size [-Wstringop-truncation] We can prevent this warning by using strpadcpy that copies string up to specified length, zeroes target array after copied string and does not raise warning when length is equal to target array size (and ending '\0' is discarded). Signed-off-by: Miroslav Rezanina --- target/s390x/kvm.c | 12 +--- target/s390x/misc_helper.c | 7 +-- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c index b8385e6b95..dc27fa36c9 100644 --- a/target/s390x/kvm.c +++ b/target/s390x/kvm.c @@ -29,6 +29,7 @@ #include "internal.h" #include "kvm_s390x.h" #include "sysemu/kvm_int.h" +#include "qemu/cutils.h" #include "qapi/error.h" #include "qemu/error-report.h" #include "qemu/timer.h" @@ -1910,18 +1911,15 @@ static void insert_stsi_3_2_2(S390CPU *cpu, __u64 addr, uint8_t ar) strlen(qemu_name))); } sysib.vm[0].ext_name_encoding = 2; /* 2 = UTF-8 */ -memset(sysib.ext_names[0], 0, sizeof(sysib.ext_names[0])); /* If hypervisor specifies zero Extended Name in STSI322 SYSIB, it's * considered by s390 as not capable of providing any Extended Name. * Therefore if no name was specified on qemu invocation, we go with the * same "KVMguest" default, which KVM has filled into short name field. */ -if (qemu_name) { -strncpy((char *)sysib.ext_names[0], qemu_name, -sizeof(sysib.ext_names[0])); -} else { -strcpy((char *)sysib.ext_names[0], "KVMguest"); -} +strpadcpy((char *)sysib.ext_names[0], + sizeof(sysib.ext_names[0]), + qemu_name ?: "KVMguest", '\0'); + /* Insert UUID */ memcpy(sysib.vm[0].uuid, _uuid, sizeof(sysib.vm[0].uuid)); diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c index 58dbc023eb..7ea90d414a 100644 --- a/target/s390x/misc_helper.c +++ b/target/s390x/misc_helper.c @@ -19,6 +19,7 @@ */ #include "qemu/osdep.h" +#include "qemu/cutils.h" #include "qemu/main-loop.h" #include "cpu.h" #include "internal.h" @@ -369,8 +370,10 @@ uint32_t HELPER(stsi)(CPUS390XState *env, uint64_t a0, uint64_t r0, uint64_t r1) ebcdic_put(sysib.sysib_322.vm[0].name, qemu_name, MIN(sizeof(sysib.sysib_322.vm[0].name), strlen(qemu_name))); -strncpy((char *)sysib.sysib_322.ext_names[0], qemu_name, -sizeof(sysib.sysib_322.ext_names[0])); +strpadcpy((char *)sysib.sysib_322.ext_names[0], + sizeof(sysib.sysib_322.ext_names[0]), + qemu_name, '\0'); + } else { ebcdic_put(sysib.sysib_322.vm[0].name, "TCGguest", 8); strcpy((char *)sysib.sysib_322.ext_names[0], "TCGguest"); -- 2.18.4
Re: [PATCH 3/5] tcg/s390: Hoist common argument loads in tcg_out_op()
On Mon, Jan 11, 2021 at 04:01:12PM +0100, Philippe Mathieu-Daudé wrote: > Signed-off-by: Philippe Mathieu-Daudé > --- > tcg/s390/tcg-target.c.inc | 252 ++ > 1 file changed, 122 insertions(+), 130 deletions(-) > > diff --git a/tcg/s390/tcg-target.c.inc b/tcg/s390/tcg-target.c.inc > index d7ef0790556..74b2314c78a 100644 > --- a/tcg/s390/tcg-target.c.inc > +++ b/tcg/s390/tcg-target.c.inc > @@ -1732,15 +1732,23 @@ static void tcg_out_qemu_st(TCGContext* s, TCGReg > data_reg, TCGReg addr_reg, > case glue(glue(INDEX_op_,x),_i64) > > static inline void tcg_out_op(TCGContext *s, TCGOpcode opc, > -const TCGArg *args, const int *const_args) > + const TCGArg args[TCG_MAX_OP_ARGS], > + const int const_args[TCG_MAX_OP_ARGS]) > { > S390Opcode op, op2; > TCGArg a0, a1, a2; > +int c2, c3, c4; > + > +a0 = args[0]; > +a1 = args[1]; > +a2 = args[2]; > +c2 = const_args[2]; > +c3 = const_args[3]; > +c4 = const_args[4]; > Hi Philippe, why isn't c1 declared? You are using it in the code? > switch (opc) { > case INDEX_op_exit_tb: > /* Reuse the zeroing that exists for goto_ptr. */ > -a0 = args[0]; > if (a0 == 0) { > tgen_gotoi(s, S390_CC_ALWAYS, tcg_code_gen_epilogue); > } else { > @@ -1750,7 +1758,6 @@ static inline void tcg_out_op(TCGContext *s, TCGOpcode > opc, > break; > > case INDEX_op_goto_tb: > -a0 = args[0]; > if (s->tb_jmp_insn_offset) { > /* > * branch displacement must be aligned for atomic patching; > @@ -1784,7 +1791,6 @@ static inline void tcg_out_op(TCGContext *s, TCGOpcode > opc, > break; > > case INDEX_op_goto_ptr: > -a0 = args[0]; > if (USE_REG_TB) { > tcg_out_mov(s, TCG_TYPE_PTR, TCG_REG_TB, a0); > } > @@ -1794,45 +1800,43 @@ static inline void tcg_out_op(TCGContext *s, > TCGOpcode opc, > OP_32_64(ld8u): > /* ??? LLC (RXY format) is only present with the extended-immediate > facility, whereas LLGC is always present. */ > -tcg_out_mem(s, 0, RXY_LLGC, args[0], args[1], TCG_REG_NONE, args[2]); > +tcg_out_mem(s, 0, RXY_LLGC, a0, a1, TCG_REG_NONE, a2); > break; > > OP_32_64(ld8s): > /* ??? LB is no smaller than LGB, so no point to using it. */ > -tcg_out_mem(s, 0, RXY_LGB, args[0], args[1], TCG_REG_NONE, args[2]); > +tcg_out_mem(s, 0, RXY_LGB, a0, a1, TCG_REG_NONE, a2); > break; > > OP_32_64(ld16u): > /* ??? LLH (RXY format) is only present with the extended-immediate > facility, whereas LLGH is always present. */ > -tcg_out_mem(s, 0, RXY_LLGH, args[0], args[1], TCG_REG_NONE, args[2]); > +tcg_out_mem(s, 0, RXY_LLGH, a0, a1, TCG_REG_NONE, a2); > break; > > case INDEX_op_ld16s_i32: > -tcg_out_mem(s, RX_LH, RXY_LHY, args[0], args[1], TCG_REG_NONE, > args[2]); > +tcg_out_mem(s, RX_LH, RXY_LHY, a0, a1, TCG_REG_NONE, a2); > break; > > case INDEX_op_ld_i32: > -tcg_out_ld(s, TCG_TYPE_I32, args[0], args[1], args[2]); > +tcg_out_ld(s, TCG_TYPE_I32, a0, a1, a2); > break; > > OP_32_64(st8): > -tcg_out_mem(s, RX_STC, RXY_STCY, args[0], args[1], > -TCG_REG_NONE, args[2]); > +tcg_out_mem(s, RX_STC, RXY_STCY, a0, a1, TCG_REG_NONE, a2); > break; > > OP_32_64(st16): > -tcg_out_mem(s, RX_STH, RXY_STHY, args[0], args[1], > -TCG_REG_NONE, args[2]); > +tcg_out_mem(s, RX_STH, RXY_STHY, a0, a1, TCG_REG_NONE, a2); > break; > > case INDEX_op_st_i32: > -tcg_out_st(s, TCG_TYPE_I32, args[0], args[1], args[2]); > +tcg_out_st(s, TCG_TYPE_I32, a0, a1, a2); > break; > > case INDEX_op_add_i32: > -a0 = args[0], a1 = args[1], a2 = (int32_t)args[2]; > -if (const_args[2]) { > +a2 = (int32_t)a2; > +if (c2) { > do_addi_32: > if (a0 == a1) { > if (a2 == (int16_t)a2) { > @@ -1852,8 +1856,8 @@ static inline void tcg_out_op(TCGContext *s, TCGOpcode > opc, > } > break; > case INDEX_op_sub_i32: > -a0 = args[0], a1 = args[1], a2 = (int32_t)args[2]; > -if (const_args[2]) { > +a2 = (int32_t)a2; > +if (c2) { > a2 = -a2; > goto do_addi_32; > } else if (a0 == a1) { > @@ -1864,8 +1868,8 @@ static inline void tcg_out_op(TCGContext *s, TCGOpcode > opc, > break; > > case INDEX_op_and_i32: > -a0 = args[0], a1 = args[1], a2 = (uint32_t)args[2]; > -if (const_args[2]) { > +a2 = (uint32_t)a2; > +if (c2) { > tcg_out_mov(s, TCG_TYPE_I32, a0, a1); >
[PATCH v2 0/2] Fixing several GCC 11 warnings
Compiling qemu using GCC 11 we got several new warnings. To allow build with --enable-werror, we need to solve issues generating these warnings. Signed-of-by: Miroslav Rezanina v2: - Patch 2 rewrite to use strpadcpy - removed Patch 3 (different version sent by Philippe Mathieu-Daudé) Miroslav Rezanina (2): Fix net.c warning on GCC 11 s390x: Fix vm name copy length net/eth.c | 3 +++ target/s390x/kvm.c | 12 +--- target/s390x/misc_helper.c | 7 +-- 3 files changed, 13 insertions(+), 9 deletions(-) -- 2.18.4
[PATCH v2 1/2] Fix net.c warning on GCC 11
When building qemu with GCC 11, compiling eth.c file produce following warning: warning: array subscript 'struct ip6_ext_hdr_routing[0]' is partly outside array bounds of 'struct ip6_ext_hdr[1]' [-Warray-bounds] This caused by retyping from ip6_ext_hdr to ip6_ext_hdr_routing that has more attributes. As this usage is expected, suppress the warning temporarily through the function using this retyping. Signed-off-by: Miroslav Rezanina --- net/eth.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/net/eth.c b/net/eth.c index 1e0821c5f8..b9bdd0435c 100644 --- a/net/eth.c +++ b/net/eth.c @@ -405,6 +405,8 @@ _eth_get_rss_ex_dst_addr(const struct iovec *pkt, int pkt_frags, struct ip6_ext_hdr *ext_hdr, struct in6_address *dst_addr) { +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Warray-bounds" struct ip6_ext_hdr_routing *rthdr = (struct ip6_ext_hdr_routing *) ext_hdr; if ((rthdr->rtype == 2) && @@ -426,6 +428,7 @@ _eth_get_rss_ex_dst_addr(const struct iovec *pkt, int pkt_frags, } return false; +#pragma GCC diagnostic pop } static bool -- 2.18.4
[PATCH v2 2/2] s390x: Use strpadcpy for copying vm name
When using strncpy with lenght equal to size of target array, GCC 11 produce following warning: warning: '__builtin_strncpy' specified bound 256 equals destination size [-Wstringop-truncation] To prevent this warning, use strpadcpy function that will enusure that provide functionality similar to strncpy but allow copy strings with length equal size of target array (and discarding endint zero) and ensure array is zeroed after copied string. Signed-off-by: Miroslav Rezanina --- target/s390x/kvm.c | 12 +--- target/s390x/misc_helper.c | 7 +-- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c index b8385e6b95..dc27fa36c9 100644 --- a/target/s390x/kvm.c +++ b/target/s390x/kvm.c @@ -29,6 +29,7 @@ #include "internal.h" #include "kvm_s390x.h" #include "sysemu/kvm_int.h" +#include "qemu/cutils.h" #include "qapi/error.h" #include "qemu/error-report.h" #include "qemu/timer.h" @@ -1910,18 +1911,15 @@ static void insert_stsi_3_2_2(S390CPU *cpu, __u64 addr, uint8_t ar) strlen(qemu_name))); } sysib.vm[0].ext_name_encoding = 2; /* 2 = UTF-8 */ -memset(sysib.ext_names[0], 0, sizeof(sysib.ext_names[0])); /* If hypervisor specifies zero Extended Name in STSI322 SYSIB, it's * considered by s390 as not capable of providing any Extended Name. * Therefore if no name was specified on qemu invocation, we go with the * same "KVMguest" default, which KVM has filled into short name field. */ -if (qemu_name) { -strncpy((char *)sysib.ext_names[0], qemu_name, -sizeof(sysib.ext_names[0])); -} else { -strcpy((char *)sysib.ext_names[0], "KVMguest"); -} +strpadcpy((char *)sysib.ext_names[0], + sizeof(sysib.ext_names[0]), + qemu_name ?: "KVMguest", '\0'); + /* Insert UUID */ memcpy(sysib.vm[0].uuid, _uuid, sizeof(sysib.vm[0].uuid)); diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c index 58dbc023eb..7ea90d414a 100644 --- a/target/s390x/misc_helper.c +++ b/target/s390x/misc_helper.c @@ -19,6 +19,7 @@ */ #include "qemu/osdep.h" +#include "qemu/cutils.h" #include "qemu/main-loop.h" #include "cpu.h" #include "internal.h" @@ -369,8 +370,10 @@ uint32_t HELPER(stsi)(CPUS390XState *env, uint64_t a0, uint64_t r0, uint64_t r1) ebcdic_put(sysib.sysib_322.vm[0].name, qemu_name, MIN(sizeof(sysib.sysib_322.vm[0].name), strlen(qemu_name))); -strncpy((char *)sysib.sysib_322.ext_names[0], qemu_name, -sizeof(sysib.sysib_322.ext_names[0])); +strpadcpy((char *)sysib.sysib_322.ext_names[0], + sizeof(sysib.sysib_322.ext_names[0]), + qemu_name, '\0'); + } else { ebcdic_put(sysib.sysib_322.vm[0].name, "TCGguest", 8); strcpy((char *)sysib.sysib_322.ext_names[0], "TCGguest"); -- 2.18.4
Re: [RFC PATCH 4/5] tcg: Restrict tcg_out_op() to arrays of TCG_MAX_OP_ARGS elements
- Original Message - > From: "Philippe Mathieu-Daudé" > To: qemu-devel@nongnu.org > Cc: "Huacai Chen" , "Aurelien Jarno" > , "Aleksandar Rikalo" > , "Thomas Huth" , "Stefan > Weil" , > qemu-ri...@nongnu.org, qemu-...@nongnu.org, "Jiaxun Yang" > , qemu-s3...@nongnu.org, > "Philippe Mathieu-Daudé" , "Cornelia Huck" > , "Richard Henderson" > , "Andrzej Zaborowski" , > "Alistair Francis" > , "Palmer Dabbelt" , "Miroslav > Rezanina" > Sent: Monday, January 11, 2021 4:01:13 PM > Subject: [RFC PATCH 4/5] tcg: Restrict tcg_out_op() to arrays of > TCG_MAX_OP_ARGS elements > > tcg_reg_alloc_op() allocates arrays of TCG_MAX_OP_ARGS elements. > > The Aarch64 target already does this since commit 8d8db193f25 > ("tcg-aarch64: Hoist common argument loads in tcg_out_op"), > SPARC since commit b357f902bff ("tcg-sparc: Hoist common argument > loads in tcg_out_op"). > > RISCV missed it upon introduction in commit bdf503819ee > ("tcg/riscv: Add the out op decoder"), MIPS since commit > 22ee3a987d5 ("tcg-mips: Hoist args loads") and i386 since > commit 42d5b514928 ("tcg/i386: Hoist common arguments in > tcg_out_op"). > > Provide this information as a hint to the compiler in the function > prototype, and update the funtion definitions. > > This fixes this warning (using GCC 11): > > tcg/aarch64/tcg-target.c.inc:1855:37: error: argument 3 of type 'const > TCGArg[16]' {aka 'const long unsigned int[16]'} with mismatched bound > [-Werror=array-parameter=] > tcg/aarch64/tcg-target.c.inc:1856:34: error: argument 4 of type 'const > int[16]' with mismatched bound [-Werror=array-parameter=] > > Reported-by: Miroslav Rezanina > Signed-off-by: Philippe Mathieu-Daudé > --- > RFC because such compiler hint is somehow "new" to me. > > Also I expect this to be superseeded by Richard 'tcg constant' > branch mentioned here: > https://www.mail-archive.com/qemu-devel@nongnu.org/msg771401.html > --- > tcg/tcg.c | 5 +++-- > tcg/i386/tcg-target.c.inc | 3 ++- > tcg/mips/tcg-target.c.inc | 3 ++- > tcg/riscv/tcg-target.c.inc | 3 ++- > tcg/tci/tcg-target.c.inc | 5 +++-- > 5 files changed, 12 insertions(+), 7 deletions(-) > > diff --git a/tcg/tcg.c b/tcg/tcg.c > index 472bf1755bf..97d074d8fab 100644 > --- a/tcg/tcg.c > +++ b/tcg/tcg.c > @@ -110,8 +110,9 @@ static void tcg_out_ld(TCGContext *s, TCGType type, > TCGReg ret, TCGReg arg1, > static bool tcg_out_mov(TCGContext *s, TCGType type, TCGReg ret, TCGReg > arg); > static void tcg_out_movi(TCGContext *s, TCGType type, > TCGReg ret, tcg_target_long arg); > -static void tcg_out_op(TCGContext *s, TCGOpcode opc, const TCGArg *args, > - const int *const_args); > +static void tcg_out_op(TCGContext *s, TCGOpcode opc, > + const TCGArg args[TCG_MAX_OP_ARGS], > + const int const_args[TCG_MAX_OP_ARGS]); > #if TCG_TARGET_MAYBE_vec > static bool tcg_out_dup_vec(TCGContext *s, TCGType type, unsigned vece, > TCGReg dst, TCGReg src); > diff --git a/tcg/i386/tcg-target.c.inc b/tcg/i386/tcg-target.c.inc > index 46e856f4421..d121dca8789 100644 > --- a/tcg/i386/tcg-target.c.inc > +++ b/tcg/i386/tcg-target.c.inc > @@ -2215,7 +2215,8 @@ static void tcg_out_qemu_st(TCGContext *s, const TCGArg > *args, bool is64) > } > > static inline void tcg_out_op(TCGContext *s, TCGOpcode opc, > - const TCGArg *args, const int *const_args) > + const TCGArg args[TCG_MAX_OP_ARGS], > + const int const_args[TCG_MAX_OP_ARGS]) > { > TCGArg a0, a1, a2; > int c, const_a2, vexop, rexw = 0; > diff --git a/tcg/mips/tcg-target.c.inc b/tcg/mips/tcg-target.c.inc > index add157f6c32..b9bb54f0ecc 100644 > --- a/tcg/mips/tcg-target.c.inc > +++ b/tcg/mips/tcg-target.c.inc > @@ -1691,7 +1691,8 @@ static void tcg_out_clz(TCGContext *s, MIPSInsn opcv2, > MIPSInsn opcv6, > } > > static inline void tcg_out_op(TCGContext *s, TCGOpcode opc, > - const TCGArg *args, const int *const_args) > + const TCGArg args[TCG_MAX_OP_ARGS], > + const int const_args[TCG_MAX_OP_ARGS]) > { > MIPSInsn i1, i2; > TCGArg a0, a1, a2; > diff --git a/tcg/riscv/tcg-target.c.inc b/tcg/riscv/tcg-target.c.inc > index c60b91ba58f..5bf0d069532 100644 > --- a/tcg/riscv/tcg-targe
Re: [RHEL7 qemu-kvm PATCH 2/3] s390x: Fix vm name copy length
- Original Message - > From: "Christian Borntraeger" > To: "Thomas Huth" , "Miroslav Rezanina" > > Cc: "qemu-s390x" , "Philippe Mathieu-Daudé" > , qemu-devel@nongnu.org > Sent: Monday, January 11, 2021 2:02:32 PM > Subject: Re: [RHEL7 qemu-kvm PATCH 2/3] s390x: Fix vm name copy length > > > > On 11.01.21 13:54, Thomas Huth wrote: > > On 11/01/2021 13.42, Miroslav Rezanina wrote: > >> > >> > >> - Original Message - > >>> From: "Thomas Huth" > >>> To: "Philippe Mathieu-Daudé" , mreza...@redhat.com, > >>> qemu-devel@nongnu.org, "qemu-s390x" > >>> > >>> Sent: Monday, January 11, 2021 1:24:57 PM > >>> Subject: Re: [RHEL7 qemu-kvm PATCH 2/3] s390x: Fix vm name copy length > >>> > >>> On 11/01/2021 13.10, Philippe Mathieu-Daudé wrote: > >>>> Hi Miroslav, > >>>> > >>>> On 1/11/21 12:30 PM, mreza...@redhat.com wrote: > >>>>> From: Miroslav Rezanina > >>>>> > >>>>> There are two cases when vm name is copied but closing \0 can be lost > >>>>> in case name is too long (>=256 characters). > >>>>> > >>>>> Updating length to copy so there is space for closing \0. > >>>>> > >>>>> Signed-off-by: Miroslav Rezanina > >>>>> --- > >>>>> target/s390x/kvm.c | 2 +- > >>>>> target/s390x/misc_helper.c | 4 +++- > >>>>> 2 files changed, 4 insertions(+), 2 deletions(-) > >>>>> > >>>>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c > >>>>> index b8385e6b95..2313b5727e 100644 > >>>>> --- a/target/s390x/kvm.c > >>>>> +++ b/target/s390x/kvm.c > >>>>> @@ -1918,7 +1918,7 @@ static void insert_stsi_3_2_2(S390CPU *cpu, __u64 > >>>>> addr, uint8_t ar) > >>>>> */ > >>>>> if (qemu_name) { > >>>>> strncpy((char *)sysib.ext_names[0], qemu_name, > >>>>> - sizeof(sysib.ext_names[0])); > >>>>> + sizeof(sysib.ext_names[0]) - 1); > >>>>> } else { > >>>>> strcpy((char *)sysib.ext_names[0], "KVMguest"); > >>>>> } > >>>> > >>>> What about using strpadcpy() instead? > >>> > >>> Yes, strpadcpy is the better way here - this field has to be padded with > >>> zeroes, so doing "- 1" is wrong here. > >> > >> Hi Thomas, > >> > >> as I wrote in reply to Phillipe - the array is memset to zeroes before the > >> if so we > >> are sure it's padded with zeroes (in this occurrence, not true for second > >> one). > > > > Ok, but dropping the last character is still wrong here. The ext_names do > > not need to be terminated with a \0 if they have the full length. > The current code is actually correct. We are perfectly fine without the final > \n if the string is really 256 bytes. > > Replacing memset + strncpy with strpadcpy is certainly a good cleanup. Is it > necessary? No. Yes, it is necessary because otherwise compiler (GCC 11) produce warning and so build fail when --enable-werror is used. Mirek > > -- Miroslav Rezanina Software Engineer - Virtualization Team Maintainer
Re: [RHEL7 qemu-kvm PATCH 2/3] s390x: Fix vm name copy length
- Original Message - > From: "Thomas Huth" > To: "Miroslav Rezanina" > Cc: "qemu-s390x" , "Philippe Mathieu-Daudé" > , qemu-devel@nongnu.org > Sent: Monday, January 11, 2021 1:54:06 PM > Subject: Re: [RHEL7 qemu-kvm PATCH 2/3] s390x: Fix vm name copy length > > On 11/01/2021 13.42, Miroslav Rezanina wrote: > > > > > > - Original Message - > >> From: "Thomas Huth" > >> To: "Philippe Mathieu-Daudé" , mreza...@redhat.com, > >> qemu-devel@nongnu.org, "qemu-s390x" > >> > >> Sent: Monday, January 11, 2021 1:24:57 PM > >> Subject: Re: [RHEL7 qemu-kvm PATCH 2/3] s390x: Fix vm name copy length > >> > >> On 11/01/2021 13.10, Philippe Mathieu-Daudé wrote: > >>> Hi Miroslav, > >>> > >>> On 1/11/21 12:30 PM, mreza...@redhat.com wrote: > >>>> From: Miroslav Rezanina > >>>> > >>>> There are two cases when vm name is copied but closing \0 can be lost > >>>> in case name is too long (>=256 characters). > >>>> > >>>> Updating length to copy so there is space for closing \0. > >>>> > >>>> Signed-off-by: Miroslav Rezanina > >>>> --- > >>>>target/s390x/kvm.c | 2 +- > >>>>target/s390x/misc_helper.c | 4 +++- > >>>>2 files changed, 4 insertions(+), 2 deletions(-) > >>>> > >>>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c > >>>> index b8385e6b95..2313b5727e 100644 > >>>> --- a/target/s390x/kvm.c > >>>> +++ b/target/s390x/kvm.c > >>>> @@ -1918,7 +1918,7 @@ static void insert_stsi_3_2_2(S390CPU *cpu, __u64 > >>>> addr, uint8_t ar) > >>>> */ > >>>>if (qemu_name) { > >>>>strncpy((char *)sysib.ext_names[0], qemu_name, > >>>> -sizeof(sysib.ext_names[0])); > >>>> +sizeof(sysib.ext_names[0]) - 1); > >>>>} else { > >>>>strcpy((char *)sysib.ext_names[0], "KVMguest"); > >>>>} > >>> > >>> What about using strpadcpy() instead? > >> > >> Yes, strpadcpy is the better way here - this field has to be padded with > >> zeroes, so doing "- 1" is wrong here. > > > > Hi Thomas, > > > > as I wrote in reply to Phillipe - the array is memset to zeroes before the > > if so we > > are sure it's padded with zeroes (in this occurrence, not true for second > > one). > > Ok, but dropping the last character is still wrong here. The ext_names do > not need to be terminated with a \0 if they have the full length. Oh, they do not have to end with \0??? I did not know it. In that case we probably have to use strpadcpy to get rid of the compiler warning. Mirek > > Thomas > -- Miroslav Rezanina Software Engineer - Virtualization Team Maintainer
Re: [RHEL7 qemu-kvm PATCH 2/3] s390x: Fix vm name copy length
- Original Message - > From: "Philippe Mathieu-Daudé" > To: mreza...@redhat.com, qemu-devel@nongnu.org > Sent: Monday, January 11, 2021 1:10:07 PM > Subject: Re: [RHEL7 qemu-kvm PATCH 2/3] s390x: Fix vm name copy length > > Hi Miroslav, > > On 1/11/21 12:30 PM, mreza...@redhat.com wrote: > > From: Miroslav Rezanina > > > > There are two cases when vm name is copied but closing \0 can be lost > > in case name is too long (>=256 characters). > > > > Updating length to copy so there is space for closing \0. > > > > Signed-off-by: Miroslav Rezanina > > --- > > target/s390x/kvm.c | 2 +- > > target/s390x/misc_helper.c | 4 +++- > > 2 files changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c > > index b8385e6b95..2313b5727e 100644 > > --- a/target/s390x/kvm.c > > +++ b/target/s390x/kvm.c > > @@ -1918,7 +1918,7 @@ static void insert_stsi_3_2_2(S390CPU *cpu, __u64 > > addr, uint8_t ar) > > */ > > if (qemu_name) { > > strncpy((char *)sysib.ext_names[0], qemu_name, > > -sizeof(sysib.ext_names[0])); > > +sizeof(sysib.ext_names[0]) - 1); > > } else { > > strcpy((char *)sysib.ext_names[0], "KVMguest"); > > } > > What about using strpadcpy() instead? > > strpadcpy((char *)sysib.sysib_322.ext_names[0], > sizeof(sysib.sysib_322.ext_names[0]), > qemu_name ?: "KVMguest", '\0'); Hi Philippe, I went with -1 here because code use memset to 0 few lines above this code, so we now there are only zeroes in the array and we do not have to write them again. > > > diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c > > index 58dbc023eb..7c478b9e58 100644 > > --- a/target/s390x/misc_helper.c > > +++ b/target/s390x/misc_helper.c > > @@ -369,8 +369,10 @@ uint32_t HELPER(stsi)(CPUS390XState *env, uint64_t a0, > > uint64_t r0, uint64_t r1) > > ebcdic_put(sysib.sysib_322.vm[0].name, qemu_name, > > MIN(sizeof(sysib.sysib_322.vm[0].name), > > strlen(qemu_name))); > > + memset((char *)sysib.sysib_322.ext_names[0], 0, > > + sizeof(sysib.sysib_322.ext_names[0])); > > strncpy((char *)sysib.sysib_322.ext_names[0], qemu_name, > > -sizeof(sysib.sysib_322.ext_names[0])); > > +sizeof(sysib.sysib_322.ext_names[0]) - 1); > > And here: > >strpadcpy((char *)sysib.sysib_322.ext_names[0], > sizeof(sysib.sysib_322.ext_names[0]), > qemu_name, '\0'); However, here we are adding memset so using strpadcpy is better choice to ensure we have \0 after name string. Mirek > > > } else { > > ebcdic_put(sysib.sysib_322.vm[0].name, "TCGguest", 8); > > strcpy((char *)sysib.sysib_322.ext_names[0], "TCGguest"); > > > > -- Miroslav Rezanina Software Engineer - Virtualization Team Maintainer
Re: [RHEL7 qemu-kvm PATCH 3/3] Fix tcg_out_op argument mismatch warning
- Original Message - > From: "Philippe Mathieu-Daudé" > To: mreza...@redhat.com, qemu-devel@nongnu.org > Cc: "Richard Henderson" , "Eric Blake" > > Sent: Monday, January 11, 2021 1:15:04 PM > Subject: Re: [RHEL7 qemu-kvm PATCH 3/3] Fix tcg_out_op argument mismatch > warning > > On 1/11/21 12:30 PM, mreza...@redhat.com wrote: > > From: Miroslav Rezanina > > > > There's prototype mismatch between tcg/tcg.c and > > tcg/aarch/tcg-target.c.inc: > > > > tcg.c: > > > > static void tcg_out_op(TCGContext *s, TCGOpcode opc, const TCGArg > > *args, > >const int *const_args); > > > > tcg-target.c.inc: > > > > static void tcg_out_op(TCGContext *s, TCGOpcode opc, > >const TCGArg args[TCG_MAX_OP_ARGS], > >const int const_args[TCG_MAX_OP_ARGS]) > > > > This missmatch cause warnings on GCC 11: > > > > tcg/aarch64/tcg-target.c.inc:1855:37: error: argument 3 of type 'const > > TCGArg[16]' {aka 'const long unsigned int[16]'} with mismatched bound > > [-Werror=array-parameter=] > > tcg/aarch64/tcg-target.c.inc:1856:34: error: argument 4 of type 'const > > int[16]' with mismatched bound [-Werror=array-parameter=] > > TIL. Interesting, compilers are getting smarter :) > > > Only architectures with this definition are aarch and sparc. Fixing both > > archs to use > > proper argument type. > > --- > > tcg/aarch64/tcg-target.c.inc | 3 +-- > > tcg/sparc/tcg-target.c.inc | 3 +-- > > 2 files changed, 2 insertions(+), 4 deletions(-) > > > > diff --git a/tcg/aarch64/tcg-target.c.inc b/tcg/aarch64/tcg-target.c.inc > > index 26f71cb599..fe6bdbf721 100644 > > --- a/tcg/aarch64/tcg-target.c.inc > > +++ b/tcg/aarch64/tcg-target.c.inc > > @@ -1852,8 +1852,7 @@ static void tcg_out_qemu_st(TCGContext *s, TCGReg > > data_reg, TCGReg addr_reg, > > static tcg_insn_unit *tb_ret_addr; > > > > static void tcg_out_op(TCGContext *s, TCGOpcode opc, > > - const TCGArg args[TCG_MAX_OP_ARGS], > > - const int const_args[TCG_MAX_OP_ARGS]) > > + const TCGArg *args, const int *const_args) > > Doing this way we loose information (that the array pointed has > TCG_MAX_OP_ARGS elements). What about letting this prototype and > fix the other uses? I'm not author of the code so I went with smaller change - forward definition in tcg.c and most tct-target.c.inc use this form. I would need someone more familiar with this part to clarify whether it's ok to go with opposite change. Mirek > > > { > > /* 99% of the time, we can signal the use of extension registers > > by looking to see if the opcode handles 64-bit data. */ > > diff --git a/tcg/sparc/tcg-target.c.inc b/tcg/sparc/tcg-target.c.inc > > index 6775bd30fc..976f0f05af 100644 > > --- a/tcg/sparc/tcg-target.c.inc > > +++ b/tcg/sparc/tcg-target.c.inc > > @@ -1294,8 +1294,7 @@ static void tcg_out_qemu_st(TCGContext *s, TCGReg > > data, TCGReg addr, > > } > > > > static void tcg_out_op(TCGContext *s, TCGOpcode opc, > > - const TCGArg args[TCG_MAX_OP_ARGS], > > - const int const_args[TCG_MAX_OP_ARGS]) > > + const TCGArg *args, const int *const_args) > > { > > TCGArg a0, a1, a2; > > int c, c2; > > > > -- Miroslav Rezanina Software Engineer - Virtualization Team Maintainer
Re: [RHEL7 qemu-kvm PATCH 2/3] s390x: Fix vm name copy length
- Original Message - > From: "Thomas Huth" > To: "Philippe Mathieu-Daudé" , mreza...@redhat.com, > qemu-devel@nongnu.org, "qemu-s390x" > > Sent: Monday, January 11, 2021 1:24:57 PM > Subject: Re: [RHEL7 qemu-kvm PATCH 2/3] s390x: Fix vm name copy length > > On 11/01/2021 13.10, Philippe Mathieu-Daudé wrote: > > Hi Miroslav, > > > > On 1/11/21 12:30 PM, mreza...@redhat.com wrote: > >> From: Miroslav Rezanina > >> > >> There are two cases when vm name is copied but closing \0 can be lost > >> in case name is too long (>=256 characters). > >> > >> Updating length to copy so there is space for closing \0. > >> > >> Signed-off-by: Miroslav Rezanina > >> --- > >> target/s390x/kvm.c | 2 +- > >> target/s390x/misc_helper.c | 4 +++- > >> 2 files changed, 4 insertions(+), 2 deletions(-) > >> > >> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c > >> index b8385e6b95..2313b5727e 100644 > >> --- a/target/s390x/kvm.c > >> +++ b/target/s390x/kvm.c > >> @@ -1918,7 +1918,7 @@ static void insert_stsi_3_2_2(S390CPU *cpu, __u64 > >> addr, uint8_t ar) > >>*/ > >> if (qemu_name) { > >> strncpy((char *)sysib.ext_names[0], qemu_name, > >> -sizeof(sysib.ext_names[0])); > >> +sizeof(sysib.ext_names[0]) - 1); > >> } else { > >> strcpy((char *)sysib.ext_names[0], "KVMguest"); > >> } > > > > What about using strpadcpy() instead? > > Yes, strpadcpy is the better way here - this field has to be padded with > zeroes, so doing "- 1" is wrong here. Hi Thomas, as I wrote in reply to Phillipe - the array is memset to zeroes before the if so we are sure it's padded with zeroes (in this occurrence, not true for second one). Mirek > > Thomas > -- Miroslav Rezanina Software Engineer - Virtualization Team Maintainer
Re: [PATCH v2] trace: use STAP_SDT_V2 to work around symbol visibility
- Original Message - > From: "Stefan Hajnoczi" > To: qemu-devel@nongnu.org > Cc: "Peter Maydell" , "Stefan Hajnoczi" > , "Daniel P . Berrangé" > , wco...@redhat.com, f...@redhat.com, kra...@redhat.com, > rjo...@redhat.com, > mreza...@redhat.com, ddepa...@redhat.com > Sent: Thursday, November 19, 2020 12:27:04 PM > Subject: [PATCH v2] trace: use STAP_SDT_V2 to work around symbol visibility > > QEMU binaries no longer launch successfully with recent SystemTap > releases. This is because modular QEMU builds link the sdt semaphores > into the main binary instead of into the shared objects where they are > used. The symbol visibility of semaphores is 'hidden' and the dynamic > linker prints an error during module loading: > > $ ./configure --enable-trace-backends=dtrace --enable-modules ... > ... > Failed to open module: > /builddir/build/BUILD/qemu-4.2.0/s390x-softmmu/../block-curl.so: undefined > symbol: qemu_curl_close_semaphore > > The long-term solution is to generate per-module dtrace .o files and > link them into the module instead of the main binary. > > In the short term we can define STAP_SDT_V2 so dtrace(1) produces a .o > file with 'default' symbol visibility instead of 'hidden'. This > workaround is small and easier to merge for QEMU 5.2. > Thanks for this fix. Reviewed-by: Miroslav Rezanina > Cc: Daniel P. Berrangé > Cc: wco...@redhat.com > Cc: f...@redhat.com > Cc: kra...@redhat.com > Cc: rjo...@redhat.com > Cc: mreza...@redhat.com > Cc: ddepa...@redhat.com > Signed-off-by: Stefan Hajnoczi > --- > v2: > * Define STAP_SDT_V2 everywhere [danpb] > --- > configure | 1 + > trace/meson.build | 4 ++-- > 2 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/configure b/configure > index 714e75b5d8..5d91d49c7b 100755 > --- a/configure > +++ b/configure > @@ -4832,6 +4832,7 @@ if have_backend "dtrace"; then >trace_backend_stap="no" >if has 'stap' ; then > trace_backend_stap="yes" > +QEMU_CFLAGS="$QEMU_CFLAGS -DSTAP_SDT_V2" >fi > fi > > diff --git a/trace/meson.build b/trace/meson.build > index d5fc45c628..843ea14495 100644 > --- a/trace/meson.build > +++ b/trace/meson.build > @@ -38,13 +38,13 @@ foreach dir : [ '.' ] + trace_events_subdirs > trace_dtrace_h = custom_target(fmt.format('trace-dtrace', 'h'), > output: fmt.format('trace-dtrace', 'h'), > input: trace_dtrace, > - command: [ 'dtrace', '-o', '@OUTPUT@', > '-h', '-s', '@INPUT@' ]) > + command: [ 'dtrace', '-DSTAP_SDT_V2', > '-o', '@OUTPUT@', '-h', '-s', '@INPUT@' ]) > trace_ss.add(trace_dtrace_h) > if host_machine.system() != 'darwin' >trace_dtrace_o = custom_target(fmt.format('trace-dtrace', 'o'), > output: fmt.format('trace-dtrace', > 'o'), > input: trace_dtrace, > - command: [ 'dtrace', '-o', '@OUTPUT@', > '-G', '-s', '@INPUT@' ]) > + command: [ 'dtrace', '-DSTAP_SDT_V2', > '-o', '@OUTPUT@', '-G', '-s', '@INPUT@' ]) >trace_ss.add(trace_dtrace_o) > endif > > -- > 2.28.0 > > -- Miroslav Rezanina Software Engineer - Virtualization Team Maintainer
Re: [PATCH] hw/arm/virt: ARM_VIRT must select ARM_GIC
- Original Message - > From: "Andrew Jones" > To: "Peter Maydell" > Cc: "Miroslav Rezanina" , "QEMU Developers" > , "qemu-arm" > , "Philippe Mathieu-Daudé" > Sent: Wednesday, November 11, 2020 6:39:32 PM > Subject: Re: [PATCH] hw/arm/virt: ARM_VIRT must select ARM_GIC > > On Wed, Nov 11, 2020 at 04:00:25PM +, Peter Maydell wrote: > > On Wed, 11 Nov 2020 at 15:05, Miroslav Rezanina > > wrote: > > > > > > - Original Message - > > > > From: "Andrew Jones" > > > > To: qemu-devel@nongnu.org, qemu-...@nongnu.org > > > > Cc: "peter maydell" , phi...@redhat.com, > > > > "Miroslav Rezanina" > > > > Sent: Wednesday, November 11, 2020 3:34:40 PM > > > > Subject: [PATCH] hw/arm/virt: ARM_VIRT must select ARM_GIC > > > > > > > > The removal of the selection of A15MPCORE from ARM_VIRT also > > > > removed what A15MPCORE selects, ARM_GIC. We still need ARM_GIC. > > > > > > > > Problems with missing dependencies solved by this patch. > > > > > > Reviewed-by: Miroslav Rezanina > > > > This is the second of this kind of "missing select" bug I've > > seen recently. I don't suppose there's some kind of testing > > we could add to 'make check' that automatically catches them? > > > > Miroslav is finding them because the RHEL build of QEMU is more selective > than upstream as to what gets pulled in. Effort keeps going into making > upstream more configurable, but it's not quite there yet for RHEL's > purposes. I'm not sure how best to test something like this upstream. > I guess it would require the flexible configuration support we don't yet > have. > We use --without-default-devices and then add only supported devices - this allow us to find this kind of errors. To prevent them by test we would need complete matrix possible of device configurations (or just usable combinations). Not always is broken build wrong - it can be caused by unusable configuration. In this case, we have correct dependency - CONFIG_ARM_GIC_KVM requires CONFIG_KVM and CONFIG_ARM_GIC. In RC1 commit bec3c97e0cf9 (hw/arm/virt: Remove dependency on Cortex-A15 MPCore peripherals) removed indirect CONFIG_ARM_GIC from CONFIG_ARM_VIRT. So anyone using CONFIG_ARM_GIC_KVM with CONFIG_ARM_VIRT now gets build error. Question here is: Is okay to use CONFIG_ARM_GIC_KVM with CONFIG_ARM_VIRT without explicit CONFIG_ARM_GIC? I can imagine both answers: 1) yes - CONFIG_ARM_VIRT implicate presence of CONFIG_ARM_GIC so no need to add it explicitly 2) no - If you want to use CONFIG_ARM_GIC_KVM you have to always add CONFIG_ARM_GIC explicitly too I'm little bit confused now - why KConfig uses depends instead of select? If using option without dependent one is not possible shouldn't be logical to just get it? Mirek > Thanks, > drew > -- Miroslav Rezanina Software Engineer - Virtualization Team Maintainer
Re: [PATCH] hw/arm/virt: ARM_VIRT must select ARM_GIC
- Original Message - > From: "Andrew Jones" > To: qemu-devel@nongnu.org, qemu-...@nongnu.org > Cc: "peter maydell" , phi...@redhat.com, "Miroslav > Rezanina" > Sent: Wednesday, November 11, 2020 3:34:40 PM > Subject: [PATCH] hw/arm/virt: ARM_VIRT must select ARM_GIC > > The removal of the selection of A15MPCORE from ARM_VIRT also > removed what A15MPCORE selects, ARM_GIC. We still need ARM_GIC. > > Fixes: bec3c97e0cf9 ("hw/arm/virt: Remove dependency on Cortex-A15 MPCore > peripherals") > Reported-by: Miroslav Rezanina > Signed-off-by: Andrew Jones > --- > hw/arm/Kconfig | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig > index 7d022eeefdbc..e69a9009cf0a 100644 > --- a/hw/arm/Kconfig > +++ b/hw/arm/Kconfig > @@ -6,6 +6,7 @@ config ARM_VIRT > imply VFIO_PLATFORM > imply VFIO_XGMAC > imply TPM_TIS_SYSBUS > +select ARM_GIC > select ACPI > select ARM_SMMUV3 > select GPIO_KEY > -- > 2.26.2 > > Problems with missing dependencies solved by this patch. Reviewed-by: Miroslav Rezanina -- Miroslav Rezanina Software Engineer - Virtualization Team Maintainer
Re: [PATCH] build: fix recurse-all target
- Original Message - > From: "Paolo Bonzini" > To: qemu-devel@nongnu.org > Cc: th...@redhat.com, "Miroslav Rezanina" > Sent: Monday, August 31, 2020 2:56:45 PM > Subject: [PATCH] build: fix recurse-all target > > The missing "/all" suffix prevents the pc-bios/ parts of the build > from running. > > In the meanwhile, -Wall has moved from QEMU_CFLAGS to CFLAGS. Simplify > everything by not passing down CFLAGS, and add -Wall in the recursive > Makefiles. > > Reported-by: Miroslav Rezanina > Fixes: 5e6d1573b4 ("remove Makefile.target", 2020-08-21) > Signed-off-by: Paolo Bonzini > --- > Makefile | 4 ++-- > pc-bios/optionrom/Makefile | 8 ++-- > pc-bios/s390-ccw/Makefile | 3 ++- > 3 files changed, 6 insertions(+), 9 deletions(-) > > diff --git a/Makefile b/Makefile > index 27bf8156ec..7230f0f1f3 100644 > --- a/Makefile > +++ b/Makefile > @@ -186,10 +186,10 @@ ROM_DIRS_RULES=$(foreach t, all clean, $(addsuffix > /$(t), $(ROM_DIRS))) > # Only keep -O and -g cflags > .PHONY: $(ROM_DIRS_RULES) > $(ROM_DIRS_RULES): > - $(call quiet-command,$(MAKE) $(SUBDIR_MAKEFLAGS) -C $(dir $@) V="$(V)" > TARGET_DIR="$(dir $@)" CFLAGS="$(filter -O% -g%,$(CFLAGS))" $(notdir $@),) > + $(call quiet-command,$(MAKE) $(SUBDIR_MAKEFLAGS) -C $(dir $@) V="$(V)" > TARGET_DIR="$(dir $@)" $(notdir $@),) > > .PHONY: recurse-all recurse-clean > -recurse-all: $(ROM_DIRS) > +recurse-all: $(addsuffix /all, $(ROM_DIRS)) > recurse-clean: $(addsuffix /clean, $(ROM_DIRS)) > > ## > diff --git a/pc-bios/optionrom/Makefile b/pc-bios/optionrom/Makefile > index 51cb6ca9d8..6495802d9f 100644 > --- a/pc-bios/optionrom/Makefile > +++ b/pc-bios/optionrom/Makefile > @@ -8,15 +8,12 @@ all: multiboot.bin linuxboot.bin linuxboot_dma.bin > kvmvapic.bin pvh.bin > @true > > include ../../config-host.mak > +CFLAGS = -O2 -g > > quiet-command = $(if $(V),$1,$(if $(2),@printf " %-7s %s\n" $2 $3 && $1, > @$1)) > cc-option = $(if $(shell $(CC) $1 -S -o /dev/null -xc /dev/null >/dev/null > 2>&1 && echo OK), $1, $2) > > -# Compiling with no optimization creates ROMs that are too large > -ifeq ($(lastword $(filter -O%, -O0 $(CFLAGS))),-O0) > -override CFLAGS += -O2 > -endif > -override CFLAGS += -march=i486 > +override CFLAGS += -march=i486 -Wall > > # Flags for dependency generation > override CPPFLAGS += -MMD -MP -MT $@ -MF $(@D)/$(*F).d > @@ -42,7 +39,6 @@ Wa = -Wa, > override ASFLAGS += -32 > override CFLAGS += $(call cc-option, $(Wa)-32) > > - > LD_I386_EMULATION ?= elf_i386 > override LDFLAGS = -m $(LD_I386_EMULATION) -T $(SRC_DIR)/flat.lds > override LDFLAGS += $(LDFLAGS_NOPIE) > diff --git a/pc-bios/s390-ccw/Makefile b/pc-bios/s390-ccw/Makefile > index cc0f77baa6..3eb785048a 100644 > --- a/pc-bios/s390-ccw/Makefile > +++ b/pc-bios/s390-ccw/Makefile > @@ -3,6 +3,7 @@ all: build-all > @true > > include ../../config-host.mak > +CFLAGS = -O2 -g > > quiet-command = $(if $(V),$1,$(if $(2),@printf " %-7s %s\n" $2 $3 && $1, > @$1)) > cc-option = $(if $(shell $(CC) $1 -S -o /dev/null -xc /dev/null > /dev/null > \ > @@ -28,7 +29,7 @@ QEMU_DGFLAGS = -MMD -MP -MT $@ -MF $(@D)/$(*F).d > OBJECTS = start.o main.o bootmap.o jump2ipl.o sclp.o menu.o \ > virtio.o virtio-scsi.o virtio-blkdev.o libc.o cio.o dasd-ipl.o > > -QEMU_CFLAGS := $(filter -W%, $(QEMU_CFLAGS)) > +QEMU_CFLAGS := -Wall $(filter -W%, $(QEMU_CFLAGS)) > QEMU_CFLAGS += -ffreestanding -fno-delete-null-pointer-checks -msoft-float > QEMU_CFLAGS += -march=z900 -fPIE -fno-strict-aliasing > QEMU_CFLAGS += -fno-asynchronous-unwind-tables > -- > 2.26.2 > > Missing roms built with the fix. Reviewed-by: Miroslav Rezanina -- Miroslav Rezanina Software Engineer - Virtualization Team Maintainer
Re: [PATCH 1/3] gdbstub: prevent uninitialized warning
On Wed, Mar 25, 2020 at 05:21:35PM +0800, Chen Qun wrote: > According to the glib function requirements, we need initialise > the variable. Otherwise there will be compilation warnings: > > qemu/gdbstub.c: In function ‘handle_query_thread_extra’: > /usr/include/glib-2.0/glib/glib-autocleanups.h:28:3: warning: > ‘cpu_name’ may be used uninitialized in this function [-Wmaybe-uninitialized] >g_free (*pp); >^~~~ > > Reported-by: Euler Robot > Signed-off-by: Chen Qun > --- > Cc: "Alex Bennée" > Cc: "Philippe Mathieu-Daudé" > --- > gdbstub.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/gdbstub.c b/gdbstub.c > index 013fb1ac0f..171e150950 100644 > --- a/gdbstub.c > +++ b/gdbstub.c > @@ -2060,8 +2060,8 @@ static void handle_query_thread_extra(GdbCmdContext > *gdb_ctx, void *user_ctx) > /* Print the CPU model and name in multiprocess mode */ > ObjectClass *oc = object_get_class(OBJECT(cpu)); > const char *cpu_model = object_class_get_name(oc); > -g_autofree char *cpu_name; > -cpu_name = object_get_canonical_path_component(OBJECT(cpu)); > +g_autofree char *cpu_name = > +object_get_canonical_path_component(OBJECT(cpu)); > g_string_printf(rs, "%s %s [%s]", cpu_model, cpu_name, > cpu->halted ? "halted " : "running"); > } else { > -- > 2.23.0 > > > Fixing broken build with -Wall. Reviewed-by: Miroslav Rezanina
[PATCH] docs: Fix virtiofsd.1 location
Patch 6a7e2bbee5 docs: add virtiofsd(1) man page introduced new man page virtiofsd.1. Unfortunately, wrong file location is used as source for install command. This cause installation of docs fail. Fixing wrong location so installation is successful. Signed-off-by: Miroslav Rezanina --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index f0e1a2f..62a367d 100644 --- a/Makefile +++ b/Makefile @@ -865,7 +865,7 @@ ifdef CONFIG_VIRTFS $(INSTALL_DATA) $(MANUAL_BUILDDIR)/interop/virtfs-proxy-helper.1 "$(DESTDIR)$(mandir)/man1" endif ifeq ($(CONFIG_LINUX)$(CONFIG_SECCOMP)$(CONFIG_LIBCAP_NG),yyy) - $(INSTALL_DATA) docs/interop/virtiofsd.1 "$(DESTDIR)$(mandir)/man1" + $(INSTALL_DATA) $(MANUAL_BUILDDIR)/interop/virtiofsd.1 "$(DESTDIR)$(mandir)/man1" endif install-datadir: -- 1.8.3.1
Re: [PATCH] Makefile: add missing mkdir MANUAL_BUILDDIR
On Mon, Jan 20, 2020 at 04:34:00PM +, Stefan Hajnoczi wrote: > The MANUAL_BUILDDIR directory is automatically created by sphinx-build > for the other targets. The index.html target does not use sphinx-build > so we must manually create the directory to avoid the following error: > > GEN docs/built/index.html > /bin/sh: docs/built/index.html: No such file or directory > > Signed-off-by: Stefan Hajnoczi > --- > Makefile | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/Makefile b/Makefile > index afa5cb6548..6562b0dc97 100644 > --- a/Makefile > +++ b/Makefile > @@ -1022,6 +1022,7 @@ $(MANUAL_BUILDDIR)/interop/qemu-ga.8: $(call > manual-deps,interop) > $(call build-manual,interop,man) > > $(MANUAL_BUILDDIR)/index.html: $(SRC_PATH)/docs/index.html.in qemu-version.h > + @mkdir -p "$(MANUAL_BUILDDIR)" > $(call quiet-command, sed "s|@@VERSION@@|${VERSION}|g" $< >$@, \ > "GEN","$@") > > -- > 2.24.1 > Fixing our issues with build, Reviewed-by: Miroslav Rezanina
Re: [PATCH 2/2] aspeed/i2c: Prevent uninitialized warning
- Original Message - > From: "Cédric Le Goater" > To: "Thomas Huth" , mreza...@redhat.com, > qemu-devel@nongnu.org > Cc: "peter maydell" , "Andrew Jeffery" > , "Joel Stanley" , > qemu-triv...@nongnu.org > Sent: Tuesday, January 21, 2020 11:44:14 AM > Subject: Re: [PATCH 2/2] aspeed/i2c: Prevent uninitialized warning > > On 1/21/20 11:02 AM, Thomas Huth wrote: > > On 21/01/2020 10.28, mreza...@redhat.com wrote: > >> From: Miroslav Rezanina > >> > >> Compiler reports uninitialized warning for cmd_flags variable. > >> > >> Adding NULL initialization to prevent this warning. > >> > >> Signed-off-by: Miroslav Rezanina > >> --- > >> hw/i2c/aspeed_i2c.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c > >> index 2da04a4..445182a 100644 > >> --- a/hw/i2c/aspeed_i2c.c > >> +++ b/hw/i2c/aspeed_i2c.c > >> @@ -400,7 +400,7 @@ static bool aspeed_i2c_check_sram(AspeedI2CBus *bus) > >> > >> static void aspeed_i2c_bus_cmd_dump(AspeedI2CBus *bus) > >> { > >> -g_autofree char *cmd_flags; > >> +g_autofree char *cmd_flags = NULL; > >> uint32_t count; > >> > >> if (bus->cmd & (I2CD_RX_BUFF_ENABLE | I2CD_RX_BUFF_ENABLE)) { > > > > Reviewed-by: Thomas Huth > > > > ... maybe someone with enough Perl-foo (i.e. not me ;-)) should add a > > check to our check_patch.pl script so that it complains when new code is > > introduced that uses g_autofree without initializing the variable... > > weird. The cmd_flags variable is assigned just after and used > in a trace. > As g_autofree is used, variable has to be initialized otherwise will compiler complain even in the case we write to variable immediately after. Mirek > C. > > -- Miroslav Rezanina Software Engineer - Virtualization Team Maintainer
Re: [Qemu-devel] [PATCH] qemu-doc: Do not hard-code the name of the QEMU binary
- Original Message - > From: "Peter Maydell" > To: "Thomas Huth" > Cc: "QEMU Developers" , "Paolo Bonzini" > , "QEMU Trivial" > , "Miroslav Rezanina" , > "Eduardo Habkost" , > "Qemu-block" , "Richard Henderson" > Sent: Tuesday, January 14, 2020 3:34:22 PM > Subject: Re: [Qemu-devel] [PATCH] qemu-doc: Do not hard-code the name of the > QEMU binary > > On Wed, 28 Aug 2019 at 10:37, Thomas Huth wrote: > > > > In our documentation, we use a mix of "$QEMU", "qemu-system-i386" and > > "qemu-system-x86_64" when we give examples to the users how to run > > QEMU. Some more consistency would be good here. Also some distributions > > use different names for the QEMU binary (e.g. "qemu-kvm" in RHEL), so > > providing more flexibility here would also be good. Thus let's define > > some variables for the names of the QEMU command and use those in the > > documentation instead: @value{qemu_system} for generic examples, and > > @value{qemu_system_x86} for examples that only work with the x86 > > binaries. > > > > Signed-off-by: Thomas Huth > > --- > > docs/qemu-block-drivers.texi | 72 ++-- > > docs/qemu-cpu-models.texi| 10 +-- > > qemu-doc.texi| 81 +++--- > > qemu-options.hx | 128 +-- > > 4 files changed, 149 insertions(+), 142 deletions(-) > > > > diff --git a/docs/qemu-block-drivers.texi b/docs/qemu-block-drivers.texi > > index c02547e28c..2c7ea49c32 100644 > > --- a/docs/qemu-block-drivers.texi > > +++ b/docs/qemu-block-drivers.texi > > @@ -2,6 +2,8 @@ > > QEMU block driver reference manual > > @c man end > > > > +@set qemu_system qemu-system-x86_64 > > + > > @c man begin DESCRIPTION > > > > @node disk_images_formats > > @@ -405,7 +407,7 @@ QEMU can automatically create a virtual FAT disk image > > from a > > directory tree. In order to use it, just type: > > > > @example > > -qemu-system-i386 linux.img -hdb fat:/my_directory > > +@value{qemu_system} linux.img -hdb fat:/my_directory > > @end example > > Do you use the ability to change the text being substituted > here downstream ? If so, heads-up that I'm working on a > conversion of this texi file to rst. I'll put in a > similar rst substitution-reference, which will look like > this: > > .. |qemu_system| replace:: qemu-system-x86_64 > > but you'll need to update your downstream processes if > you're changing the value in the texi currently. > Yes we are using it and it make our changes much more simple. Mirek > thanks > -- PMM > > -- Miroslav Rezanina Software Engineer - Virtualization Team Maintainer
Re: [Qemu-devel] [PATCH v3] qemu-ga: Convert invocation documentation to rST
- Original Message - > From: "Peter Maydell" > To: "Miroslav Rezanina" > Cc: "QEMU Developers" , "Michael Roth" > > Sent: Friday, September 20, 2019 12:52:49 PM > Subject: Re: [Qemu-devel] [PATCH v3] qemu-ga: Convert invocation > documentation to rST > > On Fri, 20 Sep 2019 at 09:57, Miroslav Rezanina wrote: > > > > On Thu, Sep 05, 2019 at 02:10:40PM +0100, Peter Maydell wrote: > > > The qemu-ga documentation is currently in qemu-ga.texi in > > > Texinfo format, which we present to the user as: > > > * a qemu-ga manpage > > > * a section of the main qemu-doc HTML documentation > > > > > > Convert the documentation to rST format, and present it to > > > the user as: > > > * a qemu-ga manpage > > > * part of the interop/ Sphinx manual > > > > > > Signed-off-by: Peter Maydell > > > Reviewed-by: Michael Roth > > > Tested-by: Michael Roth > > > This patch is breaking build for me. It fails on: > > > > perl -Ww -- /builddir/build/BUILD/qemu-4.1.0/scripts/texi2pod.pl -I > > docs -I scripts -I docs/interop -DVERSION="4.1.50" > > -DCONFDIR="/etc/qemu-kvm" scripts/texi2pod.pl > > docs/interop/qemu-ga.8.pod && pod2man --utf8 --section=8 --center=" " > > --release=" " docs/interop/qemu-ga.8.pod > docs/interop/qemu-ga.8 > > > > with: > > > >No filename or title > > > > Not sure why this is happening. > > Yes, it's a bug in the handling of in-tree builds (out-of-tree > builds work fine and we generally recommend those as better anyway). > I have a patch on list which should fix the in-tree build case: > > https://patchew.org/QEMU/20190919155957.12618-1-peter.mayd...@linaro.org/ > Thanks for info, Mirek > thanks > -- PMM > -- Miroslav Rezanina Software Engineer - Virtualization Team Maintainer
Re: [Qemu-devel] [PATCH v3] qemu-ga: Convert invocation documentation to rST
de 100644 > index f00ad830f28..000 > --- a/qemu-ga.texi > +++ /dev/null > @@ -1,137 +0,0 @@ > -@example > -@c man begin SYNOPSIS > -@command{qemu-ga} [@var{OPTIONS}] > -@c man end > -@end example > - > -@c man begin DESCRIPTION > - > -The QEMU Guest Agent is a daemon intended to be run within virtual > -machines. It allows the hypervisor host to perform various operations > -in the guest, such as: > - > -@itemize > -@item > -get information from the guest > -@item > -set the guest's system time > -@item > -read/write a file > -@item > -sync and freeze the filesystems > -@item > -suspend the guest > -@item > -reconfigure guest local processors > -@item > -set user's password > -@item > -... > -@end itemize > - > -qemu-ga will read a system configuration file on startup (located at > -@file{@value{CONFDIR}/qemu-ga.conf} by default), then parse remaining > -configuration options on the command line. For the same key, the last > -option wins, but the lists accumulate (see below for configuration > -file format). > - > -@c man end > - > -@c man begin OPTIONS > -@table @option > -@item -m, --method=@var{method} > - Transport method: one of @samp{unix-listen}, @samp{virtio-serial}, or > - @samp{isa-serial} (@samp{virtio-serial} is the default). > - > -@item -p, --path=@var{path} > - Device/socket path (the default for virtio-serial is > - @samp{/dev/virtio-ports/org.qemu.guest_agent.0}, > - the default for isa-serial is @samp{/dev/ttyS0}) > - > -@item -l, --logfile=@var{path} > - Set log file path (default is stderr). > - > -@item -f, --pidfile=@var{path} > - Specify pid file (default is @samp{/var/run/qemu-ga.pid}). > - > -@item -F, --fsfreeze-hook=@var{path} > - Enable fsfreeze hook. Accepts an optional argument that specifies > - script to run on freeze/thaw. Script will be called with > - 'freeze'/'thaw' arguments accordingly (default is > - @samp{@value{CONFDIR}/fsfreeze-hook}). If using -F with an argument, do > - not follow -F with a space (for example: > - @samp{-F/var/run/fsfreezehook.sh}). > - > -@item -t, --statedir=@var{path} > - Specify the directory to store state information (absolute paths only, > - default is @samp{/var/run}). > - > -@item -v, --verbose > - Log extra debugging information. > - > -@item -V, --version > - Print version information and exit. > - > -@item -d, --daemon > - Daemonize after startup (detach from terminal). > - > -@item -b, --blacklist=@var{list} > - Comma-separated list of RPCs to disable (no spaces, @samp{?} to list > - available RPCs). > - > -@item -D, --dump-conf > - Dump the configuration in a format compatible with @file{qemu-ga.conf} > - and exit. > - > -@item -h, --help > - Display this help and exit. > -@end table > - > -@c man end > - > -@c man begin FILES > - > -The syntax of the @file{qemu-ga.conf} configuration file follows the > -Desktop Entry Specification, here is a quick summary: it consists of > -groups of key-value pairs, interspersed with comments. > - > -@example > -# qemu-ga configuration sample > -[general] > -daemonize = 0 > -pidfile = /var/run/qemu-ga.pid > -verbose = 0 > -method = virtio-serial > -path = /dev/virtio-ports/org.qemu.guest_agent.0 > -statedir = /var/run > -@end example > - > -The list of keys follows the command line options: > -@table @option > -@item daemon= boolean > -@item method= string > -@item path= string > -@item logfile= string > -@item pidfile= string > -@item fsfreeze-hook= string > -@item statedir= string > -@item verbose= boolean > -@item blacklist= string list > -@end table > - > -@c man end > - > -@ignore > - > -@setfilename qemu-ga > -@settitle QEMU Guest Agent > - > -@c man begin AUTHOR > -Michael Roth > -@c man end > - > -@c man begin SEEALSO > -qemu(1) > -@c man end > - > -@end ignore > -- > 2.20.1 > > This patch is breaking build for me. It fails on: perl -Ww -- /builddir/build/BUILD/qemu-4.1.0/scripts/texi2pod.pl -I docs -I scripts -I docs/interop -DVERSION="4.1.50" -DCONFDIR="/etc/qemu-kvm" scripts/texi2pod.pl docs/interop/qemu-ga.8.pod && pod2man --utf8 --section=8 --center=" " --release=" " docs/interop/qemu-ga.8.pod > docs/interop/qemu-ga.8 with: No filename or title Not sure why this is happening. Miroslav Rezanina
Re: [Qemu-devel] [PATCH] qemu-doc: Do not hard-code the name of the QEMU binary
ample > #launch a QEMU instance with two NICs, each one connected > #to a TAP device > -qemu-system-i386 linux.img \ > +@value{qemu_system} linux.img \ > -netdev tap,id=nd0,ifname=tap0 -device e1000,netdev=nd0 \ > -netdev tap,id=nd1,ifname=tap1 -device rtl8139,netdev=nd1 > @end example > @@ -2481,7 +2481,7 @@ qemu-system-i386 linux.img \ > @example > #launch a QEMU instance with the default network helper to > #connect a TAP device to bridge br0 > -qemu-system-i386 linux.img -device virtio-net-pci,netdev=n1 \ > +@value{qemu_system} linux.img -device virtio-net-pci,netdev=n1 \ > -netdev tap,id=n1,"helper=/path/to/qemu-bridge-helper" > @end example > > @@ -2498,13 +2498,13 @@ Examples: > @example > #launch a QEMU instance with the default network helper to > #connect a TAP device to bridge br0 > -qemu-system-i386 linux.img -netdev bridge,id=n1 -device virtio-net,netdev=n1 > +@value{qemu_system} linux.img -netdev bridge,id=n1 -device > virtio-net,netdev=n1 > @end example > > @example > #launch a QEMU instance with the default network helper to > #connect a TAP device to bridge qemubr0 > -qemu-system-i386 linux.img -netdev bridge,br=qemubr0,id=n1 -device > virtio-net,netdev=n1 > +@value{qemu_system} linux.img -netdev bridge,br=qemubr0,id=n1 -device > virtio-net,netdev=n1 > @end example > > @item -netdev > > socket,id=@var{id}[,fd=@var{h}][,listen=[@var{host}]:@var{port}][,connect=@var{host}:@var{port}] > @@ -2519,11 +2519,11 @@ specifies an already opened TCP socket. > Example: > @example > # launch a first QEMU instance > -qemu-system-i386 linux.img \ > +@value{qemu_system} linux.img \ > -device e1000,netdev=n1,mac=52:54:00:12:34:56 \ > -netdev socket,id=n1,listen=:1234 > # connect the network of this instance to the network of the first instance > -qemu-system-i386 linux.img \ > +@value{qemu_system} linux.img \ > -device e1000,netdev=n2,mac=52:54:00:12:34:57 \ > -netdev socket,id=n2,connect=127.0.0.1:1234 > @end example > @@ -2548,15 +2548,15 @@ Use @option{fd=h} to specify an already opened UDP > multicast socket. > Example: > @example > # launch one QEMU instance > -qemu-system-i386 linux.img \ > +@value{qemu_system} linux.img \ > -device e1000,netdev=n1,mac=52:54:00:12:34:56 \ > -netdev socket,id=n1,mcast=230.0.0.1:1234 > # launch another QEMU instance on same "bus" > -qemu-system-i386 linux.img \ > +@value{qemu_system} linux.img \ > -device e1000,netdev=n2,mac=52:54:00:12:34:57 \ > -netdev socket,id=n2,mcast=230.0.0.1:1234 > # launch yet another QEMU instance on same "bus" > -qemu-system-i386 linux.img \ > +@value{qemu_system} linux.img \ > -device e1000,netdev=n3,mac=52:54:00:12:34:58 \ > -netdev socket,id=n3,mcast=230.0.0.1:1234 > @end example > @@ -2564,7 +2564,7 @@ qemu-system-i386 linux.img \ > Example (User Mode Linux compat.): > @example > # launch QEMU instance (note mcast address selected is UML's default) > -qemu-system-i386 linux.img \ > +@value{qemu_system} linux.img \ > -device e1000,netdev=n1,mac=52:54:00:12:34:56 \ > -netdev socket,id=n1,mcast=239.192.168.1:1102 > # launch UML > @@ -2573,7 +2573,7 @@ qemu-system-i386 linux.img \ > > Example (send packets from host's 1.2.3.4): > @example > -qemu-system-i386 linux.img \ > +@value{qemu_system} linux.img \ > -device e1000,netdev=n1,mac=52:54:00:12:34:56 \ > -netdev > socket,id=n1,mcast=239.192.168.1:1102,localaddr=1.2.3.4 > @end example > @@ -2633,7 +2633,7 @@ brctl addif br-lan vmtunnel0 > # on 4.3.2.1 > # launch QEMU instance - if your network has reorder or is very lossy add > ,pincounter > > -qemu-system-i386 linux.img -device e1000,netdev=n1 \ > +@value{qemu_system} linux.img -device e1000,netdev=n1 \ > -netdev > > l2tpv3,id=n1,src=4.2.3.1,dst=1.2.3.4,udp,srcport=16384,dstport=16384,rxsession=0x,txsession=0x,counter > > @end example > @@ -2650,7 +2650,7 @@ Example: > # launch vde switch > vde_switch -F -sock /tmp/myswitch > # launch QEMU instance > -qemu-system-i386 linux.img -nic vde,sock=/tmp/myswitch > +@value{qemu_system} linux.img -nic vde,sock=/tmp/myswitch > @end example > > @item -netdev vhost-user,chardev=@var{id}[,vhostforce=on|off][,queues=n] > @@ -3107,7 +3107,7 @@ and communicate. Requires the Linux @code{vhci} driver > installed. Can > be used as following: > > @example > -qemu-system-i386 [...OPTIONS...] -bt hci,vlan=5 -bt vhci,vlan=5 > +@value{qemu_system} [...OPTIONS...] -bt hci,vlan=5 -bt vhci,vlan=5 > @end example > > @item -bt device:@var{dev}[,vlan=@var{n}] > @@ -3601,7 +3601,7 @@ connections will likely be TCP-based, but also UDP, > pseudo TTY, or even > stdio are reasonable use case. The latter is allowing to start QEMU from > within gdb and establish the connection via a pipe: > @example > -(gdb) target remote | exec qemu-system-i386 -gdb stdio ... > +(gdb) target remote | exec @value{qemu_system} -gdb stdio ... > @end example > ETEXI > > @@ -4571,7 +4571,7 @@ which specify the queue number of cryptodev backend, > the default of > > @example > > - # qemu-system-x86_64 \ > + # @value{qemu_system} \ > [...] \ > -object cryptodev-backend-builtin,id=cryptodev0 \ > -device virtio-crypto-pci,id=crypto0,cryptodev=cryptodev0 \ > @@ -4591,7 +4591,7 @@ of cryptodev backend for multiqueue vhost-user, the > default of @var{queues} is 1 > > @example > > - # qemu-system-x86_64 \ > + # @value{qemu_system} \ > [...] \ > -chardev socket,id=chardev0,path=/path/to/socket \ > -object cryptodev-vhost-user,id=cryptodev0,chardev=chardev0 \ > @@ -4627,14 +4627,14 @@ The simplest (insecure) usage is to provide the > secret inline > > @example > > - # $QEMU -object secret,id=sec0,data=letmein,format=raw > + # @value{qemu_system} -object secret,id=sec0,data=letmein,format=raw > > @end example > > The simplest secure usage is to provide the secret via a file > > # printf "letmein" > mypasswd.txt > - # $QEMU -object secret,id=sec0,file=mypasswd.txt,format=raw > + # @value{qemu_system} -object secret,id=sec0,file=mypasswd.txt,format=raw > > For greater security, AES-256-CBC should be used. To illustrate usage, > consider the openssl command line tool which can encrypt the data. Note > @@ -4670,7 +4670,7 @@ and specify that to be used to decrypt the user > password. Pass the > contents of @code{iv.b64} to the second secret > > @example > - # $QEMU \ > + # @value{qemu_system} \ > -object secret,id=secmaster0,format=base64,file=key.b64 \ > -object secret,id=sec0,keyid=secmaster0,format=base64,\ > data=$SECRET,iv=$( @@ -4713,7 +4713,7 @@ negotiate keys used for attestation. The file must be > encoded in base64. > > e.g to launch a SEV guest > @example > - # $QEMU \ > + # @value{qemu_system_x86} \ > .. > -object sev-guest,id=sev0,cbitpos=47,reduced-phys-bits=5 \ > -machine ...,memory-encryption=sev0 > @@ -4735,7 +4735,7 @@ any commas in the distinguished name. > An example authorization object to validate a x509 distinguished name > would look like: > @example > - # $QEMU \ > + # @value{qemu_system} \ > ... > -object > 'authz-simple,id=auth0,identity=CN=laptop.example.com,,O=Example > Org,,L=London,,ST=London,,C=GB' \ > ... > @@ -4784,7 +4784,7 @@ a TLS x509 distinguished name, or a SASL username. > An example authorization object to validate a SASL username > would look like: > @example > - # $QEMU \ > + # @value{qemu_system} \ > ... > -object > authz-simple,id=auth0,filename=/etc/qemu/vnc-sasl.acl,refresh=yes > ... > @@ -4802,7 +4802,7 @@ An example authorization object to validate a TLS x509 > distinguished > name would look like: > > @example > - # $QEMU \ > + # @value{qemu_system} \ > ... > -object authz-pam,id=auth0,service=qemu-vnc > ... > -- > 2.18.1 > > Useful change. Reviewed-by: Miroslav Rezanina -- Miroslav Rezanina Software Engineer - Virtualization Team Maintainer
Re: [Qemu-devel] [PATCH] modules-test: fix const cast
On Thu, Aug 22, 2019 at 07:42:13PM +0200, Paolo Bonzini wrote: > Signed-off-by: Paolo Bonzini > --- > tests/modules-test.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tests/modules-test.c b/tests/modules-test.c > index 3aef0e5..a8118e9 100644 > --- a/tests/modules-test.c > +++ b/tests/modules-test.c > @@ -4,7 +4,7 @@ > static void test_modules_load(const void *data) > { > QTestState *qts; > -const char **args = data; > +const char **args = (const char **)data; > > qts = qtest_init(NULL); > qtest_module_load(qts, args[0], args[1]); > -- > 1.8.3.1 > > Reviewed-by: Miroslav Rezanina
Re: [Qemu-devel] [PATCH] modules-test: ui-spice-app is not built as module
On Thu, Aug 22, 2019 at 07:42:14PM +0200, Paolo Bonzini wrote: > $(call land, $(CONFIG_SPICE), $(CONFIG_GIO)) will never return "m" so > ui-spice-app is always linked into QEMU. > > Signed-off-by: Paolo Bonzini > --- > tests/modules-test.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/tests/modules-test.c b/tests/modules-test.c > index a8118e9..f9de3af 100644 > --- a/tests/modules-test.c > +++ b/tests/modules-test.c > @@ -53,9 +53,6 @@ int main(int argc, char *argv[]) > #ifdef CONFIG_SDL > "ui-", "sdl", > #endif > -#if defined(CONFIG_SPICE) && defined(CONFIG_GIO) > -"ui-", "spice-app", > -#endif > }; > int i; > > -- > 1.8.3.1 > > Making make check pass again for me. Reviewed-by: Miroslav Rezanina
Re: [Qemu-devel] [RFC PATCH 0/2] target: Build with CONFIG_SEMIHOSTING disabled
- Original Message - > From: "Philippe Mathieu-Daudé" > To: "Peter Maydell" > Cc: "QEMU Developers" , "Paolo Bonzini" > , "Miroslav Rezanina" > , "Richard Henderson" , > "Aleksandar Rikalo" > , "qemu-arm" , "Aleksandar > Markovic" , "Aurelien > Jarno" , "Alex Bennée" , > "Samuel Ortiz" , "Rob > Bradford" > Sent: Friday, May 31, 2019 6:40:30 PM > Subject: Re: [RFC PATCH 0/2] target: Build with CONFIG_SEMIHOSTING disabled > > On 5/31/19 6:21 PM, Peter Maydell wrote: > > On Fri, 31 May 2019 at 16:47, Philippe Mathieu-Daudé > > wrote: > >> > >> Amusingly Miroslav and myself hit this issue at the same time. > >> > >> Currently there is no way to pass a CONFIG_X to sources in target/, > >> except via a Makefile rule (and filling with stubs). > >> > >> Paolo says this is on purpose, CONFIG_X selectors are meant for > >> devices and we try to avoid having config-devices.mak in > >> config-target.h. > > > > ...but some things in target/ are devices (like the Arm CPUs, > > which inherit from TYPE_DEVICE). > > > > Is there a way we can have a Kconfig fragment that expresses > > "if you asked for an Arm CPU then this should 'select SEMIHOSTING'" ? > > Yes, but inversely, we can also deselect a feature, and all features > that requires it get deselected. My guess is Miroslav is building a > KVM-only QEMU, but upstream does not allow to build ARM without TCG. > > I'll see what happened to Samuel series "Support disabling TCG on ARM" > and see if it can be salvaged: > https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg02451.html > > I suppose in this thread: > https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg05641.html > you refer to this series (not yet merged): > https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg03137.html > > I'll try to figure what "KVM injection of interrupts" is. > What about CONFIG_ARM_VIRT - can we use it to introduce dependency on CONFIG_SEMIHOSTING or is there valid scenario of qemu build with CONFIG_ARM_VIRT enabled and CONFIG_SEMIHOSTING disabled? Mirek -- Miroslav Rezanina Software Engineer - Virtualization Team Maintainer
Re: [Qemu-devel] [RFC PATCH 06/11] target/arm: use the common interface for WRITE0/WRITEC in arm-semi
- Original Message - > From: "Alex Bennée" > To: "Miroslav Rezanina" > Cc: qemu-devel@nongnu.org, "Peter Maydell" , "Riku > Voipio" , > qemu-...@nongnu.org, "Laurent Vivier" > Sent: Friday, May 31, 2019 4:28:02 PM > Subject: Re: [Qemu-devel] [RFC PATCH 06/11] target/arm: use the common > interface for WRITE0/WRITEC in arm-semi > > > Miroslav Rezanina writes: > > > - Original Message - > >> From: "Alex Bennée" > >> To: "Miroslav Rezanina" > >> Cc: qemu-devel@nongnu.org, "Peter Maydell" , > >> "Riku Voipio" , > >> qemu-...@nongnu.org, "Laurent Vivier" > >> Sent: Friday, May 31, 2019 3:16:38 PM > >> Subject: Re: [Qemu-devel] [RFC PATCH 06/11] target/arm: use the common > >> interface for WRITE0/WRITEC in arm-semi > >> > >> > >> Miroslav Rezanina writes: > >> > >> > - Original Message - > >> >> From: "Alex Bennée" > >> >> To: "Miroslav Rezanina" > >> >> Cc: qemu-devel@nongnu.org, "Peter Maydell" , > >> >> "Riku Voipio" , > >> >> qemu-...@nongnu.org, "Laurent Vivier" > >> >> Sent: Friday, May 31, 2019 1:08:06 PM > >> >> Subject: Re: [Qemu-devel] [RFC PATCH 06/11] target/arm: use the common > >> >> interface for WRITE0/WRITEC in arm-semi > >> >> > >> >> > >> >> Miroslav Rezanina writes: > >> >> > >> >> > On Tue, May 14, 2019 at 04:52:56PM +0100, Alex Bennée wrote: > >> >> >> Now we have a common semihosting console interface use that for our > >> >> >> string output. However ARM is currently unique in also supporting > >> >> >> semihosting for linux-user so we need to replicate the API in > >> >> >> linux-user. If other architectures gain this support we can move the > >> >> >> file later. > >> >> >> > >> >> >> Signed-off-by: Alex Bennée > >> >> >> --- > >> >> >> linux-user/Makefile.objs | 2 ++ > >> >> >> linux-user/arm/semihost.c | 24 > >> >> >> target/arm/arm-semi.c | 31 ++- > >> >> >> 3 files changed, 32 insertions(+), 25 deletions(-) > >> >> >> create mode 100644 linux-user/arm/semihost.c > >> >> >> > >> >> >> diff --git a/linux-user/Makefile.objs b/linux-user/Makefile.objs > >> >> >> index 769b8d83362..285c5dfa17a 100644 > >> >> >> --- a/linux-user/Makefile.objs > >> >> >> +++ b/linux-user/Makefile.objs > >> >> >> @@ -6,4 +6,6 @@ obj-y = main.o syscall.o strace.o mmap.o signal.o \ > >> >> >> obj-$(TARGET_HAS_BFLT) += flatload.o > >> >> >> obj-$(TARGET_I386) += vm86.o > >> >> >> obj-$(TARGET_ARM) += arm/nwfpe/ > >> >> >> +obj-$(TARGET_ARM) += arm/semihost.o > >> >> >> +obj-$(TARGET_AARCH64) += arm/semihost.o > >> >> >> obj-$(TARGET_M68K) += m68k-sim.o > >> >> >> diff --git a/linux-user/arm/semihost.c b/linux-user/arm/semihost.c > >> >> >> new file mode 100644 > >> >> >> index 000..9554102a855 > >> >> >> --- /dev/null > >> >> >> +++ b/linux-user/arm/semihost.c > >> >> >> @@ -0,0 +1,24 @@ > >> >> >> +/* > >> >> >> + * ARM Semihosting Console Support > >> >> >> + * > >> >> >> + * Copyright (c) 2019 Linaro Ltd > >> >> >> + * > >> >> >> + * Currently ARM is unique in having support for semihosting > >> >> >> support > >> >> >> + * in linux-user. So for now we implement the common console API > >> >> >> but > >> >> >> + * just for arm linux-user. > >> >> >> + * > >> >> >> + * SPDX-License-Identifier: GPL-2.0-or-later > >> >> >> + */ > >> >> >> + > >> >> >> +#include "qemu/osdep.h" > >> >> >> +#include "cpu.h" > >> >> >> +#include "hw/semihosting/console.h" > &
Re: [Qemu-devel] [RFC PATCH 06/11] target/arm: use the common interface for WRITE0/WRITEC in arm-semi
- Original Message - > From: "Peter Maydell" > To: "Alex Bennée" > Cc: "Miroslav Rezanina" , "QEMU Developers" > , "Riku Voipio" > , "qemu-arm" , "Laurent Vivier" > > Sent: Friday, May 31, 2019 4:38:04 PM > Subject: Re: [Qemu-devel] [RFC PATCH 06/11] target/arm: use the common > interface for WRITE0/WRITEC in arm-semi > > On Fri, 31 May 2019 at 15:28, Alex Bennée wrote: > > Miroslav Rezanina writes: > > >From: "Alex Bennée" > > >> OK - so from the upstream source tree CONFIG_SEMIHOSTING is still =y > > >> (but I can see most of them are now =n). Isn't the simplest solution to > > >> fix-up your version of the default_config file to include SEMIHOSTING? > > >> > > > > > > It's fix but it goes against our policy of handling CONFIG options so we > > > would prefer to have this fixed - otherwise there's no meaning in having > > > config option if you can't disable it. > > > > Is that what it means? For my part it means we don't build in > > CONFIG_SEMIHOSTING for the arches that don't need it (which we were > > before). Granted it only really simplified the vl.c parsing and dropped > > support for semihosting for the linux-user targets (except ARM). > > Yes, that would be my interpretation of it. If we had > a 'config FOO' stanza for CPUs, then Arm CPUs would > "select SEMIHOSTING". If RedHat would like it to be possible > to build Arm CPUs without CONFIG_SEMIHOSTING then they're > free to submit patches for that, but that's a new feature > upstream doesn't currently support, not a bug in upstream. > (Also I'd be a bit dubious because it means that previously working > guest setups that use semihosting will break.) I partially agree here - I see difference between disabling config and omitting it. We are not not disabling CONFIG_SEMIHOSTING, we just ignore it. So we got error because it is not properly handled. Proper handling should be either auto-include it as dependency or successful build with option disabled. As there's currently no way to auto-include it through dependency, it would be good to have comment in default_config file next to it stating that it's required option. This will allow us to see it and add to our default_config we used instead upstream one. Mirek > > PS: if we had a 'config FOO' stanza for CPUs that would also > allow us to say "building Arm CPUs requires the NVIC" and > similarly for things which in QEMU are devices but which are > architecturally tightly-coupled non-optional parts of the CPU. > > thanks > -- PMM > -- Miroslav Rezanina Software Engineer - Virtualization Team Maintainer
Re: [Qemu-devel] [RFC PATCH 06/11] target/arm: use the common interface for WRITE0/WRITEC in arm-semi
- Original Message - > From: "Alex Bennée" > To: "Miroslav Rezanina" > Cc: qemu-devel@nongnu.org, "Peter Maydell" , "Riku > Voipio" , > qemu-...@nongnu.org, "Laurent Vivier" > Sent: Friday, May 31, 2019 3:16:38 PM > Subject: Re: [Qemu-devel] [RFC PATCH 06/11] target/arm: use the common > interface for WRITE0/WRITEC in arm-semi > > > Miroslav Rezanina writes: > > > - Original Message - > >> From: "Alex Bennée" > >> To: "Miroslav Rezanina" > >> Cc: qemu-devel@nongnu.org, "Peter Maydell" , > >> "Riku Voipio" , > >> qemu-...@nongnu.org, "Laurent Vivier" > >> Sent: Friday, May 31, 2019 1:08:06 PM > >> Subject: Re: [Qemu-devel] [RFC PATCH 06/11] target/arm: use the common > >> interface for WRITE0/WRITEC in arm-semi > >> > >> > >> Miroslav Rezanina writes: > >> > >> > On Tue, May 14, 2019 at 04:52:56PM +0100, Alex Bennée wrote: > >> >> Now we have a common semihosting console interface use that for our > >> >> string output. However ARM is currently unique in also supporting > >> >> semihosting for linux-user so we need to replicate the API in > >> >> linux-user. If other architectures gain this support we can move the > >> >> file later. > >> >> > >> >> Signed-off-by: Alex Bennée > >> >> --- > >> >> linux-user/Makefile.objs | 2 ++ > >> >> linux-user/arm/semihost.c | 24 > >> >> target/arm/arm-semi.c | 31 ++- > >> >> 3 files changed, 32 insertions(+), 25 deletions(-) > >> >> create mode 100644 linux-user/arm/semihost.c > >> >> > >> >> diff --git a/linux-user/Makefile.objs b/linux-user/Makefile.objs > >> >> index 769b8d83362..285c5dfa17a 100644 > >> >> --- a/linux-user/Makefile.objs > >> >> +++ b/linux-user/Makefile.objs > >> >> @@ -6,4 +6,6 @@ obj-y = main.o syscall.o strace.o mmap.o signal.o \ > >> >> obj-$(TARGET_HAS_BFLT) += flatload.o > >> >> obj-$(TARGET_I386) += vm86.o > >> >> obj-$(TARGET_ARM) += arm/nwfpe/ > >> >> +obj-$(TARGET_ARM) += arm/semihost.o > >> >> +obj-$(TARGET_AARCH64) += arm/semihost.o > >> >> obj-$(TARGET_M68K) += m68k-sim.o > >> >> diff --git a/linux-user/arm/semihost.c b/linux-user/arm/semihost.c > >> >> new file mode 100644 > >> >> index 000..9554102a855 > >> >> --- /dev/null > >> >> +++ b/linux-user/arm/semihost.c > >> >> @@ -0,0 +1,24 @@ > >> >> +/* > >> >> + * ARM Semihosting Console Support > >> >> + * > >> >> + * Copyright (c) 2019 Linaro Ltd > >> >> + * > >> >> + * Currently ARM is unique in having support for semihosting support > >> >> + * in linux-user. So for now we implement the common console API but > >> >> + * just for arm linux-user. > >> >> + * > >> >> + * SPDX-License-Identifier: GPL-2.0-or-later > >> >> + */ > >> >> + > >> >> +#include "qemu/osdep.h" > >> >> +#include "cpu.h" > >> >> +#include "hw/semihosting/console.h" > >> >> +#include "qemu.h" > >> >> + > >> >> +int qemu_semihosting_console_out(CPUArchState *env, target_ulong addr, > >> >> int len) > >> >> +{ > >> >> +void *s = lock_user_string(addr); > >> >> +len = write(STDERR_FILENO, s, len ? len : strlen(s)); > >> >> +unlock_user(s, addr, 0); > >> >> +return len; > >> >> +} > >> >> diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c > >> >> index 9e5a414dd89..253c66b172a 100644 > >> >> --- a/target/arm/arm-semi.c > >> >> +++ b/target/arm/arm-semi.c > >> >> @@ -27,6 +27,7 @@ > >> >> > >> >> #include "cpu.h" > >> >> #include "hw/semihosting/semihost.h" > >> >> +#include "hw/semihosting/console.h" > >> >> #ifdef CONFIG_USER_ONLY > >> >> #include "qemu.h" > >> >> > >> >> @@ -314,32 +315
Re: [Qemu-devel] [RFC PATCH 06/11] target/arm: use the common interface for WRITE0/WRITEC in arm-semi
- Original Message - > From: "Alex Bennée" > To: "Miroslav Rezanina" > Cc: qemu-devel@nongnu.org, "Peter Maydell" , "Riku > Voipio" , > qemu-...@nongnu.org, "Laurent Vivier" > Sent: Friday, May 31, 2019 1:08:06 PM > Subject: Re: [Qemu-devel] [RFC PATCH 06/11] target/arm: use the common > interface for WRITE0/WRITEC in arm-semi > > > Miroslav Rezanina writes: > > > On Tue, May 14, 2019 at 04:52:56PM +0100, Alex Bennée wrote: > >> Now we have a common semihosting console interface use that for our > >> string output. However ARM is currently unique in also supporting > >> semihosting for linux-user so we need to replicate the API in > >> linux-user. If other architectures gain this support we can move the > >> file later. > >> > >> Signed-off-by: Alex Bennée > >> --- > >> linux-user/Makefile.objs | 2 ++ > >> linux-user/arm/semihost.c | 24 > >> target/arm/arm-semi.c | 31 ++- > >> 3 files changed, 32 insertions(+), 25 deletions(-) > >> create mode 100644 linux-user/arm/semihost.c > >> > >> diff --git a/linux-user/Makefile.objs b/linux-user/Makefile.objs > >> index 769b8d83362..285c5dfa17a 100644 > >> --- a/linux-user/Makefile.objs > >> +++ b/linux-user/Makefile.objs > >> @@ -6,4 +6,6 @@ obj-y = main.o syscall.o strace.o mmap.o signal.o \ > >> obj-$(TARGET_HAS_BFLT) += flatload.o > >> obj-$(TARGET_I386) += vm86.o > >> obj-$(TARGET_ARM) += arm/nwfpe/ > >> +obj-$(TARGET_ARM) += arm/semihost.o > >> +obj-$(TARGET_AARCH64) += arm/semihost.o > >> obj-$(TARGET_M68K) += m68k-sim.o > >> diff --git a/linux-user/arm/semihost.c b/linux-user/arm/semihost.c > >> new file mode 100644 > >> index 000..9554102a855 > >> --- /dev/null > >> +++ b/linux-user/arm/semihost.c > >> @@ -0,0 +1,24 @@ > >> +/* > >> + * ARM Semihosting Console Support > >> + * > >> + * Copyright (c) 2019 Linaro Ltd > >> + * > >> + * Currently ARM is unique in having support for semihosting support > >> + * in linux-user. So for now we implement the common console API but > >> + * just for arm linux-user. > >> + * > >> + * SPDX-License-Identifier: GPL-2.0-or-later > >> + */ > >> + > >> +#include "qemu/osdep.h" > >> +#include "cpu.h" > >> +#include "hw/semihosting/console.h" > >> +#include "qemu.h" > >> + > >> +int qemu_semihosting_console_out(CPUArchState *env, target_ulong addr, > >> int len) > >> +{ > >> +void *s = lock_user_string(addr); > >> +len = write(STDERR_FILENO, s, len ? len : strlen(s)); > >> +unlock_user(s, addr, 0); > >> +return len; > >> +} > >> diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c > >> index 9e5a414dd89..253c66b172a 100644 > >> --- a/target/arm/arm-semi.c > >> +++ b/target/arm/arm-semi.c > >> @@ -27,6 +27,7 @@ > >> > >> #include "cpu.h" > >> #include "hw/semihosting/semihost.h" > >> +#include "hw/semihosting/console.h" > >> #ifdef CONFIG_USER_ONLY > >> #include "qemu.h" > >> > >> @@ -314,32 +315,12 @@ target_ulong do_arm_semihosting(CPUARMState *env) > >> return set_swi_errno(ts, close(arg0)); > >> } > >> case TARGET_SYS_WRITEC: > >> -{ > >> - char c; > >> - > >> - if (get_user_u8(c, args)) > >> - /* FIXME - should this error code be -TARGET_EFAULT ? */ > >> - return (uint32_t)-1; > >> - /* Write to debug console. stderr is near enough. */ > >> - if (use_gdb_syscalls()) { > >> -return arm_gdb_syscall(cpu, arm_semi_cb, "write,2,%x,1", > >> args); > >> - } else { > >> -return write(STDERR_FILENO, , 1); > >> - } > >> -} > >> +{ > >> +qemu_semihosting_console_out(env, args, 1); > >> +return 0xdeadbeef; > >> +} > >> case TARGET_SYS_WRITE0: > >> -if (!(s = lock_user_string(args))) > >> -/* FIXME - should this error code be -TARGET_EFAULT ? */ > >> -return (uint32_t)-1; > >> -
Re: [Qemu-devel] [RFC PATCH 06/11] target/arm: use the common interface for WRITE0/WRITEC in arm-semi
- Original Message - > From: "Philippe Mathieu-Daudé" > To: "Miroslav Rezanina" , "Alex Bennée" > > Cc: "Peter Maydell" , "Riku Voipio" > , qemu-devel@nongnu.org, > qemu-...@nongnu.org, "Laurent Vivier" , "Aleksandar > Markovic" > Sent: Friday, May 31, 2019 12:42:46 PM > Subject: Re: [Qemu-devel] [RFC PATCH 06/11] target/arm: use the common > interface for WRITE0/WRITEC in arm-semi > > Hi Miroslav, > > On 5/31/19 11:12 AM, Miroslav Rezanina wrote: > > On Tue, May 14, 2019 at 04:52:56PM +0100, Alex Bennée wrote: > >> Now we have a common semihosting console interface use that for our > >> string output. However ARM is currently unique in also supporting > >> semihosting for linux-user so we need to replicate the API in > >> linux-user. If other architectures gain this support we can move the > >> file later. > >> > >> Signed-off-by: Alex Bennée > >> --- > >> linux-user/Makefile.objs | 2 ++ > >> linux-user/arm/semihost.c | 24 > >> target/arm/arm-semi.c | 31 ++- > >> 3 files changed, 32 insertions(+), 25 deletions(-) > >> create mode 100644 linux-user/arm/semihost.c > >> > >> diff --git a/linux-user/Makefile.objs b/linux-user/Makefile.objs > >> index 769b8d83362..285c5dfa17a 100644 > >> --- a/linux-user/Makefile.objs > >> +++ b/linux-user/Makefile.objs > >> @@ -6,4 +6,6 @@ obj-y = main.o syscall.o strace.o mmap.o signal.o \ > >> obj-$(TARGET_HAS_BFLT) += flatload.o > >> obj-$(TARGET_I386) += vm86.o > >> obj-$(TARGET_ARM) += arm/nwfpe/ > >> +obj-$(TARGET_ARM) += arm/semihost.o > >> +obj-$(TARGET_AARCH64) += arm/semihost.o > >> obj-$(TARGET_M68K) += m68k-sim.o > >> diff --git a/linux-user/arm/semihost.c b/linux-user/arm/semihost.c > >> new file mode 100644 > >> index 000..9554102a855 > >> --- /dev/null > >> +++ b/linux-user/arm/semihost.c > >> @@ -0,0 +1,24 @@ > >> +/* > >> + * ARM Semihosting Console Support > >> + * > >> + * Copyright (c) 2019 Linaro Ltd > >> + * > >> + * Currently ARM is unique in having support for semihosting support > >> + * in linux-user. So for now we implement the common console API but > >> + * just for arm linux-user. > >> + * > >> + * SPDX-License-Identifier: GPL-2.0-or-later > >> + */ > >> + > >> +#include "qemu/osdep.h" > >> +#include "cpu.h" > >> +#include "hw/semihosting/console.h" > >> +#include "qemu.h" > >> + > >> +int qemu_semihosting_console_out(CPUArchState *env, target_ulong addr, > >> int len) > >> +{ > >> +void *s = lock_user_string(addr); > >> +len = write(STDERR_FILENO, s, len ? len : strlen(s)); > >> +unlock_user(s, addr, 0); > >> +return len; > >> +} > >> diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c > >> index 9e5a414dd89..253c66b172a 100644 > >> --- a/target/arm/arm-semi.c > >> +++ b/target/arm/arm-semi.c > >> @@ -27,6 +27,7 @@ > >> > >> #include "cpu.h" > >> #include "hw/semihosting/semihost.h" > >> +#include "hw/semihosting/console.h" > >> #ifdef CONFIG_USER_ONLY > >> #include "qemu.h" > >> > >> @@ -314,32 +315,12 @@ target_ulong do_arm_semihosting(CPUARMState *env) > >> return set_swi_errno(ts, close(arg0)); > >> } > >> case TARGET_SYS_WRITEC: > >> -{ > >> - char c; > >> - > >> - if (get_user_u8(c, args)) > >> - /* FIXME - should this error code be -TARGET_EFAULT ? */ > >> - return (uint32_t)-1; > >> - /* Write to debug console. stderr is near enough. */ > >> - if (use_gdb_syscalls()) { > >> -return arm_gdb_syscall(cpu, arm_semi_cb, "write,2,%x,1", > >> args); > >> - } else { > >> -return write(STDERR_FILENO, , 1); > >> - } > >> -} > >> +{ > >> +qemu_semihosting_console_out(env, args, 1); > >> +return 0xdeadbeef; > >> +} > >> case TARGET_SYS_WRITE0: > >> -if (!(s = lock_user_string(args))) > >> -/*
Re: [Qemu-devel] [RFC PATCH 06/11] target/arm: use the common interface for WRITE0/WRITEC in arm-semi
On Tue, May 14, 2019 at 04:52:56PM +0100, Alex Bennée wrote: > Now we have a common semihosting console interface use that for our > string output. However ARM is currently unique in also supporting > semihosting for linux-user so we need to replicate the API in > linux-user. If other architectures gain this support we can move the > file later. > > Signed-off-by: Alex Bennée > --- > linux-user/Makefile.objs | 2 ++ > linux-user/arm/semihost.c | 24 > target/arm/arm-semi.c | 31 ++- > 3 files changed, 32 insertions(+), 25 deletions(-) > create mode 100644 linux-user/arm/semihost.c > > diff --git a/linux-user/Makefile.objs b/linux-user/Makefile.objs > index 769b8d83362..285c5dfa17a 100644 > --- a/linux-user/Makefile.objs > +++ b/linux-user/Makefile.objs > @@ -6,4 +6,6 @@ obj-y = main.o syscall.o strace.o mmap.o signal.o \ > obj-$(TARGET_HAS_BFLT) += flatload.o > obj-$(TARGET_I386) += vm86.o > obj-$(TARGET_ARM) += arm/nwfpe/ > +obj-$(TARGET_ARM) += arm/semihost.o > +obj-$(TARGET_AARCH64) += arm/semihost.o > obj-$(TARGET_M68K) += m68k-sim.o > diff --git a/linux-user/arm/semihost.c b/linux-user/arm/semihost.c > new file mode 100644 > index 000..9554102a855 > --- /dev/null > +++ b/linux-user/arm/semihost.c > @@ -0,0 +1,24 @@ > +/* > + * ARM Semihosting Console Support > + * > + * Copyright (c) 2019 Linaro Ltd > + * > + * Currently ARM is unique in having support for semihosting support > + * in linux-user. So for now we implement the common console API but > + * just for arm linux-user. > + * > + * SPDX-License-Identifier: GPL-2.0-or-later > + */ > + > +#include "qemu/osdep.h" > +#include "cpu.h" > +#include "hw/semihosting/console.h" > +#include "qemu.h" > + > +int qemu_semihosting_console_out(CPUArchState *env, target_ulong addr, int > len) > +{ > +void *s = lock_user_string(addr); > +len = write(STDERR_FILENO, s, len ? len : strlen(s)); > +unlock_user(s, addr, 0); > +return len; > +} > diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c > index 9e5a414dd89..253c66b172a 100644 > --- a/target/arm/arm-semi.c > +++ b/target/arm/arm-semi.c > @@ -27,6 +27,7 @@ > > #include "cpu.h" > #include "hw/semihosting/semihost.h" > +#include "hw/semihosting/console.h" > #ifdef CONFIG_USER_ONLY > #include "qemu.h" > > @@ -314,32 +315,12 @@ target_ulong do_arm_semihosting(CPUARMState *env) > return set_swi_errno(ts, close(arg0)); > } > case TARGET_SYS_WRITEC: > -{ > - char c; > - > - if (get_user_u8(c, args)) > - /* FIXME - should this error code be -TARGET_EFAULT ? */ > - return (uint32_t)-1; > - /* Write to debug console. stderr is near enough. */ > - if (use_gdb_syscalls()) { > -return arm_gdb_syscall(cpu, arm_semi_cb, "write,2,%x,1", > args); > - } else { > -return write(STDERR_FILENO, , 1); > - } > -} > +{ > +qemu_semihosting_console_out(env, args, 1); > +return 0xdeadbeef; > +} > case TARGET_SYS_WRITE0: > -if (!(s = lock_user_string(args))) > -/* FIXME - should this error code be -TARGET_EFAULT ? */ > -return (uint32_t)-1; > -len = strlen(s); > -if (use_gdb_syscalls()) { > -return arm_gdb_syscall(cpu, arm_semi_cb, "write,2,%x,%x", > - args, len); > -} else { > -ret = write(STDERR_FILENO, s, len); > -} > -unlock_user(s, args, 0); > -return ret; > +return qemu_semihosting_console_out(env, args, 0); > case TARGET_SYS_WRITE: > GET_ARG(0); > GET_ARG(1); > -- > 2.20.1 > > Hi Alex, this patch breaks build for softmmu target when CONFIG_SEMIHOSTING is not enabled as qemu_semihosting_console_out is not defined in such case - neither linux-user/arm/semihost.c nor hw/semihosting/console.c compiled and function is not in stubs/semihost.c Mirek
Re: [Qemu-devel] [PATCH for-2.12] memfd: fix vhost-user-test on non-memfd capable host
On Wed, Mar 28, 2018 at 02:18:04PM +0200, Marc-André Lureau wrote: > On RHEL7, memfd is not supported, and vhost-user-test fails: > TEST: tests/vhost-user-test... (pid=10248) > /x86_64/vhost-user/migrate: > qemu-system-x86_64: -object memory-backend-memfd,id=mem,size=2M,: failed to > create memfd > FAIL > > There is a qemu_memfd_check() to prevent running memfd path, but it > also checks for fallback implementation. Let's specialize > qemu_memfd_check() to check memfd only, while qemu_memfd_alloc_check() > checks for the qemu_memfd_alloc() API. > > Reported-by: Miroslav Rezanina <mreza...@redhat.com> > Tested-by: Miroslav Rezanina <mreza...@redhat.com> > Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com> > --- Patch is solving failing build on RHEL 7. Needed in 2.12. Tested-by: Miroslav Rezanina <mreza...@redhat.com> > include/qemu/memfd.h | 1 + > hw/virtio/vhost.c| 2 +- > util/memfd.c | 30 +- > 3 files changed, 31 insertions(+), 2 deletions(-) > > diff --git a/include/qemu/memfd.h b/include/qemu/memfd.h > index de10198ed6..49e79634da 100644 > --- a/include/qemu/memfd.h > +++ b/include/qemu/memfd.h > @@ -18,6 +18,7 @@ > > int qemu_memfd_create(const char *name, size_t size, bool hugetlb, >uint64_t hugetlbsize, unsigned int seals, Error > **errp); > +bool qemu_memfd_alloc_check(void); > void *qemu_memfd_alloc(const char *name, size_t size, unsigned int seals, > int *fd, Error **errp); > void qemu_memfd_free(void *ptr, size_t size, int fd); > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > index 250f886acb..27c1ec5fe8 100644 > --- a/hw/virtio/vhost.c > +++ b/hw/virtio/vhost.c > @@ -1223,7 +1223,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque, > if (!(hdev->features & (0x1ULL << VHOST_F_LOG_ALL))) { > error_setg(>migration_blocker, > "Migration disabled: vhost lacks VHOST_F_LOG_ALL > feature."); > -} else if (vhost_dev_log_is_shared(hdev) && !qemu_memfd_check()) { > +} else if (vhost_dev_log_is_shared(hdev) && > !qemu_memfd_alloc_check()) { > error_setg(>migration_blocker, > "Migration disabled: failed to allocate shared > memory"); > } > diff --git a/util/memfd.c b/util/memfd.c > index 07d579ea7d..277f7211af 100644 > --- a/util/memfd.c > +++ b/util/memfd.c > @@ -173,7 +173,13 @@ enum { > MEMFD_TODO > }; > > -bool qemu_memfd_check(void) > +/** > + * qemu_memfd_alloc_check(): > + * > + * Check if qemu_memfd_alloc() can allocate, including using a > + * fallback implementation when host doesn't support memfd. > + */ > +bool qemu_memfd_alloc_check(void) > { > static int memfd_check = MEMFD_TODO; > > @@ -188,3 +194,25 @@ bool qemu_memfd_check(void) > > return memfd_check == MEMFD_OK; > } > + > +/** > + * qemu_memfd_check(): > + * > + * Check if host supports memfd. > + */ > +bool qemu_memfd_check(void) > +{ > +static int memfd_check = MEMFD_TODO; > + > +if (memfd_check == MEMFD_TODO) { > +int mfd = memfd_create("test", 0); > +if (mfd >= 0) { > +memfd_check = MEMFD_OK; > +close(mfd); > +} else { > +memfd_check = MEMFD_KO; > +} > +} > + > +return memfd_check == MEMFD_OK; > +} > -- > 2.17.0.rc1.1.g4c4f2b46a3 > >
Re: [Qemu-devel] [PATCH] tcg: fix 16-byte vector operations detection
On Wed, Mar 28, 2018 at 03:31:52PM +0200, Laurent Vivier wrote: > configure tries to detect if the compiler > supports 16-byte vector operations. > > As stated in the comment of the detection > program, there is a problem with the system > compiler on GCC on Centos 7. > > This program doesn't actually detect the problem > with GCC on RHEL7 on PPC64LE (Red Hat 4.8.5-28). > > This patch updates the test to look more like > it is in QEMU helpers, and now detects the problem. > > The error reported is: > > CC ppc64-softmmu/accel/tcg/tcg-runtime-gvec.o > ..//accel/tcg/tcg-runtime-gvec.c: In function ‘helper_gvec_shl8i’: > ../accel/tcg/tcg-runtime-gvec.c:558:26: internal compiler error: in > emit_move_insn, at expr.c:3495 >*(vec8 *)(d + i) = *(vec8 *)(a + i) << shift; > ^ > Fixes: db43267 "tcg: Add generic vector expanders" > Signed-off-by: Laurent Vivier <lviv...@redhat.com> > --- > configure | 8 > 1 file changed, 8 insertions(+) > > diff --git a/configure b/configure > index 4d0e92c96c..a2301dd0dc 100755 > --- a/configure > +++ b/configure > @@ -5054,6 +5054,14 @@ static S2 c2; > static S4 c4; > static S8 c8; > static int i; > +void helper(void *d, void *a, int shift, int i); > +void helper(void *d, void *a, int shift, int i) > +{ > + *(U1 *)(d + i) = *(U1 *)(a + i) << shift; > + *(U2 *)(d + i) = *(U2 *)(a + i) << shift; > + *(U4 *)(d + i) = *(U4 *)(a + i) << shift; > + *(U8 *)(d + i) = *(U8 *)(a + i) << shift; > +} > int main(void) > { >a1 += b1; a2 += b2; a4 += b4; a8 += b8; > -- > 2.14.3 > > Build works correctly for RHEL 7 with this patch Reviewed-by: Miroslav Rezanina <mreza...@redhat.com>
Re: [Qemu-devel] [PATCH] hmp: fix "dump-quest-memory" segfault (ppc)
- Original Message - > From: "Thomas Huth" <th...@redhat.com> > To: "Laurent Vivier" <lviv...@redhat.com>, qemu-devel@nongnu.org > Cc: "David Gibson" <da...@gibson.dropbear.id.au>, qemu-...@nongnu.org, "Dr . > David Alan Gilbert" > <dgilb...@redhat.com>, "Miroslav Rezanina" <mreza...@redhat.com> > Sent: Monday, September 11, 2017 4:36:01 PM > Subject: Re: [PATCH] hmp: fix "dump-quest-memory" segfault (ppc) > > On 11.09.2017 13:00, Laurent Vivier wrote: > > Commit fd5d23babf (hmp: fix "dump-quest-memory" segfault) > > fixes the problem for i386, do the same for ppc. > > > > Running QEMU with > > qemu-system-ppc64 -M none -nographic -m 256 > > and executing > > dump-guest-memory /dev/null 0 8192 > > results in segfault > > > > Fix by checking if we have CPU. > > > > Signed-off-by: Laurent Vivier <lviv...@redhat.com> > > --- > > target/ppc/arch_dump.c | 17 +++-- > > 1 file changed, 11 insertions(+), 6 deletions(-) > > > > diff --git a/target/ppc/arch_dump.c b/target/ppc/arch_dump.c > > index 8e9397aa58..dcb7b19950 100644 > > --- a/target/ppc/arch_dump.c > > +++ b/target/ppc/arch_dump.c > > @@ -224,17 +224,22 @@ typedef struct NoteFuncDescStruct NoteFuncDesc; > > int cpu_get_dump_info(ArchDumpInfo *info, > >const struct GuestPhysBlockList *guest_phys_blocks) > > { > > -PowerPCCPU *cpu = POWERPC_CPU(first_cpu); > > -PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu); > > - > > info->d_machine = PPC_ELF_MACHINE; > > info->d_class = ELFCLASS; > > > > -if ((*pcc->interrupts_big_endian)(cpu)) { > > -info->d_endian = ELFDATA2MSB; > > +if (first_cpu) { > > +PowerPCCPU *cpu = POWERPC_CPU(first_cpu); > > +PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu); > > + > > +if ((*pcc->interrupts_big_endian)(cpu)) { > > +info->d_endian = ELFDATA2MSB; > > +} else { > > +info->d_endian = ELFDATA2LSB; > > + } > > } else { > > -info->d_endian = ELFDATA2LSB; > > +info->d_endian = ELFDATA2MSB; > > } > > + > > /* 64KB is the max page size for pseries kernel */ > > if (strncmp(object_get_typename(qdev_get_machine()), > > "pseries-", 8) == 0) { > > > > Reviewed-by: Thomas Huth <th...@redhat.com> > We need similar fix for aarch64 too. Mirek -- Miroslav Rezanina Software Engineer - Virtualization Team
Re: [Qemu-devel] [PATCH] Whitelist sysinfo call
- 元のメッセージ - > 差出人: "Eduardo Otubo" <eduardo.ot...@profitbricks.com> > 宛先: "Serge Hallyn" <serge.hal...@ubuntu.com> > Cc: mreza...@redhat.com, qemu-devel@nongnu.org, arm...@redhat.com > 送信済み: 2016年4月12日, 火曜日 午後 1:53:47 > 件名: Re: Re: [Qemu-devel] [PATCH] Whitelist sysinfo call > > On Mon, Apr 11, 2016 at 08=19=52PM +, Serge Hallyn wrote: > > Quoting mreza...@redhat.com (mreza...@redhat.com): > > > From: Miroslav Rezanina <mreza...@redhat.com> > > > > > > Newer version of nss-softokn libraries (> 3.16.2.3) use sysinfo call > > > so qemu using rbd image hang after start when run in sandbox mode. > > > > > > To allow using rbd images in sandbox mode we have to whitelist it. > > > > > > Signed-off-by: Miroslav Rezanina <mreza...@redhat.com> > > > > Thanks. > > > > Acked-by: Serge E. Hallyn <serge.hal...@ubuntu.com> > > Also: I'll change the subject of this email a little bit for a cleaner > and more explicit commit. > > Thanks. Ok, thanks. Mirek > > > > > > --- > > > qemu-seccomp.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/qemu-seccomp.c b/qemu-seccomp.c > > > index 2866e3c..e29fca1 100644 > > > --- a/qemu-seccomp.c > > > +++ b/qemu-seccomp.c > > > @@ -250,6 +250,7 @@ static const struct QemuSeccompSyscall > > > seccomp_whitelist[] = { > > > #ifdef HAVE_CACHEFLUSH > > > { SCMP_SYS(cacheflush), 240 }, > > > #endif > > > +{ SCMP_SYS(sysinfo), 240 }, > > > }; > > > > > > int seccomp_start(void) > > > -- > > > 2.5.0 > > > > > > > > > > -- > Eduardo Otubo > ProfitBricks GmbH > -- Miroslav Rezanina Software Engineer - Virtualization Team
Re: [Qemu-devel] [PATCH] Whitelist sysinfo call
- 元のメッセージ - > 差出人: "Eduardo Otubo" <eduardo.ot...@profitbricks.com> > 宛先: mreza...@redhat.com > Cc: qemu-devel@nongnu.org, arm...@redhat.com > 送信済み: 2016年3月11日, 金曜日 午前 9:51:50 > 件名: Re: [Qemu-devel] [PATCH] Whitelist sysinfo call > > On Mon, Mar 07, 2016 at 10=34=46AM +0100, mreza...@redhat.com wrote: > > From: Miroslav Rezanina <mreza...@redhat.com> > > > > Newer version of nss-softokn libraries (> 3.16.2.3) use sysinfo call > > so qemu using rbd image hang after start when run in sandbox mode. > > > > To allow using rbd images in sandbox mode we have to whitelist it. > > > > Signed-off-by: Miroslav Rezanina <mreza...@redhat.com> > > --- > > qemu-seccomp.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/qemu-seccomp.c b/qemu-seccomp.c > > index 2866e3c..e29fca1 100644 > > --- a/qemu-seccomp.c > > +++ b/qemu-seccomp.c > > @@ -250,6 +250,7 @@ static const struct QemuSeccompSyscall > > seccomp_whitelist[] = { > > #ifdef HAVE_CACHEFLUSH > > { SCMP_SYS(cacheflush), 240 }, > > #endif > > +{ SCMP_SYS(sysinfo), 240 }, > > Are you sure you want to add this syscall to the bottom of the list? Did > you estimate the frequency it is called by running strace? > > Thanks for the patch. > Hi, Yes, it wasn't used before nss update and now is used only for rbd based images where it is called just few times upon start so drawback should be minimal. With this we do not change cost of other calls. Thanks for review and question, Mirek > -- > Eduardo Otubo > ProfitBricks GmbH > -- Miroslav Rezanina Software Engineer - Virtualization Team
Re: [Qemu-devel] [PATCH v2] slirp: use less predictable directory name in /tmp for smb config (CVE-2015-4037)
On Tue, Jun 02, 2015 at 08:46:35AM +0300, Michael Tokarev wrote: In this version I used mkdtemp(3) which is: _BSD_SOURCE || /* Since glibc 2.10: */ (_POSIX_C_SOURCE = 200809L || _XOPEN_SOURCE = 700) (POSIX.1-2008), so should be available on systems we care about. While at it, reset the resulting directory name within smb structure on error so cleanup function wont try to remove directory which we failed to create. Signed-off-by: Michael Tokarev m...@tls.msk.ru --- v2: Add resetting of the dirname on failure so that cleanup function does not try to remove directory which we failed to create. Use snprintf() as was in the original code, not strcpy(): while in this very case it does not matter at all since both strings are of known size, some people dislike strcpy() in principle. net/slirp.c | 7 +++ 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/net/slirp.c b/net/slirp.c index 0e15cf6..3533837 100644 --- a/net/slirp.c +++ b/net/slirp.c @@ -481,7 +481,6 @@ static void slirp_smb_cleanup(SlirpState *s) static int slirp_smb(SlirpState* s, const char *exported_dir, struct in_addr vserver_addr) { -static int instance; char smb_conf[128]; char smb_cmdline[128]; struct passwd *passwd; @@ -505,10 +504,10 @@ static int slirp_smb(SlirpState* s, const char *exported_dir, return -1; } -snprintf(s-smb_dir, sizeof(s-smb_dir), /tmp/qemu-smb.%ld-%d, - (long)getpid(), instance++); -if (mkdir(s-smb_dir, 0700) 0) { +snprintf(s-smb_dir, sizeof(s-smb_dir), /tmp/qemu-smb.XX); +if (!mkdtemp(s-smb_dir)) { error_report(could not create samba server dir '%s', s-smb_dir); +s-smb_dir[0] = 0; return -1; } snprintf(smb_conf, sizeof(smb_conf), %s/%s, s-smb_dir, smb.conf); -- 2.1.4 Reviewed-by: Miroslav Rezanina mreza...@redhat.com
Re: [Qemu-devel] [PATCH] slirp: use less predictable directory name in /tmp for smb config (CVE-2015-4037)
On Thu, May 28, 2015 at 02:15:43PM +0300, Michael Tokarev wrote: In this version I used mkdtemp(3) which is: _BSD_SOURCE || /* Since glibc 2.10: */ (_POSIX_C_SOURCE = 200809L || _XOPEN_SOURCE = 700) so should be available on systems we care about. Signed-off-by: Michael Tokarev m...@tls.msk.ru --- net/slirp.c | 6 ++ 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/net/slirp.c b/net/slirp.c index 9bbed74..0ad32ad 100644 --- a/net/slirp.c +++ b/net/slirp.c @@ -481,7 +481,6 @@ static void slirp_smb_cleanup(SlirpState *s) static int slirp_smb(SlirpState* s, const char *exported_dir, struct in_addr vserver_addr) { -static int instance; char smb_conf[128]; char smb_cmdline[128]; struct passwd *passwd; @@ -505,9 +504,8 @@ static int slirp_smb(SlirpState* s, const char *exported_dir, return -1; } -snprintf(s-smb_dir, sizeof(s-smb_dir), /tmp/qemu-smb.%ld-%d, - (long)getpid(), instance++); -if (mkdir(s-smb_dir, 0700) 0) { +strcpy(s-smb_dir, /tmp/qemu-smb.XX); +if (!mkdtemp(s-smb_dir)) { error_report(could not create samba server dir '%s', s-smb_dir); return -1; } -- 2.1.4 I suggest to go with this patch as: 1) It was sent first 2) Is simplier 3) Keep original behavior Reviewed-by: Miroslav Rezanina mreza...@redhat.com
Re: [Qemu-devel] [PATCH] net: fix insecure temporary file creation in SLiRP
On Mon, Jun 01, 2015 at 09:58:10AM +0200, Markus Armbruster wrote: mreza...@redhat.com writes: From: Miroslav Rezanina mreza...@redhat.com Qemu's user mode networking stack(-net user) is vulnerable to a predictable temporary file creation flaw. This patch uses mkdtemp(3) routine to fix it. Fixes CVE-2015-4037. Signed-off-by: P J P p...@fedoraproject.org Signed-off-by: Miroslav Rezanina mreza...@redhat.com --- [1] http://seclists.org/oss-sec/2015/q2/538 --- net/slirp.c | 10 +- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/net/slirp.c b/net/slirp.c index 0e15cf6..804b095 100644 --- a/net/slirp.c +++ b/net/slirp.c @@ -27,6 +27,7 @@ #ifndef _WIN32 #include pwd.h +#include stdlib.h #include sys/wait.h #endif #include net/net.h @@ -481,9 +482,9 @@ static void slirp_smb_cleanup(SlirpState *s) static int slirp_smb(SlirpState* s, const char *exported_dir, struct in_addr vserver_addr) { -static int instance; char smb_conf[128]; char smb_cmdline[128]; +char smb_dir[] = /tmp/qemu-smb.XX, *tmpdir = NULL; struct passwd *passwd; FILE *f; @@ -505,12 +506,11 @@ static int slirp_smb(SlirpState* s, const char *exported_dir, return -1; } -snprintf(s-smb_dir, sizeof(s-smb_dir), /tmp/qemu-smb.%ld-%d, - (long)getpid(), instance++); -if (mkdir(s-smb_dir, 0700) 0) { -error_report(could not create samba server dir '%s', s-smb_dir); +if (!(tmpdir = mkdtemp(smb_dir))) { +error_report(could not create samba server dir '%s', smb_dir); return -1; } +strncpy(s-smb_dir, tmpdir, sizeof(s-smb_dir)); We tend to eschew strncpy() and use our (slightly less bad) pstrcpy(). Recommend to use it here, too. Good point. Worth changing. snprintf(smb_conf, sizeof(smb_conf), %s/%s, s-smb_dir, smb.conf); f = fopen(smb_conf, w); Michael (cc'ed) already posted [PATCH] slirp: use less predictable directory name in /tmp for smb config (CVE-2015-4037)[*]. His patch clobbers s-smb_dir[] when mkdtemp() fails (missed that in my review), yours doesn't. I have to remember to check qemu-devel before preparing/sending patch. My different version already sent ratio is too high. In this case are both fixes almost identical. Mirek Suggest you guys figure out together which solution you want. Preferably with strncpy() replaced by pstrcpy(): Reviewed-by: Markus Armbruster arm...@redhat.com [*] http://lists.nongnu.org/archive/html/qemu-devel/2015-05/msg05767.html
Re: [Qemu-devel] [PATCHv3 2/2] stubs: Provide parallel_mm_init stub version
On Wed, May 13, 2015 at 10:04:23AM +0200, Markus Armbruster wrote: Paolo Bonzini pbonz...@redhat.com writes: On 12/05/2015 08:22, mreza...@redhat.com wrote: From: Miroslav Rezanina mreza...@redhat.com mips build fail with link error in case PARALLEL_CONFIG is disabled as hw/mips/mips_jazz.c calls parallel_mm_init. Due to dependecies to content of parallel.c we can't simply move it to hw/isa/isa-devices.c. This patch adds stubs/parallel.c file that contains stub version of parallel_mm_init. This ensure successful build with PARALLEL_CONFIG disabled. Signed-off-by: Miroslav Rezanina mreza...@redhat.com --- stubs/Makefile.objs | 1 + stubs/parallel.c| 8 2 files changed, 9 insertions(+) create mode 100644 stubs/parallel.c diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs index 8beff4c..ad4e110 100644 --- a/stubs/Makefile.objs +++ b/stubs/Makefile.objs @@ -24,6 +24,7 @@ stub-obj-y += mon-printf.o stub-obj-y += mon-set-error.o stub-obj-y += monitor-init.o stub-obj-y += notify-event.o +stub-obj-y += parallel.o stub-obj-$(CONFIG_SPICE) += qemu-chr-open-spice.o stub-obj-y += qtest.o stub-obj-y += reset.o diff --git a/stubs/parallel.c b/stubs/parallel.c new file mode 100644 index 000..8293d52 --- /dev/null +++ b/stubs/parallel.c @@ -0,0 +1,8 @@ +#include hw/i386/pc.h + +bool parallel_mm_init(MemoryRegion *address_space, + hwaddr base, int it_shift, qemu_irq irq, + CharDriverState *chr) +{ +return false; +} I think removing CONFIG_PARALLEL from a board that hardcodes its presence makes little sense, so I would just drop this patch. I pointed Mirek to parallel_mm_init(). Second thoughts: since we don't know omitting the device breaks guests, and aren't really interested in finding out, let's leave things as they are, i.e. drop this patch. Ok, v4 will be patch 1 only. Mirek
Re: [Qemu-devel] [PATCHv3 1/2] Move parallel_hds_isa_init to hw/isa/isa-bus.c
On Wed, May 13, 2015 at 10:01:12AM +0200, Markus Armbruster wrote: mreza...@redhat.com writes: From: Miroslav Rezanina mreza...@redhat.com Disabling CONFIG_PARALLEL cause removing parallel_hds_isa_init defined in parallel.c. This function is called during initialization of some boards so disabling CONFIG_PARALLEL cause build failure. This patch moves parallel_hds_isa_init to hw/isa/isa-bus.c so it is included in case of disabled CONFIG_PARALLEL. Build is successful but qemu will abort with Unknown device error when function is called. Can't reproduce this error. Test case: compile with CONFIG_PARALLEL=y taken out, run like $ qemu-system-x86_64 -nodefaults -S -display none -parallel stdio Signed-off-by: Miroslav Rezanina mreza...@redhat.com --- hw/char/parallel.c | 25 - hw/isa/isa-bus.c | 29 + 2 files changed, 29 insertions(+), 25 deletions(-) diff --git a/hw/char/parallel.c b/hw/char/parallel.c index 4079554..c2b553f 100644 --- a/hw/char/parallel.c +++ b/hw/char/parallel.c @@ -641,28 +641,3 @@ static void parallel_register_types(void) } type_init(parallel_register_types) - -static void parallel_init(ISABus *bus, int index, CharDriverState *chr) -{ -DeviceState *dev; -ISADevice *isadev; - -isadev = isa_create(bus, isa-parallel); -dev = DEVICE(isadev); -qdev_prop_set_uint32(dev, index, index); -qdev_prop_set_chr(dev, chardev, chr); -qdev_init_nofail(dev); -} - -void parallel_hds_isa_init(ISABus *bus, int n) -{ -int i; - -assert(n = MAX_PARALLEL_PORTS); - -for (i = 0; i n; i++) { -if (parallel_hds[i]) { -parallel_init(bus, i, parallel_hds[i]); -} -} -} diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c index 825aa62..94f645c 100644 --- a/hw/isa/isa-bus.c +++ b/hw/isa/isa-bus.c @@ -21,6 +21,7 @@ #include hw/sysbus.h #include sysemu/sysemu.h #include hw/isa/isa.h +#include hw/i386/pc.h static ISABus *isabus; @@ -267,3 +268,31 @@ MemoryRegion *isa_address_space_io(ISADevice *dev) } type_init(isabus_register_types) + +static void parallel_init(ISABus *bus, int index, CharDriverState *chr) +{ +DeviceState *dev; +ISADevice *isadev; + +isadev = isa_try_create(bus, isa-parallel); +if (!isadev) { + return; +} Not just code motion! You change the code to ignore the error. If that's what we want, the commit message needs fixing. Ouch,this is mixed in change from different code version. Intention was to move code only. This is the reason to different behavior. I will send fixed version. Mirek +dev = DEVICE(isadev); +qdev_prop_set_uint32(dev, index, index); +qdev_prop_set_chr(dev, chardev, chr); +qdev_init_nofail(dev); +} + +void parallel_hds_isa_init(ISABus *bus, int n) +{ +int i; + +assert(n = MAX_PARALLEL_PORTS); + +for (i = 0; i n; i++) { +if (parallel_hds[i]) { +parallel_init(bus, i, parallel_hds[i]); +} +} +}
Re: [Qemu-devel] [PATCHv2] parallel: Allow to disable CONFIG_PARALLEL
On Mon, May 11, 2015 at 10:40:04AM +0200, Paolo Bonzini wrote: On 11/05/2015 07:38, mreza...@redhat.com wrote: From: Miroslav Rezanina mreza...@redhat.com Disabling CONFIG_PARALLEL cause build failure as commit 07dc788 factored out initialization to parallel_hds_isa_init function in hw/char/parallel.c that is not build. Stub file is added to be able to disable CONFIG_PARALLEL. This file is used in targets using parallel_hds_isa_init and provide empty definition of this function. Signed-off-by: Miroslav Rezanina mreza...@redhat.com This patch will make -parallel a nop. The right thing to do is to fail startup whenever -parallel is passed and CONFIG_PARALLEL is disabled. This was original behavior before 07dc788. Intention of this patch is to make qemu buildable with CONFIG_PARALLEL disabled. You can move parallel_hds_isa_init and parallel_init to hw/isa/isa-bus.c, or to a new file hw/isa/isa-devices.c. Moving functions will cause abort with Unknown device error. Paolo --- hw/i386/Makefile.objs| 1 + hw/mips/Makefile.objs| 2 ++ hw/sparc64/Makefile.objs | 2 ++ stubs/parallel-stub.c| 7 +++ 4 files changed, 12 insertions(+) create mode 100644 stubs/parallel-stub.c diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs index e058a39..2b7131a 100644 --- a/hw/i386/Makefile.objs +++ b/hw/i386/Makefile.objs @@ -4,6 +4,7 @@ obj-y += pc.o pc_piix.o pc_q35.o obj-y += pc_sysfw.o obj-y += intel_iommu.o obj-$(CONFIG_XEN) += ../xenpv/ xen/ +obj-$(call lnot,$(CONFIG_PARALLEL)) += ../../stubs/parallel-stub.o obj-y += kvmvapic.o obj-y += acpi-build.o diff --git a/hw/mips/Makefile.objs b/hw/mips/Makefile.objs index 0a652f8..2e65305 100644 --- a/hw/mips/Makefile.objs +++ b/hw/mips/Makefile.objs @@ -2,3 +2,5 @@ obj-y += mips_r4k.o mips_jazz.o mips_malta.o mips_mipssim.o obj-y += addr.o cputimer.o mips_int.o obj-$(CONFIG_FULONG) += mips_fulong2e.o obj-y += gt64xxx_pci.o +obj-$(call lnot,$(CONFIG_PARALLEL)) += ../../stubs/parallel-stub.o + diff --git a/hw/sparc64/Makefile.objs b/hw/sparc64/Makefile.objs index a84cfe3..7696611 100644 --- a/hw/sparc64/Makefile.objs +++ b/hw/sparc64/Makefile.objs @@ -1 +1,3 @@ obj-y += sun4u.o +obj-$(call lnot,$(CONFIG_PARALLEL)) += ../../stubs/parallel-stub.o + diff --git a/stubs/parallel-stub.c b/stubs/parallel-stub.c new file mode 100644 index 000..949c1b2 --- /dev/null +++ b/stubs/parallel-stub.c @@ -0,0 +1,7 @@ +#include qemu/typedefs.h +#include hw/isa/isa.h +#include hw/i386/pc.h + +void parallel_hds_isa_init(ISABus *bus, int n) +{ +}
Re: [Qemu-devel] [PATCHv2] parallel: Allow to disable CONFIG_PARALLEL
On Mon, May 11, 2015 at 08:46:19AM +0200, Markus Armbruster wrote: mreza...@redhat.com writes: From: Miroslav Rezanina mreza...@redhat.com Disabling CONFIG_PARALLEL cause build failure as commit 07dc788 factored out initialization to parallel_hds_isa_init function in hw/char/parallel.c that is not build. Stub file is added to be able to disable CONFIG_PARALLEL. This file is used in targets using parallel_hds_isa_init and provide empty definition of this function. Signed-off-by: Miroslav Rezanina mreza...@redhat.com --- hw/i386/Makefile.objs| 1 + hw/mips/Makefile.objs| 2 ++ hw/sparc64/Makefile.objs | 2 ++ stubs/parallel-stub.c| 7 +++ Nitpick: the existing stub/*.c naming practice suggests stubs/parallel.c. Yeah...I forget to rename it after moving from repository root to stub directory. I originally have it in repository root as it is not included in libqemustub. So the naming can be treated as hint that something is different. However, I can rename it to follow stubs/* naming. 4 files changed, 12 insertions(+) create mode 100644 stubs/parallel-stub.c diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs index e058a39..2b7131a 100644 --- a/hw/i386/Makefile.objs +++ b/hw/i386/Makefile.objs @@ -4,6 +4,7 @@ obj-y += pc.o pc_piix.o pc_q35.o obj-y += pc_sysfw.o obj-y += intel_iommu.o obj-$(CONFIG_XEN) += ../xenpv/ xen/ +obj-$(call lnot,$(CONFIG_PARALLEL)) += ../../stubs/parallel-stub.o obj-y += kvmvapic.o obj-y += acpi-build.o Can we rely on the linker to pull parallel-stub.o from a suitable .a libqemustub.a when needed? We do not have to as parallel-stub.o is not included in libqemustub.a. It is linked directly in case CONFIG_PARALLEL is not defined (for targets using parallel_hds_isa_init). Mirek diff --git a/hw/mips/Makefile.objs b/hw/mips/Makefile.objs index 0a652f8..2e65305 100644 --- a/hw/mips/Makefile.objs +++ b/hw/mips/Makefile.objs @@ -2,3 +2,5 @@ obj-y += mips_r4k.o mips_jazz.o mips_malta.o mips_mipssim.o obj-y += addr.o cputimer.o mips_int.o obj-$(CONFIG_FULONG) += mips_fulong2e.o obj-y += gt64xxx_pci.o +obj-$(call lnot,$(CONFIG_PARALLEL)) += ../../stubs/parallel-stub.o + diff --git a/hw/sparc64/Makefile.objs b/hw/sparc64/Makefile.objs index a84cfe3..7696611 100644 --- a/hw/sparc64/Makefile.objs +++ b/hw/sparc64/Makefile.objs @@ -1 +1,3 @@ obj-y += sun4u.o +obj-$(call lnot,$(CONFIG_PARALLEL)) += ../../stubs/parallel-stub.o + diff --git a/stubs/parallel-stub.c b/stubs/parallel-stub.c new file mode 100644 index 000..949c1b2 --- /dev/null +++ b/stubs/parallel-stub.c @@ -0,0 +1,7 @@ +#include qemu/typedefs.h +#include hw/isa/isa.h +#include hw/i386/pc.h + +void parallel_hds_isa_init(ISABus *bus, int n) +{ +}
Re: [Qemu-devel] [PATCH] parallel: Allow to disable CONFIG_PARALLEL
On Tue, May 05, 2015 at 12:32:37PM +0200, Thomas Huth wrote: On Tue, 5 May 2015 12:18:05 +0200 Miroslav Rezanina mreza...@redhat.com wrote: On Tue, May 05, 2015 at 11:59:50AM +0200, Thomas Huth wrote: On Tue, 5 May 2015 11:30:49 +0200 mreza...@redhat.com wrote: From: Miroslav Rezanina mreza...@redhat.com Disabling CONFIG_PARALLEL cause build failure as commit 07dc788 factored out initialization to parallel_hds_isa_init that is not build. Make calling parallel_hds_isa_init depending on CONFIG_PARALLEL so it can be correctly disabled. Signed-off-by: Miroslav Rezanina mreza...@redhat.com --- hw/i386/pc.c| 2 ++ hw/mips/mips_fulong2e.c | 2 ++ hw/mips/mips_malta.c| 2 ++ hw/sparc64/sun4u.c | 2 ++ 4 files changed, 8 insertions(+) diff --git a/hw/i386/pc.c b/hw/i386/pc.c index a8e6be1..560464e 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -1465,7 +1465,9 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi, } serial_hds_isa_init(isa_bus, MAX_SERIAL_PORTS); +#ifdef CONFIG_PARALLEL parallel_hds_isa_init(isa_bus, MAX_PARALLEL_PORTS); +#endif Not sure, but is this pre-processor macro really defined if CONFIG_PARALLEL has been set in the makefile? I've hit some similar problem in the past and I had to discover that only the CONFIG_* options from config-host.mak get translated into #defines, all the others don't get translated. I might be wrong, but just to be sure, could you please double-check that CONFIG_PARALLEL is #defined if it's enabled in the .mak file? Yeah, you're right..I switch the behavior so thought of CONFIG_* from config-host.mak to be the one to not translated. And ifdef masked the issue. This patch is not working and has to be redone. If not: Where does the build break exactly? Does it fail for all three types, i386, mips and sun? I can't test mips/sun build but for i386 it fails on linking where parallel_hds_isa_init does not exists. That's strange, there is a CONFIG_PARALLEL=y in both, default-configs/x86_64-softmmu.mak and i386-softmmu.mak so I wonder why the parallel.o object file is not linked in your case? Thomas It is enabled by default but build fails in case you disable the option for these architectures. Mirek
Re: [Qemu-devel] [PATCH] parallel: Allow to disable CONFIG_PARALLEL
On Tue, May 05, 2015 at 11:59:50AM +0200, Thomas Huth wrote: On Tue, 5 May 2015 11:30:49 +0200 mreza...@redhat.com wrote: From: Miroslav Rezanina mreza...@redhat.com Disabling CONFIG_PARALLEL cause build failure as commit 07dc788 factored out initialization to parallel_hds_isa_init that is not build. Make calling parallel_hds_isa_init depending on CONFIG_PARALLEL so it can be correctly disabled. Signed-off-by: Miroslav Rezanina mreza...@redhat.com --- hw/i386/pc.c| 2 ++ hw/mips/mips_fulong2e.c | 2 ++ hw/mips/mips_malta.c| 2 ++ hw/sparc64/sun4u.c | 2 ++ 4 files changed, 8 insertions(+) diff --git a/hw/i386/pc.c b/hw/i386/pc.c index a8e6be1..560464e 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -1465,7 +1465,9 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi, } serial_hds_isa_init(isa_bus, MAX_SERIAL_PORTS); +#ifdef CONFIG_PARALLEL parallel_hds_isa_init(isa_bus, MAX_PARALLEL_PORTS); +#endif Not sure, but is this pre-processor macro really defined if CONFIG_PARALLEL has been set in the makefile? I've hit some similar problem in the past and I had to discover that only the CONFIG_* options from config-host.mak get translated into #defines, all the others don't get translated. I might be wrong, but just to be sure, could you please double-check that CONFIG_PARALLEL is #defined if it's enabled in the .mak file? Yeah, you're right..I switch the behavior so thought of CONFIG_* from config-host.mak to be the one to not translated. And ifdef masked the issue. This patch is not working and has to be redone. If not: Where does the build break exactly? Does it fail for all three types, i386, mips and sun? I can't test mips/sun build but for i386 it fails on linking where parallel_hds_isa_init does not exists. Mirek Thomas
Re: [Qemu-devel] [PATCHv2] Prevent segmentation fault in case of relative resolve of uri
On Mon, Feb 09, 2015 at 12:31:18PM +0100, Paolo Bonzini wrote: On 09/02/2015 11:47, mreza...@redhat.com wrote: From: Miroslav Rezanina mreza...@redhat.com It was possible to call strcmp with NULL argument, that can cause segmentation fault. Properly checking parameters to prevent this situation. Signed-off-by: Miroslav Rezanina mreza...@redhat.com --- v2: - instead of adding NULL checks to strcmp call refactor whole NULL checking path. This will remove dead code and make whole checking easier to understand. Relative path generation part is not touched as I'm not fully sure of correct behavior and purpose of this patch is to prevent segmentation fault. --- util/uri.c | 55 +-- 1 file changed, 25 insertions(+), 30 deletions(-) diff --git a/util/uri.c b/util/uri.c index 918d235..23dbaca 100644 --- a/util/uri.c +++ b/util/uri.c @@ -1964,44 +1964,39 @@ uri_resolve_relative (const char *uri, const char * base) * If the scheme / server on the URI differs from the base, * just return the URI */ -if ((ref-scheme != NULL) - ((bas-scheme == NULL) || -(strcmp (bas-scheme, ref-scheme)) || -(strcmp (bas-server, ref-server { - val = g_strdup (uri); - goto done; + +if ((ref-scheme != NULL) +((bas-scheme == NULL) || (strcmp (bas-scheme, ref-scheme { +val = g_strdup(uri); +goto done; } -if (!strcmp(bas-path, ref-path)) { - val = g_strdup(); - goto done; -} -if (bas-path == NULL) { - val = g_strdup(ref-path); - goto done; +if ((ref-server != NULL) +((bas-server == NULL) || (strcmp (bas-server, ref-server { +val = g_strdup(uri); +goto done; } + if (ref-path == NULL) { ref-path = (char *) /; - remove_path = 1; +remove_path = 1; } -/* - * At this point (at last!) we can compare the two paths - * - * First we take care of the special case where either of the - * two path components may be missing (bug 316224) - */ if (bas-path == NULL) { - if (ref-path != NULL) { - uptr = ref-path; - if (*uptr == '/') - uptr++; - /* exception characters from uri_to_string */ - val = uri_string_escape(uptr, /;=+$,); - } - goto done; +uptr = ref-path; +if (*uptr == '/') +uptr++; +/* exception characters from uri_to_string */ +val = uri_string_escape(uptr, /;=+$,); +goto done; } + +if (!strcmp(bas-path, ref-path)) { +val = g_strdup(); +goto done; +} + bptr = bas-path; -if (ref-path == NULL) { +if (remove_path == 1) { for (ix = 0; bptr[ix] != 0; ix++) { if (bptr[ix] == '/') nbslash++; @@ -2010,7 +2005,7 @@ uri_resolve_relative (const char *uri, const char * base) len = 1;/* this is for a string terminator only */ } else { /* - * Next we compare the two strings and find where they first differ + * We compare the two strings and find where they first differ */ if ((ref-path[pos] == '.') (ref-path[pos+1] == '/')) pos += 2; It's the third time a fix for this is submitted. :) [PATCH 3/3] util/uri: URI member path can be null, compare more carfully (by Markus) [PATCH 3/7] uri: avoid NULL arguments to strcmp (by me). Markus's patch was accepted. Further cleanups on the code on top of his patch are welcome, though. The logic for compare two possibly-NULL strings could be replaced by g_strcmp0, but that's only provided by glib versions 2.16 or newer, while we support 2.12. Paolo Hi Paolo, I miss the patches as they are not committed yet. The patch do almost the same think as my v1 (with one more check) that was rejected by Stefan because adding more checks only. In addition to preventing null compare, this patch removes the dead code in the function. I'll revisit it after Marcus fix is committed. Mirek
Re: [Qemu-devel] [PATCH v6] vl.c: Output error on invalid machine type
Hi, is there any issue with this patch? Mirek - Original Message - From: mreza...@redhat.com To: qemu-devel@nongnu.org Sent: Wednesday, February 5, 2014 2:44:23 PM Subject: [Qemu-devel] [PATCH v6] vl.c: Output error on invalid machine type From: Miroslav Rezanina mreza...@redhat.com Output error message using qemu's error_report() function when user provides the invalid machine type on the command line. This also saves time to find what issue is when you downgrade from one version of qemu to another that doesn't support required machine type yet (the version user downgraded to have to have this patch applied too, of course). Signed-off-by: Miroslav Rezanina mreza...@redhat.com --- v6: - print help instead of list supported machines on error vl.c | 18 -- 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/vl.c b/vl.c index 383be1b..3297c0a 100644 --- a/vl.c +++ b/vl.c @@ -2600,13 +2600,19 @@ static QEMUMachine *machine_parse(const char *name) if (machine) { return machine; } -printf(Supported machines are:\n); -for (m = first_machine; m != NULL; m = m-next) { -if (m-alias) { -printf(%-20s %s (alias of %s)\n, m-alias, m-desc, m-name); + +if (name !is_help_option(name)) { +error_report(Unsupported machine type); +printf(\nUse '-M help' to list supported machines!\n); +} else { +printf(Supported machines are:\n); +for (m = first_machine; m != NULL; m = m-next) { +if (m-alias) { +printf(%-20s %s (alias of %s)\n, m-alias, m-desc, m-name); +} +printf(%-20s %s%s\n, m-name, m-desc, + m-is_default ? (default) : ); } -printf(%-20s %s%s\n, m-name, m-desc, - m-is_default ? (default) : ); } exit(!name || !is_help_option(name)); } -- 1.8.5.3 -- Miroslav Rezanina Software Engineer - Virtualization Team
Re: [Qemu-devel] [PATCH] qemu-img: Fix content mismatch offset of image compare
- Original Message - From: Fam Zheng f...@redhat.com To: qemu-devel@nongnu.org Cc: stefa...@redhat.com, kw...@redhat.com, Miroslav Rezanina mreza...@redhat.com Sent: Wednesday, November 13, 2013 5:04:18 AM Subject: [PATCH] qemu-img: Fix content mismatch offset of image compare We were lucky to pass qemu-iotests 048 (qemu-img compare case) but when I tried to run with TEST_DIR=/tmp (tmpfs), it fails with a very weird mismatch offset. This fixes the bug. In the if branch, setting ret to 1 before using it makes dead code in the next line: pnum is never added to mismatch offset even if ret was 0. Signed-off-by: Fam Zheng f...@redhat.com --- qemu-img.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/qemu-img.c b/qemu-img.c index bf3fb4f..2bab20d 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -1020,10 +1020,10 @@ static int img_compare(int argc, char **argv) } ret = compare_sectors(buf1, buf2, nb_sectors, pnum); if (ret || pnum != nb_sectors) { -ret = 1; qprintf(quiet, Content mismatch at offset % PRId64 !\n, sectors_to_bytes( ret ? sector_num : sector_num + pnum)); +ret = 1; goto out; } } -- 1.8.4.2 Correct fix, setting variable to constant before test is definitely wrong. Reviewed-by: Miroslav Rezanina mreza...@redhat.com -- Miroslav Rezanina Software Engineer - Virtualization Team
Re: [Qemu-devel] [PATCHv2] Make usb-bt-dongle configurable
- Original Message - From: Gerd Hoffmann kra...@redhat.com To: mreza...@redhat.com Cc: qemu-devel@nongnu.org Sent: Tuesday, September 3, 2013 9:41:11 AM Subject: Re: [Qemu-devel] [PATCHv2] Make usb-bt-dongle configurable diff --git a/hw/bt/core.c b/hw/bt/core.c index 49012e0..ef27b15 100644 --- a/hw/bt/core.c +++ b/hw/bt/core.c @@ -119,3 +119,28 @@ void bt_device_done(struct bt_device_s *dev) *p = dev-next; } + +static struct bt_vlan_s { +struct bt_scatternet_s net; +int id; +struct bt_vlan_s *next; +} *first_bt_vlan; + +/* find or alloc a new bluetooth VLAN */ +struct bt_scatternet_s *qemu_find_bt_vlan(int id) +{ +struct bt_vlan_s **pvlan, *vlan; +for (vlan = first_bt_vlan; vlan != NULL; vlan = vlan-next) { +if (vlan-id == id) { +return vlan-net; +} +} +vlan = g_malloc0(sizeof(struct bt_vlan_s)); +vlan-id = id; +pvlan = first_bt_vlan; +while (*pvlan != NULL) { +pvlan = (*pvlan)-next; +} +*pvlan = vlan; +return vlan-net; +} This (and some other bits) are pure code motion from vl.c, correct? Can you split this into a separate patch please? That'll simplify the review o the actual code changes. Yes, this is pure code motion. I'll split the code to separate patches. It also doesn't make much sense to compile hw/bt/ with CONFIG_USB_BLUETOOTH=n. It's basically dead code then. Is this true? So -bt option is not useable without usb-bt-dongle? cheers, Gerd -- Miroslav Rezanina Software Engineer - Virtualization Team
[Qemu-devel] [PATCH] Make usb-bt-dongle configurable
usb-bt-dongle device can't be disabled as there's dependency in vl.c file. This patch add preprocesor condition to be able to disable it. Signed-off-by: Miroslav Rezanina mreza...@redhat.com --- hw/usb/Makefile.objs | 1 - vl.c | 18 ++ 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/hw/usb/Makefile.objs b/hw/usb/Makefile.objs index f9695e7..8892ffd 100644 --- a/hw/usb/Makefile.objs +++ b/hw/usb/Makefile.objs @@ -20,7 +20,6 @@ common-obj-$(CONFIG_USB_SERIAL) += dev-serial.o common-obj-$(CONFIG_USB_NETWORK) += dev-network.o # FIXME: make configurable too -CONFIG_USB_BLUETOOTH := y common-obj-$(CONFIG_USB_BLUETOOTH)+= dev-bluetooth.o ifeq ($(CONFIG_USB_SMARTCARD),y) diff --git a/vl.c b/vl.c index f422a1c..4330b6d 100644 --- a/vl.c +++ b/vl.c @@ -1526,8 +1526,10 @@ static void configure_msg(QemuOpts *opts) static int usb_device_add(const char *devname) { -const char *p; USBDevice *dev = NULL; +#if defined(CONFIG_USB_BLUETOOTH) || !defined(CONFIG_LINUX) +const char *p; +#endif if (!usb_enabled(false)) { return -1; @@ -1543,15 +1545,23 @@ static int usb_device_add(const char *devname) /* only the linux version is qdev-ified, usb-bsd still needs this */ if (strstart(devname, host:, p)) { dev = usb_host_device_open(usb_bus_find(-1), p); -} else +goto devtest; +} #endif +#ifdef CONFIG_USB_BLUETOOTH if (!strcmp(devname, bt) || strstart(devname, bt:, p)) { dev = usb_bt_init(usb_bus_find(-1), devname[2] ? hci_init(p) : bt_new_hci(qemu_find_bt_vlan(0))); -} else { -return -1; +goto devtest; } +#endif + +return -1; + +#if defined(CONFIG_USB_BLUETOOTH) || !defined(CONFIG_LINUX) +devtest: +#endif if (!dev) return -1; -- 1.8.3.1
Re: [Qemu-devel] [ANNOUNCE] QEMU 1.5.2 Stable released
Hi Michael, how this affect 1.5 schedule?? Is the date mentioned on http://wiki.qemu.org/Planning/1.5 still valid (just increase the build number)? Mirek Rezanina - Original Message - From: Michael Roth mdr...@linux.vnet.ibm.com To: qemu-devel@nongnu.org Cc: pmato...@redhat.com, aligu...@us.ibm.com, ler...@redhat.com, qemu-sta...@nongnu.org, lve...@redhat.com Sent: Thursday, July 25, 2013 11:44:43 PM Subject: [Qemu-devel] [ANNOUNCE] QEMU 1.5.2 Stable released The QEMU v1.5.2 stable release is now available at: http://wiki.qemu.org/download/qemu-1.5.2.tar.bz2 This is release is solely to address a security issue (CVE-2013-2231) found in the QEMU Guest Agent on Windows. More details on the nature of the CVE can be found here: http://seclists.org/oss-sec/2013/q3/161 There are 2 minor fixes for qemu-ga for Windows as well, though these are included mainly due to being dependencies of the CVE fix sent upstream. Thanks to Laszlo and the Red Hat security team for identifying/fixing the issue. ff4be47: Update VERSION for 1.5.2 release (Michael Roth) be161ae: qga: escape cmdline args when registering win32 service (CVE-2013-2231) (Laszlo Ersek) bb31546: ga_install_service(): nest error paths more idiomatically (Laszlo Ersek) af0bbf8: qga/service-win32.c: diagnostic output should go to stderr (Laszlo Ersek) 31c6ed2: qga: save state directory in ga_install_service() (Laszlo Ersek) c432c7d: qga: remove undefined behavior in ga_install_service() (Laszlo Ersek) -- Miroslav Rezanina Software Engineer - Virtualization Team
Re: [Qemu-devel] [PATCH v10 0/4] Add subcommand compare for qemu-img
Please ignore this serie. It's resended v10 instead of v11. Sorry for spam. - Original Message - From: Miroslav Rezanina mreza...@redhat.com To: qemu-devel@nongnu.org, qemu-devel@nongnu.org Cc: kw...@redhat.com, pbonz...@redhat.com, stefa...@redhat.com, ebl...@redhat.com, Miroslav Rezanina mreza...@redhat.com Sent: Wednesday, February 13, 2013 9:05:53 AM Subject: [PATCH v10 0/4] Add subcommand compare for qemu-img This is 10th version of patch adding compare subcommand that compares two images. Compare has following criteria: - only data part is compared - unallocated sectors are not read to be zeroed/unallocated to compare rest - qemu-img returns: - 0 if images are identical - 1 if images differ - 2 error on execution v10: - rebased to kwolf's block-next branch due to conflict patch (patch 2) - exit status updates (patch 3) - minor non-functional fixes v9: - Merged patch 3 and 4 (subcommmand implementation and documentation) of v8 - Added basic test for qemu-img compare - Fixed incorrect indentation - add bdrv_is_allocated_above() return value check - Add description of exit codes into the documentation - minor non-functional changes v8: - minor grammar fixes (no behavior change) v7: - split patch into pieces - Quiet mode added for all relevant subcommands - check non-shared part of disk after shared one - minor docummentation and naming fixes v6: - added handling -?, -h options for compare subcommand v5 (only minor changes): - removed redundant comment - removed dead code (goto after help()) - set final total_sectors on first assignment v4: - Fixed various typos - Added functions for empty sector check and sector-to-bytes offset conversion - Fixed command-line parameters processing v3: - options -f/-F are orthogonal - documentation updated to new syntax and behavior - used byte offset instead of sector number for output v2: - changed option for second image format to -F - changed handling of -f and -F [1] - added strict mode (-s) - added quiet mode (-q) - improved output messages [2] - rename variables for larger image handling - added man page content Signed-off-by: Miroslav Rezanina mreza...@redhat.com Miroslav Rezanina (4): block: Add synchronous wrapper for bdrv_co_is_allocated_above qemu-img: Add Quiet mode option This patch adds new qemu-img subcommand that compares content of two disk images. qemu-iotests: Add qemu-img compare test block.c| 51 +- blockdev.c |6 +- include/block/block.h |5 +- qemu-img-cmds.hx | 34 ++-- qemu-img.c | 441 +++- qemu-img.texi | 56 ++ tests/qemu-iotests/048 | 77 tests/qemu-iotests/048.out | 25 +++ tests/qemu-iotests/group |1 + 9 files changed, 626 insertions(+), 70 deletions(-) create mode 100755 tests/qemu-iotests/048 create mode 100644 tests/qemu-iotests/048.out -- Miroslav Rezanina Software Engineer - Virtualization Team
[Qemu-devel] [PATCH v11 0/4] Add subcommand compare for qemu-img
This is 11th version of patch adding compare subcommand that compares two images. Compare has following criteria: - only data part is compared - unallocated sectors are not read - qemu-img returns: - 0 if images are identical - 1 if images differ - 2 error on execution v11: - add minor enhancements to qemu-img compare test (patch 4) v10: - rebased to kwolf's block-next branch due to conflict patch (patch 2) - exit status updates (patch 3) - minor non-functional fixes v9: - Merged patch 3 and 4 (subcommmand implementation and documentation) of v8 - Added basic test for qemu-img compare - Fixed incorrect indentation - add bdrv_is_allocated_above() return value check - Add description of exit codes into the documentation - minor non-functional changes v8: - minor grammar fixes (no behavior change) v7: - split patch into pieces - Quiet mode added for all relevant subcommands - check non-shared part of disk after shared one - minor docummentation and naming fixes v6: - added handling -?, -h options for compare subcommand v5 (only minor changes): - removed redundant comment - removed dead code (goto after help()) - set final total_sectors on first assignment v4: - Fixed various typos - Added functions for empty sector check and sector-to-bytes offset conversion - Fixed command-line parameters processing v3: - options -f/-F are orthogonal - documentation updated to new syntax and behavior - used byte offset instead of sector number for output v2: - changed option for second image format to -F - changed handling of -f and -F [1] - added strict mode (-s) - added quiet mode (-q) - improved output messages [2] - rename variables for larger image handling - added man page content Signed-off-by: Miroslav Rezanina mreza...@redhat.com Miroslav Rezanina (4): block: Add synchronous wrapper for bdrv_co_is_allocated_above qemu-img: Add Quiet mode option qemu-img: Add compare subcommand qemu-iotests: Add qemu-img compare test block.c| 51 +- blockdev.c |6 +- include/block/block.h |5 +- qemu-img-cmds.hx | 34 ++-- qemu-img.c | 441 +++- qemu-img.texi | 56 ++ tests/qemu-iotests/048 | 78 tests/qemu-iotests/048.out | 31 +++ tests/qemu-iotests/group |1 + 9 files changed, 633 insertions(+), 70 deletions(-) create mode 100755 tests/qemu-iotests/048 create mode 100644 tests/qemu-iotests/048.out
[Qemu-devel] [PATCH v11 2/4] qemu-img: Add Quiet mode option
There can be a need to turn output to stdout off. This patch adds a -q option that enable Quiet mode. In Quiet mode, only errors are printed out. Signed-off-by: Miroslav Rezanina mreza...@redhat.com --- block.c | 12 ++-- blockdev.c|6 +- include/block/block.h |3 +- qemu-img-cmds.hx | 28 +- qemu-img.c| 151 ++--- qemu-img.texi |3 + 6 files changed, 134 insertions(+), 69 deletions(-) diff --git a/block.c b/block.c index 08039d2..a4d7125 100644 --- a/block.c +++ b/block.c @@ -4470,7 +4470,8 @@ bdrv_acct_done(BlockDriverState *bs, BlockAcctCookie *cookie) void bdrv_img_create(const char *filename, const char *fmt, const char *base_filename, const char *base_fmt, - char *options, uint64_t img_size, int flags, Error **errp) + char *options, uint64_t img_size, int flags, + Error **errp, bool quiet) { QEMUOptionParameter *param = NULL, *create_options = NULL; QEMUOptionParameter *backing_fmt, *backing_file, *size; @@ -4579,10 +4580,11 @@ void bdrv_img_create(const char *filename, const char *fmt, } } -printf(Formatting '%s', fmt=%s , filename, fmt); -print_option_parameters(param); -puts(); - +if (!quiet) { +printf(Formatting '%s', fmt=%s , filename, fmt); +print_option_parameters(param); +puts(); +} ret = bdrv_create(drv, filename, param); if (ret 0) { if (ret == -ENOTSUP) { diff --git a/blockdev.c b/blockdev.c index 63e6f1e..3859ac2 100644 --- a/blockdev.c +++ b/blockdev.c @@ -791,7 +791,7 @@ void qmp_transaction(BlockdevActionList *dev_list, Error **errp) bdrv_img_create(new_image_file, format, states-old_bs-filename, states-old_bs-drv-format_name, -NULL, -1, flags, local_err); +NULL, -1, flags, local_err, false); if (error_is_set(local_err)) { error_propagate(errp, local_err); goto delete_and_fail; @@ -1284,7 +1284,7 @@ void qmp_drive_mirror(const char *device, const char *target, /* create new image w/o backing file */ assert(format drv); bdrv_img_create(target, format, -NULL, NULL, NULL, size, flags, local_err); +NULL, NULL, NULL, size, flags, local_err, false); } else { switch (mode) { case NEW_IMAGE_MODE_EXISTING: @@ -1295,7 +1295,7 @@ void qmp_drive_mirror(const char *device, const char *target, bdrv_img_create(target, format, source-filename, source-drv-format_name, -NULL, size, flags, local_err); +NULL, size, flags, local_err, false); break; default: abort(); diff --git a/include/block/block.h b/include/block/block.h index 8309fc1..371b42f 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -352,7 +352,8 @@ int bdrv_load_vmstate(BlockDriverState *bs, uint8_t *buf, void bdrv_img_create(const char *filename, const char *fmt, const char *base_filename, const char *base_fmt, - char *options, uint64_t img_size, int flags, Error **errp); + char *options, uint64_t img_size, int flags, + Error **errp, bool quiet); void bdrv_set_buffer_alignment(BlockDriverState *bs, int align); void *qemu_blockalign(BlockDriverState *bs, size_t size); diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx index 259fc14..9283776 100644 --- a/qemu-img-cmds.hx +++ b/qemu-img-cmds.hx @@ -10,27 +10,27 @@ STEXI ETEXI DEF(check, img_check, -check [-f fmt] [--output=ofmt] [-r [leaks | all]] filename) +check [-q] [-f fmt] [--output=ofmt] [-r [leaks | all]] filename) STEXI -@item check [-f @var{fmt}] [--output=@var{ofmt}] [-r [leaks | all]] @var{filename} +@item check [-q] [-f @var{fmt}] [--output=@var{ofmt}] [-r [leaks | all]] @var{filename} ETEXI DEF(create, img_create, -create [-f fmt] [-o options] filename [size]) +create [-q] [-f fmt] [-o options] filename [size]) STEXI -@item create [-f @var{fmt}] [-o @var{options}] @var{filename} [@var{size}] +@item create [-q] [-f @var{fmt}] [-o @var{options}] @var{filename} [@var{size}] ETEXI DEF(commit, img_commit, -commit [-f fmt] [-t cache] filename) +commit [-q] [-f fmt] [-t cache] filename) STEXI -@item commit [-f @var{fmt}] [-t @var{cache}] @var{filename} +@item commit [-q] [-f @var{fmt}] [-t @var{cache}] @var{filename} ETEXI DEF(convert, img_convert, -convert [-c] [-p] [-f fmt] [-t cache] [-O output_fmt] [-o options] [-s snapshot_name] [-S sparse_size] filename [filename2 [...]] output_filename) +convert [-c
[Qemu-devel] [PATCH v11 3/4] qemu-img: Add compare subcommand
This patch adds new qemu-img subcommand that compares content of two disk images. Signed-off-by: Miroslav Rezanina mreza...@redhat.com --- qemu-img-cmds.hx |6 + qemu-img.c | 290 +- qemu-img.texi| 53 ++ 3 files changed, 348 insertions(+), 1 deletions(-) diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx index 9283776..4ca7e95 100644 --- a/qemu-img-cmds.hx +++ b/qemu-img-cmds.hx @@ -27,6 +27,12 @@ STEXI @item commit [-q] [-f @var{fmt}] [-t @var{cache}] @var{filename} ETEXI +DEF(compare, img_compare, +compare [-f fmt] [-F fmt] [-p] [-q] [-s] filename1 filename2) +STEXI +@item compare [-f @var{fmt}] [-F @var{fmt}] [-p] [-q] [-s] @var{filename1} @var{filename2} +ETEXI + DEF(convert, img_convert, convert [-c] [-p] [-q] [-f fmt] [-t cache] [-O output_fmt] [-o options] [-s snapshot_name] [-S sparse_size] filename [filename2 [...]] output_filename) STEXI diff --git a/qemu-img.c b/qemu-img.c index a2a2044..5f0c0c1 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -113,7 +113,12 @@ static void help(void) '-a' applies a snapshot (revert disk to saved state)\n '-c' creates a snapshot\n '-d' deletes a snapshot\n - '-l' lists all snapshots in the given image\n; + '-l' lists all snapshots in the given image\n + \n + Parameters to compare subcommand:\n + '-f' first image format\n + '-F' second image format\n + '-s' run in Strict mode - fail on different image size or sector allocation\n; printf(%s\nSupported formats:, help_msg); bdrv_iterate_format(format_print, NULL); @@ -817,6 +822,289 @@ static int compare_sectors(const uint8_t *buf1, const uint8_t *buf2, int n, #define IO_BUF_SIZE (2 * 1024 * 1024) +static int64_t sectors_to_bytes(int64_t sectors) +{ +return sectors BDRV_SECTOR_BITS; +} + +static int64_t sectors_to_process(int64_t total, int64_t from) +{ +return MIN(total - from, IO_BUF_SIZE BDRV_SECTOR_BITS); +} + +/* + * Check if passed sectors are empty (not allocated or contain only 0 bytes) + * + * Returns 0 in case sectors are filled with 0, 1 if sectors contain non-zero + * data and negative value on error. + * + * @param bs: Driver used for accessing file + * @param sect_num: Number of first sector to check + * @param sect_count: Number of sectors to check + * @param filename: Name of disk file we are checking (logging purpose) + * @param buffer: Allocated buffer for storing read data + * @param quiet: Flag for quiet mode + */ +static int check_empty_sectors(BlockDriverState *bs, int64_t sect_num, + int sect_count, const char *filename, + uint8_t *buffer, bool quiet) +{ +int pnum, ret = 0; +ret = bdrv_read(bs, sect_num, buffer, sect_count); +if (ret 0) { +error_report(Error while reading offset % PRId64 of %s: %s, + sectors_to_bytes(sect_num), filename, strerror(-ret)); +return ret; +} +ret = is_allocated_sectors(buffer, sect_count, pnum); +if (ret || pnum != sect_count) { +qprintf(quiet, Content mismatch at offset % PRId64 !\n, +sectors_to_bytes(ret ? sect_num : sect_num + pnum)); +return 1; +} + +return 0; +} + +/* + * Compares two images. Exit codes: + * + * 0 - Images are identical + * 1 - Images differ + * 1 - Error occurred + */ +static int img_compare(int argc, char **argv) +{ +const char *fmt1 = NULL, *fmt2 = NULL, *filename1, *filename2; +BlockDriverState *bs1, *bs2; +int64_t total_sectors1, total_sectors2; +uint8_t *buf1 = NULL, *buf2 = NULL; +int pnum1, pnum2; +int allocated1, allocated2; +int ret = 0; /* return value - 0 Ident, 1 Different, 1 Error */ +bool progress = false, quiet = false, strict = false; +int64_t total_sectors; +int64_t sector_num = 0; +int64_t nb_sectors; +int c, pnum; +uint64_t bs_sectors; +uint64_t progress_base; + +for (;;) { +c = getopt(argc, argv, hpf:F:sq); +if (c == -1) { +break; +} +switch (c) { +case '?': +case 'h': +help(); +break; +case 'f': +fmt1 = optarg; +break; +case 'F': +fmt2 = optarg; +break; +case 'p': +progress = true; +break; +case 'q': +quiet = true; +break; +case 's': +strict = true; +break; +} +} + +/* Progress is not shown in Quiet mode */ +if (quiet) { +progress = false; +} + + +if (optind argc - 2) { +help(); +} +filename1 = argv[optind++]; +filename2 = argv[optind++]; + +/* Initialize before goto out */ +qemu_progress_init(progress, 2.0); + +bs1 = bdrv_new_open(filename1, fmt1
[Qemu-devel] [PATCH v11 1/4] block: Add synchronous wrapper for bdrv_co_is_allocated_above
There's no synchronous wrapper for bdrv_co_is_allocated_above function so it's not possible to check for sector allocation in an image with a backing file. Signed-off-by: Miroslav Rezanina mreza...@redhat.com --- block.c | 39 +++ include/block/block.h |2 ++ 2 files changed, 41 insertions(+), 0 deletions(-) diff --git a/block.c b/block.c index 50dab8e..08039d2 100644 --- a/block.c +++ b/block.c @@ -2681,6 +2681,7 @@ int bdrv_has_zero_init(BlockDriverState *bs) typedef struct BdrvCoIsAllocatedData { BlockDriverState *bs; +BlockDriverState *base; int64_t sector_num; int nb_sectors; int *pnum; @@ -2813,6 +2814,44 @@ int coroutine_fn bdrv_co_is_allocated_above(BlockDriverState *top, return 0; } +/* Coroutine wrapper for bdrv_is_allocated_above() */ +static void coroutine_fn bdrv_is_allocated_above_co_entry(void *opaque) +{ +BdrvCoIsAllocatedData *data = opaque; +BlockDriverState *top = data-bs; +BlockDriverState *base = data-base; + +data-ret = bdrv_co_is_allocated_above(top, base, data-sector_num, + data-nb_sectors, data-pnum); +data-done = true; +} + +/* + * Synchronous wrapper around bdrv_co_is_allocated_above(). + * + * See bdrv_co_is_allocated_above() for details. + */ +int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base, +int64_t sector_num, int nb_sectors, int *pnum) +{ +Coroutine *co; +BdrvCoIsAllocatedData data = { +.bs = top, +.base = base, +.sector_num = sector_num, +.nb_sectors = nb_sectors, +.pnum = pnum, +.done = false, +}; + +co = qemu_coroutine_create(bdrv_is_allocated_above_co_entry); +qemu_coroutine_enter(co, data); +while (!data.done) { +qemu_aio_wait(); +} +return data.ret; +} + BlockInfo *bdrv_query_info(BlockDriverState *bs) { BlockInfo *info = g_malloc0(sizeof(*info)); diff --git a/include/block/block.h b/include/block/block.h index ce61883..8309fc1 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -279,6 +279,8 @@ int bdrv_co_discard(BlockDriverState *bs, int64_t sector_num, int nb_sectors); int bdrv_has_zero_init(BlockDriverState *bs); int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors, int *pnum); +int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base, +int64_t sector_num, int nb_sectors, int *pnum); void bdrv_set_on_error(BlockDriverState *bs, BlockdevOnError on_read_error, BlockdevOnError on_write_error); -- 1.7.1
[Qemu-devel] [PATCH v11 4/4] qemu-iotests: Add qemu-img compare test
Simple test for qemu-img compare to check it's working correctly. Signed-off-by: Miroslav Rezanina mreza...@redhat.com --- tests/qemu-iotests/048 | 78 tests/qemu-iotests/048.out | 31 + tests/qemu-iotests/group |1 + 3 files changed, 110 insertions(+), 0 deletions(-) create mode 100755 tests/qemu-iotests/048 create mode 100644 tests/qemu-iotests/048.out diff --git a/tests/qemu-iotests/048 b/tests/qemu-iotests/048 new file mode 100755 index 000..597e4ba --- /dev/null +++ b/tests/qemu-iotests/048 @@ -0,0 +1,78 @@ +#!/bin/bash +## +## qemu-img compare test +## +## +## Copyright (C) 2013 Red Hat, Inc. +## +## This program is free software; you can redistribute it and/or modify +## it under the terms of the GNU General Public License as published by +## the Free Software Foundation; either version 2 of the License, or +## (at your option) any later version. +## +## This program is distributed in the hope that it will be useful, +## but WITHOUT ANY WARRANTY; without even the implied warranty of +## MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +## GNU General Public License for more details. +## +## You should have received a copy of the GNU General Public License +## along with this program. If not, see http://www.gnu.org/licenses/. +## +# +# creator +owner=mreza...@redhat.com + +seq=`basename $0` +echo QA output created by $seq + +status=1# failure is the default! + +_cleanup() +{ +echo Cleanup +_cleanup_test_img +rm ${TEST_IMG2} +} +trap _cleanup; exit \$status 0 1 2 3 15 + +_compare() +{ +$QEMU_IMG compare $@ $TEST_IMG ${TEST_IMG2} +echo $? +} + +# get standard environment, filters and checks +. ./common.rc +. ./common.filter +. ./common.pattern + +_supported_fmt raw qcow qcow2 qed +_supported_proto file +_supported_os Linux + +# Setup test basic parameters +TEST_IMG2=$TEST_IMG.2 +CLUSTER_SIZE=4096 +size=1024M + +_make_test_img $size +io_pattern write 524288 $CLUSTER_SIZE $CLUSTER_SIZE 4 45 + +# Compare identical images +cp $TEST_IMG ${TEST_IMG2} +_compare +_compare -q + +# Compare images with different size +$QEMU_IMG resize $TEST_IMG +512M +_compare +_compare -s + +# Compare images with different content +io_pattern write 1228800 $CLUSTER_SIZE 0 1 67 +_compare +io_pattern write 0 $CLUSTER_SIZE 0 1 123 +_compare + +# Cleanup +status=0 diff --git a/tests/qemu-iotests/048.out b/tests/qemu-iotests/048.out new file mode 100644 index 000..68f65d5 --- /dev/null +++ b/tests/qemu-iotests/048.out @@ -0,0 +1,31 @@ +QA output created by 048 +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824 +=== IO: pattern 45 +qemu-io wrote 4096/4096 bytes at offset 524288 +4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +qemu-io wrote 4096/4096 bytes at offset 528384 +4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +qemu-io wrote 4096/4096 bytes at offset 532480 +4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +qemu-io wrote 4096/4096 bytes at offset 536576 +4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +qemu-io Images are identical. +0 +0 +Image resized. +Warning: Image size mismatch! +Images are identical. +0 +Strict mode: Image size mismatch! +1 +=== IO: pattern 67 +qemu-io wrote 4096/4096 bytes at offset 1228800 +4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +qemu-io Content mismatch at offset 1228800! +1 +=== IO: pattern 123 +qemu-io wrote 4096/4096 bytes at offset 0 +4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +qemu-io Content mismatch at offset 0! +1 +Cleanup diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group index 1bbd2bf..1c0dfef 100644 --- a/tests/qemu-iotests/group +++ b/tests/qemu-iotests/group @@ -54,3 +54,4 @@ 045 rw auto 046 rw auto aio 047 rw auto +048 img auto quick -- 1.7.1
Re: [Qemu-devel] [PATCH v11 3/4] qemu-img: Add compare subcommand
- Original Message - From: Eric Blake ebl...@redhat.com To: Miroslav Rezanina mreza...@redhat.com Cc: qemu-devel@nongnu.org, kw...@redhat.com, pbonz...@redhat.com, stefa...@redhat.com Sent: Wednesday, February 13, 2013 2:39:59 PM Subject: Re: [PATCH v11 3/4] qemu-img: Add compare subcommand On 02/13/2013 01:09 AM, Miroslav Rezanina wrote: This patch adds new qemu-img subcommand that compares content of two disk images. Signed-off-by: Miroslav Rezanina mreza...@redhat.com --- +Compare exits with @code{0} in case the images are equal and with @code{1} +in case the images differ. Negative exit code means an error occurred during There's no such thing as a negative exit code. Aaa, I forget to fix this. @stefanha: Can you please replace the work Negative with Others when applying. +execution and standard error output should contain an error message. +Following table sumarize all exit codes of compare subcommand: s/sumarize/summarizes/ @stefanha: Fix this typo too, thanks. + +@table @option + +@item 0 +Images are identical +@item 1 +Images differ +@item 2 +Error on opening an image +@item 3 +Error on checking a sector allocation +@item 4 +Error on reading data + +@end table -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org Thanks a lot, Eric. -- Miroslav Rezanina Software Engineer - Virtualization Team
Re: [Qemu-devel] [PATCH v10 3/4] This patch adds new qemu-img subcommand that compares content of two disk images.
- Original Message - From: Kevin Wolf kw...@redhat.com To: Miroslav Rezanina mreza...@redhat.com Cc: qemu-devel@nongnu.org, pbonz...@redhat.com, stefa...@redhat.com, ebl...@redhat.com Sent: Tuesday, February 12, 2013 10:36:07 AM Subject: Re: [PATCH v10 3/4] This patch adds new qemu-img subcommand that compares content of two disk images. Am 12.02.2013 08:00, schrieb Miroslav Rezanina: Signed-off-by: Miroslav Rezanina mreza...@redhat.com What happened to the commit message here? Previous versions had qemu-img: Add compare subcommand as their subject and what is now the subject was the body. Made much more sense. Stefan, if you apply the series, can you fix this up? Kevin Ups...I had to accidentally delete the line when handling the cherry-pick (from v9 to v10). Subject and commit message from v9 are valid and should be used. Sorry for the mistake. -- Miroslav Rezanina Software Engineer - Virtualization Team
[Qemu-devel] [PATCH v10 1/4] block: Add synchronous wrapper for bdrv_co_is_allocated_above
There's no synchronous wrapper for bdrv_co_is_allocated_above function so it's not possible to check for sector allocation in an image with a backing file. Signed-off-by: Miroslav Rezanina mreza...@redhat.com --- block.c | 39 +++ include/block/block.h |2 ++ 2 files changed, 41 insertions(+), 0 deletions(-) diff --git a/block.c b/block.c index 50dab8e..08039d2 100644 --- a/block.c +++ b/block.c @@ -2681,6 +2681,7 @@ int bdrv_has_zero_init(BlockDriverState *bs) typedef struct BdrvCoIsAllocatedData { BlockDriverState *bs; +BlockDriverState *base; int64_t sector_num; int nb_sectors; int *pnum; @@ -2813,6 +2814,44 @@ int coroutine_fn bdrv_co_is_allocated_above(BlockDriverState *top, return 0; } +/* Coroutine wrapper for bdrv_is_allocated_above() */ +static void coroutine_fn bdrv_is_allocated_above_co_entry(void *opaque) +{ +BdrvCoIsAllocatedData *data = opaque; +BlockDriverState *top = data-bs; +BlockDriverState *base = data-base; + +data-ret = bdrv_co_is_allocated_above(top, base, data-sector_num, + data-nb_sectors, data-pnum); +data-done = true; +} + +/* + * Synchronous wrapper around bdrv_co_is_allocated_above(). + * + * See bdrv_co_is_allocated_above() for details. + */ +int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base, +int64_t sector_num, int nb_sectors, int *pnum) +{ +Coroutine *co; +BdrvCoIsAllocatedData data = { +.bs = top, +.base = base, +.sector_num = sector_num, +.nb_sectors = nb_sectors, +.pnum = pnum, +.done = false, +}; + +co = qemu_coroutine_create(bdrv_is_allocated_above_co_entry); +qemu_coroutine_enter(co, data); +while (!data.done) { +qemu_aio_wait(); +} +return data.ret; +} + BlockInfo *bdrv_query_info(BlockDriverState *bs) { BlockInfo *info = g_malloc0(sizeof(*info)); diff --git a/include/block/block.h b/include/block/block.h index ce61883..8309fc1 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -279,6 +279,8 @@ int bdrv_co_discard(BlockDriverState *bs, int64_t sector_num, int nb_sectors); int bdrv_has_zero_init(BlockDriverState *bs); int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors, int *pnum); +int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base, +int64_t sector_num, int nb_sectors, int *pnum); void bdrv_set_on_error(BlockDriverState *bs, BlockdevOnError on_read_error, BlockdevOnError on_write_error); -- 1.7.1
[Qemu-devel] [PATCH v10 0/4] Add subcommand compare for qemu-img
This is 10th version of patch adding compare subcommand that compares two images. Compare has following criteria: - only data part is compared - unallocated sectors are not read to be zeroed/unallocated to compare rest - qemu-img returns: - 0 if images are identical - 1 if images differ - 2 error on execution v10: - rebased to kwolf's block-next branch due to conflict patch (patch 2) - exit status updates (patch 3) - minor non-functional fixes v9: - Merged patch 3 and 4 (subcommmand implementation and documentation) of v8 - Added basic test for qemu-img compare - Fixed incorrect indentation - add bdrv_is_allocated_above() return value check - Add description of exit codes into the documentation - minor non-functional changes v8: - minor grammar fixes (no behavior change) v7: - split patch into pieces - Quiet mode added for all relevant subcommands - check non-shared part of disk after shared one - minor docummentation and naming fixes v6: - added handling -?, -h options for compare subcommand v5 (only minor changes): - removed redundant comment - removed dead code (goto after help()) - set final total_sectors on first assignment v4: - Fixed various typos - Added functions for empty sector check and sector-to-bytes offset conversion - Fixed command-line parameters processing v3: - options -f/-F are orthogonal - documentation updated to new syntax and behavior - used byte offset instead of sector number for output v2: - changed option for second image format to -F - changed handling of -f and -F [1] - added strict mode (-s) - added quiet mode (-q) - improved output messages [2] - rename variables for larger image handling - added man page content Signed-off-by: Miroslav Rezanina mreza...@redhat.com Miroslav Rezanina (4): block: Add synchronous wrapper for bdrv_co_is_allocated_above qemu-img: Add Quiet mode option This patch adds new qemu-img subcommand that compares content of two disk images. qemu-iotests: Add qemu-img compare test block.c| 51 +- blockdev.c |6 +- include/block/block.h |5 +- qemu-img-cmds.hx | 34 ++-- qemu-img.c | 441 +++- qemu-img.texi | 56 ++ tests/qemu-iotests/048 | 77 tests/qemu-iotests/048.out | 25 +++ tests/qemu-iotests/group |1 + 9 files changed, 626 insertions(+), 70 deletions(-) create mode 100755 tests/qemu-iotests/048 create mode 100644 tests/qemu-iotests/048.out
[Qemu-devel] [PATCH v10 4/4] qemu-iotests: Add qemu-img compare test
Simple test for qemu-img compare to check it's working correctly. Signed-off-by: Miroslav Rezanina mreza...@redhat.com --- tests/qemu-iotests/048 | 77 tests/qemu-iotests/048.out | 25 ++ tests/qemu-iotests/group |1 + 3 files changed, 103 insertions(+), 0 deletions(-) create mode 100755 tests/qemu-iotests/048 create mode 100644 tests/qemu-iotests/048.out diff --git a/tests/qemu-iotests/048 b/tests/qemu-iotests/048 new file mode 100755 index 000..876653e --- /dev/null +++ b/tests/qemu-iotests/048 @@ -0,0 +1,77 @@ +#!/bin/bash +## +## qemu-img compare test +## +## +## Copyright (C) 2013 Red Hat, Inc. +## +## This program is free software; you can redistribute it and/or modify +## it under the terms of the GNU General Public License as published by +## the Free Software Foundation; either version 2 of the License, or +## (at your option) any later version. +## +## This program is distributed in the hope that it will be useful, +## but WITHOUT ANY WARRANTY; without even the implied warranty of +## MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +## GNU General Public License for more details. +## +## You should have received a copy of the GNU General Public License +## along with this program. If not, see http://www.gnu.org/licenses/. +## +# +# creator +owner=mreza...@redhat.com + +seq=`basename $0` +echo QA output created by $seq + +status=1# failure is the default! + +_cleanup() +{ +echo Cleanup +_cleanup_test_img +} +trap _cleanup; exit \$status 0 1 2 3 15 + +_compare() +{ +$QEMU_IMG compare $@ $TEST_IMG ${TEST_IMG2} +} + +# get standard environment, filters and checks +. ./common.rc +. ./common.filter +. ./common.pattern + +_supported_fmt raw qcow qcow2 qed +_supported_proto file +_supported_os Linux + +# Setup test basic parameters +TEST_IMG2=$TEST_IMG.2 +CLUSTER_SIZE=4096 +size=1024M + +_make_test_img $size +io_pattern write 524288 $CLUSTER_SIZE $CLUSTER_SIZE 4 45 + +# Compare identical images +cp $TEST_IMG ${TEST_IMG2} +_compare +_compare -q + +# Compare images with different size +$QEMU_IMG resize $TEST_IMG +512M +_compare +_compare -s + +# Compare images with different content +io_pattern write 1228800 $CLUSTER_SIZE 0 1 67 +_compare +io_pattern write 0 $CLUSTER_SIZE 0 1 123 +_compare + +# Cleanup +rm ${TEST_IMG2} +status=0 diff --git a/tests/qemu-iotests/048.out b/tests/qemu-iotests/048.out new file mode 100644 index 000..c276fc8 --- /dev/null +++ b/tests/qemu-iotests/048.out @@ -0,0 +1,25 @@ +QA output created by 048 +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824 +=== IO: pattern 45 +qemu-io wrote 4096/4096 bytes at offset 524288 +4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +qemu-io wrote 4096/4096 bytes at offset 528384 +4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +qemu-io wrote 4096/4096 bytes at offset 532480 +4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +qemu-io wrote 4096/4096 bytes at offset 536576 +4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +qemu-io Images are identical. +Image resized. +Warning: Image size mismatch! +Images are identical. +Strict mode: Image size mismatch! +=== IO: pattern 67 +qemu-io wrote 4096/4096 bytes at offset 1228800 +4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +qemu-io Content mismatch at offset 1228800! +=== IO: pattern 123 +qemu-io wrote 4096/4096 bytes at offset 0 +4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +qemu-io Content mismatch at offset 0! +Cleanup diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group index 1bbd2bf..1c0dfef 100644 --- a/tests/qemu-iotests/group +++ b/tests/qemu-iotests/group @@ -54,3 +54,4 @@ 045 rw auto 046 rw auto aio 047 rw auto +048 img auto quick -- 1.7.1
[Qemu-devel] [PATCH v10 3/4] This patch adds new qemu-img subcommand that compares content of two disk images.
Signed-off-by: Miroslav Rezanina mreza...@redhat.com --- qemu-img-cmds.hx |6 + qemu-img.c | 290 +- qemu-img.texi| 53 ++ 3 files changed, 348 insertions(+), 1 deletions(-) diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx index 9283776..4ca7e95 100644 --- a/qemu-img-cmds.hx +++ b/qemu-img-cmds.hx @@ -27,6 +27,12 @@ STEXI @item commit [-q] [-f @var{fmt}] [-t @var{cache}] @var{filename} ETEXI +DEF(compare, img_compare, +compare [-f fmt] [-F fmt] [-p] [-q] [-s] filename1 filename2) +STEXI +@item compare [-f @var{fmt}] [-F @var{fmt}] [-p] [-q] [-s] @var{filename1} @var{filename2} +ETEXI + DEF(convert, img_convert, convert [-c] [-p] [-q] [-f fmt] [-t cache] [-O output_fmt] [-o options] [-s snapshot_name] [-S sparse_size] filename [filename2 [...]] output_filename) STEXI diff --git a/qemu-img.c b/qemu-img.c index a2a2044..5f0c0c1 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -113,7 +113,12 @@ static void help(void) '-a' applies a snapshot (revert disk to saved state)\n '-c' creates a snapshot\n '-d' deletes a snapshot\n - '-l' lists all snapshots in the given image\n; + '-l' lists all snapshots in the given image\n + \n + Parameters to compare subcommand:\n + '-f' first image format\n + '-F' second image format\n + '-s' run in Strict mode - fail on different image size or sector allocation\n; printf(%s\nSupported formats:, help_msg); bdrv_iterate_format(format_print, NULL); @@ -817,6 +822,289 @@ static int compare_sectors(const uint8_t *buf1, const uint8_t *buf2, int n, #define IO_BUF_SIZE (2 * 1024 * 1024) +static int64_t sectors_to_bytes(int64_t sectors) +{ +return sectors BDRV_SECTOR_BITS; +} + +static int64_t sectors_to_process(int64_t total, int64_t from) +{ +return MIN(total - from, IO_BUF_SIZE BDRV_SECTOR_BITS); +} + +/* + * Check if passed sectors are empty (not allocated or contain only 0 bytes) + * + * Returns 0 in case sectors are filled with 0, 1 if sectors contain non-zero + * data and negative value on error. + * + * @param bs: Driver used for accessing file + * @param sect_num: Number of first sector to check + * @param sect_count: Number of sectors to check + * @param filename: Name of disk file we are checking (logging purpose) + * @param buffer: Allocated buffer for storing read data + * @param quiet: Flag for quiet mode + */ +static int check_empty_sectors(BlockDriverState *bs, int64_t sect_num, + int sect_count, const char *filename, + uint8_t *buffer, bool quiet) +{ +int pnum, ret = 0; +ret = bdrv_read(bs, sect_num, buffer, sect_count); +if (ret 0) { +error_report(Error while reading offset % PRId64 of %s: %s, + sectors_to_bytes(sect_num), filename, strerror(-ret)); +return ret; +} +ret = is_allocated_sectors(buffer, sect_count, pnum); +if (ret || pnum != sect_count) { +qprintf(quiet, Content mismatch at offset % PRId64 !\n, +sectors_to_bytes(ret ? sect_num : sect_num + pnum)); +return 1; +} + +return 0; +} + +/* + * Compares two images. Exit codes: + * + * 0 - Images are identical + * 1 - Images differ + * 1 - Error occurred + */ +static int img_compare(int argc, char **argv) +{ +const char *fmt1 = NULL, *fmt2 = NULL, *filename1, *filename2; +BlockDriverState *bs1, *bs2; +int64_t total_sectors1, total_sectors2; +uint8_t *buf1 = NULL, *buf2 = NULL; +int pnum1, pnum2; +int allocated1, allocated2; +int ret = 0; /* return value - 0 Ident, 1 Different, 1 Error */ +bool progress = false, quiet = false, strict = false; +int64_t total_sectors; +int64_t sector_num = 0; +int64_t nb_sectors; +int c, pnum; +uint64_t bs_sectors; +uint64_t progress_base; + +for (;;) { +c = getopt(argc, argv, hpf:F:sq); +if (c == -1) { +break; +} +switch (c) { +case '?': +case 'h': +help(); +break; +case 'f': +fmt1 = optarg; +break; +case 'F': +fmt2 = optarg; +break; +case 'p': +progress = true; +break; +case 'q': +quiet = true; +break; +case 's': +strict = true; +break; +} +} + +/* Progress is not shown in Quiet mode */ +if (quiet) { +progress = false; +} + + +if (optind argc - 2) { +help(); +} +filename1 = argv[optind++]; +filename2 = argv[optind++]; + +/* Initialize before goto out */ +qemu_progress_init(progress, 2.0); + +bs1 = bdrv_new_open(filename1, fmt1, BDRV_O_FLAGS, true, quiet); +if (!bs1) { +error_report(Can't open file %s
[Qemu-devel] [PATCH v10 2/4] qemu-img: Add Quiet mode option
There can be a need to turn output to stdout off. This patch adds a -q option that enable Quiet mode. In Quiet mode, only errors are printed out. Signed-off-by: Miroslav Rezanina mreza...@redhat.com --- block.c | 12 ++-- blockdev.c|6 +- include/block/block.h |3 +- qemu-img-cmds.hx | 28 +- qemu-img.c| 151 ++--- qemu-img.texi |3 + 6 files changed, 134 insertions(+), 69 deletions(-) diff --git a/block.c b/block.c index 08039d2..a4d7125 100644 --- a/block.c +++ b/block.c @@ -4470,7 +4470,8 @@ bdrv_acct_done(BlockDriverState *bs, BlockAcctCookie *cookie) void bdrv_img_create(const char *filename, const char *fmt, const char *base_filename, const char *base_fmt, - char *options, uint64_t img_size, int flags, Error **errp) + char *options, uint64_t img_size, int flags, + Error **errp, bool quiet) { QEMUOptionParameter *param = NULL, *create_options = NULL; QEMUOptionParameter *backing_fmt, *backing_file, *size; @@ -4579,10 +4580,11 @@ void bdrv_img_create(const char *filename, const char *fmt, } } -printf(Formatting '%s', fmt=%s , filename, fmt); -print_option_parameters(param); -puts(); - +if (!quiet) { +printf(Formatting '%s', fmt=%s , filename, fmt); +print_option_parameters(param); +puts(); +} ret = bdrv_create(drv, filename, param); if (ret 0) { if (ret == -ENOTSUP) { diff --git a/blockdev.c b/blockdev.c index 63e6f1e..3859ac2 100644 --- a/blockdev.c +++ b/blockdev.c @@ -791,7 +791,7 @@ void qmp_transaction(BlockdevActionList *dev_list, Error **errp) bdrv_img_create(new_image_file, format, states-old_bs-filename, states-old_bs-drv-format_name, -NULL, -1, flags, local_err); +NULL, -1, flags, local_err, false); if (error_is_set(local_err)) { error_propagate(errp, local_err); goto delete_and_fail; @@ -1284,7 +1284,7 @@ void qmp_drive_mirror(const char *device, const char *target, /* create new image w/o backing file */ assert(format drv); bdrv_img_create(target, format, -NULL, NULL, NULL, size, flags, local_err); +NULL, NULL, NULL, size, flags, local_err, false); } else { switch (mode) { case NEW_IMAGE_MODE_EXISTING: @@ -1295,7 +1295,7 @@ void qmp_drive_mirror(const char *device, const char *target, bdrv_img_create(target, format, source-filename, source-drv-format_name, -NULL, size, flags, local_err); +NULL, size, flags, local_err, false); break; default: abort(); diff --git a/include/block/block.h b/include/block/block.h index 8309fc1..371b42f 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -352,7 +352,8 @@ int bdrv_load_vmstate(BlockDriverState *bs, uint8_t *buf, void bdrv_img_create(const char *filename, const char *fmt, const char *base_filename, const char *base_fmt, - char *options, uint64_t img_size, int flags, Error **errp); + char *options, uint64_t img_size, int flags, + Error **errp, bool quiet); void bdrv_set_buffer_alignment(BlockDriverState *bs, int align); void *qemu_blockalign(BlockDriverState *bs, size_t size); diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx index 259fc14..9283776 100644 --- a/qemu-img-cmds.hx +++ b/qemu-img-cmds.hx @@ -10,27 +10,27 @@ STEXI ETEXI DEF(check, img_check, -check [-f fmt] [--output=ofmt] [-r [leaks | all]] filename) +check [-q] [-f fmt] [--output=ofmt] [-r [leaks | all]] filename) STEXI -@item check [-f @var{fmt}] [--output=@var{ofmt}] [-r [leaks | all]] @var{filename} +@item check [-q] [-f @var{fmt}] [--output=@var{ofmt}] [-r [leaks | all]] @var{filename} ETEXI DEF(create, img_create, -create [-f fmt] [-o options] filename [size]) +create [-q] [-f fmt] [-o options] filename [size]) STEXI -@item create [-f @var{fmt}] [-o @var{options}] @var{filename} [@var{size}] +@item create [-q] [-f @var{fmt}] [-o @var{options}] @var{filename} [@var{size}] ETEXI DEF(commit, img_commit, -commit [-f fmt] [-t cache] filename) +commit [-q] [-f fmt] [-t cache] filename) STEXI -@item commit [-f @var{fmt}] [-t @var{cache}] @var{filename} +@item commit [-q] [-f @var{fmt}] [-t @var{cache}] @var{filename} ETEXI DEF(convert, img_convert, -convert [-c] [-p] [-f fmt] [-t cache] [-O output_fmt] [-o options] [-s snapshot_name] [-S sparse_size] filename [filename2 [...]] output_filename) +convert [-c
Re: [Qemu-devel] [PATCH 2/4] qemu-img: Add Quiet mode option
Hi Eric, - Original Message - From: Eric Blake ebl...@redhat.com To: Miroslav Rezanina mreza...@redhat.com Cc: qemu-devel@nongnu.org, kw...@redhat.com, pbonz...@redhat.com, stefa...@redhat.com Sent: Friday, February 8, 2013 6:07:35 PM Subject: Re: [PATCH 2/4] qemu-img: Add Quiet mode option On 02/08/2013 01:33 AM, Miroslav Rezanina wrote: There can be a need to turn output to stdout off. This patch adds a -q option that enable Quiet mode. In Quiet mode, only errors are printed out. Signed-off-by: Miroslav Rezanina mreza...@redhat.com --- if (result.bfi.total_clusters != 0 result.bfi.allocated_clusters != 0) { -printf(% PRId64 /% PRId64 = %0.2f%% allocated, %0.2f%% fragmented\n, +/* BEWARE: parameter list not indented due to long expressions */ This code is being touched in an independent patch, with a solution much nicer than adding a BEWARE comment: https://lists.gnu.org/archive/html/qemu-devel/2013-02/msg01101.html Yeah, you're right. Unfortunatelly, this patch was sent after this one was prepared. I agree, that my solution is not best but I do not want to do any unnecessary change in code. +qprintf(quiet, +% PRId64 /% PRId64 = %0.2f%% allocated, %0.2f%% fragmented\n, result.bfi.allocated_clusters, result.bfi.total_clusters, result.bfi.allocated_clusters * 100.0 / result.bfi.total_clusters, result.bfi.fragmented_clusters * 100.0 / result.bfi.allocated_clusters); +/* end of parameter list */ } This /* end of parameter list */ comment is bogus. @@ -742,9 +775,16 @@ static int img_convert(int argc, char **argv) case 't': cache = optarg; break; +case 'q': +quiet = true; +break; } } +if (quiet) { +progress = 0; +} I'm still not sure I buy this. Since '-p' normally adds output, and '-q' normally removes output, I'm wondering if it is better to follow conventions of other apps like ls, when when faced with mutually competing options will favor the last one specified. Something like: qemu-img convert -p -q = turns off -p with quiet results qemu-img convert -q -p = turns off -q, with progress bar results Or, it might be worth actually erroring out if both -p and -q are present, in any order. I do not like when order of the - parameters matter, so I'm not going to implement this behavior. I could live with error for this combination. However, I still feel that logic -p adds something to output and -q turns output off is correct. -p does not activate output, just improve it. +++ b/qemu-img.texi @@ -54,6 +54,9 @@ indicates that target image must be compressed (qcow format only) with or without a command shows help and lists the supported formats @item -p display progress bar (convert and rebase commands only) +@item -q +Quiet mode - do not print any output (except errors). There's no progress bar +in case both @var{-q} and @var{-p} options are used. But at least your documentation matches your code, so I guess I can live with your choice that -q is always more powerful than -p, no matter what order they appear in. You have an extra space before the second @var. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org -- Miroslav Rezanina Software Engineer - Virtualization Team
Re: [Qemu-devel] [PATCH 3/4] This patch adds new qemu-img subcommand that compares content of two disk images.
- Original Message - From: Eric Blake ebl...@redhat.com To: Miroslav Rezanina mreza...@redhat.com Cc: qemu-devel@nongnu.org, kw...@redhat.com, pbonz...@redhat.com, stefa...@redhat.com Sent: Friday, February 8, 2013 8:19:45 PM Subject: Re: [PATCH 3/4] This patch adds new qemu-img subcommand that compares content of two disk images. On 02/08/2013 01:33 AM, Miroslav Rezanina wrote: This patch adds new qemu-img subcommand that compares content of two disk images. +static int64_t sectors_to_process(int64_t total, int64_t from) +{ +return MIN((total - from), (IO_BUF_SIZE BDRV_SECTOR_BITS)); Why the spurious ()? This can be written: return MIN(total - from, IO_BUF_SIZE BDRV_SECTOR_BITS); unless the definition of MIN() is horribly busted (in which case, fix the broken macro, don't make callers suffer). Just my habit - do not trust code you did not write (and not event the one you wrote). +/* + * Compares two images. Exit codes: + * + * 0 - Images are identical + * 1 - Images differ + * 0 - Error occurred + */ +static int img_compare(int argc, char **argv) +{ +const char *fmt1 = NULL, *fmt2 = NULL, *filename1, *filename2; +BlockDriverState *bs1, *bs2; +int64_t total_sectors1, total_sectors2; +uint8_t *buf1 = NULL, *buf2 = NULL; +int pnum1, pnum2; +int allocated1, allocated2; +int ret = 0; /* return value - 0 Ident, 1 Different, 2 Error */ This comment disagrees with the comment at the top of the function. I would just delete the comment here, instead of trying to keep them in sync. Yeah, I miss this one. Thx for pointing it out. Commit the changes recorded in @var{filename} in its base image. +@item compare [-f @var{fmt}] [-F @var{fmt}] [-p] [-s] [-q] @var{filename1} @var{filename2} While this line is okay long... + +Check if two images have the same content. You can compare images with +different format or settings. + +The format is probed unless you specify it by @var{-f} (used for @var{filename1}) and/or @var{-F} (used for @var{filename2}) option. ...this line should be wrapped. You're right.. + +Compare exits with @code{0} in case the images are equal and with @code{1} +in case the images differ. Negative exit code means an error occured during s/occured/occurred/ There's no such thing as a negative exit in shell. Rather, you should document that an exit code 1 indicates an error during execution. Oh, I was not aware of this fact. Thanks for explaining, have to be fixed. +execution and standard error output should contain an error message. +Error exit codes are: + +@table @option + +@item -1 +Error on opening an image +@item -2 +Error on cheking a sector allorcation s/cheking/checking/ s/allorcation/allocation/ +@item -3 +Error on reading data Does it make sense to be this fine-grained in exit reporting? I'd be okay with blindly using exit status 2 for all failures. But if you really do want to call out this many different statuses, I'd list a single table for ALL possible exits, rather than prose about success and table for failure, as in: 0 - images are identical 1 - images differ 2 - error opening an image 3 - error checking sector allocation 4 - error reading from an image Isn't failure to determine sector allocation a subset of failure to read from an image? This table is describing behavior prior the last change. I forget to update it so the values are not correct. However, allocation checking error is (or should be) on bdrv_is_allocated_above call and reading error is on bdrv_read. (Yeah, it can be problem with reading but this is hidden in bdrv). -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org -- Miroslav Rezanina Software Engineer - Virtualization Team
Re: [Qemu-devel] [PATCH v8 3/4] qemu-img: Add compare subcommand
See response inline, if no response that comment will be handled in next version. - Original Message - From: Kevin Wolf kw...@redhat.com To: Miroslav Rezanina mreza...@redhat.com Cc: qemu-devel@nongnu.org Sent: Tuesday, January 29, 2013 12:28:22 PM Subject: Re: [Qemu-devel] [PATCH v8 3/4] qemu-img: Add compare subcommand Am 14.01.2013 11:26, schrieb Miroslav Rezanina: This patch adds new qemu-img subcommand that compares content of two disk images. Signed-off-by: Miroslav Rezanina mreza...@redhat.com --- qemu-img-cmds.hx |6 ++ qemu-img.c | 266 ++ 2 files changed, 272 insertions(+), 0 deletions(-) You also need to add documentation to qemu-img.texi. A description of the exit codes should probably go there. As you found out documentation is in patch 4. This code here is needed for proper working so it's not part of patch 4. I put documentation in separate patch to have one patch with execution code only. I will add the result codes in texi file, I forgot them. diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx index 90b93e0..bc1ccfa 100644 --- a/qemu-img-cmds.hx +++ b/qemu-img-cmds.hx @@ -27,6 +27,12 @@ STEXI @item commit [-q] [-f @var{fmt}] [-t @var{cache}] @var{filename} ETEXI +DEF(compare, img_compare, +compare [-f fmt] [-F fmt] [-p] [-q] [-s] filename1 filename2) +STEXI +@item compare [-f @var{fmt}] [-F @var{fmt}] [-p] [-q] [-s] @var{filename1} @var{filename2} +ETEXI + DEF(convert, img_convert, convert [-c] [-p] [-q] [-f fmt] [-t cache] [-O output_fmt] [-o options] [-s snapshot_name] [-S sparse_size] filename [filename2 [...]] output_filename) STEXI diff --git a/qemu-img.c b/qemu-img.c index 8d55829..0c12692 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -693,6 +693,272 @@ static int compare_sectors(const uint8_t *buf1, const uint8_t *buf2, int n, #define IO_BUF_SIZE (2 * 1024 * 1024) +static int64_t sectors_to_bytes(int64_t sectors) +{ +return sectors BDRV_SECTOR_BITS; +} + +static int64_t sectors_to_process(int64_t total, int64_t from) +{ +int64_t ret = total - from; + +if (ret (IO_BUF_SIZE BDRV_SECTOR_BITS)) { +return IO_BUF_SIZE BDRV_SECTOR_BITS; +} + +return ret; +} return MIN(total - from, IO_BUF_SIZE BDRV_SECTOR_BITS) would be a bit simpler. + +/* + * Check if passed sectors are empty (not allocated or contain only 0 bytes) + * + * Returns 0 in case sectors are filled with 0, 1 if sectors contain non-zero + * data and negative value on error. + * + * @param bs: Driver used for accessing file + * @param sect_num: Number of first sector to check + * @param sect_count: Number of sectors to check + * @param filename: Name of disk file we are checking (logging purpose) + * @param buffer: Allocated buffer for storing read data + * @param quiet: Flag for quiet mode + */ +static int check_empty_sectors(BlockDriverState *bs, int64_t sect_num, + int sect_count, const char *filename, + uint8_t *buffer, bool quiet) +{ +int pnum, ret = 0; +ret = bdrv_read(bs, sect_num, buffer, sect_count); +if (ret 0) { +error_report(Error while reading offset % PRId64 of %s: %s, + sectors_to_bytes(sect_num), filename, strerror(-ret)); +return ret; +} +ret = is_allocated_sectors(buffer, sect_count, pnum); +if (ret || pnum != sect_count) { +qprintf(quiet, Content mismatch at offset % PRId64 !\n, +sectors_to_bytes(ret ? sect_num : sect_num + pnum)); +return 1; +} + +return 0; +} + +/* + * Compares two images. Exit codes: + * + * 0 - Images are identical + * 1 - Images differ + * 2 - Error occurred + */ +static int img_compare(int argc, char **argv) +{ +const char *fmt1 = NULL, *fmt2 = NULL, *filename1, *filename2; +BlockDriverState *bs1, *bs2; +int64_t total_sectors1, total_sectors2; +uint8_t *buf1 = NULL, *buf2 = NULL; +int pnum1, pnum2; +int allocated1, allocated2; +int ret = 0; /* return value - 0 Ident, 1 Different, 2 Error */ +int progress = 0, quiet = 0, strict = 0; Please use bool instead. +int64_t total_sectors; +int64_t sector_num = 0; +int64_t nb_sectors; +int c, pnum; +uint64_t bs_sectors; +uint64_t progress_base; + +for (;;) { +c = getopt(argc, argv, hpf:F:sq); +if (c == -1) { +break; +} +switch (c) { +case '?': +case 'h': +help(); +break; +case 'f': +fmt1 = optarg; +break; +case 'F': +fmt2 = optarg; +break
[Qemu-devel] [PATCH v8 0/4] Add subcommand compare for qemu-img
This is 8th version of patch adding compare subcommand that compares two images. Compare has following criteria: - only data part is compared - unallocated sectors are not read - in case of different image size, exceeding part of bigger disk has to be zeroed/unallocated to compare rest - qemu-img returns: - 0 if images are identical - 1 if images differ - 2 on error v8: - minor grammar fixes (no behavior change) v7: - split patch into pieces - Quiet mode added for all relevant subcommands - check non-shared part of disk after shared one - minor docummentation and naming fixes v6: - added handling -?, -h options for compare subcommand v5 (only minor changes): - removed redundant comment - removed dead code (goto after help()) - set final total_sectors on first assignment v4: - Fixed various typos - Added functions for empty sector check and sector-to-bytes offset conversion - Fixed command-line parameters processing v3: - options -f/-F are orthogonal - documentation updated to new syntax and behavior - used byte offset instead of sector number for output v2: - changed option for second image format to -F - changed handling of -f and -F [1] - added strict mode (-s) - added quiet mode (-q) - improved output messages [2] - rename variables for larger image handling - added man page content Signed-off-by: Miroslav Rezanina mreza...@redhat.com Miroslav Rezanina (4): block: Add synchronous wrapper for bdrv_co_is_allocated_above qemu-img: Add Quiet mode option qemu-img: Add compare subcommand Add qemu-img compare documentation block.c | 51 ++- blockdev.c|6 +- include/block/block.h |5 +- qemu-img-cmds.hx | 34 +++-- qemu-img.c| 381 + qemu-img.texi | 35 + 6 files changed, 461 insertions(+), 51 deletions(-)
[Qemu-devel] [PATCH v8 1/4] block: Add synchronous wrapper for bdrv_co_is_allocated_above
There's no synchronous wrapper for bdrv_co_is_allocated_above function so it's not possible to check for sector allocation in in mage with a backing file. This patch adds the missing synchronous wrapper. Signed-off-by: Miroslav Rezanina mreza...@redhat.com --- block.c | 39 +++ include/block/block.h |2 ++ 2 files changed, 41 insertions(+), 0 deletions(-) diff --git a/block.c b/block.c index 60873ea..8ea8bb7 100644 --- a/block.c +++ b/block.c @@ -2716,6 +2716,7 @@ int bdrv_has_zero_init(BlockDriverState *bs) typedef struct BdrvCoIsAllocatedData { BlockDriverState *bs; +BlockDriverState *base; int64_t sector_num; int nb_sectors; int *pnum; @@ -2846,6 +2847,44 @@ int coroutine_fn bdrv_co_is_allocated_above(BlockDriverState *top, return 0; } +/* Coroutine wrapper for bdrv_is_allocated_above() */ +static void coroutine_fn bdrv_is_allocated_above_co_entry(void *opaque) +{ +BdrvCoIsAllocatedData *data = opaque; +BlockDriverState *top = data-bs; +BlockDriverState *base = data-base; + +data-ret = bdrv_co_is_allocated_above(top, base, data-sector_num, + data-nb_sectors, data-pnum); +data-done = true; +} + +/* + * Synchronous wrapper around bdrv_co_is_allocated_above(). + * + * See bdrv_co_is_allocated_above() for details. + */ +int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base, +int64_t sector_num, int nb_sectors, int *pnum) +{ +Coroutine *co; +BdrvCoIsAllocatedData data = { +.bs = top, +.base = base, +.sector_num = sector_num, +.nb_sectors = nb_sectors, +.pnum = pnum, +.done = false, +}; + +co = qemu_coroutine_create(bdrv_is_allocated_above_co_entry); +qemu_coroutine_enter(co, data); +while (!data.done) { +qemu_aio_wait(); +} +return data.ret; +} + BlockInfo *bdrv_query_info(BlockDriverState *bs) { BlockInfo *info = g_malloc0(sizeof(*info)); diff --git a/include/block/block.h b/include/block/block.h index 0719339..ec62d92 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -278,6 +278,8 @@ int bdrv_co_discard(BlockDriverState *bs, int64_t sector_num, int nb_sectors); int bdrv_has_zero_init(BlockDriverState *bs); int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors, int *pnum); +int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base, +int64_t sector_num, int nb_sectors, int *pnum); void bdrv_set_on_error(BlockDriverState *bs, BlockdevOnError on_read_error, BlockdevOnError on_write_error); -- 1.7.1
[Qemu-devel] [PATCH v8 2/4] qemu-img: Add Quiet mode option
There can be a need to turn output to stdout off. This patch adds a -q option that enable Quiet mode. In Quiet mode, only errors are printed out. Signed-off-by: Miroslav Rezanina mreza...@redhat.com --- block.c | 12 +++-- blockdev.c|6 +- include/block/block.h |3 +- qemu-img-cmds.hx | 28 ++-- qemu-img.c| 108 qemu-img.texi |3 + 6 files changed, 110 insertions(+), 50 deletions(-) diff --git a/block.c b/block.c index 8ea8bb7..e563ca8 100644 --- a/block.c +++ b/block.c @@ -4512,7 +4512,8 @@ bdrv_acct_done(BlockDriverState *bs, BlockAcctCookie *cookie) void bdrv_img_create(const char *filename, const char *fmt, const char *base_filename, const char *base_fmt, - char *options, uint64_t img_size, int flags, Error **errp) + char *options, uint64_t img_size, int flags, + Error **errp, bool quiet) { QEMUOptionParameter *param = NULL, *create_options = NULL; QEMUOptionParameter *backing_fmt, *backing_file, *size; @@ -4621,10 +4622,11 @@ void bdrv_img_create(const char *filename, const char *fmt, } } -printf(Formatting '%s', fmt=%s , filename, fmt); -print_option_parameters(param); -puts(); - +if (!quiet) { +printf(Formatting '%s', fmt=%s , filename, fmt); +print_option_parameters(param); +puts(); +} ret = bdrv_create(drv, filename, param); if (ret 0) { if (ret == -ENOTSUP) { diff --git a/blockdev.c b/blockdev.c index d724e2d..3e377f9 100644 --- a/blockdev.c +++ b/blockdev.c @@ -790,7 +790,7 @@ void qmp_transaction(BlockdevActionList *dev_list, Error **errp) bdrv_img_create(new_image_file, format, states-old_bs-filename, states-old_bs-drv-format_name, -NULL, -1, flags, local_err); +NULL, -1, flags, local_err, false); if (error_is_set(local_err)) { error_propagate(errp, local_err); goto delete_and_fail; @@ -1265,7 +1265,7 @@ void qmp_drive_mirror(const char *device, const char *target, bdrv_get_geometry(bs, size); size *= 512; bdrv_img_create(target, format, -NULL, NULL, NULL, size, flags, local_err); +NULL, NULL, NULL, size, flags, local_err, false); } else { switch (mode) { case NEW_IMAGE_MODE_EXISTING: @@ -1276,7 +1276,7 @@ void qmp_drive_mirror(const char *device, const char *target, bdrv_img_create(target, format, source-filename, source-drv-format_name, -NULL, -1, flags, local_err); +NULL, -1, flags, local_err, false); break; default: abort(); diff --git a/include/block/block.h b/include/block/block.h index ec62d92..03948c6 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -347,7 +347,8 @@ int bdrv_load_vmstate(BlockDriverState *bs, uint8_t *buf, void bdrv_img_create(const char *filename, const char *fmt, const char *base_filename, const char *base_fmt, - char *options, uint64_t img_size, int flags, Error **errp); + char *options, uint64_t img_size, int flags, + Error **errp, bool quiet); void bdrv_set_buffer_alignment(BlockDriverState *bs, int align); void *qemu_blockalign(BlockDriverState *bs, size_t size); diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx index a181363..90b93e0 100644 --- a/qemu-img-cmds.hx +++ b/qemu-img-cmds.hx @@ -10,27 +10,27 @@ STEXI ETEXI DEF(check, img_check, -check [-f fmt] [-r [leaks | all]] filename) +check [-q] [-f fmt] [-r [leaks | all]] filename) STEXI -@item check [-f @var{fmt}] [-r [leaks | all]] @var{filename} +@item check [-q] [-f @var{fmt}] [-r [leaks | all]] @var{filename} ETEXI DEF(create, img_create, -create [-f fmt] [-o options] filename [size]) +create [-q] [-f fmt] [-o options] filename [size]) STEXI -@item create [-f @var{fmt}] [-o @var{options}] @var{filename} [@var{size}] +@item create [-q] [-f @var{fmt}] [-o @var{options}] @var{filename} [@var{size}] ETEXI DEF(commit, img_commit, -commit [-f fmt] [-t cache] filename) +commit [-q] [-f fmt] [-t cache] filename) STEXI -@item commit [-f @var{fmt}] [-t @var{cache}] @var{filename} +@item commit [-q] [-f @var{fmt}] [-t @var{cache}] @var{filename} ETEXI DEF(convert, img_convert, -convert [-c] [-p] [-f fmt] [-t cache] [-O output_fmt] [-o options] [-s snapshot_name] [-S sparse_size] filename [filename2 [...]] output_filename) +convert [-c] [-p] [-q] [-f fmt] [-t cache] [-O output_fmt] [-o options] [-s snapshot_name] [-S sparse_size
[Qemu-devel] [PATCH v8 3/4] qemu-img: Add compare subcommand
This patch adds new qemu-img subcommand that compares content of two disk images. Signed-off-by: Miroslav Rezanina mreza...@redhat.com --- qemu-img-cmds.hx |6 ++ qemu-img.c | 266 ++ 2 files changed, 272 insertions(+), 0 deletions(-) diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx index 90b93e0..bc1ccfa 100644 --- a/qemu-img-cmds.hx +++ b/qemu-img-cmds.hx @@ -27,6 +27,12 @@ STEXI @item commit [-q] [-f @var{fmt}] [-t @var{cache}] @var{filename} ETEXI +DEF(compare, img_compare, +compare [-f fmt] [-F fmt] [-p] [-q] [-s] filename1 filename2) +STEXI +@item compare [-f @var{fmt}] [-F @var{fmt}] [-p] [-q] [-s] @var{filename1} @var{filename2} +ETEXI + DEF(convert, img_convert, convert [-c] [-p] [-q] [-f fmt] [-t cache] [-O output_fmt] [-o options] [-s snapshot_name] [-S sparse_size] filename [filename2 [...]] output_filename) STEXI diff --git a/qemu-img.c b/qemu-img.c index 8d55829..0c12692 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -693,6 +693,272 @@ static int compare_sectors(const uint8_t *buf1, const uint8_t *buf2, int n, #define IO_BUF_SIZE (2 * 1024 * 1024) +static int64_t sectors_to_bytes(int64_t sectors) +{ +return sectors BDRV_SECTOR_BITS; +} + +static int64_t sectors_to_process(int64_t total, int64_t from) +{ +int64_t ret = total - from; + +if (ret (IO_BUF_SIZE BDRV_SECTOR_BITS)) { +return IO_BUF_SIZE BDRV_SECTOR_BITS; +} + +return ret; +} + +/* + * Check if passed sectors are empty (not allocated or contain only 0 bytes) + * + * Returns 0 in case sectors are filled with 0, 1 if sectors contain non-zero + * data and negative value on error. + * + * @param bs: Driver used for accessing file + * @param sect_num: Number of first sector to check + * @param sect_count: Number of sectors to check + * @param filename: Name of disk file we are checking (logging purpose) + * @param buffer: Allocated buffer for storing read data + * @param quiet: Flag for quiet mode + */ +static int check_empty_sectors(BlockDriverState *bs, int64_t sect_num, + int sect_count, const char *filename, + uint8_t *buffer, bool quiet) +{ +int pnum, ret = 0; +ret = bdrv_read(bs, sect_num, buffer, sect_count); +if (ret 0) { +error_report(Error while reading offset % PRId64 of %s: %s, + sectors_to_bytes(sect_num), filename, strerror(-ret)); +return ret; +} +ret = is_allocated_sectors(buffer, sect_count, pnum); +if (ret || pnum != sect_count) { +qprintf(quiet, Content mismatch at offset % PRId64 !\n, +sectors_to_bytes(ret ? sect_num : sect_num + pnum)); +return 1; +} + +return 0; +} + +/* + * Compares two images. Exit codes: + * + * 0 - Images are identical + * 1 - Images differ + * 2 - Error occurred + */ +static int img_compare(int argc, char **argv) +{ +const char *fmt1 = NULL, *fmt2 = NULL, *filename1, *filename2; +BlockDriverState *bs1, *bs2; +int64_t total_sectors1, total_sectors2; +uint8_t *buf1 = NULL, *buf2 = NULL; +int pnum1, pnum2; +int allocated1, allocated2; +int ret = 0; /* return value - 0 Ident, 1 Different, 2 Error */ +int progress = 0, quiet = 0, strict = 0; +int64_t total_sectors; +int64_t sector_num = 0; +int64_t nb_sectors; +int c, pnum; +uint64_t bs_sectors; +uint64_t progress_base; + +for (;;) { +c = getopt(argc, argv, hpf:F:sq); +if (c == -1) { +break; +} +switch (c) { +case '?': +case 'h': +help(); +break; +case 'f': +fmt1 = optarg; +break; +case 'F': +fmt2 = optarg; +break; +case 'p': +progress = 1; +break; +case 'q': +quiet = 1; +break; +case 's': +strict = 1; +break; +} +} + +/* Progress is not shown in Quiet mode */ +if (quiet) { +progress = 0; +} + + +if (optind argc - 2) { +help(); +} +filename1 = argv[optind++]; +filename2 = argv[optind++]; + +/* Initialize before goto out */ +qemu_progress_init(progress, 2.0); + +bs1 = bdrv_new_open(filename1, fmt1, BDRV_O_FLAGS, true, quiet); +if (!bs1) { +error_report(Can't open file %s, filename1); +ret = 2; +goto out3; +} + +bs2 = bdrv_new_open(filename2, fmt2, BDRV_O_FLAGS, true, quiet); +if (!bs2) { +error_report(Can't open file %s, filename2); +ret = 2; +goto out2; +} + +buf1 = qemu_blockalign(bs1, IO_BUF_SIZE); +buf2 = qemu_blockalign(bs2, IO_BUF_SIZE); +bdrv_get_geometry(bs1, bs_sectors); +total_sectors1 = bs_sectors; +bdrv_get_geometry(bs2, bs_sectors); +total_sectors2 = bs_sectors; +total_sectors = MIN(total_sectors1
[Qemu-devel] [PATCH v8 4/4] Add qemu-img compare documentation
Adding documentation for new qemu-img subcommand compare. Signed-off-by: Miroslav Rezanina mreza...@redhat.com --- qemu-img.c|7 ++- qemu-img.texi | 32 2 files changed, 38 insertions(+), 1 deletions(-) diff --git a/qemu-img.c b/qemu-img.c index 0c12692..6aebdc3 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -103,7 +103,12 @@ static void help(void) '-a' applies a snapshot (revert disk to saved state)\n '-c' creates a snapshot\n '-d' deletes a snapshot\n - '-l' lists all snapshots in the given image\n; + '-l' lists all snapshots in the given image\n + \n + Parameters to compare subcommand:\n + '-f' first image format\n + '-F' second image format\n + '-s' run in Strict mode - fail on different image size or sector allocation\n; printf(%s\nSupported formats:, help_msg); bdrv_iterate_format(format_print, NULL); diff --git a/qemu-img.texi b/qemu-img.texi index 4fdb19a..90f581a 100644 --- a/qemu-img.texi +++ b/qemu-img.texi @@ -84,6 +84,18 @@ deletes a snapshot lists all snapshots in the given image @end table +Parameters to compare subcommand: + +@table @option + +@item -f +First image format +@item -F +Second image format +@item -s +Strict mode - fail on on different image size or sector allocation +@end table + Command description: @table @option @@ -117,6 +129,26 @@ it doesn't need to be specified separately in this case. Commit the changes recorded in @var{filename} in its base image. +@item compare [-f @var{fmt}] [-F @var{fmt}] [-p] [-s] [-q] @var{filename1} @var{filename2} + +Check if two images have the same content. You can compare images with +different format or settings. + +The format is probed unless you specify it by @var{-f} (used for @var{filename1}) and/or @var{-F} (used for @var{filename2}) option. + +By default, images with different size are considered identical if the larger +image contains only unallocated and/or zeroed sectors in the area after the end +of the other image. In addition, if any sector is not allocated in one image +and contains only zero bytes in the second one, it is evaluated as equal. You +can use Strict mode by specifying the @var{-s} option. When compare runs in +Strict mode, it fails in case image size differs or a sector is allocated in +one image and is not allocated in the second one. + +By default, compare prints out a result message. This message displays +information that both images are same or the position of the first different +byte. In addition, result message can report different image size in case +Strict mode is used. + @item convert [-c] [-p] [-f @var{fmt}] [-t @var{cache}] [-O @var{output_fmt}] [-o @var{options}] [-s @var{snapshot_name}] [-S @var{sparse_size}] @var{filename} [@var{filename2} [...]] @var{output_filename} Convert the disk image @var{filename} or a snapshot @var{snapshot_name} to disk image @var{output_filename} -- 1.7.1
Re: [Qemu-devel] [PATCH v7 2/4] qemu-img: Add Quiet mode option
Hi Eric, thanks for review, reply inline. - Original Message - From: Eric Blake ebl...@redhat.com To: mreza...@redhat.com Cc: qemu-devel@nongnu.org, kw...@redhat.com, pbonz...@redhat.com, stefa...@redhat.com Sent: Wednesday, December 19, 2012 7:06:35 PM Subject: Re: [Qemu-devel] [PATCH v7 2/4] qemu-img: Add Quiet mode option On 12/17/2012 06:39 AM, mreza...@redhat.com wrote: From: Miroslav Rezanina mreza...@redhat.com Your git send-email settings threaded each message as a reply to the other, rather than the more typical setting of threading messages only as a reply to the cover letter. You may want to do 'git config format.thread shallow' rather than your current deep approach. I use temporary machine for sending patches as my usual one was not usable for me in this case, so setting can be incorrect. I will take better care of this next time. There can be need to turn output to stdout off. This patch adds a -q option that s/be need/be a need/ enable Quiet mode. In Quiet mode, only errors are printed out. Signed-off-by: Miroslav Rezanina mreza...@redhat.com --- block.c | 11 +++--- block.h | 2 +- blockdev.c | 6 ++-- qemu-img-cmds.hx | 28 +++ qemu-img.c | 108 +-- qemu-img.texi| 3 ++ 6 files changed, 108 insertions(+), 50 deletions(-) @@ -1275,7 +1275,7 @@ void qmp_drive_mirror(const char *device, const char *target, ret = bdrv_img_create(target, format, source-filename, source-drv-format_name, - NULL, -1, flags); + NULL, -1, flags,false); Space after comma. @@ -10,27 +10,27 @@ STEXI ETEXI DEF(check, img_check, -check [-f fmt] [-r [leaks | all]] filename) +check [-q] [-f fmt] [-r [leaks | all]] filename) Is it really better to list [-q] to all of the sub-commands, or would it be easier to simply declare that -q is a universal option that can appear before sub-commands, as in: qemu-img [-q] command [command options] -q is not useable for all commands so it is listed this ways as it is same for other options. #ifdef _WIN32 #include windows.h @@ -86,6 +87,7 @@ static void help(void) rebasing in this case (useful for renaming the backing file)\n '-h' with or without a command shows this help and lists the supported formats\n '-p' show progress of command (only certain commands)\n + '-q' Quiet mode - do not print any output (except errors)\n s/Quiet/quiet/ for consistency with the nearby lines not starting with a capital. I use Quiet mode as name with capital Q, so I have to choose what consistency to break - I choose the start of the description. I have no problem with using quiet instead of Quiet. +++ b/qemu-img.texi @@ -54,6 +54,9 @@ indicates that target image must be compressed (qcow format only) with or without a command shows help and lists the supported formats @item -p display progress bar (convert and rebase commands only) +@item -q +Quiet mode - do not print any output (except errors). There's no progres bar s/progres/progress/ +in case both @var{-q} and @var{-p} options are used. Should it be a hard error if both -q and -p are given? Otherwise, when dealing with conflicting options, it's more typical to have the semantic of last one wins: 'qemu-img -q -p' prints progress, and only 'qemu-img -p -q' is quiet. Depends on point of view - There's no conflict for -p and -q as -p adds progress bar to output and -q suppress output. You can imagine (in theory) -q as redirecting stdout to /dev/null. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCH v7 3/4] qemu-img: Add compare subcommand
- Original Message - From: Eric Blake ebl...@redhat.com To: mreza...@redhat.com Cc: qemu-devel@nongnu.org, kw...@redhat.com, pbonz...@redhat.com, stefa...@redhat.com Sent: Wednesday, December 19, 2012 7:12:49 PM Subject: Re: [Qemu-devel] [PATCH v7 3/4] qemu-img: Add compare subcommand On 12/17/2012 06:39 AM, mreza...@redhat.com wrote: From: Miroslav Rezanina mreza...@redhat.com This patch adds new qemu-img subcommand that compare content of two disk s/compare/compares/ images. Signed-off-by: Miroslav Rezanina mreza...@redhat.com --- @@ -587,7 +587,7 @@ static int img_commit(int argc, char **argv) } /* - * Returns true iff the first sector pointed to by 'buf' contains at least + * Returns true if the first sector pointed to by 'buf' contains at least Spurious change. 'iff' is correct here, for its mathematical meaning of if-and-only-if. You're right. I probably just see iff and change it to if without checking the reason. * a non-NUL byte. * * 'pnum' is set to the number of sectors (including and immediately following @@ -688,6 +688,272 @@ static int compare_sectors(const uint8_t *buf1, const uint8_t *buf2, int n, #define IO_BUF_SIZE (2 * 1024 * 1024) +static int64_t sectors_to_bytes(int64_t sectors) +{ +return sectors BDRV_SECTOR_BITS; Worth checking for overflow? I think it's not. This should be save as block driver should not allow sectors to cause overflow. +static int check_empty_sectors(BlockDriverState *bs, int64_t sect_num, + int sect_count, const char *filename, + uint8_t *buffer, bool quiet) +{ +int pnum, ret = 0; +ret = bdrv_read(bs, sect_num, buffer, sect_count); +if (ret 0) { +error_report(Error while reading offset % PRId64 of %s: %s, + sectors_to_bytes(sect_num), filename, strerror(-ret)); +return ret; +} +ret = is_allocated_sectors(buffer, sect_count, pnum); Is this logic backwards? Isn't it wasteful to read a sector prior to seeing if it was actually allocated? This is correct order. Function is_allocated_sector test if sectors contain any non-zero byte. We know that sector is physically allocated in the image, we test if it contains any data. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org Miroslav Rezanina
Re: [Qemu-devel] [PATCH v7 4/4] Add qemu-img compare documentation
- Original Message - From: Eric Blake ebl...@redhat.com To: mreza...@redhat.com Cc: qemu-devel@nongnu.org, kw...@redhat.com, pbonz...@redhat.com, stefa...@redhat.com Sent: Wednesday, December 19, 2012 7:15:11 PM Subject: Re: [Qemu-devel] [PATCH v7 4/4] Add qemu-img compare documentation On 12/17/2012 06:39 AM, mreza...@redhat.com wrote: From: Miroslav Rezanina mreza...@redhat.com Adding documentation for new qemu-img subcommand compare. Signed-off-by: Miroslav Rezanina mreza...@redhat.com --- qemu-img.c| 7 ++- qemu-img.texi | 32 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/qemu-img.c b/qemu-img.c index 8b4f01f..a185e9e 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -103,7 +103,12 @@ static void help(void) '-a' applies a snapshot (revert disk to saved state)\n '-c' creates a snapshot\n '-d' deletes a snapshot\n - '-l' lists all snapshots in the given image\n; + '-l' lists all snapshots in the given image\n + \n + Parameters to compare subcommand:\n + '-f' First image format\n + '-F' Second image format\n + '-s' Strict mode - fail on different image size or sector allocation\n; s/First/first/; s/Second/second/; s/Strict/strict/ for consistent appearance of starting description with lower case Yes for First and Second, but Strict mode is same case as Quiet mode in patch 02 - it's always capital S not because it's start of description. @table @option @@ -117,6 +129,26 @@ it doesn't need to be specified separately in this case. Commit the changes recorded in @var{filename} in its base image. +@item compare [-f @var{fmt}] [-F @var{fmt}] [-p] [-s] [-q] @var{filename1} @var{filename2} + +Check if two images have the same content. You can compare images with +different format or settings. + +The format is probed unless you specify it by @var{-f} (used for @var{filename1}) and/or @var{-F} (used for @var{filename2}) option. Wrap this long line. None of the long lines is wraped in qemu-img.c in this part of code so I did not wrap it intentionally. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org Miroslav Rezanina
Re: [Qemu-devel] [PATCH v6] Add compare subcommand for qemu-img
Hi Kevin, thanks for review, comments inline. - Original Message - From: Kevin Wolf kw...@redhat.com To: Miroslav Rezanina mreza...@redhat.com Cc: qemu-devel@nongnu.org, Paolo Bonzini pbonz...@redhat.com, Stefan Hajnoczi stefa...@redhat.com Sent: Tuesday, December 11, 2012 1:27:45 PM Subject: Re: [PATCH v6] Add compare subcommand for qemu-img Am 06.12.2012 13:24, schrieb Miroslav Rezanina: This is second version of patch adding compare subcommand that compares two images. Compare has following criteria: - only data part is compared - unallocated sectors are not read - in case of different image size, exceeding part of bigger disk has to be zeroed/unallocated to compare rest - qemu-img returns: - 0 if images are identical - 1 if images differ - 2 on error v6: - added handling -?, -h options for compare subcommand v5 (only minor changes): - removed redundant comment - removed dead code (goto after help()) - set final total_sectors on first assignment v4: - Fixed various typos - Added functions for empty sector check and sector-to-bytes offset conversion - Fixed command-line parameters processing v3: - options -f/-F are orthogonal - documentation updated to new syntax and behavior - used byte offset instead of sector number for output v2: - changed option for second image format to -F - changed handling of -f and -F [1] - added strict mode (-s) - added quiet mode (-q) - improved output messages [2] - rename variables for larger image handling - added man page content Signed-off-by: Miroslav Rezanina mreza...@redhat.com Please move the changelog below a --- line so that git am ignores it for the final commit message. Ok, next version probably be as multipart so this will go into the header mail. diff --git a/block.c b/block.c index 854ebd6..fdc8c45 100644 --- a/block.c +++ b/block.c @@ -2698,6 +2698,7 @@ int bdrv_has_zero_init(BlockDriverState *bs) typedef struct BdrvCoIsAllocatedData { BlockDriverState *bs; +BlockDriverState *base; int64_t sector_num; int nb_sectors; int *pnum; @@ -2828,6 +2829,44 @@ int coroutine_fn bdrv_co_is_allocated_above(BlockDriverState *top, return 0; } +/* Coroutine wrapper for bdrv_is_allocated_above() */ +static void coroutine_fn bdrv_is_allocated_above_co_entry(void *opaque) +{ +BdrvCoIsAllocatedData *data = opaque; +BlockDriverState *top = data-bs; +BlockDriverState *base = data-base; + +data-ret = bdrv_co_is_allocated_above(top, base, data-sector_num, + data-nb_sectors, data-pnum); +data-done = true; +} + +/* + * Synchronous wrapper around bdrv_co_is_allocated_above(). + * + * See bdrv_co_is_allocated_above() for details. + */ +int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base, + int64_t sector_num, int nb_sectors, int *pnum) +{ +Coroutine *co; +BdrvCoIsAllocatedData data = { +.bs = top, +.base = base, +.sector_num = sector_num, +.nb_sectors = nb_sectors, +.pnum = pnum, +.done = false, +}; + +co = qemu_coroutine_create(bdrv_is_allocated_above_co_entry); +qemu_coroutine_enter(co, data); +while (!data.done) { +qemu_aio_wait(); +} +return data.ret; +} + BlockInfo *bdrv_query_info(BlockDriverState *bs) { BlockInfo *info = g_malloc0(sizeof(*info)); diff --git a/block.h b/block.h index 722c620..2cb8d71 100644 --- a/block.h +++ b/block.h @@ -278,6 +278,8 @@ int bdrv_co_discard(BlockDriverState *bs, int64_t sector_num, int nb_sectors); int bdrv_has_zero_init(BlockDriverState *bs); int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors, int *pnum); +int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base, +int64_t sector_num, int nb_sectors, int *pnum); void bdrv_set_on_error(BlockDriverState *bs, BlockdevOnError on_read_error, BlockdevOnError on_write_error); This first part looks good. I think it could be a separate patch, however. Agree with this, was lazy to split. diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx index a181363..c076085 100644 --- a/qemu-img-cmds.hx +++ b/qemu-img-cmds.hx @@ -27,6 +27,12 @@ STEXI @item commit [-f @var{fmt}] [-t @var{cache}] @var{filename} ETEXI +DEF(compare, img_compare, +compare [-f fmt] [-F fmt] [-p] [-q] [-s] filename1 filename2) +STEXI +@item compare [-f @var{fmt}] [-F @var{fmt}] [-p] [-q] [-s] @var{filename1} @var{filename2} +ETEXI + DEF(convert, img_convert, convert [-c] [-p] [-f fmt] [-t cache] [-O output_fmt] [-o
[Qemu-devel] [PATCH v6] Add compare subcommand for qemu-img
This is second version of patch adding compare subcommand that compares two images. Compare has following criteria: - only data part is compared - unallocated sectors are not read - in case of different image size, exceeding part of bigger disk has to be zeroed/unallocated to compare rest - qemu-img returns: - 0 if images are identical - 1 if images differ - 2 on error v6: - added handling -?, -h options for compare subcommand v5 (only minor changes): - removed redundant comment - removed dead code (goto after help()) - set final total_sectors on first assignment v4: - Fixed various typos - Added functions for empty sector check and sector-to-bytes offset conversion - Fixed command-line parameters processing v3: - options -f/-F are orthogonal - documentation updated to new syntax and behavior - used byte offset instead of sector number for output v2: - changed option for second image format to -F - changed handling of -f and -F [1] - added strict mode (-s) - added quiet mode (-q) - improved output messages [2] - rename variables for larger image handling - added man page content Signed-off-by: Miroslav Rezanina mreza...@redhat.com diff --git a/block.c b/block.c index 854ebd6..fdc8c45 100644 --- a/block.c +++ b/block.c @@ -2698,6 +2698,7 @@ int bdrv_has_zero_init(BlockDriverState *bs) typedef struct BdrvCoIsAllocatedData { BlockDriverState *bs; +BlockDriverState *base; int64_t sector_num; int nb_sectors; int *pnum; @@ -2828,6 +2829,44 @@ int coroutine_fn bdrv_co_is_allocated_above(BlockDriverState *top, return 0; } +/* Coroutine wrapper for bdrv_is_allocated_above() */ +static void coroutine_fn bdrv_is_allocated_above_co_entry(void *opaque) +{ +BdrvCoIsAllocatedData *data = opaque; +BlockDriverState *top = data-bs; +BlockDriverState *base = data-base; + +data-ret = bdrv_co_is_allocated_above(top, base, data-sector_num, + data-nb_sectors, data-pnum); +data-done = true; +} + +/* + * Synchronous wrapper around bdrv_co_is_allocated_above(). + * + * See bdrv_co_is_allocated_above() for details. + */ +int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base, + int64_t sector_num, int nb_sectors, int *pnum) +{ +Coroutine *co; +BdrvCoIsAllocatedData data = { +.bs = top, +.base = base, +.sector_num = sector_num, +.nb_sectors = nb_sectors, +.pnum = pnum, +.done = false, +}; + +co = qemu_coroutine_create(bdrv_is_allocated_above_co_entry); +qemu_coroutine_enter(co, data); +while (!data.done) { +qemu_aio_wait(); +} +return data.ret; +} + BlockInfo *bdrv_query_info(BlockDriverState *bs) { BlockInfo *info = g_malloc0(sizeof(*info)); diff --git a/block.h b/block.h index 722c620..2cb8d71 100644 --- a/block.h +++ b/block.h @@ -278,6 +278,8 @@ int bdrv_co_discard(BlockDriverState *bs, int64_t sector_num, int nb_sectors); int bdrv_has_zero_init(BlockDriverState *bs); int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors, int *pnum); +int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base, +int64_t sector_num, int nb_sectors, int *pnum); void bdrv_set_on_error(BlockDriverState *bs, BlockdevOnError on_read_error, BlockdevOnError on_write_error); diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx index a181363..c076085 100644 --- a/qemu-img-cmds.hx +++ b/qemu-img-cmds.hx @@ -27,6 +27,12 @@ STEXI @item commit [-f @var{fmt}] [-t @var{cache}] @var{filename} ETEXI +DEF(compare, img_compare, +compare [-f fmt] [-F fmt] [-p] [-q] [-s] filename1 filename2) +STEXI +@item compare [-f @var{fmt}] [-F @var{fmt}] [-p] [-q] [-s] @var{filename1} @var{filename2} +ETEXI + DEF(convert, img_convert, convert [-c] [-p] [-f fmt] [-t cache] [-O output_fmt] [-o options] [-s snapshot_name] [-S sparse_size] filename [filename2 [...]] output_filename) STEXI diff --git a/qemu-img.c b/qemu-img.c index e29e01b..09abb3a 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -101,7 +101,13 @@ static void help(void) '-a' applies a snapshot (revert disk to saved state)\n '-c' creates a snapshot\n '-d' deletes a snapshot\n - '-l' lists all snapshots in the given image\n; + '-l' lists all snapshots in the given image\n + \n + Parameters to compare subcommand:\n + '-f' First image format\n + '-F' Second image format\n + '-q' Quiet mode - do not print any output (except errors)\n + '-s' Strict mode - fail on different image size or sector allocation\n; printf(%s\nSupported formats:, help_msg); bdrv_iterate_format(format_print, NULL); @@ -658,6 +664,286 @@ static int compare_sectors(const uint8_t *buf1, const uint8_t *buf2
[Qemu-devel] [PATCH v5] Add compare subcommand for qemu-img
This is second version of patch adding compare subcommand that compares two images. Compare has following criteria: - only data part is compared - unallocated sectors are not read - in case of different image size, exceeding part of bigger disk has to be zeroed/unallocated to compare rest - qemu-img returns: - 0 if images are identical - 1 if images differ - 2 on error v5 (only minor changes): - removed redundant comment - removed dead code (goto after help()) - set final total_sectors on first assignment v4: - Fixed various typos - Added functions for empty sector check and sector-to-bytes offset conversion - Fixed command-line parameters processing v3: - options -f/-F are orthogonal - documentation updated to new syntax and behavior - used byte offset instead of sector number for output v2: - changed option for second image format to -F - changed handling of -f and -F [1] - added strict mode (-s) - added quiet mode (-q) - improved output messages [2] - rename variables for larger image handling - added man page content Signed-off-by: Miroslav Rezanina mreza...@redhat.com Note: Do not move any code to separate function as suggested by Stefan. Code to extract is not reusable and require access to some of local variables (for error handling part). Only code in separate function is the checking passed sectors for zero value as this can be reused on two places in the function. diff --git a/block.c b/block.c index 854ebd6..fdc8c45 100644 --- a/block.c +++ b/block.c @@ -2698,6 +2698,7 @@ int bdrv_has_zero_init(BlockDriverState *bs) typedef struct BdrvCoIsAllocatedData { BlockDriverState *bs; +BlockDriverState *base; int64_t sector_num; int nb_sectors; int *pnum; @@ -2828,6 +2829,44 @@ int coroutine_fn bdrv_co_is_allocated_above(BlockDriverState *top, return 0; } +/* Coroutine wrapper for bdrv_is_allocated_above() */ +static void coroutine_fn bdrv_is_allocated_above_co_entry(void *opaque) +{ +BdrvCoIsAllocatedData *data = opaque; +BlockDriverState *top = data-bs; +BlockDriverState *base = data-base; + +data-ret = bdrv_co_is_allocated_above(top, base, data-sector_num, + data-nb_sectors, data-pnum); +data-done = true; +} + +/* + * Synchronous wrapper around bdrv_co_is_allocated_above(). + * + * See bdrv_co_is_allocated_above() for details. + */ +int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base, + int64_t sector_num, int nb_sectors, int *pnum) +{ +Coroutine *co; +BdrvCoIsAllocatedData data = { +.bs = top, +.base = base, +.sector_num = sector_num, +.nb_sectors = nb_sectors, +.pnum = pnum, +.done = false, +}; + +co = qemu_coroutine_create(bdrv_is_allocated_above_co_entry); +qemu_coroutine_enter(co, data); +while (!data.done) { +qemu_aio_wait(); +} +return data.ret; +} + BlockInfo *bdrv_query_info(BlockDriverState *bs) { BlockInfo *info = g_malloc0(sizeof(*info)); diff --git a/block.h b/block.h index 722c620..2cb8d71 100644 --- a/block.h +++ b/block.h @@ -278,6 +278,8 @@ int bdrv_co_discard(BlockDriverState *bs, int64_t sector_num, int nb_sectors); int bdrv_has_zero_init(BlockDriverState *bs); int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors, int *pnum); +int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base, +int64_t sector_num, int nb_sectors, int *pnum); void bdrv_set_on_error(BlockDriverState *bs, BlockdevOnError on_read_error, BlockdevOnError on_write_error); diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx index a181363..c076085 100644 --- a/qemu-img-cmds.hx +++ b/qemu-img-cmds.hx @@ -27,6 +27,12 @@ STEXI @item commit [-f @var{fmt}] [-t @var{cache}] @var{filename} ETEXI +DEF(compare, img_compare, +compare [-f fmt] [-F fmt] [-p] [-q] [-s] filename1 filename2) +STEXI +@item compare [-f @var{fmt}] [-F @var{fmt}] [-p] [-q] [-s] @var{filename1} @var{filename2} +ETEXI + DEF(convert, img_convert, convert [-c] [-p] [-f fmt] [-t cache] [-O output_fmt] [-o options] [-s snapshot_name] [-S sparse_size] filename [filename2 [...]] output_filename) STEXI diff --git a/qemu-img.c b/qemu-img.c index e29e01b..6cad518 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -101,7 +101,13 @@ static void help(void) '-a' applies a snapshot (revert disk to saved state)\n '-c' creates a snapshot\n '-d' deletes a snapshot\n - '-l' lists all snapshots in the given image\n; + '-l' lists all snapshots in the given image\n + \n + Parameters to compare subcommand:\n + '-f' First image format\n + '-F' Second image format\n + '-q' Quiet mode - do not print any output (except errors)\n + '-s
[Qemu-devel] [PATCH v4] Add compare subcommand for qemu-img
This is second version of patch adding compare subcommand that compares two images. Compare has following criteria: - only data part is compared - unallocated sectors are not read - in case of different image size, exceeding part of bigger disk has to be zeroed/unallocated to compare rest - qemu-img returns: - 0 if images are identical - 1 if images differ - 2 on error v4: - Fixed various typos - Added functions for empty sector check and sector-to-bytes offset conversion - Fixed command-line parameters processing v3: - options -f/-F are orthogonal - documentation updated to new syntax and behavior - used byte offset instead of sector number for output v2: - changed option for second image format to -F - changed handlig of -f and -F [1] - added strict mode (-s) - added quiet mode (-q) - improved output messages [2] - rename variables for larger image handling - added man page content Signed-off-by: Miroslav Rezanina mreza...@redhat.com diff --git a/block.c b/block.c index 854ebd6..fdc8c45 100644 --- a/block.c +++ b/block.c @@ -2698,6 +2698,7 @@ int bdrv_has_zero_init(BlockDriverState *bs) typedef struct BdrvCoIsAllocatedData { BlockDriverState *bs; +BlockDriverState *base; int64_t sector_num; int nb_sectors; int *pnum; @@ -2828,6 +2829,44 @@ int coroutine_fn bdrv_co_is_allocated_above(BlockDriverState *top, return 0; } +/* Coroutine wrapper for bdrv_is_allocated_above() */ +static void coroutine_fn bdrv_is_allocated_above_co_entry(void *opaque) +{ +BdrvCoIsAllocatedData *data = opaque; +BlockDriverState *top = data-bs; +BlockDriverState *base = data-base; + +data-ret = bdrv_co_is_allocated_above(top, base, data-sector_num, + data-nb_sectors, data-pnum); +data-done = true; +} + +/* + * Synchronous wrapper around bdrv_co_is_allocated_above(). + * + * See bdrv_co_is_allocated_above() for details. + */ +int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base, + int64_t sector_num, int nb_sectors, int *pnum) +{ +Coroutine *co; +BdrvCoIsAllocatedData data = { +.bs = top, +.base = base, +.sector_num = sector_num, +.nb_sectors = nb_sectors, +.pnum = pnum, +.done = false, +}; + +co = qemu_coroutine_create(bdrv_is_allocated_above_co_entry); +qemu_coroutine_enter(co, data); +while (!data.done) { +qemu_aio_wait(); +} +return data.ret; +} + BlockInfo *bdrv_query_info(BlockDriverState *bs) { BlockInfo *info = g_malloc0(sizeof(*info)); diff --git a/block.h b/block.h index 722c620..2cb8d71 100644 --- a/block.h +++ b/block.h @@ -278,6 +278,8 @@ int bdrv_co_discard(BlockDriverState *bs, int64_t sector_num, int nb_sectors); int bdrv_has_zero_init(BlockDriverState *bs); int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors, int *pnum); +int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base, +int64_t sector_num, int nb_sectors, int *pnum); void bdrv_set_on_error(BlockDriverState *bs, BlockdevOnError on_read_error, BlockdevOnError on_write_error); diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx index a181363..c076085 100644 --- a/qemu-img-cmds.hx +++ b/qemu-img-cmds.hx @@ -27,6 +27,12 @@ STEXI @item commit [-f @var{fmt}] [-t @var{cache}] @var{filename} ETEXI +DEF(compare, img_compare, +compare [-f fmt] [-F fmt] [-p] [-q] [-s] filename1 filename2) +STEXI +@item compare [-f @var{fmt}] [-F @var{fmt}] [-p] [-q] [-s] @var{filename1} @var{filename2} +ETEXI + DEF(convert, img_convert, convert [-c] [-p] [-f fmt] [-t cache] [-O output_fmt] [-o options] [-s snapshot_name] [-S sparse_size] filename [filename2 [...]] output_filename) STEXI diff --git a/qemu-img.c b/qemu-img.c index e29e01b..9cc4365 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -101,7 +101,13 @@ static void help(void) '-a' applies a snapshot (revert disk to saved state)\n '-c' creates a snapshot\n '-d' deletes a snapshot\n - '-l' lists all snapshots in the given image\n; + '-l' lists all snapshots in the given image\n + \n + Parameters to compare subcommand:\n + '-f' First image format\n + '-F' Second image format\n + '-q' Quiet mode - do not print any output (except errors)\n + '-s' Strict mode - fail on different image size or sector allocation\n; printf(%s\nSupported formats:, help_msg); bdrv_iterate_format(format_print, NULL); @@ -658,6 +664,288 @@ static int compare_sectors(const uint8_t *buf1, const uint8_t *buf2, int n, #define IO_BUF_SIZE (2 * 1024 * 1024) +/* + * Helper functionto display offset in bytes insted of sector number. + */ +static int64_t sectors_to_bytes(int64_t sectors) +{ +return sectors
[Qemu-devel] [PATCH v3] Add compare subcommand for qemu-img
This is second version of patch adding compare subcommand that compares two images. Compare has following criteria: - only data part is compared - unallocated sectors are not read - in case of different image size, exceeding part of bigger disk has to be zeroed/unallocated to compare rest - qemu-img returns: - 0 if images are identical - 1 if images differ - 2 on error v3: - options -f/-F are orthogonal - documentation updated to new syntax and behavior - used byte offset instead of sector number for output v2: - changed option for second image format to -F - changed handlig of -f and -F [1] - added strict mode (-s) - added quiet mode (-q) - improved output messages [2] - rename variables for larger image handling - added man page content Signed-off-by: Miroslav Rezanina diff --git a/block.c b/block.c index 854ebd6..fdc8c45 100644 --- a/block.c +++ b/block.c @@ -2698,6 +2698,7 @@ int bdrv_has_zero_init(BlockDriverState *bs) typedef struct BdrvCoIsAllocatedData { BlockDriverState *bs; +BlockDriverState *base; int64_t sector_num; int nb_sectors; int *pnum; @@ -2828,6 +2829,44 @@ int coroutine_fn bdrv_co_is_allocated_above(BlockDriverState *top, return 0; } +/* Coroutine wrapper for bdrv_is_allocated_above() */ +static void coroutine_fn bdrv_is_allocated_above_co_entry(void *opaque) +{ +BdrvCoIsAllocatedData *data = opaque; +BlockDriverState *top = data-bs; +BlockDriverState *base = data-base; + +data-ret = bdrv_co_is_allocated_above(top, base, data-sector_num, + data-nb_sectors, data-pnum); +data-done = true; +} + +/* + * Synchronous wrapper around bdrv_co_is_allocated_above(). + * + * See bdrv_co_is_allocated_above() for details. + */ +int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base, + int64_t sector_num, int nb_sectors, int *pnum) +{ +Coroutine *co; +BdrvCoIsAllocatedData data = { +.bs = top, +.base = base, +.sector_num = sector_num, +.nb_sectors = nb_sectors, +.pnum = pnum, +.done = false, +}; + +co = qemu_coroutine_create(bdrv_is_allocated_above_co_entry); +qemu_coroutine_enter(co, data); +while (!data.done) { +qemu_aio_wait(); +} +return data.ret; +} + BlockInfo *bdrv_query_info(BlockDriverState *bs) { BlockInfo *info = g_malloc0(sizeof(*info)); diff --git a/block.h b/block.h index 722c620..2cb8d71 100644 --- a/block.h +++ b/block.h @@ -278,6 +278,8 @@ int bdrv_co_discard(BlockDriverState *bs, int64_t sector_num, int nb_sectors); int bdrv_has_zero_init(BlockDriverState *bs); int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors, int *pnum); +int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base, +int64_t sector_num, int nb_sectors, int *pnum); void bdrv_set_on_error(BlockDriverState *bs, BlockdevOnError on_read_error, BlockdevOnError on_write_error); diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx index a181363..c076085 100644 --- a/qemu-img-cmds.hx +++ b/qemu-img-cmds.hx @@ -27,6 +27,12 @@ STEXI @item commit [-f @var{fmt}] [-t @var{cache}] @var{filename} ETEXI +DEF(compare, img_compare, +compare [-f fmt] [-F fmt] [-p] [-q] [-s] filename1 filename2) +STEXI +@item compare [-f @var{fmt}] [-F @var{fmt}] [-p] [-q] [-s] @var{filename1} @var{filename2} +ETEXI + DEF(convert, img_convert, convert [-c] [-p] [-f fmt] [-t cache] [-O output_fmt] [-o options] [-s snapshot_name] [-S sparse_size] filename [filename2 [...]] output_filename) STEXI diff --git a/qemu-img.c b/qemu-img.c index e29e01b..6294b11 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -101,7 +101,12 @@ static void help(void) '-a' applies a snapshot (revert disk to saved state)\n '-c' creates a snapshot\n '-d' deletes a snapshot\n - '-l' lists all snapshots in the given image\n; + '-l' lists all snapshots in the given image\n + Parameters to compare subcommand:\n + '-f' First image format\n + '-F' Second image format\n + '-q' Quiet mode - do not print any output (except errors)\n + '-s' Strict mode - fail on different image size or sector allocation\n; printf(%s\nSupported formats:, help_msg); bdrv_iterate_format(format_print, NULL); @@ -657,6 +662,277 @@ static int compare_sectors(const uint8_t *buf1, const uint8_t *buf2, int n, } #define IO_BUF_SIZE (2 * 1024 * 1024) +#define sector_to_bytes(sector) ((sector) BDRV_SECTOR_BITS) + +/* + * Get number of sectors that can be stored in IO buffer. + */ + +static int64_t sectors_to_process(int64_t total, int64_t from) +{ +int64_t rv = total - from; + +if (rv (IO_BUF_SIZE BDRV_SECTOR_BITS)) { +return IO_BUF_SIZE BDRV_SECTOR_BITS