Re: [Qemu-devel] [PATCH v3 0/3] RFC: 9pfs-proxy: simplify/cleanup

[Michael Tokarev <mjt@tls.msk.ru>]

Now when I reviewed the whole thing, I'd drop the error
handling change too, and fold it into even bigger change.
What I'm thinking is to refactor this code to look
significantly different, namely:

For the proxy code:

 o each filesystem method first call proxy_send_request(type, fmt, ...),
   and proxy_send_request() locks the proxy object, marshals the
   arguments according to fmt and sends the request out.

 o after sending the request, each method calls
   proxy_receive_reply(fmt, ...) (or proxy_receive_fd() -- there's
   no need in proxy_receive_status(), it is proxy_recive_reply()
   with fmt=NULL).  This proxy_receive_reply() receives the
   reply according to the fmt argument and unlocks the
   proxy object.

This way. locking code will be split into two methods, but
_all_ filesystem-method-specific code, for each method,
will be in the same function which is the only place which
"knows" everything about the method.

For proxy code it might also be a good idea to have two
v9fs_string objects embedded in the proxy structure, to
be used by the methods, so there's no need to init and
free the local-to-function strings in every method.

At the general level, v9fs_string handling is bad, it
shouldn't really free+alloc a string every time something
gets printed into it.  v9fs_path type should be dropped
completely and replaced with v9fs_string, especially since
one is often being type-cast to another.

marshal/unmarshal interface should be printf-like with %s,
so that the compiler will be able to check if the arguments
are of the right type.  Alternatively, it should be re-
factored to accept just one, typed, argument to pack/unpack,
without vararg part.  Here, an iovec iterator might be used
(to keep current position in iovec) to speed things up.

marshal/unmarshal interface should not allocate/free strings
frantically like it does now -- this allocation/freeing takes
significant time and slows down 9pfs code.  The same is true
for some other parts of 9pfs too.

Also marshal/unmarshal interface - is it really necessary to
support I/O where individual eiements (a file name, size of
that file name, or even all the fields of some guest-visible
structure) are split between several iovec elements?  Can't
we say that a single element (with some definition of "element",
which can be a single string or this string together with its
size in front, one element of a structure like stat or whole
stat structure, etc) must not be split into multiple iovecs?
If it's possible, this code can be simplified and sped up
greatly, for example, we won't need to copy the strings to
a temp buffer (especially multiple times!) and can just point
inside of a single iovec...

That's just some random thoughts.  All without acually understanding
how 9pfs communicates with the guest...  Can you describe this part
briefly please?  Or maybe I should just shut up and pretend I never
seen this code.. ;)  (which is, actually, a good possibility - I don't
really have much time to deal with it, and 9pfs is not my big interest... ;)

Thanks,

/mjt

07.03.2015 00:43, Michael Tokarev wrote:
> Try to make the code a bit less ugly.  First by moving
> errno = -retval to a common place, and second by simplifying
> common place a lot.  What's left is v9fs_receive_response().
> 
> V3: merge several small patches into larger ones,
>     drop trivial stuff
> 
> Michael Tokarev (3):
>   9pfs-proxy: simplify error handling
>   fsdev: introduce v9fs_vmarshal() and v9fs_vunmarshal()
>   9pfs-proxy: remove one half of redundrand code
> 
>  fsdev/virtio-9p-marshal.c |  38 +++--
>  fsdev/virtio-9p-marshal.h |   6 +
>  hw/9pfs/virtio-9p-proxy.c | 405 +++++-----------------------------------------
>  3 files changed, 77 insertions(+), 372 deletions(-)
>