Re: [Qemu-devel] [PATCH 2/2 v2] usb-mtp: refactor the flow of usb_mtp_write_data

[Peter Maydell]

On Tue, 26 Mar 2019 at 17:58, Bandan Das <address@hidden> wrote:
>
> There's no functional change but the flow is (hopefully)
> more consistent for both file and folder object types.
>
> Signed-off-by: Bandan Das <address@hidden>
> ---
>  hw/usb/dev-mtp.c | 53 ++++++++++++++++++++++++------------------------
>  1 file changed, 26 insertions(+), 27 deletions(-)
>
> diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
> index 4dc1317e2e..57a4d00ad0 100644
> --- a/hw/usb/dev-mtp.c
> +++ b/hw/usb/dev-mtp.c
> @@ -1599,7 +1599,7 @@ static int usb_mtp_update_object(MTPObject *parent, 
> char *name)
>      return ret;
>  }
>
> -static int usb_mtp_write_data(MTPState *s)
> +static void usb_mtp_write_data(MTPState *s, uint32_t handle)
>  {
>      MTPData *d = s->data_out;
>      MTPObject *parent =
> @@ -1616,26 +1616,32 @@ static int usb_mtp_write_data(MTPState *s)
>          if (!parent || !s->write_pending) {
>              usb_mtp_queue_result(s, RES_INVALID_OBJECTINFO, d->trans,
>                  0, 0, 0, 0);
> -        return 1;
> +        return;

The indentation here looks off.

>          }
>
>          if (s->dataset.filename) {
>              path = g_strdup_printf("%s/%s", parent->path, 
> s->dataset.filename);
>              if (s->dataset.format == FMT_ASSOCIATION) {
>                  ret = mkdir(path, mask);
> -                goto free;
> +                if (!ret) {
> +                    usb_mtp_queue_result(s, RES_OK, d->trans, 3,
> +                                         QEMU_STORAGE_ID,
> +                                         s->dataset.parent_handle,
> +                                         handle);
> +                }
> +                goto done;
>              }
> +
>              d->fd = open(path, O_CREAT | O_WRONLY |
>                           O_CLOEXEC | O_NOFOLLOW, mask);
>              if (d->fd == -1) {
> -                usb_mtp_queue_result(s, RES_STORE_FULL, d->trans,
> -                                     0, 0, 0, 0);
> +                ret = 1;
>                  goto done;
>              }
>
>              /* Return success if initiator sent 0 sized data */
>              if (!s->dataset.size) {
> -                goto success;
> +                goto done;

Doesn't this mean you're no longer sending the RES_OK result
for this case ?

>              }
>              if (d->length != MTP_WRITE_BUF_SZ && !d->pending) {
>                  d->write_status = WRITE_END;
> @@ -1647,13 +1653,12 @@ static int usb_mtp_write_data(MTPState *s)
>          rc = write_retry(d->fd, d->data, d->data_offset,
>                           d->offset - d->data_offset);
>          if (rc != d->data_offset) {
> -            usb_mtp_queue_result(s, RES_STORE_FULL, d->trans,
> -                                 0, 0, 0, 0);
> +            ret = 1;
>              goto done;
>          }
>          if (d->write_status != WRITE_END) {
>              g_free(path);
> -            return ret;
> +            return;
>          } else {
>              /*
>               * Return an incomplete transfer if file size doesn't match
> @@ -1665,16 +1670,17 @@ static int usb_mtp_write_data(MTPState *s)
>                  usb_mtp_update_object(parent, s->dataset.filename)) {
>                  usb_mtp_queue_result(s, RES_INCOMPLETE_TRANSFER, d->trans,
>                                       0, 0, 0, 0);
> -                goto done;
> +                goto close;
>              }
>          }

Similarly the code path which succeeds and falls out of the end
of this case will no longer be sending RES_OK, I think.

>      }
>
> -success:
> -    usb_mtp_queue_result(s, RES_OK, d->trans,
> -                         0, 0, 0, 0);
> -
>  done:
> +    if (ret) {
> +        usb_mtp_queue_result(s, RES_STORE_FULL, d->trans,
> +                             0, 0, 0, 0);
> +    }
> +close:
>      /*
>       * The write dataset is kept around and freed only
>       * on success or if another write request comes in
> @@ -1683,12 +1689,10 @@ done:
>          close(d->fd);
>          d->fd = -1;
>      }
> -free:
>      g_free(s->dataset.filename);
>      s->dataset.size = 0;
>      g_free(path);
>      s->write_pending = false;
> -    return ret;
>  }

thanks
-- PMM