[Qemu-devel] [PATCH 0/6] Trivial cleanups around g_malloc()

[Qemu-devel] [PATCH 0/6] Trivial cleanups around g_malloc()

[Markus Armbruster]

I'm routing these patches through qemu-trivial, even though some of
them touch actively maintained code, and could go through the
respective tree instead:

* PATCH 1 block (Kevin, Stefan)

* PATCH 3 KVM (Paolo)

* PATCH 4 migration (Juan, Amit)

* PATCH 5 VNC (Gerd)

If you want me to reroute any of them, let me know.

Markus Armbruster (6):
  onenand: g_malloc() can't fail, bury dead error handling
  rtl8139: g_malloc() can't fail, bury dead error handling
  kvm: g_malloc() can't fail, bury dead error handling
  rdma: g_malloc0() can't fail, bury dead error handling
  vnc: g_realloc() can't fail, bury dead error handling
  translate-all: Use g_try_malloc() for dynamic translator buffer

 hw/block/onenand.c |  8 +-------
 hw/net/rtl8139.c   | 14 --------------
 kvm-all.c          |  4 ----
 migration/rdma.c   |  3 ---
 translate-all.c    |  2 +-
 ui/vnc.c           |  4 ----
 6 files changed, 2 insertions(+), 33 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH 1/6] onenand: g_malloc() can't fail, bury dead error handling

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/block/onenand.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/hw/block/onenand.c b/hw/block/onenand.c
index 348630d..1b2c893 100644
--- a/hw/block/onenand.c
+++ b/hw/block/onenand.c
@@ -346,15 +346,9 @@ static inline int onenand_prog_spare(OneNANDState *s, int sec, int secn,
 static inline int onenand_erase(OneNANDState *s, int sec, int num)
 {
     uint8_t *blankbuf, *tmpbuf;
+
     blankbuf = g_malloc(512);
-    if (!blankbuf) {
-        return 1;
-    }
     tmpbuf = g_malloc(512);
-    if (!tmpbuf) {
-        g_free(blankbuf);
-        return 1;
-    }
     memset(blankbuf, 0xff, 512);
     for (; num > 0; num--, sec++) {
         if (s->blk_cur) {
-- 
1.9.3

[Qemu-devel] [PATCH 2/6] rtl8139: g_malloc() can't fail, bury dead error handling

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/net/rtl8139.c | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
index 6fa9e0a..b55e438 100644
--- a/hw/net/rtl8139.c
+++ b/hw/net/rtl8139.c
@@ -2075,20 +2075,6 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
                 "length to %d\n", txsize);
     }
 
-    if (!s->cplus_txbuffer)
-    {
-        /* out of memory */
-
-        DPRINTF("+++ C+ mode transmiter failed to reallocate %d bytes\n",
-            s->cplus_txbuffer_len);
-
-        /* update tally counter */
-        ++s->tally_counters.TxERR;
-        ++s->tally_counters.TxAbt;
-
-        return 0;
-    }
-
     /* append more data to the packet */
 
     DPRINTF("+++ C+ mode transmit reading %d bytes from host memory at "
-- 
1.9.3

[Qemu-devel] [PATCH 3/6] kvm: g_malloc() can't fail, bury dead error handling

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 kvm-all.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/kvm-all.c b/kvm-all.c
index 2f21a4e..05a79c2 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -2070,10 +2070,6 @@ int kvm_insert_breakpoint(CPUState *cpu, target_ulong addr,
         }
 
         bp = g_malloc(sizeof(struct kvm_sw_breakpoint));
-        if (!bp) {
-            return -ENOMEM;
-        }
-
         bp->pc = addr;
         bp->use_count = 1;
         err = kvm_arch_insert_sw_breakpoint(cpu, bp);
-- 
1.9.3

[Qemu-devel] [PATCH 4/6] rdma: g_malloc0() can't fail, bury dead error handling

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 migration/rdma.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/migration/rdma.c b/migration/rdma.c
index b32dbdf..306570d 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -1147,9 +1147,6 @@ static int qemu_rdma_register_and_get_keys(RDMAContext *rdma,
     /* allocate memory to store chunk MRs */
     if (!block->pmr) {
         block->pmr = g_malloc0(block->nb_chunks * sizeof(struct ibv_mr *));
-        if (!block->pmr) {
-            return -1;
-        }
     }
 
     /*
-- 
1.9.3

[Qemu-devel] [PATCH 5/6] vnc: g_realloc() can't fail, bury dead error handling

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 ui/vnc.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/ui/vnc.c b/ui/vnc.c
index a742c90..02552ee 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -659,10 +659,6 @@ void buffer_reserve(Buffer *buffer, size_t len)
     if ((buffer->capacity - buffer->offset) < len) {
         buffer->capacity += (len + 1024);
         buffer->buffer = g_realloc(buffer->buffer, buffer->capacity);
-        if (buffer->buffer == NULL) {
-            fprintf(stderr, "vnc: out of memory\n");
-            exit(1);
-        }
     }
 }
 
-- 
1.9.3

[Qemu-devel] [PATCH 6/6] translate-all: Use g_try_malloc() for dynamic translator buffer

[Markus Armbruster]

The USE_MMAP code can fail, and the caller handles the failure
already.  Let the !USE_MMAP code fail as well, for consistency.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 translate-all.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/translate-all.c b/translate-all.c
index 4a1b64f..9f47ce7 100644
--- a/translate-all.c
+++ b/translate-all.c
@@ -631,7 +631,7 @@ static inline void *alloc_code_gen_buffer(void)
 #else
 static inline void *alloc_code_gen_buffer(void)
 {
-    void *buf = g_malloc(tcg_ctx.code_gen_buffer_size);
+    void *buf = g_try_malloc(tcg_ctx.code_gen_buffer_size);
 
     if (buf == NULL) {
         return NULL;
-- 
1.9.3

Re: [Qemu-devel] [PATCH 2/6] rtl8139: g_malloc() can't fail, bury dead error handling

[Thomas Huth]

On Tue, 27 Jan 2015 17:38:27 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/net/rtl8139.c | 14 --------------
>  1 file changed, 14 deletions(-)
> 
> diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
> index 6fa9e0a..b55e438 100644
> --- a/hw/net/rtl8139.c
> +++ b/hw/net/rtl8139.c
> @@ -2075,20 +2075,6 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
>                  "length to %d\n", txsize);
>      }
> 
> -    if (!s->cplus_txbuffer)
> -    {
> -        /* out of memory */
> -
> -        DPRINTF("+++ C+ mode transmiter failed to reallocate %d bytes\n",
> -            s->cplus_txbuffer_len);
> -
> -        /* update tally counter */
> -        ++s->tally_counters.TxERR;
> -        ++s->tally_counters.TxAbt;
> -
> -        return 0;
> -    }
> -
>      /* append more data to the packet */
> 
>      DPRINTF("+++ C+ mode transmit reading %d bytes from host memory at "

Wouldn't it be better to use g_try_malloc() here instead? If the code
can handle OOM conditions, I think it's better to continue with a lost
packet here than to shut down QEMU the hard way.
(Also looking at the history of that file, the code originally used
qemu_malloc() which could fail - but instead of being replaced by
g_try_malloc(), it got replaced with g_malloc() instead which was
maybe the wrong decision).

 Thomas

Re: [Qemu-devel] [PATCH 5/6] vnc: g_realloc() can't fail, bury dead error handling

[Thomas Huth]

On Tue, 27 Jan 2015 17:38:30 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  ui/vnc.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/ui/vnc.c b/ui/vnc.c
> index a742c90..02552ee 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -659,10 +659,6 @@ void buffer_reserve(Buffer *buffer, size_t len)
>      if ((buffer->capacity - buffer->offset) < len) {
>          buffer->capacity += (len + 1024);
>          buffer->buffer = g_realloc(buffer->buffer, buffer->capacity);
> -        if (buffer->buffer == NULL) {
> -            fprintf(stderr, "vnc: out of memory\n");
> -            exit(1);
> -        }
>      }
>  }
> 

Reviewed-by: Thomas Huth <thuth@linux.vnet.ibm.com>

Re: [Qemu-devel] [PATCH 0/6] Trivial cleanups around g_malloc()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 758 bytes --]

[your mail used the wrong address for qemu-trivial; I'm redirecting my
reply]

On 01/27/2015 09:38 AM, Markus Armbruster wrote:
> I'm routing these patches through qemu-trivial, even though some of
> them touch actively maintained code, and could go through the
> respective tree instead:
> 
> * PATCH 1 block (Kevin, Stefan)
> 
> * PATCH 3 KVM (Paolo)
> 
> * PATCH 4 migration (Juan, Amit)
> 
> * PATCH 5 VNC (Gerd)
> 
> If you want me to reroute any of them, let me know.

For all 6:
Reviewed-by: Eric Blake <eblake@redhat.com>

Patch 2 was the hardest to review (the allocation occurs in context not
shown in the patch).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 3/6] kvm: g_malloc() can't fail, bury dead error handling

[Thomas Huth]

On Tue, 27 Jan 2015 17:38:28 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  kvm-all.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/kvm-all.c b/kvm-all.c
> index 2f21a4e..05a79c2 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -2070,10 +2070,6 @@ int kvm_insert_breakpoint(CPUState *cpu, target_ulong addr,
>          }
> 
>          bp = g_malloc(sizeof(struct kvm_sw_breakpoint));
> -        if (!bp) {
> -            return -ENOMEM;
> -        }
> -
>          bp->pc = addr;
>          bp->use_count = 1;
>          err = kvm_arch_insert_sw_breakpoint(cpu, bp);

I think I'd also use g_try_malloc() here instead. Looks like an error
gets reported to GDB when this function returns with an error code, so
returning -ENOMEM should be ok here, shouldn't it?

 Thomas

Re: [Qemu-devel] [PATCH 2/6] rtl8139: g_malloc() can't fail, bury dead error handling

[Markus Armbruster]

Thomas Huth <thuth@linux.vnet.ibm.com> writes:

> On Tue, 27 Jan 2015 17:38:27 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  hw/net/rtl8139.c | 14 --------------
>>  1 file changed, 14 deletions(-)
>> 
>> diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
>> index 6fa9e0a..b55e438 100644
>> --- a/hw/net/rtl8139.c
>> +++ b/hw/net/rtl8139.c
>> @@ -2075,20 +2075,6 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
>>                  "length to %d\n", txsize);
>>      }
>> 
>> -    if (!s->cplus_txbuffer)
>> -    {
>> -        /* out of memory */
>> -
>> -        DPRINTF("+++ C+ mode transmiter failed to reallocate %d bytes\n",
>> -            s->cplus_txbuffer_len);
>> -
>> -        /* update tally counter */
>> -        ++s->tally_counters.TxERR;
>> -        ++s->tally_counters.TxAbt;
>> -
>> -        return 0;
>> -    }
>> -
>>      /* append more data to the packet */
>> 
>>      DPRINTF("+++ C+ mode transmit reading %d bytes from host memory at "
>
> Wouldn't it be better to use g_try_malloc() here instead? If the code
> can handle OOM conditions, I think it's better to continue with a lost
> packet here than to shut down QEMU the hard way.
> (Also looking at the history of that file, the code originally used
> qemu_malloc() which could fail - but instead of being replaced by
> g_try_malloc(), it got replaced with g_malloc() instead which was
> maybe the wrong decision).

When allocating 64KiB[*] fails, then you're a dead process walking.

By checking for and recovering from such allocation failures, you can
try to limp on for a bit.  If your programmers have been perfectly
diligent, you'll limp on until you die in a non-recoverable allocation
failure.  More likely, your programmers have been human, and you'll die
an ugly death after some unchecked allocation or in some untested
recovery that doesn't actually work.

Our strategy is to use allocations that need checking only for large
sizes and where recovery is possible without crippling loss of
functionality.  Those are rare.  Gives us a chance to actually test
their recovery.

There's ample prededence for ditching untested allocation error recovery
in favour of simply terminating the process.

If you think recovery is worthwhile here, post a patch.  Ideally
extending tests/rtl8139-test.c to cover the error recovery.


[*] That's the value of s->cplus_txbuffer_len (apparent once you peel
off the obfuscation).

