Hi Pekka, Thank you for your comment. You are right with your findings.
I will send a new patch to fix this issue. Best regards Emre Ucan Software Group I (ADITG/SW1) Tel. +49 5121 49 6937 > -----Original Message----- > From: Pekka Paalanen [mailto:ppaala...@gmail.com] > Sent: Mittwoch, 9. November 2016 11:32 > To: Yong Bakos; Ucan, Emre (ADITG/SW1) > Cc: wayland-devel > Subject: Re: [PATCH weston v2 3/3] ivi-shell: use zalloc instead of calloc > > On Thu, 28 Jul 2016 10:23:00 -0700 > Yong Bakos <j...@humanoriented.com> wrote: > > > Hi Emre, > > > > > On Jul 28, 2016, at 7:14 AM, Ucan, Emre (ADITG/SW1) <eu...@de.adit- > jv.com> wrote: > > > > > > v2 changes: > > > - use xzalloc > > > - add an explicit include of xalloc.h in any .c file > > > that uses xzalloc. > > > > > > Signed-off-by: Emre Ucan <eu...@de.adit-jv.com> > > > > Is the intent of this patch to cause an exit() when memory allocation fails? > > See my comments inline below. > > Hi, > > thanks for the comments, my replies below. > > > > --- > > > ivi-shell/hmi-controller.c | 35 ++++++++------------ > > > ivi-shell/input-panel-ivi.c | 6 ++-- > > > ivi-shell/ivi-layout-transition.c | 62 > > > +++++++---------------------------- > > > ivi-shell/ivi-layout.c | 65 > > > +++++++------------------------------ > > > 4 files changed, 38 insertions(+), 130 deletions(-) > > > > diff --git a/ivi-shell/input-panel-ivi.c b/ivi-shell/input-panel-ivi.c > > > index 581b56b..a563e31 100644 > > > --- a/ivi-shell/input-panel-ivi.c > > > +++ b/ivi-shell/input-panel-ivi.c > > > @@ -35,6 +35,7 @@ > > > #include "input-method-unstable-v1-server-protocol.h" > > > #include "ivi-layout-private.h" > > > #include "shared/helpers.h" > > > +#include "shared/xalloc.h" > > > > > > struct input_panel_surface { > > > struct wl_resource *resource; > > > @@ -236,10 +237,7 @@ create_input_panel_surface(struct ivi_shell > *shell, > > > { > > > struct input_panel_surface *input_panel_surface; > > > > > > - input_panel_surface = calloc(1, sizeof *input_panel_surface); > > > - if (!input_panel_surface) > > > - return NULL; > > > - > > > + input_panel_surface = xzalloc(sizeof *input_panel_surface); > > > > This seems like a change in behavior. When calloc fails, this function > > will return NULL, triggering wl_resource_post_error at all call sites. > > After this change, when xzalloc fails, fail_on_null will call exit(). > > > > Just pointing this out in case it's not what you intended. (And if it is > > intentional, it should be explained in the commit message.) > > That's an excellent notice. However, I'll let is pass, since ivi-shell > is full of exit-on-OOM cases and I don't think it has been seriously > looked at. If it's important, it can be fixed in a follow-up. It's just > a fixed-size struct alloc here. > > > > surface->configure = input_panel_configure; > > > surface->configure_private = input_panel_surface; > > > weston_surface_set_label_func(surface, input_panel_get_label); > > > diff --git a/ivi-shell/ivi-layout-transition.c > > > b/ivi-shell/ivi-layout-transition.c > > > index 989ba71..1175743 100644 > > > --- a/ivi-shell/ivi-layout-transition.c > > > +++ b/ivi-shell/ivi-layout-transition.c > > > @@ -35,6 +35,8 @@ > > > #include "ivi-layout-export.h" > > > #include "ivi-layout-private.h" > > > > > > +#include "shared/xalloc.h" > > > + > > > struct ivi_layout_transition; > > > > > > typedef void (*ivi_layout_transition_frame_func)( > > > @@ -185,12 +187,7 @@ ivi_layout_transition_set_create(struct > weston_compositor *ec) > > > struct ivi_layout_transition_set *transitions; > > > struct wl_event_loop *loop; > > > > > > - transitions = malloc(sizeof(*transitions)); > > > - if (transitions == NULL) { > > > - weston_log("%s: memory allocation fails\n", __func__); > > > - return NULL; > > > - } > > > - > > > + transitions = xzalloc(sizeof *transitions); > > > > Same per my comment above. Although, in this case, the > > ivi_layout_transition_set_create call in ivi_layout_init_with_compositor > > doesn't check for a NULL return anyway (!). > > Heh, so exchanging a crash to an exit. I'm fine with that in this case. > > > And, same for the remaining occurrences in this patch. > > Right. > > I would have let this patch pass otherwise, except there are actually > quite a lot of these occurrences. The deal breaker is the following: > > > > diff --git a/ivi-shell/ivi-layout.c b/ivi-shell/ivi-layout.c > > > index 646eb05..48bec9d 100644 > > > --- a/ivi-shell/ivi-layout.c > > > +++ b/ivi-shell/ivi-layout.c > > > > @@ -1112,12 +1104,7 @@ ivi_layout_get_screens_under_layer(struct > ivi_layout_layer *ivilayer, > > > > > > if (length != 0) { > > > /* the Array must be free by module which called this > function */ > > > - *ppArray = calloc(length, sizeof(struct weston_output *)); > > > - if (*ppArray == NULL) { > > > - weston_log("fails to allocate memory\n"); > > > - return IVI_FAILED; > > > - } > > > - > > > + *ppArray = xzalloc(sizeof(struct weston_output *) * length); > > This is where I would not use xzalloc(), but calloc() is actually the > right one. Calloc will check for multiplication overflows, this here > does not. > > It's probably not meaningful in this particular patch, but it's a habit > well learnt, so that's why I don't like this change. > > There are many occurrences of this. > > > Thanks, > pq _______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
