Re: [PATCH] error handling: Use TFR() macro where applicable

[Nikita Ivanov <nivanov@cloudlinux.com>]

[-- Attachment #1: Type: text/plain, Size: 2340 bytes --]

Hi! Thanks for your notes. I'll try to send updated patches by the end of
the day.

On Fri, Oct 7, 2022 at 6:32 PM Peter Maydell <peter.maydell@linaro.org>
wrote:

> I think this patch is doing things in the wrong order. Instead of
> converting code to use the old macro that we don't like and then
> updating it again in patch 2 to use the new macro, we should
> first introduce the new macro, and then after that we can update
> code that's currently not using a macro at all to use the new one.
> This makes code review easier because we don't have to look at a
> change to this code which is then going to be rewritten anyway.


Sounds smooth. I'll refactor patches accordingly.


> >      if (ret < 0) {
> >          ret = -errno;
>
>
> > @@ -1472,8 +1472,8 @@ static ssize_t
> handle_aiocb_rw_vector(RawPosixAIOData *aiocb)
> >  {
> >      ssize_t len;
> >
> > -    TFR(
> > -        len = (aiocb->aio_type & QEMU_AIO_WRITE) ?
> > +    len = TEMP_FAILURE_RETRY(
> > +        (aiocb->aio_type & QEMU_AIO_WRITE) ?
> >              qemu_pwritev(aiocb->aio_fildes,
> >                             aiocb->io.iov,
> >                             aiocb->io.niov,
>
> I'm not sure why you've put the TEMP_FAILURE_RETRY on the outside here
> rather than just on the individual function calls.
>

The original code contained both branches in one while loop, so I was
afraid that
value `aiocb->aio_type & QEMU_AIO_WRITE` would change somehow during the
loop.
If you'll say that this is impossible, I'll adjust the code as you propose.

> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> > index b1c161c035..6e244f15fa 100644
> > --- a/include/qemu/osdep.h
> > +++ b/include/qemu/osdep.h
> > @@ -243,7 +243,13 @@ void QEMU_ERROR("code path is reachable")
> >  #define ESHUTDOWN 4099
> >  #endif
> >
> > -#define TFR(expr) do { if ((expr) != -1) break; } while (errno == EINTR)
> > +#define TEMP_FAILURE_RETRY(expr) \
>
> We can't call the macro this, because the glibc system headers already
> may define a macro of that name, so the compiler will complain if they're
> both defined at the same time, and depending on header ordering it might
> not be clear which version you're getting.
>

Sorry, my fault. I will rename it to "RETRY_ON_EINTR" as it was proposed
earlier in this thread.
-- 
Best Regards,
*Nikita Ivanov* | C developer

[-- Attachment #2: Type: text/html, Size: 3491 bytes --]