Re: [PATCH] gold: add --package-metadata

[Luca Boccassi <bluca@debian.org>]

On Fri, 29 Jul 2022 at 20:41, Cary Coutant <ccoutant@gmail.com> wrote:
>
> > Following the same format as the implementation in ld:
> > 9e2bb0cb5e74aed4158f08495534922d7108f928
> >
> > Generate a .note.package FDO package metadata ELF note, following
> > the spec: https://systemd.io/ELF_PACKAGE_METADATA/
> >
> > If the jansson library is available at build time (and it is explicitly
> > enabled), link ld to it, and use it to validate that the input is
> > correct JSON, to avoid writing garbage to the file. The
> > configure option --enable-jansson has to be used to explicitly enable
> > it (error out when not found). This allows bootstrappers (or others who
> > are not interested) to seamlessly skip it without issues.
>
> I reviewed the earlier discussion and I had some of the same questions
> and concerns as others did there, so they've all been answered. I'd
> have preferred an option syntax that would let you build up the
> metadata info one key/value pair at a time. Given that it has already
> been accepted for ld, however, I'll OK it for gold as well, with the
> following fixes...
>
> +  json_t *json = json_loads (desc, 0, &json_error);
>
> C++ coding conventions here: no space before the paren in a function call.
>
> +  if (!json)
> +    gold_fatal(_("error: --package-metadata=%s does not contain valid "
> +      "JSON: %s\n"),
> +    desc, json_error.text);
> +  else
> +    json_decref (json);
>
> Put the shorter, non-error path first (i.e., make it "if
> (json)...else..."), and use { } for the longer error path (even though
> it's only one statement, I think braces around a multi-line statement
> help readability). Also, no space before the paren.
>
> +  if (trailing_padding != 0)
> +  {
> +    posd = new Output_data_zero_fill(trailing_padding, 0);
> +    os->add_output_section_data(posd);
> +  }
>
> The braces and what's inside them need an extra two spaces of indent.
>
> -cary

Thank you for the review, sent v2 with the requested changes.

Kind regards,
Luca Boccassi