[Qemu-devel] [PATCH 0/2 RFC] Implement Hex file loader and add test case

[Qemu-devel] [PATCH 0/2 RFC] Implement Hex file loader and add test case

[Su Hang]

These series of patchs implement Intel Hexadecimal File loader and
add QTest testcase to verify the correctness of Loader.  

Su Hang (2):
  Implement .hex file loader
  Add QTest testcase for the Intel Hexadecimal Object File Loader.

 hw/arm/boot.c          |   9 +-
 hw/core/loader.c       | 280 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/loader.h    |  17 +++
 tests/Makefile.include |   2 +
 tests/hexloader-test.c |  56 ++++++++++
 tests/test.hex         |  11 ++
 6 files changed, 374 insertions(+), 1 deletion(-)
 create mode 100644 tests/hexloader-test.c
 create mode 100644 tests/test.hex

-- 
2.7.4

[Qemu-devel] [PATCH 1/2] Implement .hex file loader

[Su Hang]

This patch adds Intel Hexadecimal Object File format support to
the loader.  The file format specification is available here:
http://www.piclist.com/techref/fileext/hex/intel.htm

The file format is mainly intended for embedded systems
and microcontrollers, such as Arduino, ARM, STM32, etc.

Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Su Hang <suhang16@mails.ucas.ac.cn>
---
 hw/arm/boot.c       |   9 +-
 hw/core/loader.c    | 280 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/loader.h |  17 ++++
 3 files changed, 305 insertions(+), 1 deletion(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 9319b12fcd2a..07ce54e5936d 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -1060,8 +1060,15 @@ static void arm_load_kernel_notify(Notifier *notifier, void *data)
         kernel_size = load_aarch64_image(info->kernel_filename,
                                          info->loader_start, &entry, as);
         is_linux = 1;
+    } else if (kernel_size < 0 && strstr(info->kernel_filename, ".hex")) {
+        /* 32-bit ARM .hex file */
+        entry = info->loader_start + KERNEL_LOAD_ADDR;
+        kernel_size = load_targphys_hex_as(info->kernel_filename, entry,
+                                           info->ram_size - KERNEL_LOAD_ADDR,
+                                           as);
+        is_linux = 1;
     } else if (kernel_size < 0) {
-        /* 32-bit ARM */
+        /* 32-bit ARM binary file */
         entry = info->loader_start + KERNEL_LOAD_ADDR;
         kernel_size = load_image_targphys_as(info->kernel_filename, entry,
                                              info->ram_size - KERNEL_LOAD_ADDR,
diff --git a/hw/core/loader.c b/hw/core/loader.c
index 06bdbca53709..126832c4243c 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -1286,3 +1286,283 @@ void hmp_info_roms(Monitor *mon, const QDict *qdict)
         }
     }
 }
