[libvirt] [PATCH v2] Add warning message to XML definition files stored on disk

[Eric Blake]

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 at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20110505/5c272dcb/attachment-0001.sig>