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

[Aneesh Kumar K.V]

Michael Tokarev <address@hidden> writes:

> 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.

That is better. So what you are looking at is only the proxy marshal and
unmarshal code. I was wondering what issues you were pointing out by

   " 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."

>
> 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.

The code was originally written to be used on same os/cpu combination to
help virtfs usage via libvirt. AFAIU here we don't have issues
across 32 and 64 bit, because int remains 32 bit in both the
place. Which os/cpu combination are we looking w.r.t ILP64 ?. Please
note that the code is written primary on x86_64. So some assumptions
could be wrong. Rather than saying

   "Oh well.  I've no idea how this code has been accepted.
   It is absolute crap."

it would be nice if we take up specific issues, w.r.t the os/cpu
combination where we are running into issues, we may be able to fix this
easily.


>
> 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. ;)

That is correct. It was not written to be used in such scenarios

>
> 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: At this point i haven't picked any of the patches posted, so if
you have anything specific you want me to take up, please repost then
with correct version number (v1, v2...)

-aneesh