Re: [PATCH] read() from driverfs files can read more bytes

Re: [PATCH] read() from driverfs files can read more bytes

[Petr Vandrovec]

On  7 Feb 02 at 9:43, Patrick Mochel wrote:
> > And neither of driverfs_read_file nor driverfs_write_file supports
> > semantic we use with other filesystems: If at least one byte was 
> > read/written, return byte count (even if error happens). Only if zero 
> > bytes was written, return error code.
> 
> I would think that you would want to return the error code. Say you did:
> 
> echo "action parameter" > file
> 
> and 'parameter' is an invalid parameter, as determined by the driver. It 
> would require another arbitrary check to determine if the command 
> succeeded or not if it returned the number of bytes written. Returning 
> -EINVAL lets userspace know that it made a boo-boo. Is that not good?

If you want bidirectional communication, something like SOCK_SEQPACKET
or SOCK_DGRAM is better suitable. And if we learn open() to open
unix sockets, even 'echo' can be still used for configuring. I understand 
that it is radical change, but why these configuration points should
look like real files if they are not ones? And you do not have troubles
with supporting lseek() on them - if contents of file is "minutes\n",
is it correct to do lseek(fd, 1, SEEK_SET); write(fd, "onths\n", 6);
or is it incorrect usage?

If you do not want to change these objects from files to sockets,
I agree with your explanation for write(). But I do not agree with this
semantic for read() - reading byte after byte, and reading in one big 
chunk should not yield in different results on regular files, otherwise 
couple of nasty surprises is hidding there.
                                    Thanks,
                                        Petr Vandrovec
                                        vandrove@vc.cvut.cz
                                        

Re: [PATCH] read() from driverfs files can read more bytes

[Patrick Mochel]

On Thu, 7 Feb 2002, Petr Vandrovec wrote:

> On  7 Feb 02 at 9:43, Patrick Mochel wrote:
> > > And neither of driverfs_read_file nor driverfs_write_file supports
> > > semantic we use with other filesystems: If at least one byte was 
> > > read/written, return byte count (even if error happens). Only if zero 
> > > bytes was written, return error code.
> > 
> > I would think that you would want to return the error code. Say you did:
> > 
> > echo "action parameter" > file
> > 
> > and 'parameter' is an invalid parameter, as determined by the driver. It 
> > would require another arbitrary check to determine if the command 
> > succeeded or not if it returned the number of bytes written. Returning 
> > -EINVAL lets userspace know that it made a boo-boo. Is that not good?
> 
> If you want bidirectional communication, something like SOCK_SEQPACKET
> or SOCK_DGRAM is better suitable. And if we learn open() to open
> unix sockets, even 'echo' can be still used for configuring. I understand 
> that it is radical change, but why these configuration points should
> look like real files if they are not ones? And you do not have troubles
> with supporting lseek() on them - if contents of file is "minutes\n",
> is it correct to do lseek(fd, 1, SEEK_SET); write(fd, "onths\n", 6);
> or is it incorrect usage?
> 
> If you do not want to change these objects from files to sockets,
> I agree with your explanation for write(). But I do not agree with this
> semantic for read() - reading byte after byte, and reading in one big 
> chunk should not yield in different results on regular files, otherwise 
> couple of nasty surprises is hidding there.

Ok, I agree with your argument concerning read(). 

Concerning reading/writing from offsets, it's up to the drivers for them 
to either support it or not. In the files I've done so far, I return 0 if 
show() is called with an offset. Which will give different results if you 
read byte-by-byte or an entire chunk. 

It makes the callbacks simpler, but it is not technically correct. 

Something I did a while ago was implement default callbacks for basic 
types. So you would have something like:

default_show_u32(char * buf, size_t count, loff_t off, u32 * data)
{
	...
}

So the driver could do 

driver_show(struct device * dev, char * buf, size_t count, loff_t off)
{
	return default_show_u32(buf,count,off,&data);
}

And in the default_* handle the offset appropriately. 

If that seems reasonable, I'll dig it up and integrate it...

	-pat

Re: [PATCH] read() from driverfs files can read more bytes

[Pavel Machek]

Hi!

> Ok, I agree with your argument concerning read(). 
> 
> Concerning reading/writing from offsets, it's up to the drivers for them 
> to either support it or not. In the files I've done so far, I return 0 if 
> show() is called with an offset. Which will give different results if you 
> read byte-by-byte or an entire chunk. 
> 
> It makes the callbacks simpler, but it is not technically correct. 

[snip solution]

Why not just say those files are character devices? It is okay for
character device to behave like crazy ;-).
									Pavel
-- 
(about SSSCA) "I don't say this lightly.  However, I really think that the U.S.
no longer is classifiable as a democracy, but rather as a plutocracy." --hpa

Re: [PATCH] read() from driverfs files can read more bytes

[Petr Vandrovec]

On  7 Feb 02 at 10:27, Patrick Mochel wrote:

> Concerning reading/writing from offsets, it's up to the drivers for them 
> to either support it or not. In the files I've done so far, I return 0 if 
> show() is called with an offset. Which will give different results if you 
> read byte-by-byte or an entire chunk. 
> 
> It makes the callbacks simpler, but it is not technically correct. 

