Re: [libvirt] [PATCH v1 1/4] conf: add xen type for channels

On 09/16/2016 05:43 PM, Joao Martins wrote: > So far only guestfwd and virtio were supported. Add an additional > for Xen as libxl channels create Xen console visible to the guest. > > Signed-off-by: Joao Martins <joao.m.martins(a)oracle.com&gt; > --- > docs/schemas/domaincommon.rng | 11 +++++++++++ > src/conf/domain_conf.c | 18 ++++++++++++++---- > src/conf/domain_conf.h | 1 + > src/qemu/qemu_command.c | 1 + > 4 files changed, 27 insertions(+), 4 deletions(-) > > diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng > index 8aaa67e..5901452 100644 > --- a/docs/schemas/domaincommon.rng > +++ b/docs/schemas/domaincommon.rng > @@ -3686,6 +3686,16 @@ > </optional> > </element> > </define> > + <define name="xenTarget"> > + <element name="target"> > + <attribute name="type"> > + <value>xen</value> > + </attribute> > + <optional> > + <attribute name="name"/> > + </optional> > + </element> > + </define> > <define name="channel"> > <element name="channel"> > <ref name="qemucdevSrcType"/> > @@ -3694,6 +3704,7 @@ > <choice> > <ref name="guestfwdTarget"/> > <ref name="virtioTarget"/> > + <ref name="xenTarget"/> > </choice> > <optional> > <ref name="alias"/> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index 0828041..196799d 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -434,7 +434,8 @@ VIR_ENUM_IMPL(virDomainChrChannelTarget, > VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_LAST, > "none", > "guestfwd", > - "virtio") > + "virtio", > + "xen") > > VIR_ENUM_IMPL(virDomainChrConsoleTarget, > VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_LAST, > @@ -2068,6 +2069,7 @@ void virDomainChrDefFree(virDomainChrDefPtr def) > VIR_FREE(def->target.addr); > break; > > + case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_XEN: > case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO: > VIR_FREE(def->target.name); > break; > @@ -9877,10 +9879,12 @@ virDomainChrDefParseTargetXML(virDomainChrDefPtr def, > virSocketAddrSetPort(def->target.addr, port); > break; > > + case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_XEN: > case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO: > def->target.name = virXMLPropString(cur, "name"); > > - if (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE) && > + if (def->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO && > + !(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE) && > (stateStr = virXMLPropString(cur, "state"))) { > int tmp; I guess we'll need an answer to your question about the 'state' attribute to know if this is needed.

> @@ -10171,7 +10175,8 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, > /* path can be auto generated */ > if (!path && > (!chr_def || > - chr_def->targetType != VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO)) { > + (chr_def->targetType != VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_XEN && > + chr_def->targetType != VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO))) { > virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > _("Missing source path attribute for char device")); > goto error; > @@ -14373,6 +14378,7 @@ virDomainChrEquals(virDomainChrDefPtr src, > if (src->targetType != tgt->targetType) > return false; > switch ((virDomainChrChannelTargetType) src->targetType) { > + case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_XEN: > case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO: > return STREQ_NULLABLE(src->target.name, tgt->target.name); > break; > @@ -18310,6 +18316,8 @@ virDomainChannelDefCheckABIStability(virDomainChrDefPtr src, > } > > switch (src->targetType) { > + > + case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_XEN: > case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO: > if (STRNEQ_NULLABLE(src->target.name, dst->target.name)) { > virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > @@ -21432,11 +21440,13 @@ virDomainChrDefFormat(virBufferPtr buf, > break; > } > > + case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_XEN: > case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO: > if (def->target.name) > virBufferEscapeString(buf, " name='%s'", def->target.name); > > - if (def->state != VIR_DOMAIN_CHR_DEVICE_STATE_DEFAULT && > + if (def->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO && > + def->state != VIR_DOMAIN_CHR_DEVICE_STATE_DEFAULT && > !(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE)) { > virBufferAsprintf(buf, " state='%s'", > virDomainChrDeviceStateTypeToString(def->state)); Same here.

Looks good otherwise.