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 > <[email protected]> > --- > 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 */ > }; > > _______________________________________________ general mailing list [email protected] http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
