Re: [PATCH] Add extra 'info os' information types for Linux

[Tom Tromey <tromey@redhat.com>]

>>>>> ">" == Kwok Cheung Yeung <kcy@codesourcery.com> writes:

>> This patch greatly extends the OS information types available to the
>> user using the 'info os' command in Linux. Since the OS info backend
>> is now unified, this information can be obtained from both local and
>> remote (gdbserver) targets.

I think the idea is good.

>> +static int
>> +compare_processes (const void *process1, const void *process2)

Most of the new functions need introductory comments.

>> +  static struct buffer buffer;

Wow, another growable buffer type.  I didn't know about this one.
Let's see.. VEC, dynstr, obstack, buffer... can we have a 5th?  :)

>> +	  pid_t *process_list = (pid_t *) xmalloc (list_block_size * 2 * sizeof (pid_t));

Too bad VEC isn't available here.

>> +	      char procentry[sizeof ("/proc/4294967295")];

One occasionally hears talk of 64 bit PIDs.

>> +#if 0
>> +			  struct stat statbuf;
>> +			  stat (dp2->d_name, &statbuf);
>> +#endif

No new #if 0 code.

>> +static void
>> +time_from_int (char *time, int maxlen, int seconds)

Why int and not just time_t?
...

>> +		  items_read = sscanf (buf,
>> +				       "%d %d %o %d %d %d %d %u %u %u %u %d %d %d",
>> +				       &key, &shmid, &perms, &size,
>> +				       &cpid, &lpid,
>> +				       &nattch,
>> +				       &uid, &gid, &cuid, &cgid,
>> +				       &atime, &dtime, &ctime);
[...]
>> +		      char atime_str[32], dtime_str[32], ctime_str[32];
[...]
>> +		      time_from_int (atime_str, sizeof (atime_str), atime);
>> +		      time_from_int (dtime_str, sizeof (dtime_str), dtime);
>> +		      time_from_int (ctime_str, sizeof (ctime_str), ctime);

I think it is probably better to use long at least.

Tom