Re: [PATCH] fs: replace int param with size_t for seq_open_private()

Re: [PATCH] fs: replace int param with size_t for seq_open_private()

[Alexey Dobriyan]

>  void *__seq_open_private(struct file *f, const struct seq_operations *ops,
> - int psize)
> + size_t psize)

It should be "unsigned int" at most.
As almost all in-kernel lengths.

    Alexey

Re: [PATCH] fs: replace int param with size_t for seq_open_private()

[Rob Jones]

I don't agree. Ultimately this parameter ends up as a parameter to
kmalloc where it is expected to be a size_t.

On 01/09/14 14:43, Alexey Dobriyan wrote:
>>   void *__seq_open_private(struct file *f, const struct seq_operations *ops,
>> - int psize)
>> + size_t psize)
>
> It should be "unsigned int" at most.
> As almost all in-kernel lengths.
>
>      Alexey
>
>

-- 
Rob Jones
Codethink Ltd
mailto:rob.jones@codethink.co.uk
tel:+44 161 236 5575

Re: [PATCH] fs: replace int param with size_t for seq_open_private()

[Alexey Dobriyan]

On Mon, Sep 1, 2014 at 4:54 PM, Rob Jones <rob.jones@codethink.co.uk> wrote:
>
>
> On 01/09/14 14:43, Alexey Dobriyan wrote:
>>>
>>>   void *__seq_open_private(struct file *f, const struct seq_operations
>>> *ops,
>>> - int psize)
>>> + size_t psize)
>>
>>
>> It should be "unsigned int" at most.
>> As almost all in-kernel lengths.

> I don't agree. Ultimately this parameter ends up as a parameter to
> kmalloc where it is expected to be a size_t.

Which is a mistake too because allocations are never that large.

Save REX prefix, keep length unsigned int!

   Alexey

Re: [PATCH] fs: replace int param with size_t for seq_open_private()

[Rob Jones]

On 01/09/14 15:13, Alexey Dobriyan wrote:
> On Mon, Sep 1, 2014 at 4:54 PM, Rob Jones <rob.jones@codethink.co.uk> wrote:
>>
>>
>> On 01/09/14 14:43, Alexey Dobriyan wrote:
>>>>
>>>>    void *__seq_open_private(struct file *f, const struct seq_operations
>>>> *ops,
>>>> - int psize)
>>>> + size_t psize)
>>>
>>>
>>> It should be "unsigned int" at most.
>>> As almost all in-kernel lengths.
>
>> I don't agree. Ultimately this parameter ends up as a parameter to
>> kmalloc where it is expected to be a size_t.
>
> Which is a mistake too because allocations are never that large.

Yet.

>
> Save REX prefix, keep length unsigned int!

I am happy to do this if the general opinion is that it is the right
thing but I would like more input first. Using size_t was suggested by
Andrew Morton which sounds to me like a heavy gun on the side of size_t.

>
>     Alexey
>
>

-- 
Rob Jones
Codethink Ltd
mailto:rob.jones@codethink.co.uk
tel:+44 161 236 5575

Re: [PATCH] fs: replace int param with size_t for seq_open_private()

[Al Viro]

On Mon, Sep 01, 2014 at 03:38:51PM +0100, Rob Jones wrote:
> >>kmalloc where it is expected to be a size_t.
> >
> >Which is a mistake too because allocations are never that large.
> 
> Yet.

*raised eyebrow*

You do realize that kmalloc() gives physically contiguous allocation, right?
And refuses to allocate more than KMALLOC_MAX_SIZE, while we are at it.
With allocations anywhere near such range being very heavily discouraged.

There might or might not be point in using size_t for kmalloc() argument,
but "future-proofing" isn't it.

Re: [PATCH] fs: replace int param with size_t for seq_open_private()

[Rob Jones]

On 01/09/14 22:22, Al Viro wrote:
> On Mon, Sep 01, 2014 at 03:38:51PM +0100, Rob Jones wrote:
>>>> kmalloc where it is expected to be a size_t.
>>>
>>> Which is a mistake too because allocations are never that large.
>>
>> Yet.
>
> *raised eyebrow*
>
> You do realize that kmalloc() gives physically contiguous allocation, right?

