[Qemu-devel] [Bug 1798780] [NEW] hw/usb/dev-mtp.c:1616: bad test ?

[Qemu-devel] [Bug 1798780] [NEW] hw/usb/dev-mtp.c:1616: bad test ?

[dcb]

Public bug reported:

hw/usb/dev-mtp.c:1616:52: warning: logical ‘or’ of collectively
exhaustive tests is always true [-Wlogical-op]

Source code is

                if ((ret == -1) && (errno != EINTR || errno != EAGAIN ||
                                    errno != EWOULDBLOCK)) {

Maybe better code

                if ((ret == -1) && (errno != EINTR && errno != EAGAIN &&
                                    errno != EWOULDBLOCK)) {

** Affects: qemu
     Importance: Undecided
         Status: New

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1798780

Title:
  hw/usb/dev-mtp.c:1616: bad test ?

Status in QEMU:
  New

Bug description:
  hw/usb/dev-mtp.c:1616:52: warning: logical ‘or’ of collectively
  exhaustive tests is always true [-Wlogical-op]

  Source code is

                  if ((ret == -1) && (errno != EINTR || errno != EAGAIN ||
                                      errno != EWOULDBLOCK)) {

  Maybe better code

                  if ((ret == -1) && (errno != EINTR && errno != EAGAIN &&
                                      errno != EWOULDBLOCK)) {

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1798780/+subscriptions

Re: [Qemu-devel] [Bug 1798780] [NEW] hw/usb/dev-mtp.c:1616: bad test ?

[Peter Maydell]

On Fri, 19 Oct 2018 at 10:22, dcb <1798780@bugs.launchpad.net> wrote:
> hw/usb/dev-mtp.c:1616:52: warning: logical ‘or’ of collectively
> exhaustive tests is always true [-Wlogical-op]
>
> Source code is
>
>                 if ((ret == -1) && (errno != EINTR || errno != EAGAIN ||
>                                     errno != EWOULDBLOCK)) {
>
> Maybe better code
>
>                 if ((ret == -1) && (errno != EINTR && errno != EAGAIN &&
>                                     errno != EWOULDBLOCK)) {

Hi Gerd, Bandan -- I was going through older launchpad bugs and
noticed that this one about a dubious conditional in dev-mtp.c is
still unfixed.

Is the file descriptor being used here one that's in non-blocking
mode? If so, then busy-waiting in a loop while the write() returns
EWOULDBLOCK is probably not what you wanted. If it's not then
there's no need to check for EAGAIN or EWOULDBLOCK, I think.
Consider using qemu_write_full() instead of open-coding
the retry loop ?

thanks
-- PMM

Re: [Qemu-devel] [Bug 1798780] [NEW] hw/usb/dev-mtp.c:1616: bad test ?

[Bandan Das]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Fri, 19 Oct 2018 at 10:22, dcb <1798780@bugs.launchpad.net> wrote:
>> hw/usb/dev-mtp.c:1616:52: warning: logical ‘or’ of collectively
>> exhaustive tests is always true [-Wlogical-op]
>>
>> Source code is
>>
>>                 if ((ret == -1) && (errno != EINTR || errno != EAGAIN ||
>>                                     errno != EWOULDBLOCK)) {
>>
>> Maybe better code
>>
>>                 if ((ret == -1) && (errno != EINTR && errno != EAGAIN &&
>>                                     errno != EWOULDBLOCK)) {
>
> Hi Gerd, Bandan -- I was going through older launchpad bugs and
> noticed that this one about a dubious conditional in dev-mtp.c is
> still unfixed.
>
> Is the file descriptor being used here one that's in non-blocking
> mode? If so, then busy-waiting in a loop while the write() returns
> EWOULDBLOCK is probably not what you wanted. If it's not then
> there's no need to check for EAGAIN or EWOULDBLOCK, I think.
> Consider using qemu_write_full() instead of open-coding
> the retry loop ?
>

Thanks for the suggestion, Peter. I agree, this isn't
non-blocking and qemu_write_full() can be used. Following up
with a patch.

Bandan

> thanks
> -- PMM

[Qemu-devel] [PATCH] usb-mtp: replace the homebrew write with qemu_write_full

[Bandan Das]

qemu_write_full takes care of partial blocking writes,
as in cases of larger file sizes

Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Bandan <bsd@redhat.com>
---
 hw/usb/dev-mtp.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index 31fa83589b..53e20e2161 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -1630,25 +1630,16 @@ static char *utf16_to_str(uint8_t len, uint16_t *arr)
 /* Wrapper around write, returns 0 on failure */
 static uint64_t write_retry(int fd, void *buf, uint64_t size, off_t offset)
 {
-        uint64_t bytes_left = size, ret;
+        uint64_t ret = 0;
 
         if (lseek(fd, offset, SEEK_SET) < 0) {
             goto done;
         }
 
-        while (bytes_left > 0) {
-                ret = write(fd, buf, bytes_left);
-                if ((ret == -1) && (errno != EINTR || errno != EAGAIN ||
-                                    errno != EWOULDBLOCK)) {
-                        break;
-                }
-                bytes_left -= ret;
-                buf += ret;
-        }
+        ret = qemu_write_full(fd, buf, size);
 
 done:
-        return size - bytes_left;
+        return ret;
 }
 
 static void usb_mtp_update_object(MTPObject *parent, char *name)
-- 
2.20.1

Re: [Qemu-devel] [PATCH] usb-mtp: replace the homebrew write with qemu_write_full

[Gerd Hoffmann]

On Tue, Jan 22, 2019 at 07:41:16AM -0500, Bandan Das wrote:
> 
> qemu_write_full takes care of partial blocking writes,
> as in cases of larger file sizes
> 
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Bandan <bsd@redhat.com>

Hmm, doesn't apply, and git fails to do a 3way merge too due to unknown
sha1.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] usb-mtp: replace the homebrew write with qemu_write_full

[Bandan Das]

Gerd Hoffmann <kraxel@redhat.com> writes:

> On Tue, Jan 22, 2019 at 07:41:16AM -0500, Bandan Das wrote:
>> 
>> qemu_write_full takes care of partial blocking writes,
>> as in cases of larger file sizes
>> 
>> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
>> Signed-off-by: Bandan <bsd@redhat.com>
>
> Hmm, doesn't apply, and git fails to do a 3way merge too due to unknown
> sha1.
>

Oops, sorry, I realize now this is on top of the write buffer breakup patches.

Should I resend a v2 on top of master and send a v3 for the write buffer breakup
patches ?

> cheers,
>   Gerd

Re: [Qemu-devel] [PATCH] usb-mtp: replace the homebrew write with qemu_write_full

[Gerd Hoffmann]

On Thu, Jan 24, 2019 at 03:19:03AM -0500, Bandan Das wrote:
> Gerd Hoffmann <kraxel@redhat.com> writes:
> 
> > On Tue, Jan 22, 2019 at 07:41:16AM -0500, Bandan Das wrote:
> >> 
> >> qemu_write_full takes care of partial blocking writes,
> >> as in cases of larger file sizes
> >> 
> >> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> >> Signed-off-by: Bandan <bsd@redhat.com>
> >
> > Hmm, doesn't apply, and git fails to do a 3way merge too due to unknown
> > sha1.
> 
> Oops, sorry, I realize now this is on top of the write buffer breakup patches.

Hmm, they are queued up already, so that should have worked.

> Should I resend a v2 on top of master and send a v3 for the write buffer breakup
> patches ?

Can you just send a single series with both this fix and the breakup
patches, against latest master?

thanks,
  Gerd

[Qemu-devel] [Bug 1798780] Re: hw/usb/dev-mtp.c:1616: bad test ?

[Thomas Huth]

** Changed in: qemu
       Status: Fix Committed => Fix Released

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1798780

Title:
  hw/usb/dev-mtp.c:1616: bad test ?

Status in QEMU:
  Fix Released

Bug description:
  hw/usb/dev-mtp.c:1616:52: warning: logical ‘or’ of collectively
  exhaustive tests is always true [-Wlogical-op]

  Source code is

                  if ((ret == -1) && (errno != EINTR || errno != EAGAIN ||
                                      errno != EWOULDBLOCK)) {

  Maybe better code

                  if ((ret == -1) && (errno != EINTR && errno != EAGAIN &&
                                      errno != EWOULDBLOCK)) {

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1798780/+subscriptions