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

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?

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:

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.