Do please try to not be quite so patronizing. It's very counter-
productive.

> And refuses to allocate more than KMALLOC_MAX_SIZE, while we are at it.
> With allocations anywhere near such range being very heavily discouraged.
>
> There might or might not be point in using size_t for kmalloc() argument,
> but "future-proofing" isn't it.

Indeed, and I am following up those arguments with people that may want
to be constructive.

>
>

-- 
Rob Jones
Codethink Ltd
mailto:rob.jones@codethink.co.uk
tel:+44 161 236 5575

Re: [PATCH] fs: replace int param with size_t for seq_open_private()

[Rob Jones]

On 12/09/14 15:16, Richard Weinberger wrote:
> On Thu, Sep 11, 2014 at 6:25 PM, Rob Jones <rob.jones@codethink.co.uk> wrote:
>>
>>
>> On 01/09/14 16:36, Al Viro wrote:
>>>
>>> On Mon, Sep 01, 2014 at 02:17:08PM +0100, Rob Jones wrote:
>>>
>>>>    void *__seq_open_private(struct file *f, const struct seq_operations
>>>> *ops,
>>>> -               int psize)
>>>> +               size_t psize)
>>>
>>>
>>> <sarcasm>
>>> It is a horrible limitation to impose, indeed.  Why, a lousy
>>> 2 gigabytes per line in procfs file - that's intolerable...
>>> </sarcasm>
>>>
>>>
>>
>> OK, I know this is a trivial patch but I've gone away and thought about
>> it and done some reading to see what the rest of the world thinks about
>> using size_t vs unsigned int (signed int is an abomination in this
>> context regardless).
>>
>> I think Al's sarcasm is misplaced.
>>
>> The correct type to use here *is* size_t. It's about consistency and,
>> more importantly, it's about not making assumptions about the hardware
>> architecture. It's included in the language for very good reasons and
>> it seems to me to be risky to ignore those reasons.
>
> Please don't forget to patch all for loops to use size_t instead of int too.
>

Yes, I'm sure we've all read that argument too. Now try behaving like a 
grown up.

-- 
Rob Jones
Codethink Ltd
mailto:rob.jones@codethink.co.uk
tel:+44 161 236 5575

Re: [PATCH] fs: replace int param with size_t for seq_open_private()

[Richard Weinberger]

On Thu, Sep 11, 2014 at 6:25 PM, Rob Jones <rob.jones@codethink.co.uk> wrote:
>
>
> On 01/09/14 16:36, Al Viro wrote:
>>
>> On Mon, Sep 01, 2014 at 02:17:08PM +0100, Rob Jones wrote:
>>
>>>   void *__seq_open_private(struct file *f, const struct seq_operations
>>> *ops,
>>> -               int psize)
>>> +               size_t psize)
>>
>>
>> <sarcasm>
>> It is a horrible limitation to impose, indeed.  Why, a lousy
>> 2 gigabytes per line in procfs file - that's intolerable...
>> </sarcasm>
>>
>>
>
> OK, I know this is a trivial patch but I've gone away and thought about
> it and done some reading to see what the rest of the world thinks about
> using size_t vs unsigned int (signed int is an abomination in this
> context regardless).
>
> I think Al's sarcasm is misplaced.
>
> The correct type to use here *is* size_t. It's about consistency and,
> more importantly, it's about not making assumptions about the hardware
> architecture. It's included in the language for very good reasons and
> it seems to me to be risky to ignore those reasons.

Please don't forget to patch all for loops to use size_t instead of int too.

-- 
Thanks,
//richard

Re: [PATCH] fs: replace int param with size_t for seq_open_private()

[Rob Jones]

On 01/09/14 16:36, Al Viro wrote:
> On Mon, Sep 01, 2014 at 02:17:08PM +0100, Rob Jones wrote:
>
>>   void *__seq_open_private(struct file *f, const struct seq_operations *ops,
>> -		int psize)
>> +		size_t psize)
>
> <sarcasm>
> It is a horrible limitation to impose, indeed.  Why, a lousy
> 2 gigabytes per line in procfs file - that's intolerable...
> </sarcasm>
>
>

OK, I know this is a trivial patch but I've gone away and thought about
it and done some reading to see what the rest of the world thinks about
using size_t vs unsigned int (signed int is an abomination in this
context regardless).

