[PATCH weston v2 3/3] ivi-shell: use zalloc instead of calloc

[Ucan, Emre (ADITG/SW1)]

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:ppaalanen at 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 <junk at humanoriented.com> wrote:
> 
> > Hi Emre,
> >
> > > On Jul 28, 2016, at 7:14 AM, Ucan, Emre (ADITG/SW1) <eucan at 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 <eucan at 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