[libvirt] Re: [Libvir] Reopening the old discussion about virDomainBlockPeek

[Jim Meyering]

"Richard W.M. Jones" <rjones at redhat.com> wrote:
> Suggested patch attached.

Looks fine.  Now that you've added the format string
argument, we can give diagnostics that are more useful not
just to users, but also to those who end up debugging things.

> It seems a little bit perverse to have to put gettext around
> "%s: %s" but that's what make syntax-check wants.
...
On second though, you can think of this as encouragement to
avoid format strings with no translatable text.

I've found that bare "filename: strerror (errno)" diagnostics
pose enough of a problem from the superficial "where did that come from"
perspective that I try hard now always to provide a little more
information (and thus something that is translatable).
Of course, the minimal diagnostics are often hard to understand, too.
I suspect that users don't mind getting that little bit of added info...

> Index: src/xend_internal.c
> ===================================================================
...
> @@ -3925,12 +3937,14 @@ xenDaemonDomainMigratePrepare (virConnec
>      if (uri_in == NULL) {
>          r = gethostname (hostname, HOST_NAME_MAX+1);
>          if (r == -1) {
> -            virXendError (dconn, VIR_ERR_SYSTEM_ERROR, strerror (errno));
> +            virXendError (dconn, VIR_ERR_SYSTEM_ERROR,
> +                          "%s", strerror (errno));

maybe give more info:

                          "gethostname failed: %s", strerror (errno));

>              return -1;
>          }
>          *uri_out = strdup (hostname);
>          if (*uri_out == NULL) {
> -            virXendError (dconn, VIR_ERR_SYSTEM_ERROR, strerror (errno));
> +            virXendError (dconn, VIR_ERR_SYSTEM_ERROR,
> +                          "%s", strerror (errno));

likewise, (but I don't like this one, but maybe
better than nothing):

                          "hostname strdup failed: %s", strerror (errno));

>              return -1;
>          }
>      }
> @@ -4575,14 +4589,15 @@ xenDaemonDomainBlockPeek (virDomainPtr d
>
>      if (!data.ok) {
>          virXendError (domain->conn, VIR_ERR_INVALID_ARG,
> -                      _("invalid path"));
> +                      _("%s: invalid path"), path);
>          return -1;
>      }
>
>      /* The path is correct, now try to open it and get its size. */
>      fd = open (path, O_RDONLY);
>      if (fd == -1) {
> -        virXendError (domain->conn, VIR_ERR_SYSTEM_ERROR, strerror (errno));
> +        virXendError (domain->conn, VIR_ERR_SYSTEM_ERROR,
> +                      _("%s: %s"), path, strerror (errno));

For the above, how about

                     _("%s: failed to open for reading: %s"),
                     path, strerror (errno));

and for the one below:

                      _("%s: lseek failed: %s"), path, strerror (errno));

>          goto done;
>      }
>
> @@ -4592,7 +4607,8 @@ xenDaemonDomainBlockPeek (virDomainPtr d
>       */
>      if (lseek (fd, offset, SEEK_SET) == (off_t) -1 ||
>          saferead (fd, buffer, size) == (ssize_t) -1) {
> -        virXendError (domain->conn, VIR_ERR_SYSTEM_ERROR, strerror (errno));
> +        virXendError (domain->conn, VIR_ERR_SYSTEM_ERROR,
> +                      _("%s: %s"), path, strerror (errno));