Re: [libvirt] PATCH: 10/25: Remove use of strerror()
"Daniel P. Berrange" <berrange(a)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.