[libvirt] [PATCH 05/11] conf: Add channel state for virtio channels to the XML

[Jiri Denemark]

On Wed, Nov 19, 2014 at 11:23:18 +0100, Peter Krempa wrote:
> To track state of virtio channels this patch adds a new output-only
> attribute called 'state' to the <target> element of virtio channels.
> 
> This will be later populated with the guest state of the channel.
> ---
>  docs/formatdomain.html.in                          |  9 ++++-
>  docs/schemas/domaincommon.rng                      |  3 ++
>  src/conf/domain_conf.c                             | 35 ++++++++++++++++--
>  src/conf/domain_conf.h                             | 12 ++++++
>  .../qemuxml2argv-channel-virtio-state.args         | 17 +++++++++
>  .../qemuxml2argv-channel-virtio-state.xml          | 42 +++++++++++++++++++++
>  tests/qemuxml2argvtest.c                           |  2 +
>  .../qemuxml2xmlout-channel-virtio-state-active.xml | 43 ++++++++++++++++++++++
>  ...emuxml2xmlout-channel-virtio-state-inactive.xml | 42 +++++++++++++++++++++
>  tests/qemuxml2xmltest.c                            |  1 +
>  10 files changed, 200 insertions(+), 6 deletions(-)
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-state.args
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-state.xml
>  create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-virtio-state-active.xml
>  create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-virtio-state-inactive.xml
> 
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 9364eb5..229783d 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -4911,7 +4911,7 @@ qemu-kvm -net nic,model=? /dev/null
>      </channel>
>      <channel type='unix'>
>        <source mode='bind' path='/var/lib/libvirt/qemu/f16x86_64.agent'/>
> -      <target type='virtio' name='org.qemu.guest_agent.0'/>
> +      <target type='virtio' name='org.qemu.guest_agent.0' status='connected'/>

s/status/state/

>      </channel>
>      <channel type='spicevmc'>
>        <target type='virtio' name='com.redhat.spice.0'/>
> @@ -4950,7 +4950,12 @@ qemu-kvm -net nic,model=? /dev/null
>          This is very useful in case of a qemu guest agent, where users don't
>          usually care about the source path since it's libvirt who talks to
>          the guest agent. In case users want to utilize this feature, they should
> -        leave <code><source></code> element out.
> +        leave <code><source></code> element out. <span class="since">Since
> +        1.2.11</span>the active XML for a virtio channel may contain an optional

s/the/ the/

> +        <code>state</code> attribute that reflects whether a process in the
> +        guest is active on the channel. This is an output-only attribute.
> +        Possible values for the <code>state</code> attribute are
> +        <code>connected</code> and <code>disconnected</code>.
>        </dd>
>        <dt><code>spicevmc</code></dt>
>        <dd>Paravirtualized SPICE channel. The domain must also have a
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 6863ec6..9d23f3b 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -3377,6 +3377,9 @@
>        <optional>
>          <attribute name="name"/>
>        </optional>
> +      <optional>
> +        <attribute name="state"/>

I think you can be more specific and use

           <attribute name="state">
             <choice>
               <value>connected</value>
               <value>disconnected</value>
             </choice>
           </attribute>

> +      </optional>
>      </element>
>    </define>
>    <define name="channel">
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 5f4b9f6..f1c07b1 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -410,6 +410,11 @@ VIR_ENUM_IMPL(virDomainNetInterfaceLinkState, VIR_DOMAIN_NET_INTERFACE_LINK_STAT
>                "up",
>                "down")
> 
> +VIR_ENUM_IMPL(virDomainChrDeviceState, VIR_DOMAIN_CHR_DEVICE_STATE_LAST,
> +              "default",
> +              "connected",
> +              "disconnected");
> +
>  VIR_ENUM_IMPL(virDomainChrSerialTarget,
>                VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_LAST,
>                "isa-serial",
> @@ -7857,13 +7862,15 @@ virDomainChrTargetTypeFromString(virDomainChrDefPtr def,
> 
>  static int
>  virDomainChrDefParseTargetXML(virDomainChrDefPtr def,
> -                              xmlNodePtr cur)
> +                              xmlNodePtr cur,
> +                              unsigned int flags)
>  {
>      int ret = -1;
>      unsigned int port;
>      char *targetType = virXMLPropString(cur, "type");
>      char *addrStr = NULL;
>      char *portStr = NULL;
> +    char *stateStr = NULL;
> 
>      if ((def->targetType =
>           virDomainChrTargetTypeFromString(def, def->deviceType,
> @@ -7920,6 +7927,20 @@ virDomainChrDefParseTargetXML(virDomainChrDefPtr def,
> 
>          case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO:
>              def->target.name = virXMLPropString(cur, "name");
> +
> +            if (!(flags & VIR_DOMAIN_XML_INACTIVE) &&
> +                (stateStr = virXMLPropString(cur, "state"))) {
> +                int tmp;
> +
> +                if ((tmp = virDomainChrDeviceStateTypeFromString(stateStr)) < 0) {

You should probably change the check to tmp <= 0 so that you don't allow
the XML to contain state='default'.

> +                    virReportError(VIR_ERR_XML_ERROR,
> +                                   _("invalid channel state value '%s'"),
> +                                   stateStr);
> +                    goto error;
> +                }
> +
> +                def->state = tmp;
> +            }
>              break;
>          }
>          break;
...

ACK, everything else looks OK.

Jirka