[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 1/4] xml: Add attribute to <description> element to hold a short note



On 01/13/2012 11:17 AM, Peter Krempa wrote:
> This patch adds an optional attribute note=""  to the <description>
> element in the domain XML. This attribute can hold a short note defined
> by the user to ease the identification of domains. The note is limited
> to 40 characters.

I'd go one step further, and reject newlines.

> 
>  *docs/formatdomain.html.in
>  *docs/schemas/domaincommon.rng
>         - add schema grammar for the new element and documentation
> 
>   *src/conf/domain_conf.c
>   *src/conf/domain_conf.h
>         - add field to hold the new attribute
>         - add code to parse and create XML with the new attribute
> ---
>  docs/formatdomain.html.in     |    7 +++++--
>  docs/schemas/domaincommon.rng |   14 +++++++++++++-
>  src/conf/domain_conf.c        |   27 +++++++++++++++++++++++++--
>  src/conf/domain_conf.h        |    1 +
>  4 files changed, 44 insertions(+), 5 deletions(-)
> 

> @@ -58,7 +59,9 @@
>        <dd>The content of the <code>description</code> element provides a
>        human readable description of the virtual machine. This data is not
>        used by libvirt in any way, it can contain any information the user
> -      wants. <span class="since">Since 0.7.2</span></dd>
> +      wants. <span class="since">Since 0.7.2</span> The optional attribute
> +      <code>note</code> provides space for a shorter description.
> +      <span class="since">Since 0.9.10</span></dd>

The end result has confusing punctuation.  I'd suggest:

+      wants, <span class="since">since 0.7.2</span>. The optional attribute
+      <code>note</code> provides space for a shorter description,
+      capped at 40 bytes and with no newline,
+      <span class="since">since 0.9.10</span>.</dd>

> +++ b/docs/schemas/domaincommon.rng
> @@ -10,7 +10,19 @@
>      -->
>    <define name="description">
>      <element name="description">
> -      <text/>
> +      <optional>
> +        <attribute name="note">
> +          <data type="string"/>

We can make this enforce length restrictions:

<data type="string">
  <param name='pattern'>[^\n]{0,40}</param>
</data>

> +        </attribute>
> +      </optional>
> +      <choice>
> +        <group>
> +          <text/>
> +        </group>
> +        <group>
> +          <empty/>
> +        </group>
> +      </choice>

No need for a <group> around a single element.  This whole layout may
change if we go with an optional <title> instead of <description
note='...'>, but I'd almost rather see:

<element name='description'>
  <choice>
    <group>
      <attribute name='note'>...</attribute>
      <choice>
        <text/>
        <empty/>
      </choice>
    </group>
    <text/>
  </choice>
</element>

that is, you must have the attribute, or text, or both, but not
<description/> with neither.

> +    /* Extract short description of domain (note) */
> +    def->note = virXPathString("string(./description[1]/@note)", ctxt);
> +    if (def->note) {
> +        if (strchr(def->note, '\n')) {
> +            virDomainReportError(VIR_ERR_XML_ERROR,
> +                               "%s", _("Note attribute can't contain newlines"));
> +            goto error;
> +        }
> +
> +        if (strlen(def->note) > 40) {

This checks for bytes, not characters (some 40-character UTF-8 strings
can occupy well more than 100 bytes).  I'm probably okay with a byte
limit, but hope we don't get complaints about someone unable to write a
single word of 12 characters in their favorite locale.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]