diff mbox

[v2,01/13] Use unsigned types for the 'len' argument of all memory read/write functions

Message ID 1456847859-4771-1-git-send-email-martin.galvan@tallertechnologies.com
State New
Headers show

Commit Message

Martin Galvan March 1, 2016, 3:57 p.m. UTC 
---
 cpus.c                    |  2 +-
 exec.c                    | 29 +++++++++++++++--------------
 include/exec/cpu-all.h    |  2 +-
 include/exec/cpu-common.h | 10 +++++-----
 include/exec/memory.h     | 10 +++++-----
 kvm-all.c                 |  2 +-
 6 files changed, 28 insertions(+), 27 deletions(-)

--
1.9.1

Comments

Eric Blake March 1, 2016, 4:05 p.m. UTC | #1 
On 03/01/2016 08:57 AM, Martin Galvan wrote:

Missing a S-o-b line; we cannot accept patches unless you sign them:
http://wiki.qemu.org/Contribute/SubmitAPatch
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=f6f94e2a#n297

Subject line is long, and lacking a 'topic: summary' prefix.  I'd suggest:

mem: use unsigned 'len' for read/write

Then give more details, if needed, in the body of the commit message.

Also, your threading didn't work right.  Patch 1/13 was sent with:

References:
<1456847859-4771-1-git-send-email-martin.galvan@tallertechnologies.com>

but with no In-Reply-To:. Meanwhile, the 00/13 cover letter was sent with:

Message-Id:
<1456847681-4532-1-git-send-email-martin.galvan@tallertechnologies.com>

which is subtly different from the references of all the other patches,
making it appear as separate threads.

> ---
>  cpus.c                    |  2 +-
>  exec.c                    | 29 +++++++++++++++--------------
>  include/exec/cpu-all.h    |  2 +-
>  include/exec/cpu-common.h | 10 +++++-----
>  include/exec/memory.h     | 10 +++++-----
>  kvm-all.c                 |  2 +-
>  6 files changed, 28 insertions(+), 27 deletions(-)
> 
> diff --git a/cpus.c b/cpus.c
> index 9592163..e7aa8cc 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -1602,7 +1602,7 @@ void qmp_memsave(int64_t addr, int64_t size, const char *filename,
>                   bool has_cpu, int64_t cpu_index, Error **errp)
>  {
>      FILE *f;
> -    uint32_t l;
> +    size_t l;

These are both unsigned types.  So based on the subject line alone, this
doesn't fit in the patch, unless the commit message body goes into more
details on why you are changing the size of the type, and not just the
signedness.
Martin Galvan March 1, 2016, 5:56 p.m. UTC | #2 
On Tue, Mar 1, 2016 at 1:05 PM, Eric Blake <eblake@redhat.com> wrote:
> On 03/01/2016 08:57 AM, Martin Galvan wrote:
>
> Missing a S-o-b line; we cannot accept patches unless you sign them:
> http://wiki.qemu.org/Contribute/SubmitAPatch
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=f6f94e2a#n297

Oh, I forgot that. I did have a s-o-b in v1, I just forgot to add it here.

> Subject line is long, and lacking a 'topic: summary' prefix.  I'd suggest:
>
> mem: use unsigned 'len' for read/write
>
> Then give more details, if needed, in the body of the commit message.

Yeah, I thought it was too long myself. Will do.

> Also, your threading didn't work right.  Patch 1/13 was sent with:
>
> References:
> <1456847859-4771-1-git-send-email-martin.galvan@tallertechnologies.com>
>
> but with no In-Reply-To:. Meanwhile, the 00/13 cover letter was sent with:
>
> Message-Id:
> <1456847681-4532-1-git-send-email-martin.galvan@tallertechnologies.com>
>
> which is subtly different from the references of all the other patches,
> making it appear as separate threads.

Yeah, I think I messed that up. Sorry.

>>      FILE *f;
>> -    uint32_t l;
>> +    size_t l;
>
> These are both unsigned types.  So based on the subject line alone, this
> doesn't fit in the patch, unless the commit message body goes into more
> details on why you are changing the size of the type, and not just the
> signedness.

You're right, I recall changing that for size correctness, rather than
signedness. I'll mention in the commit message that I also fixed those
issues in a couple of places.

> (adding an appropriate prefix for each maintainer that you are targetting will help).

I actually thought of that, but the files grouped into each e-mail
weren't necessarily related (e.g. Paolo's include some stuff in exec/
and kvm-all.c). I'll see what I can do, though.

Before I re-send them, however, it'd be nice if the patches themselves
could be reviewed.
diff mbox

Patch

