"Daniel P. Berrange" <berrange@xxxxxxxxxx> 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.
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list