+
+typedef enum HexRecord HexRecord;
+enum HexRecord {
+    DATA_RECORD = 0,
+    EOF_RECORD,
+    EXT_SEG_ADDR_RECORD,
+    START_SEG_ADDR_RECORD,
+    EXT_LINEAR_ADDR_RECORD,
+    START_LINEAR_ADDR_RECORD,
+};
+
+typedef union HexLine HexLine;
+union HexLine {
+    uint8_t buf[0x25];
+    struct __attribute__((packed)) {
+        uint8_t byte_count;
+        uint16_t address;
+        uint8_t record_type;
+        uint8_t data[0x25 - 0x5];
+        uint8_t checksum;
+    };
+};
+
+static uint8_t ctoh(char c)
+{
+    return (c & 0x10) ? /*0-9*/ c & 0xf : /*A-F, a-f*/ (c & 0xf) + 9;
+}
+
+static uint8_t validate_checksum(HexLine *record)
+{
+    uint8_t result = 0, i = 0;
+
+    for (; i < (record->byte_count + 5); ++i) {
+        result += record->buf[i];
+    }
+
+    return result == 0;
+}
+
+/* return pointer of bin_blob or NULL if error */
+static uint8_t *parse_hex_blob(char *filename, size_t *p_size)
+{
+    int fd;
+    off_t hex_blob_size;
+    uint8_t *p_data = NULL;
+    uint8_t *hex_blob;
+    uint8_t *hex_blob_ori;         /* used to free temporary memory */
+    uint8_t *bin_buf;
+    uint8_t *end;
+    uint8_t idx = 0;
+    uint8_t in_process = 0;        /* avoid re-enter */
+    uint8_t low_nibble = 0;        /* process two hex char into 8-bits */
+    uint8_t ext_linear_record = 0; /* record non-constitutes block */
+    uint32_t next_address_to_write = 0;
+    uint32_t current_address = 0;
+    uint32_t last_address = 0;
+    HexLine line = {0};
+
+    fd = open(filename, O_RDONLY);
+    if (fd < 0) {
+        return NULL;
+    }
+    hex_blob_size = lseek(fd, 0, SEEK_END);
+    if (hex_blob_size < 0) {
+        close(fd);
+        return NULL;
+    }
+    hex_blob = g_malloc(hex_blob_size);
+    hex_blob_ori = hex_blob;
+    bin_buf = g_malloc(hex_blob_size * 2);
+    lseek(fd, 0, SEEK_SET);
+    if (read(fd, hex_blob, hex_blob_size) != hex_blob_size) {
+        close(fd);
+        goto hex_parser_exit;
+    }
+    close(fd);
+
+    memset(line.buf, 0, sizeof(HexLine));
+    end = (uint8_t *)hex_blob + hex_blob_size;
+
+    do {
+        switch ((uint8_t)(*hex_blob)) {
+        case '\r':
+        case '\n':
+            if (!in_process) {
+                break;
+            }
+
+            in_process = 0;
+            if (validate_checksum(&line) == 0) {
+                p_data = NULL;
+                goto hex_parser_exit;
+            }
+
+            line.address = bswap16(line.address);
+            switch (line.record_type) {
+            case DATA_RECORD:
+                current_address =
+                    (next_address_to_write & 0xffff0000) | line.address;
+                /* verify this is a continous block of memory */
+                if (current_address != next_address_to_write ||
+                    ext_linear_record) {
+                    if (!ext_linear_record) {
+                        /* Store next address to write */
+                        last_address = next_address_to_write;
+                        next_address_to_write = current_address;
+                    }
+                    ext_linear_record = 0;
+                    memset(bin_buf + last_address, 0x0,
+                           current_address - last_address);
+                }
+
+                /* copy from line buffer to output bin_buf */
+                memcpy((uint8_t *)bin_buf + current_address,
+                       (uint8_t *)line.data, line.byte_count);
+                /* Save next address to write */
+                last_address = current_address;
+                next_address_to_write = current_address + line.byte_count;
+                break;
+
+            case EOF_RECORD:
+                /* nothing to do */
+                break;
+            case EXT_SEG_ADDR_RECORD:
+                /* save next address to write,
+                 * in case of non-continous block of memory */
+                ext_linear_record = 1;
+                last_address = next_address_to_write;
+                next_address_to_write =
+                    ((line.data[0] << 12) | (line.data[1] << 4));
+                break;
+            case START_SEG_ADDR_RECORD:
+                /* TODO */
+                break;
+
+            case EXT_LINEAR_ADDR_RECORD:
+                /* save next address to write,
+                 * in case of non-continous block of memory */
+                ext_linear_record = 1;
+                last_address = next_address_to_write;
+                next_address_to_write =
+                    ((line.data[0] << 24) | (line.data[1] << 16));
+                break;
+            case START_LINEAR_ADDR_RECORD:
+                /* TODO */
+                break;
+
+            default:
+                p_data = NULL;
+                goto hex_parser_exit;
+            }
+            break;
+
+        /* start of a new record. */
+        case ':':
+            memset(line.buf, 0, sizeof(HexLine));
+            in_process = 1;
+            low_nibble = 0;
+            idx = 0;
+            break;
+
+        /* decoding lines */
+        default:
+            if (low_nibble) {
+                line.buf[idx] |= ctoh((uint8_t)(*hex_blob)) & 0xf;
+                ++idx;
+            } else {
+                line.buf[idx] = ctoh((uint8_t)(*hex_blob)) << 4;
+            }
+
+            low_nibble = !low_nibble;
+            break;
+        }
+
+    } while (++hex_blob != end);
+
+    *p_size = (size_t)next_address_to_write;
+    p_data = g_malloc(next_address_to_write);
+
+    memcpy(p_data, bin_buf, next_address_to_write);
+hex_parser_exit:
+    g_free(hex_blob_ori);
+    g_free(bin_buf);
+    return p_data;
+}
+
+/* return size or -1 if error */
+int load_targphys_hex_as(const char *filename, hwaddr addr, uint64_t max_sz,
+                         AddressSpace *as)
+{
+    return rom_add_hex_file(filename, NULL, addr, -1, false, NULL, as);
+}
+
+/* return size -1 if error */
+int rom_add_hex_file(const char *file, const char *fw_dir, hwaddr addr,
+                     int32_t bootindex, bool option_rom, MemoryRegion *mr,
+                     AddressSpace *as)
+{
+    MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
+    Rom *rom;
+    char devpath[100];
+    size_t datasize = 0;
+
+    if (as && mr) {
+        fprintf(stderr, "Specifying an Address Space and Memory Region is "
+                        "not valid when loading a rom\n");
+        /* We haven't allocated anything so we don't need any cleanup */
+        return -1;
+    }
+
+    rom = g_malloc0(sizeof(*rom));
+    rom->name = g_strdup(file);
+    rom->path = qemu_find_file(QEMU_FILE_TYPE_BIOS, rom->name);
+    rom->as = as;
+    if (rom->path == NULL) {
+        rom->path = g_strdup(file);
+    }
+
+    rom->data = parse_hex_blob(rom->path, &datasize);
+    if (rom->data == NULL) {
+        fprintf(stderr, "failed to parse hex file '%s': %s\n", rom->path,
+                strerror(errno));
+        goto err;
+    }
+    rom->datasize = datasize;
+
+    if (fw_dir) {
+        rom->fw_dir = g_strdup(fw_dir);
+        rom->fw_file = g_strdup(file);
+    }
+    rom->addr = addr;
+    rom->romsize = rom->datasize;
+
+    rom_insert(rom);
+
+    if (rom->fw_file && fw_cfg) {
+        const char *basename;
+        char fw_file_name[FW_CFG_MAX_FILE_PATH];
+        void *data;
+
+        basename = strrchr(rom->fw_file, '/');
+        if (basename) {
+            ++basename;
+        } else {
+            basename = rom->fw_file;
+        }
+        snprintf(fw_file_name, sizeof(fw_file_name), "%s/%s", rom->fw_dir,
+                 basename);
+        snprintf(devpath, sizeof(devpath), "/rom@%s", fw_file_name);
+
+        if ((!option_rom || mc->option_rom_has_mr) && mc->rom_file_has_mr) {
+            data = rom_set_mr(rom, OBJECT(fw_cfg), devpath, true);
+        } else {
+            data = rom->data;
+        }
+
+        fw_cfg_add_file(fw_cfg, fw_file_name, data, rom->romsize);
+    } else {
+        if (mr) {
+            rom->mr = mr;
+            snprintf(devpath, sizeof(devpath), "/rom@%s", file);
+        } else {
+            snprintf(devpath, sizeof(devpath), "/rom@" TARGET_FMT_plx, addr);
+        }
+    }
+
+    add_boot_device_path(bootindex, NULL, devpath);
+    return rom->datasize;
+
+err:
+    g_free(rom->path);
+    g_free(rom->name);
+    if (fw_dir) {
+        g_free(rom->fw_dir);
+        g_free(rom->fw_file);
+    }
+    g_free(rom);
+
+    return -1;
+}
diff --git a/include/hw/loader.h b/include/hw/loader.h
index 5ed3fd8ae67a..176b11221a27 100644
--- a/include/hw/loader.h
+++ b/include/hw/loader.h
@@ -28,6 +28,20 @@ ssize_t load_image_size(const char *filename, void *addr, size_t size);
 int load_image_targphys_as(const char *filename,
                            hwaddr addr, uint64_t max_sz, AddressSpace *as);
 
+/**load_image_targphys_hex_as:
+ * @filename: Path to the .hex file
+ * @addr: Address to load the .hex file to
+ * @max_sz: The maximum size of the .hex file to load
+ * @as: The AddressSpace to load the .hex file to. The value of
+ *      address_space_memory is used if nothing is supplied here.
+ *
+ * Load a fixed .hex file into memory.
+ *
+ * Returns the size of the loaded .hex file on success, -1 otherwise.
+ */
+int load_targphys_hex_as(const char *filename,
+                         hwaddr addr, uint64_t max_sz, AddressSpace *as);
+
 /** load_image_targphys:
  * Same as load_image_targphys_as(), but doesn't allow the caller to specify
  * an AddressSpace.
@@ -210,6 +224,9 @@ void pstrcpy_targphys(const char *name,
 extern bool option_rom_has_mr;
 extern bool rom_file_has_mr;
 
+int rom_add_hex_file(const char *file, const char *fw_dir,
+                 hwaddr addr, int32_t bootindex,
+                 bool option_rom, MemoryRegion *mr, AddressSpace *as);
 int rom_add_file(const char *file, const char *fw_dir,
                  hwaddr addr, int32_t bootindex,
                  bool option_rom, MemoryRegion *mr, AddressSpace *as);
-- 
2.7.4

[Qemu-devel] [PATCH v2] scripts/checkpatch.pl: Bug fix

[Su Hang]

checkpatch.pl stops complaining about following pattern:
"""
do {
    //do somethins;
} while (conditions);
"""

One things need to be mentioned:
Becasue `if`, `while` and `for` check have been done in this
`if` block(Line: 2356), and this block contains following statement:
""" Line: 2379
$suppress_ifbraces{$ln + $offset} = 1;
"""
So the behind block may never run:
""" Line: 2415
if (!defined $suppress_ifbraces{$linenr - 1} &&
    $line =~ /\b(if|while|for|else)\b/ &&
    $line !~ /\#\s*if/ &&
    $line !~ /\#\s*else/) {
"""
I'm not sure, please give me some advice.

(Sorry, I don't know this patch should base on which commit,
 so I generate this patch based on
 commit:fb8446d94ec7a3dc0c3a7e7da672406476f075ac,
 I choose this by `git log -2 scripts/checkpath.pl`.
 Sincerely say sorry, if I have misunderstand any meaning.)

Signed-off-by: Su Hang <suhang16@mails.ucas.ac.cn>
---
 scripts/checkpatch.pl | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index a88af61ed4ee..d6f0747ba20a 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2352,8 +2352,22 @@ sub process {
 			}
 		}

-# check for missing bracing round if etc
-		if ($line =~ /(^.*)\bif\b/ && $line !~ /\#\s*if/) {
+# check for missing bracing around if etc
+		if ($line =~ /(^.*)\b(?:if|while|for)\b/ &&
+			$line !~ /\#\s*if/) {
+			my $allowed = 0;
+
+			# Check the pre-context.
+			if ($line =~ /(\}.*?)$/) {
+				my $pre = $1;
+
+				if ($line !~ /else/) {
+					print "APW: ALLOWED: pre<$pre> line<$line>\n"
+						if $dbg_adv_apw;
+					$allowed = 1;
+				}
+			}
+
 			my ($level, $endln, @chunks) =
 				ctx_statement_full($linenr, $realcnt, 1);
                         if ($dbg_adv_apw) {
@@ -2362,7 +2376,6 @@ sub process {
                                 if $#chunks >= 1;
                         }
 			if ($#chunks >= 0 && $level == 0) {
-				my $allowed = 0;
 				my $seen = 0;
 				my $herectx = $here . "\n";
 				my $ln = $linenr - 1;
@@ -2406,7 +2419,7 @@ sub process {
                                             $allowed = 1;
 					}
 				}
-				if ($seen != ($#chunks + 1)) {
+				if ($seen != ($#chunks + 1) && !$allowed) {
 					ERROR("braces {} are necessary for all arms of this statement\n" . $herectx);
 				}
 			}
--
2.7.4

[Qemu-devel] [PATCH 2/2] Add QTest testcase for the Hex File Loader

[Su Hang]

'test.hex' file is a bare metal ARM software stored in Hexadecimal
Object Format. When it's loaded by QEMU, it will print "Hello world!\n"
on console.

`pre_store` array in 'hexloader-test.c' file, stores the binary format
of 'test.hex' file, which is used to verify correctness.

Signed-off-by: Su Hang <suhang16@mails.ucas.ac.cn>
---
 tests/Makefile.include |  2 ++
 tests/hexloader-test.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/test.hex         | 11 ++++++++++
 3 files changed, 69 insertions(+)
 create mode 100644 tests/hexloader-test.c
 create mode 100644 tests/test.hex

diff --git a/tests/Makefile.include b/tests/Makefile.include
index 3b9a5e31a2c2..f4a3e71f34ee 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -380,6 +380,7 @@ check-qtest-arm-y += tests/test-arm-mptimer$(EXESUF)
 gcov-files-arm-y += hw/timer/arm_mptimer.c
 check-qtest-arm-y += tests/boot-serial-test$(EXESUF)
 check-qtest-arm-y += tests/sdhci-test$(EXESUF)
+check-qtest-arm-y += tests/hexloader-test$(EXESUF)
 
 check-qtest-aarch64-y = tests/numa-test$(EXESUF)
 check-qtest-aarch64-y += tests/sdhci-test$(EXESUF)
@@ -755,6 +756,7 @@ tests/qmp-test$(EXESUF): tests/qmp-test.o
 tests/device-introspect-test$(EXESUF): tests/device-introspect-test.o
 tests/rtc-test$(EXESUF): tests/rtc-test.o
 tests/m48t59-test$(EXESUF): tests/m48t59-test.o
+tests/hexloader-test$(EXESUF): tests/hexloader-test.o
 tests/endianness-test$(EXESUF): tests/endianness-test.o
 tests/spapr-phb-test$(EXESUF): tests/spapr-phb-test.o $(libqos-obj-y)
 tests/prom-env-test$(EXESUF): tests/prom-env-test.o $(libqos-obj-y)
diff --git a/tests/hexloader-test.c b/tests/hexloader-test.c
new file mode 100644
index 000000000000..b1a491dcd9de
--- /dev/null
+++ b/tests/hexloader-test.c
@@ -0,0 +1,56 @@
+/*
+ * QTest testcase for the Intel Hexadecimal Object File Loader
+ *
+ * Authors:
+ *  Su Hang <suhang16@mails.ucas.ac.cn> 2018
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "libqtest.h"
+
+#define BIN_SIZE 146
+
+static unsigned char pre_store[BIN_SIZE] = {
+    4,   208, 159, 229, 22,  0,   0,   235, 254, 255, 255, 234, 152, 16,  1,
+    0,   4,   176, 45,  229, 0,   176, 141, 226, 12,  208, 77,  226, 8,   0,
+    11,  229, 6,   0,   0,   234, 8,   48,  27,  229, 0,   32,  211, 229, 44,
+    48,  159, 229, 0,   32,  131, 229, 8,   48,  27,  229, 1,   48,  131, 226,
+    8,   48,  11,  229, 8,   48,  27,  229, 0,   48,  211, 229, 0,   0,   83,
+    227, 244, 255, 255, 26,  0,   0,   160, 225, 0,   208, 139, 226, 4,   176,
+    157, 228, 30,  255, 47,  225, 0,   16,  31,  16,  0,   72,  45,  233, 4,
+    176, 141, 226, 8,   0,   159, 229, 230, 255, 255, 235, 0,   0,   160, 225,
+    0,   136, 189, 232, 132, 0,   1,   0,   0,   16,  31,  16,  72,  101, 108,
+    108, 111, 32,  119, 111, 114, 108, 100, 33,  10,  0};
+
+/* success if no crash or abort */
+static void hex_loader_test(void)
+{
+    unsigned int i;
+    unsigned char memory_content[BIN_SIZE];
+    const unsigned int base_addr = 0x00010000;
+
+    QTestState *s = qtest_startf(
+        "-M versatilepb -m 128M -nographic -kernel ../tests/test.hex");
+
+    for (i = 0; i < BIN_SIZE; ++i) {
+        memory_content[i] = qtest_readb(s, base_addr + i);
+        g_assert_cmpuint(memory_content[i], ==, pre_store[i]);
+    }
+    qtest_quit(s);
+}
+
+int main(int argc, char **argv)
+{
+    int ret;
+
+    g_test_init(&argc, &argv, NULL);
+
+    qtest_add_func("/tmp/hex_loader", hex_loader_test);
+    ret = g_test_run();
+
+    return ret;
+}
diff --git a/tests/test.hex b/tests/test.hex
new file mode 100644
index 000000000000..59b96e3e6fa7
--- /dev/null
+++ b/tests/test.hex
@@ -0,0 +1,11 @@
+:1000000004D09FE5160000EBFEFFFFEA9810010008
+:1000100004B02DE500B08DE20CD04DE208000BE5F8
+:10002000060000EA08301BE50020D3E52C309FE5F0
+:10003000002083E508301BE5013083E208300BE542
+:1000400008301BE50030D3E5000053E3F4FFFF1A4E
+:100050000000A0E100D08BE204B09DE41EFF2FE180
+:1000600000101F1000482DE904B08DE208009FE544
+:10007000E6FFFFEB0000A0E10088BDE8840001007E
+:1000800000101F1048656C6C6F20776F726C6421D4
+:020090000A0064
+:00000001FF
-- 
2.7.4

