8:51 a.m.
On Tue, Feb 12, 2008 at 04:36:04AM +0000, Daniel P. Berrange wrote:
This patch provides the general purpose code for the storage driver.
The storage_driver.{c,h} file implements the libvirt internal storage
driver API. The storage_conf.{c,h} file takes care of parsing and
formatting the XML for pools and volumes. This should be reusable by
the test driver's impl of the storage APIs in the future. The final
storage_backend.{c,h} file provides a 2nd level internal driver API.
This allows the main storage driver to call into storage pool specific
impls for iSCSI, disk, filesystem, LVM and more. The storage_backend.h
definitions allow the specific drivers to declare particular parts of
the generic pool XML which are mandatory. For example, a disk pool
always requires a source device element.
[...]
+/* Flags to indicate mandatory components in the pool source */
+enum {
+ VIR_STORAGE_BACKEND_POOL_SOURCE_HOST = (1<<0),
+ VIR_STORAGE_BACKEND_POOL_SOURCE_DEVICE = (1<<1),
wondering about 1<<2 , is that the storage option your removed compared
to previous patches ?
+ VIR_STORAGE_BACKEND_POOL_SOURCE_DIR = (1<<3),
+ VIR_STORAGE_BACKEND_POOL_SOURCE_ADAPTER = (1<<4),
+};
[...]
+static virStoragePoolDefPtr virStoragePoolDefParseDoc(virConnectPtr
conn,
+ xmlXPathContextPtr ctxt,
xmlNodePtr root) {
[...]
+ if (options->flags &
VIR_STORAGE_BACKEND_POOL_SOURCE_DEVICE) {
+ xmlNodePtr *nodeset = NULL;
+ int nsource, i;
+
+ if ((nsource = virXPathNodeSet("/pool/source/device", ctxt,
&nodeset)) <= 0) {
+ virStorageReportError(conn, VIR_ERR_XML_ERROR, "cannot extract source
devices");
+ goto cleanup;
+ }
+ if ((ret->source.devices = calloc(nsource, sizeof(*ret->source.devices)))
== NULL) {
+ free(nodeset);
+ virStorageReportError(conn, VIR_ERR_NO_MEMORY, "device");
+ goto cleanup;
+ }
+ for (i = 0 ; i < nsource ; i++) {
+ xmlChar *path = xmlGetProp(nodeset[i], BAD_CAST "path");
+ if (path == NULL) {
+ virStorageReportError(conn, VIR_ERR_XML_ERROR, "missing source
device path");
hum nodeset is leaked here it seems.
+ goto cleanup;
+ }
+ ret->source.devices[i].path = (char *)path;
+ }
+ free(nodeset);
+ ret->source.ndevice = nsource;
+ }
[...]
+ authType =
virXPathString("string(/pool/source/auth/@type)", ctxt);
+ if (authType == NULL) {
+ ret->source.authType = VIR_STORAGE_POOL_AUTH_NONE;
+ } else {
+ if (STREQ(authType, "chap")) {
+ ret->source.authType = VIR_STORAGE_POOL_AUTH_CHAP;
+ } else {
+ virStorageReportError(conn, VIR_ERR_XML_ERROR, "unknown auth type
'%s'", (const char *)authType);
+ goto cleanup;
hum, the cleanup of authType is more complex than necessary, i would just
free(authType); here before goto cleanup;
+ }
+ free(authType);
+ authType = NULL;
remove this affectation
+ }
+
[...]
+ return ret;
+
+ cleanup:
+ free(uuid);
+ if (type)
+ xmlFree(type);
+ if (authType)
+ xmlFree(authType);
and remove this, basically keeping authType processing to just the block
where it is used.
+ virStoragePoolDefFree(ret);
+ return NULL;
+}
+
+virStoragePoolDefPtr virStoragePoolDefParse(virConnectPtr conn,
+ const char *xmlStr, const char *filename) {
+ virStoragePoolDefPtr ret = NULL;
+ xmlDocPtr xml = NULL;
+ xmlNodePtr node = NULL;
+ xmlXPathContextPtr ctxt = NULL;
+
[...]
+ xmlFreeDoc(xml);
+
+ return ret;
+
+ cleanup:
+ xmlXPathFreeContext(ctxt);
+ if (xml)
+ xmlFreeDoc(xml);
+ return NULL;
+}
since we try to remove if (x) free(x) style, just call xmlFreeDoc(xml);
since xmlFreeDoc handles NULLs fine.
[...]
+ cleanup:
+ if (buf) virBufferFree(buf);
same here
+ return NULL;
[...]
+ ret->target.path =
virXPathString("string(/volume/target/path)", ctxt);
+ if (options->formatFromString) {
+ char *format = virXPathString("string(/volume/target/format/@type)",
ctxt);
+ if ((ret->target.format = (options->formatFromString)(conn, format)) <
0) {
So you have in a structure an optional function to do the formatting,
feels a bit strange, but okay
[...]
+ if (xml)
+ xmlFreeDoc(xml);
one more, they can probably be all cleaned up after the commit anyway
[...]
> +static virStoragePoolObjPtr
> +virStoragePoolObjLoad(virStorageDriverStatePtr driver,
> + const char *file,
> + const char *path,
> + const char *xml,
> + const char *autostartLink) {
> + virStoragePoolDefPtr def;
> + virStoragePoolObjPtr pool;
> +
> + if (!(def = virStoragePoolDefParse(NULL, xml, file))) {
> + virErrorPtr err = virGetLastError();
> + virStorageLog("Error parsing storage pool config '%s' :
%s",
> + path, err->message);
> + return NULL;
+ }
+
> + if (!virFileMatchesNameSuffix(file, def->name, ".xml")) {
> + virStorageLog("Storage Pool config filename '%s' does not match
pool name '%s'",
> + path, def->name);
> + virStoragePoolDefFree(def);
> + return NULL;
+ }
+
> + if (!(pool = virStoragePoolObjAssignDef(NULL, driver, def))) {
> + virStorageLog("Failed to load storage pool config '%s': out of
memory", path);
> + virStoragePoolDefFree(def);
> + return NULL;
> + }
Shouldn't autostartLink and path be checked against NULL
+ pool->configFile = strdup(path);
+ if (pool->configFile == NULL) {
+ virStorageLog("Failed to load storage pool config '%s': out of
memory", path);
+ virStoragePoolDefFree(def);
+ return NULL;
+ }
+ pool->autostartLink = strdup(autostartLink);
+ if (pool->autostartLink == NULL) {
+ virStorageLog("Failed to load storage pool config '%s': out of
memory", path);
+ virStoragePoolDefFree(def);
+ return NULL;
+ }
Since that seems assumed in that function, okay it is not public, so
if internally we know the value is never NULL, fine, but otherwise we would
get a confusing error message.
[...]
diff -r 77cf7f42edd4 src/storage_driver.c
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/src/storage_driver.c Thu Feb 07 12:59:40 2008 -0500
[...]
+#define storageLog(msg...) fprintf(stderr, msg)
any way to integrate into normal error reporting ? But that should not
block from commiting to CVs.
[...]
+static int storageListPools(virConnectPtr conn, char **const names,
int nnames) {
+ virStorageDriverStatePtr driver =
(virStorageDriverStatePtr)conn->storagePrivateData;
+ virStoragePoolObjPtr pool = driver->pools;
+ int got = 0, i;
+ while (pool && got < nnames) {
+ if (virStoragePoolObjIsActive(pool)) {
+ if (!(names[got] = strdup(pool->def->name))) {
+ virStorageReportError(conn, VIR_ERR_NO_MEMORY, "names");
+ goto cleanup;
+ }
+ got++;
+ }
+ pool = pool->next;
+ }
+ return got;
+
+ cleanup:
+ for (i = 0 ; i < got ; i++) {
+ free(names[i]);
+ names[i] = NULL;
+ }
Hum, that looks right, but when passed a array like that it may be a
good idea to memset(0) it it can help triggering errors early or the task
of debugging.
[...]
+
+static int storageListDefinedPools(virConnectPtr conn, char **const names, int nnames)
{
+ virStorageDriverStatePtr driver =
(virStorageDriverStatePtr)conn->storagePrivateData;
+ virStoragePoolObjPtr pool = driver->pools;
+ int got = 0, i;
+ while (pool && got < nnames) {
+ if (!virStoragePoolObjIsActive(pool)) {
+ if (!(names[got] = strdup(pool->def->name))) {
+ virStorageReportError(conn, VIR_ERR_NO_MEMORY, "names");
+ goto cleanup;
+ }
+ got++;
+ }
+ pool = pool->next;
+ }
+ return got;
+
+ cleanup:
+ for (i = 0 ; i < got ; i++) {
+ free(names[i]);
+ names[i] = NULL;
+ }
+ return -1;
+}
Similar here.
[...]
> +static int storagePoolDelete(virStoragePoolPtr obj,
> + unsigned int flags) {
> + virStorageDriverStatePtr driver =
(virStorageDriverStatePtr)obj->conn->storagePrivateData;
> + virStoragePoolObjPtr pool = virStoragePoolObjFindByUUID(driver, obj->uuid);
> + virStorageBackendPtr backend;
> +
> + if (!pool) {
> + virStorageReportError(obj->conn, VIR_ERR_INVALID_STORAGE_POOL,
> + "no storage pool with matching uuid");
> + return -1;
+ }
+
> + if ((backend = virStorageBackendForType(pool->def->type)) ==
NULL) {
> + return -1;
+ }
+
> + if (virStoragePoolObjIsActive(pool)) {
> + virStorageReportError(obj->conn, VIR_ERR_INTERNAL_ERROR,
> + "storage pool is still active");
> + return -1;
+ }
+
> + if (backend->deletePool &&
> + backend->deletePool(obj->conn, pool, flags) < 0)
> + return -1;
> +
> + return 0;
> +}
So you return 0 is the backend has no deletePool defined, looks a bit strange
+
+static int storagePoolRefresh(virStoragePoolPtr obj,
+ unsigned int flags ATTRIBUTE_UNUSED) {
we assume backend->refreshPool is always there, maybe a test is in
order.
+ if ((ret = backend->refreshPool(obj->conn, pool)) < 0)
{
+ if (backend->stopPool)
+ backend->stopPool(obj->conn, pool);
okay, huge, but looks fine.
One thing I realize is that it adds a lot of new strings for localization
so it's better to push early if we want to get decent translations.
Daniel
--
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard | virtualization library http://libvirt.org/
veillard(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/