[libvirt] [PATCH v2] Add warning message to XML definition files stored on disk
Users often edit XML file stored in configuration directory thinking of modifying a domain/network/pool/etc. Thus it is wise to let them know they are using the wrong way and give them hint. --- diff to v1: - instead of pointing users to web, write down the actual virsh command - write to passed FD instead of buffer src/conf/domain_conf.c | 10 +++++-- src/conf/domain_conf.h | 3 +- src/conf/network_conf.c | 8 ++++- src/conf/network_conf.h | 3 +- src/conf/nwfilter_conf.c | 14 ++++++++-- src/conf/nwfilter_conf.h | 6 +++- src/conf/storage_conf.c | 6 ++++- src/conf/storage_conf.h | 3 +- src/libvirt_private.syms | 1 + src/nwfilter/nwfilter_driver.c | 3 +- src/storage/storage_driver.c | 3 +- src/util/util.c | 50 ++++++++++++++++++++++++++++++++++++++++ src/util/util.h | 20 ++++++++++++++++ 13 files changed, 114 insertions(+), 16 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 2a681d9..a4be770 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8485,7 +8485,8 @@ error: int virDomainSaveXML(const char *configDir, virDomainDefPtr def, - const char *xml) + const char *xml, + unsigned int flags) { char *configFile = NULL; int fd = -1, ret = -1; @@ -8510,6 +8511,9 @@ int virDomainSaveXML(const char *configDir, goto cleanup; } + if (flags & VIR_XML_EMIT_WARNING) + virEmitXMLWarning(fd, def->name, VIR_XML_EDIT_COMMAND_DOMAIN); + towrite = strlen(xml); if (safewrite(fd, xml, towrite) < 0) { virReportSystemError(errno, @@ -8543,7 +8547,7 @@ int virDomainSaveConfig(const char *configDir, VIR_DOMAIN_XML_WRITE_FLAGS))) goto cleanup; - if (virDomainSaveXML(configDir, def, xml)) + if (virDomainSaveXML(configDir, def, xml, VIR_XML_EMIT_WARNING)) goto cleanup; ret = 0; @@ -8563,7 +8567,7 @@ int virDomainSaveStatus(virCapsPtr caps, if (!(xml = virDomainObjFormat(caps, obj, flags))) goto cleanup; - if (virDomainSaveXML(statusDir, obj->def, xml)) + if (virDomainSaveXML(statusDir, obj->def, xml, VIR_XML_EMIT_WARNING)) goto cleanup; ret = 0; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 1dadf98..3841f1b 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1346,7 +1346,8 @@ void virDomainControllerInsertPreAlloced(virDomainDefPtr def, int virDomainSaveXML(const char *configDir, virDomainDefPtr def, - const char *xml); + const char *xml, + unsigned int flags); int virDomainSaveConfig(const char *configDir, virDomainDefPtr def); diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 5738757..012caa8 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -899,7 +899,8 @@ char *virNetworkDefFormat(const virNetworkDefPtr def) int virNetworkSaveXML(const char *configDir, virNetworkDefPtr def, - const char *xml) + const char *xml, + unsigned int flags) { char *configFile = NULL; int fd = -1, ret = -1; @@ -925,6 +926,9 @@ int virNetworkSaveXML(const char *configDir, goto cleanup; } + if (flags & VIR_XML_EMIT_WARNING) + virEmitXMLWarning(fd, def->name, VIR_XML_EDIT_COMMAND_NETWORK); + towrite = strlen(xml); if (safewrite(fd, xml, towrite) < 0) { virReportSystemError(errno, @@ -959,7 +963,7 @@ int virNetworkSaveConfig(const char *configDir, if (!(xml = virNetworkDefFormat(def))) goto cleanup; - if (virNetworkSaveXML(configDir, def, xml)) + if (virNetworkSaveXML(configDir, def, xml, VIR_XML_EMIT_WARNING)) goto cleanup; ret = 0; diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 281124b..6e39234 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -162,7 +162,8 @@ int virNetworkIpDefNetmask(const virNetworkIpDefPtr def, int virNetworkSaveXML(const char *configDir, virNetworkDefPtr def, - const char *xml); + const char *xml, + unsigned int flags); int virNetworkSaveConfig(const char *configDir, virNetworkDefPtr def); diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index 615c0f1..66a31d1 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -2223,7 +2223,8 @@ virNWFilterObjFindByName(virNWFilterObjListPtr nwfilters, const char *name) int virNWFilterSaveXML(const char *configDir, virNWFilterDefPtr def, - const char *xml) + const char *xml, + unsigned int flags) { char *configFile = NULL; int fd = -1, ret = -1; @@ -2249,6 +2250,9 @@ int virNWFilterSaveXML(const char *configDir, goto cleanup; } + if (flags & VIR_XML_EMIT_WARNING) + virEmitXMLWarning(fd, def->name, VIR_XML_EDIT_COMMAND_NWFILTER); + towrite = strlen(xml); if (safewrite(fd, xml, towrite) < 0) { virReportSystemError(errno, @@ -2283,7 +2287,7 @@ int virNWFilterSaveConfig(const char *configDir, if (!(xml = virNWFilterDefFormat(def))) goto cleanup; - if (virNWFilterSaveXML(configDir, def, xml)) + if (virNWFilterSaveXML(configDir, def, xml, VIR_XML_EMIT_WARNING)) goto cleanup; ret = 0; @@ -2608,7 +2612,8 @@ virNWFilterLoadAllConfigs(virConnectPtr conn, int virNWFilterObjSaveDef(virNWFilterDriverStatePtr driver, virNWFilterObjPtr nwfilter, - virNWFilterDefPtr def) + virNWFilterDefPtr def, + unsigned int flags) { char *xml; int fd = -1, ret = -1; @@ -2645,6 +2650,9 @@ virNWFilterObjSaveDef(virNWFilterDriverStatePtr driver, goto cleanup; } + if (flags & VIR_XML_EMIT_WARNING) + virEmitXMLWarning(fd, def->name, VIR_XML_EDIT_COMMAND_NWFILTER); + towrite = strlen(xml); if (safewrite(fd, xml, towrite) != towrite) { virReportSystemError(errno, diff --git a/src/conf/nwfilter_conf.h b/src/conf/nwfilter_conf.h index 12e2a5c..219bf7e 100644 --- a/src/conf/nwfilter_conf.h +++ b/src/conf/nwfilter_conf.h @@ -603,7 +603,8 @@ virNWFilterObjPtr virNWFilterObjFindByName(virNWFilterObjListPtr nwfilters, int virNWFilterObjSaveDef(virNWFilterDriverStatePtr driver, virNWFilterObjPtr nwfilter, - virNWFilterDefPtr def); + virNWFilterDefPtr def, + unsigned int flags); int virNWFilterObjDeleteDef(virNWFilterObjPtr nwfilter); @@ -621,7 +622,8 @@ char *virNWFilterDefFormat(virNWFilterDefPtr def); int virNWFilterSaveXML(const char *configDir, virNWFilterDefPtr def, - const char *xml); + const char *xml, + unsigned int flags); int virNWFilterSaveConfig(const char *configDir, virNWFilterDefPtr def); diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 116898d..b729bbe 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1509,7 +1509,8 @@ virStoragePoolLoadAllConfigs(virStoragePoolObjListPtr pools, int virStoragePoolObjSaveDef(virStorageDriverStatePtr driver, virStoragePoolObjPtr pool, - virStoragePoolDefPtr def) { + virStoragePoolDefPtr def, + unsigned int flags) { char *xml; int fd = -1, ret = -1; ssize_t towrite; @@ -1551,6 +1552,9 @@ virStoragePoolObjSaveDef(virStorageDriverStatePtr driver, goto cleanup; } + if (flags & VIR_XML_EMIT_WARNING) + virEmitXMLWarning(fd, def->name, VIR_XML_EDIT_COMMAND_STORAGE_POOL); + towrite = strlen(xml); if (safewrite(fd, xml, towrite) != towrite) { virReportSystemError(errno, diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index 271441a..9bbc25d 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -366,7 +366,8 @@ virStoragePoolObjPtr virStoragePoolObjAssignDef(virStoragePoolObjListPtr pools, int virStoragePoolObjSaveDef(virStorageDriverStatePtr driver, virStoragePoolObjPtr pool, - virStoragePoolDefPtr def); + virStoragePoolDefPtr def, + unsigned int flags); int virStoragePoolObjDeleteDef(virStoragePoolObjPtr pool); void virStorageVolDefFree(virStorageVolDefPtr def); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 1b22be6..2cbec44 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -897,6 +897,7 @@ virArgvToString; virAsprintf; virBuildPathInternal; virDirCreate; +virEmitXMLWarning; virEnumFromString; virEnumToString; virEventAddHandle; diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index 8af3f8a..2d0db73 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -343,7 +343,8 @@ nwfilterDefine(virConnectPtr conn, if (!(nwfilter = virNWFilterObjAssignDef(conn, &driver->nwfilters, def))) goto cleanup; - if (virNWFilterObjSaveDef(driver, nwfilter, def) < 0) { + if (virNWFilterObjSaveDef(driver, nwfilter, def, + VIR_XML_EMIT_WARNING) < 0) { virNWFilterObjRemove(&driver->nwfilters, nwfilter); def = NULL; goto cleanup; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 5118ffb..acfc791 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -586,7 +586,8 @@ storagePoolDefine(virConnectPtr conn, if (!(pool = virStoragePoolObjAssignDef(&driver->pools, def))) goto cleanup; - if (virStoragePoolObjSaveDef(driver, pool, def) < 0) { + if (virStoragePoolObjSaveDef(driver, pool, def, + VIR_XML_EMIT_WARNING) < 0) { virStoragePoolObjRemove(&driver->pools, pool); def = NULL; goto cleanup; diff --git a/src/util/util.c b/src/util/util.c index 9041ab6..a622e08 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -3207,3 +3207,53 @@ bool virIsDevMapperDevice(const char *devname ATTRIBUTE_UNUSED) return false; } #endif + +VIR_ENUM_IMPL(virXMLEditCommand, VIR_XML_EDIT_COMMAND_LAST, + "", + "edit", + "net-edit", + "nwfilter-edit", + "pool-edit") + +int virEmitXMLWarning(int fd, + const char *name, + unsigned int cmd) { + size_t len; + const char *cmd_str = virXMLEditCommandTypeToString(cmd); + const char *prologue = _("<!--\n\ +WARNING: THIS IS AN AUTO-GENERATED FILE. CHANGES TO IT ARE LIKELY TO BE \n\ +OVERWRITTEN AND LOST. Changes to this xml configuration should be made using:\n\ +virsh "); + const char *epilogue = _("\n\ +or other application using the libvirt API.\n\ +-->\n\n"); + + if (fd < 0 || !name) + return -1; + + if (!cmd_str || STREQ(cmd_str, "")) { + VIR_WARN("unknown command code %d", cmd); + return -1; + } + + len = strlen(prologue); + if (safewrite(fd, prologue, len) != len) + return -1; + + len = strlen(cmd_str); + if (safewrite(fd, cmd_str, len) != len) + return -1; + + if (safewrite(fd, " ", 1) != 1) + return -1; + + len = strlen(name); + if (safewrite(fd, name, len) != len) + return -1; + + len = strlen(epilogue); + if (safewrite(fd, epilogue, len) != len) + return -1; + + return 0; +} diff --git a/src/util/util.h b/src/util/util.h index d320c40..c0ca5f4 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -299,4 +299,24 @@ int virBuildPathInternal(char **path, ...) ATTRIBUTE_SENTINEL; char *virTimestamp(void); bool virIsDevMapperDevice(const char *devname) ATTRIBUTE_NONNULL(1); + +enum { + VIR_XML_EMIT_WARNING = (1 << 0), +}; + +enum virXMLEditCommand { + VIR_XML_EDIT_COMMAND_UNKNOWN = 0, + VIR_XML_EDIT_COMMAND_DOMAIN, + VIR_XML_EDIT_COMMAND_NETWORK, + VIR_XML_EDIT_COMMAND_NWFILTER, + VIR_XML_EDIT_COMMAND_STORAGE_POOL, + + VIR_XML_EDIT_COMMAND_LAST +}; + +VIR_ENUM_DECL(virXMLEditCommand) + +int virEmitXMLWarning(int fd, + const char *name, + unsigned int cmd) ATTRIBUTE_NONNULL(2); #endif /* __VIR_UTIL_H__ */ -- 1.7.4.4
On 05/05/2011 05:25 AM, Michal Privoznik wrote:
Users often edit XML file stored in configuration directory thinking of modifying a domain/network/pool/etc. Thus it is wise to let them know they are using the wrong way and give them hint. --- diff to v1: - instead of pointing users to web, write down the actual virsh command - write to passed FD instead of buffer
My apologies for not noticing sooner, but...
int virDomainSaveXML(const char *configDir, virDomainDefPtr def, - const char *xml) + const char *xml, + unsigned int flags) { char *configFile = NULL; int fd = -1, ret = -1; @@ -8510,6 +8511,9 @@ int virDomainSaveXML(const char *configDir, goto cleanup; }
+ if (flags & VIR_XML_EMIT_WARNING) + virEmitXMLWarning(fd, def->name, VIR_XML_EDIT_COMMAND_DOMAIN);
Every last caller to this function is now passing the same flag, so the flags argument is redundant. I thought we might have a difference where sometimes we need it and sometimes we don't, but I don't see any cases where we don't. I'd prefer a v3 that touches fewer lines of code by not adding the extra flags argument, but just unconditionally calling virEmitXMLWarning here...
+++ b/src/util/util.c @@ -3207,3 +3207,53 @@ bool virIsDevMapperDevice(const char *devname ATTRIBUTE_UNUSED) return false; } #endif + +VIR_ENUM_IMPL(virXMLEditCommand, VIR_XML_EDIT_COMMAND_LAST, + "",
Why do we need the "" entry?
+ "edit", + "net-edit", + "nwfilter-edit", + "pool-edit") + +int virEmitXMLWarning(int fd, + const char *name, + unsigned int cmd) { + size_t len; + const char *cmd_str = virXMLEditCommandTypeToString(cmd); + const char *prologue = _("<!--\n\ +WARNING: THIS IS AN AUTO-GENERATED FILE. CHANGES TO IT ARE LIKELY TO BE \n\ +OVERWRITTEN AND LOST. Changes to this xml configuration should be made using:\n\ +virsh ");
Do we really want these strings translated, or are they okay always being in English? The rest of the xml file is locale independent, and I'm worried that if we put in a translated message, that a translator might not form a well-formed xml comment in their translation, which breaks things. Furthermore, this is in a root-accessible file, much like libvirtd.conf or qemu.conf, where we aren't translating any comments in those files.
+++ b/src/util/util.h @@ -299,4 +299,24 @@ int virBuildPathInternal(char **path, ...) ATTRIBUTE_SENTINEL; char *virTimestamp(void);
bool virIsDevMapperDevice(const char *devname) ATTRIBUTE_NONNULL(1); + +enum { + VIR_XML_EMIT_WARNING = (1 << 0), +};
Since there was no one in your patch that passed 0, we don't need this flag.
+ +enum virXMLEditCommand { + VIR_XML_EDIT_COMMAND_UNKNOWN = 0,
Since this command is completely internal, we should never be passing it a bad command, and can start directly with domain. But do we even need an enum, or can we just pass a const char * and save ourselves the effort of a lookup in the first place? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Michal Privoznik