[libvirt] [PATCH 0/5] storage_conf: cleanups
*** BLURB HERE *** Osier Yang (5): storage_conf: Fix the coding stype in storage_conf.c storage_conf: Fix indentions in storage_conf.c storage_conf: Various fixes or improvements on pool def parsing storage_conf: Left fixes or improvements for storage_conf.c storage_conf: Improve the coding style in storage_conf.h src/conf/storage_conf.c | 611 +++++++++++++++++++++++++----------------------- src/conf/storage_conf.h | 120 +++++----- 2 files changed, 381 insertions(+), 350 deletions(-) -- 1.8.1.4
Changes: * Remove the useless space in "for" statement (e.g. for (i = 0 ; i < something ; i++) * Change the function's style to: void foo(bar) { printf("foo is not bar\n"); } * Don't lose "{}" for "if...else" branches if one of the branch has more than one line block. Example of the old ones: if (a) { printf("a is not funny"); } else printf("a is funny"); * Remove the 1 space before "goto" label. * Remove the useless blank line(s) * Add blank line if it can make the code more clear to eyes. --- src/conf/storage_conf.c | 175 ++++++++++++++++++++++++++++-------------------- 1 file changed, 104 insertions(+), 71 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index e21c39a..643c3cc 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -130,7 +130,6 @@ struct _virStoragePoolOptions { typedef struct _virStoragePoolTypeInfo virStoragePoolTypeInfo; typedef virStoragePoolTypeInfo *virStoragePoolTypeInfoPtr; - struct _virStoragePoolTypeInfo { int poolType; virStoragePoolOptions poolOptions; @@ -252,9 +251,10 @@ static virStoragePoolTypeInfo poolTypeInfo[] = { static virStoragePoolTypeInfoPtr -virStoragePoolTypeInfoLookup(int type) { +virStoragePoolTypeInfoLookup(int type) +{ unsigned int i; - for (i = 0; i < ARRAY_CARDINALITY(poolTypeInfo) ; i++) + for (i = 0; i < ARRAY_CARDINALITY(poolTypeInfo); i++) if (poolTypeInfo[i].poolType == type) return &poolTypeInfo[i]; @@ -264,7 +264,8 @@ virStoragePoolTypeInfoLookup(int type) { } static virStoragePoolOptionsPtr -virStoragePoolOptionsForPoolType(int type) { +virStoragePoolOptionsForPoolType(int type) +{ virStoragePoolTypeInfoPtr backend = virStoragePoolTypeInfoLookup(type); if (backend == NULL) return NULL; @@ -272,7 +273,8 @@ virStoragePoolOptionsForPoolType(int type) { } static virStorageVolOptionsPtr -virStorageVolOptionsForPoolType(int type) { +virStorageVolOptionsForPoolType(int type) +{ virStoragePoolTypeInfoPtr backend = virStoragePoolTypeInfoLookup(type); if (backend == NULL) return NULL; @@ -281,7 +283,8 @@ virStorageVolOptionsForPoolType(int type) { void -virStorageVolDefFree(virStorageVolDefPtr def) { +virStorageVolDefFree(virStorageVolDefPtr def) +{ int i; if (!def) @@ -290,7 +293,7 @@ virStorageVolDefFree(virStorageVolDefPtr def) { VIR_FREE(def->name); VIR_FREE(def->key); - for (i = 0 ; i < def->source.nextent ; i++) { + for (i = 0; i < def->source.nextent; i++) { VIR_FREE(def->source.extents[i].path); } VIR_FREE(def->source.extents); @@ -327,12 +330,12 @@ virStoragePoolSourceClear(virStoragePoolSourcePtr source) if (!source) return; - for (i = 0 ; i < source->nhost ; i++) { + for (i = 0; i < source->nhost; i++) { VIR_FREE(source->hosts[i].name); } VIR_FREE(source->hosts); - for (i = 0 ; i < source->ndevice ; i++) { + for (i = 0; i < source->ndevice; i++) { VIR_FREE(source->devices[i].freeExtents); VIR_FREE(source->devices[i].path); } @@ -363,7 +366,8 @@ virStoragePoolSourceFree(virStoragePoolSourcePtr source) } void -virStoragePoolDefFree(virStoragePoolDefPtr def) { +virStoragePoolDefFree(virStoragePoolDefPtr def) +{ if (!def) return; @@ -378,7 +382,8 @@ virStoragePoolDefFree(virStoragePoolDefPtr def) { void -virStoragePoolObjFree(virStoragePoolObjPtr obj) { +virStoragePoolObjFree(virStoragePoolObjPtr obj) +{ if (!obj) return; @@ -395,7 +400,8 @@ virStoragePoolObjFree(virStoragePoolObjPtr obj) { VIR_FREE(obj); } -void virStoragePoolObjListFree(virStoragePoolObjListPtr pools) +void +virStoragePoolObjListFree(virStoragePoolObjListPtr pools) { unsigned int i; for (i = 0 ; i < pools->count ; i++) @@ -412,7 +418,7 @@ virStoragePoolObjRemove(virStoragePoolObjListPtr pools, virStoragePoolObjUnlock(pool); - for (i = 0 ; i < pools->count ; i++) { + for (i = 0; i < pools->count; i++) { virStoragePoolObjLock(pools->objs[i]); if (pools->objs[i] == pool) { virStoragePoolObjUnlock(pools->objs[i]); @@ -436,7 +442,8 @@ virStoragePoolObjRemove(virStoragePoolObjListPtr pools, static int virStoragePoolDefParseAuthChap(xmlXPathContextPtr ctxt, - virStoragePoolAuthChapPtr auth) { + virStoragePoolAuthChapPtr auth) +{ auth->login = virXPathString("string(./auth/@login)", ctxt); if (auth->login == NULL) { virReportError(VIR_ERR_XML_ERROR, @@ -456,7 +463,8 @@ virStoragePoolDefParseAuthChap(xmlXPathContextPtr ctxt, static int virStoragePoolDefParseAuthCephx(xmlXPathContextPtr ctxt, - virStoragePoolAuthCephxPtr auth) { + virStoragePoolAuthCephxPtr auth) +{ char *uuid = NULL; auth->username = virXPathString("string(./auth/@username)", ctxt); if (auth->username == NULL) { @@ -496,7 +504,8 @@ static int virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, virStoragePoolSourcePtr source, int pool_type, - xmlNodePtr node) { + xmlNodePtr node) +{ int ret = -1; xmlNodePtr relnode, *nodeset = NULL; char *authType = NULL; @@ -547,7 +556,7 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, goto cleanup; } - for (i = 0 ; i < source->nhost ; i++) { + for (i = 0; i < source->nhost; i++) { name = virXMLPropString(nodeset[i], "name"); if (name == NULL) { virReportError(VIR_ERR_XML_ERROR, @@ -725,11 +734,13 @@ cleanup: return ret; } + static int virStorageDefParsePerms(xmlXPathContextPtr ctxt, virStoragePermsPtr perms, const char *permxpath, - int defaultmode) { + int defaultmode) +{ char *mode; long v; int ret = -1; @@ -797,7 +808,8 @@ error: } static virStoragePoolDefPtr -virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) { +virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) +{ virStoragePoolOptionsPtr options; virStoragePoolDefPtr ret; xmlNodePtr source_node; @@ -944,7 +956,7 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) { return ret; - cleanup: +cleanup: VIR_FREE(uuid); xmlFree(type); virStoragePoolDefFree(ret); @@ -953,7 +965,8 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) { virStoragePoolDefPtr virStoragePoolDefParseNode(xmlDocPtr xml, - xmlNodePtr root) { + xmlNodePtr root) +{ xmlXPathContextPtr ctxt = NULL; virStoragePoolDefPtr def = NULL; @@ -978,7 +991,8 @@ cleanup: static virStoragePoolDefPtr virStoragePoolDefParse(const char *xmlStr, - const char *filename) { + const char *filename) +{ virStoragePoolDefPtr ret = NULL; xmlDocPtr xml; @@ -1022,25 +1036,27 @@ virStoragePoolSourceFormat(virBufferPtr buf, if ((options->flags & VIR_STORAGE_POOL_SOURCE_DEVICE) && src->ndevice) { - for (i = 0 ; i < src->ndevice ; i++) { + for (i = 0; i < src->ndevice; i++) { if (src->devices[i].nfreeExtent) { virBufferAsprintf(buf," <device path='%s'>\n", src->devices[i].path); - for (j = 0 ; j < src->devices[i].nfreeExtent ; j++) { + for (j = 0; j < src->devices[i].nfreeExtent; j++) { virBufferAsprintf(buf, " <freeExtent start='%llu' end='%llu'/>\n", src->devices[i].freeExtents[j].start, src->devices[i].freeExtents[j].end); } virBufferAddLit(buf," </device>\n"); - } - else + } else { virBufferAsprintf(buf, " <device path='%s'/>\n", src->devices[i].path); + } } } + if ((options->flags & VIR_STORAGE_POOL_SOURCE_DIR) && src->dir) virBufferAsprintf(buf," <dir path='%s'/>\n", src->dir); + if ((options->flags & VIR_STORAGE_POOL_SOURCE_ADAPTER)) { if (src->adapter.type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST || src->adapter.type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) @@ -1058,6 +1074,7 @@ virStoragePoolSourceFormat(virBufferPtr buf, virBufferAsprintf(buf," name='%s'/>\n", src->adapter.data.name); } } + if ((options->flags & VIR_STORAGE_POOL_SOURCE_NAME) && src->name) virBufferAsprintf(buf," <name>%s</name>\n", src->name); @@ -1080,7 +1097,6 @@ virStoragePoolSourceFormat(virBufferPtr buf, virBufferAsprintf(buf," <format type='%s'/>\n", format); } - if (src->authType == VIR_STORAGE_POOL_AUTH_CHAP) virBufferAsprintf(buf," <auth type='chap' login='%s' passwd='%s'/>\n", src->auth.chap.login, @@ -1119,7 +1135,8 @@ virStoragePoolSourceFormat(virBufferPtr buf, char * -virStoragePoolDefFormat(virStoragePoolDefPtr def) { +virStoragePoolDefFormat(virStoragePoolDefPtr def) +{ virStoragePoolOptionsPtr options; virBuffer buf = VIR_BUFFER_INITIALIZER; const char *type; @@ -1151,9 +1168,10 @@ virStoragePoolDefFormat(virStoragePoolDefPtr def) { if (virStoragePoolSourceFormat(&buf, options, &def->source) < 0) goto cleanup; - /* RBD and Sheepdog devices are not local block devs nor files, so it doesn't - * have a target */ - if (def->type != VIR_STORAGE_POOL_RBD && def->type != VIR_STORAGE_POOL_SHEEPDOG) { + /* RBD and Sheepdog devices are not local block devs nor files, so it + * doesn't have a target */ + if (def->type != VIR_STORAGE_POOL_RBD && + def->type != VIR_STORAGE_POOL_SHEEPDOG) { virBufferAddLit(&buf," <target>\n"); if (def->target.path) @@ -1181,9 +1199,9 @@ virStoragePoolDefFormat(virStoragePoolDefPtr def) { return virBufferContentAndReset(&buf); - no_memory: +no_memory: virReportOOMError(); - cleanup: +cleanup: virBufferFreeAndReset(&buf); return NULL; } @@ -1209,7 +1227,8 @@ virStorageSize(const char *unit, static virStorageVolDefPtr virStorageVolDefParseXML(virStoragePoolDefPtr pool, - xmlXPathContextPtr ctxt) { + xmlXPathContextPtr ctxt) +{ virStorageVolDefPtr ret; virStorageVolOptionsPtr options; char *allocation = NULL; @@ -1234,7 +1253,7 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, } /* Auto-generated so deliberately ignore */ - /*ret->key = virXPathString("string(./key)", ctxt);*/ + /* ret->key = virXPathString("string(./key)", ctxt); */ capacity = virXPathString("string(./capacity)", ctxt); unit = virXPathString("string(./capacity/@unit)", ctxt); @@ -1289,8 +1308,6 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, goto cleanup; } - - ret->backingStore.path = virXPathString("string(./backingStore/path)", ctxt); if (options->formatFromString) { char *format = virXPathString("string(./backingStore/format/@type)", ctxt); @@ -1315,7 +1332,7 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, return ret; - cleanup: +cleanup: VIR_FREE(allocation); VIR_FREE(capacity); VIR_FREE(unit); @@ -1326,7 +1343,8 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, virStorageVolDefPtr virStorageVolDefParseNode(virStoragePoolDefPtr pool, xmlDocPtr xml, - xmlNodePtr root) { + xmlNodePtr root) +{ xmlXPathContextPtr ctxt = NULL; virStorageVolDefPtr def = NULL; @@ -1352,7 +1370,8 @@ cleanup: static virStorageVolDefPtr virStorageVolDefParse(virStoragePoolDefPtr pool, const char *xmlStr, - const char *filename) { + const char *filename) +{ virStorageVolDefPtr ret = NULL; xmlDocPtr xml; @@ -1450,7 +1469,8 @@ virStorageVolTargetDefFormat(virStorageVolOptionsPtr options, char * virStorageVolDefFormat(virStoragePoolDefPtr pool, - virStorageVolDefPtr def) { + virStorageVolDefPtr def) +{ virStorageVolOptionsPtr options; virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -1466,7 +1486,7 @@ virStorageVolDefFormat(virStoragePoolDefPtr pool, if (def->source.nextent) { int i; const char *thispath = NULL; - for (i = 0 ; i < def->source.nextent ; i++) { + for (i = 0; i < def->source.nextent; i++) { if (thispath == NULL || STRNEQ(thispath, def->source.extents[i].path)) { if (thispath != NULL) @@ -1508,9 +1528,9 @@ virStorageVolDefFormat(virStoragePoolDefPtr pool, return virBufferContentAndReset(&buf); - no_memory: +no_memory: virReportOOMError(); - cleanup: +cleanup: virBufferFreeAndReset(&buf); return NULL; } @@ -1518,10 +1538,11 @@ virStorageVolDefFormat(virStoragePoolDefPtr pool, virStoragePoolObjPtr virStoragePoolObjFindByUUID(virStoragePoolObjListPtr pools, - const unsigned char *uuid) { + const unsigned char *uuid) +{ unsigned int i; - for (i = 0 ; i < pools->count ; i++) { + for (i = 0; i < pools->count; i++) { virStoragePoolObjLock(pools->objs[i]); if (!memcmp(pools->objs[i]->def->uuid, uuid, VIR_UUID_BUFLEN)) return pools->objs[i]; @@ -1533,7 +1554,8 @@ virStoragePoolObjFindByUUID(virStoragePoolObjListPtr pools, virStoragePoolObjPtr virStoragePoolObjFindByName(virStoragePoolObjListPtr pools, - const char *name) { + const char *name) +{ unsigned int i; for (i = 0 ; i < pools->count ; i++) { @@ -1548,7 +1570,8 @@ virStoragePoolObjFindByName(virStoragePoolObjListPtr pools, virStoragePoolObjPtr virStoragePoolSourceFindDuplicateDevices(virStoragePoolObjPtr pool, - virStoragePoolDefPtr def) { + virStoragePoolDefPtr def) +{ unsigned int i, j; for (i = 0; i < pool->def->source.ndevice; i++) { @@ -1565,7 +1588,7 @@ void virStoragePoolObjClearVols(virStoragePoolObjPtr pool) { unsigned int i; - for (i = 0 ; i < pool->volumes.count ; i++) + for (i = 0; i < pool->volumes.count; i++) virStorageVolDefFree(pool->volumes.objs[i]); VIR_FREE(pool->volumes.objs); @@ -1574,10 +1597,11 @@ virStoragePoolObjClearVols(virStoragePoolObjPtr pool) virStorageVolDefPtr virStorageVolDefFindByKey(virStoragePoolObjPtr pool, - const char *key) { + const char *key) +{ unsigned int i; - for (i = 0 ; i < pool->volumes.count ; i++) + for (i = 0; i < pool->volumes.count; i++) if (STREQ(pool->volumes.objs[i]->key, key)) return pool->volumes.objs[i]; @@ -1586,7 +1610,8 @@ virStorageVolDefFindByKey(virStoragePoolObjPtr pool, virStorageVolDefPtr virStorageVolDefFindByPath(virStoragePoolObjPtr pool, - const char *path) { + const char *path) +{ unsigned int i; for (i = 0 ; i < pool->volumes.count ; i++) @@ -1598,10 +1623,11 @@ virStorageVolDefFindByPath(virStoragePoolObjPtr pool, virStorageVolDefPtr virStorageVolDefFindByName(virStoragePoolObjPtr pool, - const char *name) { + const char *name) +{ unsigned int i; - for (i = 0 ; i < pool->volumes.count ; i++) + for (i = 0; i < pool->volumes.count; i++) if (STREQ(pool->volumes.objs[i]->name, name)) return pool->volumes.objs[i]; @@ -1610,7 +1636,8 @@ virStorageVolDefFindByName(virStoragePoolObjPtr pool, virStoragePoolObjPtr virStoragePoolObjAssignDef(virStoragePoolObjListPtr pools, - virStoragePoolDefPtr def) { + virStoragePoolDefPtr def) +{ virStoragePoolObjPtr pool; if ((pool = virStoragePoolObjFindByName(pools, def->name))) { @@ -1655,7 +1682,8 @@ static virStoragePoolObjPtr virStoragePoolObjLoad(virStoragePoolObjListPtr pools, const char *file, const char *path, - const char *autostartLink) { + const char *autostartLink) +{ virStoragePoolDefPtr def; virStoragePoolObjPtr pool; @@ -1701,7 +1729,8 @@ virStoragePoolObjLoad(virStoragePoolObjListPtr pools, int virStoragePoolLoadAllConfigs(virStoragePoolObjListPtr pools, const char *configDir, - const char *autostartDir) { + const char *autostartDir) +{ DIR *dir; struct dirent *entry; @@ -1792,7 +1821,8 @@ virStoragePoolObjSaveDef(virStorageDriverStatePtr driver, } int -virStoragePoolObjDeleteDef(virStoragePoolObjPtr pool) { +virStoragePoolObjDeleteDef(virStoragePoolObjPtr pool) +{ if (!pool->configFile) { virReportError(VIR_ERR_INTERNAL_ERROR, _("no config file for %s"), pool->def->name); @@ -1814,7 +1844,7 @@ virStoragePoolSourceListNewSource(virStoragePoolSourceListPtr list) { virStoragePoolSourcePtr source; - if (VIR_REALLOC_N(list->sources, list->nsources+1) < 0) { + if (VIR_REALLOC_N(list->sources, list->nsources + 1) < 0) { virReportOOMError(); return NULL; } @@ -1825,7 +1855,8 @@ virStoragePoolSourceListNewSource(virStoragePoolSourceListPtr list) return source; } -char *virStoragePoolSourceListFormat(virStoragePoolSourceListPtr def) +char * +virStoragePoolSourceListFormat(virStoragePoolSourceListPtr def) { virStoragePoolOptionsPtr options; virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -1856,9 +1887,9 @@ char *virStoragePoolSourceListFormat(virStoragePoolSourceListPtr def) return virBufferContentAndReset(&buf); - no_memory: +no_memory: virReportOOMError(); - cleanup: +cleanup: virBufferFreeAndReset(&buf); return NULL; } @@ -1874,9 +1905,10 @@ char *virStoragePoolSourceListFormat(virStoragePoolSourceListPtr def) * 0 if pool is new * 1 if pool is a duplicate */ -int virStoragePoolObjIsDuplicate(virStoragePoolObjListPtr pools, - virStoragePoolDefPtr def, - unsigned int check_active) +int +virStoragePoolObjIsDuplicate(virStoragePoolObjListPtr pools, + virStoragePoolDefPtr def, + unsigned int check_active) { int ret = -1; int dupPool = 0; @@ -1926,8 +1958,9 @@ cleanup: return ret; } -int virStoragePoolSourceFindDuplicate(virStoragePoolObjListPtr pools, - virStoragePoolDefPtr def) +int +virStoragePoolSourceFindDuplicate(virStoragePoolObjListPtr pools, + virStoragePoolDefPtr def) { int i; int ret = 1; @@ -1973,7 +2006,6 @@ int virStoragePoolSourceFindDuplicate(virStoragePoolObjListPtr pools, } break; case VIR_STORAGE_POOL_ISCSI: - { matchpool = virStoragePoolSourceFindDuplicateDevices(pool, def); if (matchpool) { if (matchpool->def->source.nhost == 1 && def->source.nhost == 1) { @@ -1989,7 +2021,6 @@ int virStoragePoolSourceFindDuplicate(virStoragePoolObjListPtr pools, matchpool = NULL; } break; - } case VIR_STORAGE_POOL_FS: case VIR_STORAGE_POOL_LOGICAL: case VIR_STORAGE_POOL_DISK: @@ -2013,12 +2044,14 @@ int virStoragePoolSourceFindDuplicate(virStoragePoolObjListPtr pools, return ret; } -void virStoragePoolObjLock(virStoragePoolObjPtr obj) +void +virStoragePoolObjLock(virStoragePoolObjPtr obj) { virMutexLock(&obj->lock); } -void virStoragePoolObjUnlock(virStoragePoolObjPtr obj) +void +virStoragePoolObjUnlock(virStoragePoolObjPtr obj) { virMutexUnlock(&obj->lock); } -- 1.8.1.4
On Thu, May 16, 2013 at 08:40:50PM +0800, Osier Yang wrote:
Changes: * Remove the useless space in "for" statement (e.g. for (i = 0 ; i < something ; i++)
It would be pretty easy to extend build-aux/bracket-spacing.pl to validate correct whitespace around the ';' in for loops.
* Change the function's style to: void foo(bar) { printf("foo is not bar\n"); }
* Don't lose "{}" for "if...else" branches if one of the branch has more than one line block. Example of the old ones: if (a) { printf("a is not funny"); } else printf("a is funny");
* Remove the 1 space before "goto" label.
* Remove the useless blank line(s)
* Add blank line if it can make the code more clear to eyes. --- src/conf/storage_conf.c | 175 ++++++++++++++++++++++++++++-------------------- 1 file changed, 104 insertions(+), 71 deletions(-)
ACK Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 20/05/13 19:26, Daniel P. Berrange wrote:
On Thu, May 16, 2013 at 08:40:50PM +0800, Osier Yang wrote:
Changes: * Remove the useless space in "for" statement (e.g. for (i = 0 ; i < something ; i++) It would be pretty easy to extend build-aux/bracket-spacing.pl to validate correct whitespace around the ';' in for loops.
I will add the validation in another patch. Pushed this. Osier
Uses the 4 spaces for indention. --- src/conf/storage_conf.c | 176 ++++++++++++++++++++++++------------------------ 1 file changed, 88 insertions(+), 88 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 643c3cc..8fa805b 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -146,106 +146,106 @@ virStorageVolumeFormatFromString(const char *format) } static virStoragePoolTypeInfo poolTypeInfo[] = { - { .poolType = VIR_STORAGE_POOL_LOGICAL, - .poolOptions = { - .flags = (VIR_STORAGE_POOL_SOURCE_NAME | - VIR_STORAGE_POOL_SOURCE_DEVICE), - .defaultFormat = VIR_STORAGE_POOL_LOGICAL_LVM2, - .formatFromString = virStoragePoolFormatLogicalTypeFromString, - .formatToString = virStoragePoolFormatLogicalTypeToString, - }, + {.poolType = VIR_STORAGE_POOL_LOGICAL, + .poolOptions = { + .flags = (VIR_STORAGE_POOL_SOURCE_NAME | + VIR_STORAGE_POOL_SOURCE_DEVICE), + .defaultFormat = VIR_STORAGE_POOL_LOGICAL_LVM2, + .formatFromString = virStoragePoolFormatLogicalTypeFromString, + .formatToString = virStoragePoolFormatLogicalTypeToString, + }, }, - { .poolType = VIR_STORAGE_POOL_DIR, - .volOptions = { - .defaultFormat = VIR_STORAGE_FILE_RAW, - .formatFromString = virStorageVolumeFormatFromString, - .formatToString = virStorageFileFormatTypeToString, - }, + {.poolType = VIR_STORAGE_POOL_DIR, + .volOptions = { + .defaultFormat = VIR_STORAGE_FILE_RAW, + .formatFromString = virStorageVolumeFormatFromString, + .formatToString = virStorageFileFormatTypeToString, + }, }, - { .poolType = VIR_STORAGE_POOL_FS, - .poolOptions = { - .flags = (VIR_STORAGE_POOL_SOURCE_DEVICE), - .defaultFormat = VIR_STORAGE_POOL_FS_AUTO, - .formatFromString = virStoragePoolFormatFileSystemTypeFromString, - .formatToString = virStoragePoolFormatFileSystemTypeToString, - }, + {.poolType = VIR_STORAGE_POOL_FS, + .poolOptions = { + .flags = (VIR_STORAGE_POOL_SOURCE_DEVICE), + .defaultFormat = VIR_STORAGE_POOL_FS_AUTO, + .formatFromString = virStoragePoolFormatFileSystemTypeFromString, + .formatToString = virStoragePoolFormatFileSystemTypeToString, + }, .volOptions = { - .defaultFormat = VIR_STORAGE_FILE_RAW, - .formatFromString = virStorageVolumeFormatFromString, - .formatToString = virStorageFileFormatTypeToString, - }, + .defaultFormat = VIR_STORAGE_FILE_RAW, + .formatFromString = virStorageVolumeFormatFromString, + .formatToString = virStorageFileFormatTypeToString, + }, }, - { .poolType = VIR_STORAGE_POOL_NETFS, - .poolOptions = { - .flags = (VIR_STORAGE_POOL_SOURCE_HOST | - VIR_STORAGE_POOL_SOURCE_DIR), - .defaultFormat = VIR_STORAGE_POOL_NETFS_AUTO, - .formatFromString = virStoragePoolFormatFileSystemNetTypeFromString, - .formatToString = virStoragePoolFormatFileSystemNetTypeToString, - }, + {.poolType = VIR_STORAGE_POOL_NETFS, + .poolOptions = { + .flags = (VIR_STORAGE_POOL_SOURCE_HOST | + VIR_STORAGE_POOL_SOURCE_DIR), + .defaultFormat = VIR_STORAGE_POOL_NETFS_AUTO, + .formatFromString = virStoragePoolFormatFileSystemNetTypeFromString, + .formatToString = virStoragePoolFormatFileSystemNetTypeToString, + }, .volOptions = { - .defaultFormat = VIR_STORAGE_FILE_RAW, - .formatFromString = virStorageVolumeFormatFromString, - .formatToString = virStorageFileFormatTypeToString, - }, + .defaultFormat = VIR_STORAGE_FILE_RAW, + .formatFromString = virStorageVolumeFormatFromString, + .formatToString = virStorageFileFormatTypeToString, + }, }, - { .poolType = VIR_STORAGE_POOL_ISCSI, - .poolOptions = { - .flags = (VIR_STORAGE_POOL_SOURCE_HOST | - VIR_STORAGE_POOL_SOURCE_DEVICE | - VIR_STORAGE_POOL_SOURCE_INITIATOR_IQN), - }, + {.poolType = VIR_STORAGE_POOL_ISCSI, + .poolOptions = { + .flags = (VIR_STORAGE_POOL_SOURCE_HOST | + VIR_STORAGE_POOL_SOURCE_DEVICE | + VIR_STORAGE_POOL_SOURCE_INITIATOR_IQN), + }, .volOptions = { - .formatToString = virStoragePoolFormatDiskTypeToString, - } + .formatToString = virStoragePoolFormatDiskTypeToString, + } }, - { .poolType = VIR_STORAGE_POOL_SCSI, - .poolOptions = { - .flags = (VIR_STORAGE_POOL_SOURCE_ADAPTER), - }, - .volOptions = { - .formatToString = virStoragePoolFormatDiskTypeToString, - } + {.poolType = VIR_STORAGE_POOL_SCSI, + .poolOptions = { + .flags = (VIR_STORAGE_POOL_SOURCE_ADAPTER), + }, + .volOptions = { + .formatToString = virStoragePoolFormatDiskTypeToString, + } }, - { .poolType = VIR_STORAGE_POOL_RBD, - .poolOptions = { - .flags = (VIR_STORAGE_POOL_SOURCE_HOST | - VIR_STORAGE_POOL_SOURCE_NETWORK | - VIR_STORAGE_POOL_SOURCE_NAME), - }, - .volOptions = { - .defaultFormat = VIR_STORAGE_FILE_RAW, - .formatToString = virStoragePoolFormatDiskTypeToString, - } + {.poolType = VIR_STORAGE_POOL_RBD, + .poolOptions = { + .flags = (VIR_STORAGE_POOL_SOURCE_HOST | + VIR_STORAGE_POOL_SOURCE_NETWORK | + VIR_STORAGE_POOL_SOURCE_NAME), + }, + .volOptions = { + .defaultFormat = VIR_STORAGE_FILE_RAW, + .formatToString = virStoragePoolFormatDiskTypeToString, + } }, - { .poolType = VIR_STORAGE_POOL_SHEEPDOG, - .poolOptions = { - .flags = (VIR_STORAGE_POOL_SOURCE_HOST | - VIR_STORAGE_POOL_SOURCE_NETWORK | - VIR_STORAGE_POOL_SOURCE_NAME), - }, - .volOptions = { - .defaultFormat = VIR_STORAGE_FILE_RAW, - .formatToString = virStoragePoolFormatDiskTypeToString, - } + {.poolType = VIR_STORAGE_POOL_SHEEPDOG, + .poolOptions = { + .flags = (VIR_STORAGE_POOL_SOURCE_HOST | + VIR_STORAGE_POOL_SOURCE_NETWORK | + VIR_STORAGE_POOL_SOURCE_NAME), + }, + .volOptions = { + .defaultFormat = VIR_STORAGE_FILE_RAW, + .formatToString = virStoragePoolFormatDiskTypeToString, + } }, - { .poolType = VIR_STORAGE_POOL_MPATH, - .volOptions = { - .formatToString = virStoragePoolFormatDiskTypeToString, - } + {.poolType = VIR_STORAGE_POOL_MPATH, + .volOptions = { + .formatToString = virStoragePoolFormatDiskTypeToString, + } }, - { .poolType = VIR_STORAGE_POOL_DISK, - .poolOptions = { - .flags = (VIR_STORAGE_POOL_SOURCE_DEVICE), - .defaultFormat = VIR_STORAGE_POOL_DISK_UNKNOWN, - .formatFromString = virStoragePoolFormatDiskTypeFromString, - .formatToString = virStoragePoolFormatDiskTypeToString, - }, - .volOptions = { - .defaultFormat = VIR_STORAGE_VOL_DISK_NONE, - .formatFromString = virStorageVolFormatDiskTypeFromString, - .formatToString = virStorageVolFormatDiskTypeToString, - }, + {.poolType = VIR_STORAGE_POOL_DISK, + .poolOptions = { + .flags = (VIR_STORAGE_POOL_SOURCE_DEVICE), + .defaultFormat = VIR_STORAGE_POOL_DISK_UNKNOWN, + .formatFromString = virStoragePoolFormatDiskTypeFromString, + .formatToString = virStoragePoolFormatDiskTypeToString, + }, + .volOptions = { + .defaultFormat = VIR_STORAGE_VOL_DISK_NONE, + .formatFromString = virStorageVolFormatDiskTypeFromString, + .formatToString = virStorageVolFormatDiskTypeToString, + }, } }; -- 1.8.1.4
On Thu, May 16, 2013 at 08:40:51PM +0800, Osier Yang wrote:
Uses the 4 spaces for indention. --- src/conf/storage_conf.c | 176 ++++++++++++++++++++++++------------------------ 1 file changed, 88 insertions(+), 88 deletions(-)
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 20/05/13 19:27, Daniel P. Berrange wrote:
On Thu, May 16, 2013 at 08:40:51PM +0800, Osier Yang wrote:
Uses the 4 spaces for indention. --- src/conf/storage_conf.c | 176 ++++++++++++++++++++++++------------------------ 1 file changed, 88 insertions(+), 88 deletions(-) ACK
Pushed.
virStorageDefParsePerms: * Use uid_t/gid_t to do casting virStoragePoolDefParseSource: * Improve the error message for source "name" parsing * Remove the useless casting (const char *) virStoragePoolDefParseAuthChap: * Fix the wrong error message virStoragePoolDefParseAuthCephx: * Don't leak "uuid" virStoragePoolDefParseXML: * Remove the useless casting "(const char *)" * Use VIR_ERR_XML_ERROR for pool 'type' parsing * Remove the xmlFree() * s/tmppath/path/, * Create "virStoragePoolDefPtr def", and use ret to track the return value; frees the strings at "cleanup" label instead if freeing them in the middle. * Other small changes. --- src/conf/storage_conf.c | 169 ++++++++++++++++++++++++------------------------ 1 file changed, 85 insertions(+), 84 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 8fa805b..dd55d2c 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -446,15 +446,15 @@ virStoragePoolDefParseAuthChap(xmlXPathContextPtr ctxt, { auth->login = virXPathString("string(./auth/@login)", ctxt); if (auth->login == NULL) { - virReportError(VIR_ERR_XML_ERROR, - "%s", _("missing auth host attribute")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing auth login attribute")); return -1; } auth->passwd = virXPathString("string(./auth/@passwd)", ctxt); if (auth->passwd == NULL) { - virReportError(VIR_ERR_XML_ERROR, - "%s", _("missing auth passwd attribute")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing auth passwd attribute")); return -1; } @@ -466,10 +466,12 @@ virStoragePoolDefParseAuthCephx(xmlXPathContextPtr ctxt, virStoragePoolAuthCephxPtr auth) { char *uuid = NULL; + int ret = -1; + auth->username = virXPathString("string(./auth/@username)", ctxt); if (auth->username == NULL) { - virReportError(VIR_ERR_XML_ERROR, - "%s", _("missing auth username attribute")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing auth username attribute")); return -1; } @@ -485,19 +487,22 @@ virStoragePoolDefParseAuthCephx(xmlXPathContextPtr ctxt, if (auth->secret.usage != NULL) { virReportError(VIR_ERR_XML_ERROR, "%s", _("either auth secret uuid or usage expected")); - return -1; + goto cleanup; } if (virUUIDParse(uuid, auth->secret.uuid) < 0) { - virReportError(VIR_ERR_XML_ERROR, - "%s", _("invalid auth secret uuid")); - return -1; + virReportError(VIR_ERR_XML_ERROR, "%s", + _("invalid auth secret uuid")); + goto cleanup; } auth->secret.uuidUsable = true; } else { auth->secret.uuidUsable = false; } - return 0; + ret = 0; +cleanup: + VIR_FREE(uuid); + return ret; } static int @@ -526,7 +531,7 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, source->name = virXPathString("string(./name)", ctxt); if (pool_type == VIR_STORAGE_POOL_RBD && source->name == NULL) { virReportError(VIR_ERR_XML_ERROR, "%s", - _("missing mandatory 'name' field for RBD pool name")); + _("element 'name' is mandatory for RBD pool name")); goto cleanup; } @@ -559,8 +564,8 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, for (i = 0; i < source->nhost; i++) { name = virXMLPropString(nodeset[i], "name"); if (name == NULL) { - virReportError(VIR_ERR_XML_ERROR, - "%s", _("missing storage pool host name")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing storage pool host name")); goto cleanup; } source->hosts[i].name = name; @@ -595,8 +600,8 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, char *path = virXMLPropString(nodeset[i], "path"); if (path == NULL) { VIR_FREE(nodeset); - virReportError(VIR_ERR_XML_ERROR, - "%s", _("missing storage pool source device path")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing storage pool source device path")); goto cleanup; } source->devices[i].path = path; @@ -667,7 +672,7 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, } else { virReportError(VIR_ERR_XML_ERROR, _("unknown auth type '%s'"), - (const char *)authType); + authType); goto cleanup; } } @@ -768,8 +773,8 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt, if (virStrToLong_i(mode, NULL, 8, &tmp) < 0 || (tmp & ~0777)) { VIR_FREE(mode); - virReportError(VIR_ERR_XML_ERROR, - "%s", _("malformed octal mode")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("malformed octal mode")); goto error; } perms->mode = tmp; @@ -780,22 +785,22 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt, perms->uid = (uid_t) -1; } else { if (virXPathLong("number(./owner)", ctxt, &v) < 0) { - virReportError(VIR_ERR_XML_ERROR, - "%s", _("malformed owner element")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("malformed owner element")); goto error; } - perms->uid = (int)v; + perms->uid = (uid_t)v; } if (virXPathNode("./group", ctxt) == NULL) { perms->gid = (gid_t) -1; } else { if (virXPathLong("number(./group)", ctxt, &v) < 0) { - virReportError(VIR_ERR_XML_ERROR, - "%s", _("malformed group element")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("malformed group element")); goto error; } - perms->gid = (int)v; + perms->gid = (gid_t)v; } /* NB, we're ignoring missing labels here - they'll simply inherit */ @@ -811,85 +816,81 @@ static virStoragePoolDefPtr virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) { virStoragePoolOptionsPtr options; - virStoragePoolDefPtr ret; + virStoragePoolDefPtr ret = NULL; + virStoragePoolDefPtr def; xmlNodePtr source_node; char *type = NULL; char *uuid = NULL; - char *tmppath; + char *target_path = NULL; - if (VIR_ALLOC(ret) < 0) { + if (VIR_ALLOC(def) < 0) { virReportOOMError(); return NULL; } type = virXPathString("string(./@type)", ctxt); - if ((ret->type = virStoragePoolTypeFromString((const char *)type)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unknown storage pool type %s"), (const char*)type); + if ((def->type = virStoragePoolTypeFromString(type)) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("unknown storage pool type %s"), type); goto cleanup; } - xmlFree(type); - type = NULL; - - if ((options = virStoragePoolOptionsForPoolType(ret->type)) == NULL) { + if ((options = virStoragePoolOptionsForPoolType(def->type)) == NULL) goto cleanup; - } source_node = virXPathNode("./source", ctxt); if (source_node) { - if (virStoragePoolDefParseSource(ctxt, &ret->source, ret->type, + if (virStoragePoolDefParseSource(ctxt, &def->source, def->type, source_node) < 0) goto cleanup; } - ret->name = virXPathString("string(./name)", ctxt); - if (ret->name == NULL && + def->name = virXPathString("string(./name)", ctxt); + if (def->name == NULL && options->flags & VIR_STORAGE_POOL_SOURCE_NAME) - ret->name = ret->source.name; - if (ret->name == NULL) { - virReportError(VIR_ERR_XML_ERROR, - "%s", _("missing pool source name element")); + def->name = def->source.name; + if (def->name == NULL) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing pool source name element")); goto cleanup; } uuid = virXPathString("string(./uuid)", ctxt); if (uuid == NULL) { - if (virUUIDGenerate(ret->uuid) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("unable to generate uuid")); + if (virUUIDGenerate(def->uuid) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("unable to generate uuid")); goto cleanup; } } else { - if (virUUIDParse(uuid, ret->uuid) < 0) { - virReportError(VIR_ERR_XML_ERROR, - "%s", _("malformed uuid element")); + if (virUUIDParse(uuid, def->uuid) < 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("malformed uuid element")); goto cleanup; } - VIR_FREE(uuid); } if (options->flags & VIR_STORAGE_POOL_SOURCE_HOST) { - if (!ret->source.nhost) { - virReportError(VIR_ERR_XML_ERROR, - "%s", + if (!def->source.nhost) { + virReportError(VIR_ERR_XML_ERROR, "%s", _("missing storage pool source host name")); goto cleanup; } } if (options->flags & VIR_STORAGE_POOL_SOURCE_DIR) { - if (!ret->source.dir) { - virReportError(VIR_ERR_XML_ERROR, - "%s", _("missing storage pool source path")); + if (!def->source.dir) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing storage pool source path")); goto cleanup; } } + if (options->flags & VIR_STORAGE_POOL_SOURCE_NAME) { - if (ret->source.name == NULL) { + if (def->source.name == NULL) { /* source name defaults to pool name */ - ret->source.name = strdup(ret->name); - if (ret->source.name == NULL) { + def->source.name = strdup(def->name); + if (def->source.name == NULL) { virReportOOMError(); goto cleanup; } @@ -897,30 +898,30 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) } if (options->flags & VIR_STORAGE_POOL_SOURCE_ADAPTER) { - if (!ret->source.adapter.type) { + if (!def->source.adapter.type) { virReportError(VIR_ERR_XML_ERROR, "%s", _("missing storage pool source adapter")); goto cleanup; } - if (ret->source.adapter.type == + if (def->source.adapter.type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) { - if (!ret->source.adapter.data.fchost.wwnn || - !ret->source.adapter.data.fchost.wwpn) { + if (!def->source.adapter.data.fchost.wwnn || + !def->source.adapter.data.fchost.wwpn) { virReportError(VIR_ERR_XML_ERROR, "%s", _("'wwnn' and 'wwpn' must be specified for adapter " "type 'fchost'")); goto cleanup; } - if (!virValidateWWN(ret->source.adapter.data.fchost.wwnn) || - !virValidateWWN(ret->source.adapter.data.fchost.wwpn)) + if (!virValidateWWN(def->source.adapter.data.fchost.wwnn) || + !virValidateWWN(def->source.adapter.data.fchost.wwpn)) goto cleanup; - } else if (ret->source.adapter.type == + } else if (def->source.adapter.type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) { - if (!ret->source.adapter.data.name) { - virReportError(VIR_ERR_XML_ERROR, - "%s", _("missing storage pool source adapter name")); + if (!def->source.adapter.data.name) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing storage pool source adapter name")); goto cleanup; } } @@ -928,9 +929,9 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) /* If DEVICE is the only source type, then its required */ if (options->flags == VIR_STORAGE_POOL_SOURCE_DEVICE) { - if (!ret->source.ndevice) { - virReportError(VIR_ERR_XML_ERROR, - "%s", _("missing storage pool source device name")); + if (!def->source.ndevice) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing storage pool source device name")); goto cleanup; } } @@ -938,29 +939,29 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) /* When we are working with a virtual disk we can skip the target * path and permissions */ if (!(options->flags & VIR_STORAGE_POOL_SOURCE_NETWORK)) { - if ((tmppath = virXPathString("string(./target/path)", ctxt)) == NULL) { - virReportError(VIR_ERR_XML_ERROR, - "%s", _("missing storage pool target path")); + if (!(target_path = virXPathString("string(./target/path)", ctxt))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing storage pool target path")); goto cleanup; } - ret->target.path = virFileSanitizePath(tmppath); - VIR_FREE(tmppath); - if (!ret->target.path) + def->target.path = virFileSanitizePath(target_path); + if (!def->target.path) goto cleanup; - if (virStorageDefParsePerms(ctxt, &ret->target.perms, + if (virStorageDefParsePerms(ctxt, &def->target.perms, "./target/permissions", DEFAULT_POOL_PERM_MODE) < 0) goto cleanup; } - return ret; - + ret = def; cleanup: VIR_FREE(uuid); - xmlFree(type); - virStoragePoolDefFree(ret); - return NULL; + VIR_FREE(type); + VIR_FREE(target_path); + if (!ret) + virStoragePoolDefFree(ret); + return ret; } virStoragePoolDefPtr -- 1.8.1.4
On Thu, May 16, 2013 at 08:40:52PM +0800, Osier Yang wrote:
virStorageDefParsePerms: * Use uid_t/gid_t to do casting
Bug fix
virStoragePoolDefParseSource: * Improve the error message for source "name" parsing * Remove the useless casting (const char *)
virStoragePoolDefParseAuthChap: * Fix the wrong error message
Bug fix
virStoragePoolDefParseAuthCephx: * Don't leak "uuid"
Bug fix
virStoragePoolDefParseXML: * Remove the useless casting "(const char *)" * Use VIR_ERR_XML_ERROR for pool 'type' parsing * Remove the xmlFree() * s/tmppath/path/, * Create "virStoragePoolDefPtr def", and use ret to track the return value; frees the strings at "cleanup" label instead if freeing them in the middle. * Other small changes. --- src/conf/storage_conf.c | 169 ++++++++++++++++++++++++------------------------ 1 file changed, 85 insertions(+), 84 deletions(-)
You've got a bunch of bug fixes & style cleanups all mixed together here. We should have strict separation of bugs into individiual patch, separate from any style changes. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 05/16/2013 06:40 AM, Osier Yang wrote:
virStorageDefParsePerms: * Use uid_t/gid_t to do casting
@@ -780,22 +785,22 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt, perms->uid = (uid_t) -1; } else { if (virXPathLong("number(./owner)", ctxt, &v) < 0) { - virReportError(VIR_ERR_XML_ERROR, - "%s", _("malformed owner element")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("malformed owner element")); goto error; } - perms->uid = (int)v; + perms->uid = (uid_t)v;
As Daniel said, please separate uid_t/gid_t bug fixes into their own patch, as it is one of my trigger words for a patch needing extra careful review, and yet I had to hunt for where you were doing it. Still buggy, but now in a different way. Pre-patch, if you have a 64-bit long and 64-bit uid_t (is there such a platform? I don't know of one), you were silently throwing away the most significant bits. But you are not guaranteed that uid_t and long are the same size. On another platform where uid_t is 32-bit but long is 64-bit (and Linux on x86_64 is such a platform), then when I pass in 0x100000000, you should reject it as invalid, rather than silently truncating to 0. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
virStorageVolDefParseXML: * Create "virStorageVolDefPtr def", and use ret to track the return value; frees the strings at "cleanup" label instead of freeing them in the middle. virStorageVolDefFormat: * Use macro NULLSTR --- src/conf/storage_conf.c | 93 ++++++++++++++++++++++++------------------------- 1 file changed, 46 insertions(+), 47 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index dd55d2c..431a8eb 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -972,8 +972,8 @@ virStoragePoolDefParseNode(xmlDocPtr xml, virStoragePoolDefPtr def = NULL; if (STRNEQ((const char *)root->name, "pool")) { - virReportError(VIR_ERR_XML_ERROR, - "%s", _("unknown root element for storage pool")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("unknown root element for storage pool")); goto cleanup; } @@ -1149,8 +1149,8 @@ virStoragePoolDefFormat(virStoragePoolDefPtr def) type = virStoragePoolTypeToString(def->type); if (!type) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("unexpected pool type")); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("unexpected pool type")); goto cleanup; } virBufferAsprintf(&buf, "<pool type='%s'>\n", type); @@ -1214,8 +1214,8 @@ virStorageSize(const char *unit, unsigned long long *ret) { if (virStrToLong_ull(val, NULL, 10, ret) < 0) { - virReportError(VIR_ERR_XML_ERROR, - "%s", _("malformed capacity element")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("malformed capacity element")); return -1; } /* off_t is signed, so you cannot create a file larger than 2**63 @@ -1230,64 +1230,62 @@ static virStorageVolDefPtr virStorageVolDefParseXML(virStoragePoolDefPtr pool, xmlXPathContextPtr ctxt) { - virStorageVolDefPtr ret; + virStorageVolDefPtr def; virStorageVolOptionsPtr options; char *allocation = NULL; char *capacity = NULL; char *unit = NULL; xmlNodePtr node; + virStorageVolDefPtr ret = NULL; options = virStorageVolOptionsForPoolType(pool->type); if (options == NULL) return NULL; - if (VIR_ALLOC(ret) < 0) { + if (VIR_ALLOC(def) < 0) { virReportOOMError(); return NULL; } - ret->name = virXPathString("string(./name)", ctxt); - if (ret->name == NULL) { - virReportError(VIR_ERR_XML_ERROR, - "%s", _("missing volume name element")); + def->name = virXPathString("string(./name)", ctxt); + if (def->name == NULL) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing volume name element")); goto cleanup; } /* Auto-generated so deliberately ignore */ - /* ret->key = virXPathString("string(./key)", ctxt); */ + /* def->key = virXPathString("string(./key)", ctxt); */ capacity = virXPathString("string(./capacity)", ctxt); unit = virXPathString("string(./capacity/@unit)", ctxt); if (capacity == NULL) { - virReportError(VIR_ERR_XML_ERROR, - "%s", _("missing capacity element")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing capacity element")); goto cleanup; } - if (virStorageSize(unit, capacity, &ret->capacity) < 0) + if (virStorageSize(unit, capacity, &def->capacity) < 0) goto cleanup; - VIR_FREE(capacity); VIR_FREE(unit); allocation = virXPathString("string(./allocation)", ctxt); if (allocation) { unit = virXPathString("string(./allocation/@unit)", ctxt); - if (virStorageSize(unit, allocation, &ret->allocation) < 0) + if (virStorageSize(unit, allocation, &def->allocation) < 0) goto cleanup; - VIR_FREE(allocation); - VIR_FREE(unit); } else { - ret->allocation = ret->capacity; + def->allocation = def->capacity; } - ret->target.path = virXPathString("string(./target/path)", ctxt); + def->target.path = virXPathString("string(./target/path)", ctxt); if (options->formatFromString) { char *format = virXPathString("string(./target/format/@type)", ctxt); if (format == NULL) - ret->target.format = options->defaultFormat; + def->target.format = options->defaultFormat; else - ret->target.format = (options->formatFromString)(format); + def->target.format = (options->formatFromString)(format); - if (ret->target.format < 0) { + if (def->target.format < 0) { virReportError(VIR_ERR_XML_ERROR, _("unknown volume format type %s"), format); VIR_FREE(format); @@ -1296,28 +1294,28 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, VIR_FREE(format); } - if (virStorageDefParsePerms(ctxt, &ret->target.perms, + if (virStorageDefParsePerms(ctxt, &def->target.perms, "./target/permissions", DEFAULT_VOL_PERM_MODE) < 0) goto cleanup; node = virXPathNode("./target/encryption", ctxt); if (node != NULL) { - ret->target.encryption = virStorageEncryptionParseNode(ctxt->doc, + def->target.encryption = virStorageEncryptionParseNode(ctxt->doc, node); - if (ret->target.encryption == NULL) + if (def->target.encryption == NULL) goto cleanup; } - ret->backingStore.path = virXPathString("string(./backingStore/path)", ctxt); + def->backingStore.path = virXPathString("string(./backingStore/path)", ctxt); if (options->formatFromString) { char *format = virXPathString("string(./backingStore/format/@type)", ctxt); if (format == NULL) - ret->backingStore.format = options->defaultFormat; + def->backingStore.format = options->defaultFormat; else - ret->backingStore.format = (options->formatFromString)(format); + def->backingStore.format = (options->formatFromString)(format); - if (ret->backingStore.format < 0) { + if (def->backingStore.format < 0) { virReportError(VIR_ERR_XML_ERROR, _("unknown volume format type %s"), format); VIR_FREE(format); @@ -1326,19 +1324,19 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, VIR_FREE(format); } - if (virStorageDefParsePerms(ctxt, &ret->backingStore.perms, + if (virStorageDefParsePerms(ctxt, &def->backingStore.perms, "./backingStore/permissions", DEFAULT_VOL_PERM_MODE) < 0) goto cleanup; - return ret; - + ret = def; cleanup: VIR_FREE(allocation); VIR_FREE(capacity); VIR_FREE(unit); - virStorageVolDefFree(ret); - return NULL; + if (!ret) + virStorageVolDefFree(ret); + return ret; } virStorageVolDefPtr @@ -1350,8 +1348,8 @@ virStorageVolDefParseNode(virStoragePoolDefPtr pool, virStorageVolDefPtr def = NULL; if (STRNEQ((const char *)root->name, "volume")) { - virReportError(VIR_ERR_XML_ERROR, - "%s", _("unknown root element for storage vol")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("unknown root element for storage vol")); goto cleanup; } @@ -1481,7 +1479,7 @@ virStorageVolDefFormat(virStoragePoolDefPtr pool, virBufferAddLit(&buf, "<volume>\n"); virBufferAsprintf(&buf," <name>%s</name>\n", def->name); - virBufferAsprintf(&buf," <key>%s</key>\n", def->key ? def->key : "(null)"); + virBufferAsprintf(&buf," <key>%s</key>\n", NULLSTR(def->key)); virBufferAddLit(&buf, " <source>\n"); if (def->source.nextent) { @@ -1658,8 +1656,8 @@ virStoragePoolObjAssignDef(virStoragePoolObjListPtr pools, } if (virMutexInit(&pool->lock) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("cannot initialize mutex")); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot initialize mutex")); VIR_FREE(pool); return NULL; } @@ -1694,7 +1692,8 @@ virStoragePoolObjLoad(virStoragePoolObjListPtr pools, if (!virFileMatchesNameSuffix(file, def->name, ".xml")) { virReportError(VIR_ERR_XML_ERROR, - _("Storage pool config filename '%s' does not match pool name '%s'"), + _("Storage pool config filename '%s' " + "does not match pool name '%s'"), path, def->name); virStoragePoolDefFree(def); return NULL; @@ -1807,8 +1806,8 @@ virStoragePoolObjSaveDef(virStorageDriverStatePtr driver, } if (!(xml = virStoragePoolDefFormat(def))) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("failed to generate XML")); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to generate XML")); return -1; } @@ -1870,8 +1869,8 @@ virStoragePoolSourceListFormat(virStoragePoolSourceListPtr def) type = virStoragePoolTypeToString(def->type); if (!type) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("unexpected pool type")); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("unexpected pool type")); goto cleanup; } -- 1.8.1.4
On Thu, May 16, 2013 at 08:40:53PM +0800, Osier Yang wrote:
virStorageVolDefParseXML: * Create "virStorageVolDefPtr def", and use ret to track the return value; frees the strings at "cleanup" label instead of freeing them in the middle.
virStorageVolDefFormat: * Use macro NULLSTR --- src/conf/storage_conf.c | 93 ++++++++++++++++++++++++------------------------- 1 file changed, 46 insertions(+), 47 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index dd55d2c..431a8eb 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -972,8 +972,8 @@ virStoragePoolDefParseNode(xmlDocPtr xml, virStoragePoolDefPtr def = NULL;
if (STRNEQ((const char *)root->name, "pool")) { - virReportError(VIR_ERR_XML_ERROR, - "%s", _("unknown root element for storage pool")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("unknown root element for storage pool")); goto cleanup; }
@@ -1149,8 +1149,8 @@ virStoragePoolDefFormat(virStoragePoolDefPtr def)
type = virStoragePoolTypeToString(def->type); if (!type) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("unexpected pool type")); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("unexpected pool type")); goto cleanup; } virBufferAsprintf(&buf, "<pool type='%s'>\n", type); @@ -1214,8 +1214,8 @@ virStorageSize(const char *unit, unsigned long long *ret) { if (virStrToLong_ull(val, NULL, 10, ret) < 0) { - virReportError(VIR_ERR_XML_ERROR, - "%s", _("malformed capacity element")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("malformed capacity element")); return -1; } /* off_t is signed, so you cannot create a file larger than 2**63 @@ -1230,64 +1230,62 @@ static virStorageVolDefPtr virStorageVolDefParseXML(virStoragePoolDefPtr pool, xmlXPathContextPtr ctxt) { - virStorageVolDefPtr ret; + virStorageVolDefPtr def; virStorageVolOptionsPtr options; char *allocation = NULL; char *capacity = NULL; char *unit = NULL; xmlNodePtr node; + virStorageVolDefPtr ret = NULL;
IMHO having 2 variables for the same thing is a bit odd. IOW, I think the current code is better than what you've done here. If you want to have unified cleanup, do it like cleanup: VIR_FREE(allocation); VIR_FREE(capacity); VIR_FREE(unit); return ret; error: virStorageVolDefFree(ret); ret = NULL; goto cleanup; Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Changes: * Remove useless comments * Remove useless blank lines * If the struct member is a enum type, comment it like /* enum fooBar */ * Break the long lines * Prefer the common function style for the inline function --- src/conf/storage_conf.h | 120 ++++++++++++++++++++++++------------------------ 1 file changed, 59 insertions(+), 61 deletions(-) diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index 567a4ac..4a1feb3 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -30,11 +30,7 @@ # include <libxml/tree.h> -/* Shared structs */ - - -typedef struct _virStoragePerms virStoragePerms; typedef virStoragePerms *virStoragePermsPtr; struct _virStoragePerms { mode_t mode; @@ -43,8 +39,6 @@ struct _virStoragePerms { char *label; }; -/* Storage volumes */ - typedef struct _virStorageTimestamps virStorageTimestamps; typedef virStorageTimestamps *virStorageTimestampsPtr; struct _virStorageTimestamps { @@ -90,17 +84,17 @@ struct _virStorageVolTarget { virStoragePerms perms; virStorageTimestampsPtr timestamps; int type; /* only used by disk backend for partition type */ - /* Currently used only in virStorageVolDef.target, not in .backingstore. */ + + /* only used in vol->target, not in vol->backingstore. */ virStorageEncryptionPtr encryption; }; - typedef struct _virStorageVolDef virStorageVolDef; typedef virStorageVolDef *virStorageVolDefPtr; struct _virStorageVolDef { char *name; char *key; - int type; /* virStorageVolType enum */ + int type; /* enum virStorageVolType */ unsigned int building; @@ -120,9 +114,6 @@ struct _virStorageVolDefList { }; - -/* Storage pools */ - enum virStoragePoolType { VIR_STORAGE_POOL_DIR, /* Local directory */ VIR_STORAGE_POOL_FS, /* Local filesystem */ @@ -184,9 +175,8 @@ struct _virStoragePoolSourceHost { /* - * For MSDOS partitions, the free area - * is important when creating - * logical partitions + * For MSDOS partitions, the free area is important when + * creating logical partitions */ enum virStorageFreeType { VIR_STORAGE_FREE_NONE = 0, @@ -203,13 +193,12 @@ typedef virStoragePoolSourceDeviceExtent *virStoragePoolSourceDeviceExtentPtr; struct _virStoragePoolSourceDeviceExtent { unsigned long long start; unsigned long long end; - int type; /* free space type */ + int type; /* enum virStorageFreeType */ }; typedef struct _virStoragePoolSourceInitiatorAttr virStoragePoolSourceInitiatorAttr; struct _virStoragePoolSourceInitiatorAttr { - /* Initiator IQN */ - char *iqn; + char *iqn; /* Initiator IQN */ }; /* @@ -222,9 +211,11 @@ struct _virStoragePoolSourceDevice { int nfreeExtent; virStoragePoolSourceDeviceExtentPtr freeExtents; char *path; - int format; /* Pool specific source format */ + int format; /* Pool specific source format */ + /* When the source device is a physical disk, - the geometry data is needed */ + * the geometry data is needed + */ struct _geometry { int cyliders; int heads; @@ -290,25 +281,25 @@ struct _virStoragePoolSource { /* Product name of the source*/ char *product; - int format; /* Pool type specific format such as filesystem type, or lvm version, etc */ + /* Pool type specific format such as filesystem type, + * or lvm version, etc. + */ + int format; }; - typedef struct _virStoragePoolTarget virStoragePoolTarget; typedef virStoragePoolTarget *virStoragePoolTargetPtr; struct _virStoragePoolTarget { - char *path; /* Optional local filesystem mapping */ - virStoragePerms perms; /* Default permissions for volumes */ + char *path; /* Optional local filesystem mapping */ + virStoragePerms perms; /* Default permissions for volumes */ }; - typedef struct _virStoragePoolDef virStoragePoolDef; typedef virStoragePoolDef *virStoragePoolDefPtr; struct _virStoragePoolDef { - /* General metadata */ char *name; unsigned char uuid[VIR_UUID_BUFLEN]; - int type; /* virStoragePoolType */ + int type; /* enum virStoragePoolType */ unsigned long long allocation; /* bytes */ unsigned long long capacity; /* bytes */ @@ -343,9 +334,6 @@ struct _virStoragePoolObjList { virStoragePoolObjPtr *objs; }; - - - typedef struct _virStorageDriverState virStorageDriverState; typedef virStorageDriverState *virStorageDriverStatePtr; @@ -367,7 +355,9 @@ struct _virStoragePoolSourceList { }; -static inline int virStoragePoolObjIsActive(virStoragePoolObjPtr pool) { +static inline int +virStoragePoolObjIsActive(virStoragePoolObjPtr pool) +{ return pool->active; } @@ -375,19 +365,25 @@ int virStoragePoolLoadAllConfigs(virStoragePoolObjListPtr pools, const char *configDir, const char *autostartDir); -virStoragePoolObjPtr virStoragePoolObjFindByUUID(virStoragePoolObjListPtr pools, - const unsigned char *uuid); -virStoragePoolObjPtr virStoragePoolObjFindByName(virStoragePoolObjListPtr pools, - const char *name); -virStoragePoolObjPtr virStoragePoolSourceFindDuplicateDevices(virStoragePoolObjPtr pool, - virStoragePoolDefPtr def); - -virStorageVolDefPtr virStorageVolDefFindByKey(virStoragePoolObjPtr pool, - const char *key); -virStorageVolDefPtr virStorageVolDefFindByPath(virStoragePoolObjPtr pool, - const char *path); -virStorageVolDefPtr virStorageVolDefFindByName(virStoragePoolObjPtr pool, - const char *name); +virStoragePoolObjPtr +virStoragePoolObjFindByUUID(virStoragePoolObjListPtr pools, + const unsigned char *uuid); +virStoragePoolObjPtr +virStoragePoolObjFindByName(virStoragePoolObjListPtr pools, + const char *name); +virStoragePoolObjPtr +virStoragePoolSourceFindDuplicateDevices(virStoragePoolObjPtr pool, + virStoragePoolDefPtr def); + +virStorageVolDefPtr +virStorageVolDefFindByKey(virStoragePoolObjPtr pool, + const char *key); +virStorageVolDefPtr +virStorageVolDefFindByPath(virStoragePoolObjPtr pool, + const char *path); +virStorageVolDefPtr +virStorageVolDefFindByName(virStoragePoolObjPtr pool, + const char *name); void virStoragePoolObjClearVols(virStoragePoolObjPtr pool); @@ -397,18 +393,22 @@ virStoragePoolDefPtr virStoragePoolDefParseNode(xmlDocPtr xml, xmlNodePtr root); char *virStoragePoolDefFormat(virStoragePoolDefPtr def); -virStorageVolDefPtr virStorageVolDefParseString(virStoragePoolDefPtr pool, - const char *xml); -virStorageVolDefPtr virStorageVolDefParseFile(virStoragePoolDefPtr pool, - const char *filename); -virStorageVolDefPtr virStorageVolDefParseNode(virStoragePoolDefPtr pool, - xmlDocPtr xml, - xmlNodePtr root); +virStorageVolDefPtr +virStorageVolDefParseString(virStoragePoolDefPtr pool, + const char *xml); +virStorageVolDefPtr +virStorageVolDefParseFile(virStoragePoolDefPtr pool, + const char *filename); +virStorageVolDefPtr +virStorageVolDefParseNode(virStoragePoolDefPtr pool, + xmlDocPtr xml, + xmlNodePtr root); char *virStorageVolDefFormat(virStoragePoolDefPtr pool, virStorageVolDefPtr def); -virStoragePoolObjPtr virStoragePoolObjAssignDef(virStoragePoolObjListPtr pools, - virStoragePoolDefPtr def); +virStoragePoolObjPtr +virStoragePoolObjAssignDef(virStoragePoolObjListPtr pools, + virStoragePoolDefPtr def); int virStoragePoolObjSaveDef(virStorageDriverStatePtr driver, virStoragePoolObjPtr pool, @@ -481,7 +481,6 @@ enum virStoragePoolFormatDisk { VIR_STORAGE_POOL_DISK_LVM2, VIR_STORAGE_POOL_DISK_LAST, }; - VIR_ENUM_DECL(virStoragePoolFormatDisk) enum virStoragePoolFormatLogical { @@ -492,11 +491,11 @@ enum virStoragePoolFormatLogical { VIR_ENUM_DECL(virStoragePoolFormatLogical) /* - * XXX these are basically partition types. + * XXX: these are basically partition types. * - * fdisk has a bazillion partition ID types - * parted has practically none, and splits the - * info across 3 different attributes. + * fdisk has a bazillion partition ID types parted has + * practically none, and splits the * info across 3 + * different attributes. * * So this is a semi-generic set */ @@ -522,9 +521,8 @@ enum virStorageVolTypeDisk { }; /* - * Mapping of Parted fs-types - * MUST be kept in the same order - * as virStorageVolFormatDisk + * Mapping of Parted fs-types MUST be kept in the + * same order as virStorageVolFormatDisk */ enum virStoragePartedFsType { VIR_STORAGE_PARTED_FS_TYPE_NONE = 0, -- 1.8.1.4
On 16/05/13 20:40, Osier Yang wrote:
Changes: * Remove useless comments * Remove useless blank lines * If the struct member is a enum type, comment it like /* enum fooBar */ * Break the long lines * Prefer the common function style for the inline function --- src/conf/storage_conf.h | 120 ++++++++++++++++++++++++------------------------ 1 file changed, 59 insertions(+), 61 deletions(-)
diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index 567a4ac..4a1feb3 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -30,11 +30,7 @@
# include <libxml/tree.h>
-/* Shared structs */
- - -typedef struct _virStoragePerms virStoragePerms; Accidently removed this line. With the following diff squashed in:
diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index 4a1feb3..8e739ff 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -30,7 +30,7 @@ # include <libxml/tree.h> - +typedef struct _virStoragePerms virStoragePerms; typedef virStoragePerms *virStoragePermsPtr; struct _virStoragePerms { mode_t mode;
On Thu, May 16, 2013 at 08:40:54PM +0800, Osier Yang wrote:
Changes: * Remove useless comments * Remove useless blank lines * If the struct member is a enum type, comment it like /* enum fooBar */ * Break the long lines * Prefer the common function style for the inline function --- src/conf/storage_conf.h | 120 ++++++++++++++++++++++++------------------------ 1 file changed, 59 insertions(+), 61 deletions(-)
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 20/05/13 19:33, Daniel P. Berrange wrote:
On Thu, May 16, 2013 at 08:40:54PM +0800, Osier Yang wrote:
Changes: * Remove useless comments * Remove useless blank lines * If the struct member is a enum type, comment it like /* enum fooBar */ * Break the long lines * Prefer the common function style for the inline function --- src/conf/storage_conf.h | 120 ++++++++++++++++++++++++------------------------ 1 file changed, 59 insertions(+), 61 deletions(-) ACK
Pushed. Osier
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Osier Yang