Re: [Qemu-devel] [PATCH 0/2 RFC] Implement Hex file loader and add test case

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 1522768634-5548-1-git-send-email-suhang16@mails.ucas.ac.cn
Subject: [Qemu-devel] [PATCH 0/2 RFC] Implement Hex file loader and add test case

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]               patchew/1522768634-5548-1-git-send-email-suhang16@mails.ucas.ac.cn -> patchew/1522768634-5548-1-git-send-email-suhang16@mails.ucas.ac.cn
Switched to a new branch 'test'
ae6e4ea631 Add QTest testcase for the Hex File Loader
3392bbf17c Implement .hex file loader

=== OUTPUT BEGIN ===
Checking PATCH 1/2: Implement .hex file loader...
ERROR: braces {} are necessary for all arms of this statement
#220: FILE: hw/core/loader.c:1463:
+    } while (++hex_blob != end);
[...]

total: 1 errors, 0 warnings, 328 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 2/2: Add QTest testcase for the Hex File Loader...
=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [PATCH v2] scripts/checkpatch.pl: Bug fix

[Su Hang]

Please ignore this email, this patch was accidentally sent.

"Su Hang" <suhang16@mails.ucas.ac.cn>wrote:
> checkpatch.pl stops complaining about following pattern:
> """
> do {
>     //do somethins;
> } while (conditions);
> """
> 
> One things need to be mentioned:
> Becasue `if`, `while` and `for` check have been done in this
> `if` block(Line: 2356), and this block contains following statement:
> """ Line: 2379
> $suppress_ifbraces{$ln + $offset} = 1;
> """
> So the behind block may never run:
> """ Line: 2415
> if (!defined $suppress_ifbraces{$linenr - 1} &&
>     $line =~ /\b(if|while|for|else)\b/ &&
>     $line !~ /\#\s*if/ &&
>     $line !~ /\#\s*else/) {
> """
> I'm not sure, please give me some advice.
> 
> (Sorry, I don't know this patch should base on which commit,
>  so I generate this patch based on
>  commit:fb8446d94ec7a3dc0c3a7e7da672406476f075ac,
>  I choose this by `git log -2 scripts/checkpath.pl`.
>  Sincerely say sorry, if I have misunderstand any meaning.)
> 
> Signed-off-by: Su Hang <suhang16@mails.ucas.ac.cn>
> ---
>  scripts/checkpatch.pl | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index a88af61ed4ee..d6f0747ba20a 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2352,8 +2352,22 @@ sub process {
>  			}
>  		}
> 
> -# check for missing bracing round if etc
> -		if ($line =~ /(^.*)\bif\b/ && $line !~ /\#\s*if/) {
> +# check for missing bracing around if etc
> +		if ($line =~ /(^.*)\b(?:if|while|for)\b/ &&
> +			$line !~ /\#\s*if/) {
> +			my $allowed = 0;
> +
> +			# Check the pre-context.
> +			if ($line =~ /(\}.*?)$/) {
> +				my $pre = $1;
> +
> +				if ($line !~ /else/) {
> +					print "APW: ALLOWED: pre<$pre> line<$line>\n"
> +						if $dbg_adv_apw;
> +					$allowed = 1;
> +				}
> +			}
> +
>  			my ($level, $endln, @chunks) =
>  				ctx_statement_full($linenr, $realcnt, 1);
>                          if ($dbg_adv_apw) {
> @@ -2362,7 +2376,6 @@ sub process {
>                                  if $#chunks >= 1;
>                          }
>  			if ($#chunks >= 0 && $level == 0) {
> -				my $allowed = 0;
>  				my $seen = 0;
>  				my $herectx = $here . "\n";
>  				my $ln = $linenr - 1;
> @@ -2406,7 +2419,7 @@ sub process {
>                                              $allowed = 1;
>  					}
>  				}
> -				if ($seen != ($#chunks + 1)) {
> +				if ($seen != ($#chunks + 1) && !$allowed) {
>  					ERROR("braces {} are necessary for all arms of this statement\n" . $herectx);
>  				}
>  			}
> --
> 2.7.4
> 

Re: [Qemu-devel] [PATCH v2] scripts/checkpatch.pl: Bug fix

[Su Hang]

Thanks for your reply!
I'm glad to understand where problem lies.

Su Hang

> -----Original Messages-----
> From: "Fam Zheng" <famz@redhat.com>
> Sent Time: 2018-03-16 15:08:19 (Friday)
> To: "Su Hang" <suhang16@mails.ucas.ac.cn>
> Cc: peter.maydell@linaro.org, vsementsov@virtuozzo.com, qemu-devel@nongnu.org
> Subject: Re: [Qemu-devel] [PATCH v2] scripts/checkpatch.pl: Bug fix
> 
> On Fri, 03/16 14:31, Su Hang wrote:
> > Because I generate my first patch on master, it should work.
> 
> Nope. "git am" doesn't know to resolve the conflict automatically despite it
> being straightforward, unfortunately:
> 
> http://patchew.org/QEMU/1521169105-32041-1-git-send-email-suhang16@mails.ucas.ac.cn/
> 
> Fam

Re: [Qemu-devel] [PATCH v2] scripts/checkpatch.pl: Bug fix

[Fam Zheng]

On Fri, 03/16 14:31, Su Hang wrote:
> Because I generate my first patch on master, it should work.

Nope. "git am" doesn't know to resolve the conflict automatically despite it
being straightforward, unfortunately:

http://patchew.org/QEMU/1521169105-32041-1-git-send-email-suhang16@mails.ucas.ac.cn/

Fam

Re: [Qemu-devel] [PATCH v2] scripts/checkpatch.pl: Bug fix

[Su Hang]

Because I generate my first patch on master, it should work.

I have a little trouble understanding what Mr.Peter wants me to do,
would you please spare a little of your time and have a look at it?
It will help me. Please...

Su Hang


> -----Original Messages-----
> From: "Fam Zheng" <famz@redhat.com>
> Sent Time: 2018-03-16 14:15:20 (Friday)
> To: "Su Hang" <suhang16@mails.ucas.ac.cn>
> Cc: peter.maydell@linaro.org, eblake@redhat.com, vsementsov@virtuozzo.com, qemu-devel@nongnu.org
> Subject: Re: [Qemu-devel]  [PATCH v2] scripts/checkpatch.pl: Bug fix
> 
> On Fri, 03/16 10:58, Su Hang wrote:
> > (Sorry, I don't know this patch should base on which commit,
> >  so I generate this patch based on
> >  commit:fb8446d94ec7a3dc0c3a7e7da672406476f075ac,
> >  I choose this by `git log -2 scripts/checkpath.pl`.
> >  Sincerely say sorry, if I have misunderstand any meaning.)
> 
> This should base on master. Basing on an older commit will only make it hard for
> maintainers to apply it.
> 
> (Don't be sorry at all, thank you for spending your time improving QEMU!)
> 
> Fam

Re: [Qemu-devel] [PATCH v2] scripts/checkpatch.pl: Bug fix

[Fam Zheng]

On Fri, 03/16 10:58, Su Hang wrote:
> (Sorry, I don't know this patch should base on which commit,
>  so I generate this patch based on
>  commit:fb8446d94ec7a3dc0c3a7e7da672406476f075ac,
>  I choose this by `git log -2 scripts/checkpath.pl`.
>  Sincerely say sorry, if I have misunderstand any meaning.)

This should base on master. Basing on an older commit will only make it hard for
maintainers to apply it.

(Don't be sorry at all, thank you for spending your time improving QEMU!)

Fam

[Qemu-devel] [PATCH v2] scripts/checkpatch.pl: Bug fix

[Su Hang]

checkpatch.pl stops complaining about following pattern:
"""
do {
    //do somethins;
} while (conditions);
"""

One things need to be mentioned:
Becasue `if`, `while` and `for` check have been done in this
`if` block(Line: 2356), and this block contains following statement:
""" Line: 2379
$suppress_ifbraces{$ln + $offset} = 1;
"""
So the behind block may never run:
""" Line: 2415
if (!defined $suppress_ifbraces{$linenr - 1} &&
    $line =~ /\b(if|while|for|else)\b/ &&
    $line !~ /\#\s*if/ &&
    $line !~ /\#\s*else/) {
"""
I'm not sure, please give me some advice.

(Sorry, I don't know this patch should base on which commit,
 so I generate this patch based on
 commit:fb8446d94ec7a3dc0c3a7e7da672406476f075ac,
 I choose this by `git log -2 scripts/checkpath.pl`.
 Sincerely say sorry, if I have misunderstand any meaning.)

Signed-off-by: Su Hang <suhang16@mails.ucas.ac.cn>
---
 scripts/checkpatch.pl | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index a88af61ed4ee..d6f0747ba20a 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2352,8 +2352,22 @@ sub process {
 			}
 		}

-# check for missing bracing round if etc
-		if ($line =~ /(^.*)\bif\b/ && $line !~ /\#\s*if/) {
+# check for missing bracing around if etc
+		if ($line =~ /(^.*)\b(?:if|while|for)\b/ &&
+			$line !~ /\#\s*if/) {
+			my $allowed = 0;
+
+			# Check the pre-context.
+			if ($line =~ /(\}.*?)$/) {
+				my $pre = $1;
+
+				if ($line !~ /else/) {
+					print "APW: ALLOWED: pre<$pre> line<$line>\n"
+						if $dbg_adv_apw;
+					$allowed = 1;
+				}
+			}
+
 			my ($level, $endln, @chunks) =
 				ctx_statement_full($linenr, $realcnt, 1);
                         if ($dbg_adv_apw) {
@@ -2362,7 +2376,6 @@ sub process {
                                 if $#chunks >= 1;
                         }
 			if ($#chunks >= 0 && $level == 0) {
-				my $allowed = 0;
 				my $seen = 0;
 				my $herectx = $here . "\n";
 				my $ln = $linenr - 1;
@@ -2406,7 +2419,7 @@ sub process {
                                             $allowed = 1;
 					}
 				}
-				if ($seen != ($#chunks + 1)) {
+				if ($seen != ($#chunks + 1) && !$allowed) {
 					ERROR("braces {} are necessary for all arms of this statement\n" . $herectx);
 				}
 			}
--
2.7.4