[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCHv2 2/2] remote: react to failures on wakeupFD



Eric Blake wrote:
> discussion: https://www.redhat.com/archives/libvir-list/2010-March/msg00443.html
>
> * src/remote/remote_driver.c (remoteIO, remoteIOEventLoop): Report
> failures on pipe used for wakeup.
> Reported by Chris Lalancette.
> ---
>
>> > -            ignore_value(saferead(priv->wakeupReadFD, &ignore,
>> > -                                  sizeof(ignore)));
>> > +            if (saferead(priv->wakeupReadFD, &ignore, sizeof(ignore))
>> > +                != sizeof(ignore)) {
>> > +                virReportSystemError(errno ? errno : 0,
>> This looks fine, but "errno ? errno : 0" is equivalent to just "errno".
>> Which makes me think you'll want to separate the saferead-fails case
>> (where errno is nonzero) from the
>> saferead-returns-non-negative-<=-sizeof-ignore case (in which
>> case virReportSystemError would print "Success" for errno=0).
>
> Is this rewrite more what you had in mind? (No change to patch 1/2).

Thanks.
That looks better.  No more "Success" diagnostic upon error.

>  src/remote/remote_driver.c |   31 +++++++++++++++++++++++++++----
>  1 files changed, 27 insertions(+), 4 deletions(-)
>
> diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
> index ebcfcd8..e3df27b 100644
> --- a/src/remote/remote_driver.c
> +++ b/src/remote/remote_driver.c
> @@ -9523,9 +9523,18 @@ remoteIOEventLoop(virConnectPtr conn,
>          remoteDriverLock(priv);
>
>          if (fds[1].revents) {
> +            ssize_t s;
>              DEBUG0("Woken up from poll by other thread");
> -            ignore_value(saferead(priv->wakeupReadFD, &ignore,
> -                                  sizeof(ignore)));
> +            s = saferead(priv->wakeupReadFD, &ignore, sizeof(ignore));
> +            if (s < 0) {
> +                virReportSystemError(errno, "%s",
> +                                     _("read on wakeup fd failed"));
> +                goto error;
> +            } else if (s != sizeof(ignore)) {
> +                remoteError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                            _("read on wakeup fd failed"));
> +                goto error;
> +            }
>          }
>
>          if (ret < 0) {
> @@ -9661,6 +9670,7 @@ remoteIO(virConnectPtr conn,
>          /* Stick ourselves on the end of the wait queue */
>          struct remote_thread_call *tmp = priv->waitDispatch;
>          char ignore = 1;
> +        ssize_t s;
>          while (tmp && tmp->next)
>              tmp = tmp->next;
>          if (tmp)
> @@ -9668,8 +9678,21 @@ remoteIO(virConnectPtr conn,
>          else
>              priv->waitDispatch = thiscall;
>
> -        /* Force other thread to wakup from poll */
> -        ignore_value(safewrite(priv->wakeupSendFD, &ignore, sizeof(ignore)));
> +        /* Force other thread to wakeup from poll */
> +        s = safewrite(priv->wakeupSendFD, &ignore, sizeof(ignore));
> +        if (s < 0) {
> +            char errout[1024];
> +            remoteError(VIR_ERR_INTERNAL_ERROR,
> +                        _("failed to wake up polling thread: %s"),
> +                        virStrerror(errno, errout, sizeof errout));
> +            VIR_FREE(thiscall);
> +            return -1;
> +        } else if (s != sizeof(ignore)) {
> +            remoteError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                        _("failed to wake up polling thread"));
> +            VIR_FREE(thiscall);
> +            return -1;
> +        }
>
>          DEBUG("Going to sleep %d %p %p", thiscall->proc_nr, priv->waitDispatch, thiscall);
>          /* Go to sleep while other thread is working... */


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]