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@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list