Peter Maydell <peter.mayd...@linaro.org> writes:
> On Fri, 8 Mar 2019 at 19:39, Bandan Das <b...@redhat.com> wrote:
>>
>> Peter Maydell <peter.mayd...@linaro.org> writes:
>> > But the two places in usb_mtp_get_data() that call
>> > usb_mtp_write_metadata() still don't check its return
>> > value: don't they need to handle failure too?
>> >
>> I believe this is ok because:
>> The return value of usb_mtp_write_data is only used to check if mkdir
>> failed and update s->result in usb_mtp_write_metadata().
>> The next time usb_mtp_handle_data is called, it will process s->result.
>
> I think I still don't really understand the error handling
> in this function. Why do we deal with mkdir() failing by
> having the function return -1 and then doing
> usb_mtp_queue_result(s, RES_STORE_FULL, ...)
> (but only at one callsite), whereas for open() or write()
> failing we do the usb_mtp_queue_result(s, RES_STORE_FULL, ...)
> inside the function itself?
>
usb_mtp_write_metadata() handles the sendobjectinfo phase where the
initiator sends the metadata associated with the incoming object.
For a file, the name and the size is sent and once the responder sends
back OK, the initiator starts the sendobject phase. For a folder,
the name of the folder is sent with size being 0, and
no sendobject phase follows.
So, the reason I am sending back the return
value is because for a folder, I want to send a success or a failure
based on whether mkdir succeeded but for a file object, I want to return
success so that the next phase can continue. Is this rewrite better ?
static void usb_mtp_object_delete(MTPState *s, uint32_t handle,
@@ -1674,7 +1666,13 @@ static void usb_mtp_write_data(MTPState *s)
if (s->dataset.filename) {
path = g_strdup_printf("%s/%s", parent->path, s->dataset.filename);
if (s->dataset.format == FMT_ASSOCIATION) {
- d->fd = mkdir(path, mask);
+ if (mkdir(path, mask)) {
+ usb_mtp_queue_result(s, RES_STORE_FULL, d->trans,
+ 0, 0, 0, 0);
+ } else {
+ usb_mtp_queue_result(s, RES_STORE_FULL, d->trans,
+ 0, 0, 0, 0);
+ }
goto free;
}
d->fd = open(path, O_CREAT | O_WRONLY | O_CLOEXEC | O_NOFOLLOW,
mask);
@@ -1769,17 +1767,10 @@ static void usb_mtp_write_metadata(MTPState *s,
uint64_t dlen)
if (s->dataset.format == FMT_ASSOCIATION) {
usb_mtp_write_data(s);
- /* next_handle will be allocated to the newly created dir */
- if (d->fd == -1) {
- usb_mtp_queue_result(s, RES_STORE_FULL, d->trans,
- 0, 0, 0, 0);
- return;
- }
- d->fd = -1;
+ } 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,
> thanks
> -- PMM