Re: [Qemu-devel] [PATCH 3/6] kvm: g_malloc() can't fail, bury dead error handling

[Markus Armbruster]

Thomas Huth <thuth@linux.vnet.ibm.com> writes:

> On Tue, 27 Jan 2015 17:38:28 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  kvm-all.c | 4 ----
>>  1 file changed, 4 deletions(-)
>> 
>> diff --git a/kvm-all.c b/kvm-all.c
>> index 2f21a4e..05a79c2 100644
>> --- a/kvm-all.c
>> +++ b/kvm-all.c
>> @@ -2070,10 +2070,6 @@ int kvm_insert_breakpoint(CPUState *cpu, target_ulong addr,
>>          }
>> 
>>          bp = g_malloc(sizeof(struct kvm_sw_breakpoint));
>> -        if (!bp) {
>> -            return -ENOMEM;
>> -        }
>> -
>>          bp->pc = addr;
>>          bp->use_count = 1;
>>          err = kvm_arch_insert_sw_breakpoint(cpu, bp);
>
> I think I'd also use g_try_malloc() here instead. Looks like an error
> gets reported to GDB when this function returns with an error code, so
> returning -ENOMEM should be ok here, shouldn't it?

Same rationale as for PATCH 2.  sizeof(struct kvm_sw_breakpoint) is 40
bytes on my system.

