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

[Nicolas Morey-Chaisemartin]

Sasha Khapyorsky a écrit :
> 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.).
>
>   
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
>
> 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.
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.
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 ...



Nicolas