What about extremelly nice stuff Al Viro made for us in
fs/seq_file.c ? It made putting stuff into procfs really easy...
                                            Petr Vandrovec
                                            vandrove@vc.cvut.cz
                                            
 

Re: [PATCH] read() from driverfs files can read more bytes

[Patrick Mochel]

On Thu, 7 Feb 2002, Petr Vandrovec wrote:

> On  7 Feb 02 at 10:27, Patrick Mochel wrote:
> 
> > Concerning reading/writing from offsets, it's up to the drivers for them 
> > to either support it or not. In the files I've done so far, I return 0 if 
> > show() is called with an offset. Which will give different results if you 
> > read byte-by-byte or an entire chunk. 
> > 
> > It makes the callbacks simpler, but it is not technically correct. 
> 
> What about extremelly nice stuff Al Viro made for us in
> fs/seq_file.c ? It made putting stuff into procfs really easy...

It is really nice, but it's too much for the common case. The goal is to 
have each file export one and only one value. Setting up an iterator is 
overkill for one value.

	-pat

Re: [PATCH] read() from driverfs files can read more bytes

[Alexander Viro]

On Thu, 7 Feb 2002, Patrick Mochel wrote:

> It is really nice, but it's too much for the common case. The goal is to 
> have each file export one and only one value. Setting up an iterator is 
> overkill for one value.

You don't have to use the iterator side of that.

Re: [PATCH] read() from driverfs files can read more bytes

[Patrick Mochel]

On Thu, 7 Feb 2002, Alexander Viro wrote:

> 
> 
> On Thu, 7 Feb 2002, Patrick Mochel wrote:
> 
> > It is really nice, but it's too much for the common case. The goal is to 
> > have each file export one and only one value. Setting up an iterator is 
> > overkill for one value.
> 
> You don't have to use the iterator side of that.

Well, I'll be...

I like the seq_ stuff, and the ->read() side of things take care of the 
issues discussed in this thread. What's even nicer is that if I convert to 
that, driver callbacks become something like either:

int driver_show(struct device * dev, struct seq_file * m)

or 

int driver_show(struct device * dev, char * buf)


Have you considered doing write()?

	-pat

Re: [PATCH] read() from driverfs files can read more bytes

[Alexander Viro]

On Thu, 7 Feb 2002, Patrick Mochel wrote:

> issues discussed in this thread. What's even nicer is that if I convert to 
> that, driver callbacks become something like either:
> 
> int driver_show(struct device * dev, struct seq_file * m)
> 
> or 
> 
> int driver_show(struct device * dev, char * buf)

Preferably the former.
 
> Have you considered doing write()?

I had and that's going to be resurrected when remount() will be dealt
with (options-parsing both benefits a lot from and is a good testbed for
such helpers).

Re: [PATCH] read() from driverfs files can read more bytes

[Thomas Hood]

> Can you also check for size >= PAGE_SIZE on enter
> to entry->show() procedure?

FYI I recently wrote a patch for procfs which addresses a
similar problem there.  See the link at:
   http://panopticon.csustan.edu/thood/pnpbios.html

--
Thomas Hood

Re: [PATCH] read() from driverfs files can read more bytes

[Petr Vandrovec]

On  7 Feb 02 at 8:45, Patrick Mochel wrote:
> On Thu, 7 Feb 2002, Andrey Panin wrote:
> > Attached patch adds check that returned value is less then requested 
> > byte count. I know that actual callback function device_read_status()
> > should also be fixed, but I found this bug after midnight and 
> > decided to sleep a little :)
> 
> That sanity check was in there, once upon a time. However, in moving the 
> weight from the driver callbacks to the driverfs read_file() and 
> write_file(), it must have got dropped...
> 
> Thank you. It's been applied and will be pushed forward.

[I have only 2.5.3 sources here yet]

Can you also check for size >= PAGE_SIZE on enter to entry->show()
procedure? It looks ugly to me that each driver has to check for this
constant unless it wants to smash some innocent kernel memory.

And neither of driverfs_read_file nor driverfs_write_file supports
semantic we use with other filesystems: If at least one byte was 
read/written, return byte count (even if error happens). Only if zero 
bytes was written, return error code.
                                Thanks,
                                            Petr Vandrovec
                                            vandrove@vc.cvut.cz

Re: [PATCH] read() from driverfs files can read more bytes

[Patrick Mochel]

> Can you also check for size >= PAGE_SIZE on enter to entry->show()
> procedure? It looks ugly to me that each driver has to check for this
> constant unless it wants to smash some innocent kernel memory.

Done. Thanks.

> And neither of driverfs_read_file nor driverfs_write_file supports
> semantic we use with other filesystems: If at least one byte was 
> read/written, return byte count (even if error happens). Only if zero 
> bytes was written, return error code.

I would think that you would want to return the error code. Say you did:

echo "action parameter" > file

and 'parameter' is an invalid parameter, as determined by the driver. It 
would require another arbitrary check to determine if the command 
succeeded or not if it returned the number of bytes written. Returning 
-EINVAL lets userspace know that it made a boo-boo. Is that not good?

	-pat