[PATCH] xen/xen-scsiback: Need go to fail after xenbus_dev_error()

[PATCH] xen/xen-scsiback: Need go to fail after xenbus_dev_error()

[Chen Gang]

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

Re: [Xen-devel] [PATCH] xen/xen-scsiback: Need go to fail after xenbus_dev_error()

[Juergen Gross]

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>

Re: [Xen-devel] [PATCH] xen/xen-scsiback: Need go to fail after xenbus_dev_error()

[Jan Beulich]

>>> 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

Re: [Xen-devel] [PATCH] xen/xen-scsiback: Need go to fail after xenbus_dev_error()

[Chen Gang]

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

Re: [Xen-devel] [PATCH] xen/xen-scsiback: Need go to fail after xenbus_dev_error()

[Juergen Gross]

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

Re: [Xen-devel] [PATCH] xen/xen-scsiback: Need go to fail after xenbus_dev_error()

[Chen Gang]

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

Re: [Xen-devel] [PATCH] xen/xen-scsiback: Need go to fail after xenbus_dev_error()

[David Vrabel]

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

Re: [Xen-devel] [PATCH] xen/xen-scsiback: Need go to fail after xenbus_dev_error()

[Chen Gang]

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

Re: [Xen-devel] [PATCH] xen/xen-scsiback: Need go to fail after xenbus_dev_error()

[Juergen Gross]

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

Re: [Xen-devel] [PATCH] xen/xen-scsiback: Need go to fail after xenbus_dev_error()

[Chen Gang]

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

Re: [Xen-devel] [PATCH] xen/xen-scsiback: Need go to fail after xenbus_dev_error()

[Chen Gang]

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