[PATCH] s390: Fix me in dasd_eer.c

[PATCH] s390: Fix me in dasd_eer.c

[Nicholas Krause]

This patch changes return type to EMEDUIMTYPE in function, dasd_eer_enable
for when checking if the medium has no errors according to this function.

Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
---
 drivers/s390/block/dasd_eer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/s390/block/dasd_eer.c b/drivers/s390/block/dasd_eer.c
index 21ef63c..08ee040 100644
--- a/drivers/s390/block/dasd_eer.c
+++ b/drivers/s390/block/dasd_eer.c
@@ -462,7 +462,7 @@ int dasd_eer_enable(struct dasd_device *device)
 		return 0;
 
 	if (!device->discipline || strcmp(device->discipline->name, "ECKD"))
-		return -EPERM;	/* FIXME: -EMEDIUMTYPE ? */
+		return -EMEDIUMTYPE;	/* FIXME: -EMEDIUMTYPE ? */
 
 	cqr = dasd_kmalloc_request(DASD_ECKD_MAGIC, 1 /* SNSS */,
 				   SNSS_DATA_SIZE, device);
-- 
1.9.1

Re: [PATCH] s390: Fix me in dasd_eer.c

[One Thousand Gnomes]

On Tue, 22 Jul 2014 02:29:32 -0400
Nicholas Krause <xerofoify@gmail.com> wrote:

> This patch changes return type to EMEDUIMTYPE in function, dasd_eer_enable
> for when checking if the medium has no errors according to this function.
> 
> Signed-off-by: Nicholas Krause <xerofoify@gmail.com>

What distributions have you tested this with and what configurations?

Re: [PATCH] s390: Fix me in dasd_eer.c

[Nick Krause]

On Wed, Jul 23, 2014 at 8:18 AM, One Thousand Gnomes
<gnomes@lxorguk.ukuu.org.uk> wrote:
> On Tue, 22 Jul 2014 02:29:32 -0400
> Nicholas Krause <xerofoify@gmail.com> wrote:
>
>> This patch changes return type to EMEDUIMTYPE in function, dasd_eer_enable
>> for when checking if the medium has no errors according to this function.
>>
>> Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
>
> What distributions have you tested this with and what configurations?

I have not tested this patch but based on the logical statements this
seems to be
correct.
Nick

Re: [PATCH] s390: Fix me in dasd_eer.c

[Stefan Weinhuber]

Nicholas Krause <xerofoify@gmail.com> wrote on 2014-07-22 08:29:32:

[..] 
> Subject:
> 
> [PATCH] s390: Fix me in dasd_eer.c
> 
> This patch changes return type to EMEDUIMTYPE in function, 
dasd_eer_enable
> for when checking if the medium has no errors according to this 
function.
> 
> Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
> ---
>  drivers/s390/block/dasd_eer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/s390/block/dasd_eer.c 
b/drivers/s390/block/dasd_eer.c
> index 21ef63c..08ee040 100644
> --- a/drivers/s390/block/dasd_eer.c
> +++ b/drivers/s390/block/dasd_eer.c
> @@ -462,7 +462,7 @@ int dasd_eer_enable(struct dasd_device *device)
>        return 0;
> 
>     if (!device->discipline || strcmp(device->discipline->name, "ECKD"))
> -      return -EPERM;   /* FIXME: -EMEDIUMTYPE ? */
> +      return -EMEDIUMTYPE;   /* FIXME: -EMEDIUMTYPE ? */
> 
>     cqr = dasd_kmalloc_request(DASD_ECKD_MAGIC, 1 /* SNSS */,
>                 SNSS_DATA_SIZE, device);
> -- 
> 1.9.1
>

Hm, after some consideration, I have to disagree with your suggestion.
If you try to enable EER on an FBA device, you will get the following
result with current code:

$ echo 1 > eer_enabled
-bash: echo: write error: Operation not permitted

and with your code the following:

$ echo 1 > eer_enabled
-bash: echo: write error: Wrong medium type

When I wrote this code, I was not sure which one is better. But today I 
say
that the 'Operation not permitted' is more to the point. An FBA device
has no (changable) medium, so what could be wrong about its type?
Could be confusing.

>From your patch description I do not really get why you want to change the
return code. Why do you think that EMEDIUMTYPE is better than EPERM?

Mit freundlichen Grüßen / Kind regards
 
Stefan Weinhuber

Re: [PATCH] s390: Fix me in dasd_eer.c

[Stefan Weinhuber]

Nicholas Krause <xerofoify@gmail.com> wrote on 2014-07-22 08:29:32:

[..] 
> Subject:
> 
> [PATCH] s390: Fix me in dasd_eer.c
> 
> This patch changes return type to EMEDUIMTYPE in function, 
dasd_eer_enable
> for when checking if the medium has no errors according to this 
function.
> 
> Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
> ---
>  drivers/s390/block/dasd_eer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/s390/block/dasd_eer.c 
b/drivers/s390/block/dasd_eer.c
> index 21ef63c..08ee040 100644
> --- a/drivers/s390/block/dasd_eer.c
> +++ b/drivers/s390/block/dasd_eer.c
> @@ -462,7 +462,7 @@ int dasd_eer_enable(struct dasd_device *device)
>        return 0;
> 
>     if (!device->discipline || strcmp(device->discipline->name, "ECKD"))
> -      return -EPERM;   /* FIXME: -EMEDIUMTYPE ? */
> +      return -EMEDIUMTYPE;   /* FIXME: -EMEDIUMTYPE ? */
> 
>     cqr = dasd_kmalloc_request(DASD_ECKD_MAGIC, 1 /* SNSS */,
>                 SNSS_DATA_SIZE, device);
> -- 
> 1.9.1
>

