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(-) >
