On 26.05.2011 07:30, Laine Stump wrote:
> On 05/24/2011 02:24 AM, Daniel Veillard wrote:
>> On Thu, May 19, 2011 at 04:51:29PM -0400, Laine Stump wrote:
>>> From: Michal Privoznik<mprivozn@xxxxxxxxxx>
>>>
>>> ---
>>> src/conf/interface_conf.c | 45 ++++++++++++++++++++++++
>>> src/conf/interface_conf.h | 4 ++
>>> src/test/test_driver.c | 83
>>> +++++++++++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 132 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/src/conf/interface_conf.c b/src/conf/interface_conf.c
>>> index f3848bd..4ff68aa 100644
>>> --- a/src/conf/interface_conf.c
>>> +++ b/src/conf/interface_conf.c
>>> @@ -1227,6 +1227,51 @@ void
>>> virInterfaceObjListFree(virInterfaceObjListPtr interfaces)
>>> interfaces->count = 0;
>>> }
>>>
>>> +int virInterfaceObjListClone(virConnectPtr conn,
>>> + virInterfaceObjListPtr src,
>>> + virInterfaceObjListPtr dest)
>>> +{
>>> + int ret = -1;
>>> + unsigned int i, cnt;
>>> +
>>> + if (!src || !dest)
>>> + goto end;
>>> +
>>> + cnt = src->count;
>>> + for (i = 0; i< cnt; i++) {
>>> + virInterfaceDefPtr def = src->objs[i]->def;
>>> + virInterfaceDefPtr backup;
>>> + virInterfaceObjPtr iface;
>>> + char *xml = virInterfaceDefFormat(def);
>>> +
>>> + if (!xml) {
>>> + virReportOOMError();
>>> + goto no_memory;
>>> + }
>>> +
>>> + if ((backup = virInterfaceDefParseString(xml)) == NULL) {
>>> + VIR_FREE(xml);
>>> + goto no_memory;
>>> + }
>>> +
>>> + VIR_FREE(xml);
>>> + if ((iface = virInterfaceAssignDef(dest, backup)) == NULL)
>>> + goto no_memory;
>>> +
>>> + virInterfaceObjUnlock(iface);
>>> +
>>> + conn->refs++;
>>> + }
>>> +
>>> + ret = cnt;
>>> +end:
>>> + return ret;
>>> +
>>> +no_memory:
>>> + virInterfaceObjListFree(dest);
>> I'ts always a problem when the callee frees up memory allocated by
>> the caller in case of error, as it's so easy to mess things up. I
>> would rather not do that here, and have the caller do the free if we
>> fail.
>> example: src == NULL, dest != NULL, and we leak dest.
I don't think so. There is a check for either src or dest being NULL at
the beginning of this function. And if so - we don't touch anything and
return -1 (meaning error).
>
> But virInterfaceObjListFree(dest) doesn't free dest itself, it only
> frees the items that are contained by dest (not dest itself), and those
> items were not allocated by the caller, but were allocated within
> virInterfaceObjListClone(), so I don't see the problem you see.
Yes. We don't do VIR_FREE(dest) here, nor virInterfaceObjListFree().
>
> There *is* a problem in general with this function, though - dest is not
> cleaned out before copying over the contents of src, so if it starts out
> non-empty, there may be extra stuff in there at the end.
Actually yes. So either rename this to virInterfaceObjListAppend()
(virInterfaceAssignDef() appends to the end) or free list items of dest
just before copying.
>
> Michal - since you wrote it, maybe you should answer this question -
> shouldn't virInterfaceObjListClone() call virInterfaceObjListFree(dest)
> as soon as it determines dest != NULL?
>
>>> + goto end;
>>> +}
>>> +
>>> virInterfaceObjPtr virInterfaceAssignDef(virInterfaceObjListPtr
>>> interfaces,
>>> const virInterfaceDefPtr def)
>>> {
>>> diff --git a/src/conf/interface_conf.h b/src/conf/interface_conf.h
>>> index 6073b49..870a8ee 100644
>>> --- a/src/conf/interface_conf.h
>>> +++ b/src/conf/interface_conf.h
>>> @@ -192,6 +192,10 @@ virInterfaceObjPtr virInterfaceFindByName(const
>>> virInterfaceObjListPtr
>>> void virInterfaceDefFree(virInterfaceDefPtr def);
>>> void virInterfaceObjFree(virInterfaceObjPtr iface);
>>> void virInterfaceObjListFree(virInterfaceObjListPtr vms);
>>> +int virInterfaceObjListClone(virConnectPtr conn,
>>> + virInterfaceObjListPtr src,
>>> + virInterfaceObjListPtr dest);
>>> +
>>>
>>> virInterfaceObjPtr virInterfaceAssignDef(virInterfaceObjListPtr
>>> interfaces,
>>> const virInterfaceDefPtr
>>> def);
>>> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
>>> index e86bc4e..b58c5d2 100644
>>> --- a/src/test/test_driver.c
>>> +++ b/src/test/test_driver.c
>>> @@ -85,6 +85,8 @@ struct _testConn {
>>> virDomainObjList domains;
>>> virNetworkObjList networks;
>>> virInterfaceObjList ifaces;
>>> + bool transaction_running;
>>> + virInterfaceObjList backupIfaces;
>>> virStoragePoolObjList pools;
>>> virNodeDeviceObjList devs;
>>> int numCells;
>>> @@ -3455,6 +3457,84 @@ cleanup:
>>> return ret;
>>> }
>>>
>>> +static int testInterfaceChangeBegin(virConnectPtr conn,
>>> + unsigned int flags
>>> ATTRIBUTE_UNUSED)
>>> +{
>>> + testConnPtr privconn = conn->privateData;
>>> + int ret = -1;
>>> +
>>> + testDriverLock(privconn);
>>> + if (privconn->transaction_running) {
>>> + testError(VIR_ERR_OPERATION_INVALID, _("there is another
>>> transaction "
>>> + "running."));
>>> + goto cleanup;
>>> + }
>>> +
>>> + privconn->transaction_running = true;
>>> +
>>> + if (virInterfaceObjListClone(conn,&privconn->ifaces,
>>> +&privconn->backupIfaces)< 0)
>>> + goto cleanup;
>>> +
>>> + ret = 0;
>>> +cleanup:
>>> + testDriverUnlock(privconn);
>>> + return ret;
>>> +}
>>> +
>>> +static int testInterfaceChangeCommit(virConnectPtr conn,
>>> + unsigned int flags
>>> ATTRIBUTE_UNUSED)
>>> +{
>>> + testConnPtr privconn = conn->privateData;
>>> + int ret = -1;
>>> +
>>> + testDriverLock(privconn);
>>> +
>>> + if (!privconn->transaction_running) {
>>> + testError(VIR_ERR_OPERATION_INVALID, _("no transaction
>>> running, "
>>> + "nothing to be commited."));
>>> + goto cleanup;
>>> + }
>>> +
>>> + virInterfaceObjListFree(&privconn->backupIfaces);
>>> + privconn->transaction_running = false;
>>> +
>>> + ret = 0;
>>> +
>>> +cleanup:
>>> + testDriverUnlock(privconn);
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static int testInterfaceChangeRollback(virConnectPtr conn,
>>> + unsigned int flags
>>> ATTRIBUTE_UNUSED)
>>> +{
>>> + testConnPtr privconn = conn->privateData;
>>> + int ret = -1;
>>> +
>>> + testDriverLock(privconn);
>>> +
>>> + if (!privconn->transaction_running) {
>>> + testError(VIR_ERR_OPERATION_INVALID, _("no transaction
>>> running, "
>>> + "nothing to rollback."));
>>> + goto cleanup;
>>> + }
>>> +
>>> + virInterfaceObjListFree(&privconn->ifaces);
>>> + privconn->ifaces.count = privconn->backupIfaces.count;
>>> + privconn->ifaces.objs = privconn->backupIfaces.objs;
>>> + privconn->backupIfaces.count = 0;
>>> + privconn->backupIfaces.objs = NULL;
>>> +
>>> + privconn->transaction_running = false;
>>> +
>>> + ret = 0;
>>> +
>>> +cleanup:
>>> + testDriverUnlock(privconn);
>>> + return ret;
>>> +}
>>>
>>> static char *testInterfaceGetXMLDesc(virInterfacePtr iface,
>>> unsigned int flags
>>> ATTRIBUTE_UNUSED)
>>> @@ -5428,6 +5508,9 @@ static virInterfaceDriver testInterfaceDriver = {
>>> .interfaceCreate = testInterfaceCreate, /* 0.7.0 */
>>> .interfaceDestroy = testInterfaceDestroy, /* 0.7.0 */
>>> .interfaceIsActive = testInterfaceIsActive, /* 0.7.3 */
>>> + .interfaceChangeBegin = testInterfaceChangeBegin, /* 0.9.2 */
>>> + .interfaceChangeCommit = testInterfaceChangeCommit, /* 0.9.2 */
>>> + .interfaceChangeRollback = testInterfaceChangeRollback, /* 0.9.2 */
>>> };
>> Except for the small nit on how virInterfaceObjListClone()
>> handle errors, fine, and ACK once fixed (just do the free in the
>> caller on error).
>>
>> Daniel
>>
>
> --
> libvir-list mailing list
> libvir-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list