* [PATCH] xen/xen-scsiback: Need go to fail after xenbus_dev_error() @ 2014-09-26 16:38 Chen Gang 2014-09-29 4:32 ` Juergen Gross 2014-09-29 4:32 ` [Xen-devel] " Juergen Gross 0 siblings, 2 replies; 22+ messages in thread From: Chen Gang @ 2014-09-26 16:38 UTC (permalink / raw) To: David Vrabel; +Cc: xen-devel, linux-kernel When failure occurs, after xenbus_dev_error(), need go to fail to let upper caller know about it. Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com> --- drivers/xen/xen-scsiback.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c index 847bc9c..3e430e1 100644 --- a/drivers/xen/xen-scsiback.c +++ b/drivers/xen/xen-scsiback.c @@ -1222,8 +1222,10 @@ static int scsiback_probe(struct xenbus_device *dev, err = xenbus_printf(XBT_NIL, dev->nodename, "feature-sg-grant", "%u", SG_ALL); - if (err) + if (err) { xenbus_dev_error(dev, err, "writing feature-sg-grant"); + goto fail; + } xenbus_switch_state(dev, XenbusStateInitWait); return 0; -- 1.9.3 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH] xen/xen-scsiback: Need go to fail after xenbus_dev_error() 2014-09-26 16:38 [PATCH] xen/xen-scsiback: Need go to fail after xenbus_dev_error() Chen Gang @ 2014-09-29 4:32 ` Juergen Gross 2014-09-29 4:32 ` [Xen-devel] " Juergen Gross 1 sibling, 0 replies; 22+ messages in thread From: Juergen Gross @ 2014-09-29 4:32 UTC (permalink / raw) To: Chen Gang, David Vrabel; +Cc: xen-devel, linux-kernel On 09/26/2014 06:38 PM, Chen Gang wrote: > When failure occurs, after xenbus_dev_error(), need go to fail to let > upper caller know about it. > > Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com> > --- > drivers/xen/xen-scsiback.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c > index 847bc9c..3e430e1 100644 > --- a/drivers/xen/xen-scsiback.c > +++ b/drivers/xen/xen-scsiback.c > @@ -1222,8 +1222,10 @@ static int scsiback_probe(struct xenbus_device *dev, > > err = xenbus_printf(XBT_NIL, dev->nodename, "feature-sg-grant", "%u", > SG_ALL); > - if (err) > + if (err) { > xenbus_dev_error(dev, err, "writing feature-sg-grant"); > + goto fail; > + } > > xenbus_switch_state(dev, XenbusStateInitWait); > return 0; > Hmm, not testing for failure was on purpose. Advertising this feature is just for tuning purposes, not mandatory. OTOH it would really be a strange error if this xenbus_printf() fails but all other operations are working, and signaling an error at the time when it first shows up is a good thing. So: Acked-by: Juergen Gross <jgross@suse.com> ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Xen-devel] [PATCH] xen/xen-scsiback: Need go to fail after xenbus_dev_error() 2014-09-26 16:38 [PATCH] xen/xen-scsiback: Need go to fail after xenbus_dev_error() Chen Gang 2014-09-29 4:32 ` Juergen Gross @ 2014-09-29 4:32 ` Juergen Gross 2014-09-29 8:41 ` Jan Beulich 2014-09-29 8:41 ` Jan Beulich 1 sibling, 2 replies; 22+ messages in thread From: Juergen Gross @ 2014-09-29 4:32 UTC (permalink / raw) To: Chen Gang, David Vrabel; +Cc: xen-devel, linux-kernel On 09/26/2014 06:38 PM, Chen Gang wrote: > When failure occurs, after xenbus_dev_error(), need go to fail to let > upper caller know about it. > > Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com> > --- > drivers/xen/xen-scsiback.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c > index 847bc9c..3e430e1 100644 > --- a/drivers/xen/xen-scsiback.c > +++ b/drivers/xen/xen-scsiback.c > @@ -1222,8 +1222,10 @@ static int scsiback_probe(struct xenbus_device *dev, > > err = xenbus_printf(XBT_NIL, dev->nodename, "feature-sg-grant", "%u", > SG_ALL); > - if (err) > + if (err) { > xenbus_dev_error(dev, err, "writing feature-sg-grant"); > + goto fail; > + } > > xenbus_switch_state(dev, XenbusStateInitWait); > return 0; > Hmm, not testing for failure was on purpose. Advertising this feature is just for tuning purposes, not mandatory. OTOH it would really be a strange error if this xenbus_printf() fails but all other operations are working, and signaling an error at the time when it first shows up is a good thing. So: Acked-by: Juergen Gross <jgross@suse.com> ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Xen-devel] [PATCH] xen/xen-scsiback: Need go to fail after xenbus_dev_error() 2014-09-29 4:32 ` [Xen-devel] " Juergen Gross @ 2014-09-29 8:41 ` Jan Beulich 2014-09-29 9:31 ` Chen Gang 2014-09-29 9:31 ` Chen Gang 2014-09-29 8:41 ` Jan Beulich 1 sibling, 2 replies; 22+ messages in thread From: Jan Beulich @ 2014-09-29 8:41 UTC (permalink / raw) To: Chen Gang, Juergen Gross; +Cc: David Vrabel, xen-devel, linux-kernel >>> On 29.09.14 at 06:32, <JGross@suse.com> wrote: > On 09/26/2014 06:38 PM, Chen Gang wrote: >> When failure occurs, after xenbus_dev_error(), need go to fail to let >> upper caller know about it. >> >> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com> >> --- >> drivers/xen/xen-scsiback.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c >> index 847bc9c..3e430e1 100644 >> --- a/drivers/xen/xen-scsiback.c >> +++ b/drivers/xen/xen-scsiback.c >> @@ -1222,8 +1222,10 @@ static int scsiback_probe(struct xenbus_device *dev, >> >> err = xenbus_printf(XBT_NIL, dev->nodename, "feature-sg-grant", "%u", >> SG_ALL); >> - if (err) >> + if (err) { >> xenbus_dev_error(dev, err, "writing feature-sg-grant"); >> + goto fail; >> + } >> >> xenbus_switch_state(dev, XenbusStateInitWait); >> return 0; >> > > Hmm, not testing for failure was on purpose. Advertising this feature > is just for tuning purposes, not mandatory. > > OTOH it would really be a strange error if this xenbus_printf() fails > but all other operations are working, and signaling an error at the > time when it first shows up is a good thing. So: I disagree - failure to announce optional features should not lead to general failure. And this should be consistent across drivers; for existing examples see xen_blkbk_flush_diskcache() and xen_blkbk_discard(). Jan ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Xen-devel] [PATCH] xen/xen-scsiback: Need go to fail after xenbus_dev_error() 2014-09-29 8:41 ` Jan Beulich @ 2014-09-29 9:31 ` Chen Gang 2014-09-29 9:34 ` Juergen Gross 2014-09-29 9:34 ` [Xen-devel] " Juergen Gross 2014-09-29 9:31 ` Chen Gang 1 sibling, 2 replies; 22+ messages in thread From: Chen Gang @ 2014-09-29 9:31 UTC (permalink / raw) To: Jan Beulich, Juergen Gross; +Cc: David Vrabel, xen-devel, linux-kernel On 9/29/14 16:41, Jan Beulich wrote: >>>> On 29.09.14 at 06:32, <JGross@suse.com> wrote: >> On 09/26/2014 06:38 PM, Chen Gang wrote: >>> When failure occurs, after xenbus_dev_error(), need go to fail to let >>> upper caller know about it. >>> >>> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com> >>> --- >>> drivers/xen/xen-scsiback.c | 4 +++- >>> 1 file changed, 3 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c >>> index 847bc9c..3e430e1 100644 >>> --- a/drivers/xen/xen-scsiback.c >>> +++ b/drivers/xen/xen-scsiback.c >>> @@ -1222,8 +1222,10 @@ static int scsiback_probe(struct xenbus_device *dev, >>> >>> err = xenbus_printf(XBT_NIL, dev->nodename, "feature-sg-grant", "%u", >>> SG_ALL); >>> - if (err) >>> + if (err) { >>> xenbus_dev_error(dev, err, "writing feature-sg-grant"); >>> + goto fail; >>> + } >>> >>> xenbus_switch_state(dev, XenbusStateInitWait); >>> return 0; >>> >> >> Hmm, not testing for failure was on purpose. Advertising this feature >> is just for tuning purposes, not mandatory. >> >> OTOH it would really be a strange error if this xenbus_printf() fails >> but all other operations are working, and signaling an error at the >> time when it first shows up is a good thing. So: > > I disagree - failure to announce optional features should not lead to > general failure. And this should be consistent across drivers; for > existing examples see xen_blkbk_flush_diskcache() and > xen_blkbk_discard(). > During scsiback_probe(), can we sure that "feature-sg-grant" is optional feature? For me, only according to its name, I guess not: it is about security which is always necessary in kernel (although SG_ALL). Thanks -- Chen Gang Open, share, and attitude like air, water, and life which God blessed ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] xen/xen-scsiback: Need go to fail after xenbus_dev_error() 2014-09-29 9:31 ` Chen Gang @ 2014-09-29 9:34 ` Juergen Gross 2014-09-29 9:34 ` [Xen-devel] " Juergen Gross 1 sibling, 0 replies; 22+ messages in thread From: Juergen Gross @ 2014-09-29 9:34 UTC (permalink / raw) To: Chen Gang, Jan Beulich; +Cc: xen-devel, David Vrabel, linux-kernel On 09/29/2014 11:31 AM, Chen Gang wrote: > On 9/29/14 16:41, Jan Beulich wrote: >>>>> On 29.09.14 at 06:32, <JGross@suse.com> wrote: >>> On 09/26/2014 06:38 PM, Chen Gang wrote: >>>> When failure occurs, after xenbus_dev_error(), need go to fail to let >>>> upper caller know about it. >>>> >>>> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com> >>>> --- >>>> drivers/xen/xen-scsiback.c | 4 +++- >>>> 1 file changed, 3 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c >>>> index 847bc9c..3e430e1 100644 >>>> --- a/drivers/xen/xen-scsiback.c >>>> +++ b/drivers/xen/xen-scsiback.c >>>> @@ -1222,8 +1222,10 @@ static int scsiback_probe(struct xenbus_device *dev, >>>> >>>> err = xenbus_printf(XBT_NIL, dev->nodename, "feature-sg-grant", "%u", >>>> SG_ALL); >>>> - if (err) >>>> + if (err) { >>>> xenbus_dev_error(dev, err, "writing feature-sg-grant"); >>>> + goto fail; >>>> + } >>>> >>>> xenbus_switch_state(dev, XenbusStateInitWait); >>>> return 0; >>>> >>> >>> Hmm, not testing for failure was on purpose. Advertising this feature >>> is just for tuning purposes, not mandatory. >>> >>> OTOH it would really be a strange error if this xenbus_printf() fails >>> but all other operations are working, and signaling an error at the >>> time when it first shows up is a good thing. So: >> >> I disagree - failure to announce optional features should not lead to >> general failure. And this should be consistent across drivers; for >> existing examples see xen_blkbk_flush_diskcache() and >> xen_blkbk_discard(). >> > > During scsiback_probe(), can we sure that "feature-sg-grant" is optional > feature? For me, only according to its name, I guess not: it is about > security which is always necessary in kernel (although SG_ALL). It is optional. Otherwise the interface would be broken, as I've added this feature recently and "old" clients (pre 3.18) don't know about it. The feature is NOT about security, but about capability to support larger SCSI requests than the old interface did. Juergen ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Xen-devel] [PATCH] xen/xen-scsiback: Need go to fail after xenbus_dev_error() 2014-09-29 9:31 ` Chen Gang 2014-09-29 9:34 ` Juergen Gross @ 2014-09-29 9:34 ` Juergen Gross 2014-09-29 9:59 ` Chen Gang 2014-09-29 9:59 ` [Xen-devel] " Chen Gang 1 sibling, 2 replies; 22+ messages in thread From: Juergen Gross @ 2014-09-29 9:34 UTC (permalink / raw) To: Chen Gang, Jan Beulich; +Cc: David Vrabel, xen-devel, linux-kernel On 09/29/2014 11:31 AM, Chen Gang wrote: > On 9/29/14 16:41, Jan Beulich wrote: >>>>> On 29.09.14 at 06:32, <JGross@suse.com> wrote: >>> On 09/26/2014 06:38 PM, Chen Gang wrote: >>>> When failure occurs, after xenbus_dev_error(), need go to fail to let >>>> upper caller know about it. >>>> >>>> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com> >>>> --- >>>> drivers/xen/xen-scsiback.c | 4 +++- >>>> 1 file changed, 3 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c >>>> index 847bc9c..3e430e1 100644 >>>> --- a/drivers/xen/xen-scsiback.c >>>> +++ b/drivers/xen/xen-scsiback.c >>>> @@ -1222,8 +1222,10 @@ static int scsiback_probe(struct xenbus_device *dev, >>>> >>>> err = xenbus_printf(XBT_NIL, dev->nodename, "feature-sg-grant", "%u", >>>> SG_ALL); >>>> - if (err) >>>> + if (err) { >>>> xenbus_dev_error(dev, err, "writing feature-sg-grant"); >>>> + goto fail; >>>> + } >>>> >>>> xenbus_switch_state(dev, XenbusStateInitWait); >>>> return 0; >>>> >>> >>> Hmm, not testing for failure was on purpose. Advertising this feature >>> is just for tuning purposes, not mandatory. >>> >>> OTOH it would really be a strange error if this xenbus_printf() fails >>> but all other operations are working, and signaling an error at the >>> time when it first shows up is a good thing. So: >> >> I disagree - failure to announce optional features should not lead to >> general failure. And this should be consistent across drivers; for >> existing examples see xen_blkbk_flush_diskcache() and >> xen_blkbk_discard(). >> > > During scsiback_probe(), can we sure that "feature-sg-grant" is optional > feature? For me, only according to its name, I guess not: it is about > security which is always necessary in kernel (although SG_ALL). It is optional. Otherwise the interface would be broken, as I've added this feature recently and "old" clients (pre 3.18) don't know about it. The feature is NOT about security, but about capability to support larger SCSI requests than the old interface did. Juergen ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] xen/xen-scsiback: Need go to fail after xenbus_dev_error() 2014-09-29 9:34 ` [Xen-devel] " Juergen Gross @ 2014-09-29 9:59 ` Chen Gang 2014-09-29 9:59 ` [Xen-devel] " Chen Gang 1 sibling, 0 replies; 22+ messages in thread From: Chen Gang @ 2014-09-29 9:59 UTC (permalink / raw) To: Juergen Gross, Jan Beulich; +Cc: xen-devel, David Vrabel, linux-kernel On 9/29/14 17:34, Juergen Gross wrote: > On 09/29/2014 11:31 AM, Chen Gang wrote: >> On 9/29/14 16:41, Jan Beulich wrote: >>>>>> On 29.09.14 at 06:32, <JGross@suse.com> wrote: >>>> On 09/26/2014 06:38 PM, Chen Gang wrote: >>>>> When failure occurs, after xenbus_dev_error(), need go to fail to let >>>>> upper caller know about it. >>>>> >>>>> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com> >>>>> --- >>>>> drivers/xen/xen-scsiback.c | 4 +++- >>>>> 1 file changed, 3 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c >>>>> index 847bc9c..3e430e1 100644 >>>>> --- a/drivers/xen/xen-scsiback.c >>>>> +++ b/drivers/xen/xen-scsiback.c >>>>> @@ -1222,8 +1222,10 @@ static int scsiback_probe(struct xenbus_device *dev, >>>>> >>>>> err = xenbus_printf(XBT_NIL, dev->nodename, "feature-sg-grant", "%u", >>>>> SG_ALL); >>>>> - if (err) >>>>> + if (err) { >>>>> xenbus_dev_error(dev, err, "writing feature-sg-grant"); >>>>> + goto fail; >>>>> + } >>>>> >>>>> xenbus_switch_state(dev, XenbusStateInitWait); >>>>> return 0; >>>>> >>>> >>>> Hmm, not testing for failure was on purpose. Advertising this feature >>>> is just for tuning purposes, not mandatory. >>>> >>>> OTOH it would really be a strange error if this xenbus_printf() fails >>>> but all other operations are working, and signaling an error at the >>>> time when it first shows up is a good thing. So: >>> >>> I disagree - failure to announce optional features should not lead to >>> general failure. And this should be consistent across drivers; for >>> existing examples see xen_blkbk_flush_diskcache() and >>> xen_blkbk_discard(). >>> >> >> During scsiback_probe(), can we sure that "feature-sg-grant" is optional >> feature? For me, only according to its name, I guess not: it is about >> security which is always necessary in kernel (although SG_ALL). > > It is optional. Otherwise the interface would be broken, as I've added > this feature recently and "old" clients (pre 3.18) don't know about it. > > The feature is NOT about security, but about capability to support > larger SCSI requests than the old interface did. > OK, thanks. All of you said sounds reasonable to me: "it is optional, need print related warning and continue". If no any additional reply within 2 days, I shall send patch v2 for it: "use dev_warn() instead of xenbus_dev_error() and remove 'fail' code block" Thanks. -- Chen Gang Open, share, and attitude like air, water, and life which God blessed ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Xen-devel] [PATCH] xen/xen-scsiback: Need go to fail after xenbus_dev_error() 2014-09-29 9:34 ` [Xen-devel] " Juergen Gross 2014-09-29 9:59 ` Chen Gang @ 2014-09-29 9:59 ` Chen Gang 2014-09-29 13:57 ` David Vrabel 2014-09-29 13:57 ` [Xen-devel] " David Vrabel 1 sibling, 2 replies; 22+ messages in thread From: Chen Gang @ 2014-09-29 9:59 UTC (permalink / raw) To: Juergen Gross, Jan Beulich; +Cc: David Vrabel, xen-devel, linux-kernel On 9/29/14 17:34, Juergen Gross wrote: > On 09/29/2014 11:31 AM, Chen Gang wrote: >> On 9/29/14 16:41, Jan Beulich wrote: >>>>>> On 29.09.14 at 06:32, <JGross@suse.com> wrote: >>>> On 09/26/2014 06:38 PM, Chen Gang wrote: >>>>> When failure occurs, after xenbus_dev_error(), need go to fail to let >>>>> upper caller know about it. >>>>> >>>>> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com> >>>>> --- >>>>> drivers/xen/xen-scsiback.c | 4 +++- >>>>> 1 file changed, 3 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c >>>>> index 847bc9c..3e430e1 100644 >>>>> --- a/drivers/xen/xen-scsiback.c >>>>> +++ b/drivers/xen/xen-scsiback.c >>>>> @@ -1222,8 +1222,10 @@ static int scsiback_probe(struct xenbus_device *dev, >>>>> >>>>> err = xenbus_printf(XBT_NIL, dev->nodename, "feature-sg-grant", "%u", >>>>> SG_ALL); >>>>> - if (err) >>>>> + if (err) { >>>>> xenbus_dev_error(dev, err, "writing feature-sg-grant"); >>>>> + goto fail; >>>>> + } >>>>> >>>>> xenbus_switch_state(dev, XenbusStateInitWait); >>>>> return 0; >>>>> >>>> >>>> Hmm, not testing for failure was on purpose. Advertising this feature >>>> is just for tuning purposes, not mandatory. >>>> >>>> OTOH it would really be a strange error if this xenbus_printf() fails >>>> but all other operations are working, and signaling an error at the >>>> time when it first shows up is a good thing. So: >>> >>> I disagree - failure to announce optional features should not lead to >>> general failure. And this should be consistent across drivers; for >>> existing examples see xen_blkbk_flush_diskcache() and >>> xen_blkbk_discard(). >>> >> >> During scsiback_probe(), can we sure that "feature-sg-grant" is optional >> feature? For me, only according to its name, I guess not: it is about >> security which is always necessary in kernel (although SG_ALL). > > It is optional. Otherwise the interface would be broken, as I've added > this feature recently and "old" clients (pre 3.18) don't know about it. > > The feature is NOT about security, but about capability to support > larger SCSI requests than the old interface did. > OK, thanks. All of you said sounds reasonable to me: "it is optional, need print related warning and continue". If no any additional reply within 2 days, I shall send patch v2 for it: "use dev_warn() instead of xenbus_dev_error() and remove 'fail' code block" Thanks. -- Chen Gang Open, share, and attitude like air, water, and life which God blessed ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] xen/xen-scsiback: Need go to fail after xenbus_dev_error() 2014-09-29 9:59 ` [Xen-devel] " Chen Gang @ 2014-09-29 13:57 ` David Vrabel 2014-09-29 13:57 ` [Xen-devel] " David Vrabel 1 sibling, 0 replies; 22+ messages in thread From: David Vrabel @ 2014-09-29 13:57 UTC (permalink / raw) To: Chen Gang, Juergen Gross, Jan Beulich Cc: xen-devel, David Vrabel, linux-kernel On 29/09/14 10:59, Chen Gang wrote: > > > If no any additional reply within 2 days, I shall send patch v2 for it: > > "use dev_warn() instead of xenbus_dev_error() and remove 'fail' code block" I think this driver is fine as-is and does not need any changes. David ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Xen-devel] [PATCH] xen/xen-scsiback: Need go to fail after xenbus_dev_error() 2014-09-29 9:59 ` [Xen-devel] " Chen Gang 2014-09-29 13:57 ` David Vrabel @ 2014-09-29 13:57 ` David Vrabel 2014-09-30 6:32 ` Chen Gang 2014-09-30 6:32 ` Chen Gang 1 sibling, 2 replies; 22+ messages in thread From: David Vrabel @ 2014-09-29 13:57 UTC (permalink / raw) To: Chen Gang, Juergen Gross, Jan Beulich Cc: xen-devel, David Vrabel, linux-kernel On 29/09/14 10:59, Chen Gang wrote: > > > If no any additional reply within 2 days, I shall send patch v2 for it: > > "use dev_warn() instead of xenbus_dev_error() and remove 'fail' code block" I think this driver is fine as-is and does not need any changes. David ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Xen-devel] [PATCH] xen/xen-scsiback: Need go to fail after xenbus_dev_error() 2014-09-29 13:57 ` [Xen-devel] " David Vrabel @ 2014-09-30 6:32 ` Chen Gang 2014-09-30 6:59 ` Juergen Gross 2014-09-30 6:59 ` [Xen-devel] " Juergen Gross 2014-09-30 6:32 ` Chen Gang 1 sibling, 2 replies; 22+ messages in thread From: Chen Gang @ 2014-09-30 6:32 UTC (permalink / raw) To: David Vrabel, Juergen Gross, Jan Beulich; +Cc: xen-devel, linux-kernel On 9/29/14 21:57, David Vrabel wrote: > On 29/09/14 10:59, Chen Gang wrote: >> >> >> If no any additional reply within 2 days, I shall send patch v2 for it: >> >> "use dev_warn() instead of xenbus_dev_error() and remove 'fail' code block" > > I think this driver is fine as-is and does not need any changes. > OK, at least, at present, it is not a bug (will cause any issue). But for me, xenbus_dev_error() seems for printing generic errors, dev_warn() is more suitable than it. And 'fail' code block is useless now, need be removed, too (which will let compiler report warning). Thanks. -- Chen Gang Open, share, and attitude like air, water, and life which God blessed ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] xen/xen-scsiback: Need go to fail after xenbus_dev_error() 2014-09-30 6:32 ` Chen Gang @ 2014-09-30 6:59 ` Juergen Gross 2014-09-30 6:59 ` [Xen-devel] " Juergen Gross 1 sibling, 0 replies; 22+ messages in thread From: Juergen Gross @ 2014-09-30 6:59 UTC (permalink / raw) To: Chen Gang, David Vrabel, Jan Beulich; +Cc: xen-devel, linux-kernel On 09/30/2014 08:32 AM, Chen Gang wrote: > On 9/29/14 21:57, David Vrabel wrote: >> On 29/09/14 10:59, Chen Gang wrote: >>> >>> >>> If no any additional reply within 2 days, I shall send patch v2 for it: >>> >>> "use dev_warn() instead of xenbus_dev_error() and remove 'fail' code block" >> >> I think this driver is fine as-is and does not need any changes. >> > > OK, at least, at present, it is not a bug (will cause any issue). > > But for me, xenbus_dev_error() seems for printing generic errors, > dev_warn() is more suitable than it. I'm unbiased regarding this one. > > And 'fail' code block is useless now, need be removed, too (which will > let compiler report warning). This should be part of the patch making the 'fail' block useless. Juergen ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Xen-devel] [PATCH] xen/xen-scsiback: Need go to fail after xenbus_dev_error() 2014-09-30 6:32 ` Chen Gang 2014-09-30 6:59 ` Juergen Gross @ 2014-09-30 6:59 ` Juergen Gross 2014-09-30 7:50 ` Chen Gang 2014-09-30 7:50 ` [Xen-devel] " Chen Gang 1 sibling, 2 replies; 22+ messages in thread From: Juergen Gross @ 2014-09-30 6:59 UTC (permalink / raw) To: Chen Gang, David Vrabel, Jan Beulich; +Cc: xen-devel, linux-kernel On 09/30/2014 08:32 AM, Chen Gang wrote: > On 9/29/14 21:57, David Vrabel wrote: >> On 29/09/14 10:59, Chen Gang wrote: >>> >>> >>> If no any additional reply within 2 days, I shall send patch v2 for it: >>> >>> "use dev_warn() instead of xenbus_dev_error() and remove 'fail' code block" >> >> I think this driver is fine as-is and does not need any changes. >> > > OK, at least, at present, it is not a bug (will cause any issue). > > But for me, xenbus_dev_error() seems for printing generic errors, > dev_warn() is more suitable than it. I'm unbiased regarding this one. > > And 'fail' code block is useless now, need be removed, too (which will > let compiler report warning). This should be part of the patch making the 'fail' block useless. Juergen ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] xen/xen-scsiback: Need go to fail after xenbus_dev_error() 2014-09-30 6:59 ` [Xen-devel] " Juergen Gross @ 2014-09-30 7:50 ` Chen Gang 2014-09-30 7:50 ` [Xen-devel] " Chen Gang 1 sibling, 0 replies; 22+ messages in thread From: Chen Gang @ 2014-09-30 7:50 UTC (permalink / raw) To: Juergen Gross, David Vrabel, Jan Beulich; +Cc: xen-devel, linux-kernel On 9/30/14 14:59, Juergen Gross wrote: > On 09/30/2014 08:32 AM, Chen Gang wrote: >> On 9/29/14 21:57, David Vrabel wrote: >>> On 29/09/14 10:59, Chen Gang wrote: >>>> >>>> >>>> If no any additional reply within 2 days, I shall send patch v2 for it: >>>> >>>> "use dev_warn() instead of xenbus_dev_error() and remove 'fail' code block" >>> >>> I think this driver is fine as-is and does not need any changes. >>> >> >> OK, at least, at present, it is not a bug (will cause any issue). >> >> But for me, xenbus_dev_error() seems for printing generic errors, >> dev_warn() is more suitable than it. > > I'm unbiased regarding this one. > After check all related code for xenbus_printf() and xenbus_dev_error(), for me: if xenbus_printf() is for optional error, it will print warning; all xenbus_dev_error() are not for optional error, except 2 area: drivers/pci/xen-pcifront.c:866: xenbus_dev_error(pdev->xdev, err, drivers/pci/xen-pcifront.c:947: xenbus_dev_error(pdev->xdev, err, In fact, for me, not only they need be improved, but also skip 'err' for pcifront_scan_root() and pcifront_rescan_root(), are they bugs? (I guess they are). If they are really bugs, I shall send related patch for it. >> >> And 'fail' code block is useless now, need be removed, too (which will >> let compiler report warning). > > This should be part of the patch making the 'fail' block useless. > Yeah, originally, it really should be, but if this patch can continue, for me, can remove it in this patch, too (for the original patch, I intended to remain it for discussing and analysing in this patch). But all together, if you stick to remove 'fail' code block in original patch, for me, it is OK. Thanks. -- Chen Gang Open, share, and attitude like air, water, and life which God blessed ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Xen-devel] [PATCH] xen/xen-scsiback: Need go to fail after xenbus_dev_error() 2014-09-30 6:59 ` [Xen-devel] " Juergen Gross 2014-09-30 7:50 ` Chen Gang @ 2014-09-30 7:50 ` Chen Gang 2014-09-30 10:23 ` Chen Gang 2014-09-30 10:23 ` Chen Gang 1 sibling, 2 replies; 22+ messages in thread From: Chen Gang @ 2014-09-30 7:50 UTC (permalink / raw) To: Juergen Gross, David Vrabel, Jan Beulich; +Cc: xen-devel, linux-kernel On 9/30/14 14:59, Juergen Gross wrote: > On 09/30/2014 08:32 AM, Chen Gang wrote: >> On 9/29/14 21:57, David Vrabel wrote: >>> On 29/09/14 10:59, Chen Gang wrote: >>>> >>>> >>>> If no any additional reply within 2 days, I shall send patch v2 for it: >>>> >>>> "use dev_warn() instead of xenbus_dev_error() and remove 'fail' code block" >>> >>> I think this driver is fine as-is and does not need any changes. >>> >> >> OK, at least, at present, it is not a bug (will cause any issue). >> >> But for me, xenbus_dev_error() seems for printing generic errors, >> dev_warn() is more suitable than it. > > I'm unbiased regarding this one. > After check all related code for xenbus_printf() and xenbus_dev_error(), for me: if xenbus_printf() is for optional error, it will print warning; all xenbus_dev_error() are not for optional error, except 2 area: drivers/pci/xen-pcifront.c:866: xenbus_dev_error(pdev->xdev, err, drivers/pci/xen-pcifront.c:947: xenbus_dev_error(pdev->xdev, err, In fact, for me, not only they need be improved, but also skip 'err' for pcifront_scan_root() and pcifront_rescan_root(), are they bugs? (I guess they are). If they are really bugs, I shall send related patch for it. >> >> And 'fail' code block is useless now, need be removed, too (which will >> let compiler report warning). > > This should be part of the patch making the 'fail' block useless. > Yeah, originally, it really should be, but if this patch can continue, for me, can remove it in this patch, too (for the original patch, I intended to remain it for discussing and analysing in this patch). But all together, if you stick to remove 'fail' code block in original patch, for me, it is OK. Thanks. -- Chen Gang Open, share, and attitude like air, water, and life which God blessed ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Xen-devel] [PATCH] xen/xen-scsiback: Need go to fail after xenbus_dev_error() 2014-09-30 7:50 ` [Xen-devel] " Chen Gang @ 2014-09-30 10:23 ` Chen Gang 2014-09-30 10:23 ` Chen Gang 1 sibling, 0 replies; 22+ messages in thread From: Chen Gang @ 2014-09-30 10:23 UTC (permalink / raw) To: Juergen Gross, David Vrabel, Jan Beulich; +Cc: xen-devel, linux-kernel On 9/30/14 15:50, Chen Gang wrote: > On 9/30/14 14:59, Juergen Gross wrote: >> On 09/30/2014 08:32 AM, Chen Gang wrote: >>> On 9/29/14 21:57, David Vrabel wrote: >>>> On 29/09/14 10:59, Chen Gang wrote: >>>>> >>>>> >>>>> If no any additional reply within 2 days, I shall send patch v2 for it: >>>>> >>>>> "use dev_warn() instead of xenbus_dev_error() and remove 'fail' code block" >>>> >>>> I think this driver is fine as-is and does not need any changes. >>>> >>> >>> OK, at least, at present, it is not a bug (will cause any issue). >>> >>> But for me, xenbus_dev_error() seems for printing generic errors, >>> dev_warn() is more suitable than it. >> >> I'm unbiased regarding this one. >> > > After check all related code for xenbus_printf() and xenbus_dev_error(), > for me: if xenbus_printf() is for optional error, it will print warning; > all xenbus_dev_error() are not for optional error, except 2 area: > > drivers/pci/xen-pcifront.c:866: xenbus_dev_error(pdev->xdev, err, > drivers/pci/xen-pcifront.c:947: xenbus_dev_error(pdev->xdev, err, And for this 2 xenbus_dev_error(), they have no much negative effect (not check return value, and according to the code below, readers can easily understand, they are for optional failure). But for our case, I recommend to use dev_warn() instead of, or readers is really easy to misunderstand (xenbus_dev_error, and 'grant'), then may send spam again (like me). > > In fact, for me, not only they need be improved, but also skip 'err' for > pcifront_scan_root() and pcifront_rescan_root(), are they bugs? (I guess > they are). If they are really bugs, I shall send related patch for it. > If no any additional reply for them within 2 days, I shall assume they are bugs, and send related patch for them, in next month (2014-10-??). >>> >>> And 'fail' code block is useless now, need be removed, too (which will >>> let compiler report warning). >> >> This should be part of the patch making the 'fail' block useless. >> The original related patch is canceled, so we need not remove 'fail' block (it still seems useful, although it is not). > > Yeah, originally, it really should be, but if this patch can continue, > for me, can remove it in this patch, too (for the original patch, I > intended to remain it for discussing and analysing in this patch). > > But all together, if you stick to remove 'fail' code block in original > patch, for me, it is OK. > Thanks. -- Chen Gang Open, share, and attitude like air, water, and life which God blessed ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] xen/xen-scsiback: Need go to fail after xenbus_dev_error() 2014-09-30 7:50 ` [Xen-devel] " Chen Gang 2014-09-30 10:23 ` Chen Gang @ 2014-09-30 10:23 ` Chen Gang 1 sibling, 0 replies; 22+ messages in thread From: Chen Gang @ 2014-09-30 10:23 UTC (permalink / raw) To: Juergen Gross, David Vrabel, Jan Beulich; +Cc: xen-devel, linux-kernel On 9/30/14 15:50, Chen Gang wrote: > On 9/30/14 14:59, Juergen Gross wrote: >> On 09/30/2014 08:32 AM, Chen Gang wrote: >>> On 9/29/14 21:57, David Vrabel wrote: >>>> On 29/09/14 10:59, Chen Gang wrote: >>>>> >>>>> >>>>> If no any additional reply within 2 days, I shall send patch v2 for it: >>>>> >>>>> "use dev_warn() instead of xenbus_dev_error() and remove 'fail' code block" >>>> >>>> I think this driver is fine as-is and does not need any changes. >>>> >>> >>> OK, at least, at present, it is not a bug (will cause any issue). >>> >>> But for me, xenbus_dev_error() seems for printing generic errors, >>> dev_warn() is more suitable than it. >> >> I'm unbiased regarding this one. >> > > After check all related code for xenbus_printf() and xenbus_dev_error(), > for me: if xenbus_printf() is for optional error, it will print warning; > all xenbus_dev_error() are not for optional error, except 2 area: > > drivers/pci/xen-pcifront.c:866: xenbus_dev_error(pdev->xdev, err, > drivers/pci/xen-pcifront.c:947: xenbus_dev_error(pdev->xdev, err, And for this 2 xenbus_dev_error(), they have no much negative effect (not check return value, and according to the code below, readers can easily understand, they are for optional failure). But for our case, I recommend to use dev_warn() instead of, or readers is really easy to misunderstand (xenbus_dev_error, and 'grant'), then may send spam again (like me). > > In fact, for me, not only they need be improved, but also skip 'err' for > pcifront_scan_root() and pcifront_rescan_root(), are they bugs? (I guess > they are). If they are really bugs, I shall send related patch for it. > If no any additional reply for them within 2 days, I shall assume they are bugs, and send related patch for them, in next month (2014-10-??). >>> >>> And 'fail' code block is useless now, need be removed, too (which will >>> let compiler report warning). >> >> This should be part of the patch making the 'fail' block useless. >> The original related patch is canceled, so we need not remove 'fail' block (it still seems useful, although it is not). > > Yeah, originally, it really should be, but if this patch can continue, > for me, can remove it in this patch, too (for the original patch, I > intended to remain it for discussing and analysing in this patch). > > But all together, if you stick to remove 'fail' code block in original > patch, for me, it is OK. > Thanks. -- Chen Gang Open, share, and attitude like air, water, and life which God blessed ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] xen/xen-scsiback: Need go to fail after xenbus_dev_error() 2014-09-29 13:57 ` [Xen-devel] " David Vrabel 2014-09-30 6:32 ` Chen Gang @ 2014-09-30 6:32 ` Chen Gang 1 sibling, 0 replies; 22+ messages in thread From: Chen Gang @ 2014-09-30 6:32 UTC (permalink / raw) To: David Vrabel, Juergen Gross, Jan Beulich; +Cc: xen-devel, linux-kernel On 9/29/14 21:57, David Vrabel wrote: > On 29/09/14 10:59, Chen Gang wrote: >> >> >> If no any additional reply within 2 days, I shall send patch v2 for it: >> >> "use dev_warn() instead of xenbus_dev_error() and remove 'fail' code block" > > I think this driver is fine as-is and does not need any changes. > OK, at least, at present, it is not a bug (will cause any issue). But for me, xenbus_dev_error() seems for printing generic errors, dev_warn() is more suitable than it. And 'fail' code block is useless now, need be removed, too (which will let compiler report warning). Thanks. -- Chen Gang Open, share, and attitude like air, water, and life which God blessed ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] xen/xen-scsiback: Need go to fail after xenbus_dev_error() 2014-09-29 8:41 ` Jan Beulich 2014-09-29 9:31 ` Chen Gang @ 2014-09-29 9:31 ` Chen Gang 1 sibling, 0 replies; 22+ messages in thread From: Chen Gang @ 2014-09-29 9:31 UTC (permalink / raw) To: Jan Beulich, Juergen Gross; +Cc: xen-devel, David Vrabel, linux-kernel On 9/29/14 16:41, Jan Beulich wrote: >>>> On 29.09.14 at 06:32, <JGross@suse.com> wrote: >> On 09/26/2014 06:38 PM, Chen Gang wrote: >>> When failure occurs, after xenbus_dev_error(), need go to fail to let >>> upper caller know about it. >>> >>> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com> >>> --- >>> drivers/xen/xen-scsiback.c | 4 +++- >>> 1 file changed, 3 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c >>> index 847bc9c..3e430e1 100644 >>> --- a/drivers/xen/xen-scsiback.c >>> +++ b/drivers/xen/xen-scsiback.c >>> @@ -1222,8 +1222,10 @@ static int scsiback_probe(struct xenbus_device *dev, >>> >>> err = xenbus_printf(XBT_NIL, dev->nodename, "feature-sg-grant", "%u", >>> SG_ALL); >>> - if (err) >>> + if (err) { >>> xenbus_dev_error(dev, err, "writing feature-sg-grant"); >>> + goto fail; >>> + } >>> >>> xenbus_switch_state(dev, XenbusStateInitWait); >>> return 0; >>> >> >> Hmm, not testing for failure was on purpose. Advertising this feature >> is just for tuning purposes, not mandatory. >> >> OTOH it would really be a strange error if this xenbus_printf() fails >> but all other operations are working, and signaling an error at the >> time when it first shows up is a good thing. So: > > I disagree - failure to announce optional features should not lead to > general failure. And this should be consistent across drivers; for > existing examples see xen_blkbk_flush_diskcache() and > xen_blkbk_discard(). > During scsiback_probe(), can we sure that "feature-sg-grant" is optional feature? For me, only according to its name, I guess not: it is about security which is always necessary in kernel (although SG_ALL). Thanks -- Chen Gang Open, share, and attitude like air, water, and life which God blessed ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] xen/xen-scsiback: Need go to fail after xenbus_dev_error() 2014-09-29 4:32 ` [Xen-devel] " Juergen Gross 2014-09-29 8:41 ` Jan Beulich @ 2014-09-29 8:41 ` Jan Beulich 1 sibling, 0 replies; 22+ messages in thread From: Jan Beulich @ 2014-09-29 8:41 UTC (permalink / raw) To: Chen Gang, Juergen Gross; +Cc: xen-devel, David Vrabel, linux-kernel >>> On 29.09.14 at 06:32, <JGross@suse.com> wrote: > On 09/26/2014 06:38 PM, Chen Gang wrote: >> When failure occurs, after xenbus_dev_error(), need go to fail to let >> upper caller know about it. >> >> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com> >> --- >> drivers/xen/xen-scsiback.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c >> index 847bc9c..3e430e1 100644 >> --- a/drivers/xen/xen-scsiback.c >> +++ b/drivers/xen/xen-scsiback.c >> @@ -1222,8 +1222,10 @@ static int scsiback_probe(struct xenbus_device *dev, >> >> err = xenbus_printf(XBT_NIL, dev->nodename, "feature-sg-grant", "%u", >> SG_ALL); >> - if (err) >> + if (err) { >> xenbus_dev_error(dev, err, "writing feature-sg-grant"); >> + goto fail; >> + } >> >> xenbus_switch_state(dev, XenbusStateInitWait); >> return 0; >> > > Hmm, not testing for failure was on purpose. Advertising this feature > is just for tuning purposes, not mandatory. > > OTOH it would really be a strange error if this xenbus_printf() fails > but all other operations are working, and signaling an error at the > time when it first shows up is a good thing. So: I disagree - failure to announce optional features should not lead to general failure. And this should be consistent across drivers; for existing examples see xen_blkbk_flush_diskcache() and xen_blkbk_discard(). Jan ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH] xen/xen-scsiback: Need go to fail after xenbus_dev_error() @ 2014-09-26 16:38 Chen Gang 0 siblings, 0 replies; 22+ messages in thread From: Chen Gang @ 2014-09-26 16:38 UTC (permalink / raw) To: David Vrabel; +Cc: xen-devel, linux-kernel When failure occurs, after xenbus_dev_error(), need go to fail to let upper caller know about it. Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com> --- drivers/xen/xen-scsiback.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c index 847bc9c..3e430e1 100644 --- a/drivers/xen/xen-scsiback.c +++ b/drivers/xen/xen-scsiback.c @@ -1222,8 +1222,10 @@ static int scsiback_probe(struct xenbus_device *dev, err = xenbus_printf(XBT_NIL, dev->nodename, "feature-sg-grant", "%u", SG_ALL); - if (err) + if (err) { xenbus_dev_error(dev, err, "writing feature-sg-grant"); + goto fail; + } xenbus_switch_state(dev, XenbusStateInitWait); return 0; -- 1.9.3 ^ permalink raw reply related [flat|nested] 22+ messages in thread
end of thread, other threads:[~2014-09-30 10:17 UTC | newest] Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-09-26 16:38 [PATCH] xen/xen-scsiback: Need go to fail after xenbus_dev_error() Chen Gang 2014-09-29 4:32 ` Juergen Gross 2014-09-29 4:32 ` [Xen-devel] " Juergen Gross 2014-09-29 8:41 ` Jan Beulich 2014-09-29 9:31 ` Chen Gang 2014-09-29 9:34 ` Juergen Gross 2014-09-29 9:34 ` [Xen-devel] " Juergen Gross 2014-09-29 9:59 ` Chen Gang 2014-09-29 9:59 ` [Xen-devel] " Chen Gang 2014-09-29 13:57 ` David Vrabel 2014-09-29 13:57 ` [Xen-devel] " David Vrabel 2014-09-30 6:32 ` Chen Gang 2014-09-30 6:59 ` Juergen Gross 2014-09-30 6:59 ` [Xen-devel] " Juergen Gross 2014-09-30 7:50 ` Chen Gang 2014-09-30 7:50 ` [Xen-devel] " Chen Gang 2014-09-30 10:23 ` Chen Gang 2014-09-30 10:23 ` Chen Gang 2014-09-30 6:32 ` Chen Gang 2014-09-29 9:31 ` Chen Gang 2014-09-29 8:41 ` Jan Beulich 2014-09-26 16:38 Chen Gang
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.