Re: [PATCH] seq_file:update file->f_pos when lseek() to m->read_pos

[Gu Zheng]

Hi Jiaxing,
	Please refer to inline comment.:)
On 07/02/2013 08:43 AM, Jiaxing Wang wrote:

> On 07/01/2013 08:41 PM, fangdong wrote:
>> On 06/29/2013 05:11 AM, Jiaxing Wang wrote:
>>> After pread(), file->f_pos and m->read_pos get different,
>>> and lseek() to m->read_pos did not update file->f_pos, then
>>> a subsequent read may read from a wrong position, the following
>>> program shows the problem:
>>>
>>>      char str1[32] = { 0 };
>>>      char str2[32] = { 0 };
>>>      int poffset = 10;
>>>      int count = 20;
>>>
>>>      /*open any seq file*/
>>>      int fd = open("/proc/modules", O_RDONLY);
>>>
>>>      pread(fd, str1, count, poffset);
>>>      printf("pread:%s\n", str1);
>>>
>>>      /*seek to where m->read_pos is*/
>>>      lseek(fd, poffset+count, SEEK_SET);
>>>
>>>      /*supposed to read from poffset+count, but this read from position 0*/
>>>      read(fd, str2, count);
>>>      printf("read:%s\n", str2);
>>>
>>> Signed-off-by: Jiaxing Wang <hello.wjx@xxxxxxxxx>
>>> ---
>>>   fs/seq_file.c | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/fs/seq_file.c b/fs/seq_file.c
>>> index 774c1eb..4b22b26 100644
>>> --- a/fs/seq_file.c
>>> +++ b/fs/seq_file.c
>>> @@ -328,7 +328,8 @@ loff_t seq_lseek(struct file *file, loff_t offset, int whence)
>>>                   m->read_pos = offset;
>>>                   retval = file->f_pos = offset;
>>>               }
>>> -        }
>>> +        } else
>>> +            file->f_pos = offset;
>>>       }
>>>       file->f_version = m->version;
>>>       mutex_unlock(&m->lock);
>>>
>> This does not appear to be a problem, in linux man page, the behaver seems clearly defined:
>>
>> DESCRIPTION
>>        pread() reads up to count bytes from file descriptor fd at offset  off-set  (from the start of the file) into the buffer starting at buf.  The file offset is not changed.
>>
>>        pwrite() writes up to count bytes from the buffer starting at buf  to the  file  descriptor  fd  at  offset  offset.   The file offset is not changed.
>>
>> -- 
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
> There's no problem that pread() don't change file->f_pos, but I think lseek() should have changed it.

No, the way that seq_seek deal with pos is right. Remember this seq file, it is
an iterator interface, e.g. it provides infos of each element of a list, so the
we should make sure that the info we read from seq file is the whole infos of
each entry, so seq_lseek can not set pos to a optional point, only can on the
points that divided by a whole infos size of each entry. If we can not set pos
on the special position, ste it as 0, I think it's reasonable.

Thanks,
Gu  

> Any comments from other people, and Alexander?
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/