[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]

On 17:04 Mon 09 Feb     , Nicolas Morey-Chaisemartin wrote:
> >   
> Dumping port guid is specially useful to generate config files.  I've
> never had the need to dump nodeguid. If people need it, why not make a
> global dump.
> If not, it may be simpler to rename to dump_portguids

Sure, we can start this way.

> >
> > Try opensm/osm_indent (many places in the patch will be affected).
> >
> >   
> Last time I tried osm_indent, it introduced a real lot of changes to the
> code (even the one I didn't edited) so I haven't used it on my patches.

You can extract related changes by editing diff file. In any case
osm_indent will let you idea about how code should be formatted.

> I'll fix the indentation.
> >> +	/* 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?
> >
> >   
> I didn't knew that, I was just checking my pointers to be sure.
> >> +	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.
> >
> >   
> This is some code I used for another function that looks at LFT table.

It is not in a main stream, right?

> In the other case, I need the SM to be master.
> I'll change it.
> >> +	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.
> >
> >   
> See comment below
> >
> >> +	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.
> >
> >   
> 
> For the last 2 comments, the purpose is to be able to easily script the
> configuration file generation. We have netlist generation here and it's
> much easier to be able to just do
> echo "getguid exit_after_run file $dir/root_guid_file.txt root_sw" |
> opensm ...

Hmm, OpenSM main purpose is much different than just fabric statistics
dumps generation :). If the only thing you need is port guids list you
can parse 'ibnetdiscover' output - it will be much faster and not
destructive (you even can find some trivial script in ibsim tree -
'tests/get_all_ca_port_guids.sh').

And in any case "two command approach" can work via pipe too:

( echo "getguid exit_after_run file $dir/root_guid_file.txt root_sw" ; \
  echo "exit_opensm" ) | opensm ...

Sasha