[ofa-general] Re: [PATCH] opensm/osm_console.c : Added getguid function to console to generate a list of guid matching one or more regexps

[Sasha Khapyorsky]

Hi Nicolas,

Some initial comments...

On 09:26 Mon 09 Feb     , Nicolas Morey Chaisemartin wrote:
> This add a getguid functionnality to openSM console which makes it really 
> easy to generate cn_guid_file, root_guid_file and such
> by dumping into a file all port guids whom nodedesc contains at least one 
> of the provided regexps

I see that this specific command is about port guids and not node guids.
What is about better name such "dump_portguids"? (Another possibility
would be implementation of single "dump" command with various parameters
such as "config", "portguids", "nodeguids", etc.).

>
> Signed-off-by: Nicolas Morey-Chaisemartin 
> <nicolas.morey-chaisemartin at ext.bull.net>
> ---
>  opensm/opensm/osm_console.c |  131 
> +++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 131 insertions(+), 0 deletions(-)
>
>

> diff --git a/opensm/opensm/osm_console.c b/opensm/opensm/osm_console.c
> index c6e8e59..e4dc6e9 100644
> --- a/opensm/opensm/osm_console.c
> +++ b/opensm/opensm/osm_console.c
> @@ -42,6 +42,7 @@
>  #include <sys/types.h>
>  #include <sys/socket.h>
>  #include <netdb.h>
> +#include <regex.h>
>  #ifdef ENABLE_OSM_CONSOLE_SOCKET
>  #include <arpa/inet.h>
>  #endif
> @@ -1172,6 +1173,135 @@ static void version_parse(char **p_last, osm_opensm_t * p_osm, FILE * out)
>  	fprintf(out, "%s build %s %s\n", p_osm->osm_version, __DATE__, __TIME__);
>  }
>  
> +typedef struct _regexp_list {
> +       regex_t exp;
> +       struct _regexp_list* next;
> +} regexp_list_t;
> +
> +
> +static void getguid_parse(char **p_last, osm_opensm_t *p_osm, FILE *out)
> +{
> +	cl_qmap_t *p_port_guid_tbl;
> +	osm_port_t* p_port;
> +	osm_port_t* p_next_port;
> +
> +	regexp_list_t* p_head_regexp=NULL;
> +	regexp_list_t* p_regexp;
> +	
> +	/* Option variables*/
> +	char* p_cmd=NULL;
> +	FILE* output=out;
> +	int exit_after_run=0;
> +	extern volatile unsigned int osm_exit_flag;
> +
> +	/* Read commande line */
> +
> +	while(1){

Try opensm/osm_indent (many places in the patch will be affected).

> +		p_cmd = next_token(p_last);
> +		if (p_cmd) {
> +			if (strcmp(p_cmd, "exit_after_run") == 0) {
> +				exit_after_run = 1;
> +			} else if (strcmp(p_cmd, "file") == 0) {
> +				p_cmd=next_token(p_last);
> +				if(p_cmd){
> +					output = fopen(p_cmd,"w+");
> +					if(output == NULL){
> +						fprintf(out,"Could not open file %s: %s\n",p_cmd,strerror(errno));
> +						output = out;
> +					}
> +				} else {
> +					/* No file name passed */
> +					fprintf(out,"No file name passed\n");
> +				}
> +			} else {
> +				p_regexp = malloc(sizeof(*p_regexp));
> +				if(regcomp(&(p_regexp->exp),p_cmd,REG_NOSUB|REG_EXTENDED)!=0){
> +					fprintf(out,"Couldn't parse regular expression %s. Skipping it.\n",p_cmd);
> +				}
> +				p_regexp->next = p_head_regexp;
> +				p_head_regexp = p_regexp;
> +			}
> +		} else {
> +			/* No more tokens */
> +			break;
> +		}

Here and in other places - no need braces about single operation.

> +	}
> +
> +	/* Check we have at least one expression to match */
> +	if(p_head_regexp == NULL){
> +		fprintf(out,"No valid expression provided. Aborting\n");
> +		return;
> +	}
> +
> +	/* Ensure this SM is master (so we have the LFT) */
> +
> + getguid_wait_init:
> +	if(osm_exit_flag)
> +		return;
> +	cl_spinlock_acquire(&p_osm->sm.state_lock);
> +	/* If the subnet struct is not properly initialized, we exit */
> +	if(p_osm->sm.p_subn == NULL){
> +	  cl_spinlock_release(&p_osm->sm.state_lock);
> +	  sleep(1);
> +	  goto getguid_wait_init;
> +	}

The console is initialized after osm_subnet. When will the case
(p_osm->sm.p_subn == NULL) be valid?

> +	if(p_osm->sm.p_subn->sm_state != IB_SMINFO_STATE_MASTER){
> +	  cl_spinlock_release(&p_osm->sm.state_lock);
> +	  sleep(1);
> +	  goto getguid_wait_init;
> +	}

This will cause to endless loop when OpenSM is in Standby or Inactive
states.

> +	cl_spinlock_release(&p_osm->sm.state_lock);
> +	if(p_osm->sm.p_subn->need_update != 0){
> +	  sleep(1);
> +	  goto getguid_wait_init;
> +	}

Subnet discovery/setup could take some time. An user may want to use
console for other things in this time. I don't think that sleeping is
suitable here, better to print "try later" message or like this.

> +
> +	/* Subnet doesn't need to be updated so we can carry on */
> +
> +
> +	CL_PLOCK_EXCL_ACQUIRE(p_osm->sm.p_lock);
> +	p_port_guid_tbl = &(p_osm->sm.p_subn->port_guid_tbl);
> +
> +
> +

No need more than one empty line as separator (osm_indent... :)).

