Re: [PATCH 1/1] cio_ignore_proc_seq_next should increase position index

[Cornelia Huck <cohuck@redhat.com>]

On Fri, 7 Feb 2020 14:13:05 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 24.01.20 06:48, Vasily Averin wrote:
> > if seq_file .next fuction does not change position index,
> > read after some lseek can generate unexpected output.
> > 
> > https://bugzilla.kernel.org/show_bug.cgi?id=206283
> > Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
> > ---
> >  drivers/s390/cio/blacklist.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/s390/cio/blacklist.c b/drivers/s390/cio/blacklist.c
> > index 2a3f874..9cebff8 100644
> > --- a/drivers/s390/cio/blacklist.c
> > +++ b/drivers/s390/cio/blacklist.c
> > @@ -303,8 +303,10 @@ struct ccwdev_iter {
> >  cio_ignore_proc_seq_next(struct seq_file *s, void *it, loff_t *offset)
> >  {
> >  	struct ccwdev_iter *iter;
> > +	loff_t p = *offset;
> >  
> > -	if (*offset >= (__MAX_SUBCHANNEL + 1) * (__MAX_SSID + 1))
> > +	(*offset)++;
> > +	if (p >= (__MAX_SUBCHANNEL + 1) * (__MAX_SSID + 1))
> >  		return NULL;
> >  	iter = it;
> >  	if (iter->devno == __MAX_SUBCHANNEL) {
> > @@ -314,7 +316,6 @@ struct ccwdev_iter {
> >  			return NULL;
> >  	} else
> >  		iter->devno++;
> > -	(*offset)++;
> >  	return iter;
> >  }
> >  
> >   
> 
> I guess this fixes one aspect:
> "dd: /proc/cio_ignore: cannot skip to specified offset"
> is now gone. So I am tempted to apply this. 

This is definitely an improvement.

> 
> but this code is still fishy:

I'm surprised it took that long; it's been 14 years since I messed
up^W^Wwrote this and there's basically only been a memory leak fix from
you in the meantime... that said, ...

> 
> $ cat /proc/cio_ignore 
> 0.0.fe00-0.0.fefe
> 0.0.ff00-0.0.ffff
> $ dd if=/proc/cio_ignore status=none
> 0.0.fe00-0.0.fefe
> 0.0.ff00-0.0.ffff
> $ dd if=/proc/cio_ignore status=none bs=10
> 0.0.fe00-0.0.fefe
> 0.0.ff00-0.0.ff01-0.0.ff02-0.0.ff03-0.0.ff04-0.0.ff05-0.0.ff06-0.0.ff07-0.0.ff08-0.0.ffff
> $ dd if=/proc/cio_ignore status=none bs=10 skip=1
> .0.fefe
> 0.0.ff00-0.0.ff01-0.0.ff02-0.0.ff03-0.0.ff04-0.0.ff05-0.0.ff06-0.0.ff07-0.0.ff08-0.0.ffff

...what we are doing is translating something that is basically a
per-possible-device value into a range, as otherwise the output would
be quite unreadable for humans. I'm not sure what the semantics should
be if you read in small chunks etc., as the ranges are assembled
on-the-fly.

> Peter, any opinions on this?

I *think* I originally modeled /proc/cio_ignore on a long-gone dasd
procfs file (a very long time before converting it to a seq file); do
we have any other examples of files that do a similar
individual-values-to-ranges conversion?