Re: [libvirt] [PATCHv2 7/7] interface: implement a test driver for network config transaction API.

[Laine Stump <laine laine org>]

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 redhat com>
> >
> > ---
> >   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.

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.

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.

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