On Fri, Feb 01, 2013 at 11:18:25 +0000, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange@xxxxxxxxxx>
>
> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list