Re: [Virtio-fs] [PATCH v3 1/2] virtiofsd: add definition of fuse_buf_writev()

[piaojun <piaojun@huawei.com>]

Hi Stefan,

On 2019/8/9 16:14, Stefan Hajnoczi wrote:
> On Thu, Aug 08, 2019 at 10:28:26PM +0800, piaojun wrote:
>> On 2019/8/8 17:38, Stefan Hajnoczi wrote:
>>> On Thu, Aug 08, 2019 at 02:15:27PM +0800, piaojun wrote:
>>>> +{
>>>> +	ssize_t res, i, buf_index, iovcnt;
>>>> +	struct iovec * iov;
>>>> +	int fd = out_buf->buf[0].fd;
>>>> +	off_t pos = out_buf->buf[0].pos;
>>>
>>> A struct fuse_bufvec may have multiple elements but this function
>>> assumes it only has 1.  Please use struct fuse_buf *out_buf instead.
>>> This way it's clear that only 1 fuse_buf will be written.
>>
>> Do you mean that assign *out_buf with &out_buf->buf[0] in
>> fuse_buf_writev() and then operate out_buf in the following process?
> 
> Since this function assumes that out_buf is only a single struct
> fuse_buf, please change the argument to struct fuse_buf instead of
> struct fuse_bufvec.  This way it's clear what the function expects.

Agreed, already fixed in PATCH v4.

> 
>>>
>>>> +
>>>> +	if (in_buf->count > 2)
>>>> +		iovcnt = in_buf->count - 1;
>>>> +	else
>>>> +		iovcnt = 1;
>>>> +
>>>> +	iov = calloc(iovcnt, sizeof(struct iovec));
>>>> +	if (!iov)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	for (i = 0, buf_index = 1; i < iovcnt; i++, buf_index++) {
>>>> +		iov[i].iov_base = in_buf->buf[buf_index].mem;
>>>> +		iov[i].iov_len = in_buf->buf[buf_index].size;
>>>> +	}
>>>
>>> Why is in_buf->buf[0] is skipped?
>>
>> This part of code seems a little complex and in_buf->buf[0].count just
>> tricks me. Maybe I need handle two situations:
>> 1. Iterate from in_buf->buf[0].mem when in_buf->buf[0].count is 1;
>> 2. buf[0].size is set 0 in do_write_buf() when in_buf->buf[0].count > 1,
>> and it's better to skip it. Perhaps it's also ok to iterate from buf[0],
>> as writev skip the iov with zero len. This looks like a hack.
> 
> I would perform writev() with all of in_buf's buffers, even if some of
> them have 0 length.  That way this function does not make any
> assumptions about in_buf's layout.

Agreed, already fixed in PATCH v4.

Jun

> 
> The only requirement is that in_buf buffers are all memory buffers (not
> file descriptors).
>