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

[Pekka Paalanen]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 801 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20161109/abf9be6b/attachment.sig>