[ofa-general] Re: [Infiniband-Diags] [PATCH] saquery exit with non-zero code on bad input

[Al Chu]

Hey Sasha,

On Thu, 2008-04-03 at 18:25 +0000, Sasha Khapyorsky wrote:
> Hi Al,
> 
> On 11:29 Tue 01 Apr     , Al Chu wrote:
> > 
> > If an input into saquery isn't found, saquery still exits with '0'
> > status, so it poses a problem in scripting.
> > 
> > This patch exits w/ non-zero if the input isn't found by saquery.
> 
> I guess by input you mean "SA records". Right?

When the user inputs a nodename, lid, or guid, normally for a noderecord
info query (-N).

> > The actual status code I selected to return can be revised.  I just sort
> > of picked one.
> 
> This patch cares only about print_node_records()? What about other
> queries?

As far as I can tell, most of the other queries do result in a non-zero
exit code already when an input isn't found.  

wopri at root:./saquery --src-to-dst fake:fake; echo $?
Failed to find lid for "fake"
Failed to find lid for "fake"
Path record for fake -> fake
50

A little more playing around suggests there are some queries that also
have issues.

wopri at root:./saquery -x fakename; echo $?
Failed to find lid for "fakename"
LinkRecord dump:
                FromLID....................17
<snip>
                FromPort...................1
                ToPort.....................1
                ToLID......................11
0

I suppose we should handle this one in a different patch.

> > Signed-off-by: Albert L. Chu <chu11 at llnl.gov>
> > ---
> >  infiniband-diags/src/saquery.c |   13 +++++++++++++
> >  1 files changed, 13 insertions(+), 0 deletions(-)
> > 
> > diff --git a/infiniband-diags/src/saquery.c b/infiniband-diags/src/saquery.c
> > index ed61721..f801385 100644
> > --- a/infiniband-diags/src/saquery.c
> > +++ b/infiniband-diags/src/saquery.c
> > @@ -839,6 +839,7 @@ print_node_records(osm_bind_handle_t bind_handle)
> >  	ib_node_record_t *node_record = NULL;
> >  	ib_net16_t        attr_offset = ib_get_attr_offset(sizeof(*node_record));
> >  	ib_api_status_t   status;
> > +	unsigned int      output_count = 0;
> >  
> >  	status  = get_all_records(bind_handle, IB_MAD_ATTR_NODE_RECORD, attr_offset, 0);
> >  	if (status != IB_SUCCESS)
> > @@ -855,12 +856,14 @@ print_node_records(osm_bind_handle_t bind_handle)
> >  		} else if (node_print_desc == NAME_OF_LID) {
> >  			if (requested_lid == cl_ntoh16(node_record->lid)) {
> >  				print_node_record(node_record);
> > +				output_count++;
> >  			}
> >  		} else if (node_print_desc == NAME_OF_GUID) {
> >  			ib_node_info_t *p_ni = &(node_record->node_info);
> >  
> >  			if (requested_guid == cl_ntoh64(p_ni->port_guid)) {
> >  				print_node_record(node_record);
> > +				output_count++;
> >  			}
> >  		} else {
> >  			if (!requested_name ||
> > @@ -868,6 +871,7 @@ print_node_records(osm_bind_handle_t bind_handle)
> >  				     (char *)node_record->node_desc.description,
> >  				     sizeof(node_record->node_desc.description)) == 0)) {
> >  				print_node_record(node_record);
> > +				output_count++;
> >  				if (node_print_desc == UNIQUE_LID_ONLY) {
> >  					return_mad();
> >  					exit(0);
> > @@ -876,6 +880,15 @@ print_node_records(osm_bind_handle_t bind_handle)
> >  		}
> >  	}
> >  	return_mad();
> > +	if ((requested_lid_flag
> > +	     || requested_guid_flag
> > +	     || requested_name)
> > +	    && !output_count) {
> > +		/* need non-zero error code to indicate input not matched.
> > +		 * this seems as good as any other status error code.
> > +		 */ 
> > +		status = IB_NOT_FOUND;
> > +	}
> >  	return (status);
> 
> What about just to 'return result.status' here?

If the user input a string/lid/guid that doesn't exist in the fabric,
print_node_records() can still return 0 b/c the current status is based
solely on the success of the call to get_all_records(), not on whether
the user's input was found or not.

Al


> Sasha
-- 
Albert Chu
chu11 at llnl.gov
925-422-5311
Computer Scientist
High Performance Systems Division
Lawrence Livermore National Laboratory