| Message ID | 577E277C02000078000FBEBB@prv-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> -----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
On 07/07/16 08:57, Jan Beulich wrote:
> Only a positive return value indicates success.
This is not correct.
David
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
> -----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
> -----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
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
> -----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
>>> 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
--- 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;
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(-)