Re: [PATCH 1/4] Initialize unit mask fields related to "extra:" properly

[Maynard J. <may...@us...>]

On 01/28/2014 01:35 PM, William Cohen wrote:
> On 01/28/2014 12:00 PM, Maynard Johnson wrote:
>> On 01/27/2014 01:39 PM, William Cohen wrote:
>>> The extra field should be initialized to EXTRA_NONE to be consistent
>>> with the rest of oprofile.  The name field should be also be set to a
>>> sane default for the cases where unit masks do not have names.
>> Will,
>> I don't think this patch is needed.  Are you aware of a particular problem that this patch solves?  In read_unit_masks(), a new op_unit_mask object is created using new_unit_mask(), which does an xmalloc of sizeof(struct op_unit_mask), which includes the um array of size MAX_UNIT_MASK.  So all the um fields (name, extra, value, desc) are initialized to zero.  As for initializing entry->extra = EXTRA_NONE . . . while that would do no harm (AFICT), it's not necessary (since it's zero already), and it sort of goes against the idea of where EXTRA_NONE is used elsewhere (to indicate that a named um actually does *not* have any real extra bits).
>>
>> If you (or Andi or Suravee) see any whole in my analysis, let me know.
> 
> Hi Maynard,
> 
> I didn't realize that the value of the name field was already zeroed out.  You are right that entry->name initialization is redundant.
> 
> However, the EXTRA_NONE is required otherwise there are two possible values that mean nothing is in the extra field: 0 and EXTRA_NONE.  By explicitly setting it to EXTRA_NONE makes the check in libop/op_xml_events.c xml_help_for_event() simpler. 
True, but as I mentioned in my reply to your PATCH 2/4, the 'extra' attribute should be removed from the XML output anyway.  I will put together a patch to do that and post it soon.

-Maynard

> 
> -Will
>>
>> -Maynard
>>>
>>> Signed-off-by: William Cohen <wc...@re...>
>>> ---
>>>  libop/op_events.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/libop/op_events.c b/libop/op_events.c
>>> index 358a154..f4b439f 100644
>>> --- a/libop/op_events.c
>>> +++ b/libop/op_events.c
>>> @@ -224,6 +224,8 @@ static void parse_um(struct op_unit_mask * um, char const * line)
>>>  static void parse_um_entry(struct op_described_um * entry, char const * line)
>>>  {
>>>  	char const * c = line;
>>> +	entry->extra = EXTRA_NONE;
>>> +	entry->name = NULL;
>>>
>>>  	/* value */
>>>  	c = skip_ws(c);
>>> @@ -248,8 +250,6 @@ static void parse_um_entry(struct op_described_um * entry, char const * line)
>>>  		entry->name = op_xstrndup(c, strcspn(c, " \t"));
>>>  		c = skip_nonws(c);
>>>  		c = skip_ws(c);
>>> -	} else {
>>> -		entry->extra = 0;
>>>  	}
>>>
>>>  	/* desc */
>>>
>>
>