Re: [PATCH] xen-netback: correct return value checks on xenbus_scanf()

Re: [PATCH] xen-netback: correct return value checks on xenbus_scanf()

[Paul Durrant]

> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of Jan
> Beulich
> Sent: 07 July 2016 08:57
> To: Wei Liu
> Cc: xen-devel@lists.xenproject.org; netdev@vger.kernel.org
> Subject: [Xen-devel] [PATCH] xen-netback: correct return value checks on
> xenbus_scanf()
> 
> Only a positive return value indicates success.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Paul Durrant <paul.durrant@citrix.com>

> ---
>  drivers/net/xen-netback/xenbus.c |   26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
> 
> --- 4.7-rc6-xenbus_scanf.orig/drivers/net/xen-netback/xenbus.c
> +++ 4.7-rc6-xenbus_scanf/drivers/net/xen-netback/xenbus.c
> @@ -741,7 +741,7 @@ static void xen_mcast_ctrl_changed(struc
>  	int val;
> 
>  	if (xenbus_scanf(XBT_NIL, dev->otherend,
> -			 "request-multicast-control", "%d", &val) < 0)
> +			 "request-multicast-control", "%d", &val) <= 0)
>  		val = 0;
>  	vif->multicast_control = !!val;
>  }
> @@ -890,7 +890,7 @@ static void connect(struct backend_info
>  	err = xenbus_scanf(XBT_NIL, dev->otherend,
>  			   "multi-queue-num-queues",
>  			   "%u", &requested_num_queues);
> -	if (err < 0) {
> +	if (err <= 0) {
>  		requested_num_queues = 1; /* Fall back to single queue */
>  	} else if (requested_num_queues > xenvif_max_queues) {
>  		/* buggy or malicious guest */
> @@ -1056,7 +1056,7 @@ static int connect_data_rings(struct bac
>  	if (err < 0) {
>  		err = xenbus_scanf(XBT_NIL, xspath,
>  				   "event-channel", "%u", &tx_evtchn);
> -		if (err < 0) {
> +		if (err <= 0) {
>  			xenbus_dev_fatal(dev, err,
>  					 "reading %s/event-channel(-tx/rx)",
>  					 xspath);
> @@ -1092,10 +1092,10 @@ static int read_xenbus_vif_flags(struct
>  	err = xenbus_scanf(XBT_NIL, dev->otherend, "request-rx-copy",
> "%u",
>  			   &rx_copy);
>  	if (err == -ENOENT) {
> -		err = 0;
> +		err = 1;
>  		rx_copy = 0;
>  	}
> -	if (err < 0) {
> +	if (err <= 0) {
>  		xenbus_dev_fatal(dev, err, "reading %s/request-rx-copy",
>  				 dev->otherend);
>  		return err;
> @@ -1104,7 +1104,7 @@ static int read_xenbus_vif_flags(struct
>  		return -EOPNOTSUPP;
> 
>  	if (xenbus_scanf(XBT_NIL, dev->otherend,
> -			 "feature-rx-notify", "%d", &val) < 0)
> +			 "feature-rx-notify", "%d", &val) <= 0)
>  		val = 0;
>  	if (!val) {
>  		/* - Reduce drain timeout to poll more frequently for
> @@ -1116,7 +1116,7 @@ static int read_xenbus_vif_flags(struct
>  	}
> 
>  	if (xenbus_scanf(XBT_NIL, dev->otherend, "feature-sg",
> -			 "%d", &val) < 0)
> +			 "%d", &val) <= 0)
>  		val = 0;
>  	vif->can_sg = !!val;
> 
> @@ -1124,25 +1124,25 @@ static int read_xenbus_vif_flags(struct
>  	vif->gso_prefix_mask = 0;
> 
>  	if (xenbus_scanf(XBT_NIL, dev->otherend, "feature-gso-tcpv4",
> -			 "%d", &val) < 0)
> +			 "%d", &val) <= 0)
>  		val = 0;
>  	if (val)
>  		vif->gso_mask |= GSO_BIT(TCPV4);
> 
>  	if (xenbus_scanf(XBT_NIL, dev->otherend, "feature-gso-tcpv4-
> prefix",
> -			 "%d", &val) < 0)
> +			 "%d", &val) <= 0)
>  		val = 0;
>  	if (val)
>  		vif->gso_prefix_mask |= GSO_BIT(TCPV4);
> 
>  	if (xenbus_scanf(XBT_NIL, dev->otherend, "feature-gso-tcpv6",
> -			 "%d", &val) < 0)
> +			 "%d", &val) <= 0)
>  		val = 0;
>  	if (val)
>  		vif->gso_mask |= GSO_BIT(TCPV6);
> 
>  	if (xenbus_scanf(XBT_NIL, dev->otherend, "feature-gso-tcpv6-
> prefix",
> -			 "%d", &val) < 0)
> +			 "%d", &val) <= 0)
>  		val = 0;
>  	if (val)
>  		vif->gso_prefix_mask |= GSO_BIT(TCPV6);
> @@ -1156,12 +1156,12 @@ static int read_xenbus_vif_flags(struct
>  	}
> 
>  	if (xenbus_scanf(XBT_NIL, dev->otherend, "feature-no-csum-
> offload",
> -			 "%d", &val) < 0)
> +			 "%d", &val) <= 0)
>  		val = 0;
>  	vif->ip_csum = !val;
> 
>  	if (xenbus_scanf(XBT_NIL, dev->otherend, "feature-ipv6-csum-
> offload",
> -			 "%d", &val) < 0)
> +			 "%d", &val) <= 0)
>  		val = 0;
>  	vif->ipv6_csum = !!val;
> 
> 
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] xen-netback: correct return value checks on xenbus_scanf()

[David Vrabel]

On 07/07/16 08:57, Jan Beulich wrote:
> Only a positive return value indicates success.

This is not correct.

David

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] xen-netback: correct return value checks on xenbus_scanf()

[Wei Liu]

On Thu, Jul 07, 2016 at 10:58:16AM +0100, David Vrabel wrote:
> On 07/07/16 08:57, Jan Beulich wrote:
> > Only a positive return value indicates success.
> 
> This is not correct.
> 

Do you mean the commit message is not correct or the code is not
correct? If it is the formal, do you have any suggestion to fix it?

(I was going to just ack this because Paul already reviewed it)

Wei.

> David

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] xen-netback: correct return value checks on xenbus_scanf()

[Paul Durrant]

> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-
> owner@vger.kernel.org] On Behalf Of Wei Liu
> Sent: 07 July 2016 11:35
> To: David Vrabel
> Cc: Jan Beulich; Wei Liu; xen-devel@lists.xenproject.org;
> netdev@vger.kernel.org
> Subject: Re: [Xen-devel] [PATCH] xen-netback: correct return value checks
> on xenbus_scanf()
> 
> On Thu, Jul 07, 2016 at 10:58:16AM +0100, David Vrabel wrote:
> > On 07/07/16 08:57, Jan Beulich wrote:
> > > Only a positive return value indicates success.
> >
> > This is not correct.
> >

If Xen's vsscanf follows the semantics of scanf(3) then 0 is a failure so I think the comment is correct.

  Paul

> 
> Do you mean the commit message is not correct or the code is not
> correct? If it is the formal, do you have any suggestion to fix it?
> 
> (I was going to just ack this because Paul already reviewed it)
> 
> Wei.
> 
> > David

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] xen-netback: correct return value checks on xenbus_scanf()

[Paul Durrant]

> -----Original Message-----
> From: Paul Durrant
> Sent: 07 July 2016 11:41
> To: Wei Liu; David Vrabel
> Cc: Jan Beulich; Wei Liu; xen-devel@lists.xenproject.org;
> netdev@vger.kernel.org
> Subject: RE: [Xen-devel] [PATCH] xen-netback: correct return value checks
> on xenbus_scanf()
> 
> > -----Original Message-----
> > From: netdev-owner@vger.kernel.org [mailto:netdev-
> > owner@vger.kernel.org] On Behalf Of Wei Liu
> > Sent: 07 July 2016 11:35
> > To: David Vrabel
> > Cc: Jan Beulich; Wei Liu; xen-devel@lists.xenproject.org;
> > netdev@vger.kernel.org
> > Subject: Re: [Xen-devel] [PATCH] xen-netback: correct return value checks
> > on xenbus_scanf()
> >
> > On Thu, Jul 07, 2016 at 10:58:16AM +0100, David Vrabel wrote:
> > > On 07/07/16 08:57, Jan Beulich wrote:
> > > > Only a positive return value indicates success.
> > >
> > > This is not correct.
> > >
> 
> If Xen's vsscanf follows the semantics of scanf(3) then 0 is a failure so I think
> the comment is correct.
> 

s/Xen/the kernel/

>   Paul
> 
> >
> > Do you mean the commit message is not correct or the code is not
> > correct? If it is the formal, do you have any suggestion to fix it?
> >
> > (I was going to just ack this because Paul already reviewed it)
> >
> > Wei.
> >
> > > David

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] xen-netback: correct return value checks on xenbus_scanf()

[David Vrabel]

On 07/07/16 11:35, Wei Liu wrote:
> On Thu, Jul 07, 2016 at 10:58:16AM +0100, David Vrabel wrote:
>> On 07/07/16 08:57, Jan Beulich wrote:
>>> Only a positive return value indicates success.
>>
>> This is not correct.
>>
> 
> Do you mean the commit message is not correct or the code is not
> correct? If it is the formal, do you have any suggestion to fix it?

This code is correct as-is, thus the commit message is wrong or misleading.

David

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] xen-netback: correct return value checks on xenbus_scanf()

[Paul Durrant]

> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-
> owner@vger.kernel.org] On Behalf Of David Vrabel
> Sent: 07 July 2016 11:45
> To: Wei Liu; David Vrabel
> Cc: xen-devel@lists.xenproject.org; Jan Beulich; netdev@vger.kernel.org
> Subject: Re: [Xen-devel] [PATCH] xen-netback: correct return value checks
> on xenbus_scanf()
> 
> On 07/07/16 11:35, Wei Liu wrote:
> > On Thu, Jul 07, 2016 at 10:58:16AM +0100, David Vrabel wrote:
> >> On 07/07/16 08:57, Jan Beulich wrote:
> >>> Only a positive return value indicates success.
> >>
> >> This is not correct.
> >>
> >
> > Do you mean the commit message is not correct or the code is not
> > correct? If it is the formal, do you have any suggestion to fix it?
> 
> This code is correct as-is, thus the commit message is wrong or misleading.
> 

Is that true? Jan is correct in saying that only >0 is an indicator of success according to the usual semantics of sccanf(). Personally I think the code would be clearer if the checks for failure were < 1 rather than <= 0.

  Paul

> David
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] xen-netback: correct return value checks on xenbus_scanf()

[Jan Beulich]

>>> On 07.07.16 at 12:55, <Paul.Durrant@citrix.com> wrote:
>>  -----Original Message-----
>> From: netdev-owner@vger.kernel.org [mailto:netdev-
>> owner@vger.kernel.org] On Behalf Of David Vrabel
>> Sent: 07 July 2016 11:45
>> To: Wei Liu; David Vrabel
>> Cc: xen-devel@lists.xenproject.org; Jan Beulich; netdev@vger.kernel.org 
>> Subject: Re: [Xen-devel] [PATCH] xen-netback: correct return value checks
>> on xenbus_scanf()
>> 
>> On 07/07/16 11:35, Wei Liu wrote:
>> > On Thu, Jul 07, 2016 at 10:58:16AM +0100, David Vrabel wrote:
>> >> On 07/07/16 08:57, Jan Beulich wrote:
>> >>> Only a positive return value indicates success.
>> >>
>> >> This is not correct.
>> >>
>> >
>> > Do you mean the commit message is not correct or the code is not
>> > correct? If it is the formal, do you have any suggestion to fix it?
>> 
>> This code is correct as-is, thus the commit message is wrong or misleading.
> 
> Is that true? Jan is correct in saying that only >0 is an indicator of 
> success according to the usual semantics of sccanf().

As was correctly pointed out, xenbus_scanf(), other than scanf(),
can't return zero right now (which I think has corner cases where
this might be a problem). So if I would get the feeling that a
correction (benign or not at this point in time) would be accepted,
what about "Only a positive return value is guaranteed to indicates
success" as commit description?

> Personally I think the 
> code would be clearer if the checks for failure were < 1 rather than <= 0.

I'd be fine with that, albeit if comparing with any non-zero number
then I think it would better be == or != instead of < or <=.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH] xen-netback: correct return value checks on xenbus_scanf()

[Jan Beulich]

Only a positive return value indicates success.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
 drivers/net/xen-netback/xenbus.c |   26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

--- 4.7-rc6-xenbus_scanf.orig/drivers/net/xen-netback/xenbus.c
+++ 4.7-rc6-xenbus_scanf/drivers/net/xen-netback/xenbus.c
@@ -741,7 +741,7 @@ static void xen_mcast_ctrl_changed(struc
 	int val;
 
 	if (xenbus_scanf(XBT_NIL, dev->otherend,
-			 "request-multicast-control", "%d", &val) < 0)
+			 "request-multicast-control", "%d", &val) <= 0)
 		val = 0;
 	vif->multicast_control = !!val;
 }
@@ -890,7 +890,7 @@ static void connect(struct backend_info
 	err = xenbus_scanf(XBT_NIL, dev->otherend,
 			   "multi-queue-num-queues",
 			   "%u", &requested_num_queues);
-	if (err < 0) {
+	if (err <= 0) {
 		requested_num_queues = 1; /* Fall back to single queue */
 	} else if (requested_num_queues > xenvif_max_queues) {
 		/* buggy or malicious guest */
@@ -1056,7 +1056,7 @@ static int connect_data_rings(struct bac
 	if (err < 0) {
 		err = xenbus_scanf(XBT_NIL, xspath,
 				   "event-channel", "%u", &tx_evtchn);
-		if (err < 0) {
+		if (err <= 0) {
 			xenbus_dev_fatal(dev, err,
 					 "reading %s/event-channel(-tx/rx)",
 					 xspath);
@@ -1092,10 +1092,10 @@ static int read_xenbus_vif_flags(struct
 	err = xenbus_scanf(XBT_NIL, dev->otherend, "request-rx-copy", "%u",
 			   &rx_copy);
 	if (err == -ENOENT) {
-		err = 0;
+		err = 1;
 		rx_copy = 0;
 	}
-	if (err < 0) {
+	if (err <= 0) {
 		xenbus_dev_fatal(dev, err, "reading %s/request-rx-copy",
 				 dev->otherend);
 		return err;
@@ -1104,7 +1104,7 @@ static int read_xenbus_vif_flags(struct
 		return -EOPNOTSUPP;
 
 	if (xenbus_scanf(XBT_NIL, dev->otherend,
-			 "feature-rx-notify", "%d", &val) < 0)
+			 "feature-rx-notify", "%d", &val) <= 0)
 		val = 0;
 	if (!val) {
 		/* - Reduce drain timeout to poll more frequently for
@@ -1116,7 +1116,7 @@ static int read_xenbus_vif_flags(struct
 	}
 
 	if (xenbus_scanf(XBT_NIL, dev->otherend, "feature-sg",
-			 "%d", &val) < 0)
+			 "%d", &val) <= 0)
 		val = 0;
 	vif->can_sg = !!val;
 
@@ -1124,25 +1124,25 @@ static int read_xenbus_vif_flags(struct
 	vif->gso_prefix_mask = 0;
 
 	if (xenbus_scanf(XBT_NIL, dev->otherend, "feature-gso-tcpv4",
-			 "%d", &val) < 0)
+			 "%d", &val) <= 0)
 		val = 0;
 	if (val)
 		vif->gso_mask |= GSO_BIT(TCPV4);
 
 	if (xenbus_scanf(XBT_NIL, dev->otherend, "feature-gso-tcpv4-prefix",
-			 "%d", &val) < 0)
+			 "%d", &val) <= 0)
 		val = 0;
 	if (val)
 		vif->gso_prefix_mask |= GSO_BIT(TCPV4);
 
 	if (xenbus_scanf(XBT_NIL, dev->otherend, "feature-gso-tcpv6",
-			 "%d", &val) < 0)
+			 "%d", &val) <= 0)
 		val = 0;
 	if (val)
 		vif->gso_mask |= GSO_BIT(TCPV6);
 
 	if (xenbus_scanf(XBT_NIL, dev->otherend, "feature-gso-tcpv6-prefix",
-			 "%d", &val) < 0)
+			 "%d", &val) <= 0)
 		val = 0;
 	if (val)
 		vif->gso_prefix_mask |= GSO_BIT(TCPV6);
@@ -1156,12 +1156,12 @@ static int read_xenbus_vif_flags(struct
 	}
 
 	if (xenbus_scanf(XBT_NIL, dev->otherend, "feature-no-csum-offload",
-			 "%d", &val) < 0)
+			 "%d", &val) <= 0)
 		val = 0;
 	vif->ip_csum = !val;
 
 	if (xenbus_scanf(XBT_NIL, dev->otherend, "feature-ipv6-csum-offload",
-			 "%d", &val) < 0)
+			 "%d", &val) <= 0)
 		val = 0;
 	vif->ipv6_csum = !!val;
 




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel