Re: Compiling C++ modules

[Avi Kivity <avi@argo.co.il>]

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.