[PATCH 1/1] opensm/main.c: opensm cannot be killed while asking for port guid

[PATCH 1/1] opensm/main.c: opensm cannot be killed while asking for port guid

[Michael Reed]

opensm enters an uninterruptible loop when the user enters
"-g 0" on the command line.  The only way to kill opensm is to
background the process and send "kill -9".

This patch provides the user an out in get_port_guid() by
introducing "0" as a valid selection.  This patch also changes
the "Lame choice!" error message to something a bit more, uh,
"professional".  main() is modified to test the return code.
This was probably a bug as a return of 0 is returned under a
number of different circumstances by get_port_guid().

Applies to 1.15 RC2.

Signed-off-by: Michael Reed <mdr-sJ/iWh9BUns@public.gmane.org>


--- /tmp/OFED-1.5-rc2/main.c	2009-11-06 08:56:59.089100487 -0800
+++ opensm/main.c	2009-11-06 09:42:34.698963811 -0800
@@ -434,15 +434,19 @@ static ib_net64_t get_port_guid(IN osm_o
 			       i + 1, cl_ntoh64(attr_array[i].port_guid),
 			       attr_array[i].lid,
 			       ib_get_port_state_str(attr_array[i].link_state));
-		printf("\nEnter choice (1-%u): ", i);
+		printf("\n\t0: Exit\n");
+		printf("\nEnter choice (0-%u): ", i);
 		fflush(stdout);
 		if (scanf("%u", &choice) <= 0) {
 			char junk[128];
 			if (scanf("%s", junk) <= 0)
 				printf("\nError: Cannot scan!\n");
-		} else if (choice && choice <= num_ports)
+		}
+		else if (choice == 0)
+			return (0);
+		else if (choice <= num_ports)
 			break;
-		printf("\nError: Lame choice!\n");
+		printf("\nError: Please try again.\n");
 	}
 	choice--;
 	printf("Choice guid=0x%" PRIx64 "\n",
@@ -1039,6 +1043,9 @@ int main(int argc, char *argv[])
 	if (opt.guid == 0 || cl_hton64(opt.guid) == CL_HTON64(INVALID_GUID))
 		opt.guid = get_port_guid(&osm, opt.guid);
 
+	if (opt.guid == 0)
+		goto Exit;
+
 	status = osm_opensm_bind(&osm, opt.guid);
 	if (status != IB_SUCCESS) {
 		printf("\nError from osm_opensm_bind (0x%X)\n", status);
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] opensm/main.c: opensm cannot be killed while asking for port guid

[Sasha Khapyorsky]

On 12:41 Fri 06 Nov     , Michael Reed wrote:
> opensm enters an uninterruptible loop when the user enters
> "-g 0" on the command line.  The only way to kill opensm is to
> background the process and send "kill -9".

The reason is that the port chooser is running in code section where all
signals are blocked. Which is bad in general, but resulted by not
perfect opensm/vendor/complid inter dependencies: sub threads are
activated (complib_init(), osm_opensm_init(), osm_opensm_bind()) with
blocked signals. The port chooser should run in the middle of this chain
after opensm vendor initialization (osm_opensm_init()), but before opensm
vendor binding (osm_opensm_bind()) - it requires a GUID value.

I don't immediately know how to improve this without significant
complib/vendor/opensm interfaces changing.

> This patch provides the user an out in get_port_guid() by
> introducing "0" as a valid selection.  This patch also changes
> the "Lame choice!" error message to something a bit more, uh,
> "professional". main() is modified to test the return code.
> This was probably a bug as a return of 0 is returned under a
> number of different circumstances by get_port_guid().
> 
> Applies to 1.15 RC2.

Please next time generate patch against the current master branch.

> Signed-off-by: Michael Reed <mdr-sJ/iWh9BUns@public.gmane.org>

Applied with minor change (see below). Thanks.

> 
> 
> --- /tmp/OFED-1.5-rc2/main.c	2009-11-06 08:56:59.089100487 -0800
> +++ opensm/main.c	2009-11-06 09:42:34.698963811 -0800
> @@ -434,15 +434,19 @@ static ib_net64_t get_port_guid(IN osm_o
>  			       i + 1, cl_ntoh64(attr_array[i].port_guid),
>  			       attr_array[i].lid,
>  			       ib_get_port_state_str(attr_array[i].link_state));
> -		printf("\nEnter choice (1-%u): ", i);
> +		printf("\n\t0: Exit\n");
> +		printf("\nEnter choice (0-%u): ", i);
>  		fflush(stdout);
>  		if (scanf("%u", &choice) <= 0) {
>  			char junk[128];
>  			if (scanf("%s", junk) <= 0)
>  				printf("\nError: Cannot scan!\n");
> -		} else if (choice && choice <= num_ports)
> +		}
> +		else if (choice == 0)
> +			return (0);
> +		else if (choice <= num_ports)
>  			break;
> -		printf("\nError: Lame choice!\n");
> +		printf("\nError: Please try again.\n");

