Re: Compiling C++ modules

[Avi Kivity]

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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/