From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 9095945A10; Mon, 23 Sep 2024 15:30:18 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 416D44027D; Mon, 23 Sep 2024 15:30:18 +0200 (CEST) Received: from mail-ej1-f51.google.com (mail-ej1-f51.google.com [209.85.218.51]) by mails.dpdk.org (Postfix) with ESMTP id 3BEA040274 for ; Mon, 23 Sep 2024 15:30:16 +0200 (CEST) Received: by mail-ej1-f51.google.com with SMTP id a640c23a62f3a-a8d4979b843so549009666b.3 for ; Mon, 23 Sep 2024 06:30:16 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pantheon.tech; s=google; t=1727098216; x=1727703016; darn=dpdk.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=HgiIB7TF9osEdPEF6z9U3RNEwJJcxrPfj5OKF4/YCYw=; b=uZSkYhPXQ+Yy102rfnOrvMCbtHd9tg+TLFfGjEr4EmO6yszsCjOp1DdZn/YJcS5ceM osDPx+9ev9H6F/4LqEomgvtiI58a6U7NVAUTbOuO2Wvr17VwO40DTRQrOOksiA5i0Y0l D4F2+eHf2Z9MIXV/NQpnf6qA7Q5zCpTqT3MqoBZZezNwnsxF6hMRYpKKIlao9Srnub8x YKs15JmraNKFOzMsOUllQ7+k2myqkQ5PCTYTMFAzy5RAZFKLSMzeOa3opnSv7KLX2Mez heS3NENr2snB0ZphtAjli/vLv8nZ1v9Z72oXO5oZS4RkcPDFda7TjzIy56RKWcwI1z7Z /aHg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1727098216; x=1727703016; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=HgiIB7TF9osEdPEF6z9U3RNEwJJcxrPfj5OKF4/YCYw=; b=dZaQgLfxtn6GAnqaV7XOK9oHhUEiEopZtQJQ9QZ5Mj3Ajg4H0CWXo4oru4qb0uzsJg QB+3r4YcxmWzbZlmcJK12AQM/We07roapALXqX7fhPds//euXaxnBZ9fXRLqc0Kjb0dT ggzU4gaT2M4syyNg34umniVJsPTt5yXcgn3/sxuTHpBvtQCCZ6lPf+sl9DoQuq25E8ks 28w4dwCAKM6Adca1lRTiv9/kco5kdNg7h78ydv7r/CPpGplNZuZr8Ux3dVhgWN8XPqyz L2we9U1YTMet5bpJsjKiI3Z+pM/oB9Eph1gX6AspV7y5yEU4e6lOlcERAHqLXVTLNkqB NQ/Q== X-Forwarded-Encrypted: i=1; AJvYcCVh1svj4gM1c96vjmQwonzJpxQLof9YpaLZlAtyiO9ivahq4MOz3LNKKDOPxfCi/nUJacA=@dpdk.org X-Gm-Message-State: AOJu0Yybwi5Ms5KUpmLWMAI8yPgv4G1xcR4wluKtLrgFcOQGJTXQjYc3 MqBOBPRj0aTXaqBPp0OMKafyp0K+wDNKerSAzyzI2D9k8X7cRgfHlcIWhTP2A6M= X-Google-Smtp-Source: AGHT+IEVbrkc3WaeXDYU78Om9lWkgOzR0T2b6Q0rvWxkP7UXFu0/2/PxvR40uS8MxpKqgaGsQ7vNzw== X-Received: by 2002:a17:907:d5a1:b0:a8d:3d36:3169 with SMTP id a640c23a62f3a-a90d58417e2mr1099839566b.63.1727098215779; Mon, 23 Sep 2024 06:30:15 -0700 (PDT) Received: from [192.168.200.22] ([84.245.121.62]) by smtp.gmail.com with ESMTPSA id a640c23a62f3a-a90612b3072sm1223029366b.108.2024.09.23.06.30.14 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 23 Sep 2024 06:30:15 -0700 (PDT) Message-ID: Date: Mon, 23 Sep 2024 15:30:13 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v5 1/1] dts: add text parser for testpmd verbose output To: Jeremy Spewock Cc: alex.chapman@arm.com, paul.szczepanek@arm.com, Luca.Vizzarro@arm.com, Honnappa.Nagarahalli@arm.com, wathsala.vithanage@arm.com, probb@iol.unh.edu, npratte@iol.unh.edu, thomas@monjalon.net, yoan.picchi@foss.arm.com, dev@dpdk.org References: <20240729203955.267942-1-jspewock@iol.unh.edu> <20240918170528.14545-1-jspewock@iol.unh.edu> <20240918170528.14545-2-jspewock@iol.unh.edu> <92be8e4d-cfc6-4b07-b014-1d9dec051b91@pantheon.tech> Content-Language: en-US From: =?UTF-8?Q?Juraj_Linke=C5=A1?= In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org >> >> 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).