On 02/16/2017 02:57 PM, John Ferlan wrote: > On 02/16/2017 09:26 AM, Joao Martins wrote: >> On 02/16/2017 11:47 AM, John Ferlan wrote: >>> FYI: Couple of Coverity issues resulted from these patches. >>> >>>> int >>>> +libxlDomainMigrationPrepareTunnel3(virConnectPtr dconn, >>>> + virStreamPtr st, >>>> + virDomainDefPtr *def, >>>> + const char *cookiein, >>>> + int cookieinlen, >>>> + unsigned int flags) >>>> +{ >>>> + libxlMigrationCookiePtr mig = NULL; >>>> + libxlDriverPrivatePtr driver = dconn->privateData; >>>> + virDomainObjPtr vm = NULL; >>>> + libxlMigrationDstArgs *args = NULL; >>>> + virThread thread; >>>> + bool taint_hook = false; >>>> + libxlDomainObjPrivatePtr priv = NULL; >>>> + char *xmlout = NULL; >>>> + int dataFD[2] = { -1, -1 }; >>>> + int ret = -1; >>>> + >>>> + if (libxlDomainMigrationPrepareAny(dconn, def, cookiein, cookieinlen, >>>> + &mig, &xmlout, &taint_hook) < 0) >>> >>> Coverity notes that '@mig' will be leaked in this case when "if >>> (!cookiein || !cookieinlen) {" is true in libxlMigrationEatCookie and >>> failures from libxlDomainMigrationPrepareAny don't free it. >> >> /nods. This diff should do then? It covers both cases I think. > > What about: > > args->migcookie = mig; > > > and then successful virThreadCreate(&thread, false, > libxlDoMigrateReceive, args) where 'args' is unref'd, but the cookie > isn't free'd. > > Certainly you wouldn't want to free the cookie and then let the thread > use it! \facepalm - you're right.
> So I think the libxlMigrationCookieFree needs to be done in the error: > label and in the thread cleanup. Also after you assign @mig to > migcookie, set 'mig = NULL'; Yeap, let me send that in a follow-up patch shortly. The thread cleanup you mean libxlDoMigrateReceive right? There I think we could simply move unref args to cleanup label (which would free the cookie). But probably should go into a separate patch as it seems to be an issue to all migration paths. >> diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c >> index ba1ca5c..6a7d458 100644 >> --- a/src/libxl/libxl_migration.c >> +++ b/src/libxl/libxl_migration.c >> @@ -637,6 +637,7 @@ libxlDomainMigrationPrepareTunnel3(virConnectPtr dconn, >> } >> >> done: >> + libxlMigrationCookieFree(mig); >> if (vm) >> virObjectUnlock(vm); >> >> >> >>>> + goto error; >>>> + >>>> + if (!(vm = virDomainObjListAdd(driver->domains, *def, >>>> + driver->xmlopt, >>>> + VIR_DOMAIN_OBJ_LIST_ADD_LIVE | >>>> + VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE, >>>> + NULL))) >>>> + goto error; >>>> + >>>> + *def = NULL; >>>> + priv = vm->privateData; >>>> + >>>> + if (taint_hook) { >>>> + /* Domain XML has been altered by a hook script. */ >>>> + priv->hookRun = true; >>>> + } >>>> + >>>> + /* >>>> + * The data flow of tunnel3 migration in the dest side: >>>> + * stream -> pipe -> recvfd of libxlDomainStartRestore >>>> + */ >>>> + if (pipe(dataFD) < 0) >>>> + goto error; >>>> + >>>> + /* Stream data will be written to pipeIn */ >>>> + if (virFDStreamOpen(st, dataFD[1]) < 0) >>>> + goto error; >>>> + dataFD[1] = -1; /* 'st' owns the FD now & will close it */ >>>> + >>>> + if (libxlMigrationDstArgsInitialize() < 0) >>>> + goto error; >>>> + >>>> + if (!(args = virObjectNew(libxlMigrationDstArgsClass))) >>>> + goto error; >>>> + >>>> + args->conn = dconn; >>>> + args->vm = vm; >>>> + args->flags = flags; >>>> + args->migcookie = mig; >>>> + /* Receive from pipeOut */ >>>> + args->recvfd = dataFD[0]; >>>> + args->nsocks = 0; >>>> + if (virThreadCreate(&thread, false, libxlDoMigrateReceive, args) < 0) >>>> { >>>> + virReportError(VIR_ERR_OPERATION_FAILED, "%s", >>>> + _("Failed to create thread for receiving migration >>>> data")); >>>> + goto error; >>>> + } >>>> + >>>> + ret = 0; >>>> + goto done; >>>> + >>>> + error: >>>> + VIR_FORCE_CLOSE(dataFD[1]); >>>> + VIR_FORCE_CLOSE(dataFD[0]); >>>> + virObjectUnref(args); >>>> + /* Remove virDomainObj from domain list */ >>>> + if (vm) { >>>> + virDomainObjListRemove(driver->domains, vm); >>>> + vm = NULL; >>>> + } >>>> + >>>> + done: >>>> + if (vm) >>>> + virObjectUnlock(vm); >>>> + >>>> + return ret; >>>> +} >>> >>> [...] >>> >>>> + >>>> +static int >>>> +libxlMigrationStartTunnel(libxlDriverPrivatePtr driver, >>>> + virDomainObjPtr vm, >>>> + unsigned long flags, >>>> + virStreamPtr st, >>>> + struct libxlTunnelControl *tc) >>>> +{ >>>> + libxlTunnelMigrationThread *arg = NULL; >>>> + int ret = -1; >>>> + >>>> + if (VIR_ALLOC(tc) < 0) >>>> + goto out; >>>> + >>>> + tc->dataFD[0] = -1; >>>> + tc->dataFD[1] = -1; >>>> + if (pipe(tc->dataFD) < 0) { >>>> + virReportError(errno, "%s", _("Unable to make pipes")); >>> >>> Coverity notes that failures would cause a leak for @tc (I assume here >>> and of course if virThreadCreate fails. Perhaps the 'best' way to >>> handle that would be to set tc = NULL after ThreadCreate success and >>> just call libxlMigrationStopTunnel in "out:"... >> >> But libxlMigrationStopTunnel is always called (hence should be freeing @tc), >> whether libxlMigrationStartTunnel failed or not. How about this? >> >> @@ -907,8 +908,9 @@ libxlMigrationStartTunnel(libxlDriverPrivatePtr driver, >> virDomainObjPtr vm, >> unsigned long flags, >> virStreamPtr st, >> - struct libxlTunnelControl *tc) >> + struct libxlTunnelControl **tnl) >> { >> + struct libxlTunnelControl *tc = *tnl; > > This doesn't do much... since @tc would be allocated below and *tnl > wouldn't be updated... Thus, the following is better: > > if (VIR_ALLOC(tc) < 0) > goto out; > *tnl = tc; > OK. >> libxlTunnelMigrationThread *arg = NULL; >> int ret = -1; >> >> @@ -1045,7 +1047,7 @@ libxlDoMigrateP2P(libxlDriverPrivatePtr driver, >> >> VIR_DEBUG("Perform3 uri=%s", NULLSTR(uri_out)); >> if (flags & VIR_MIGRATE_TUNNELLED) >> - ret = libxlMigrationStartTunnel(driver, vm, flags, st, tc); >> + ret = libxlMigrationStartTunnel(driver, vm, flags, st, &tc); >> else >> ret = libxlDomainMigrationPerform(driver, vm, NULL, NULL, >> uri_out, NULL, flags); >> -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list