[PATCH 5/8] ch_monitor: Add pty json builder function

[Douglas, William]

On Fri, 2021-08-27 at 17:38 +0100, Daniel P. Berrangé wrote:
> On Thu, Aug 26, 2021 at 02:59:19PM -0700, William Douglas wrote:
> > Add function to build the the json structure to configure a PTY in
> > cloud-hypervisor. The configuration only supports a single serial
> > or
> > console device.
> > 
> > The devices themselves still aren't allowed in configurations yet
> > though.
> > 
> > Signed-off-by: William Douglas <william.douglas at intel.com>
> > ---
> >  src/ch/ch_monitor.c | 56
> > +++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 56 insertions(+)
> > 
> > diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c
> > index 28d1c213cc..1ff956b61e 100644
> > --- a/src/ch/ch_monitor.c
> > +++ b/src/ch/ch_monitor.c
> > @@ -89,6 +89,59 @@ virCHMonitorBuildCPUJson(virJSONValue *content,
> > virDomainDef *vmdef)
> >      return -1;
> >  }
> >  
> > +static int
> > +virCHMonitorBuildPTYJson(virJSONValue *content, virDomainDef
> > *vmdef)
> > +{
> > +    virJSONValue *ptys = virJSONValueNewObject();
> > +
> > +    if ((vmdef->nconsoles &&
> > +         vmdef->consoles[0]->source->type ==
> > VIR_DOMAIN_CHR_TYPE_PTY)
> > +        && (vmdef->nserials &&
> > +            vmdef->serials[0]->source->type ==
> > VIR_DOMAIN_CHR_TYPE_PTY)) {
> > +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > +                       _("Only a single console or serial can be
> > configured for this domain"));
> > +        return -1;
> > +    } else if (vmdef->nconsoles > 1) {
> > +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > +                       _("Only a single console can be configured
> > for this domain"));
> > +        return -1;
> > +    } else if (vmdef->nserials > 1) {
> > +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > +                       _("Only a single serial can be configured
> > for this domain"));
> > +        return -1;
> > +    }
> 
> IIRC, we'd recommend that these checks go into the
> 'deviceValidateCallback'
> impl that you have in ch_domain.c, so that they get run at time the
> user
> loads the XML into libvirt, rather than when the Vm is started.

Looking a little at the qemu code I was curious if this should be in
the domainValidateCallback instead?

> 
> The code below can still safely assume the checks have been done by
> the
> validate function.
> 
> > +
> > +    if (vmdef->nconsoles) {
> > +        g_autoptr(virJSONValue) pty = virJSONValueNewObject();
> > +        if (vmdef->consoles[0]->source->type ==
> > VIR_DOMAIN_CHR_TYPE_PTY) {
> > +            if (virJSONValueObjectAppendString(pty, "mode", "Pty")
> > < 0)
> > +                return -1;
> > +            if (virJSONValueObjectAppend(content, "console", &pty)
> > < 0)
> > +                return -1;
> > +        } else {
> > +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > +                           _("Console can only be enabled for a
> > PTY"));
> > +            return -1;
> > +        }
> 
> The check for type != PTY ought to be in the above validation block
> too, so this code then just assumes PTY.
> 
> > +    }
> > +
> > +    if (vmdef->nserials) {
> > +        g_autoptr(virJSONValue) pty = virJSONValueNewObject();
> > +        if (vmdef->serials[0]->source->type ==
> > VIR_DOMAIN_CHR_TYPE_PTY) {
> > +            if (virJSONValueObjectAppendString(ptys, "mode",
> > "Pty") < 0)
> > +                return -1;
> > +            if (virJSONValueObjectAppend(content, "serial", &pty)
> > < 0)
> > +                return -1;
> > +        } else {
> > +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > +                           _("Serial can only be enabled for a
> > PTY"));
> > +            return -1;
> > +        }
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> >  static int
> >  virCHMonitorBuildKernelRelatedJson(virJSONValue *content,
> > virDomainDef *vmdef)
> >  {
> > @@ -370,6 +423,9 @@ virCHMonitorBuildVMJson(virDomainDef *vmdef,
> > char **jsonstr)
> >          goto cleanup;
> >      }
> >  
> > +    if (virCHMonitorBuildPTYJson(content, vmdef) < 0)
> > +        goto cleanup;
> > +
> >      if (virCHMonitorBuildCPUJson(content, vmdef) < 0)
> >          goto cleanup;
> >  
> > -- 
> > 2.31.1
> > 
> 
> Regards,
> Daniel