[DTrace-devel] [PATCH v4 15/25] libcommon: DOF parsing

[Nick Alcock]

On 11 Oct 2022, Kris Van Hees verbalised:

> I think we discussed that you were going to update this comment to reflect that
> older runtime loaders were not performing the relocation for us while newer
> ones do.  And that we are keeping this code to support both.  So, the XXX should
> be dropped and the comment should be updated, e.g. to say:
>
> 			 * This is a bit ugly but it is necessary for because
> 			 * some linkers retain the relocation records for the
> 			 * .SUNW_dof section in shared libraries.  In that
> 			 * case, the runtime loader already performed the
> 			 * relocation, so we do not have to do anything here.

Done, modulo the obvious grammatical error :P

>> +	/*
>> +	 * Pass back info on the probes and their associated tracepoints.
>> +	 */
>> +	for (i = 0; i < probes_msg.probes.nprobes; i++) {
>> +		dof_parsed_t			*probe_msg;
>> +		size_t				probe_msg_size;
>> +		dtrace_helper_probedesc_t	dhpb;
>> +		char				*ptr;
>> +
>> +		probe = (dof_probe_t *)(uintptr_t)(daddr +
>> +						   prb_sec->dofs_offset +
>> +						   i * prb_sec->dofs_entsize);
>
> Once you have the probe, I think you should put the code to actually validate
> and emit the probe into its own function.  This one is processing a provider,
> and it really seems odd that you emit the probe data here but then call
> emit_probe() to emit the list of tracepoints.

OK. I kept it as it was to minimize the amount of change from the
corresponding v1 code, but if you don't need that any more I can do this
and make everything a bit easier to read :)

> Perhaps this function should be named emit_provider(), emitting the name of the
> provider and the number of probes.  It then calls emit_probe() on every probe
> in the provider.

God knows its existing function name could hardly be worse. Adjusted as
you suggest.

> Then emit_probe() emits the data for the probe (name and number of tracepoints)
> and calls emit_tp() for every of its tracepoints.

Much better. Sorry, this was incremental evolution and I'm afraid it
showed :( (The dhpb generation still happens in emit_provider because
either we do that or we have to push most of emit_provider's variables
into emit_probe as parameters: dhpb is really parameter marshalling
anyway, so the caller is the right place for it.)

>> +void
>> +dof_parse_probes(int out, dof_helper_t *dhp, dof_hdr_t *dof)
>
> This should be dof_parse_providers() since that is what it is doing.

Ack, adjusted. I was thinking "it parses lots of probes" but of course
it's actually parsing a blob of DOF... dof_parse() is probably a better
name yet actually, given the name of the source file.

>> +/*
>> + * Result of DOF probe parsing (currently used only for probe creation. We receive a probes info
>> + * structure, followed by N probe info structures each of which is followed by
>> + * possibly many tracepoint info structures, all tagged.  Things not useful for
>> + * probe creation (like args, translated types, etc) are not returned.
>> + *
>> + * On error, a PIT_ERR structure is returned with an error message.
>> + */
>> +
>> +typedef enum dof_parsed_info {
>> +	PIT_PROBES = 0,
>> +	PIT_PROBE = 1,
>> +	PIT_TRACEPOINT = 2,
>> +	PIT_ERR = 3
>> +} dof_parsed_info_t;
>
> The naming here (and below) still seems rather odd to me.  Something named
> PIT_PROBES would make me assume that a list of probes will be contained in the
> message.  But that is not the case... instead you are using it to pass the
> number of probes.  Would something like PIT_PROBE_COUNT or PIT_PROBE_NUM make
> more sense?
> 
> Of course, since probes are always grouped under a provider, perhaps it would
> make more sense to have a PIT_PROVIDER messaege (and perhaps PIT should become
> DIT for DOF Info Type)

Yes, it should. I couldn't think of a good prefix. PIT_NPROBES would be
better than what I have now, but not as good as DIT_PROVIDER.

>                        that provicdes the name of the provider and the number
> of probes in that provider?  And then this is followed by DIT_PROBE messages
> (with associated DIT_TRACEPOINT messages) to provide the probes and their
> tracepoints?  That seems to be more in line with how DTrace structures the
> data.

Yeah. Makes sense, and also matches how dof_parser is actually emitting
it. Makes the daemon-side parsing a bit simpler too (less picking apart
names).

>> +
>> +typedef struct dof_parsed {
>> +	/*
>> +	 * Size of this instance of this structure.
>> +	 */
>> +	size_t size;
>> +
>> +	dof_parsed_info_t type;
>> +
>> +	__extension__ union {
>> +		struct dpi_probes_info {
>> +			/*
>> +			 * Number of probes that follow.
>> +			 */
>> +			size_t nprobes;
>> +		} probes;
>
> I'd make this:
> 		struct dpi_provider_info {
> 			/*
> 			 * Number of probes that follow.
> 			 */
> 			size_t nprobes;
>
> 			/*
> 			 * Provider name.
> 			 */
> 			char name[1];
> 		} provider;

Ack.

>> +		struct dpi_probe_info {
>> +			/*
>> +			 * Number of tracepoints that follow.
>> +			 */
>> +			size_t ntp;
>> +
>> +			/*
>> +			 * Four \0-separated strings.
>> +			 */
>> +			char probe_name[1];
>
> Perhaps just 'name' since the context already identifies this as a probe name?

Adjusted. (And now three such strings :) ). Code noticeably simpler as a
result.

-- 
NULL && (void)