"Try again" is good, but it is useful to provide a reason too, so let's
do it even more "professional":

	printf("\nError: Lame choice! Please try again.\n");

Sasha

>  	}
>  	choice--;
>  	printf("Choice guid=0x%" PRIx64 "\n",
> @@ -1039,6 +1043,9 @@ int main(int argc, char *argv[])
>  	if (opt.guid == 0 || cl_hton64(opt.guid) == CL_HTON64(INVALID_GUID))
>  		opt.guid = get_port_guid(&osm, opt.guid);
>  
> +	if (opt.guid == 0)
> +		goto Exit;
> +
>  	status = osm_opensm_bind(&osm, opt.guid);
>  	if (status != IB_SUCCESS) {
>  		printf("\nError from osm_opensm_bind (0x%X)\n", status);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] opensm/main.c: opensm cannot be killed while asking for port guid

[Sammy Wilborn]

On Fri, Nov 13, 2009 at 04:55:27AM +0200, Sasha Khapyorsky wrote:
> On 12:41 Fri 06 Nov     , Michael Reed wrote:
> > opensm enters an uninterruptible loop when the user enters
> > "-g 0" on the command line.  The only way to kill opensm is to
> > background the process and send "kill -9".
> 
> The reason is that the port chooser is running in code section where all
> signals are blocked. Which is bad in general, but resulted by not
> perfect opensm/vendor/complid inter dependencies: sub threads are
> activated (complib_init(), osm_opensm_init(), osm_opensm_bind()) with
> blocked signals. The port chooser should run in the middle of this chain
> after opensm vendor initialization (osm_opensm_init()), but before opensm
> vendor binding (osm_opensm_bind()) - it requires a GUID value.
> 
> I don't immediately know how to improve this without significant
> complib/vendor/opensm interfaces changing.
> 
> > This patch provides the user an out in get_port_guid() by
> > introducing "0" as a valid selection.  This patch also changes
> > the "Lame choice!" error message to something a bit more, uh,
> > "professional". main() is modified to test the return code.
> > This was probably a bug as a return of 0 is returned under a
> > number of different circumstances by get_port_guid().
> > 
> > Applies to 1.15 RC2.
> 
> Please next time generate patch against the current master branch.
> 
> > Signed-off-by: Michael Reed <mdr-sJ/iWh9BUns@public.gmane.org>
> 
> Applied with minor change (see below). Thanks.
> 
> > 
> > 
> > --- /tmp/OFED-1.5-rc2/main.c	2009-11-06 08:56:59.089100487 -0800
> > +++ opensm/main.c	2009-11-06 09:42:34.698963811 -0800
> > @@ -434,15 +434,19 @@ static ib_net64_t get_port_guid(IN osm_o
> >  			       i + 1, cl_ntoh64(attr_array[i].port_guid),
> >  			       attr_array[i].lid,
> >  			       ib_get_port_state_str(attr_array[i].link_state));
> > -		printf("\nEnter choice (1-%u): ", i);
> > +		printf("\n\t0: Exit\n");
> > +		printf("\nEnter choice (0-%u): ", i);
> >  		fflush(stdout);
> >  		if (scanf("%u", &choice) <= 0) {
> >  			char junk[128];
> >  			if (scanf("%s", junk) <= 0)
> >  				printf("\nError: Cannot scan!\n");
> > -		} else if (choice && choice <= num_ports)
> > +		}
> > +		else if (choice == 0)
> > +			return (0);
> > +		else if (choice <= num_ports)
> >  			break;
> > -		printf("\nError: Lame choice!\n");
> > +		printf("\nError: Please try again.\n");
> 
> "Try again" is good, but it is useful to provide a reason too, so let's
> do it even more "professional":
> 
> 	printf("\nError: Lame choice! Please try again.\n");

