Re: iter_file_splice_write() return value (was: Re: ->splice_write() via ->write_iter())

[Geert Uytterhoeven]

Ping?

fs/splice.c:954: warning: âretâ may be used uninitialized in this function

The splice syscall explicitly checks for len != 0, but this code is also
called from do_sendfile(), where it's difficult to see if len is ever 0.

On Mon, Jun 16, 2014 at 9:11 AM, Geert Uytterhoeven
<geert@xxxxxxxxxxxxxx> wrote:
> Hi Al,
>
> On Thu, Jun 12, 2014 at 8:14 PM, Linux Kernel Mailing List
> <linux-kernel@xxxxxxxxxxxxxxx> wrote:
>> Gitweb:     http://git.kernel.org/linus/;a=commit;h=8d0207652cbe27d1f962050737848e5ad4671958
>> Commit:     8d0207652cbe27d1f962050737848e5ad4671958
>
>>     ->splice_write() via ->write_iter()
>>
>>     iter_file_splice_write() - a ->splice_write() instance that gathers the
>>     pipe buffers, builds a bio_vec-based iov_iter covering those and feeds
>>     it to ->write_iter().  A bunch of simple cases coverted to that...
>>
>>     [AV: fixed the braino spotted by Cyrill]
>>
>>     Signed-off-by: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
>
>> @@ -1052,6 +1053,145 @@ generic_file_splice_write(struct pipe_inode_info *pipe, struct file *out,
>>
>>  EXPORT_SYMBOL(generic_file_splice_write);
>>
>> +/**
>> + * iter_file_splice_write - splice data from a pipe to a file
>> + * @pipe:      pipe info
>> + * @out:       file to write to
>> + * @ppos:      position in @out
>> + * @len:       number of bytes to splice
>> + * @flags:     splice modifier flags
>> + *
>> + * Description:
>> + *    Will either move or copy pages (determined by @flags options) from
>> + *    the given pipe inode to the given file.
>> + *    This one is ->write_iter-based.
>
> What's the meaning of the return value? This is not documented.
>
>> +ssize_t
>> +iter_file_splice_write(struct pipe_inode_info *pipe, struct file *out,
>> +                         loff_t *ppos, size_t len, unsigned int flags)
>> +{
>> +       struct splice_desc sd = {
>> +               .total_len = len,
>> +               .flags = flags,
>> +               .pos = *ppos,
>> +               .u.file = out,
>> +       };
>> +       int nbufs = pipe->buffers;
>> +       struct bio_vec *array = kcalloc(nbufs, sizeof(struct bio_vec),
>> +                                       GFP_KERNEL);
>> +       ssize_t ret;
>
> fs/splice.c:954: warning: âretâ may be used uninitialized in this function
>
>> +
>> +       if (unlikely(!array))
>> +               return -ENOMEM;
>> +
>> +       pipe_lock(pipe);
>> +
>> +       splice_from_pipe_begin(&sd);
>> +       while (sd.total_len) {
>
> If the "len" parameter is zero, this loop will be skipped, and "ret" will
> be uninitialized.
>
> What should it return? 0? -EINVAL?
>
>> +       }
>> +done:
>> +       kfree(array);
>> +       splice_from_pipe_end(pipe, &sd);
>> +
>> +       pipe_unlock(pipe);
>> +
>> +       if (sd.num_spliced)
>> +               ret = sd.num_spliced;
>> +
>> +       return ret;
>> +}

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
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/