[libvirt] [PATCH v3 03/13] Turn virDomainObjList into an opaque virObject

[Jiri Denemark]

On Fri, Feb 01, 2013 at 11:18:25 +0000, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange at redhat.com>
> 
> As a step towards making virDomainObjList thread-safe turn it
> into an opaque virObject, preventing any direct access to its
> internals.
> 
> As part of this a new method virDomainObjListForEach is
> introduced to replace all existing usage of virHashForEach
...
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 990e6e4..3861e68 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
...
> @@ -15224,6 +15241,34 @@ cleanup:
>      return -1;
>  }
>  
> +
> +struct virDomainListIterData {
> +    virDomainObjListIterator callback;
> +    void *opaque;
> +    int ret;
> +};
> +
> +static void virDomainObjListHelper(void *payload, const void *name ATTRIBUTE_UNUSED, void *opaque)

    static void
    virDomainObjListHelper(void *payload,
                           const void *name ATTRIBUTE_UNUSED,
                           void *opaque)

would better fit out style and would not be longer then 80 chars :-)

> +{
> +    struct virDomainListIterData *data = opaque;
> +
> +    if (data->callback(payload, data->opaque) < 0)
> +        data->ret = -1;
> +}
> +
> +int virDomainObjListForEach(virDomainObjListPtr doms,
> +                            virDomainObjListIterator callback,
> +                            void *opaque)
> +{
> +    struct virDomainListIterData data = {
> +        callback, opaque, 0,
> +    };
> +    virHashForEach(doms->objs, virDomainObjListHelper, &data);
> +
> +    return data.ret;
> +}
> +
> +
>  int virDomainChrDefForeach(virDomainDefPtr def,
>                             bool abortOnError,
>                             virDomainChrDefIterator iter,
...
> diff --git a/src/conf/nwfilter_conf.h b/src/conf/nwfilter_conf.h
> index 35f8dde..b4573f5 100644
> --- a/src/conf/nwfilter_conf.h
> +++ b/src/conf/nwfilter_conf.h
...
> @@ -725,14 +725,14 @@ void virNWFilterObjUnlock(virNWFilterObjPtr obj);
>  void virNWFilterLockFilterUpdates(void);
>  void virNWFilterUnlockFilterUpdates(void);
>  
> -int virNWFilterConfLayerInit(virHashIterator domUpdateCB);
> +int virNWFilterConfLayerInit(virDomainObjListIterator domUpdateCB);
>  void virNWFilterConfLayerShutdown(void);
>  
>  int virNWFilterInstFiltersOnAllVMs(virConnectPtr conn);
>  
>  
>  typedef int (*virNWFilterRebuild)(virConnectPtr conn,
> -                                  virHashIterator, void *data);
> +                                  virDomainObjListIterator, void *data);

While changing this line, you could remove this mixture of styles for
function prototypes:

    ...
    virDomainObjListIterator domUpdateCB,
    void *data);

>  typedef void (*virNWFilterVoidCall)(void);
>  
>  
...
> diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c
> index 46f30ca..6eacbca 100644
> --- a/src/openvz/openvz_conf.c
> +++ b/src/openvz/openvz_conf.c
...
> @@ -594,35 +595,20 @@ int openvzLoadDomains(struct openvz_driver *driver) {
>          }
>          *line++ = '\0';
>  
> -        if (!(dom = virDomainObjNew(driver->caps)))
> -             goto cleanup;
> -
> -        if (VIR_ALLOC(dom->def) < 0)
> +        if (VIR_ALLOC(def) < 0)
>              goto no_memory;
>  
> -        dom->def->virtType = VIR_DOMAIN_VIRT_OPENVZ;
> -
> -        if (STREQ(status, "stopped")) {
> -            virDomainObjSetState(dom, VIR_DOMAIN_SHUTOFF,
> -                                 VIR_DOMAIN_SHUTOFF_UNKNOWN);
> -        } else {
> -            virDomainObjSetState(dom, VIR_DOMAIN_RUNNING,
> -                                 VIR_DOMAIN_RUNNING_UNKNOWN);
> -        }
> +        def->virtType = VIR_DOMAIN_VIRT_OPENVZ;
>  
> -        dom->pid = veid;
>          if (virDomainObjGetState(dom, NULL) == VIR_DOMAIN_SHUTOFF)
> -            dom->def->id = -1;
> +            def->id = -1;
>          else
> -            dom->def->id = veid;
> -        /* XXX OpenVZ doesn't appear to have concept of a transient domain */
> -        dom->persistent = 1;
> -
> -        if (virAsprintf(&dom->def->name, "%i", veid) < 0)
> +            def->id = veid;

You moved all the code that creates dom further but left this one here.
I think you actually wanted to change

    if (virDomainObjGetState(dom, NULL) == VIR_DOMAIN_SHUTOFF)
        def->id = -1;
    else
        def->id = veid;

to

    if (STREQ(status, "stopped"))
        def->id = -1;
    else
        def->id = veid;

> +        if (virAsprintf(&def->name, "%i", veid) < 0)
>              goto no_memory;
>  
>          openvzGetVPSUUID(veid, uuidstr, sizeof(uuidstr));
> -        ret = virUUIDParse(uuidstr, dom->def->uuid);
> +        ret = virUUIDParse(uuidstr, def->uuid);
...
> +        if (!(dom = virDomainObjListAdd(driver->domains,
> +                                        driver->caps,
> +                                        def,
> +                                        STRNEQ(status, "stopped"))))
>              goto cleanup;
> +
> +        if (STREQ(status, "stopped")) {
> +            virDomainObjSetState(dom, VIR_DOMAIN_SHUTOFF,
> +                                 VIR_DOMAIN_SHUTOFF_UNKNOWN);
> +        } else {
> +            virDomainObjSetState(dom, VIR_DOMAIN_RUNNING,
> +                                 VIR_DOMAIN_RUNNING_UNKNOWN);
>          }
> +        /* XXX OpenVZ doesn't appear to have concept of a transient domain */
> +        dom->persistent = 1;
> +        dom->pid = veid;

Shouldn't dom->pid be set only if the domain is running?

>  
>          virObjectUnlock(dom);
>          dom = NULL;
> +        def = NULL;
>      }
>  
>      virCommandFree(cmd);
...

ACK with the small issues fixed.

Jirka