Re: [PATCH] Implement IP_STAT on NetBSD

[Simon Marchi <simark@simark.ca>]

On 2020-04-13 3:43 p.m., Kamil Rytarowski wrote:
>>> @@ -433,6 +568,149 @@ nbsd_nat_target::info_proc (const char *args, enum info_proc_what what)
>>>        else
>>>  	warning (_("unable to fetch virtual memory map"));
>>>      }
>>> +  if (do_stat)
>>> +    {
>>> +      /* First, try with Linux-compat /proc/<pid>/stat. */
>>> +      char filename[100];
>>> +      xsnprintf (filename, sizeof filename, "/proc/%d/stat", pid);
>>
>> Use
>>
>>   std::string filename = string_printf (...);
>>
> 
> OK. Meanwhile linux-tdep.c could be upgraded by a Linux maintainer.

Ok

>>> +      gdb::unique_xmalloc_ptr<char> statstr
>>> +	= target_fileio_read_stralloc (NULL, filename);
>>> +
>>> +      /* Then fallback to emulating /proc/<pid>/stat with sysctl(3).  */
>>> +      if (statstr == nullptr)
>>> +	statstr = nbsd_pid_to_statstr (pid);
>>
>> I find it a bit strange to take the route of formatting the contents of kinfo_proc2,
>> only to parse it here.  I think it would have been more natural to do:
>>
>> - If /proc/#/stat is available: string -> struct some_internal_struct -> print function
>> - If /proc/#/stat is available: kinfo_proc2 -> struct some_internal_struct -> print function
>>
> 
> 
> This compat shim is only to emulate Linux... it's possible to follow the
> FreeBSD case and just print local kinfo_proc2 as-is (that is fully
> OS-specific).
> 
> What do you think?
> 
> The same question applies to IP_STATUS.

When you put it like this... it just seems useless to read the info from /proc.

If sysctl is guaranteed to always be there and is the blessed way of getting the
information, then let's just use that.  The goal is not to emulate Linux, but to
use each OS's interface to get the required information.
>>> +#if 0	/* Don't know how architecture-dependent the rest is...
>>> +	   Anyway the signal bitmap info is available from "status".  */
>>
>> Either figure out how to print this correctly, or let's just leave it out.
>>
> 
> This comment was copied as-is from gdb/linux-tdep.c.

In my opinion it's not great to leave this as-is in linux-tdep.c either, so I'd
suggest to avoid spreading it.

Simon