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

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(a)intel.com&gt; > --- >  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.

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