[libvirt] [PATCH 2/3] avoid closing fd more than once

[Wen Congyang]

At 05/30/2012 09:17 PM, Eric Blake Wrote:
> On 05/30/2012 03:20 AM, Wen Congyang wrote:
>> fdstream:
>>     If fd is fds[0] or fds[1], we should set to -1 if we meet
>>     some error.
>>
>>     childfd is fds[0] or fds[1], so we should close it only when
>>     virFDStreamOpenFileInternal() successes.
>>
>> qemu_migration:
>>     If we migrate to fd, spec->fwdType is not MIGRATION_FWD_DIRECT,
>>     we will close spec->dest.fd.local in qemuMigrationRun(). So we
>>     should set spec->dest.fd.local to -1 in qemuMigrationRun(). 
>>
>> command:
>>     we should not set *outfd or *errfd if virExecWithHook() failed
>>     because the caller may close these fds.
> 
> We should split this into three separate patches, to aid backporting
> each patch across appropriate versions.  Needs a v2 for this reason; as
> I want these bugs fixed sooner rather than later, I'll probably help by
> reposting things myself.
> 
>>
>> ---
>>  src/fdstream.c            |   15 ++++++++++-----
>>  src/qemu/qemu_migration.c |    4 +++-
>>  src/util/command.c        |    8 ++++----
>>  3 files changed, 17 insertions(+), 10 deletions(-)
>>
>> diff --git a/src/fdstream.c b/src/fdstream.c
>> index 32d386d..d0ea0ee 100644
>> --- a/src/fdstream.c
>> +++ b/src/fdstream.c
>> @@ -581,6 +581,7 @@ virFDStreamOpenFileInternal(virStreamPtr st,
>>      struct stat sb;
>>      virCommandPtr cmd = NULL;
>>      int errfd = -1;
>> +    int childfd = -1;
>>  
>>      VIR_DEBUG("st=%p path=%s oflags=%x offset=%llu length=%llu mode=%o",
>>                st, path, oflags, offset, length, mode);
>> @@ -619,7 +620,6 @@ virFDStreamOpenFileInternal(virStreamPtr st,
>>      if ((st->flags & VIR_STREAM_NONBLOCK) &&
>>          (!S_ISCHR(sb.st_mode) &&
>>           !S_ISFIFO(sb.st_mode))) {
>> -        int childfd;
>>  
>>          if ((oflags & O_ACCMODE) == O_RDWR) {
>>              streamsReportError(VIR_ERR_INTERNAL_ERROR,
>> @@ -652,15 +652,20 @@ virFDStreamOpenFileInternal(virStreamPtr st,
>>          }
>>          virCommandSetErrorFD(cmd, &errfd);
>>  
>> -        if (virCommandRunAsync(cmd, NULL) < 0)
>> +        if (virCommandRunAsync(cmd, NULL) < 0) {
>> +            /* donot close fd twice if we meet some error */
> 
> s/donot/don't/
> 
>> +            fd = -1;
>>              goto error;
>> -
>> -        VIR_FORCE_CLOSE(childfd);
>> +        }
>>      }
>>  
>> -    if (virFDStreamOpenInternal(st, fd, cmd, errfd, length) < 0)
>> +    if (virFDStreamOpenInternal(st, fd, cmd, errfd, length) < 0) {
>> +        /* donot close fd twice if we meet some error */
> 
> and again.
> 
>> +        fd = -1;
>>          goto error;
>> +    }
>>  
>> +    VIR_FORCE_CLOSE(childfd);
>>      return 0;
> 
> Doesn't this leak childfd on error?

No, childfd is fds[0] or fds[1]. We have closed fds[0] and fds[1] on error,
so we should not close childfd on error.

> 
> Maybe a better solution here would be that when we assign fd and childfd
> to elements of fds[], then we also assign fds to -1, as in:

Good idea.

Thanks
Wen Congyang

> 
> diff --git i/src/fdstream.c w/src/fdstream.c
> index 32d386d..b797a05 100644
> --- i/src/fdstream.c
> +++ w/src/fdstream.c
> @@ -581,6 +581,7 @@ virFDStreamOpenFileInternal(virStreamPtr st,
>      struct stat sb;
>      virCommandPtr cmd = NULL;
>      int errfd = -1;
> +    int childfd = -1;
> 
>      VIR_DEBUG("st=%p path=%s oflags=%x offset=%llu length=%llu mode=%o",
>                st, path, oflags, offset, length, mode);
> @@ -619,7 +620,6 @@ virFDStreamOpenFileInternal(virStreamPtr st,
>      if ((st->flags & VIR_STREAM_NONBLOCK) &&
>          (!S_ISCHR(sb.st_mode) &&
>           !S_ISFIFO(sb.st_mode))) {
> -        int childfd;
> 
>          if ((oflags & O_ACCMODE) == O_RDWR) {
>              streamsReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -650,6 +650,7 @@ virFDStreamOpenFileInternal(virStreamPtr st,
>              fd = fds[1];
>              virCommandSetInputFD(cmd, childfd);
>          }
> +        fds[0] = fds[1] = -1;
>          virCommandSetErrorFD(cmd, &errfd);
> 
>          if (virCommandRunAsync(cmd, NULL) < 0)
> @@ -668,6 +669,7 @@ error:
>      VIR_FORCE_CLOSE(fds[0]);
>      VIR_FORCE_CLOSE(fds[1]);
>      VIR_FORCE_CLOSE(fd);
> +    VIR_FORCE_CLOSE(childfd);
>      if (oflags & O_CREAT)
>          unlink(path);
>      return -1;
> 
> 
>> +++ b/src/qemu/qemu_migration.c
>> @@ -1910,8 +1910,10 @@ qemuMigrationRun(struct qemud_driver *driver,
>>          break;
>>  
>>      case MIGRATION_DEST_FD:
>> -        if (spec->fwdType != MIGRATION_FWD_DIRECT)
>> +        if (spec->fwdType != MIGRATION_FWD_DIRECT) {
>>              fd = spec->dest.fd.local;
>> +            spec->dest.fd.local = -1;
>> +        }
>>          ret = qemuMonitorMigrateToFd(priv->mon, migrate_flags,
>>                                       spec->dest.fd.qemu);
>>          VIR_FORCE_CLOSE(spec->dest.fd.qemu);
> 
> This hunk is probably okay on its own.
> 
>> diff --git a/src/util/command.c b/src/util/command.c
>> index eaa9f16..2723fde 100644
>> --- a/src/util/command.c
>> +++ b/src/util/command.c
>> @@ -493,6 +493,10 @@ virExecWithHook(const char *const*argv,
>>      }
>>  
>>      if (pid) { /* parent */
>> +        if (forkRet < 0) {
>> +            goto cleanup;
>> +        }
>> +
>>          VIR_FORCE_CLOSE(null);
>>          if (outfd && *outfd == -1) {
>>              VIR_FORCE_CLOSE(pipeout[1]);
>> @@ -503,10 +507,6 @@ virExecWithHook(const char *const*argv,
>>              *errfd = pipeerr[0];
>>          }
>>  
>> -        if (forkRet < 0) {
>> -            goto cleanup;
>> -        }
>> -
>>          *retpid = pid;
>>  
>>          if (binary != argv[0])
> 
> And this one is probably okay as well (I'll have to spend more time
> reading it); again a reason for splitting these into independent patches.
>