Enable repository wide style checking
Subsystems (5)
cover Enable repository wide style checking
Historically QEMU has relied on the checkpatch.pl script, borrowed from Linux, to check coding style compliance on patches which are submitted. For what it is designed for, it does a reasonable job, but I feel that QEMU would benefit from some more checking in this area, in particular checks that run across the entire repository, not just new patches.
Rather than attempt to replace checkpatch.pl, this series illustrates how we can augment our existing style compliance checking. This imports the infrastructure from GNULIB which provides a 'syntax-check' target in the makefiles.
By default the checks are run against all files which are committed to GIT, as identified by the 'vc-list-files' script. On a per-check basis you can define a list of file exclusions, so you can skip checks in places where there are valid reasons for the exceptional code style. It is also possible to set up rules so that they can be skipped on a per-line basis with magic comments in the source though this is not illustrated in this series.
Each style rule is provided by a separate make target named with an 'sc_' prefix. There are some boilerplate make rules provided to simplify the process of defining new checks. The maint.mk file provides the set of rules defined by the GNULIB project and the cfg.mk file is intended to contain any further rules desired by QEMU. That is not to say that QEMU must use all the rules defined by GNULIB - it is possible to turn each rule on/off globally as desired.
In this series the first patch sets up the infrastructure such that any developer can do
make syntax-check
to see violations in their local tree. The following patches then incrementally enable some of the rules and fix up the violations they detect. None of the things I've detected are particularly interesting/troublesome until I get to the second to last patch where I look for non-reentrant safe POSIX function usage. There are some areas identified that I think have potential lead to data corruption / crashes if we're unlucky with usage across threads.
The very last patch illustrates how we would mandate that inclusion of 'osdep.h' as the first header in all .c files Although I've enabled it, I didn't try to fix the .c files so 'make syntax-check' will fail with patch 10 applied. It should however pass for all patches upto #9
There are plenty more rules that could be enabled and there is obviously scope to right more custom rules.
The intent would be that developers run 'make syntax-check' before sending patches to check rules. It is also wired into 'make check' to make it harder to forget. It would also be expected that maintainers would run syntax-check on any patches they receive from contributors and reject them if failing, in the same way they'd be rejected if checkpatch.pl fails. Finally the automated build systems that various people are running should also run the 'make syntax-check' if they are not already running the 'make check' rule.
Daniel P. Berrange (10):
tests: import GNULIB's syntax-check infrastructure
maint: remove double semicolons in many files
maint: remove / fix many doubled words
maint: remove unused include for assert.h
maint: remove unused include for dirent.h
maint: remove unused include for signal.h
maint: remove unused include for strings.h
maint: avoid useless "if (foo) free(foo)" pattern
maint: add check for use of POSIX functions which are not reentrant
safe
maint: enable checking for qemu/osdep.h header usage
Makefile | 5 + Makefile.nonreentrant | 120 ++++ backends/hostmem-file.c | 4 +- block/qcow2-cluster.c | 4 +- block/qcow2-refcount.c | 2 +- block/vhdx.c | 2 +- bsd-user/elfload.c | 4 +- bsd-user/signal.c | 1 - cfg.mk | 156 +++++ disas/ia64.c | 1 - disas/microblaze.c | 1 - disas/sparc.c | 3 +- docs/libcacard.txt | 4 +- docs/multiseat.txt | 2 +- docs/specs/qcow2.txt | 2 +- docs/specs/rocker.txt | 2 +- fsdev/virtio-9p-marshal.c | 1 - hw/arm/vexpress.c | 4 +- hw/block/xen_disk.c | 1 - hw/bt/hci.c | 9 +- hw/core/loader.c | 3 +- hw/core/qdev-properties.c | 4 +- hw/display/exynos4210_fimd.c | 4 +- hw/i386/xen/xen_platform.c | 2 - hw/intc/arm_gic.c | 2 +- hw/mips/mips_r4k.c | 4 +- hw/net/fsl_etsec/rings.c | 4 +- hw/net/rocker/rocker.c | 4 +- hw/net/rocker/rocker_desc.c | 8 +- hw/net/rtl8139.c | 2 +- hw/net/xen_nic.c | 1 - hw/nvram/fw_cfg.c | 4 +- hw/pci-host/prep.c | 4 +- hw/pci/shpc.c | 1 - hw/sd/sd.c | 3 +- hw/tpm/tpm_passthrough.c | 2 - hw/usb/hcd-xhci.c | 4 +- hw/usb/host-libusb.c | 2 +- hw/usb/redirect.c | 2 - hw/vfio/common.c | 2 +- hw/vfio/pci.c | 1 - hw/xen/xen_pt_config_init.c | 4 +- include/block/block.h | 2 +- include/exec/memory.h | 2 +- linux-user/elfload.c | 2 +- linux-user/signal.c | 1 - maint.mk | 1228 ++++++++++++++++++++++++++++++++++++++++ migration/rdma.c | 2 +- migration/savevm.c | 8 +- numa.c | 2 +- os-win32.c | 1 - page_cache.c | 1 - qemu-char.c | 5 +- qemu-doc.texi | 2 +- qemu-img.texi | 2 +- qemu-options.hx | 2 +- qga/commands-win32.c | 2 +- scripts/useless-if-before-free | 207 +++++++ scripts/vc-list-files | 113 ++++ target-arm/cpu.h | 4 +- target-arm/helper.c | 2 +- target-arm/translate.c | 2 +- target-i386/translate.c | 1 - target-lm32/helper.c | 2 +- target-microblaze/op_helper.c | 1 - target-microblaze/translate.c | 2 +- target-mips/helper.c | 1 - target-moxie/helper.c | 3 +- target-moxie/translate.c | 1 - target-sh4/helper.c | 1 - target-sh4/op_helper.c | 1 - target-tricore/helper.c | 1 - tests/Makefile | 2 +- tests/bios-tables-test.c | 36 +- tests/tcg/testthread.c | 1 - tests/test-xbzrle.c | 2 - ui/spice-display.c | 14 +- util/bitmap.c | 2 +- 78 files changed, 1901 insertions(+), 155 deletions(-) create mode 100644 Makefile.nonreentrant create mode 100644 cfg.mk create mode 100644 maint.mk create mode 100755 scripts/useless-if-before-free create mode 100755 scripts/vc-list-files
Signature · 1 line
2.4.3
RFC PATCH 1/10 tests: import GNULIB's syntax-check infrastructure
Imports the infrastructure for GNULIB's 'syntax-check' make target. Similar to QEMU's checkpatch.pl script this allows for checking code style rules. The important differences are that it checks across the entire codebase instead of just the submitted patch, and can have each rule individually turned off on a per-source file basis.
In this commit, maint.mk is the file taken from GNULIB source tree (at $GNULIBGIT/top/maint.mk) under GPLv3 license. The non-syntax check related cruft deleted, but broadly the aim is to maintain this unchanged to make it easier to import new versions from GNULIB in the future.
The cfg.mk is where QEMU specific configuration lives. This comprises the list of rules to skip entirely, any per-rule file exclusions and custom defined rules. Two helper files are also added to the scripts dir, vc-list-files is used to get a list of all files committed to GIT, and useless-if-before-free is a rule specific helper
In this initial commit all rules which currently fail are completely disabled. Patches which follow this will look to selectively enable some useful rules and fix up the fallout in current code.
It is intended that people run 'make syntax-check' on each patch before they submit, but it is also wired up to be run by 'make check' so that maintainers automated build tests also exercise the code.
--- Makefile | 5 + cfg.mk | 139 +++++ maint.mk | 1228 ++++++++++++++++++++++++++++++++++++++++ scripts/useless-if-before-free | 207 +++++++ scripts/vc-list-files | 113 ++++ tests/Makefile | 2 +- 6 files changed, 1693 insertions(+), 1 deletion(-) create mode 100644 cfg.mk create mode 100644 maint.mk create mode 100755 scripts/useless-if-before-free create mode 100755 scripts/vc-list-files
Makefile +5 -0
@@ -163,6 +163,11 @@ endif all: $(DOCS) $(TOOLS) $(HELPERS-y) recurse-all modules +include $(SRC_PATH)/cfg.mk +include $(SRC_PATH)/maint.mk + +check:: syntax-check + config-host.h: config-host.h-timestamp config-host.h-timestamp: config-host.mak qemu-options.def: $(SRC_PATH)/qemu-options.hx
cfg.mk new file +139 -0
139 lines not on this page · raw message
maint.mk new file +1228 -0
1,228 lines not on this page · raw message
scripts/useless-if-before-free new file +207 -0
207 lines not on this page · raw message
scripts/vc-list-files new file +113 -0
113 lines not on this page · raw message
tests/Makefile +1 -1
@@ -523,7 +523,7 @@ check-qapi-schema: $(patsubst %,check-%, $(check-qapi-schema-y)) check-qtest: $(patsubst %,check-qtest-%, $(QTEST_TARGETS)) check-unit: $(patsubst %,check-%, $(check-unit-y)) check-block: $(patsubst %,check-%, $(check-block-y)) -check: check-qapi-schema check-unit check-qtest +check:: check-qapi-schema check-unit check-qtest check-clean: $(MAKE) -C tests/tcg clean rm -rf $(check-unit-y) tests/*.o $(QEMU_IOTESTS_HELPERS-y)
Signature · 1 line
2.4.3
RFC PATCH 2/10 maint: remove double semicolons in many files
In reply to Daniel P. BerrangeA number of source files have statements accidentally terminated by a double semicolon - eg 'foo = bar;;'. This is harmless but a mistake none the less.
The tcg/ia64/tcg-target.c file is whitelisted because it has valid use of ';;' in a comment containing assembly code.
--- block/vhdx.c | 2 +- cfg.mk | 5 ++++- hw/arm/vexpress.c | 4 ++-- hw/intc/arm_gic.c | 2 +- numa.c | 2 +- qga/commands-win32.c | 2 +- 6 files changed, 10 insertions(+), 7 deletions(-)
block/vhdx.c +1 -1
@@ -1454,7 +1454,7 @@ static int vhdx_create_new_metadata(BlockDriverState *bs,uint32_t offset =0 ;void *buffer =NULL ;void *entry_buffer; -VHDXMetadataTableHeader *md_table;; +VHDXMetadataTableHeader *md_table;VHDXMetadataTableEntry *md_table_entry;/* Metadata entries */
cfg.mk +4 -1
@@ -53,7 +53,6 @@ local-checks-to-skip = \ sc_prohibit_cvs_keyword \ sc_prohibit_dirent_without_use \ sc_prohibit_doubled_word \ - sc_prohibit_double_semicolon \ sc_prohibit_empty_lines_at_EOF \ sc_prohibit_hash_without_use \ sc_prohibit_jm_in_m4 \
@@ -137,3 +136,7 @@ sc_copyright_format: # We don't use this feature of maint.mk. prev_version_file = /dev/null + + +exclude_file_name_regexp--sc_prohibit_double_semicolon = \ + ^tcg/ia64/tcg-target\.c$
hw/arm/vexpress.c +2 -2
@@ -541,7 +541,7 @@ static void vexpress_common_init(MachineState *machine) {VexpressMachineState *vms =VEXPRESS_MACHINE (machine);VexpressMachineClass *vmc =VEXPRESS_MACHINE_GET_CLASS (machine); -VEDBoardInfo *daughterboard = vmc->daughterboard;; +VEDBoardInfo *daughterboard = vmc->daughterboard;DeviceState *dev, *sysctl, *pl041;qemu_irq pic[64 ];uint32_t sys_id;
@@ -762,7 +762,7 @@ static void vexpress_a9_class_init(ObjectClass *oc, void *data) mc->name =TYPE_VEXPRESS_A9_MACHINE ; mc->desc ="ARM Versatile Express for Cortex-A9" ; - vmc->daughterboard = &a9_daughterboard;; + vmc->daughterboard = &a9_daughterboard; }static void vexpress_a15_class_init (ObjectClass *oc,void *data)
hw/intc/arm_gic.c +1 -1
@@ -239,7 +239,7 @@ uint32_t gic_acknowledge_irq(GICState *s, int cpu, MemTxAttrs attrs)* for the case where this GIC supports grouping and the pending interrupt * is in the wrong group. */ - irq =gic_get_current_pending_irq (s, cpu, attrs);; + irq =gic_get_current_pending_irq (s, cpu, attrs);if (irq >=GIC_MAXIRQ ) {DPRINTF ("ACK, no pending interrupt or it is hidden: %d\n" , irq);
numa.c +1 -1
@@ -280,7 +280,7 @@ static void validate_numa_cpus(void)bitmap_and (seen_cpus, seen_cpus, numa_info[i].node_cpu,MAX_CPUMASK_BITS );error_report ("CPU(s) present in multiple NUMA nodes: %s" , -enumerate_cpus (seen_cpus, max_cpus));; +enumerate_cpus (seen_cpus, max_cpus));exit (EXIT_FAILURE ); } bitmap_or(seen_cpus, seen_cpus,
qga/commands-win32.c +1 -1
@@ -657,7 +657,7 @@ static GuestFilesystemInfo *build_guest_fsinfo(char *guid, Error **errp) fs->mountpoint =g_strndup (mnt_point, len); } fs->type =g_strdup (fs_name); - fs->disk =build_guest_disk_info (guid, errp);; + fs->disk =build_guest_disk_info (guid, errp); free:g_free (mnt_point);return fs;
Signature · 1 line
2.4.3
Re: [Qemu-devel] [PATCH RFC 02/10] maint: remove double semicolons in many files
Quoted 9 lines · Daniel P. Berrange
A number of source files have statements accidentally terminated by a double semicolon - eg 'foo = bar;;'. This is harmless but a mistake none the less.
The
tcg/ia64/tcg-target.cfile is whitelisted because it has valid use of ';;' in a comment containing assembly code.Signed-off-by Daniel P. Berrange <[email protected]>
Quoted 8 lines
--- block/vhdx.c | 2 +- cfg.mk | 5 ++++- hw/arm/vexpress.c | 4 ++-- hw/intc/arm_gic.c | 2 +- numa.c | 2 +- qga/commands-win32.c | 2 +- 6 files changed, 10 insertions(+), 7 deletions(-)
If you kept the "enable the check in cfg.mk" change out of the patches like these, we could review and commit them without them being tangled up or waiting on the review of the syntax-checking infrastructure itself...
thanks
-- PMM
Re: [Qemu-devel] [PATCH RFC 02/10] maint: remove double semicolons in many files
Quoted 19 lines · Peter Maydell
Quoted 9 lines · Daniel P. Berrange
A number of source files have statements accidentally terminated by a double semicolon - eg 'foo = bar;;'. This is harmless but a mistake none the less.
The
tcg/ia64/tcg-target.cfile is whitelisted because it has valid use of ';;' in a comment containing assembly code.Signed-off-by Daniel P. Berrange <[email protected]>Quoted 8 lines
--- block/vhdx.c | 2 +- cfg.mk | 5 ++++- hw/arm/vexpress.c | 4 ++-- hw/intc/arm_gic.c | 2 +- numa.c | 2 +- qga/commands-win32.c | 2 +- 6 files changed, 10 insertions(+), 7 deletions(-)
Quoted 4 lines
If you kept the "enable the check in cfg.mk" change out of the patches like these, we could review and commit them without them being tangled up or waiting on the review of the syntax-checking infrastructure itself...
Sure, I just really wanted to illustrate the use of the syntax check infrastructure with this series of fixes. Now I've done that I'll resubmit just the fixes, while debate over the checking infrastructure continues.
Regards,
Daniel
Signature · 4 lines
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
RFC PATCH 3/10 maint: remove / fix many doubled words
In reply to Daniel P. BerrangeMany source files have doubled words (eg "the the", "to to", and so on). Most of these can simply be removed, but a couple were actual mis-spellings (eg "to to" instead of "to do"). There was even one triple word score "to to to" :-)
--- block/qcow2-cluster.c | 4 ++-- block/qcow2-refcount.c | 2 +- cfg.mk | 4 +++- docs/libcacard.txt | 4 ++-- docs/multiseat.txt | 2 +- docs/specs/qcow2.txt | 2 +- docs/specs/rocker.txt | 2 +- hw/net/rtl8139.c | 2 +- hw/usb/host-libusb.c | 2 +- hw/vfio/common.c | 2 +- include/block/block.h | 2 +- include/exec/memory.h | 2 +- linux-user/elfload.c | 2 +- migration/rdma.c | 2 +- qemu-doc.texi | 2 +- qemu-img.texi | 2 +- qemu-options.hx | 2 +- target-arm/cpu.h | 4 ++-- target-arm/helper.c | 2 +- target-arm/translate.c | 2 +- target-lm32/helper.c | 2 +- target-microblaze/translate.c | 2 +- target-moxie/helper.c | 2 +- util/bitmap.c | 2 +- 24 files changed, 29 insertions(+), 27 deletions(-)
block/qcow2-cluster.c +2 -2
@@ -498,7 +498,7 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset, *cluster_offset =0 ; -/* seek the the l2 offset in the l1 table */ +/* seek the l2 offset in the l1 table */ l1_index = offset >> l1_bits;if (l1_index >= s->l1_size) {
@@ -612,7 +612,7 @@ static int get_cluster_table(BlockDriverState *bs, uint64_t offset,uint64_t *l2_table =NULL ;int ret; -/* seek the the l2 offset in the l1 table */ +/* seek the l2 offset in the l1 table */ l1_index = offset >> (s->l2_bits + s->cluster_bits);if (l1_index >= s->l1_size) {
block/qcow2-refcount.c +1 -1
@@ -1294,7 +1294,7 @@ static int realloc_refcount_array(BDRVQcowState *s, void **array,/* * Increases the refcount for a range of clusters in a given refcount table. * This is used to construct a temporary refcount table out of L1 and L2 tables -* which can be compared the the refcount table saved in the image. +* which can be compared the refcount table saved in the image. * * Modifies the number of errors in res. */
cfg.mk +3 -1
@@ -52,7 +52,6 @@ local-checks-to-skip = \ sc_prohibit_atoi_atof \ sc_prohibit_cvs_keyword \ sc_prohibit_dirent_without_use \ - sc_prohibit_doubled_word \ sc_prohibit_empty_lines_at_EOF \ sc_prohibit_hash_without_use \ sc_prohibit_jm_in_m4 \
@@ -140,3 +139,6 @@ prev_version_file = /dev/null exclude_file_name_regexp--sc_prohibit_double_semicolon = \ ^tcg/ia64/tcg-target\.c$ + +exclude_file_name_regexp--sc_prohibit_doubled_word = \ + ^(disas/sparc\.c|scripts/checkpatch\.pl|target-s390x/insn-data\.def)$
docs/libcacard.txt +2 -2
@@ -327,7 +327,7 @@ and applet. int vcard_emul_get_login_count(VCard *card); - This function returns the the number of remaining login attempts for this + This function returns the number of remaining login attempts for this card. If the card emulator does not know, or the card does not have a way of giving this information, this function returns -1.
@@ -421,7 +421,7 @@ functions: The vcard is the value returned from vcard_new. The type is the card type emulator that this card should presented to the guest as. The flags are card type emulator specific options. The certs, - cert_len, and keys are all arrays of length cert_count. These are the + cert_len, and keys are all arrays of length cert_count. These are the same of the parameters xxxx_card_init() accepts. Finally the card is associated with its reader by the call:
docs/multiseat.txt +1 -1
@@ -135,7 +135,7 @@ configuration: TAG+="seat", ENV{ID_AUTOSEAT}="1" Patch with this rule has been submitted to upstream udev/systemd, was -accepted and and should be included in the next systemd release (222). +accepted and should be included in the next systemd release (222). So, if your guest has this or a newer version, multiseat will work just fine without any manual guest configuration.
docs/specs/qcow2.txt +1 -1
@@ -257,7 +257,7 @@ L2 table entry: 63: 0 for a cluster that is unused or requires COW, 1 if its refcount is exactly one. This information is only accurate - in L2 tables that are reachable from the the active L1 + in L2 tables that are reachable from the active L1 table. Standard Cluster Descriptor:
docs/specs/rocker.txt +1 -1
@@ -297,7 +297,7 @@ but not fired. If only partial credits are returned, the interrupt remains masked but the device generates an interrupt, signaling the driver that more outstanding work is available. -(* this masking is unrelated to to the MSI-X interrupt mask register) +(* this masking is unrelated to the MSI-X interrupt mask register) Endianness ----------
hw/net/rtl8139.c +1 -1
@@ -43,7 +43,7 @@* Added rx/tx buffer reset when enabling rx/tx operation * * 2010-Feb-04 Frediano Ziglio: Rewrote timer support using QEMU timer only -* when strictly needed (required for for +* when strictly needed (required for * Darwin) * 2011-Mar-22 Benjamin Poirier: Implemented VLAN offloading */
hw/usb/host-libusb.c +1 -1
@@ -1429,7 +1429,7 @@ static void usb_host_free_streams(USBDevice *udev, USBEndpoint **eps,* still present in the first place. Attemping to contine where we * left off is impossible. * -* What we are going to to to here is emulate a surprise removal of +* What we are going to do here is emulate a surprise removal of * the usb device passed through, then kick host scan so the device * will get re-attached (and re-initialized by the guest) in case it * is still present.
hw/vfio/common.c +1 -1
@@ -226,7 +226,7 @@ static int vfio_dma_map(VFIOContainer *container, hwaddr iova,/* * Try the mapping, if it fails with EBUSY, unmap the region and try * again. This shouldn't be necessary, but we sometimes see it in -* the the VGA ROM space. +* the VGA ROM space. */ if (ioctl(container->fd, VFIO_IOMMU_MAP_DMA, &map) == 0 || (errno == EBUSY && vfio_dma_unmap(container, iova, size) == 0 &&
include/block/block.h +1 -1
@@ -23,7 +23,7 @@ typedef struct BlockDriverInfo {bool is_dirty;/* * True if unallocated blocks read back as zeroes. This is equivalent -* to the the LBPRZ flag in the SCSI logical block provisioning page. +* to the LBPRZ flag in the SCSI logical block provisioning page. */ bool unallocated_blocks_are_zero;/*
include/exec/memory.h +1 -1
@@ -324,7 +324,7 @@ void memory_region_unref(MemoryRegion *mr);* @owner: the object that tracks the region's reference count * @ops: a structure containing read and write callbacks to be used when * I/O is performed on the region. -* @opaque: passed to to the read and write callbacks of the @ops structure. +* @opaque: passed to the read and write callbacks of the @ops structure. * @name: used for debugging; not visible to the user or ABI * @size: size of the region. */
linux-user/elfload.c +1 -1
@@ -3016,7 +3016,7 @@ static int elf_core_dump(int signr, const CPUArchState *env)if (dump_write (fd, &elf,sizeof (elf)) !=0 ) goto out; -/* fill in in-memory version of notes */ +/* fill in the in-memory version of notes */ if (fill_note_info (&info, signr, env) <0 ) goto out;
migration/rdma.c +1 -1
@@ -778,7 +778,7 @@ static void qemu_rdma_dump_gid(const char *who, struct rdma_cm_id *id)* * If the source VM connects with an IPv4 address without knowing that the * destination has bound to '[::]' the migration will unconditionally fail -* unless the management software is explicitly listening on the the IPv4 +* unless the management software is explicitly listening on the IPv4 * address while using a RoCE-based device. * * If the source VM connects with an IPv6 address, then we're OK because we can
qemu-doc.texi +1 -1
@@ -1766,7 +1766,7 @@ Use @code{set architecture i8086} to dump 16 bit code. Then use Advanced debugging options: -The default single stepping behavior is step with the IRQs and timer service routines off. It is set this way because when gdb executes a single step it expects to advance beyond the current instruction. With the IRQs and and timer service routines on, a single step might jump into the one of the interrupt or exception vectors instead of executing the current instruction. This means you may hit the same breakpoint a number of times before executing the instruction gdb wants to have executed. Because there are rare circumstances where you want to single step into an interrupt vector the behavior can be controlled from GDB. There are three commands you can query and set the single step behavior: +The default single stepping behavior is step with the IRQs and timer service routines off. It is set this way because when gdb executes a single step it expects to advance beyond the current instruction. With the IRQs and timer service routines on, a single step might jump into the one of the interrupt or exception vectors instead of executing the current instruction. This means you may hit the same breakpoint a number of times before executing the instruction gdb wants to have executed. Because there are rare circumstances where you want to single step into an interrupt vector the behavior can be controlled from GDB. There are three commands you can query and set the single step behavior: @table @code @item maintenance packet qqemu.sstepbits
qemu-img.texi +1 -1
@@ -103,7 +103,7 @@ First image format @item -F Second image format @item -s -Strict mode - fail on on different image size or sector allocation +Strict mode - fail on different image size or sector allocation @end table Parameters to convert subcommand:
qemu-options.hx +1 -1
@@ -3147,7 +3147,7 @@ provide cycle accurate emulation. Modern CPUs contain superscalar out of order cores with complex cache hierarchies. The number of instructions executed often has little or no correlation with actual performance. -@option{align=on} will activate the delay algorithm which will try to +@option{align=on} will activate the delay algorithm which will try to synchronise the host clock and the virtual clock. The goal is to have a guest running at the real frequency imposed by the shift option. Whenever the guest clock is behind the host clock and if
target-arm/cpu.h +2 -2
@@ -170,7 +170,7 @@ typedef struct CPUARMState {uint32_t GE ;/* cpsr[19:16] */ uint32_t thumb;/* cpsr[5]. 0 = arm mode, 1 = thumb mode. */ uint32_t condexec_bits;/* IT bits. cpsr[15:10,26:25]. */ -uint64_t daif;/* exception masks, in the bits they are in in PSTATE */ +uint64_t daif;/* exception masks, in the bits they are in PSTATE */ uint64_t elr_el[4 ];/* AArch64 exception link regs */ uint64_t sp_el[4 ];/* AArch64 banked stack pointers */
@@ -1477,7 +1477,7 @@ bool write_list_to_cpustate(ARMCPU *cpu);*/ bool write_cpustate_to_list (ARMCPU *cpu); -/* Does the core conform to the the "MicroController" profile. e.g. Cortex-M3. +/* Does the core conform to the "MicroController" profile. e.g. Cortex-M3. Note the M in older cores (eg. ARM7TDMI) stands for Multiply. These are conventional cores (ie. Application or Realtime profile). */
target-arm/helper.c +1 -1
@@ -1856,7 +1856,7 @@ static void vmsa_ttbcr_raw_write(CPUARMState *env, const ARMCPRegInfo *ri, } } -/* Update the masks corresponding to the the TCR bank being written +/* Update the masks corresponding to the TCR bank being written * Note that we always calculate mask and base_mask, but * they are only used for short-descriptor tables (ie if EAE is 0); * for long-descriptor tables the TCR fields are used differently
target-arm/translate.c +1 -1
@@ -8500,7 +8500,7 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn) }/* Perform base writeback before the loaded value to ensure correct behavior with overlapping index registers. -ldrd with base writeback is is undefined if the +ldrd with base writeback is undefined if the destination and index registers overlap. */ if (!(insn & (1 <<24 ))) {gen_add_datah_offset (s, insn, address_offset, addr);
target-lm32/helper.c +1 -1
@@ -81,7 +81,7 @@ void lm32_watchpoint_insert(CPULM32State *env, int idx, target_ulong address,switch (wp_type) {case LM32_WP_DISABLED : -/* nothing to to */ +/* nothing to do */ break ;case LM32_WP_READ : flags =BP_CPU |BP_STOP_BEFORE_ACCESS |BP_MEM_READ ;
target-microblaze/translate.c +1 -1
@@ -1027,7 +1027,7 @@ static void dec_load(DisasContext *dc)tcg_temp_free (v);if (ex) {/* lwx */ -/* no support for for AXI exclusive so always clear C */ +/* no support for AXI exclusive so always clear C */ write_carryi (dc,0 ); }
target-moxie/helper.c +1 -1
@@ -56,7 +56,7 @@ void helper_raise_exception(CPUMoxieState *env, int ex)/* Stash the address where the exception occurred. */ cpu_restore_state (cs,GETPC ()); env->sregs[5 ] = env->pc; -/* Jump the the exception handline routine. */ +/* Jump to the exception handline routine. */ env->pc = env->sregs[1 ];cpu_loop_exit (cs); }
util/bitmap.c +1 -1
@@ -14,7 +14,7 @@#include "qemu/atomic.h" /* -* bitmaps provide an array of bits, implemented using an an +* bitmaps provide an array of bits, implemented using an * array of unsigned longs. The number of valid bits in a * given bitmap does _not_ need to be an exact multiple of * BITS_PER_LONG.
Signature · 1 line
2.4.3
Re: [Qemu-devel] [PATCH RFC 03/10] maint: remove / fix many doubled words
Quoted 60 lines · Daniel P. Berrange
Many source files have doubled words (eg "the the", "to to", and so on). Most of these can simply be removed, but a couple were actual mis-spellings (eg "to to" instead of "to do"). There was even one triple word score "to to to" :-)
Signed-off-by Daniel P. Berrange <[email protected]>--- block/qcow2-cluster.c | 4 ++-- block/qcow2-refcount.c | 2 +- cfg.mk | 4 +++- docs/libcacard.txt | 4 ++-- docs/multiseat.txt | 2 +- docs/specs/qcow2.txt | 2 +- docs/specs/rocker.txt | 2 +- hw/net/rtl8139.c | 2 +- hw/usb/host-libusb.c | 2 +- hw/vfio/common.c | 2 +- include/block/block.h | 2 +- include/exec/memory.h | 2 +- linux-user/elfload.c | 2 +- migration/rdma.c | 2 +- qemu-doc.texi | 2 +- qemu-img.texi | 2 +- qemu-options.hx | 2 +- target-arm/cpu.h | 4 ++-- target-arm/helper.c | 2 +- target-arm/translate.c | 2 +- target-lm32/helper.c | 2 +- target-microblaze/translate.c | 2 +- target-moxie/helper.c | 2 +- util/bitmap.c | 2 +- 24 files changed, 29 insertions(+), 27 deletions(-)block/qcow2-cluster.c +2
-2@@ -498,7 +498,7 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset, *cluster_offset =0 ; -/* seek the the l2 offset in the l1 table */ +/* seek the l2 offset in the l1 table */ l1_index = offset >> l1_bits;if (l1_index >= s->l1_size) {@@ -612,7 +612,7 @@ static int get_cluster_table(BlockDriverState *bs, uint64_t offset,uint64_t *l2_table =NULL ;int ret; -/* seek the the l2 offset in the l1 table */ +/* seek the l2 offset in the l1 table */ l1_index = offset >> (s->l2_bits + s->cluster_bits);if (l1_index >= s->l1_size) {block/qcow2-refcount.c +0
-0
block/qcow2-refcount.c static int realloc_refcount_array(BDRVQcowState *s, void **array, · line 1300
Quoted 8 lines
block/qcow2-refcount.c +1
-1@@ -1294,7 +1294,7 @@ static int realloc_refcount_array(BDRVQcowState *s, void **array,/* * Increases the refcount for a range of clusters in a given refcount table. * This is used to construct a temporary refcount table out of L1 and L2 tables -* which can be compared the the refcount table saved in the image. +* which can be compared the refcount table saved in the image. * * Modifies the number of errors in res. */
I think all of the three hunks above should actually replace the first "the" with a "to" instead of removing it.
Max
RFC PATCH 4/10 maint: remove unused include for assert.h
In reply to Daniel P. BerrangeA number of files were including assert.h but not using any of the functions it provides
--- cfg.mk | 1 - disas/ia64.c | 1 - hw/i386/xen/xen_platform.c | 2 -- linux-user/signal.c | 1 - target-microblaze/op_helper.c | 1 - target-moxie/helper.c | 1 - target-moxie/translate.c | 1 - target-sh4/op_helper.c | 1 - tests/test-xbzrle.c | 1 - 9 files changed, 10 deletions(-)
cfg.mk +0 -1
@@ -48,7 +48,6 @@ local-checks-to-skip = \ sc_preprocessor_indentation \ sc_program_name \ sc_prohibit_always_true_header_tests \ - sc_prohibit_assert_without_use \ sc_prohibit_atoi_atof \ sc_prohibit_cvs_keyword \ sc_prohibit_dirent_without_use \
disas/ia64.c +0 -1
@@ -18,7 +18,6 @@ along with this file; see the file COPYING. If not, see <http://www.gnu.org/licenses/>. */ -#include <assert.h> #include <string.h> #include "disas/bfd.h"
hw/i386/xen/xen_platform.c +0 -2
@@ -23,8 +23,6 @@* THE SOFTWARE. */ -#include <assert.h> -#include "hw/hw.h" #include "hw/i386/pc.h" #include "hw/ide.h"
linux-user/signal.c +0 -1
@@ -22,7 +22,6 @@#include <stdarg.h> #include <unistd.h> #include <errno.h> -#include <assert.h> #include <sys/ucontext.h> #include <sys/resource.h>
target-microblaze/op_helper.c +0 -1
@@ -18,7 +18,6 @@* License along with this library; if not, see <http://www.gnu.org/licenses/>. */ -#include <assert.h> #include "cpu.h" #include "exec/helper-proto.h" #include "qemu/host-utils.h"
target-moxie/helper.c +0 -1
@@ -19,7 +19,6 @@#include <stdio.h> #include <string.h> -#include <assert.h> #include "config.h" #include "cpu.h"
target-moxie/translate.c +0 -1
@@ -26,7 +26,6 @@#include <stdio.h> #include <string.h> #include <inttypes.h> -#include <assert.h> #include "cpu.h" #include "exec/exec-all.h"
target-sh4/op_helper.c +0 -1
@@ -16,7 +16,6 @@* You should have received a copy of the GNU Lesser General Public * License along with this library; if not, see <http://www.gnu.org/licenses/>. */ -#include <assert.h> #include <stdlib.h> #include "cpu.h" #include "exec/helper-proto.h"
tests/test-xbzrle.c +0 -1
@@ -16,7 +16,6 @@#include <strings.h> #include <string.h> #include <sys/time.h> -#include <assert.h> #include "qemu-common.h" #include "include/migration/migration.h"
Signature · 1 line
2.4.3
RFC PATCH 5/10 maint: remove unused include for dirent.h
In reply to Daniel P. BerrangeA number of files were including dirent.h but not using any of the functions it provides
--- cfg.mk | 1 - fsdev/virtio-9p-marshal.c | 1 - hw/tpm/tpm_passthrough.c | 2 -- hw/usb/redirect.c | 1 - hw/vfio/pci.c | 1 - qemu-char.c | 1 - 6 files changed, 7 deletions(-)
cfg.mk +0 -1
@@ -50,7 +50,6 @@ local-checks-to-skip = \ sc_prohibit_always_true_header_tests \ sc_prohibit_atoi_atof \ sc_prohibit_cvs_keyword \ - sc_prohibit_dirent_without_use \ sc_prohibit_empty_lines_at_EOF \ sc_prohibit_hash_without_use \ sc_prohibit_jm_in_m4 \
fsdev/virtio-9p-marshal.c +0 -1
@@ -14,7 +14,6 @@#include <glib.h> #include <glib/gprintf.h> #include <sys/types.h> -#include <dirent.h> #include <sys/time.h> #include <utime.h> #include <sys/uio.h>
hw/tpm/tpm_passthrough.c +0 -2
@@ -22,8 +22,6 @@* License along with this library; if not, see <http://www.gnu.org/licenses/> */ -#include <dirent.h> -#include "qemu-common.h" #include "qapi/error.h" #include "qemu/error-report.h"
hw/usb/redirect.c +0 -1
@@ -33,7 +33,6 @@#include "qemu/iov.h" #include "sysemu/char.h" -#include <dirent.h> #include <sys/ioctl.h> #include <signal.h> #include <usbredirparser.h>
hw/vfio/pci.c +0 -1
@@ -18,7 +18,6 @@* Copyright (C) 2008, IBM, Muli Ben-Yehuda ([email protected]) */ -#include <dirent.h> #include <linux/vfio.h> #include <sys/ioctl.h> #include <sys/mman.h>
qemu-char.c +0 -1
@@ -51,7 +51,6 @@#include <netinet/in.h> #include <net/if.h> #include <arpa/inet.h> -#include <dirent.h> #include <netdb.h> #include <sys/select.h> #ifdef CONFIG_BSD
Signature · 1 line
2.4.3
RFC PATCH 6/10 maint: remove unused include for signal.h
In reply to Daniel P. BerrangeA number of files were including signal.h but not using any of the functions it provides
--- bsd-user/signal.c | 1 - cfg.mk | 1 - hw/block/xen_disk.c | 1 - hw/net/xen_nic.c | 1 - hw/usb/redirect.c | 1 - os-win32.c | 1 - target-i386/translate.c | 1 - target-mips/helper.c | 1 - target-sh4/helper.c | 1 - target-tricore/helper.c | 1 - tests/tcg/testthread.c | 1 - 11 files changed, 11 deletions(-)
bsd-user/signal.c +0 -1
@@ -21,7 +21,6 @@#include <string.h> #include <stdarg.h> #include <unistd.h> -#include <signal.h> #include <errno.h> #include "qemu.h"
cfg.mk +0 -1
@@ -57,7 +57,6 @@ local-checks-to-skip = \ sc_prohibit_path_max_allocation \ sc_prohibit_quotearg_without_use \ sc_prohibit_quote_without_use \ - sc_prohibit_signal_without_use \ sc_prohibit_S_IS_definition \ sc_prohibit_stat_st_blocks \ sc_prohibit_strcmp \
hw/block/xen_disk.c +0 -1
@@ -24,7 +24,6 @@#include <stdarg.h> #include <string.h> #include <unistd.h> -#include <signal.h> #include <inttypes.h> #include <time.h> #include <fcntl.h>
hw/net/xen_nic.c +0 -1
@@ -24,7 +24,6 @@#include <stdarg.h> #include <string.h> #include <unistd.h> -#include <signal.h> #include <inttypes.h> #include <fcntl.h> #include <errno.h>
hw/usb/redirect.c +0 -1
@@ -34,7 +34,6 @@#include "sysemu/char.h" #include <sys/ioctl.h> -#include <signal.h> #include <usbredirparser.h> #include <usbredirfilter.h>
os-win32.c +0 -1
@@ -26,7 +26,6 @@#include <mmsystem.h> #include <unistd.h> #include <fcntl.h> -#include <signal.h> #include <time.h> #include <errno.h> #include <sys/time.h>
target-i386/translate.c +0 -1
@@ -21,7 +21,6 @@#include <stdio.h> #include <string.h> #include <inttypes.h> -#include <signal.h> #include "qemu/host-utils.h" #include "cpu.h"
target-mips/helper.c +0 -1
@@ -21,7 +21,6 @@#include <stdio.h> #include <string.h> #include <inttypes.h> -#include <signal.h> #include "cpu.h" #include "sysemu/kvm.h"
target-sh4/helper.c +0 -1
@@ -21,7 +21,6 @@#include <stdio.h> #include <string.h> #include <inttypes.h> -#include <signal.h> #include "cpu.h"
target-tricore/helper.c +0 -1
@@ -20,7 +20,6 @@#include <stdio.h> #include <string.h> #include <inttypes.h> -#include <signal.h> #include "cpu.h"
tests/tcg/testthread.c +0 -1
@@ -2,7 +2,6 @@#include <stdlib.h> #include <stdio.h> #include <string.h> -#include <signal.h> #include <unistd.h> #include <inttypes.h> #include <pthread.h>
Signature · 1 line
2.4.3
RFC PATCH 7/10 maint: remove unused include for strings.h
In reply to Daniel P. BerrangeA number of files were including strings.h but not using any of the functions it provides
--- cfg.mk | 1 - disas/microblaze.c | 1 - hw/pci/shpc.c | 1 - page_cache.c | 1 - tests/test-xbzrle.c | 1 - 5 files changed, 5 deletions(-)
cfg.mk +0 -1
@@ -60,7 +60,6 @@ local-checks-to-skip = \ sc_prohibit_S_IS_definition \ sc_prohibit_stat_st_blocks \ sc_prohibit_strcmp \ - sc_prohibit_strings_without_use \ sc_prohibit_strncpy \ sc_prohibit_test_double_equal \ sc_prohibit_test_minus_ao \
disas/microblaze.c +0 -1
@@ -582,7 +582,6 @@ static const char pvr_register_prefix[] = "rpvr";#endif /* MICROBLAZE_OPC */ #include "disas/bfd.h" -#include <strings.h> #define get_field_rd (instr) get_field(instr, RD_MASK, RD_LOW)#define get_field_r1 (instr) get_field(instr, RA_MASK, RA_LOW)
hw/pci/shpc.c +0 -1
@@ -1,5 +1,4 @@#include "qemu-common.h" -#include <strings.h> #include <stdint.h> #include "qemu/range.h" #include "qemu/error-report.h"
page_cache.c +0 -1
@@ -15,7 +15,6 @@#include <stdint.h> #include <stdio.h> #include <stdlib.h> -#include <strings.h> #include <string.h> #include <sys/time.h> #include <sys/types.h>
tests/test-xbzrle.c +0 -1
@@ -13,7 +13,6 @@#include <stdint.h> #include <stdio.h> #include <stdlib.h> -#include <strings.h> #include <string.h> #include <sys/time.h> #include "qemu-common.h"
Signature · 1 line
2.4.3
RFC PATCH 8/10 maint: avoid useless "if (foo) free(foo)" pattern
In reply to Daniel P. BerrangeThe free() and g_free() functions both happily accept NULL on any platform QEMU builds on. As such putting a conditional 'if (foo)' check before calls to 'free(foo)' merely serves to bloat the lines of code.
--- backends/hostmem-file.c | 4 +--- bsd-user/elfload.c | 4 +--- cfg.mk | 4 +++- disas/sparc.c | 3 +-- hw/bt/hci.c | 9 +++------ hw/core/loader.c | 3 +-- hw/core/qdev-properties.c | 4 +--- hw/display/exynos4210_fimd.c | 4 +--- hw/mips/mips_r4k.c | 4 +--- hw/net/fsl_etsec/rings.c | 4 +--- hw/net/rocker/rocker.c | 4 +--- hw/net/rocker/rocker_desc.c | 8 ++------ hw/nvram/fw_cfg.c | 4 +--- hw/pci-host/prep.c | 4 +--- hw/sd/sd.c | 3 +-- hw/usb/hcd-xhci.c | 4 +--- hw/xen/xen_pt_config_init.c | 4 +--- migration/savevm.c | 8 ++------ qemu-char.c | 4 +--- tests/bios-tables-test.c | 36 +++++++++++++----------------------- ui/spice-display.c | 14 ++++---------- 21 files changed, 42 insertions(+), 94 deletions(-)
backends/hostmem-file.c +1 -3
@@ -83,9 +83,7 @@ static void set_mem_path(Object *o, const char *str, Error **errp)error_setg (errp,"cannot change property value" );return ; } -if (fb->mem_path) { -g_free (fb->mem_path); - } +g_free (fb->mem_path); fb->mem_path =g_strdup (str); }
bsd-user/elfload.c +1 -3
@@ -1355,9 +1355,7 @@ int load_elf_binary(struct linux_binprm * bprm, struct target_pt_regs * regs, } }if (!bprm->p) { -if (elf_interpreter) { -free (elf_interpreter); - } +free (elf_interpreter);free (elf_phdata);close (bprm->fd);return -E2BIG ;
cfg.mk +3 -1
@@ -25,7 +25,6 @@ SED = sed # Tests not to run as part of "make distcheck". local-checks-to-skip = \ sc_always_defined_macros \ - sc_avoid_if_before_free \ sc_bindtextdomain \ sc_cast_of_alloca_return_value \ sc_cast_of_argument_to_free \
@@ -138,3 +137,6 @@ exclude_file_name_regexp--sc_prohibit_double_semicolon = \ exclude_file_name_regexp--sc_prohibit_doubled_word = \ ^(disas/sparc\.c|scripts/checkpatch\.pl|target-s390x/insn-data\.def)$ + +exclude_file_name_regexp--sc_avoid_if_before_free = \ + ^(libdecnumber)$
disas/sparc.c +1 -2
@@ -2622,8 +2622,7 @@ build_hash_table (const sparc_opcode **opcode_table,memset (hash_table,0 ,HASH_SIZE *sizeof (hash_table[0 ]));memset (hash_count,0 ,HASH_SIZE *sizeof (hash_count[0 ])); -if (hash_buf !=NULL ) -free (hash_buf); +free (hash_buf); hash_buf =malloc (sizeof (* hash_buf) * num_opcodes);for (i = num_opcodes -1 ; i >=0 ; --i) {
hw/bt/hci.c +3 -6
@@ -1151,8 +1151,7 @@ static void bt_hci_reset(struct bt_hci_s *hci) hci->event_mask[7 ] =0x00 ; hci->device.inquiry_scan =0 ; hci->device.page_scan =0 ; -if (hci->device.lmp_name) -g_free ((void *) hci->device.lmp_name); +g_free ((void *) hci->device.lmp_name); hci->device.lmp_name =NULL ; hci->device.class[0 ] =0x00 ; hci->device.class[1 ] =0x00 ;
@@ -1829,8 +1828,7 @@ static void bt_submit_hci(struct HCIInfo *info,case cmd_opcode_pack (OGF_HOST_CTL ,OCF_CHANGE_LOCAL_NAME ):LENGTH_CHECK (change_local_name); -if (hci->device.lmp_name) -g_free ((void *) hci->device.lmp_name); +g_free ((void *) hci->device.lmp_name); hci->device.lmp_name =g_strndup (PARAM (change_local_name, name),sizeof (PARAM (change_local_name, name)));bt_hci_event_complete_status (hci,HCI_SUCCESS );
@@ -2231,8 +2229,7 @@ static void bt_hci_done(struct HCIInfo *info)bt_device_done (&hci->device); -if (hci->device.lmp_name) -g_free ((void *) hci->device.lmp_name); +g_free ((void *) hci->device.lmp_name);/* Be gentle and send DISCONNECT to all connected peers and those * currently waiting for us to accept or reject a connection request.
hw/core/loader.c +1 -2
@@ -594,8 +594,7 @@ static int load_uboot_image(const char *filename, hwaddr *ep, hwaddr *loadaddr, ret = hdr->ih_size; out: -if (data) -g_free (data); +g_free (data);close (fd);return ret; }
hw/core/qdev-properties.c +1 -3
@@ -422,9 +422,7 @@ static void set_string(Object *obj, Visitor *v, void *opaque,error_propagate (errp, local_err);return ; } -if (*ptr) { -g_free (*ptr); - } +g_free (*ptr); *ptr = str; }
hw/display/exynos4210_fimd.c +1 -3
@@ -1354,9 +1354,7 @@ static void exynos4210_fimd_reset(DeviceState *d)fimd_update_get_alpha (s, w); } -if (s->ifb !=NULL ) { -g_free (s->ifb); - } +g_free (s->ifb); s->ifb =NULL ;exynos4210_fimd_invalidate (s);
hw/mips/mips_r4k.c +1 -3
@@ -252,9 +252,7 @@ void mips_r4k_init(MachineState *machine)fprintf (stderr,"qemu: Warning, could not load MIPS bios '%s'\n" , bios_name); } -if (filename) { -g_free (filename); - } +g_free (filename);if (kernel_filename) { loaderparams.ram_size = ram_size;
hw/net/fsl_etsec/rings.c +1 -3
@@ -464,9 +464,7 @@ static void rx_init_frame(eTSEC *etsec, const uint8_t *buf, size_t size) etsec->rx_fcb_size =0 ; } -if (etsec->rx_buffer !=NULL ) { -g_free (etsec->rx_buffer); - } +g_free (etsec->rx_buffer);/* Do not copy the frame for now */ etsec->rx_buffer = (uint8_t *)buf;
hw/net/rocker/rocker.c +1 -3
@@ -265,9 +265,7 @@ err_bad_io: err_no_mem: err_bad_attr:for (i =0 ; i <ROCKER_TX_FRAGS_MAX ; i++) { -if (iov[i].iov_base) { -g_free (iov[i].iov_base); - } +g_free (iov[i].iov_base); }return err;
hw/net/rocker/rocker_desc.c +2 -6
@@ -136,9 +136,7 @@ bool desc_ring_set_size(DescRing *ring, uint32_t size) }for (i =0 ; i < ring->size; i++) { -if (ring->info[i].buf) { -g_free (ring->info[i].buf); - } +g_free (ring->info[i].buf); } ring->size = size;
@@ -360,9 +358,7 @@ DescRing *desc_ring_alloc(Rocker *r, int index)void desc_ring_free (DescRing *ring) { -if (ring->info) { -g_free (ring->info); - } +g_free (ring->info);g_free (ring); }
hw/nvram/fw_cfg.c +1 -3
@@ -187,9 +187,7 @@ static void fw_cfg_bootsplash(FWCfgState *s)g_free (filename);return ; } -if (boot_splash_filedata !=NULL ) { -g_free (boot_splash_filedata); - } +g_free (boot_splash_filedata); boot_splash_filedata = (uint8_t *)file_data; boot_splash_filedata_size = file_size;
hw/pci-host/prep.c +1 -3
@@ -328,9 +328,7 @@ static void raven_realize(PCIDevice *d, Error **errp)if (bios_size <0 || bios_size >BIOS_SIZE ) {hw_error ("qemu: could not load bios image '%s'\n" , s->bios_name); } -if (filename) { -g_free (filename); - } +g_free (filename); } }
hw/sd/sd.c +1 -2
@@ -412,8 +412,7 @@ static void sd_reset(SDState *sd)sd_set_cardstatus (sd);sd_set_sdstatus (sd); -if (sd->wp_groups) -g_free (sd->wp_groups); +g_free (sd->wp_groups); sd->wp_switch = sd->blk ?blk_is_read_only (sd->blk) : false; sd->wpgrps_size = sect; sd->wp_groups =bitmap_new (sd->wpgrps_size);
hw/usb/hcd-xhci.c +1 -3
@@ -1453,9 +1453,7 @@ static int xhci_ep_nuke_one_xfer(XHCITransfer *t, TRBCCode report) t->running_retry =0 ; killed =1 ; } -if (t->trbs) { -g_free (t->trbs); - } +g_free (t->trbs); t->trbs =NULL ; t->trb_count = t->trb_alloced =0 ;
hw/xen/xen_pt_config_init.c +1 -3
@@ -1918,9 +1918,7 @@ void xen_pt_config_delete(XenPCIPassthroughState *s)if (s->msix) {xen_pt_msix_delete (s); } -if (s->msi) { -g_free (s->msi); - } +g_free (s->msi);/* free all register group entry */ QLIST_FOREACH_SAFE(reg_group, &s->reg_grps, entries, next_grp) {
migration/savevm.c +2 -6
@@ -551,9 +551,7 @@ void unregister_savevm(DeviceState *dev, const char *idstr, void *opaque) QTAILQ_FOREACH_SAFE(se, &savevm_state.handlers, entry, new_se) {if (strcmp (se->idstr, id) ==0 && se->opaque == opaque) {QTAILQ_REMOVE (&savevm_state.handlers, se, entry); -if (se->compat) { -g_free (se->compat); - } +g_free (se->compat);g_free (se->ops);g_free (se); }
@@ -612,9 +610,7 @@ void vmstate_unregister(DeviceState *dev, const VMStateDescription *vmsd, QTAILQ_FOREACH_SAFE(se, &savevm_state.handlers, entry, new_se) {if (se->vmsd == vmsd && se->opaque == opaque) {QTAILQ_REMOVE (&savevm_state.handlers, se, entry); -if (se->compat) { -g_free (se->compat); - } +g_free (se->compat);g_free (se); } }
qemu-char.c +1 -3
@@ -2710,9 +2710,7 @@ static int tcp_set_msgfds(CharDriverState *chr, int *fds, int num)TCPCharDriver *s = chr->opaque;/* clear old pending fd array */ -if (s->write_msgfds) { -g_free (s->write_msgfds); - } +g_free (s->write_msgfds);if (num) { s->write_msgfds =g_malloc (num *sizeof (int ));
tests/bios-tables-test.c +13 -23
@@ -161,31 +161,23 @@ static void free_test_data(test_data *data)AcpiSdtTable *temp;int i; -if (data->rsdt_tables_addr) { -g_free (data->rsdt_tables_addr); - } +g_free (data->rsdt_tables_addr);for (i =0 ; i < data->tables->len; ++i) { temp = &g_array_index (data->tables, AcpiSdtTable, i); -if (temp->aml) { -g_free (temp->aml); - } -if (temp->aml_file) { -if (!temp->tmp_files_retain && -g_strstr_len (temp->aml_file,-1 ,"aml-" )) { -unlink (temp->aml_file); - } -g_free (temp->aml_file); +g_free (temp->aml); +if (temp->aml_file && + !temp->tmp_files_retain && +g_strstr_len (temp->aml_file,-1 ,"aml-" )) { +unlink (temp->aml_file); } -if (temp->asl) { -g_free (temp->asl); - } -if (temp->asl_file) { -if (!temp->tmp_files_retain) { -unlink (temp->asl_file); - } -g_free (temp->asl_file); +g_free (temp->aml_file); +g_free (temp->asl); +if (temp->asl_file && + !temp->tmp_files_retain) { +unlink (temp->asl_file); } +g_free (temp->asl_file); }g_array_free (data->tables, false);
@@ -420,9 +412,7 @@ static void dump_aml_files(test_data *data, bool rebuild)close (fd); -if (aml_file) { -g_free (aml_file); - } +g_free (aml_file); } }
ui/spice-display.c +4 -10
@@ -737,9 +737,7 @@ static void display_mouse_set(DisplayChangeListener *dcl,qemu_mutex_lock (&ssd->lock); ssd->ptr_x = x; ssd->ptr_y = y; -if (ssd->ptr_move) { -g_free (ssd->ptr_move); - } +g_free (ssd->ptr_move); ssd->ptr_move =qemu_spice_create_cursor_update (ssd,NULL , on);qemu_mutex_unlock (&ssd->lock); }
@@ -752,13 +750,9 @@ static void display_mouse_define(DisplayChangeListener *dcl,qemu_mutex_lock (&ssd->lock); ssd->hot_x = c->hot_x; ssd->hot_y = c->hot_y; -if (ssd->ptr_move) { -g_free (ssd->ptr_move); - ssd->ptr_move =NULL ; - } -if (ssd->ptr_define) { -g_free (ssd->ptr_define); - } +g_free (ssd->ptr_move); + ssd->ptr_move =NULL ; +g_free (ssd->ptr_define); ssd->ptr_define =qemu_spice_create_cursor_update (ssd, c,0 );qemu_mutex_unlock (&ssd->lock); }
Signature · 1 line
2.4.3
RFC PATCH 9/10 maint: add check for use of POSIX functions which are not reentrant safe
In reply to Daniel P. BerrangeImports a rule from libvirt to make sure that for any POSIX function which is not reentrant safe, the _r variant is used in the source.
While it is entirely possible that many of the QEMU uses are in fact safe, it is pretty difficult to prove that conclusively due to the increasing use of 3rd party libraries. For features like spice, glusterfs, rbd, iscsi these libraries may either be used from QEMU non-main threads, or can be secretly using threads themselves behind QEMU's back.
Given this, the only safe thing todo is to forbid all use of the non-reentrant safe POSIX functions. While Linux platforms have long had the full set of _r variants, other OS may not be so lucky, particularly Mingw32, so fixing this will require some portability code on various platforms.
Since fixing the current QEMU usage is non-trivial, the check is disabled for any functions QEMU currently relies on. IOW, this check merely stops the current problem getting any worse. Future work will have to look at fixing existing violations.
--- Makefile.nonreentrant | 120 ++++++++++++++++++++++++++++++++++++++++++++++++++ cfg.mk | 14 ++++++ 2 files changed, 134 insertions(+) create mode 100644 Makefile.nonreentrant
Makefile.nonreentrant new file +120 -0
@@ -0,0 +1,120 @@ +## Functions for sc_prohibit_nonreentrant -*- makefile -*- +## +## Copyright (C) 2009-2010, 2013 Red Hat, Inc. +## +## This library is free software; you can redistribute it and/or +## modify it under the terms of the GNU Lesser General Public +## License as published by the Free Software Foundation; either +## version 2.1 of the License, or (at your option) any later version. +## +## This library 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 +## Lesser General Public License for more details. +## +## You should have received a copy of the GNU Lesser General Public +## License along with this library. If not, see +## <http://www.gnu.org/licenses/>. + +# +# Generated by running the following on Fedora 9: +# +# nm -D --defined-only /lib/libc.so.6 \ +# | grep '_r$' \ +# | awk '{print $3}' \ +# | grep -v __ \ +# | grep -v qsort \ # Red herring since we don't need to pass extra args to qsort comparator +# | grep -v readdir \ # This is safe as long as each DIR * instance is only used by one thread +# | sort \ +# | uniq \ +# | sed -e 's/_r//' +# +# Also manually add in all inet_* functions some of which +# are not threadsafe and do not have _r variants. They are +# all deprecated in favour of getnameinfo/getaddrinfo +# + +# Current QEMU violations are commented out. Eventual goal +# is to uncomment all these functions, once QEMU code is +# fixed +NON_REENTRANT = +NON_REENTRANT += asctime +NON_REENTRANT += ctime +NON_REENTRANT += drand48 +NON_REENTRANT += ecvt +NON_REENTRANT += erand48 +NON_REENTRANT += ether_aton +NON_REENTRANT += ether_ntoa +#NON_REENTRANT += fcvt +NON_REENTRANT += fgetgrent +NON_REENTRANT += fgetpwent +NON_REENTRANT += fgetspent +NON_REENTRANT += getaliasbyname +NON_REENTRANT += getaliasent +NON_REENTRANT += getdate +NON_REENTRANT += getgrent +NON_REENTRANT += getgrgid +NON_REENTRANT += getgrnam +NON_REENTRANT += gethostbyaddr +NON_REENTRANT += gethostbyname2 +#NON_REENTRANT += gethostbyname +NON_REENTRANT += gethostent +NON_REENTRANT += getlogin +#NON_REENTRANT += getmntent +NON_REENTRANT += getnetbyaddr +NON_REENTRANT += getnetbyname +NON_REENTRANT += getnetent +NON_REENTRANT += getnetgrent +NON_REENTRANT += getprotobyname +NON_REENTRANT += getprotobynumber +NON_REENTRANT += getprotoent +NON_REENTRANT += getpwent +#NON_REENTRANT += getpwnam +#NON_REENTRANT += getpwuid +NON_REENTRANT += getrpcbyname +NON_REENTRANT += getrpcbynumber +NON_REENTRANT += getrpcent +NON_REENTRANT += getservbyname +NON_REENTRANT += getservbyport +NON_REENTRANT += getservent +NON_REENTRANT += getspent +NON_REENTRANT += getspnam +NON_REENTRANT += getutent +NON_REENTRANT += getutid +NON_REENTRANT += getutline +#NON_REENTRANT += gmtime +NON_REENTRANT += hcreate +NON_REENTRANT += hdestroy +NON_REENTRANT += hsearch +NON_REENTRANT += initstate +NON_REENTRANT += jrand48 +NON_REENTRANT += lcong48 +#NON_REENTRANT += localtime +NON_REENTRANT += lrand48 +NON_REENTRANT += mrand48 +NON_REENTRANT += nrand48 +#NON_REENTRANT += ptsname +NON_REENTRANT += qecvt +NON_REENTRANT += qfcvt +#NON_REENTRANT += random +#NON_REENTRANT += rand +NON_REENTRANT += seed48 +NON_REENTRANT += setstate +NON_REENTRANT += sgetspent +NON_REENTRANT += srand48 +#NON_REENTRANT += srandom +#NON_REENTRANT += strerror +#NON_REENTRANT += strtok +NON_REENTRANT += tmpnam +NON_REENTRANT += ttyname +#NON_REENTRANT += inet_addr +#NON_REENTRANT += inet_aton +NON_REENTRANT += inet_lnaof +NON_REENTRANT += inet_makeaddr +NON_REENTRANT += inet_netof +NON_REENTRANT += inet_network +NON_REENTRANT += inet_nsap_addr +NON_REENTRANT += inet_nsap_ntoa +#NON_REENTRANT += inet_ntoa +#NON_REENTRANT += inet_ntop +#NON_REENTRANT += inet_pton
cfg.mk +14 -0
@@ -128,6 +128,20 @@ sc_copyright_format: halt='spell Red Hat as two words' \ $(_sc_search_regexp) +# Use a subshell for each function, to give the optimal warning message. +include $(srcdir)/Makefile.nonreentrant + +sc_prohibit_nonreentrant: + @for i in $(NON_REENTRANT) ; \ + do \ + (prohibit="\\<$$i *\\(" \ + in_vc_files='\.[ch]$$' \ + halt="use $${i}_r, not $$i" \ + $(_sc_search_regexp) \ + ) || fail=1; \ + done ; \ + exit $$fail + # We don't use this feature of maint.mk. prev_version_file = /dev/null
Signature · 1 line
2.4.3
RFC PATCH 10/10 maint: enable checking for qemu/osdep.h header usage
In reply to Daniel P. BerrangeUncomment rules which mandate that qemu/osdep.h is included in all .c file, and that it appears in the file before any other includes.
NB, this change isn't intended to be applied as it obviously fails. It is just an illustration of how we'd enforce such a header usage rule.
--- cfg.mk | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
cfg.mk +2 -2
@@ -63,8 +63,6 @@ local-checks-to-skip = \ sc_prohibit_test_double_equal \ sc_prohibit_test_minus_ao \ sc_prohibit_undesirable_word_seq \ - sc_require_config_h \ - sc_require_config_h_first \ sc_require_test_exit_idiom \ sc_root_tests \ sc_space_tab \
@@ -80,6 +78,8 @@ local-checks-to-skip = \ sc_Wundef_boolean \ $(NULL) +config_h_header ?= "qemu/osdep\.h" + # Files that should never cause syntax check failures. VC_LIST_ALWAYS_EXCLUDE_REGEX = \ (^HACKING|\.po|^maint\.mk|^cfg\.mk|^pc-bios/.*)$$
Signature · 1 line
2.4.3
Quoted 11 lines · Daniel P. Berrange
Historically QEMU has relied on the checkpatch.pl script, borrowed from Linux, to check coding style compliance on patches which are submitted. For what it is designed for, it does a reasonable job, but I feel that QEMU would benefit from some more checking in this area, in particular checks that run across the entire repository, not just new patches.
Rather than attempt to replace checkpatch.pl, this series illustrates how we can augment our existing style compliance checking. This imports the infrastructure from GNULIB which provides a 'syntax-check' target in the makefiles.
I like this in principle, but I notice that the GNULIB infrastructure is GPLv3. I really would strongly prefer that we not introduce any GPLv3 code into QEMU's git tree. I don't want to get into trying to figure out whether part of our makefile or build infrastructure being GPLv3 implies the generated binaries are GPLv3 or not; the simplest approach is just to not allow any GPLv3 code into the git repo.
Is there no GPLv2 or GPLv2-or-later version of this code?
thanks
-- PMM
[adding the gnulib list]
Quoted 21 lines · Peter Maydell
Quoted 11 lines · Daniel P. Berrange
Historically QEMU has relied on the checkpatch.pl script, borrowed from Linux, to check coding style compliance on patches which are submitted. For what it is designed for, it does a reasonable job, but I feel that QEMU would benefit from some more checking in this area, in particular checks that run across the entire repository, not just new patches.
Rather than attempt to replace checkpatch.pl, this series illustrates how we can augment our existing style compliance checking. This imports the infrastructure from GNULIB which provides a 'syntax-check' target in the makefiles.
I like this in principle, but I notice that the GNULIB infrastructure is GPLv3. I really would strongly prefer that we not introduce any GPLv3 code into QEMU's git tree. I don't want to get into trying to figure out whether part of our makefile or build infrastructure being GPLv3 implies the generated binaries are GPLv3 or not; the simplest approach is just to not allow any GPLv3 code into the git repo.
Quoted 1 line
Is there no GPLv2 or GPLv2-or-later version of this code?
Looks like gnulib.git commit fb7fde6f6 was the one that upgraded maint.mk to GPLv3+, on the grounds that building with GPLv3+ code does not infect your binary (see also the arguments in Autoconf, Automake, gcc... - but those tend to include an explicit exception clause to make it clear). Then again, qemu also avoids use of Autoconf and Automake.
It's worth asking the gnulib folks for an opinion on whether relaxing the license on maint.mk and GNUmakefile to explicitly go back to GPLv2+, and/or explicitly add some explicit exception clause like gcc that makes it clear that using these files to build does not taint the built product. Personally, I see no problem with using GPLv3'd tools (after all, qemu requires GPLv3 GNU make, and gcc is also GPLv3 although clang can step around that one), but I also see your reluctance of even having a file in the qemu.git repo that has a GPLv3 clause.
As one of the contributors to gnulib, I'm okay with my contributions to the files being relaxed; but we either need FSF blessing or consent from all contributors to the files in question, which could get a bit daunting:
$ git shortlog top/maint.mk build-aux/useless-if-before-free \ build-aux/vc-list-files | grep '^[^[:space:]]' Akim Demaille (16): Alfred M. Szmidt (2): Assaf Gordon (1): Daniel P. Berrange (1): Eric Blake (54): Gary V. Vaughan (4): Giuseppe Scrivano (1): Jim Meyering (174): Jiri Denemark (1): Joel E. Denny (2): Jose E. Marchesi (5): Karl Heuer (1): Martin von Gagern (1): Paul Eggert (4): Peter Rosin (1): Peter Simons (2): Pádraig Brady (6): Ralf Wildenhues (4): Reuben Thomas (2): Simon Josefsson (16): Soren Hansen (1): Stefano Lattarini (2): Theophile Ranquet (1):
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Quoted 8 lines · Eric Blake
It's worth asking the gnulib folks for an opinion on whether relaxing the license on maint.mk and GNUmakefile to explicitly go back to GPLv2+, and/or explicitly add some explicit exception clause like gcc that makes it clear that using these files to build does not taint the built product. Personally, I see no problem with using GPLv3'd tools (after all, qemu requires GPLv3 GNU make, and gcc is also GPLv3 although clang can step around that one), but I also see your reluctance of even having a file in the qemu.git repo that has a GPLv3 clause.
Right; we don't ship make or gcc in our code repo, and using external-to-the-repository tools which happen to be GPLv3 is obviously fine. Similarly, if you used the maint.mk script externally as a tool which allowed you to find bugs which you submitted patches to fix that wouldn't be a problem. I just don't want a GPLv3-licensed file in the git repo and an integrated part of our build-and-test system...
I would certainly appreciate a maint.mk with a GPLv2-or-later license. Our other options are (a) use the last v2+ version (which is what we do with our binutils disassemblers)
(b) do the style checks we care about some other way or
(c) don't bother doing the style checks at all.
(Incidentally we build fine with the GPLv2 GNU make 3.81, and we'll continue to do so as long as that's the make that ships in OSX. So the truly paranoid (among whom I don't count myself) can build QEMU with no GPLv3 tooling at all.)
thanks
-- PMM
Quoted 10 lines · Peter Maydell
Quoted 8 lines · Eric Blake
It's worth asking the gnulib folks for an opinion on whether relaxing the license on maint.mk and GNUmakefile to explicitly go back to GPLv2+, and/or explicitly add some explicit exception clause like gcc that makes it clear that using these files to build does not taint the built product. Personally, I see no problem with using GPLv3'd tools (after all, qemu requires GPLv3 GNU make, and gcc is also GPLv3 although clang can step around that one), but I also see your reluctance of even having a file in the qemu.git repo that has a GPLv3 clause.
Quoted 7 lines
Right; we don't ship make or gcc in our code repo, and using external-to-the-repository tools which happen to be GPLv3 is obviously fine. Similarly, if you used the maint.mk script externally as a tool which allowed you to find bugs which you submitted patches to fix that wouldn't be a problem. I just don't want a GPLv3-licensed file in the git repo and an integrated part of our build-and-test system...
Ok, I certainly understand why we can't have GPLv3 code built into QEMU, but I thought build-system tests would be ok because it does not affect the built binaries in any way.
Quoted 5 lines
I would certainly appreciate a maint.mk with a GPLv2-or-later license. Our other options are (a) use the last v2+ version (which is what we do with our binutils disassemblers)
(b) do the style checks we care about some other way or
(c) don't bother doing the style checks at all.
Option (b) could involve re-factoring the existing check_patch.pl script to give us the 2 main benefits from the gnulib check code
- Ability to turn on/off individual rules on a per-file basis
- Ability to run against the entire codebase not just patches
IIUC, the check_patch.pl script was imported from Linux, so I'm not sure if there is a general desire to minimize the divergance from the original file, or whether refactoring would be welcome ?
I can certainly explore the viability of such an approach if people are conceptually open to some significant changes to check_patch.pl
Regards,
Daniel
Signature · 4 lines
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Quoted 3 lines · Peter Maydell
I just don't want
a GPLv3-licensed file in the git repo and an integrated part of our build-and-test system...
My kneejerk reaction is that the build procedures in question are large enough that they should stay GPLv3. If you don't want those files in your git repo you can simply fetch them as part of your bootstrap or autogen.sh or whatever. Although this might not mollify people who worry about GPLv3 cooties infecting their executables, catering to paranoia is not high on our list of things to do.
Quoted 6 lines · Paul Eggert
Quoted 3 lines · Peter Maydell
I just don't want
a GPLv3-licensed file in the git repo and an integrated part of our build-and-test system...
Quoted 6 lines
My kneejerk reaction is that the build procedures in question are large enough that they should stay GPLv3. If you don't want those files in your git repo you can simply fetch them as part of your bootstrap or
autogen.shor whatever. Although this might not mollify people who worry about GPLv3 cooties infecting their executables, catering to paranoia is not high on our list of things to do.
My objections to the GPLv3 here are purely pragmatic. QEMU contains too much GPLv2-only code to feasibly rewrite, and GPLv2 and v3 aren't compatible. Therefore we can't use GPLv3 code. That's sometimes awkward for us in that it prevents us using code from other free software projects, but that's the way licensing works.
thanks
-- PMM