I think Al's sarcasm is misplaced.

The correct type to use here *is* size_t. It's about consistency and,
more importantly, it's about not making assumptions about the hardware
architecture. It's included in the language for very good reasons and
it seems to me to be risky to ignore those reasons.

I would like the patch to be considered for inclusion, it costs nothing
and could avoid a future problem coming up to bite us in the bum.

-- 
Rob Jones
Codethink Ltd
mailto:rob.jones@codethink.co.uk
tel:+44 161 236 5575

Re: [PATCH] fs: replace int param with size_t for seq_open_private()

[Rob Jones]

On 01/09/14 16:36, Al Viro wrote:
> On Mon, Sep 01, 2014 at 02:17:08PM +0100, Rob Jones wrote:
>
>>   void *__seq_open_private(struct file *f, const struct seq_operations *ops,
>> -		int psize)
>> +		size_t psize)
>
> <sarcasm>
> It is a horrible limitation to impose, indeed.  Why, a lousy
> 2 gigabytes per line in procfs file - that's intolerable...
> </sarcasm>
>

<shrug>

Consistency?

kmalloc() expects a size_t and I haven't heard anyone whingeing about that.

Ultimately, this function calls kmalloc().

>

-- 
Rob Jones
Codethink Ltd
mailto:rob.jones@codethink.co.uk
tel:+44 161 236 5575

Re: [PATCH] fs: replace int param with size_t for seq_open_private()

[Al Viro]

On Mon, Sep 01, 2014 at 02:17:08PM +0100, Rob Jones wrote:

>  void *__seq_open_private(struct file *f, const struct seq_operations *ops,
> -		int psize)
> +		size_t psize)

<sarcasm>
It is a horrible limitation to impose, indeed.  Why, a lousy
2 gigabytes per line in procfs file - that's intolerable...
</sarcasm> 

[PATCH] fs: replace int param with size_t for seq_open_private()

[Rob Jones]

also for __seq_open_private()

Resubmitted due to email address typo.

Signed-off-by: Rob Jones <rob.jones@codethink.co.uk>
---
 fs/seq_file.c            |    4 ++--
 include/linux/seq_file.h |    4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/seq_file.c b/fs/seq_file.c
index 1d641bb..dc2dfec 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -640,7 +640,7 @@ int seq_release_private(struct inode *inode, struct file *file)
 EXPORT_SYMBOL(seq_release_private);
 
 void *__seq_open_private(struct file *f, const struct seq_operations *ops,
-		int psize)
+		size_t psize)
 {
 	int rc;
 	void *private;
@@ -666,7 +666,7 @@ out:
 EXPORT_SYMBOL(__seq_open_private);
 
 int seq_open_private(struct file *filp, const struct seq_operations *ops,
-		int psize)
+		size_t psize)
 {
 	return __seq_open_private(filp, ops, psize) ? 0 : -ENOMEM;
 }
diff --git a/include/linux/seq_file.h b/include/linux/seq_file.h
index 52e0097..9382339 100644
--- a/include/linux/seq_file.h
+++ b/include/linux/seq_file.h
@@ -140,8 +140,8 @@ static inline int seq_nodemask_list(struct seq_file *m, nodemask_t *mask)
 int single_open(struct file *, int (*)(struct seq_file *, void *), void *);
 int single_open_size(struct file *, int (*)(struct seq_file *, void *), void *, size_t);
 int single_release(struct inode *, struct file *);
-void *__seq_open_private(struct file *, const struct seq_operations *, int);
-int seq_open_private(struct file *, const struct seq_operations *, int);
+void *__seq_open_private(struct file *, const struct seq_operations *, size_t);
+int seq_open_private(struct file *, const struct seq_operations *, size_t);
 int seq_release_private(struct inode *, struct file *);
 int seq_put_decimal_ull(struct seq_file *m, char delimiter,
 			unsigned long long num);
-- 
1.7.10.4

Re: [PATCH] fs: replace int param with size_t for seq_open_private()

[Rob Jones]

Resubmitting - mistranscribed Alexander Viro's email address.