Hm, after some consideration, I have to disagree with your suggestion.
If you try to enable EER on an FBA device, you will get the following
result with current code:

$ echo 1 > eer_enabled
-bash: echo: write error: Operation not permitted

and with your code the following:

$ echo 1 > eer_enabled
-bash: echo: write error: Wrong medium type

When I wrote this code, I was not sure which one is better. But today I 
say
that the 'Operation not permitted' is more to the point. An FBA device
has no (changable) medium, so what could be wrong about its type?
Could be confusing.

From your patch description I do not really get why you want to change the
return code. Why do you think that EMEDIUMTYPE is better than EPERM?

Mit freundlichen Grüßen / Kind regards
 
Stefan Weinhuber

Re: [PATCH] s390: Fix me in dasd_eer.c

[Nick Krause]

On Mon, Jul 28, 2014 at 12:02 PM, Stefan Weinhuber <WEIN@de.ibm.com> wrote:
> Nicholas Krause <xerofoify@gmail.com> wrote on 2014-07-22 08:29:32:
>
> [..]
>> Subject:
>>
>> [PATCH] s390: Fix me in dasd_eer.c
>>
>> This patch changes return type to EMEDUIMTYPE in function,
> dasd_eer_enable
>> for when checking if the medium has no errors according to this
> function.
>>
>> Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
>> ---
>>  drivers/s390/block/dasd_eer.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/s390/block/dasd_eer.c
> b/drivers/s390/block/dasd_eer.c
>> index 21ef63c..08ee040 100644
>> --- a/drivers/s390/block/dasd_eer.c
>> +++ b/drivers/s390/block/dasd_eer.c
>> @@ -462,7 +462,7 @@ int dasd_eer_enable(struct dasd_device *device)
>>        return 0;
>>
>>     if (!device->discipline || strcmp(device->discipline->name, "ECKD"))
>> -      return -EPERM;   /* FIXME: -EMEDIUMTYPE ? */
>> +      return -EMEDIUMTYPE;   /* FIXME: -EMEDIUMTYPE ? */
>>
>>     cqr = dasd_kmalloc_request(DASD_ECKD_MAGIC, 1 /* SNSS */,
>>                 SNSS_DATA_SIZE, device);
>> --
>> 1.9.1
>>
>
> Hm, after some consideration, I have to disagree with your suggestion.
> If you try to enable EER on an FBA device, you will get the following
> result with current code:
>
> $ echo 1 > eer_enabled
> -bash: echo: write error: Operation not permitted
>
> and with your code the following:
>
> $ echo 1 > eer_enabled
> -bash: echo: write error: Wrong medium type
>
> When I wrote this code, I was not sure which one is better. But today I
> say
> that the 'Operation not permitted' is more to the point. An FBA device
> has no (changable) medium, so what could be wrong about its type?
> Could be confusing.
>
> From your patch description I do not really get why you want to change the
> return code. Why do you think that EMEDIUMTYPE is better than EPERM?
>
> Mit freundlichen Grüßen / Kind regards
>
> Stefan Weinhuber
>
Stefan,
>From my reading seemed more like EMEDUIMTYPE return but seems I was
wrong here. If you want I can remove the fix me message in a patch if you
want to keep this return type.
Regards Nick

Re: [PATCH] s390: Fix me in dasd_eer.c

[Nick Krause]

On Mon, Jul 28, 2014 at 12:02 PM, Stefan Weinhuber <WEIN@de.ibm.com> wrote:
> Nicholas Krause <xerofoify@gmail.com> wrote on 2014-07-22 08:29:32:
>
> [..]
>> Subject:
>>
>> [PATCH] s390: Fix me in dasd_eer.c
>>
>> This patch changes return type to EMEDUIMTYPE in function,
> dasd_eer_enable
>> for when checking if the medium has no errors according to this
> function.
>>
>> Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
>> ---
>>  drivers/s390/block/dasd_eer.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/s390/block/dasd_eer.c
> b/drivers/s390/block/dasd_eer.c
>> index 21ef63c..08ee040 100644
>> --- a/drivers/s390/block/dasd_eer.c
>> +++ b/drivers/s390/block/dasd_eer.c
>> @@ -462,7 +462,7 @@ int dasd_eer_enable(struct dasd_device *device)
>>        return 0;
>>
>>     if (!device->discipline || strcmp(device->discipline->name, "ECKD"))
>> -      return -EPERM;   /* FIXME: -EMEDIUMTYPE ? */
>> +      return -EMEDIUMTYPE;   /* FIXME: -EMEDIUMTYPE ? */
>>
>>     cqr = dasd_kmalloc_request(DASD_ECKD_MAGIC, 1 /* SNSS */,
>>                 SNSS_DATA_SIZE, device);
>> --
>> 1.9.1
>>
>
> Hm, after some consideration, I have to disagree with your suggestion.
> If you try to enable EER on an FBA device, you will get the following
> result with current code:
>
> $ echo 1 > eer_enabled
> -bash: echo: write error: Operation not permitted
>
> and with your code the following:
>
> $ echo 1 > eer_enabled
> -bash: echo: write error: Wrong medium type
>
> When I wrote this code, I was not sure which one is better. But today I
> say
> that the 'Operation not permitted' is more to the point. An FBA device
> has no (changable) medium, so what could be wrong about its type?
> Could be confusing.
>
> From your patch description I do not really get why you want to change the
> return code. Why do you think that EMEDIUMTYPE is better than EPERM?
>
> Mit freundlichen Grüßen / Kind regards
>
> Stefan Weinhuber
>
Stefan,
From my reading seemed more like EMEDUIMTYPE return but seems I was
wrong here. If you want I can remove the fix me message in a patch if you
want to keep this return type.
Regards Nick