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@xxxxxxxxx>
> > ---
> > 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