Re: [Qemu-devel] [PATCH 0/6] Trivial cleanups around g_malloc()

[Markus Armbruster]

I screwed up cc: qemu-trivial, my apologies.

Markus Armbruster <armbru@redhat.com> writes:

> I'm routing these patches through qemu-trivial, even though some of
> them touch actively maintained code, and could go through the
> respective tree instead:
>
> * PATCH 1 block (Kevin, Stefan)
>
> * PATCH 3 KVM (Paolo)
>
> * PATCH 4 migration (Juan, Amit)
>
> * PATCH 5 VNC (Gerd)
>
> If you want me to reroute any of them, let me know.
>
> Markus Armbruster (6):
>   onenand: g_malloc() can't fail, bury dead error handling
>   rtl8139: g_malloc() can't fail, bury dead error handling
>   kvm: g_malloc() can't fail, bury dead error handling
>   rdma: g_malloc0() can't fail, bury dead error handling
>   vnc: g_realloc() can't fail, bury dead error handling
>   translate-all: Use g_try_malloc() for dynamic translator buffer
>
>  hw/block/onenand.c |  8 +-------
>  hw/net/rtl8139.c   | 14 --------------
>  kvm-all.c          |  4 ----
>  migration/rdma.c   |  3 ---
>  translate-all.c    |  2 +-
>  ui/vnc.c           |  4 ----
>  6 files changed, 2 insertions(+), 33 deletions(-)

