| Message ID | jpgsgvsc5ls.fsf@linux.bootlegged.copy (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] usb-mtp: fix return status of delete | expand |
On Tue, 12 Mar 2019 at 18:25, Bandan Das <bsd@redhat.com> wrote: > > > Spotted by Coverity: CID 1399414 > > mtp delete allows the return status of delete succeeded, > partial_delete or readonly - when none of the objects could be > deleted. Give more meaningful names to return values of the > delete function. > > Some initiators recurse over the objects themselves. In that case, > only READ_ONLY can be returned. > > Signed-off-by: Bandan Das <bsd@redhat.com> > --- > v2: > Change the enum variable names and specify them as bits > Add a comment describing the bit definitions > Modify commit message slightly > > hw/usb/dev-mtp.c | 62 ++++++++++++++++++++++++++---------------------- > 1 file changed, 34 insertions(+), 28 deletions(-) > > diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c > index 06e376bcd2..2b6fe77a6b 100644 > --- a/hw/usb/dev-mtp.c > +++ b/hw/usb/dev-mtp.c > @@ -1135,11 +1135,19 @@ static MTPData *usb_mtp_get_object_prop_value(MTPState *s, MTPControl *c, > return d; > } > > -/* Return correct return code for a delete event */ > +/* > + * Return values when object @o is deleted. > + * If at least one of the deletions succeeded, > + * DELETE_SUCCESS is set and if at least one > + * of the deletions failed, DELETE_FAILURE is > + * set. Both bits being set (DELETE_PARTIAL) > + * signifies a RES_PARITAL_DELETE being sent "PARTIAL" > + * back to the initiator. > + */ > + switch (ret) { > + case DELETE_SUCCESS: > usb_mtp_queue_result(s, RES_OK, trans, > 0, 0, 0, 0); > - return; > + break; > + case DELETE_FAILURE: > + usb_mtp_queue_result(s, RES_STORE_READ_ONLY, > + trans, 0, 0, 0, 0); > + break; > + case DELETE_PARTIAL: > + usb_mtp_queue_result(s, RES_PARTIAL_DELETE, > + trans, 0, 0, 0, 0); > + break; > + default: > + assert(ret != 0); You could just write the default case as: default: g_assert_not_reached(); I think ? > } Otherwise Reviewed-by: Peter Maydell <peter.maydell@linaro.org> thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On Tue, 12 Mar 2019 at 18:25, Bandan Das <bsd@redhat.com> wrote: >> >> >> Spotted by Coverity: CID 1399414 >> >> mtp delete allows the return status of delete succeeded, >> partial_delete or readonly - when none of the objects could be >> deleted. Give more meaningful names to return values of the >> delete function. >> >> Some initiators recurse over the objects themselves. In that case, >> only READ_ONLY can be returned. >> >> Signed-off-by: Bandan Das <bsd@redhat.com> >> --- >> v2: >> Change the enum variable names and specify them as bits >> Add a comment describing the bit definitions >> Modify commit message slightly >> >> hw/usb/dev-mtp.c | 62 ++++++++++++++++++++++++++---------------------- >> 1 file changed, 34 insertions(+), 28 deletions(-) >> >> diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c >> index 06e376bcd2..2b6fe77a6b 100644 >> --- a/hw/usb/dev-mtp.c >> +++ b/hw/usb/dev-mtp.c >> @@ -1135,11 +1135,19 @@ static MTPData *usb_mtp_get_object_prop_value(MTPState *s, MTPControl *c, >> return d; >> } >> >> -/* Return correct return code for a delete event */ >> +/* >> + * Return values when object @o is deleted. >> + * If at least one of the deletions succeeded, >> + * DELETE_SUCCESS is set and if at least one >> + * of the deletions failed, DELETE_FAILURE is >> + * set. Both bits being set (DELETE_PARTIAL) >> + * signifies a RES_PARITAL_DELETE being sent > > "PARTIAL" > >> + * back to the initiator. >> + */ > >> + switch (ret) { >> + case DELETE_SUCCESS: >> usb_mtp_queue_result(s, RES_OK, trans, >> 0, 0, 0, 0); >> - return; >> + break; >> + case DELETE_FAILURE: >> + usb_mtp_queue_result(s, RES_STORE_READ_ONLY, >> + trans, 0, 0, 0, 0); >> + break; >> + case DELETE_PARTIAL: >> + usb_mtp_queue_result(s, RES_PARTIAL_DELETE, >> + trans, 0, 0, 0, 0); >> + break; >> + default: >> + assert(ret != 0); > > You could just write the default case as: > default: > g_assert_not_reached(); > > I think ? > >> } > Thanks, sent a v3. > Otherwise > Reviewed-by: Peter Maydell <peter.maydell@linaro.org> > Does the reviewed-by tag get picked up by the scripts for a newer version of the patch or I should add it if I send one with minor edits ? Bandan > thanks > -- PMM
Patchew URL: https://patchew.org/QEMU/jpgsgvsc5ls.fsf@linux.bootlegged.copy/ Hi, This series failed the docker-mingw@fedora build test. Please find the testing commands and their output below. If you have Docker installed, you can probably reproduce it locally. === TEST SCRIPT BEGIN === #!/bin/bash time make docker-test-mingw@fedora SHOW_ENV=1 J=14 NETWORK=1 === TEST SCRIPT END === The full log is available at http://patchew.org/logs/jpgsgvsc5ls.fsf@linux.bootlegged.copy/testing.docker-mingw@fedora/?type=message. --- Email generated automatically by Patchew [http://patchew.org/]. Please send your feedback to patchew-devel@redhat.com
diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c index 06e376bcd2..2b6fe77a6b 100644 --- a/hw/usb/dev-mtp.c +++ b/hw/usb/dev-mtp.c @@ -1135,11 +1135,19 @@ static MTPData *usb_mtp_get_object_prop_value(MTPState *s, MTPControl *c, return d; } -/* Return correct return code for a delete event */ +/* + * Return values when object @o is deleted. + * If at least one of the deletions succeeded, + * DELETE_SUCCESS is set and if at least one + * of the deletions failed, DELETE_FAILURE is + * set. Both bits being set (DELETE_PARTIAL) + * signifies a RES_PARITAL_DELETE being sent + * back to the initiator. + */ enum { - ALL_DELETE, - PARTIAL_DELETE, - READ_ONLY, + DELETE_SUCCESS = (1 << 0), + DELETE_FAILURE = (1 << 1), + DELETE_PARTIAL = (DELETE_FAILURE | DELETE_SUCCESS), }; /* Assumes that children, if any, have been already freed */ @@ -1155,8 +1163,7 @@ static void usb_mtp_object_free_one(MTPState *s, MTPObject *o) static int usb_mtp_deletefn(MTPState *s, MTPObject *o, uint32_t trans) { MTPObject *iter, *iter2; - bool partial_delete = false; - bool success = false; + int ret = 0; /* * TODO: Add support for Protection Status @@ -1165,34 +1172,28 @@ static int usb_mtp_deletefn(MTPState *s, MTPObject *o, uint32_t trans) QLIST_FOREACH(iter, &o->children, list) { if (iter->format == FMT_ASSOCIATION) { QLIST_FOREACH(iter2, &iter->children, list) { - usb_mtp_deletefn(s, iter2, trans); + ret |= usb_mtp_deletefn(s, iter2, trans); } } } if (o->format == FMT_UNDEFINED_OBJECT) { if (remove(o->path)) { - partial_delete = true; + ret |= DELETE_FAILURE; } else { usb_mtp_object_free_one(s, o); - success = true; + ret |= DELETE_SUCCESS; } } else if (o->format == FMT_ASSOCIATION) { if (rmdir(o->path)) { - partial_delete = true; + ret |= DELETE_FAILURE; } else { usb_mtp_object_free_one(s, o); - success = true; + ret |= DELETE_SUCCESS; } } - if (success && partial_delete) { - return PARTIAL_DELETE; - } - if (!success && partial_delete) { - return READ_ONLY; - } - return ALL_DELETE; + return ret; } static void usb_mtp_object_delete(MTPState *s, uint32_t handle, @@ -1226,19 +1227,24 @@ static void usb_mtp_object_delete(MTPState *s, uint32_t handle, } ret = usb_mtp_deletefn(s, o, trans); - if (ret == PARTIAL_DELETE) { - usb_mtp_queue_result(s, RES_PARTIAL_DELETE, - trans, 0, 0, 0, 0); - return; - } else if (ret == READ_ONLY) { - usb_mtp_queue_result(s, RES_STORE_READ_ONLY, trans, - 0, 0, 0, 0); - return; - } else { + switch (ret) { + case DELETE_SUCCESS: usb_mtp_queue_result(s, RES_OK, trans, 0, 0, 0, 0); - return; + break; + case DELETE_FAILURE: + usb_mtp_queue_result(s, RES_STORE_READ_ONLY, + trans, 0, 0, 0, 0); + break; + case DELETE_PARTIAL: + usb_mtp_queue_result(s, RES_PARTIAL_DELETE, + trans, 0, 0, 0, 0); + break; + default: + assert(ret != 0); } + + return; } static void usb_mtp_command(MTPState *s, MTPControl *c)
Spotted by Coverity: CID 1399414 mtp delete allows the return status of delete succeeded, partial_delete or readonly - when none of the objects could be deleted. Give more meaningful names to return values of the delete function. Some initiators recurse over the objects themselves. In that case, only READ_ONLY can be returned. Signed-off-by: Bandan Das <bsd@redhat.com> --- v2: Change the enum variable names and specify them as bits Add a comment describing the bit definitions Modify commit message slightly hw/usb/dev-mtp.c | 62 ++++++++++++++++++++++++++---------------------- 1 file changed, 34 insertions(+), 28 deletions(-)