On Thu, Nov 06, 2008 at 09:09:20AM -0500, Ben Guthro wrote:
> The following patch implements a framework for emitting events from the Xen driver, via monitoring xenstore.
>
> Currently, this only emits the STARTED, and STOPPED events, as this is all that seems to be available via monitoring xenstore.
A progressive approach is fine, even that minimal set is better
than nothing and it sets the framework.
[...]
> +int
> +xenUnifiedAddDomainInfo(xenUnifiedDomainInfoListPtr list,
> + char *name, char *uuid)
> +{
[...]
> + if (VIR_ALLOC(info) < 0) {
> + xenUnifiedError (NULL, VIR_ERR_NO_MEMORY, "allocating info");
> + return -1;
> + }
> + info->name = strdup(name);
> + info->uuid = strdup(uuid);
Seems we should catch for NULL here in the two strdups
> + /* Make space on list */
> + n = list->count;
> + if (VIR_REALLOC_N(list->doms, n + 1) < 0) {
> + xenUnifiedError (NULL, VIR_ERR_NO_MEMORY, "reallocating list");
> + VIR_FREE(info);
and the info->name and info->uuid should be VIR_FREE'd too
> + return -1;
> + }
> +
> + list->doms[n] = info;
> + list->count++;
> + return 0;
> +}
I would suggest to just make a memory_error: label at the end of the
function, deallocate what was allocated there and simplify the 4 memory
error paths
[...]
> +/* xenUnifiedDomainInfoPtr:
> + * The minimal state we have about active domains
> + * This is the minmal info necessary to still get a
> + * virDomainPtr when the domain goes away
> + */
> +struct _xenUnifiedDomainInfo {
> + char *name;
> + char *uuid;
> +};
The id is the only way to check the status with a direct hypercall,
i.e. not involving a costly RPC. If we fully trust the xenstore
callbacks then it's not needed, but if we were to double check (i don't
know why i'm so suspicious ...) then the id would be nice here too.
[...]
> int
> xenStoreClose(virConnectPtr conn)
> {
> + int fd;
> xenUnifiedPrivatePtr priv;
>
> if (conn == NULL) {
> @@ -325,10 +374,23 @@ xenStoreClose(virConnectPtr conn)
> }
>
> priv = (xenUnifiedPrivatePtr) conn->privateData;
> +
> + xenStoreRemoveWatch(conn, "@introduceDomain", "introduceDomain");
> + xenStoreRemoveWatch(conn, "@releaseDomain", "releaseDomain");
what hapens if we fail to remove a watch ? It's probably fine to
ignore but it seems it could fail potentially
/usr/include/xs.h
/* Remove a watch on a node: implicitly acks any outstanding watch.
* Returns false on failure (no watch on that node).
*/
bool xs_unwatch(struct xs_handle *h, const char *path, const char
*token);
[...]
> +int xenStoreRemoveWatch(virConnectPtr conn,
> + const char *path,
> + const char *token)
> +{
> + int i;
> + xenStoreWatchListPtr list;
> + xenUnifiedPrivatePtr priv = (xenUnifiedPrivatePtr) conn->privateData;
> +
> + if (priv->xshandle == NULL)
> + return -1;
> +
> + list = priv->xsWatchList;
> + if(!list)
> + return -1;
> +
> + for (i = 0 ; i < list->count ; i++) {
> + if( STREQ(list->watches[i]->path, path) &&
> + STREQ(list->watches[i]->token, token)) {
> +
> + xs_unwatch(priv->xshandle,
> + list->watches[i]->path,
> + list->watches[i]->path);
maybe we should report errors there ... but continue ?
> + VIR_FREE(list->watches[i]->path);
> + VIR_FREE(list->watches[i]->token);
> + VIR_FREE(list->watches[i]);
> +
> + if (i < (list->count - 1))
> + memmove(list->watches + i,
> + list->watches + i + 1,
> + sizeof(*(list->watches)) *
> + (list->count - (i + 1)));
> +
> + if (VIR_REALLOC_N(list->watches,
> + list->count - 1) < 0) {
> + ; /* Failure to reduce memory allocation isn't fatal */
> + }
> + list->count--;
> +#ifndef PROXY
> + virUnrefConnect(conn);
> +#endif
> + return 0;
> + }
> + }
> + return -1;
> +}
Overall looks fine to me,
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list