Re: [PATCH] relayfs redux, part 2

[Tom Zanussi <zanussi@us.ibm.com>]

Roman Zippel writes:
 > Hi,
 > 
 > On Fri, 28 Jan 2005, Tom Zanussi wrote:
 > 
 > > +static inline int rchan_create_file(const char *chanpath,
 > > +				    struct dentry **dentry,
 > > +				    struct rchan_buf *data)
 > > +{
 > > +	int err;
 > > +	const char * fname;
 > > +	struct dentry *topdir;
 > > +
 > > +	err = rchan_create_dir(chanpath, &fname, &topdir);
 > > +	if (err && (err != -EEXIST))
 > > +		return err;
 > > +
 > > +	err = relayfs_create_file(fname, topdir, dentry, data, S_IRUSR);
 > > +
 > > +	return err;
 > > +}
 > 
 > What protects topdir from being removed inbetween?
 > Why is necessary to let the user create/remove files/dirs at all?

Good point - file/dir creation/removal should only be done by
relay_open/close.  I'll make sure to fix both of these things.

[...]

 > 
 > For the first version I would suggest to use just local_irq_save/_restore.
 > Getting it right with local_add_return is not trivial and I'm pretty sure 
 > your relay_switch_buffer() gets it wrong, e.g. the caller for whom (offset 
 > < bufsize) must close the subbuffer. Also buffer->data in relay_reserve 
 > may have become invalid (e.g. by an interrupt just before it).
 > 

Yes, there are a couple of bugs here, but I think they're easily fixed
- thanks for spotting them.

For the buffer->data problem, the value of buffer->data at the time
offset is reserved would have to be saved along with it for it to make
sense.

For relay_switch_buffer(), only the offset of the first event that
didn't fit would be less than bufsize so if there was a check for that
i.e. if offset < bufsize, calculate padding, otherwise don't and just
continue, the interrupt would continue through and actually accomplish
the buffer switch, while the interrupted event would calculate the
padding and then exit immediately because of the (offset + length <
bufsize) recheck.  Both would then loop around again to retry and
succeed.  Well, I'm not sure that explanation is clear, but it seems
like it should work with that small change.

Please let me know if you disagree or see any other problems.

 > You can also move all the rchan_buf members which are not written to in 
 > the event path and which are common to all channels back to rchan.

Yes, I was actually planning on doing that, but hadn't gotten to it
yet.


Thanks,

Tom