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 B704645460; Fri, 14 Jun 2024 18:00:03 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 451E440B95; Fri, 14 Jun 2024 18:00:03 +0200 (CEST) Received: from mail-oo1-f43.google.com (mail-oo1-f43.google.com [209.85.161.43]) by mails.dpdk.org (Postfix) with ESMTP id C8D90402D0 for ; Fri, 14 Jun 2024 18:00:01 +0200 (CEST) Received: by mail-oo1-f43.google.com with SMTP id 006d021491bc7-5ba70a0ed75so1188506eaf.1 for ; Fri, 14 Jun 2024 09:00:01 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1718380801; x=1718985601; 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=SP2aeU5MC4aJnxej1A7063Y6oMwuoY80QJ74dAKVFeg=; b=gP+ev6dBWzsOENDWFp+CxK9hJDR2jhjogzjr37fcoSNURw+zX9VBZDRQE3IKP2B42p L1dI9T87BJeU6j4D/HogTh4qCSuH0Q0v6OODC0KKlEwAZhYdrZUVItyBSj3PZ5wSkLun 5HVl1IJ5tIRXmlyyoMCsOD6iLN6F5yYgRe6gc= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1718380801; x=1718985601; 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=SP2aeU5MC4aJnxej1A7063Y6oMwuoY80QJ74dAKVFeg=; b=wQY5fbB3HdgEI8JMeBHl7nKxrfO2aO8cTQfXfh0WO25J/GMQhWbnmSpm2LTLQpT7Jd etdQZT+Ec7ZVwlIXUF5XZJXyqaIhCE8H727LuOPfMHUOZM3XG1HJ8zA0NTwAhPjXjXPR +YqJ3b4aPL91ziZDnpeeLP2BHFrT7BicR4xmxZnFbWgbfDneSmQDZwb6jBSnWsUpv1oA CQ4FUQRUMIrHiXusRdH+rx+iTseAdLmWo3Dzrlf5Ozd+qoU3kb7lyWyPojCLABbH6RGt vv0pfAJmEUNnME7nCPGa7GqrA0x1X/wIXOdsYBbNPPvTZyh0sLQBwQBqS+uq3b65M9xR gOOg== X-Forwarded-Encrypted: i=1; AJvYcCXEazK00MzKFE5Ub7SZ6GjSBHzCMcEYViMeUoh2/1Iw05AjZywmhyYhI6jLzcrwbPV7SMNKXv/rWYDovn8= X-Gm-Message-State: AOJu0YyvJnoxppPKcsOP+DNSFWcqfQwLJaIw1InkwUHzE5YeZS3WsTmI atGsm+z/wc8HVvkJMaifk0jVlR5OQb1ODMkKPJxaJ1ECHJEL25QCEVGklBJ6PlUejKOsm7Hm4af cQNNeREzlj3MMkg9jItynjbo/Ie++YgCKM3YtVQ== X-Google-Smtp-Source: AGHT+IEtQaBylSRe0sbYxd34bsqv83BJiJ/OOytNibacGDg/HPb5yZWhO3MV+jgaoJ9lkcFiaiVlIHWCxlx1EPgG/mQ= X-Received: by 2002:a05:6820:80c:b0:5ba:f20c:361b with SMTP id 006d021491bc7-5bdadc84948mr3568641eaf.8.1718380800871; Fri, 14 Jun 2024 09:00:00 -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: Patrick Robb Date: Fri, 14 Jun 2024 11:59:50 -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, paul.szczepanek@arm.com, yoan.picchi@foss.arm.com, jspewock@iol.unh.edu, 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 On Fri, Jun 14, 2024 at 11:03=E2=80=AFAM Dean Marx wrot= e: > + 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}") I wonder whether, when convenient, we want to name the methods more or less 1:1 according to the actual testpmd text command they send? I.e. in this case should the method be named vlan_set_filter_on instead of vlan_filter_set_on (aligns better with "vlan set filter on {port}")? The intellisense provided by the testpmd methods is indeed a QoL improvement for folks writing testsuites, but at the same time people who use testpmd will always have the real commands ingrained in their thoughts, so if we try to stay as true to those as possible, we get the stability and intellisense and also the method names are still intuitive. Maybe even think tiny things like renaming the method set_forward_mode to set_fwd_mode to align 1:1 with testpmd is good. That's just my perspective though - I would be interested to see whether others feel the same or not. > + if verify: > + if "Invalid port" in filter_cmd_output: > + 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: > + self._logger.debug(f"Failed to disable vlan filter: \n{f= ilter_cmd_output}") > + raise InteractiveCommandExecutionError("Testpmd failed t= o disable vlan filter.") > + > + def rx_vlan_add(self, vlan: int =3D 0, port: int =3D 0, verify: bool= =3D True): > + """Add specified vlan tag to the filter list on a port. > + > + Args: > + vlan: The vlan tag to add, should be within 1-4094. Worth clarifying that 4094 is the extended vlan range? Normal range is 1-10= 05? > + > + def port_stop_all(self): > + """Stop all ports.""" > + self.send_command("port stop all") So the idea is to have distinct port_stop_all() and port_stop(self, port: int) methods? I think this is reasonable. > + > + def port_start_all(self): > + """Start all ports.""" > + self.send_command("port start all") > + > + def tx_vlan_set(self, port: int =3D 0, vlan: int =3D 0, verify: bool= =3D True): > + """Set hardware insertion of vlan tags in packets sent on a port= . > + > + Args: > + port: The port number to use, should be within 0-32. > + vlan: The vlan tag to insert, should be within 1-4094. > + verify: If :data:`True`, the output of the command is scanne= d to verify that > + vlan insertion 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_insert_output =3D self.send_command(f"tx_vlan set {port} {v= lan}") > + if verify: > + if ("Please stop port" in vlan_insert_output or "Invalid vla= n_id" in vlan_insert_output > + or "Invalid port" in vlan_insert_output): > + self._logger.debug(f"Failed to set vlan insertion tag: \= n{vlan_insert_output}") > + raise InteractiveCommandExecutionError("Testpmd failed t= o set vlan insertion tag.") > + > + def tx_vlan_reset(self, port: int =3D 0, verify: bool =3D True): > + """Disable hardware insertion of vlan tags in packets sent on a = 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 insertion 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_insert_output =3D self.send_command(f"tx_vlan set {port}") > + if verify: > + if "Please stop port" in vlan_insert_output or "Invalid port= " in vlan_insert_output: > + self._logger.debug(f"Failed to reset vlan insertion: \n{= vlan_insert_output}") > + raise InteractiveCommandExecutionError("Testpmd failed t= o reset vlan insertion.") Could possibly be combined with tx_vlan_set() as one method which handles both set and reset, but I think splitting them out is fine. > + > def close(self) -> None: > """Overrides :meth:`~.interactive_shell.close`.""" > self.send_command("quit", "") > -- > 2.44.0 >