Re: [RFC] Fix hpux_major_release variable setting

[Joel Brobecker <brobecker@adacore.com>]

>   The patch below fixes that problem by moving the call to uname to
>   hppa-hpux-nat.c
> 
>   Is this patch OK?

I think that for a platform such as pa-hpux, this is fine (MarkK,
who just informed us that he has access to HP-UX hardware as well,
might want to coment as well).  But I've since learnt that the correct
way of doing this is to use target_xfer_partial routine.  The reason
why I think it's OK this way is that the fix seems contains to pa/hp-ux
and I don't see a lot of benefit in spending time and energy into trying
to support HP-UX 10.x.

>   I am unsure about the way to move it to the header... Would it be
>   better to declare it without external in the header?

I don't think that this would work. You'd end up with multiple definitions
of the same global variable.

>   This would then mean that I should also move
>   DEFAULT_HPUX_MAJOR_RELEASE macro to the header, but it would have
>   the advantage of allowing to not use hard-coded 11 in
>   _initialize_hppa_hpux_nat.

You can just leave the variable untouched instead of setting it to 11...

> 2011-01-14  Pierre Muller  <muller@ics.u-strasbg.fr>
> 
> 	* solib-som.h (hpux_major_release): Declare variable here.
> 	* solib-som.c:  Remove <sys/utsname.h> header.
> 	(DEFAULT_HPUX_MAJOR_RELEASE): New macro.
> 	(hpux_major_release): Make global, change default value to
> 	DEFAULT_HPUX_MAJOR_RELEASE.
> 	(get_hpux_major_release): Simply return HPUX_MAJOR_RELEASE. 
> 	* hppa-hpux-nat.c: Add <sys/utsname.h> include.
> 	Add "solib-som.h" header.
> 	(_initialize_hppa_hpux_nat): Set hpux_major_release variable by
> 	analyzis of uname call return.

> +/* Variable storing HP-UX major release number.
> +   On non native system, simply suppose that the major release number is
                                  ^^^^^^^^
                                  assume
> 11.
> +   hppa-hpux-nat.c initialization code sets this number to
> +   the real one on startup in native conditions.  */

Suggest the following formatting (following the 70 characters guideline,
as well as an empty line for better clarity (personal opinion, can be
ignored).  I'd also add a comment explaining why this value is computed
elsewhere. So here is a suggested rewordin:

/* Variable storing HP-UX major release number.

   On non-native system, simply assume that the major release number
   is 11.  On native systems, hppa-hpux-nat.c initialization code
   sets this number to the real one on startup in native conditions.
   
   We cannot compute this value here, because we need to make a native
   call to "uname".  We are are not allowed to do that from here, as
   this file is used for both native and cross debugging.  */

> +#define DEFAULT_HPUX_MAJOR_RELEASE 11
> +int
> +hpux_major_release = DEFAULT_HPUX_MAJOR_RELEASE;

Formatting:

int hpux_major_release = DEFAULT_HPUX_MAJOR_RELEASE;

(I'm wondering if the DEFAULT_HPUX_MAJOR_RELEASE macro really brings much).

>  static int
>  get_hpux_major_release (void)

Let's get rid of this function at the same time, WDYT? It should be
a simple search-and-replace...

> +  struct utsname x;
> +  char *p;
>  
> +  uname (&x);
> +  p = strchr (x.release, '.');
> +  hpux_major_release = p ? atoi (p + 1) : 11;

Let's make that a function, and call the function from the
initialization code.  I think it'll be easier to understand.
Otherwise, a small comment, and a new-line after the code block
would help visually separate this operation from the rest.

-- 
Joel