[libvirt] [PATCHv2] util: ensure safe{read, write, zero} return is checked

[Chris Lalancette]

On 03/03/2010 01:47 PM, Eric Blake wrote:
> Based on a warning from coverity.  The safe* functions
> guarantee complete transactions on success, but don't guarantee
> freedom from failure.

<snip>

> diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
> index b7b2e09..d9cae8b 100644
> --- a/src/remote/remote_driver.c
> +++ b/src/remote/remote_driver.c
> @@ -7868,26 +7868,35 @@ remoteIOReadBuffer(virConnectPtr conn,
>              if (ret == -1) {
>                  if (errno == EINTR)
>                      goto resend;
>                  if (errno == EWOULDBLOCK)
>                      return 0;
> 
>                  char errout[1024] = "\0";
>                  if (priv->errfd != -1) {
> -                    saferead(priv->errfd, errout, sizeof(errout));
> +                    if (saferead(priv->errfd, errout, sizeof(errout)) < 0) {
> +                        virReportSystemError(errno, "%s",
> +                                             _("cannot recv data: unknown reason"));
> +                        return -1;
> +                    }
>                  }

Minor point, but is "unknown reason" really the right thing we want to say
here?  Won't we know the reason from errno?

> 
>                  virReportSystemError(errno,
>                                       _("cannot recv data: %s"), errout);
> 
>              } else {
>                  char errout[1024] = "\0";
>                  if (priv->errfd != -1) {
> -                    saferead(priv->errfd, errout, sizeof(errout));
> +                    if (saferead(priv->errfd, errout, sizeof(errout)) < 0) {
> +                        errorf (in_open ? NULL : conn,
> +                                VIR_ERR_SYSTEM_ERROR,
> +                                _("server closed connection: unknown reason"));
> +                        return -1;
> +                    }
>                  }

Same here.

> 
>                  errorf (in_open ? NULL : conn,
>                          VIR_ERR_SYSTEM_ERROR,
>                          _("server closed connection: %s"), errout);
>              }
>              return -1;
>          }
> @@ -8490,17 +8499,18 @@ remoteIOEventLoop(virConnectPtr conn,
>              goto repoll;
> 
>          ignore_value (pthread_sigmask(SIG_SETMASK, &oldmask, NULL));
> 
>          remoteDriverLock(priv);
> 
>          if (fds[1].revents) {
>              DEBUG0("Woken up from poll by other thread");
> -            saferead(priv->wakeupReadFD, &ignore, sizeof(ignore));
> +            ignore_value (saferead(priv->wakeupReadFD, &ignore,
> +                                   sizeof(ignore)));

Hm.  Are we sure we want to ignore the return value here?  On the
one hand, the fact that we failed to read this byte is irrelevant;
it has done it's job to wake us out of poll.  On the other hand,
if we can't read this one byte, that probably means something more
serious is wrong and we might (probably will?) have problems later.
Should we goto error here?

>          }
> 
>          if (ret < 0) {
>              if (errno == EWOULDBLOCK)
>                  continue;
>              virReportSystemError(errno,
>                                   "%s", _("poll on socket failed"));
>              goto error;
> @@ -8634,17 +8644,17 @@ remoteIO(virConnectPtr conn,
>          while (tmp && tmp->next)
>              tmp = tmp->next;
>          if (tmp)
>              tmp->next = thiscall;
>          else
>              priv->waitDispatch = thiscall;
> 
>          /* Force other thread to wakup from poll */
> -        safewrite(priv->wakeupSendFD, &ignore, sizeof(ignore));
> +        ignore_value (safewrite(priv->wakeupSendFD, &ignore, sizeof(ignore)));

Same here.

-- 
Chris Lalancette