[Skiboot] [PATCH v2] core/device: Remove redundant malloc
stewart at linux.ibm.com
Mon Feb 25 17:02:22 AEDT 2019
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;
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?
OPAL Architect, IBM.
More information about the Skiboot