On 03/24/2015 06:06 AM, Erik Skultety wrote: > This patch introduces virStoragePoolSaveStatus to properly format the > status XML. It also adds virStoragePoolDefFormatBuf function, which > alike in the network driver, formats the whole storage pool definition into > a buffer that might have been previously modified by > virStoragePoolSaveStatus to insert <poolstatus> element. The original > virStoragePoolDefFormat function had to be modified accordingly to use > virBuffer. > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1177733 > --- > src/conf/storage_conf.c | 139 ++++++++++++++++++++++++++++++++++------------- > src/conf/storage_conf.h | 6 +- > src/libvirt_private.syms | 1 + > 3 files changed, 107 insertions(+), 39 deletions(-) > In a bit of bikeshedding - this patch does a couple of things and could be split into a couple of patches... Ironically you bundled things together here, but separated them for the stateDir changes (patches 2, 4, & 7). first one just creates DefFormatBuf and has DefFormat call it second one creates the virStoragePoolSaveXML, has the config code use it third one creates virStoragePoolSaveStatus which use the new API's > diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c > index b070448..5d984f3 100644 > --- a/src/conf/storage_conf.c > +++ b/src/conf/storage_conf.c > @@ -1150,76 +1150,90 @@ virStoragePoolSourceFormat(virBufferPtr buf, > } > > > -char * > -virStoragePoolDefFormat(virStoragePoolDefPtr def) > +int static int - only this function cares; otherwise we have to ensure the 'buf' and 'def' arguments aren't NULL in the .h file... This way we control having non NULL values being presented. > +virStoragePoolDefFormatBuf(virBufferPtr buf, > + virStoragePoolDefPtr def) > { > virStoragePoolOptionsPtr options; > - virBuffer buf = VIR_BUFFER_INITIALIZER; > - const char *type; > char uuid[VIR_UUID_STRING_BUFLEN]; > + const char *type; > > options = virStoragePoolOptionsForPoolType(def->type); > if (options == NULL) > - return NULL; > + goto error; > > type = virStoragePoolTypeToString(def->type); > if (!type) { > virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > _("unexpected pool type")); > - goto cleanup; > + goto error; > } > - virBufferAsprintf(&buf, "<pool type='%s'>\n", type); > - virBufferAdjustIndent(&buf, 2); > - virBufferEscapeString(&buf, "<name>%s</name>\n", def->name); > + virBufferAsprintf(buf, "<pool type='%s'>\n", type); > + virBufferAdjustIndent(buf, 2); > + virBufferEscapeString(buf, "<name>%s</name>\n", def->name); > > virUUIDFormat(def->uuid, uuid); > - virBufferAsprintf(&buf, "<uuid>%s</uuid>\n", uuid); > + virBufferAsprintf(buf, "<uuid>%s</uuid>\n", uuid); > > - virBufferAsprintf(&buf, "<capacity unit='bytes'>%llu</capacity>\n", > + virBufferAsprintf(buf, "<capacity unit='bytes'>%llu</capacity>\n", > def->capacity); > - virBufferAsprintf(&buf, "<allocation unit='bytes'>%llu</allocation>\n", > + virBufferAsprintf(buf, "<allocation unit='bytes'>%llu</allocation>\n", > def->allocation); > - virBufferAsprintf(&buf, "<available unit='bytes'>%llu</available>\n", > + virBufferAsprintf(buf, "<available unit='bytes'>%llu</available>\n", > def->available); > > - if (virStoragePoolSourceFormat(&buf, options, &def->source) < 0) > - goto cleanup; > + if (virStoragePoolSourceFormat(buf, options, &def->source) < 0) > + goto error; > > /* RBD, Sheepdog, and Gluster devices are not local block devs nor > * files, so they don't have a target */ > if (def->type != VIR_STORAGE_POOL_RBD && > def->type != VIR_STORAGE_POOL_SHEEPDOG && > def->type != VIR_STORAGE_POOL_GLUSTER) { > - virBufferAddLit(&buf, "<target>\n"); > - virBufferAdjustIndent(&buf, 2); > + virBufferAddLit(buf, "<target>\n"); > + virBufferAdjustIndent(buf, 2); > > - virBufferEscapeString(&buf, "<path>%s</path>\n", def->target.path); > + virBufferEscapeString(buf, "<path>%s</path>\n", def->target.path); > > - virBufferAddLit(&buf, "<permissions>\n"); > - virBufferAdjustIndent(&buf, 2); > - virBufferAsprintf(&buf, "<mode>0%o</mode>\n", > + virBufferAddLit(buf, "<permissions>\n"); > + virBufferAdjustIndent(buf, 2); > + virBufferAsprintf(buf, "<mode>0%o</mode>\n", > def->target.perms.mode); > - virBufferAsprintf(&buf, "<owner>%d</owner>\n", > + virBufferAsprintf(buf, "<owner>%d</owner>\n", > (int) def->target.perms.uid); > - virBufferAsprintf(&buf, "<group>%d</group>\n", > + virBufferAsprintf(buf, "<group>%d</group>\n", > (int) def->target.perms.gid); > - virBufferEscapeString(&buf, "<label>%s</label>\n", > + virBufferEscapeString(buf, "<label>%s</label>\n", > def->target.perms.label); > > - virBufferAdjustIndent(&buf, -2); > - virBufferAddLit(&buf, "</permissions>\n"); > - virBufferAdjustIndent(&buf, -2); > - virBufferAddLit(&buf, "</target>\n"); > + virBufferAdjustIndent(buf, -2); > + virBufferAddLit(buf, "</permissions>\n"); > + virBufferAdjustIndent(buf, -2); > + virBufferAddLit(buf, "</target>\n"); > } > - virBufferAdjustIndent(&buf, -2); > - virBufferAddLit(&buf, "</pool>\n"); > + virBufferAdjustIndent(buf, -2); > + virBufferAddLit(buf, "</pool>\n"); > + > + return 0; > + > + error: > + return -1; > +} > + > +char * > +virStoragePoolDefFormat(virStoragePoolDefPtr def) > +{ > + virBuffer buf = VIR_BUFFER_INITIALIZER; > + > + if (virStoragePoolDefFormatBuf(&buf, def) < 0) > + goto error; > > if (virBufferCheckError(&buf) < 0) > - goto cleanup; > + goto error; > > return virBufferContentAndReset(&buf); > > - cleanup: > + error: > virBufferFreeAndReset(&buf); > return NULL; > } > @@ -1899,11 +1913,60 @@ virStoragePoolLoadAllConfigs(virStoragePoolObjListPtr pools, > return ret; > } > > + > +static int virStoragePoolSaveXML(const char *configFile, > + virStoragePoolDefPtr def, > + const char *xml) static int virStoragePoolSaveXML(const char *path, (since it won't be the "configFile" only...) > +{ > + char uuidstr[VIR_UUID_STRING_BUFLEN]; > + int ret = -1; > + > + virUUIDFormat(def->uuid, uuidstr); > + ret = virXMLSaveFile(configFile, s/configFile/path > + virXMLPickShellSafeComment(def->name, uuidstr), > + "pool-edit", xml); > + > + return ret; > +} > + > + > +int virStoragePoolSaveStatus(const char *stateFile, > + virStoragePoolDefPtr def) int virStoragePoolSaveStatus(... > +{ > + virBuffer buf = VIR_BUFFER_INITIALIZER; > + int ret = -1; > + char *xml; > + > + virBufferAddLit(&buf, "<poolstatus>\n"); poolstate ? > + virBufferAdjustIndent(&buf, 2); > + > + if (virStoragePoolDefFormatBuf(&buf, def) < 0) > + goto error; > + > + virBufferAdjustIndent(&buf, -2); > + virBufferAddLit(&buf, "</poolstatus>\n"); poolstate ? > + > + if (virBufferCheckError(&buf) < 0) > + goto error; > + > + if (!(xml = virBufferContentAndReset(&buf))) > + goto error; > + > + if (virStoragePoolSaveXML(stateFile, def, xml)) > + goto error; > + > + ret = 0; > + > + error: > + VIR_FREE(xml); > + return ret; > +} > + > + > int > virStoragePoolSaveConfig(const char *configFile, > virStoragePoolDefPtr def) > { > - char uuidstr[VIR_UUID_STRING_BUFLEN]; > char *xml; > int ret = -1; > > @@ -1913,12 +1976,12 @@ virStoragePoolSaveConfig(const char *configFile, > return -1; > } > > - virUUIDFormat(def->uuid, uuidstr); > - ret = virXMLSaveFile(configFile, > - virXMLPickShellSafeComment(def->name, uuidstr), > - "pool-edit", xml); > - VIR_FREE(xml); > + if (virStoragePoolSaveXML(configFile, def, xml)) > + goto cleanup; > > + ret = 0; > + cleanup: > + VIR_FREE(xml); > return ret; > } > > diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h > index 8ccc947..99b2f4a 100644 > --- a/src/conf/storage_conf.h > +++ b/src/conf/storage_conf.h > @@ -345,6 +345,8 @@ virStoragePoolDefPtr virStoragePoolDefParseFile(const char *filename); > virStoragePoolDefPtr virStoragePoolDefParseNode(xmlDocPtr xml, > xmlNodePtr root); > char *virStoragePoolDefFormat(virStoragePoolDefPtr def); > +int virStoragePoolDefFormatBuf(virBufferPtr buf, > + virStoragePoolDefPtr def); Unnecessary if static int Obvious if this gets split into 3 patches, then there's a few changes to the following stuff changes too John > > typedef enum { > /* do not require volume capacity at all */ > @@ -372,7 +374,9 @@ virStoragePoolObjPtr > virStoragePoolObjAssignDef(virStoragePoolObjListPtr pools, > virStoragePoolDefPtr def); > > -int virStoragePoolSaveConfig(const char *configDir, > +int virStoragePoolSaveStatus(const char *stateFile, > + virStoragePoolDefPtr def); > +int virStoragePoolSaveConfig(const char *configFile, > virStoragePoolDefPtr def); > int virStoragePoolObjSaveDef(virStorageDriverStatePtr driver, > virStoragePoolObjPtr pool, > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms > index 33222f0..689a08f 100644 > --- a/src/libvirt_private.syms > +++ b/src/libvirt_private.syms > @@ -811,6 +811,7 @@ virStoragePoolObjRemove; > virStoragePoolObjSaveDef; > virStoragePoolObjUnlock; > virStoragePoolSaveConfig; > +virStoragePoolSaveStatus; > virStoragePoolSourceAdapterTypeFromString; > virStoragePoolSourceAdapterTypeToString; > virStoragePoolSourceClear; > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list