Re: [Qemu-devel] [PATCH 1/3] 9pfs-proxy: simplify error handling

[Michael Tokarev]

10.03.2015 20:41, Aneesh Kumar K.V пишет:
> Michael Tokarev <address@hidden> writes:
> 
>> 08.03.2015 19:27, Aneesh Kumar K.V wrote:
>>> Michael Tokarev <address@hidden> writes:
>> []
>>>> Actually, after reading almost whole 9pfs and fsdev code, I can
>>>> say with great confidence this code is nearly hopeless.
>>>
>>> Is that about the 9pfs-proxy code, or the rest of 9pfs. I understand
>>
>> Well. the marshal/unmarshal interface is in core code as far as
>> I can see, and it is very fragile at best, as the below example of
>> its usage shows.  I haven't dug deeper.  So far, it was only the
>> 9pfs proxy code.
> 
> May be i am missing something here. Can you help me understand the
> issue. 
> 
>> static int v9fs_receive_status(V9fsProxy *proxy,
>>                                struct iovec *reply, int *status)
>> ...
>>     proxy_unmarshal(reply, 0, "dd", &header.type, &header.size);
>>     if (header.size != sizeof(int)) {
>>         *status = -ENOBUFS;
>>     }
>> ...
>>     proxy_unmarshal(reply, PROXY_HDR_SZ, "d", status);
>>
>> proxy_unmarshall(), for "d" element, expects an int32_t
>> pointer.  Here we have int pointer, and compare its
>> size with sizeof(int).  This is a generic problem of whole
>> v9fs_(un)marshall interface, which is in the core of 9pfs...
>> this is a status return, which is int32.
> 
> Proxy helper do write sizeof(int) as a part of header response. So
> it read the header.size and check whether it is same as what it is
> expecting. If not it error out. So i am not sure what the issue you are
> listing here.

Nope.  Both proxy helper and qemu uses v9fs_marshal/unmarshal interface
which writes/reads a 4-byte uint32, which is the same size no matter
which compiler/architecture you're on.  Not only these functions uses
this size "on wire", they also expect the same type of argument to the
function.

However, as shown in the above example, at least some users of this
interface uses int* instead of int32_t*, and sizeof(int) is compiler-
and architecture-dependent, it is not fixed to 4 bytes (or else we'd
not need a int32_t to start with, and life would be much simpler).

The rule is that if you exchange data with some other process, unless
it is a forked version of your own process, you should use some fixed
interface, which is not dependent on some conditions.

This is not some made-up situation really, even in this context: for
example, in Debian we allow to install packages of different architectures
on the same machine, and they work together.  We'we qemu-system-common
package which contains the proxy helper, and eg qemu-system-x86 or
qemu-system-arm, and it is perfectly valid to have 64bit qemu-system-common
working together with 32bit qemu-system-mips, so that 32bit qemu binary
will talk with 64bit proxy.

Well, you can treat the proxy helper to be internal part of qemu which
can't be intermixed between versions and architectures -- after all,
little-endian-arch qemu can't talk to big-endian proxy since there's
no byte swapping is going on -- it's a good idea to mention this in
the package. ;)

But your v9fs_marshal interface supposed to be generic and should be
used right to start with.  All args of v9fs_(un)marshall should carry
the same types of arguments these functions expect, which are
int32_t not int, int64_t not long or long long or timespec_t or any
other special type and so on.  Or else BadThings will happen, and
often you wont even notice.

(Note: I haven't audited this interface usage in 9pfs-local.)

But the most problematic part here is that you don't see if you made
a mistake and used different argument type, or missed an argument,
to one of these 2 functions.  There's no argument checking in this
interface, whatsoever.  There's a reason why we have __attribute__(format)
gcc extension to check printf arguments.

That's 2 reasons why I proposed to make these marshal/unmarshal
function to deal with one argument at a time, but with an explicitly
typed argument.  It all will be transparent and obvious and compiler
will tell you the mistakes.

Thanks,

/mjt