Re: [PATCH v5 1/1] dts: add text parser for testpmd verbose output

["Juraj Linkeš" <juraj.linkes@pantheon.tech>]

>>
>> The question is when the exception would be raised, or, in other words,
>> what should we do when hasattr(cls, name) is False. If I understand this
>> correctly, is it's False, then name is not among the flags and that
>> means testpmd returned an unsupported flag, which shouldn't happen, but
>> if it does in the future, we would be better off throwing an exception,
>> or at very least, log a warning, so that we have an indication that we
>> need to add support for a new flag.
> 
> This is a good point. Realistically if it is ever false that would
> mean we have a gap in implementation. I like the idea of flagging a
> loud warning over throwing an exception in this case though since if
> we threw an exception that would stop all test cases that use OL flags
> to stop working even if they don't require the new flag. That would
> definitely get the problem fixed sooner, but would also shutdown
> automated testing until it then.
> 

It's a tradeoff between risking CI being affected (if a new flag is 
added without also adding it to DTS) and how noticable the warning is. I 
guess we can implement something in CI that will look for warnings like 
these?

>>
>>> +                flag |= cls[name]
>>> +        return flag
>>> +
>>> +    @classmethod
>>> +    def make_parser(cls) -> ParserFn:
>>> +        """Makes a parser function.
>>> +
>>> +        Returns:
>>> +            ParserFn: A dictionary for the `dataclasses.field` metadata argument containing a
>>> +                parser function that makes an instance of this flag from text.
>>> +        """
>>> +        return TextParser.wrap(
>>> +            TextParser.wrap(TextParser.find(r"ol_flags: ([^\n]+)"), str.split),
>>> +            cls.from_str_list,
>>> +        )
>>
>> The RSSOffloadTypesFlag does the split in its from_list_string method.
>> Do we want to do the same here?
>>
>> Maybe could create a ParsableFlag (or Creatable? Or something else)
>> superclass that would implement these from_* methods (from_list_string,
>> from_str) and subclass it. Flags should be subclassable if they don't
>> contain members.
>>
>> The superclass would be useful so that we don't redefine the same method
>> over and over and so that it's clear what's already available.
> 
> I like this idea a lot. Basically all of these flags that are used in
> parsers are going to need something like that which is going to be
> basically the same so just implementing it one time would be great.
> I'm not sure if it fits the scope of this series though, do you think
> I should write it and add it here or in a separate patch?
> 

A separate patch seems better, as it touches different parts of the 
code. We should probably implement the same logic in this patch (without 
the exception or warning and with the same if condition) and then make 
changes in the other patch.

>>
>>
>>> @@ -656,6 +1147,9 @@ def stop(self, verify: bool = True) -> None:
>>>            Raises:
>>>                InteractiveCommandExecutionError: If `verify` is :data:`True` and the command to stop
>>>                    forwarding results in an error.
>>> +
>>> +        Returns:
>>> +            Output gathered from sending the stop command.
>>
>> This not just from sending the stop command, but everything else that
>> preceded (when collecting the verbose output), right?
> 
> Technically yes, but that's just due to the nature of how interactive
> shells aren't perfect when it comes to asynchronous output. That's why
> I tried to be sneaky and say that it is the "output gathered from
> sending the stop command" trying to imply that it is not just the
> output of the `stop` command, but all the output that is gathered from
> sending it. I can update this though.
> 
>>
>>
>>> diff --git a/dts/framework/utils.py b/dts/framework/utils.py
>>
>>> @@ -27,6 +27,12 @@
>>>    from .exception import ConfigurationError
>>>
>>>    REGEX_FOR_PCI_ADDRESS: str = "/[0-9a-fA-F]{4}:[0-9a-fA-F]{2}:[0-9a-fA-F]{2}.[0-9]{1}/"
>>> +_REGEX_FOR_COLON_SEP_MAC: str = r"(?:[\da-fA-F]{2}:){5}[\da-fA-F]{2}"
>>> +_REGEX_FOR_HYPHEN_SEP_MAC: str = r"(?:[\da-fA-F]{2}-){5,7}[\da-fA-F]{2}"
>>
>> {5,7} should be just 5 repetitions. When could it be more?
> 
> I added it for EUI-64 addresses, but maybe this isn't very relevant
> here since I just read that they are encouraged on non-ethernet
> devices. I can remove it if it doesn't seem worth it to capture.
> 

 From what I gather the EUI-64 address is composed from MAC addresses, 
but it's a different identifier. I'd say if we ever need it we can add 
it as a separate regex (and look for both if we need to).