How about the following?

 	printf("\nError: Invalid choice! Please try again.\n");

--Sammy

> 
> Sasha
> 
> >  	}
> >  	choice--;
> >  	printf("Choice guid=0x%" PRIx64 "\n",
> > @@ -1039,6 +1043,9 @@ int main(int argc, char *argv[])
> >  	if (opt.guid == 0 || cl_hton64(opt.guid) == CL_HTON64(INVALID_GUID))
> >  		opt.guid = get_port_guid(&osm, opt.guid);
> >  
> > +	if (opt.guid == 0)
> > +		goto Exit;
> > +
> >  	status = osm_opensm_bind(&osm, opt.guid);
> >  	if (status != IB_SUCCESS) {
> >  		printf("\nError from osm_opensm_bind (0x%X)\n", status);
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] opensm/main.c: opensm cannot be killed while asking for port guid

[Sasha Khapyorsky]

On 19:24 Thu 12 Nov     , Sammy Wilborn wrote:
> > > -		printf("\nError: Lame choice!\n");
> > > +		printf("\nError: Please try again.\n");
> > 
> > "Try again" is good, but it is useful to provide a reason too, so let's
> > do it even more "professional":
> > 
> > 	printf("\nError: Lame choice! Please try again.\n");
> 
> How about the following?
> 
>  	printf("\nError: Invalid choice! Please try again.\n");

This works too. Would you care about the patch?

Sasha
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] opensm/main.c: opensm cannot be killed while asking for port guid

[Michael Reed]

Sasha Khapyorsky wrote:
> On 19:24 Thu 12 Nov     , Sammy Wilborn wrote:
>>>> -		printf("\nError: Lame choice!\n");
>>>> +		printf("\nError: Please try again.\n");
>>> "Try again" is good, but it is useful to provide a reason too, so let's
>>> do it even more "professional":
>>>
>>> 	printf("\nError: Lame choice! Please try again.\n");
>> How about the following?
>>
>>  	printf("\nError: Invalid choice! Please try again.\n");
> 
> This works too. Would you care about the patch?

I think the point is to not upset the customer by suggesting that
they are "lame".  OFED, meet customer spending LOTS of money.  Customer,
meet OFED!  :)  Do you need a patch submitted making that change?

And, thanks for the comment about using the current master branch.
I'll try to do that in the future.  I'm still coming up to speed on
processes and remain mostly "git" illiterate.

Thanks,
 Mike

> 
> Sasha
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] opensm/main.c: opensm cannot be killed while asking for port guid

[Sasha Khapyorsky]

On 12:55 Fri 13 Nov     , Michael Reed wrote:
> 
> > This works too. Would you care about the patch?
> 
> I think the point is to not upset the customer by suggesting that
> they are "lame".

I don't think that our customers can be upset so easily :). Seriously
the original phrase is here for many many years and was introduced by
Mellanox OpenSM production branch merging back, likely they didn't
have a lot of complains about this. On my memory it is the first one.
OTOH it is really minor and IMO feel free to change this.

> OFED, meet customer spending LOTS of money.  Customer,
> meet OFED!  :)

EWG list is a better place to discuss OFED and customer's money.

> Do you need a patch submitted making that change?

I don't care about this. If you are please submit the patch, this is a
normal procedure. Alternatively you can open bug in OFED bugzilla and
wait that somebody will do it for you or do nothing.

Sasha
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html