On 01/09/14 14:13, Rob Jones wrote:
> also for __seq_open_private()
>
> Signed-off-by: Rob Jones <rob.jones@codethink.co.uk>
> ---
>   fs/seq_file.c            |    4 ++--
>   include/linux/seq_file.h |    4 ++--
>   2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/fs/seq_file.c b/fs/seq_file.c
> index 1d641bb..dc2dfec 100644
> --- a/fs/seq_file.c
> +++ b/fs/seq_file.c
> @@ -640,7 +640,7 @@ int seq_release_private(struct inode *inode, struct file *file)
>   EXPORT_SYMBOL(seq_release_private);
>
>   void *__seq_open_private(struct file *f, const struct seq_operations *ops,
> -		int psize)
> +		size_t psize)
>   {
>   	int rc;
>   	void *private;
> @@ -666,7 +666,7 @@ out:
>   EXPORT_SYMBOL(__seq_open_private);
>
>   int seq_open_private(struct file *filp, const struct seq_operations *ops,
> -		int psize)
> +		size_t psize)
>   {
>   	return __seq_open_private(filp, ops, psize) ? 0 : -ENOMEM;
>   }
> diff --git a/include/linux/seq_file.h b/include/linux/seq_file.h
> index 52e0097..9382339 100644
> --- a/include/linux/seq_file.h
> +++ b/include/linux/seq_file.h
> @@ -140,8 +140,8 @@ static inline int seq_nodemask_list(struct seq_file *m, nodemask_t *mask)
>   int single_open(struct file *, int (*)(struct seq_file *, void *), void *);
>   int single_open_size(struct file *, int (*)(struct seq_file *, void *), void *, size_t);
>   int single_release(struct inode *, struct file *);
> -void *__seq_open_private(struct file *, const struct seq_operations *, int);
> -int seq_open_private(struct file *, const struct seq_operations *, int);
> +void *__seq_open_private(struct file *, const struct seq_operations *, size_t);
> +int seq_open_private(struct file *, const struct seq_operations *, size_t);
>   int seq_release_private(struct inode *, struct file *);
>   int seq_put_decimal_ull(struct seq_file *m, char delimiter,
>   			unsigned long long num);
>

-- 
Rob Jones
Codethink Ltd
mailto:rob.jones@codethink.co.uk
tel:+44 161 236 5575

[PATCH] fs: replace int param with size_t for seq_open_private()

[Rob Jones]

also for __seq_open_private()

Signed-off-by: Rob Jones <rob.jones@codethink.co.uk>
---
 fs/seq_file.c            |    4 ++--
 include/linux/seq_file.h |    4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/seq_file.c b/fs/seq_file.c
index 1d641bb..dc2dfec 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -640,7 +640,7 @@ int seq_release_private(struct inode *inode, struct file *file)
 EXPORT_SYMBOL(seq_release_private);
 
 void *__seq_open_private(struct file *f, const struct seq_operations *ops,
-		int psize)
+		size_t psize)
 {
 	int rc;
 	void *private;
@@ -666,7 +666,7 @@ out:
 EXPORT_SYMBOL(__seq_open_private);
 
 int seq_open_private(struct file *filp, const struct seq_operations *ops,
-		int psize)
+		size_t psize)
 {
 	return __seq_open_private(filp, ops, psize) ? 0 : -ENOMEM;
 }
diff --git a/include/linux/seq_file.h b/include/linux/seq_file.h
index 52e0097..9382339 100644
--- a/include/linux/seq_file.h
+++ b/include/linux/seq_file.h
@@ -140,8 +140,8 @@ static inline int seq_nodemask_list(struct seq_file *m, nodemask_t *mask)
 int single_open(struct file *, int (*)(struct seq_file *, void *), void *);
 int single_open_size(struct file *, int (*)(struct seq_file *, void *), void *, size_t);
 int single_release(struct inode *, struct file *);
-void *__seq_open_private(struct file *, const struct seq_operations *, int);
-int seq_open_private(struct file *, const struct seq_operations *, int);
+void *__seq_open_private(struct file *, const struct seq_operations *, size_t);
+int seq_open_private(struct file *, const struct seq_operations *, size_t);
 int seq_release_private(struct inode *, struct file *);
 int seq_put_decimal_ull(struct seq_file *m, char delimiter,
 			unsigned long long num);
-- 
1.7.10.4