diff mbox

[2/3] rng-egd: Free old chr_name value before setting new one

Message ID 1401480140-18653-3-git-send-email-ehabkost@redhat.com
State New
Headers show

Commit Message

Eduardo Habkost May 30, 2014, 8:02 p.m. UTC 
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Cc: Anthony Liguori <aliguori@amazon.com>
Cc: Amos Kong <akong@redhat.com>
---
 backends/rng-egd.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Markus Armbruster June 2, 2014, 12:58 p.m. UTC | #1 
Eduardo Habkost <ehabkost@redhat.com> writes:

> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> Cc: Anthony Liguori <aliguori@amazon.com>
> Cc: Amos Kong <akong@redhat.com>
> ---
>  backends/rng-egd.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/backends/rng-egd.c b/backends/rng-egd.c
> index 25bb3b4..2962795 100644
> --- a/backends/rng-egd.c
> +++ b/backends/rng-egd.c
> @@ -169,6 +169,7 @@ static void rng_egd_set_chardev(Object *obj, const char *value, Error **errp)
>      if (b->opened) {
>          error_set(errp, QERR_PERMISSION_DENIED);
>      } else {
> +        g_free(s->chr_name);
>          s->chr_name = g_strdup(value);
>      }
>  }

I see you searched for the bug elsewhere.  Appreciated.  Do you think
you got them all?

Aside: I hate

    if (bad) {
        handle error
    } else {
        continue normal work
    }

The normal flow gets nested deeper and deeper.  When the code grows,
normal work can easily end up after the conditional by mistake.  Better:

    if (bad) {
        handle error
        return
    }
    continue normal work

Anyway, not your fault.

Reviewed-by: Markus Armbruster <armbru@redhat.com>
Eduardo Habkost June 2, 2014, 2:50 p.m. UTC | #2 
On Mon, Jun 02, 2014 at 02:58:05PM +0200, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> > Cc: Anthony Liguori <aliguori@amazon.com>
> > Cc: Amos Kong <akong@redhat.com>
> > ---
> >  backends/rng-egd.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/backends/rng-egd.c b/backends/rng-egd.c
> > index 25bb3b4..2962795 100644
> > --- a/backends/rng-egd.c
> > +++ b/backends/rng-egd.c
> > @@ -169,6 +169,7 @@ static void rng_egd_set_chardev(Object *obj, const char *value, Error **errp)
> >      if (b->opened) {
> >          error_set(errp, QERR_PERMISSION_DENIED);
> >      } else {
> > +        g_free(s->chr_name);
> >          s->chr_name = g_strdup(value);
> >      }
> >  }
> 
> I see you searched for the bug elsewhere.  Appreciated.  Do you think
> you got them all?

I grepped for all object_property_add_str occurrences, and (unless I
made a mistake) I fixed all of them.
diff mbox

Patch

diff --git a/backends/rng-egd.c b/backends/rng-egd.c
index 25bb3b4..2962795 100644
--- a/backends/rng-egd.c
+++ b/backends/rng-egd.c
@@ -169,6 +169,7 @@  static void rng_egd_set_chardev(Object *obj, const char *value, Error **errp)
     if (b->opened) {
         error_set(errp, QERR_PERMISSION_DENIED);
     } else {
+        g_free(s->chr_name);
         s->chr_name = g_strdup(value);
     }
 }