[libvirt] PATCH: 10/25: Remove use of strerror()

[Jim Meyering]

"Daniel P. Berrange" <berrange at redhat.com> wrote:
> The strerror() method is not guarenteed to be re-entrant, which is
> rather a pain because the strerror_r() method is rather unpleasant
...

Good work.
A couple of small problems and a question:

...
> @@ -614,8 +618,9 @@ int main(int argc, char *argv[])
>
>          if (pid > 0) {
>              if ((rc = virFileWritePid(LXC_STATE_DIR, name, pid)) != 0) {
> -                fprintf(stderr, _("Unable to write pid file: %s\n"),
> -                        strerror(rc));
> +                virReportSystemError(NULL, errno,
> +                                     _("Unable to write pid file '%s/%s.pid'"),
> +                                     LXC_STATE_DIR, name);
>                  _exit(1);
>              }
>

Looks like that should be "rc", not "errno".

...
> @@ -471,9 +474,9 @@ networkAddMasqueradingIptablesRules(virC
>                                           network->def->network,
>                                           network->def->bridge,
>                                           network->def->forwardDev))) {
> -        networkReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
> -                         _("failed to add iptables rule to allow forwarding to '%s' : %s\n"),
> -                         network->def->bridge, strerror(err));
> +        virReportSystemError(conn, err,
> +                             _("failed to add iptables rule to allow forwarding to '%s'"),
> +                             network->def->bridge);
>          goto masqerr2;
>      }
>

There's a stray trailing newline, above.
Obviously it was there before your changes, too.

...
> @@ -3455,23 +3452,23 @@ static int qemudDomainSetAutostart(virDo
>              int err;
>
>              if ((err = virFileMakePath(driver->autostartDir))) {
> -                qemudReportError(dom->conn, dom, NULL, VIR_ERR_INTERNAL_ERROR,
> -                                 _("cannot create autostart directory %s: %s"),
> -                                 driver->autostartDir, strerror(err));
> +                virReportSystemError(dom->conn, errno,
> +                                     _("cannot create autostart directory %s"),
> +                                     driver->autostartDir);

I think you want to retain the use of "err", above,
since virFileMakePath maps a missing / to EINVAL.

> diff --git a/src/remote_internal.c b/src/remote_internal.c
> diff --git a/src/storage_conf.c b/src/storage_conf.c
> --- a/src/storage_conf.c
> +++ b/src/storage_conf.c
> @@ -43,6 +43,8 @@
>  #include "util.h"
>  #include "memory.h"
>
> +#define VIR_FROM_THIS VIR_FROM_STORAGE
> +
>  /* Work around broken limits.h on debian etch */
>  #if defined __GNUC__ && defined _GCC_LIMITS_H_ && ! defined ULLONG_MAX
>  # define ULLONG_MAX   ULONG_LONG_MAX
> @@ -1405,9 +1407,9 @@ virStoragePoolObjSaveDef(virConnectPtr c
>          char path[PATH_MAX];
>
>          if ((err = virFileMakePath(driver->configDir))) {
> -            virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR,
> -                                  _("cannot create config directory %s: %s"),
> -                                  driver->configDir, strerror(err));
> +            virStorageReportError(conn, errno,
> +                                  _("cannot create config directory %s"),
> +                                  driver->configDir);

Same here.

Incidentally, virFileMakePath will have to go, eventually.  Not only
is it recursive in the number of components of the name being created,
but allocating PATH_MAX for each stack frame is very wasteful, and might
make it easy to blow the stack/DoS -- if there's a way to make libvirtd
call it with an abusive argument.  Worst still, it creates directories
with mkdir(path, 0777).  No virFileMakePath caller is careful to chmod
_all_ of the possibly-just-created directories, and even if they all were,
there'd still be a race.

...
> diff --git a/src/test.c b/src/test.c
...
> @@ -336,7 +339,7 @@ static int testOpenFromFile(virConnectPt
>      virDomainObjPtr dom;
>      testConnPtr privconn;
>      if (VIR_ALLOC(privconn) < 0) {
> -        testError(NULL, VIR_ERR_NO_MEMORY, "testConn");
> +        virReportOOMError(conn);

I don't remember the rules for NULL vs non-NULL conn.
Is this s/NULL/conn/ transform valid?
I think there's one more like it.