Re: [Qemu-devel] [PATCH 0/6] Trivial cleanups around g_malloc()

[Gonglei]

On 2015/1/28 19:16, Markus Armbruster wrote:

> I screwed up cc: qemu-trivial, my apologies.
> 
> Markus Armbruster <armbru@redhat.com> writes:
> 
>> I'm routing these patches through qemu-trivial, even though some of
>> them touch actively maintained code, and could go through the
>> respective tree instead:
>>
>> * PATCH 1 block (Kevin, Stefan)
>>
>> * PATCH 3 KVM (Paolo)
>>
>> * PATCH 4 migration (Juan, Amit)
>>
>> * PATCH 5 VNC (Gerd)
>>
>> If you want me to reroute any of them, let me know.
>>
>> Markus Armbruster (6):
>>   onenand: g_malloc() can't fail, bury dead error handling
>>   rtl8139: g_malloc() can't fail, bury dead error handling
>>   kvm: g_malloc() can't fail, bury dead error handling
>>   rdma: g_malloc0() can't fail, bury dead error handling
>>   vnc: g_realloc() can't fail, bury dead error handling
>>   translate-all: Use g_try_malloc() for dynamic translator buffer
>>
>>  hw/block/onenand.c |  8 +-------
>>  hw/net/rtl8139.c   | 14 --------------
>>  kvm-all.c          |  4 ----
>>  migration/rdma.c   |  3 ---
>>  translate-all.c    |  2 +-
>>  ui/vnc.c           |  4 ----
>>  6 files changed, 2 insertions(+), 33 deletions(-)
> 

For serials:
Reviewed-by: Gonglei <arei.gonglei@huawei.com>

Regards,
-Gonglei

Re: [Qemu-devel] [PATCH 0/6] Trivial cleanups around g_malloc()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 820 bytes --]

On 01/28/2015 05:31 PM, Gonglei wrote:
> On 2015/1/28 19:16, Markus Armbruster wrote:
> 
>> I screwed up cc: qemu-trivial, my apologies.
>>

>>> Markus Armbruster (6):
>>>   onenand: g_malloc() can't fail, bury dead error handling
>>>   rtl8139: g_malloc() can't fail, bury dead error handling
>>>   kvm: g_malloc() can't fail, bury dead error handling
>>>   rdma: g_malloc0() can't fail, bury dead error handling
>>>   vnc: g_realloc() can't fail, bury dead error handling
>>>   translate-all: Use g_try_malloc() for dynamic translator buffer
>>>

>>
> 
> For serials:

s/serials/series/

> Reviewed-by: Gonglei <arei.gonglei@huawei.com>
> 
> Regards,
> -Gonglei
> 
> 
> 
> 
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 0/6] Trivial cleanups around g_malloc()

[Gonglei]

On 2015/1/29 10:21, Eric Blake wrote:

> On 01/28/2015 05:31 PM, Gonglei wrote:
>> On 2015/1/28 19:16, Markus Armbruster wrote:
>>
>>> I screwed up cc: qemu-trivial, my apologies.
>>>
> 
>>>> Markus Armbruster (6):
>>>>   onenand: g_malloc() can't fail, bury dead error handling
>>>>   rtl8139: g_malloc() can't fail, bury dead error handling
>>>>   kvm: g_malloc() can't fail, bury dead error handling
>>>>   rdma: g_malloc0() can't fail, bury dead error handling
>>>>   vnc: g_realloc() can't fail, bury dead error handling
>>>>   translate-all: Use g_try_malloc() for dynamic translator buffer
>>>>
> 
>>>
>>
>> For serials:
> 
> s/serials/series/

Yes, thanks :)

> 
>> Reviewed-by: Gonglei <arei.gonglei@huawei.com>
>>
>> Regards,
>> -Gonglei
>>
>>
>>
>>
>>
>