| Message ID | b98f675750ef0535cab41225240db1657fc2fe00.1425678142.git.mjt@msgid.tls.msk.ru |
|---|---|
| State | New |
| Headers | show |
On 03/06/2015 02:43 PM, Michael Tokarev wrote: > All filesystem methods that call common v9fs_request() function > also convert return value to errno. Move this conversion to the > common function and remove redundand error handling in methods. s/redundand/redundant/
07.03.2015 23:37, Eric Blake wrote: > On 03/06/2015 02:43 PM, Michael Tokarev wrote: >> All filesystem methods that call common v9fs_request() function >> also convert return value to errno. Move this conversion to the >> common function and remove redundand error handling in methods. > > s/redundand/redundant/ Heh. Is this all that I can say about this patch? ;) Actually, after reading almost whole 9pfs and fsdev code, I can say with great confidence this code is nearly hopeless. Patch 3 shows just one (huge) example. There are so many issues with this code, I'm afraid I don't have know the words to express it. Again, patch 3 shows a good example. Another example: 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. Oh well. I've no idea how this code has been accepted. It is absolute crap. /mjt
08.03.2015 01:10, Michael Tokarev wrote:
[]
> Heh. Is this all that I can say about this patch? ;)
s/I/you/ ofcourse ;)
/mjt
Michael Tokarev <mjt@tls.msk.ru> writes: > All filesystem methods that call common v9fs_request() function > also convert return value to errno. Move this conversion to the > common function and remove redundand error handling in methods. > > I didn't remove local `retval' variable in simple functions to > keep the code consistent. > > Also, proxy_truncate() seem to prefer zero successful return > instead of returning whatever the helper returned, maybe this > should be changed. > > This also removes (harmless) double call to v9fs_string_free() > in proxy_mkdir(), and renames local variables in some functions > for consistency. Can you keep the variable rename as a separate patch. That will make it easier to review. Let me know if you want me to do that for you. -aneesh
Michael Tokarev <mjt@tls.msk.ru> writes: > 07.03.2015 23:37, Eric Blake wrote: >> On 03/06/2015 02:43 PM, Michael Tokarev wrote: >>> All filesystem methods that call common v9fs_request() function >>> also convert return value to errno. Move this conversion to the >>> common function and remove redundand error handling in methods. >> >> s/redundand/redundant/ > > Heh. Is this all that I can say about this patch? ;) > > 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 that the error handling can definitely get some cleanup. I mentioned that in my previous mail mail. http://mid.gmane.org/87oav7iy5v.fsf@linux.vnet.ibm.com > Patch 3 shows just one (huge) example. There are so many issues > with this code, I'm afraid I don't have know the words to express > it. > > Again, patch 3 shows a good example. Another example: > > 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. > > Oh well. I've no idea how this code has been accepted. > It is absolute crap. > -aneesh
08.03.2015 19:27, Aneesh Kumar K.V wrote: > Michael Tokarev <mjt@tls.msk.ru> 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. > that the error handling can definitely get some cleanup. I mentioned > that in my previous mail > mail. http://mid.gmane.org/87oav7iy5v.fsf@linux.vnet.ibm.com I've shown probs in the code itself, not the visible behavour. Visible behavour is much easier to fix here. Thanks, /mjt >> Patch 3 shows just one (huge) example. There are so many issues >> with this code, I'm afraid I don't have know the words to express >> it. >> >> Again, patch 3 shows a good example. Another example: >> >> 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. >> >> Oh well. I've no idea how this code has been accepted. >> It is absolute crap. >> > > -aneesh >
Michael Tokarev <mjt@tls.msk.ru> writes: > 08.03.2015 19:27, Aneesh Kumar K.V wrote: >> Michael Tokarev <mjt@tls.msk.ru> 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. -aneesh
10.03.2015 20:41, Aneesh Kumar K.V пишет: > Michael Tokarev <mjt@tls.msk.ru> writes: > >> 08.03.2015 19:27, Aneesh Kumar K.V wrote: >>> Michael Tokarev <mjt@tls.msk.ru> 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
Michael Tokarev <mjt@tls.msk.ru> writes: > 10.03.2015 20:41, Aneesh Kumar K.V пишет: >> Michael Tokarev <mjt@tls.msk.ru> writes: >> >>> 08.03.2015 19:27, Aneesh Kumar K.V wrote: >>>> Michael Tokarev <mjt@tls.msk.ru> 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
11.03.2015 11:46, Aneesh Kumar K.V wrote: [] > ... 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. The specific issues were outlined and partially fixed by the v3 patchset. This is, for example, the way v9fs_request() is called with arguments for v9fs_marshal function, which are ignored and new arguments are constructed inside _request. That's what I refer to when saying it is a complete crap. Do you not agree? :) [] > 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...) I sent a v3 (last) with correct version number, it has just 3 (rather big) patches: http://thread.gmane.org/gmane.comp.emulators.qemu/324157 . Thanks, /mjt
11.03.2015 23:05, Michael Tokarev wrote: > I sent a v3 (last) with correct version number, it has just 3 (rather > big) patches: http://thread.gmane.org/gmane.comp.emulators.qemu/324157 . And this is what I think should be done (part of that thread), at least for the proxy code: http://article.gmane.org/gmane.comp.emulators.qemu/324585 Thanks, /mjt
diff --git a/hw/9pfs/virtio-9p-proxy.c b/hw/9pfs/virtio-9p-proxy.c index 0904130..13064b6 100644 --- a/hw/9pfs/virtio-9p-proxy.c +++ b/hw/9pfs/virtio-9p-proxy.c @@ -291,7 +291,7 @@ static int v9fs_receive_status(V9fsProxy *proxy, /* * Proxy->header and proxy->request written to socket by QEMU process. * This request read by proxy helper process - * returns 0 on success and -errno on error + * returns 0 on success and -1 (setting errno) on error */ static int v9fs_request(V9fsProxy *proxy, int type, void *response, const char *fmt, ...) @@ -299,7 +299,7 @@ static int v9fs_request(V9fsProxy *proxy, int type, dev_t rdev; va_list ap; int size = 0; - int retval = 0; + int retval, err; uint64_t offset; ProxyHeader header = { 0, 0}; struct timespec spec[2]; @@ -310,10 +310,11 @@ static int v9fs_request(V9fsProxy *proxy, int type, qemu_mutex_lock(&proxy->mutex); - if (proxy->sockfd == -1) { + if (proxy->sockfd < 0) { retval = -EIO; - goto err_out; + goto out; } + iovec = &proxy->out_iovec; reply = &proxy->in_iovec; va_start(ap, fmt); @@ -529,15 +530,15 @@ static int v9fs_request(V9fsProxy *proxy, int type, va_end(ap); if (retval < 0) { - goto err_out; + goto out; } /* marshal the header details */ proxy_marshal(iovec, 0, "dd", header.type, header.size); header.size += PROXY_HDR_SZ; - retval = qemu_write_full(proxy->sockfd, iovec->iov_base, header.size); - if (retval != header.size) { + err = qemu_write_full(proxy->sockfd, iovec->iov_base, header.size); + if (err != header.size) { goto close_error; } @@ -548,9 +549,7 @@ static int v9fs_request(V9fsProxy *proxy, int type, * A file descriptor is returned as response for * T_OPEN,T_CREATE on success */ - if (v9fs_receivefd(proxy->sockfd, &retval) < 0) { - goto close_error; - } + err = v9fs_receivefd(proxy->sockfd, &retval); break; case T_MKNOD: case T_MKDIR: @@ -564,51 +563,44 @@ static int v9fs_request(V9fsProxy *proxy, int type, case T_REMOVE: case T_LSETXATTR: case T_LREMOVEXATTR: - if (v9fs_receive_status(proxy, reply, &retval) < 0) { - goto close_error; - } + err = v9fs_receive_status(proxy, reply, &retval); break; case T_LSTAT: case T_READLINK: case T_STATFS: case T_GETVERSION: - if (v9fs_receive_response(proxy, type, &retval, response) < 0) { - goto close_error; - } + err = v9fs_receive_response(proxy, type, &retval, response); break; case T_LGETXATTR: case T_LLISTXATTR: if (!size) { - if (v9fs_receive_status(proxy, reply, &retval) < 0) { - goto close_error; - } + err = v9fs_receive_status(proxy, reply, &retval); } else { - if (v9fs_receive_response(proxy, type, &retval, response) < 0) { - goto close_error; - } + err = v9fs_receive_response(proxy, type, &retval, response); } break; } -err_out: - qemu_mutex_unlock(&proxy->mutex); - return retval; - + if (err < 0) { close_error: - close(proxy->sockfd); - proxy->sockfd = -1; + close(proxy->sockfd); + proxy->sockfd = -1; + retval = -EIO; + } + +out: + if (retval < 0) { + errno = -retval; + retval = -1; + } qemu_mutex_unlock(&proxy->mutex); - return -EIO; + return retval; } static int proxy_lstat(FsContext *fs_ctx, V9fsPath *fs_path, struct stat *stbuf) { int retval; retval = v9fs_request(fs_ctx->private, T_LSTAT, stbuf, "s", fs_path); - if (retval < 0) { - errno = -retval; - return -1; - } return retval; } @@ -619,7 +611,6 @@ static ssize_t proxy_readlink(FsContext *fs_ctx, V9fsPath *fs_path, retval = v9fs_request(fs_ctx->private, T_READLINK, buf, "sd", fs_path, bufsz); if (retval < 0) { - errno = -retval; return -1; } return strlen(buf); @@ -639,10 +630,6 @@ static int proxy_open(FsContext *ctx, V9fsPath *fs_path, int flags, V9fsFidOpenState *fs) { fs->fd = v9fs_request(ctx->private, T_OPEN, NULL, "sd", fs_path, flags); - if (fs->fd < 0) { - errno = -fs->fd; - fs->fd = -1; - } return fs->fd; } @@ -654,7 +641,6 @@ static int proxy_opendir(FsContext *ctx, fs->dir = NULL; fd = v9fs_request(ctx->private, T_OPEN, NULL, "sd", fs_path, O_DIRECTORY); if (fd < 0) { - errno = -fd; return -1; } fs->dir = fdopendir(fd); @@ -738,9 +724,6 @@ static int proxy_chmod(FsContext *fs_ctx, V9fsPath *fs_path, FsCred *credp) int retval; retval = v9fs_request(fs_ctx->private, T_CHMOD, NULL, "sd", fs_path, credp->fc_mode); - if (retval < 0) { - errno = -retval; - } return retval; } @@ -757,10 +740,6 @@ static int proxy_mknod(FsContext *fs_ctx, V9fsPath *dir_path, &fullname, credp->fc_mode, credp->fc_rdev, credp->fc_uid, credp->fc_gid); v9fs_string_free(&fullname); - if (retval < 0) { - errno = -retval; - retval = -1; - } return retval; } @@ -776,11 +755,6 @@ static int proxy_mkdir(FsContext *fs_ctx, V9fsPath *dir_path, retval = v9fs_request(fs_ctx->private, T_MKDIR, NULL, "sddd", &fullname, credp->fc_mode, credp->fc_uid, credp->fc_gid); v9fs_string_free(&fullname); - if (retval < 0) { - errno = -retval; - retval = -1; - } - v9fs_string_free(&fullname); return retval; } @@ -809,10 +783,6 @@ static int proxy_open2(FsContext *fs_ctx, V9fsPath *dir_path, const char *name, &fullname, flags, credp->fc_mode, credp->fc_uid, credp->fc_gid); v9fs_string_free(&fullname); - if (fs->fd < 0) { - errno = -fs->fd; - fs->fd = -1; - } return fs->fd; } @@ -832,10 +802,6 @@ static int proxy_symlink(FsContext *fs_ctx, const char *oldpath, &target, &fullname, credp->fc_uid, credp->fc_gid); v9fs_string_free(&fullname); v9fs_string_free(&target); - if (retval < 0) { - errno = -retval; - retval = -1; - } return retval; } @@ -850,20 +816,14 @@ static int proxy_link(FsContext *ctx, V9fsPath *oldpath, retval = v9fs_request(ctx->private, T_LINK, NULL, "ss", oldpath, &newpath); v9fs_string_free(&newpath); - if (retval < 0) { - errno = -retval; - retval = -1; - } return retval; } static int proxy_truncate(FsContext *ctx, V9fsPath *fs_path, off_t size) { int retval; - retval = v9fs_request(ctx->private, T_TRUNCATE, NULL, "sq", fs_path, size); if (retval < 0) { - errno = -retval; return -1; } return 0; @@ -884,9 +844,6 @@ static int proxy_rename(FsContext *ctx, const char *oldpath, &oldname, &newname); v9fs_string_free(&oldname); v9fs_string_free(&newname); - if (retval < 0) { - errno = -retval; - } return retval; } @@ -895,9 +852,6 @@ static int proxy_chown(FsContext *fs_ctx, V9fsPath *fs_path, FsCred *credp) int retval; retval = v9fs_request(fs_ctx->private, T_CHOWN, NULL, "sdd", fs_path, credp->fc_uid, credp->fc_gid); - if (retval < 0) { - errno = -retval; - } return retval; } @@ -909,9 +863,6 @@ static int proxy_utimensat(FsContext *s, V9fsPath *fs_path, fs_path, buf[0].tv_sec, buf[0].tv_nsec, buf[1].tv_sec, buf[1].tv_nsec); - if (retval < 0) { - errno = -retval; - } return retval; } @@ -923,9 +874,6 @@ static int proxy_remove(FsContext *ctx, const char *path) v9fs_string_sprintf(&name, "%s", path); retval = v9fs_request(ctx->private, T_REMOVE, NULL, "s", &name); v9fs_string_free(&name); - if (retval < 0) { - errno = -retval; - } return retval; } @@ -951,10 +899,6 @@ static int proxy_statfs(FsContext *s, V9fsPath *fs_path, struct statfs *stbuf) { int retval; retval = v9fs_request(s->private, T_STATFS, stbuf, "s", fs_path); - if (retval < 0) { - errno = -retval; - return -1; - } return retval; } @@ -969,9 +913,6 @@ static ssize_t proxy_lgetxattr(FsContext *ctx, V9fsPath *fs_path, retval = v9fs_request(ctx->private, T_LGETXATTR, value, "dss", size, fs_path, &xname); v9fs_string_free(&xname); - if (retval < 0) { - errno = -retval; - } return retval; } @@ -980,10 +921,7 @@ static ssize_t proxy_llistxattr(FsContext *ctx, V9fsPath *fs_path, { int retval; retval = v9fs_request(ctx->private, T_LLISTXATTR, value, "ds", size, - fs_path); - if (retval < 0) { - errno = -retval; - } + fs_path); return retval; } @@ -1005,9 +943,6 @@ static int proxy_lsetxattr(FsContext *ctx, V9fsPath *fs_path, const char *name, fs_path, &xname, &xvalue, size, flags); v9fs_string_free(&xname); v9fs_string_free(&xvalue); - if (retval < 0) { - errno = -retval; - } return retval; } @@ -1022,9 +957,6 @@ static int proxy_lremovexattr(FsContext *ctx, V9fsPath *fs_path, retval = v9fs_request(ctx->private, T_LREMOVEXATTR, NULL, "ss", fs_path, &xname); v9fs_string_free(&xname); - if (retval < 0) { - errno = -retval; - } return retval; } @@ -1046,7 +978,7 @@ static int proxy_renameat(FsContext *ctx, V9fsPath *olddir, const char *old_name, V9fsPath *newdir, const char *new_name) { - int ret; + int retval; V9fsString old_full_name, new_full_name; v9fs_string_init(&old_full_name); @@ -1055,30 +987,30 @@ static int proxy_renameat(FsContext *ctx, V9fsPath *olddir, v9fs_string_sprintf(&old_full_name, "%s/%s", olddir->data, old_name); v9fs_string_sprintf(&new_full_name, "%s/%s", newdir->data, new_name); - ret = proxy_rename(ctx, old_full_name.data, new_full_name.data); + retval = proxy_rename(ctx, old_full_name.data, new_full_name.data); v9fs_string_free(&old_full_name); v9fs_string_free(&new_full_name); - return ret; + return retval; } static int proxy_unlinkat(FsContext *ctx, V9fsPath *dir, const char *name, int flags) { - int ret; + int retval; V9fsString fullname; v9fs_string_init(&fullname); v9fs_string_sprintf(&fullname, "%s/%s", dir->data, name); - ret = proxy_remove(ctx, fullname.data); + retval = proxy_remove(ctx, fullname.data); v9fs_string_free(&fullname); - return ret; + return retval; } static int proxy_ioc_getversion(FsContext *fs_ctx, V9fsPath *path, mode_t st_mode, uint64_t *st_gen) { - int err; + int retval; /* Do not try to open special files like device nodes, fifos etc * we can get fd for regular files and directories only @@ -1087,12 +1019,8 @@ static int proxy_ioc_getversion(FsContext *fs_ctx, V9fsPath *path, errno = ENOTTY; return -1; } - err = v9fs_request(fs_ctx->private, T_GETVERSION, st_gen, "s", path); - if (err < 0) { - errno = -err; - err = -1; - } - return err; + retval = v9fs_request(fs_ctx->private, T_GETVERSION, st_gen, "s", path); + return retval; } static int connect_namedsocket(const char *path)
All filesystem methods that call common v9fs_request() function also convert return value to errno. Move this conversion to the common function and remove redundand error handling in methods. I didn't remove local `retval' variable in simple functions to keep the code consistent. Also, proxy_truncate() seem to prefer zero successful return instead of returning whatever the helper returned, maybe this should be changed. This also removes (harmless) double call to v9fs_string_free() in proxy_mkdir(), and renames local variables in some functions for consistency. Signed-off-by: Michael Tokarev <mjt@tls.msk.ru> --- hw/9pfs/virtio-9p-proxy.c | 142 ++++++++++++---------------------------------- 1 file changed, 35 insertions(+), 107 deletions(-)