Re: [Qemu-devel] [PATCH 2/2 v2] usb-mtp: refactor the flow of usb_mtp_write_data
Peter Maydell writes:
> On Tue, 26 Mar 2019 at 17:58, Bandan Das wrote:
>>
>
...
> Doesn't this mean you're no longer sending the RES_OK result
> for this case ?
>
Ugh, I messed up this version. I will send a v3 and
fix the indentation too. Thanks for noticing!
Bandan
>> }
>> 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
Re: [Qemu-devel] [PATCH 2/2 v2] usb-mtp: refactor the flow of usb_mtp_write_data
On Tue, 26 Mar 2019 at 17:58, Bandan Das 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
> ---
> 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
[Qemu-devel] [PATCH 2/2 v2] usb-mtp: refactor the flow of usb_mtp_write_data
There's no functional change but the flow is (hopefully)
more consistent for both file and folder object types.
Signed-off-by: Bandan Das
---
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;
}
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;
}
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;
}
}
}
-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;
}
static void usb_mtp_write_metadata(MTPState *s, uint64_t dlen)
@@ -1725,16 +1729,11 @@ static void usb_mtp_write_metadata(MTPState *s,
uint64_t dlen)
s->write_pending = true;
if (s->dataset.format == FMT_ASSOCIATION) {
-if (usb_mtp_write_data(s)) {
-/* next_handle will be allocated to the newly created dir */
-usb_mtp_queue_result(s, RES_STORE_FULL, d->trans,
- 0, 0, 0, 0);
-return;
-}
+usb_mtp_write_data(s, next_handle);
+} else {
+usb_mtp_queue_result(s, RES_OK, d->trans, 3, QEMU_STORAGE_ID,
+ s->dataset.parent_handle, next_handle);
}
-
-usb_mtp_queue_result(s, RES_OK, d->trans, 3, QEMU_STORAGE_ID,
- s->dataset.parent_handle, next_handle);
}
static void usb_mtp_get_data(MTPState *s, mtp_container *container,
@@ -1814,14 +1813,14 @@ static void usb_mtp_get_data(MTPState *s, mtp_container
*container,
} else {
d->write_status = WRITE_START;
}
-usb_mtp_write_data(s);
+usb_mtp_write_data(s, 0);
usb_mtp_data_free(s->data_out);
s->data_out = NULL;
return;
}
if (d->data_offset == d->length) {