diff --git a/cpus.c b/cpus.c
index 9592163..e7aa8cc 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1602,7 +1602,7 @@  void qmp_memsave(int64_t addr, int64_t size, const char *filename,
                  bool has_cpu, int64_t cpu_index, Error **errp)
 {
     FILE *f;
-    uint32_t l;
+    size_t l;
     CPUState *cpu;
     uint8_t buf[1024];
     int64_t orig_addr = addr, orig_size = size;
diff --git a/exec.c b/exec.c
index c62c439..b5c26d6 100644
--- a/exec.c
+++ b/exec.c
@@ -2458,9 +2458,10 @@  MemoryRegion *get_system_io(void)
 /* physical memory access (slow version, mainly for debug) */
 #if defined(CONFIG_USER_ONLY)
 int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
-                        uint8_t *buf, int len, int is_write)
+                        uint8_t *buf, size_t len, int is_write)
 {
-    int l, flags;
+    size_t l;
+    int flags;
     target_ulong page;
     void * p;

@@ -2517,7 +2518,7 @@  static void invalidate_and_set_dirty(MemoryRegion *mr, hwaddr addr,
     cpu_physical_memory_set_dirty_range(addr, length, dirty_log_mask);
 }

-static int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr)
+static hwaddr memory_access_size(MemoryRegion *mr, size_t l, hwaddr addr)
 {
     unsigned access_size_max = mr->ops->valid.max_access_size;

@@ -2571,7 +2572,7 @@  static bool prepare_mmio_access(MemoryRegion *mr)
 static MemTxResult address_space_write_continue(AddressSpace *as, hwaddr addr,
                                                 MemTxAttrs attrs,
                                                 const uint8_t *buf,
-                                                int len, hwaddr addr1,
+                                                size_t len, hwaddr addr1,
                                                 hwaddr l, MemoryRegion *mr)
 {
     uint8_t *ptr;
@@ -2642,7 +2643,7 @@  static MemTxResult address_space_write_continue(AddressSpace *as, hwaddr addr,
 }

 MemTxResult address_space_write(AddressSpace *as, hwaddr addr, MemTxAttrs attrs,
-                                const uint8_t *buf, int len)
+                                const uint8_t *buf, size_t len)
 {
     hwaddr l;
     hwaddr addr1;
@@ -2664,7 +2665,7 @@  MemTxResult address_space_write(AddressSpace *as, hwaddr addr, MemTxAttrs attrs,
 /* Called within RCU critical section.  */
 MemTxResult address_space_read_continue(AddressSpace *as, hwaddr addr,
                                         MemTxAttrs attrs, uint8_t *buf,
-                                        int len, hwaddr addr1, hwaddr l,
+                                        size_t len, hwaddr addr1, hwaddr l,
                                         MemoryRegion *mr)
 {
     uint8_t *ptr;
@@ -2732,7 +2733,7 @@  MemTxResult address_space_read_continue(AddressSpace *as, hwaddr addr,
 }

 MemTxResult address_space_read_full(AddressSpace *as, hwaddr addr,
-                                    MemTxAttrs attrs, uint8_t *buf, int len)
+                                    MemTxAttrs attrs, uint8_t *buf, size_t len)
 {
     hwaddr l;
     hwaddr addr1;
@@ -2752,7 +2753,7 @@  MemTxResult address_space_read_full(AddressSpace *as, hwaddr addr,
 }

 MemTxResult address_space_rw(AddressSpace *as, hwaddr addr, MemTxAttrs attrs,
-                             uint8_t *buf, int len, bool is_write)
+                             uint8_t *buf, size_t len, bool is_write)
 {
     if (is_write) {
         return address_space_write(as, addr, attrs, (uint8_t *)buf, len);
@@ -2762,7 +2763,7 @@  MemTxResult address_space_rw(AddressSpace *as, hwaddr addr, MemTxAttrs attrs,
 }

 void cpu_physical_memory_rw(hwaddr addr, uint8_t *buf,
-                            int len, int is_write)
+                            size_t len, int is_write)
 {
     address_space_rw(&address_space_memory, addr, MEMTXATTRS_UNSPECIFIED,
                      buf, len, is_write);
@@ -2812,12 +2813,12 @@  static inline void cpu_physical_memory_write_rom_internal(AddressSpace *as,

 /* used for ROM loading : can write in RAM and ROM */
 void cpu_physical_memory_write_rom(AddressSpace *as, hwaddr addr,
-                                   const uint8_t *buf, int len)
+                                   const uint8_t *buf, size_t len)
 {
     cpu_physical_memory_write_rom_internal(as, addr, buf, len, WRITE_DATA);
 }

-void cpu_flush_icache_range(hwaddr start, int len)
+void cpu_flush_icache_range(hwaddr star t, size_t len)
 {
     /*
      * This function should do the same thing as an icache flush that was
@@ -3661,14 +3662,14 @@  void stq_be_phys(AddressSpace *as, hwaddr addr, uint64_t val)

 /* virtual memory access for debug (includes writing to ROM) */
 int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
-                        uint8_t *buf, int len, int is_write)
+                        uint8_t *buf, size_t len, int is_write)
 {
-    int l;
+    size_t l;
     hwaddr phys_addr;
     target_ulong page;

     while (len > 0) {
-        int asidx;
+        hwaddr asidx;
         MemTxAttrs attrs;

         page = addr & TARGET_PAGE_MASK;
diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
index 08e5093..c824d5f 100644
--- a/include/exec/cpu-all.h
+++ b/include/exec/cpu-all.h
@@ -282,6 +282,6 @@  void dump_opcount_info(FILE *f, fprintf_function cpu_fprintf);
 #endif /* !CONFIG_USER_ONLY */

 int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
-                        uint8_t *buf, int len, int is_write);
+                        uint8_t *buf, size_t len, int is_write);

 #endif /* CPU_ALL_H */
diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
index a0ad2ac..f413387 100644
--- a/include/exec/cpu-common.h
+++ b/include/exec/cpu-common.h
@@ -71,14 +71,14 @@  void qemu_ram_unset_idstr(ram_addr_t addr);
 const char *qemu_ram_get_idstr(RAMBlock *rb);

 void cpu_physical_memory_rw(hwaddr addr, uint8_t *buf,
-                            int len, int is_write);
+                            size_t len, int is_write);
 static inline void cpu_physical_memory_read(hwaddr addr,
-                                            void *buf, int len)
+                                            void *buf, size_t len)
 {
     cpu_physical_memory_rw(addr, buf, len, 0);
 }
 static inline void cpu_physical_memory_write(hwaddr addr,
-                                             const void *buf, int len)
+                                             const void *buf, size_t len)
 {
     cpu_physical_memory_rw(addr, (void *)buf, len, 1);
 }
@@ -125,8 +125,8 @@  void stq_phys(AddressSpace *as, hwaddr addr, uint64_t val);
 #endif

 void cpu_physical_memory_write_rom(AddressSpace *as, hwaddr addr,
-                                   const uint8_t *buf, int len);
-void cpu_flush_icache_range(hwaddr start, int len);
+                                   const uint8_t *buf, size_t len);
+void cpu_flush_icache_range(hwaddr start, size_t len);

 extern struct MemoryRegion io_mem_rom;
 extern struct MemoryRegion io_mem_notdirty;
