Re: [libvirt] [PATCH v2] Add warning message to XML definition files stored on disk
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 619 bytes)