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