diff --git a/include/exec/memory.h b/include/exec/memory.h
index d5284c2..a4d34f1 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -1233,7 +1233,7 @@  void address_space_destroy(AddressSpace *as);
  */
 MemTxResult address_space_rw(AddressSpace *as, hwaddr addr,
                              MemTxAttrs attrs, uint8_t *buf,
-                             int len, bool is_write);
+                             size_t len, bool is_write);

 /**
  * address_space_write: write to address space.
@@ -1249,7 +1249,7 @@  MemTxResult address_space_rw(AddressSpace *as, hwaddr addr,
  */
 MemTxResult address_space_write(AddressSpace *as, hwaddr addr,
                                 MemTxAttrs attrs,
-                                const uint8_t *buf, int len);
+                                const uint8_t *buf, size_t len);

 /* address_space_ld*: load from an address space
  * address_space_st*: store to an address space
@@ -1384,10 +1384,10 @@  void address_space_unmap(AddressSpace *as, void *buffer, hwaddr len,
 /* Internal functions, part of the implementation of address_space_read.  */
 MemTxResult address_space_read_continue(AddressSpace *as, hwaddr addr,
                                         MemTxAttrs attrs, uint8_t *buf,
-                                        int len, hwaddr addr1, hwaddr l,
+                                        size_t len, hwaddr addr1, size_t l,
 					MemoryRegion *mr);
 MemTxResult address_space_read_full(AddressSpace *as, hwaddr addr,
-                                    MemTxAttrs attrs, uint8_t *buf, int len);
+                                    MemTxAttrs attrs, uint8_t *buf, size_t len);
 void *qemu_get_ram_ptr(RAMBlock *ram_block, ram_addr_t addr);

 static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
@@ -1413,7 +1413,7 @@  static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
  */
 static inline __attribute__((__always_inline__))
 MemTxResult address_space_read(AddressSpace *as, hwaddr addr, MemTxAttrs attrs,
-                               uint8_t *buf, int len)
+                               uint8_t *buf, size_t len)
 {
     MemTxResult result = MEMTX_OK;
     hwaddr l, addr1;
diff --git a/kvm-all.c b/kvm-all.c
index a65e73f..14098b6 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -1703,7 +1703,7 @@  void kvm_set_sigmask_len(KVMState *s, unsigned int sigmask_len)
 }

 static void kvm_handle_io(uint16_t port, MemTxAttrs attrs, void *data, int direction,
-                          int size, uint32_t count)
+                          size_t size, uint32_t count)
 {
     int i;
     uint8_t *ptr = data;