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.
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list