Re: [PATCH/RFC 01/02 v2] Refactor PRPSINFO handling on Binutils

[Sergio Durigan Junior <sergiodj@redhat.com>]

On Tuesday, January 01 2013, Jan Kratochvil wrote:

> thanks for the updated, I have just seen some more minor issues:

Thanks for the comments.  I have some questions.

> On Sun, 30 Dec 2012 02:49:36 +0100, Sergio Durigan Junior wrote:
>> --- a/bfd/Makefile.in
>> +++ b/bfd/Makefile.in
>> @@ -1068,7 +1068,7 @@ BUILD_CFILES = \
>>  CFILES = $(SOURCE_CFILES) $(BUILD_CFILES)
>>  SOURCE_HFILES = \
>>  	aout-target.h aoutf1.h aoutx.h coffcode.h coffswap.h ecoffswap.h \
>> -	elf-bfd.h elf-hppa.h elf32-hppa.h \
>> +	elf-bfd.h elf-psinfo.h elf-hppa.h elf32-hppa.h \
>>  	elf64-hppa.h elfcode.h elfcore.h \
>>  	freebsd.h genlink.h go32stub.h \
>>  	libaout.h libbfd.h libcoff.h libecoff.h libhppa.h libieee.h \
>> diff --git a/bfd/elf-bfd.h b/bfd/elf-bfd.h
>> index b8d82b1..90ced7a 100644
>> --- a/bfd/elf-bfd.h
>> +++ b/bfd/elf-bfd.h
>> @@ -1722,6 +1722,38 @@ struct elf_obj_tdata
>>    (elf_known_obj_attributes (bfd) [OBJ_ATTR_PROC])
>>  #define elf_other_obj_attributes_proc(bfd) \
>>    (elf_other_obj_attributes (bfd) [OBJ_ATTR_PROC])
>> +
>> +#ifndef ELF_PRARGSZ
>> +/* Maximum size of the arguments that can be stored in a PRPSINFO
>> +   structure.  */
>> +
>> +#define ELF_PRARGSZ (80)
>> +#endif
>
> This should be unrelated to the system definition, BFD now defines everything
> about PRPSINFO on its own and different system definiton of it can just break
> it.  Host system may be some exotic kind with no core files support by
> BFD.

Ok, I understand your argument, but it is not clear what you are
suggesting.  This definition should be present here and in the
elf-psinfo.h file.

>> +/* Internal structure which holds information to be included in the
>> +   PRPSINFO section of the corefile.
>> +
>> +   This is an "internal" structure in the sense that it should be used to
>> +   pass information to BFD (via the `elfcore_write_prpsinfo', for example),
>> +   so things like endianess shouldn't be an issue.  This structure will
>> +   eventually be converted in one of the `elf_external_*' structures
>> +   below.  */
>> +
>> +struct elf_internal_prpsinfo
>> +  {
>> +    char pr_state;			/* Numeric process state.  */
>> +    char pr_sname;			/* Char for pr_state.  */
>> +    char pr_zomb;			/* Zombie.  */
>> +    char pr_nice;			/* Nice val.  */
>> +    unsigned long pr_flag;		/* Flags.  */
>> +    unsigned int pr_uid;
>> +    unsigned int pr_gid;
>> +    int pr_pid, pr_ppid, pr_pgrp, pr_sid;
>> +    /* Lots missing */
>
> This comment seems off-topic here.  It fully represents the core file
> structure.

I can remove the comment, but it doesn't say that the structure does not
represent a corefile struct.  It just explains the reason why it is
called "internal".

>> +
>> +   This file is part of BFD, the Binary File Descriptor library.
>> +
>> +   This program is free software; you can redistribute it and/or modify
>> +   it under the terms of the GNU General Public License as published by
>> +   the Free Software Foundation; either version 3 of the License, or
>> +   (at your option) any later version.
>> +
>> +   This program is distributed in the hope that it will be useful,
>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +   GNU General Public License for more details.
>> +
>> +   You should have received a copy of the GNU General Public License
>> +   along with this program; if not, write to the Free Software
>> +   Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston,
>> +   MA 02110-1301, USA.  */
>> +
>> +#ifndef ELF_PRARGSZ
>> +/* Maximum size of the arguments that can be stored in a PRPSINFO
>> +   structure.  */
>> +
>> +#define ELF_PRARGSZ (80)
>> +#endif
>
> The same comment as above.

The same question as above.

>> +/* Process info.  In the end we do provide typedefs for them.  */
>> +
>> +typedef struct elf_external_prpsinfo32 prpsinfo32_t;
>> +typedef struct elf_external_prpsinfo64 prpsinfo64_t;
>
> There was some comment that *_t types are reserved for system (POSIX?) use.
> So when BFD now now longer uses the system ones and fully defines them on its
> own they should have some different namespace, not *_t.

Ok, but renaming these types will require renaming the files that use
them as well.

Thanks,

-- 
Sergio