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