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 88CDD45482; Mon, 17 Jun 2024 16:38:12 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id C6BE240674; Mon, 17 Jun 2024 16:38:07 +0200 (CEST) Received: from mail-pj1-f49.google.com (mail-pj1-f49.google.com [209.85.216.49]) by mails.dpdk.org (Postfix) with ESMTP id 2CE9C4028A for ; Mon, 17 Jun 2024 16:38:05 +0200 (CEST) Received: by mail-pj1-f49.google.com with SMTP id 98e67ed59e1d1-2c306e87b1fso3453353a91.3 for ; Mon, 17 Jun 2024 07:38:05 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1718635084; x=1719239884; darn=dpdk.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=gPpWIUU/VZk1OrzL6EfQFH++mRnEPr5pJd/rZhe/WlI=; b=fZn7WmBK0dVhK7gQ/YrLLYLHsjbNZ2dszVhufd1TT+FCH7kQJT0YKqi8VosM8bQLww 1PkvGwqcMrUHinaGii9gPuCtKRC8j4Ic3Nn2KDwWo6RTRLty/i1c2HDLPJY9rWjnmuFq k36jeQyqurbg/HbXjFzIVBDssTQIMam7hJt9E= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1718635084; x=1719239884; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=gPpWIUU/VZk1OrzL6EfQFH++mRnEPr5pJd/rZhe/WlI=; b=JCYt4UDtlUv4gJNof+4p99GwC480grQC0UkXrUntGqV1J1hBISrDMXH4FfxL0Q7Gq5 hwNkZFHkgFj5QeI8WMTJGI7ztfqnDbSwmXvrOO/5gPNJiLwIbAq3nkOQtT9/qSyRy81p cmvwGgF4bJsIxJWHwfkT868rvy2vdpNkYJnGyUg/6Ly3mfDEqzpxG+x6F31x3ZLcODf5 Otrc+PE9aPeD/nTc5C9zYIAXCBnHFB2sVv22LUo/pDSMlGttb54c8okz4UNhu0Xu8VjQ Mb38ofenvO7zYLr8d+Jt6K0z2k+qnh4Ph294wlMVNLCErafiQVJq4LWwvW+3blXno3Rk cfmg== X-Forwarded-Encrypted: i=1; AJvYcCWc+OWRwtjB6mRFQKWsBa9zZUaSpUY5GA3w7jdSBNEkynOpWCO495wARPPTr9d+g1vpQ8TdP6bYAL9adiQ= X-Gm-Message-State: AOJu0YxS2QdwJL/McwhogdYPeF5Dhp0NWsWSdH3R8zlmb0AfH3HRDCk3 nRXNB6QyELgDjsQkiAiEtBfXIFcJKN0m/wt5RDevqwgeXH0FWt/T8C3bpjFbJEV8v7ilvuuEp39 1IGFQRTIS0AFNLEu57myVzXHMEWOapiSevLHf/A== X-Google-Smtp-Source: AGHT+IGycBVdmNw85TJOm1rMkYUCjPe6zW0KBras1a/ilv5VsefMKAgKL5d9YXkBJF84yD+KmZwGwEJ2VByW/OYBI98= X-Received: by 2002:a17:90a:af88:b0:2c4:e048:4bec with SMTP id 98e67ed59e1d1-2c4e0484c6cmr6956698a91.47.1718635084266; Mon, 17 Jun 2024 07:38:04 -0700 (PDT) MIME-Version: 1.0 References: <20240611161606.23881-2-dmarx@iol.unh.edu> <20240614150238.26374-1-dmarx@iol.unh.edu> <20240614150238.26374-2-dmarx@iol.unh.edu> In-Reply-To: <20240614150238.26374-2-dmarx@iol.unh.edu> From: Jeremy Spewock Date: Mon, 17 Jun 2024 10:37:53 -0400 Message-ID: Subject: Re: [PATCH v3 1/3] Added VLAN commands to testpmd_shell class To: Dean Marx Cc: Honnappa.Nagarahalli@arm.com, juraj.linkes@pantheon.tech, probb@iol.unh.edu, paul.szczepanek@arm.com, yoan.picchi@foss.arm.com, bruce.richardson@intel.com, luca.vizzarro@arm.com, dev@dpdk.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 In the methods you added here, you made all of the parameters to them optional with a default. This might make sense to allow developers to be less verbose when calling the method with the defaults, but in the case of things like the port ID, vlan IDs, and things of that nature I think making these positional arguments that are required makes more sense for readability. For example, if I were to call testpmd.vlan_filter_set_on(), it is not clear where this vlan filter is being modified. The default is 0 so it will set it on port 0, but if you're just reading the test suite you wouldn't know that without looking at the testpmd class. Verify is left as an optional parameter because we really want to verify that the commands were successful in all cases, but there could be an obscure reason in the future that a developer might not really care if a command to testpmd was successful or not. I also noticed in most cases where you were verifying commands that you had to check if multiple failures messages were present. Would it be easier to check if the command was successful by querying things like VLANs on a port or VLAN filtering status of a port instead of trying to cover all error cases? On Fri, Jun 14, 2024 at 11:03=E2=80=AFAM Dean Marx wrot= e: > > The testpmd_shell class is intended for sending commands to > a remote testpmd session within a test suite, and as of right now > it is missing most commands. Added commands for vlan filtering, > stripping, and insertion, as well as stopping and starting ports > during testpmd runtime. While it's true that the testpmd class is missing a lot of commands that exist in testpmd, that isn't really why you are adding them. Maybe switching this to say something like "It is missing functionality required for verifying VLAN features on a PMD." might be more descriptive. > > Signed-off-by: Dean Marx > --- > + def vlan_filter_set_on(self, port: int =3D 0, verify: bool =3D True)= : > + """Set vlan filter on. > + > + Args: > + port: The port number to use, should be within 0-32. > + verify: If :data:`True`, the output of the command is scanne= d to verify that > + vlan filtering was enabled successfully. If not, it is > + considered an error. > + > + Raises: > + InteractiveCommandExecutionError: If `verify` is :data:`True= ` and the filter > + fails to update. > + """ > + filter_cmd_output =3D self.send_command(f"vlan set filter on {po= rt}") > + if verify: > + if "Invalid port" in filter_cmd_output: This doesn't really verify that the filter was set as much as it verifies that the port exists. Is there anything that testpmd prints when it is successful, or possibly a way to query existing VLAN filters on a port to see if it was set? > + self._logger.debug(f"Failed to enable vlan filter: \n{fi= lter_cmd_output}") > + raise InteractiveCommandExecutionError("Testpmd failed t= o enable vlan filter.") > + > + def vlan_filter_set_off(self, port: int =3D 0, verify: bool =3D True= ): > + """Set vlan filter off. > + > + Args: > + port: The port number to use, should be within 0-32. > + verify: If :data:`True`, the output of the command is scanne= d to verify that > + vlan filtering was disabled successfully. If not, it is > + considered an error. > + > + Raises: > + InteractiveCommandExecutionError: If `verify` is :data:`True= ` and the filter > + fails to update. > + """ > + filter_cmd_output =3D self.send_command(f"vlan set filter off {p= ort}") > + if verify: > + if "Invalid port" in filter_cmd_output: Same thing as above comment about verification. > + self._logger.debug(f"Failed to disable vlan filter: \n{f= ilter_cmd_output}") > + raise InteractiveCommandExecutionError("Testpmd failed t= o disable vlan filter.") > + > + def vlan_strip_set_on(self, port: int =3D 0, verify: bool =3D True): > + """Enable vlan stripping on the specified port. > + > + Args: > + port: The port number to use, should be within 0-32. > + verify: If :data:`True`, the output of the command is scanne= d to verify that > + vlan stripping was enabled on the specified port. If not= , it is > + considered an error. > + > + Raises: > + InteractiveCommandExecutionError: If `verify` is :data:`True= ` and the filter > + fails to update. > + """ > + vlan_strip_output =3D self.send_command(f"vlan set strip on {por= t}") > + if verify: > + if "Invalid port" in vlan_strip_output: This is similar to above where it isn't really verifying that the vlan stripping was enabled. > + self._logger.debug(f"Failed to add vlan tag: \n{vlan_str= ip_output}") > + raise InteractiveCommandExecutionError("Testpmd failed t= o add vlan tag.") > + > + def vlan_strip_set_off(self, port: int =3D 0, verify: bool =3D True)= : > + """Disable vlan stripping on the specified port. > + > + Args: > + port: The port number to use, should be within 0-32 > + verify: If :data:`True`, the output of the command is scanne= d to verify that > + vlan stripping was disabled on the specified port. If no= t, it is > + considered an error. > + > + Raises: > + InteractiveCommandExecutionError: If `verify` is :data:`True= ` and the filter > + fails to update. > + """ > + vlan_strip_output =3D self.send_command(f"vlan set strip off {po= rt}") > + if verify: > + if "Invalid port" in vlan_strip_output: Same here. > + self._logger.debug(f"Failed to enable stripping: \n{vlan= _strip_output}") > + raise InteractiveCommandExecutionError(f"Testpmd failed = to enable stripping on port {port}.") On all of the other verification methods the message for the error doesn't include specifics like what port it was on or what vlan tag failed. I am generally more in favor of including specific information like this and many of the other testpmd methods do, so if possible I think we should add some of this information to the other methods here as well to make it consistent. > + > 2.44.0 >