[1/5] 9pfs-proxy: simplify v9fs_request(), P1

This simplifies code in v9fs_request() a bit by replacing several
ifs with a common variable check and rearranging error/cleanup
code a bit.

Signet-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 hw/9pfs/virtio-9p-proxy.c | 48 ++++++++++++++++++++---------------------------
 1 file changed, 20 insertions(+), 28 deletions(-)

Michael Tokarev <mjt@tls.msk.ru> writes:

> This simplifies code in v9fs_request() a bit by replacing several
> ifs with a common variable check and rearranging error/cleanup
> code a bit.

Is this -V2 of
http://mid.gmane.org/b98f675750ef0535cab41225240db1657fc2fe00.1425678142.git.mjt@msgid.tls.msk.ru

I am slightly confused with the patch series. It does split the patch as
I wanted, but i am not sure which one is the latest, so that i can start
applying them.

>
> Signet-off-by: Michael Tokarev <mjt@tls.msk.ru>
> ---
>  hw/9pfs/virtio-9p-proxy.c | 48 ++++++++++++++++++++---------------------------
>  1 file changed, 20 insertions(+), 28 deletions(-)
>
> diff --git a/hw/9pfs/virtio-9p-proxy.c b/hw/9pfs/virtio-9p-proxy.c
> index 59c7445..f252fe4 100644
> --- a/hw/9pfs/virtio-9p-proxy.c
> +++ b/hw/9pfs/virtio-9p-proxy.c
> @@ -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 = 0, 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,41 +563,34 @@ 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:
>      qemu_mutex_unlock(&proxy->mutex);
> -    return -EIO;
> +    return retval;
>  }
>  
>  static int proxy_lstat(FsContext *fs_ctx, V9fsPath *fs_path, struct stat *stbuf)
> -- 
> 2.1.4

08.03.2015 19:39, Aneesh Kumar K.V wrote:
> Michael Tokarev <mjt@tls.msk.ru> writes:
> 
>> This simplifies code in v9fs_request() a bit by replacing several
>> ifs with a common variable check and rearranging error/cleanup
>> code a bit.
> 
> Is this -V2 of
> http://mid.gmane.org/b98f675750ef0535cab41225240db1657fc2fe00.1425678142.git.mjt@msgid.tls.msk.ru

No, this one (simplify v9fs_reqeust(), P1) was a first version.
The above URL points to the v2, a second version.

> I am slightly confused with the patch series. It does split the patch as
> I wanted, but i am not sure which one is the latest, so that i can start
> applying them.

That URL points to last.  I merged several small patches into larger ones.

Initially I thought I just plug a small resource leak so the patch will
be small.  But the more I understood the code, the bigger the changes
were becoming.  So at some point it become unreasonable to keep small
changes since they're related to bigger changes and since bigger changes
touches the same code anyway.

Thanks,

/mjt

10.03.2015 07:19, Michael Tokarev wrote:
> 08.03.2015 19:39, Aneesh Kumar K.V wrote:
>> Michael Tokarev <mjt@tls.msk.ru> writes:
>>
>>> This simplifies code in v9fs_request() a bit by replacing several
>>> ifs with a common variable check and rearranging error/cleanup
>>> code a bit.
>>
>> Is this -V2 of
>> http://mid.gmane.org/b98f675750ef0535cab41225240db1657fc2fe00.1425678142.git.mjt@msgid.tls.msk.ru
> 
> No, this one (simplify v9fs_reqeust(), P1) was a first version.
> The above URL points to the v2, a second version.

Actually the url points to v3, not v2, of the patchset.  Yet it
is still the latest ;)

Thanks,

/mjt

Michael Tokarev <mjt@tls.msk.ru> writes:

> 08.03.2015 19:39, Aneesh Kumar K.V wrote:
>> Michael Tokarev <mjt@tls.msk.ru> writes:
>> 
>>> This simplifies code in v9fs_request() a bit by replacing several
>>> ifs with a common variable check and rearranging error/cleanup
>>> code a bit.
>> 
>> Is this -V2 of
>> http://mid.gmane.org/b98f675750ef0535cab41225240db1657fc2fe00.1425678142.git.mjt@msgid.tls.msk.ru
>
> No, this one (simplify v9fs_reqeust(), P1) was a first version.
> The above URL points to the v2, a second version.

Please post an update patch series with the variable rename and the v9fs
simplification as two different patches.


>
>> I am slightly confused with the patch series. It does split the patch as
>> I wanted, but i am not sure which one is the latest, so that i can start
>> applying them.
>
> That URL points to last.  I merged several small patches into larger ones.
>
> Initially I thought I just plug a small resource leak so the patch will
> be small.  But the more I understood the code, the bigger the changes
> were becoming.  So at some point it become unreasonable to keep small
> changes since they're related to bigger changes and since bigger changes
> touches the same code anyway.
>

Please keep the patches smaller. If needed i can fold them up when
merging the changes.

-aneesh