[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v3 19/34] Adapt to VIR_STRDUP and VIR_STRNDUP in src/rpc/*



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

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]