Re: PATCH: Print out bits of SOM headers

[Nick Clifton <nickc at redhat dot com>]

Hi Mark,

> 2004-11-08  Mark Mitchell  <mark@codesourcery.com>
>
> 	* som.c (som_bfd_print_private_bfd_data): New function.
> 	(som_object_setup): Save the auxiliary header.  Don't assume that
> 	zero is an invalid entry point for a shared library.
> 	(som_object_p): Allocate the auxiliary header on the heap.

Could you make a couple of tidy-ups to the patch first please ?

> + static bfd_boolean som_bfd_print_private_bfd_data
> +   PARAMS ((bfd *, void *));

We are using ISO C90 in binutils now, and even though this file has not 
yet been converted, I think that it is better that any new code added 
should follow this standard.  Hence the PARAMS macro is no longer needed.

> !   struct som_exec_auxhdr aux_hdr;
> --- 2193,2203 ----
> !   struct som_exec_auxhdr *aux_hdr = NULL;

My gut feeling here is that if you are changing the type of a variable 
you ought to change its name as well.  Hence I would recommend changing 
aux_hdr to aux_hdr_ptr.

> +       aux_hdr = ((struct som_exec_auxhdr *)
> + 		 bfd_zalloc (abfd, 
> + 			     (bfd_size_type) sizeof (struct som_exec_auxhdr)));

bfd_zalloc returns a (void *) so there is no need for the cast to 
(struct som_exec_auxhdr *).  Also, and this is just a personal 
preference, I think that it is safer to take the size of the pointer 
being allocated rather than the type that you know the pointer to be. 
That way if the pointer's type is ever changed you do not have to alter 
the code inside the alloc.  ie, I would suggest that the above statement 
could be coded as:

   aux_hdr_ptr = bfd_zalloc (abfd, (bfd_size_type) sizeof (* aux_hdr_ptr));

> + /* Display the SOM header.  */
> + 
> + static bfd_boolean
> + som_bfd_print_private_bfd_data (abfd, farg)
> +      bfd *abfd;
> +      void *farg;

ISO C90 again - please include the function's parameter types inside the 
parentheses.

> +       auxhdr = &exec_header->som_auxhdr;
> +       if (auxhdr->mandatory)
> + 	fprintf (f, "mandatory ");

For paranoia purposes I would suggest that you check for a NULL auxhdr 
before using it, in case the som_exec_auxhdr structure is corrupt.

With these changes made, please consider the patch approved.

Cheers
  Nick