Messages in this thread | | | Date | Tue, 02 May 2006 17:54:41 +0300 | From | Avi Kivity <> | Subject | Re: Compiling C++ modules |
| |
Al Viro wrote: > On Tue, May 02, 2006 at 04:52:53PM +0300, Avi Kivity wrote: > >> static ssize_t do_sendfile(int out_fd, int in_fd, loff_t *ppos, >> size_t count, loff_t max) >> { >> loff_t pos; >> ssize_t retval; >> >> /* >> * Get input file, and verify that it is ok.. >> */ >> light_file_ptr in_file(in_fd); >> > > *snerk* > Good luck defining copying and conversion to file * for that puppy. > >
class light_file_ptr { public: explicit light_file_ptr(int fd) { _file = fget_light(fd, &_fput_needed); } ~light_file_ptr() { fput_light(_file, _fput_needed); } bool valid() const { return _file != 0; } struct file *operator->() // allowed for libs :) { return _file; } private: struct file *_file; int _fput_needed; };
>> struct inode *in_inode = in_file->dentry()->inode(); >> > > Lovely. Let's expose all fields as methods? > >
If you like. I won't insist.
>> if (!in_inode) >> return -EINVAL; >> > > BTW, that can't happen. Applies to the original as well. > >
I believe this bug is more visible when there is less code in the function.
>> // I'm assuming here that the default sendfile() returns -EINVAL >> if (!ppos) >> ppos = &in_file->f_pos; >> else >> if (!(in_file->mode() & FMODE_PREAD)) >> return -ESPIPE; >> > > As opposed to ->readable() for checking FMODE_READ? > >
Forgot, sorry. I'll redo the patch.
>> light_file_ptr out_file(out_fd); >> if (!out_file) >> return -EBADF; >> > > ? >
Sorry, !out_file.valid().
>> if (!max) >> max = min(in_inode->i_sb->s_maxbytes, >> out_inode->i_sb->s_maxbytes); >> > > While we are at it, that's the only place where in_inode and out_inode > are used. Also... how does one remember which of ->dentry, ->inode > and ->i_sb are methods and which are public fields? >
I usually make all publics either methods (a class) or fields (a struct).
>> // now, with exceptions >> static ssize_t do_sendfile(int out_fd, int in_fd, loff_t *ppos, >> size_t count, loff_t max) >> { >> loff_t pos; >> >> /* >> * Get input file, and verify that it is ok.. >> */ >> light_file_ptr in_file(in_fd); >> in_file->verify_readable(); >> > > That assumes that error value returned in that case is the same everywhere. > It isn't. >
Okay, in_file->verify_readable(EBADF); Yuck.
Thanks for the review.
-- Do not meddle in the internals of kernels, for they are subtle and quick to panic.
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |