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

[Nick Clifton <nickc@redhat.com>]

Hi Luca,

   First of all a couple of questions about the need for the patch itself.

   Firstly - why modify the linker itself ?  Couldn't the same effect be
   achieved by using the assembler to create an object file and then
   including that in the link along with the other objects ?

   Secondly - why build json verification into the linker ?  It does not
   feel like a linkery kind of thing to do, and because the verification
   is dependent upon the linker being configured and built with the feature
   enabled, it means that programmers cannot rely upon their packaging
   notes always being checked.  To me, I think that it would be better to
   have a pre- or post- links stage where the note is checked by a separate
   tool.

   Lastly, since this option is being used to create a note section, maybe
   we ought to consider the need for a generic mechanism for the linker to
   create sections - other than using linker scripts.  For example, suppose
   that instead of --package-metadata=<JSON> there was an option:
   --add-note=<SEC-NAME>,<NOTE-NAME>,<CONTENTS>,[<VERIFIER>]
   which would basically do the same thing, but in a more generic fashion.

   Some thoughts on the patch itself:

> +  if (t->o->build_id.after_write_object_contents != NULL && !(*t->o->build_id.after_write_object_contents) (abfd))
> +    return false;

Line length - the if-statement should be over two lines.

> +  if (t->o->package_metadata.after_write_object_contents != NULL)
> +    return (*t->o->package_metadata.after_write_object_contents) (abfd);

For consistency sake, it would be nice if these two lines
were turned into a two predicate if-statement like the ones
above.  That way if other after_write_xxx functions are added
in the future, then this if-statement will not need to be
modified.

> +@kindex --package-metadata=@var{JSON}
> +@item --package-metadata=@var{JSON}
> +Request the creation of a @code{.note.package} ELF note section.  The
> +contents of the note are in JSON format, as per the package metadata
> +specification.  For more information see:
> +https://systemd.io/COREDUMP_PACKAGE_METADATA/

You should mention that if the JSON argument is missing/empty then this
will disable the creation of the metadata note, if one had been enabled
by an earlier occurrence of the --package-metdata option.

You should also add an entry to the ld/NEWS file about this new feature.

It might also be worth mentioning that the contents of the JSON text
will be checked for conformance to the standard, if the linker has been
configured and built that way.

Also - can the JSON data be read from a file ?  I can easily see problems
being encountered with the length of the linker command line if the JSON
data can only be specified as text.  (Possibly this can be avoided by using
the @file syntax already supported by the linker).

> -  if (ldelf_emit_note_gnu_build_id != NULL)
> +  if (ldelf_emit_note_gnu_build_id != NULL ||
> +      ldelf_emit_note_fdo_package_metadata != NULL)

Formatting - the || should be at the start of the next line.

> +  if (!ldelf_emit_note_fdo_package_metadata || strlen(ldelf_emit_note_fdo_package_metadata) == 0)
> +    return false;

Formatting - a space between strlen and the opening parenthesis please.

> +  json_t *json = json_loads(ldelf_emit_note_fdo_package_metadata, 0, &json_error);
> +    json_decref(json);

Likewise for these two function calls.


> +if { [istarget frv-*-*] || [istarget lm32-*-*] } {
> +    return
> +}

Why are these architectures being skipped ?

Cheers
   Nick