On 05/03/2013 08:53 AM, Michal Privoznik wrote:
--- src/rpc/gendispatch.pl | 21 ++++--------
Ah, you DID notice the code generator, so that answers my comment on 18/34.
src/rpc/virnetclient.c | 16 ++++------ src/rpc/virnetmessage.c | 12 ++++--- src/rpc/virnetsaslcontext.c | 6 ++-- src/rpc/virnetserver.c | 6 ++-- src/rpc/virnetserverclient.c | 10 ++---- src/rpc/virnetservermdns.c | 6 ++-- src/rpc/virnetsocket.c | 10 +++--- src/rpc/virnetsshsession.c | 76 ++++++++++++++++++++++---------------------- src/rpc/virnettlscontext.c | 26 +++++++-------- 10 files changed, 82 insertions(+), 107 deletions(-)
+++ b/src/rpc/virnetclient.c @@ -317,17 +318,14 @@ static virNetClientPtr virNetClientNew(virNetSocketPtr sock, client->wakeupSendFD = wakeupFD[1]; wakeupFD[0] = wakeupFD[1] = -1;
- if (hostname && - !(client->hostname = strdup(hostname))) - goto no_memory; + if (hostname && VIR_STRDUP(client->hostname, hostname) < 0) + goto error;
Can simplify with NULL source.
+++ b/src/rpc/virnetmessage.c @@ -485,21 +486,22 @@ void virNetMessageSaveError(virNetMessageErrorPtr rerr) rerr->code = verr->code; rerr->domain = verr->domain; if (verr->message && VIR_ALLOC(rerr->message) == 0) - *rerr->message = strdup(verr->message); + ignore_value(VIR_STRDUP_QUIET(*rerr->message, verr->message));
Correct use of VIR_STRDUP_QUIET here. Hmm - wonder if there is a pre-existing bug. If VIR_ALLOC succeeds but strdup fails, rerr->message points to a NULL pointer. The intent was that we _try_ to copy the string, but if it fails, we are okay for the RPC message to go out with a loss of information - but that should be done with rerr->message being a NULL pointer, not pointing to a NULL pointer. That is, I think this code should read: if (verr->message && VIR_ALLOC(rerr->message) == 0 && VIR_STRDUP_QUIET(*rerr->message, verr->message) < 0) VIR_FREE(rerr->message);
rerr->level = verr->level; if (verr->str1 && VIR_ALLOC(rerr->str1) == 0) - *rerr->str1 = strdup(verr->str1); + ignore_value(VIR_STRDUP_QUIET(*rerr->str1, verr->str1)); if (verr->str2 && VIR_ALLOC(rerr->str2) == 0) - *rerr->str2 = strdup(verr->str2); + ignore_value(VIR_STRDUP_QUIET(*rerr->str2, verr->str2)); if (verr->str3 && VIR_ALLOC(rerr->str3) == 0) - *rerr->str3 = strdup(verr->str3); + ignore_value(VIR_STRDUP_QUIET(*rerr->str3, verr->str3));
Likewise for these three best-effort strings.
rerr->int1 = verr->int1; rerr->int2 = verr->int2; } else { rerr->code = VIR_ERR_INTERNAL_ERROR; rerr->domain = VIR_FROM_RPC; if (VIR_ALLOC(rerr->message) == 0) - *rerr->message = strdup(_("Library function returned error but did not set virError")); + ignore_value(VIR_STRDUP_QUIET(*rerr->message, + _("Library function returned error but did not set virError")));
And another case where I think we should free rerr->message if strdup fails.
+++ b/src/rpc/virnetserver.c @@ -387,11 +388,8 @@ virNetServerPtr virNetServerNew(size_t min_workers, srv->privileged = geteuid() == 0; srv->autoShutdownInhibitFd = -1;
- if (mdnsGroupName && - !(srv->mdnsGroupName = strdup(mdnsGroupName))) { - virReportOOMError(); + if (mdnsGroupName && VIR_STRDUP(srv->mdnsGroupName, mdnsGroupName) < 0)
Can be simplified.
+++ b/src/rpc/virnetserverclient.c @@ -681,22 +681,16 @@ virNetServerClientCreateIdentity(virNetServerClientPtr client) #if WITH_SASL if (client->sasl) { const char *identity = virNetSASLSessionGetIdentity(client->sasl); - if (identity && - !(saslname = strdup(identity))) { - virReportOOMError(); + if (identity && VIR_STRDUP(saslname, identity) < 0)
Can be simplified.
goto cleanup; - } } #endif
#if WITH_GNUTLS if (client->tls) { const char *identity = virNetTLSSessionGetX509DName(client->tls); - if (identity && - !(x509dname = strdup(identity))) { - virReportOOMError(); + if (identity && VIR_STRDUP(x509dname, identity) < 0)
and again.
+++ b/src/rpc/virnetsocket.c @@ -800,10 +800,10 @@ virNetSocketNewConnectLibSSH2(const char *host, if (virNetSSHSessionSetChannelCommand(sess, command) != 0) goto error;
- if (!(authMethodNext = authMethodsCopy = strdup(authMethods))) { - virReportOOMError(); + if (VIR_STRDUP(authMethodsCopy, authMethods) < 0) goto error; - } + + authMethodNext = authMethodsCopy;
while ((authMethod = strsep(&authMethodNext, ","))) {
Wonder if we should use virStringSplit instead of strsep().
+++ b/src/rpc/virnetsshsession.c @@ -233,7 +233,7 @@ virNetSSHKbIntCb(const char *name ATTRIBUTE_UNUSED,
/* fill data structures for auth callback */ for (i = 0; i < num_prompts; i++) { - if (!(askcred[i].prompt = strdup(prompts[i].text))) { + if (VIR_STRDUP(askcred[i].prompt, prompts[i].text) < 0) { priv->authCbErr = VIR_NET_SSH_AUTHCB_OOM; goto cleanup;
Double-oom, unless you fix line 741 in virNetSSHAuthenticateKeyboardInteractive()'s switch statement to stop reporting oom when authCbErr says it has already been reported.
@@ -1037,15 +1039,17 @@ virNetSSHSessionAuthAddPrivKeyAuth(virNetSSHSessionPtr sess,
virObjectLock(sess);
- if (!(user = strdup(username)) || - !(file = strdup(keyfile))) - goto no_memory; + if (VIR_STRDUP(user, username) < 0 || + VIR_STRDUP(file, keyfile) < 0) + goto error;
- if (password && !(pass = strdup(password))) - goto no_memory; + if (password && VIR_STRDUP(pass, password) < 0) + goto error;
Can be simplified, and merged in with previous 'if'.
@@ -1111,10 +1115,8 @@ virNetSSHSessionSetChannelCommand(virNetSSHSessionPtr sess,
VIR_FREE(sess->channelCommand);
- if (command && !(sess->channelCommand = strdup(command))) { - virReportOOMError(); + if (command && VIR_STRDUP(sess->channelCommand, command) < 0) ret = -1;
Can be simplified.
- }
virObjectUnlock(sess); return ret; @@ -1137,8 +1139,8 @@ virNetSSHSessionSetHostKeyVerification(virNetSSHSessionPtr sess,
VIR_FREE(sess->hostname);
- if (hostname && !(sess->hostname = strdup(hostname))) - goto no_memory; + if (hostname && VIR_STRDUP(sess->hostname, hostname) < 0) + goto error;
and again.
+++ b/src/rpc/virnettlscontext.c @@ -1168,11 +1167,8 @@ virNetTLSSessionPtr virNetTLSSessionNew(virNetTLSContextPtr ctxt, if (!(sess = virObjectLockableNew(virNetTLSSessionClass))) return NULL;
- if (hostname && - !(sess->hostname = strdup(hostname))) { - virReportOOMError(); + if (hostname && VIR_STRDUP(sess->hostname, hostname) < 0) goto error;
and again. ACK with minor cleanups. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org