Re: [RFA 5/7 take 4] Improved linker-debugger interface

[Pedro Alves <palves at redhat dot com>]

On 05/31/2013 02:22 PM, Gary Benson wrote:
> Pedro Alves wrote:
>> On 05/30/2013 11:43 AM, Gary Benson wrote:
>>> +	      char name[32] = { '\0' };
>>> +
>>> +	      /* Fedora 17 and Red Hat Enterprise Linux 6.2-6.4
>>> +		 shipped with an early version of the probes code in
>>> +		 which the probes' names were prefixed with "rtld_"
>>> +		 and the "map_failed" probe did not exist.  The
>>> +		 locations of the probes are otherwise the same, so
>>> +		 we check for probes with prefixed names if probes
>>> +		 with unprefixed names are not present.  */
>>> +
>>> +	      if (with_prefix)
>>> +		strncat (name, "rtld_", sizeof (name) - strlen (name) - 1);
>>> +
>>> +	      strncat (name, probe_info[i].name,
>>> +		       sizeof (name) - strlen (name) - 1);
>>
>> BTW, I'd rather this was written as something like:
>>
>> 	      char name[32];
>>
>> 	      if (with_prefix)
>> 		xsnprintf (name, sizeof (name), "rtld_%s", probe_info[i].name);
>> 	      else
>> 		xsnprintf (name, sizeof (name), "%s", probe_info[i].name);
>>
>>
>> strncat like that is really not future proof, as we'll just get get
>> a silently truncated string if a new probe appears with a name that
>> is too big.  xsnprintf OTOH gdb_asserts the string fits in the
>> buffer.  Granted, whoever added such a probe would probably notice
>> this, but it's a style best avoided.  (Witness how so few strncat
>> calls there are in gdb.  There are more strcat calls, but that only
>> shows that whenever we _do_ care about string overruns, we tend to
>> use something else, not strncat.)
>> Plus, it looks more readable to me that way.  :-)
>> Alternatively, we could add a xstrncat that asserts truncation
>> never happens, though xsnprintf in this case still looks a bit
>> more readable to me :-)
> 
> How about this:
> 
> 	      const char *name = probe_info[i].name;
> 	      char buf[32];
> 
> 	      /* Fedora 17 and Red Hat Enterprise Linux 6.2-6.4
> 		 shipped with an early version of the probes code in
> 		 which the probes' names were prefixed with "rtld_"
> 		 and the "map_failed" probe did not exist.  The
> 		 locations of the probes are otherwise the same, so
> 		 we check for probes with prefixed names if probes
> 		 with unprefixed names are not present.  */
> 	      if (with_prefix)
> 		{
> 		  xsnprintf (buf, sizeof (buf), "rtld_%s", name);
> 		  name = buf;
> 		}
> 
> Revised patch attached.

That's great, thanks.

-- 
Pedro Alves