There are some errors are occured if CLI reqests spp_primary for adding or deleting ports even though no forwarder thread is running. Without the thread, it has no mean to add or delete resources for the thread This update is to add checking if there is a forwarder thread is running, or refuse the request for the thread. It also includes other tiny bug fixes. Yasufumi Ogawa (7): shared: revise vdev prefixs of pcap and nullpmd shared: fix wrong port_type in parsing dev name cli: add filter for running pri commands cli: fix error in asking ports if no pri forwarder cli: fix terminated if spp_primary is not running cli: move logfile to under project log dir cli: fix parsing forward and stop commands src/cli/commands/pri.py | 109 ++++++++++++++++++++++++++++++-------- src/cli/spp_common.py | 19 ++++--- src/cli/spp_ctl_client.py | 15 +++--- src/shared/common.c | 18 +++---- src/shared/common.h | 4 +- 5 files changed, 118 insertions(+), 47 deletions(-) -- 2.17.1
In DPDK, there are two prefixes of RTE eth device, 'eth_' and 'net_' fo r a reason of compatibility. 'eth_' is old one. SPP defines all of prefixes as following. #define VDEV_ETH_VHOST "eth_vhost" #define VDEV_NET_VHOST "net_vhost" ... However, pcap and nullpmd have only 'eth_' or 'net_'. SPP defines them such as VDEV_NULL for simplicity, but it might be confusing for considering the naming rule. This update is revise the prefixes to avoid this ambiguity. Signed-off-by: Yasufumi Ogawa <yasufum.o@gmail.com> --- src/shared/common.c | 16 ++++++++-------- src/shared/common.h | 4 ++-- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/shared/common.c b/src/shared/common.c index 3145617..a098aed 100644 --- a/src/shared/common.c +++ b/src/shared/common.c @@ -99,11 +99,11 @@ int parse_dev_name(char *dev_name, int *port_type, int *port_id) *port_id = (int)strtol(pid_str, NULL, 10); *port_type = VHOST; - } else if (strncmp(dev_name, VDEV_PCAP, - strlen(VDEV_PCAP)) == 0) { - dev_str_len = strlen(VDEV_PCAP); + } else if (strncmp(dev_name, VDEV_NET_PCAP, + strlen(VDEV_NET_PCAP)) == 0) { + dev_str_len = strlen(VDEV_NET_PCAP); pid_len = dev_name_len - dev_str_len; - strncpy(pid_str, dev_name + strlen(VDEV_PCAP), + strncpy(pid_str, dev_name + strlen(VDEV_NET_PCAP), pid_len); *port_id = (int)strtol(pid_str, NULL, 10); *port_type = PCAP; @@ -119,11 +119,11 @@ int parse_dev_name(char *dev_name, int *port_type, int *port_id) *port_id = (int)strtol(pid_str, NULL, 10); *port_type = TAP; - } else if (strncmp(dev_name, VDEV_NULL, - strlen(VDEV_NULL)) == 0) { - dev_str_len = strlen(VDEV_NULL); + } else if (strncmp(dev_name, VDEV_ETH_NULL, + strlen(VDEV_ETH_NULL)) == 0) { + dev_str_len = strlen(VDEV_ETH_NULL); pid_len = dev_name_len - dev_str_len; - strncpy(pid_str, dev_name + strlen(VDEV_NULL), + strncpy(pid_str, dev_name + strlen(VDEV_ETH_NULL), pid_len); *port_id = (int)strtol(pid_str, NULL, 10); *port_type = PCAP; diff --git a/src/shared/common.h b/src/shared/common.h index 431ad3e..ef8372a 100644 --- a/src/shared/common.h +++ b/src/shared/common.h @@ -36,10 +36,10 @@ #define VDEV_NET_RING "net_ring" #define VDEV_ETH_VHOST "spp_vhost" #define VDEV_NET_VHOST "net_vhost" -#define VDEV_PCAP "net_pcap" +#define VDEV_NET_PCAP "net_pcap" #define VDEV_ETH_TAP "eth_tap" #define VDEV_NET_TAP "net_tap" -#define VDEV_NULL "eth_null" +#define VDEV_ETH_NULL "eth_null" /* Command. */ -- 2.17.1
Correct parse_dev_name() which returns port_type PCAP for eth_null wrongly because of a typo. This patch is to fix to return NULLPMD correctly. Fixes: 146516c139c2 ("shared: add parsing ethdev name") Signed-off-by: Yasufumi Ogawa <yasufum.o@gmail.com> --- src/shared/common.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/shared/common.c b/src/shared/common.c index a098aed..85e2f06 100644 --- a/src/shared/common.c +++ b/src/shared/common.c @@ -126,7 +126,7 @@ int parse_dev_name(char *dev_name, int *port_type, int *port_id) strncpy(pid_str, dev_name + strlen(VDEV_ETH_NULL), pid_len); *port_id = (int)strtol(pid_str, NULL, 10); - *port_type = PCAP; + *port_type = NULLPMD; /* TODO(yasufum) add checking invalid port type and return -1 */ } else { -- 2.17.1
For spp_primary, some of methods such as _run_add(), _run_del() or so, should not be executed if forwarder thread is not running. This update is to add util function for checking the forwarder is running, and refuse required operation if the forwarder is not running. Signed-off-by: Yasufumi Ogawa <yasufum.o@gmail.com> --- src/cli/commands/pri.py | 59 ++++++++++++++++++++++++++++++++--------- 1 file changed, 46 insertions(+), 13 deletions(-) diff --git a/src/cli/commands/pri.py b/src/cli/commands/pri.py index 9212897..1443e9e 100644 --- a/src/cli/commands/pri.py +++ b/src/cli/commands/pri.py @@ -46,6 +46,20 @@ class SppPrimary(object): temp = temp + "{vhost_cli}" self.launch_template = temp + def _do_if_forwarder_exists(self, status, func, params): + """Execute command of func if forwarder thread is existing. + + Params status is a json object, func is a method of this class such + as _run_add() or _run_del() or so, and params is a set of given + parameters. + """ + + # It includes `forwarder` if spp_primary runs a forwarder thread. + if 'forwarder' in status: + func(params) + else: + print('No forwarder is running.') + def run(self, cmd, cli_config): """Called from do_pri() to Send command to primary process.""" @@ -60,11 +74,15 @@ class SppPrimary(object): # use short name common_err_codes = self.spp_ctl_cli.rest_common_error_codes + # Get status here for inspecting if forwarder exists. Do not run + # command such as `add` or `del` if forwarder does not exist. + res = self.spp_ctl_cli.get('primary/status') + status = res.json() + if subcmd == 'status': - res = self.spp_ctl_cli.get('primary/status') if res is not None: if res.status_code == 200: - self.print_status(res.json()) + self.print_status(status) elif res.status_code in common_err_codes: # Print default error message pass @@ -72,16 +90,17 @@ class SppPrimary(object): print('Error: unknown response from status.') elif subcmd == 'add': - self._run_add(params) + self._do_if_forwarder_exists(status, self._run_add, params) elif subcmd == 'del': - self._run_del(params) + self._do_if_forwarder_exists(status, self._run_del, params) elif subcmd == 'forward' or cmd == 'stop': - self._run_forward_or_stop(cmd) + self._do_if_forwarder_exists(status, + self._run_forward_or_stop, params) elif subcmd == 'patch': - self._run_patch(params) + self._do_if_forwarder_exists(status, self._run_patch, params) elif subcmd == 'launch': wait_time = float(cli_config['sec_wait_launch']['val']) @@ -217,8 +236,10 @@ class SppPrimary(object): temp = '{s6}{rid:2} {rx:10} {tx:10} {rx_d:10} {tx_d:10}' for rports in json_obj['ring_ports']: print(temp.format(s6=sep*6, - rid=rports['id'], rx=rports['rx'], tx=rports['tx'], - rx_d=rports['rx_drop'], tx_d=rports['tx_drop'])) + rid=rports['id'], + rx=rports['rx'], tx=rports['tx'], + rx_d=rports['rx_drop'], + tx_d=rports['tx_drop'])) except KeyError as e: logger.error('{} is not defined!'.format(e)) @@ -233,7 +254,11 @@ class SppPrimary(object): if res is not None: error_codes = self.spp_ctl_cli.rest_common_error_codes if res.status_code == 200: - return res.json()['forwarder']['ports'] + if 'forwarder' in res.json(): + return res.json()['forwarder']['ports'] + else: + # Do nothing if there is no forwarder + pass elif res.status_code in error_codes: pass else: @@ -251,7 +276,11 @@ class SppPrimary(object): if res is not None: error_codes = self.spp_ctl_cli.rest_common_error_codes if res.status_code == 200: - return res.json()['forwarder']['patches'] + if 'forwarder' in res.json(): + return res.json()['forwarder']['patches'] + else: + # Do nothing if there is no forwarder + pass elif res.status_code in error_codes: pass else: @@ -270,9 +299,13 @@ class SppPrimary(object): if res is not None: error_codes = self.spp_ctl_cli.rest_common_error_codes if res.status_code == 200: - ports = res.json()['forwarder']['ports'] - patches = res.json()['forwarder']['patches'] - return ports, patches + if 'forwarder' in res.json(): + ports = res.json()['forwarder']['ports'] + patches = res.json()['forwarder']['patches'] + return ports, patches + else: + # Do nothing if there is no forwarder + pass elif res.status_code in error_codes: pass else: -- 2.17.1
SPP CLI retrieves status of spp_primary not only for `status` command but also others, such as `add` or `del`, to check the result of the commands. For the commands other than `status`, SPP CLI is terminated if it retrieves the status and forwarder thread is not running because SPP CLI expects to receive the status including 'forwarder' attribute, but does not exist. This update is to fix the issue. SPP CLI tells you that forwarder thread is not running for the cases. Signed-off-by: Yasufumi Ogawa <yasufum.o@gmail.com> --- src/cli/commands/pri.py | 36 ++++++++++++++++++++++++++++++------ 1 file changed, 30 insertions(+), 6 deletions(-) diff --git a/src/cli/commands/pri.py b/src/cli/commands/pri.py index 1443e9e..f7d86bb 100644 --- a/src/cli/commands/pri.py +++ b/src/cli/commands/pri.py @@ -263,6 +263,7 @@ class SppPrimary(object): pass else: print('Error: unknown response for get_ports.') + return None def _get_patches(self): """Get all of patched ports as a list of dicts. @@ -285,6 +286,7 @@ class SppPrimary(object): pass else: print('Error: unknown response for get_patches.') + return None def _get_ports_and_patches(self): """Get all of ports and patches at once. @@ -309,7 +311,8 @@ class SppPrimary(object): elif res.status_code in error_codes: pass else: - print('Error: unknown response 3.') + print('Error: unknown response.') + return None, None def _get_patched_ports(self): """Get all of patched ports as a list. @@ -619,8 +622,16 @@ class SppPrimary(object): if len(sub_tokens) < 3: tmp_ary = [] + # Update to current status self.ports, self.patches = self._get_ports_and_patches() + if self.ports is None: + self.ports, self.patches = [], [] + return [] + elif self.patches is None: + self.ports, self.patches = [], [] + return [] + # Patched ports should not be included in the candidate of del. patched_ports = self._get_patched_ports() @@ -635,7 +646,7 @@ class SppPrimary(object): else: tmp_ary.append(kw) - # Physical port cannot be removed. + # Exclude phy ports which cannot be deleted. for p in tmp_ary: if not p.startswith('phy:'): res.append(p) @@ -653,6 +664,13 @@ class SppPrimary(object): self.ports, self.patches = self._get_ports_and_patches() + if self.ports is None: + self.ports, self.patches = [], [] + return [] + elif self.patches is None: + self.ports, self.patches = [], [] + return [] + # Get patched ports of src and dst to be used for completion. src_ports = [] dst_ports = [] @@ -735,8 +753,6 @@ class SppPrimary(object): elif params[0] in self.ports: print("'%s' is already added." % params[0]) else: - self.ports = self._get_ports() - req_params = {'action': 'add', 'port': params[0]} res = self.spp_ctl_cli.put('primary/ports', req_params) @@ -749,6 +765,11 @@ class SppPrimary(object): else: print('Error: unknown response for add.') + self.ports = self._get_ports() # update to current status + if self.ports is None: + print('Cannot retrieve ports from spp_primary') + self.ports = [] + def _run_del(self, params): """Run `del` command.""" @@ -757,8 +778,6 @@ class SppPrimary(object): elif 'phy:' in params[0]: print("Cannot delete phy port '%s'." % params[0]) else: - self.patches = self._get_patches() - # Patched ports should not be deleted. patched_ports = self._get_patched_ports() @@ -776,6 +795,11 @@ class SppPrimary(object): else: print('Error: unknown response for del.') + self.patches = self._get_patches() # update to current status + if self.patches is None: + print('Cannot retrieve patches from spp_primary') + self.patches = [] + def _run_forward_or_stop(self, cmd): """Run `forward` or `stop` command.""" -- 2.17.1
If there is no spp_primary is running, SPP CLI is terminated when it requests something to spp-ctl. This update is to fix the issue. Signed-off-by: Yasufumi Ogawa <yasufum.o@gmail.com> --- src/cli/commands/pri.py | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/cli/commands/pri.py b/src/cli/commands/pri.py index f7d86bb..b39f911 100644 --- a/src/cli/commands/pri.py +++ b/src/cli/commands/pri.py @@ -77,7 +77,17 @@ class SppPrimary(object): # Get status here for inspecting if forwarder exists. Do not run # command such as `add` or `del` if forwarder does not exist. res = self.spp_ctl_cli.get('primary/status') - status = res.json() + + # Check if spp_primary is running. + error_codes = self.spp_ctl_cli.rest_common_error_codes + if res.status_code in error_codes: + if res.status_code == 404: + print('No spp_primary is running.') + else: + print('Error: spp_primary is not running normaly.') + return None + else: + status = res.json() if subcmd == 'status': if res is not None: -- 2.17.1
SPP CLI outputs log as `spp/cli/log/spp_cli.log` because the contents are mainly about implementation and no need for users. Some error messages for users are just displayed in the CLI. However, it is annoying that users have to remove the error message for failing completion. SPP CLI has got stabled and almost no need to output tiny logs of implementation. It is the time to move CLI's log to `spp/log` as similar to other processes. This update is to move logfile to `spp/log/spp_cli.log`, and to show the error messages in the logfile. Signed-off-by: Yasufumi Ogawa <yasufum.o@gmail.com> --- src/cli/spp_common.py | 19 +++++++++++-------- src/cli/spp_ctl_client.py | 15 ++++++++------- 2 files changed, 19 insertions(+), 15 deletions(-) diff --git a/src/cli/spp_common.py b/src/cli/spp_common.py index b717fb0..947a2fa 100644 --- a/src/cli/spp_common.py +++ b/src/cli/spp_common.py @@ -9,21 +9,24 @@ import os PORT_TYPES = ['phy', 'ring', 'vhost', 'pcap', 'nullpmd'] SEC_TYPES = ['nfv', 'vf', 'mirror', 'pcap'] -LOGFILE = 'spp_cli.log' # name of logfile under `/src/controller/log/` +# Setup logger object +# Logfile is generated as 'spp/log/spp_cli.log'. +log_filename = 'spp_cli.log' # name of logfile +logdir = '{}/../../log'.format(os.path.dirname(__file__)) +logfile = '{}/{}'.format(logdir, log_filename) -# Current server under management of SPP CLI. -cur_server_addr = None +os.system('mkdir -p {}'.format(logdir)) -# Setup logger object -logger = logging.getLogger(__name__) # handler = logging.StreamHandler() -os.system("mkdir -p %s/log" % (os.path.dirname(__file__))) - -logfile = '%s/log/%s' % (os.path.dirname(__file__), LOGFILE) handler = logging.FileHandler(logfile) handler.setLevel(logging.DEBUG) formatter = logging.Formatter( '%(asctime)s,[%(filename)s][%(name)s][%(levelname)s]%(message)s') handler.setFormatter(formatter) + +logger = logging.getLogger(__name__) logger.setLevel(logging.DEBUG) logger.addHandler(handler) + +# Current server under management of SPP CLI. +cur_server_addr = None diff --git a/src/cli/spp_ctl_client.py b/src/cli/spp_ctl_client.py index ff95136..7418353 100644 --- a/src/cli/spp_ctl_client.py +++ b/src/cli/spp_ctl_client.py @@ -2,6 +2,7 @@ # SPDX-License-Identifier: BSD-3-Clause # Copyright(c) 2018 Nippon Telegraph and Telephone Corporation +from .spp_common import logger import requests @@ -29,17 +30,17 @@ class SppCtlClient(object): # TODO(yasufum) revise print message to more appropriate # for spp.py. if res.status_code == 400: - print('Syntax or lexical error, or SPP returns ' + - 'error for the request.') + logger.info('Syntax or lexical error, or SPP ' + 'returns error for the request.') elif res.status_code == 404: - print('URL is not supported, or no SPP process ' + - 'of client-id in a URL.') + logger.info('URL is not supported, or no SPP ' + 'process of client-id in a URL.') elif res.status_code == 500: - print('System error occured in spp-ctl.') + logger.info('System error occured in spp-ctl.') return res except requests.exceptions.ConnectionError: - print('Error: Failed to connect to spp-ctl.') + logger.info('Error: Failed to connect to spp-ctl.') return None return wrapper @@ -84,7 +85,7 @@ class SppCtlClient(object): if ent['type'] == ptype: ids.append(ent['client-id']) except KeyError as e: - print('Error: {} is not defined!'.format(e)) + logger.info('Error: {} is not defined!'.format(e)) return ids def get_sec_procs(self, ptype): -- 2.17.1
This update is to fix a bug in which _run_forward_or_stop() expects a str type argument, but passed a list. Fixes: 17c6d451fa60 ("cli: add filter for running pri commands") Signed-off-by: Yasufumi Ogawa <yasufum.o@gmail.com> --- src/cli/commands/pri.py | 6 +++--- src/cli/spp_ctl_client.py | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/cli/commands/pri.py b/src/cli/commands/pri.py index b39f911..a13137d 100644 --- a/src/cli/commands/pri.py +++ b/src/cli/commands/pri.py @@ -105,9 +105,9 @@ class SppPrimary(object): elif subcmd == 'del': self._do_if_forwarder_exists(status, self._run_del, params) - elif subcmd == 'forward' or cmd == 'stop': + elif subcmd == 'forward' or subcmd == 'stop': self._do_if_forwarder_exists(status, - self._run_forward_or_stop, params) + self._run_forward_or_stop, subcmd) elif subcmd == 'patch': self._do_if_forwarder_exists(status, self._run_patch, params) @@ -818,7 +818,7 @@ class SppPrimary(object): elif cmd == 'stop': req_params = {'action': 'stop'} else: - print('Unknown command. "forward" or "stop"?') + print('Unknown command {}. "forward" or "stop"?'.format(cmd)) res = self.spp_ctl_cli.put('primary/forward', req_params) diff --git a/src/cli/spp_ctl_client.py b/src/cli/spp_ctl_client.py index 7418353..7b366b2 100644 --- a/src/cli/spp_ctl_client.py +++ b/src/cli/spp_ctl_client.py @@ -31,10 +31,10 @@ class SppCtlClient(object): # for spp.py. if res.status_code == 400: logger.info('Syntax or lexical error, or SPP ' - 'returns error for the request.') + 'returns error for the request.') elif res.status_code == 404: logger.info('URL is not supported, or no SPP ' - 'process of client-id in a URL.') + 'process of client-id in a URL.') elif res.status_code == 500: logger.info('System error occured in spp-ctl.') -- 2.17.1