> +	p_next_port = (osm_port_t*)cl_qmap_head(p_port_guid_tbl);
> +	while (p_next_port != (osm_port_t*)cl_qmap_end(p_port_guid_tbl)) {
> +
> +		p_port = p_next_port;
> +		p_next_port = (osm_port_t*)cl_qmap_next(&p_next_port->map_item);
> +
> +		for(p_regexp = p_head_regexp;p_regexp!=NULL;p_regexp = p_regexp->next){
> +			if(regexec(&(p_regexp->exp),p_port->p_node->print_desc,0,NULL,0) == 0){
> +				fprintf(output,"0x%"PRIxLEAST64"\n",cl_ntoh64(p_port->p_physp->port_guid));
> +			}
> +		}
> +	}
> +	
> +CL_PLOCK_RELEASE(p_osm->sm.p_lock);
> +	if(output != out)
> +		fclose(output);
> +	if(exit_after_run)
> +		osm_exit_flag = 1;

Why this 'exit_after_run'?

If you need functionality to exit OpenSM triggered from console (but it
is not clear for me why) use another command.

> +
> +}
> +
> +
> +
> +

No need more than one empty line as separator (osm_indent... :)).

> +static void help_getguid(FILE * out, int detail)
> +{
> +	fprintf(out, "getguid [exit_after_run|file filename] regexp1 [regexp2 [regexp3 ...]] -- Dump port GUID matching a regexp \n");
> +	if (detail) {
> +		fprintf(out,
> +			"getguid -- Dump all the port GUID whom node_desc matches one of the provided regexp\n");
> +		fprintf(out,
> +			"   [file filename] -- Send the port GUID list to the specified file instead of regular output\n");
> +		fprintf(out,
> +			 "   [exit_after_run] -- Quit OpenSM once the port GUID have been displayed\n");
> +	}
> +
> +}
> +
>  /* more parse routines go here */
>  
>  static const struct command console_cmds[] = {
> @@ -1192,6 +1322,7 @@ static const struct command console_cmds[] = {
>  #ifdef ENABLE_OSM_PERF_MGR
>  	{"perfmgr", &help_perfmgr, &perfmgr_parse},
>  #endif				/* ENABLE_OSM_PERF_MGR */
> +	{"getguid", &help_getguid, &getguid_parse},
>  	{NULL, NULL, NULL}	/* end of array */
>  };
>  
>