[Skiboot] [PATCH v2] core/device: Remove redundant malloc

[Stewart Smith]

Jordan Niethe <jniethe5 at gmail.com> writes:

> Currently dt_new_addr() / dt_new_2addr() malloc a scatch buffer for the
> node name. This is then duplicated in new_node(). This makes the initial
> allocation redundant. To address this the allocation was removed from
> dt_new_addr() / dt_new_2addr() and new_node() was changed to a varadic
> function to accomodate the different name format.
>
> Ideally vsnprintf() would be used to calculate the length of the
> resulting name so a buffer could be dynamically allocated.
> This would then be filled with another call to vsnprintf(). However the
> implemenation of vsnprintf() does not behave correctly when called with
> a size of 0 so this is not possible.

feel free to fix our vsnprintf() implementation :)

> Instead a statically allocated buffer is used. This assumes a maximum
> length for a name.
> The optimization for ro memory names in take_name() is no longer
> peformed. This is because the arguements to new_node() are intended for
> vsnprintf() and can not be readly checked using is_rodata().
> This does not significantly impact memory usage.
>
> Signed-off-by: Jordan Niethe <jniethe5 at gmail.com>
> ...
> It is possible to check read only memory by checking for a format string
> indicator. This will now be performed.

I'm a bit nervous about doing this rather than relying on is_rodata() as
we could *theoretically* construct the format string at runtime, and
then this'd break in weird ways. Instead, let's just stick to
is_rodata() and it should all be fine.

> ---
>  core/device.c | 41 ++++++++++++++++++++---------------------
>  1 file changed, 20 insertions(+), 21 deletions(-)
>
> diff --git a/core/device.c b/core/device.c
> index 6364a60e93ec..bfdfc7a22206 100644
> --- a/core/device.c
> +++ b/core/device.c
> @@ -44,15 +44,31 @@ static void free_name(const char *name)
>  		free((char *)name);
>  }
>  
> -static struct dt_node *new_node(const char *name)
> +static struct dt_node *new_node(const char *fmt, ...)
>  {
>  	struct dt_node *node = malloc(sizeof *node);
> +	char name[256];

Is this limit coming from the fdt spec? Or just arbitrary? If so, we
should probably check we don't hit it. IIRC our vsnprintf *does* manage
to return how much we printed into the destination buffer so we can at
least detect this overflow.

Additionally, how does this affect the code coverage of 'make
coverage-report', do we end up at the same or are we missing anything?

-- 
Stewart Smith
OPAL Architect, IBM.