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.
--
Eric Blake eblake@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list