[PATCH 04/10] ch_monitor: Add pty json builder function

[Michal Prívozník]

On 6/30/21 1:05 AM, 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.
> 
> Signed-off-by: William Douglas <william.douglas at intel.com>
> ---
>  src/ch/ch_monitor.c | 59 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 59 insertions(+)
> 
> diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c
> index 5e7d6e3189..d4289b75ce 100644
> --- a/src/ch/ch_monitor.c
> +++ b/src/ch/ch_monitor.c
> @@ -89,6 +89,65 @@ virCHMonitorBuildCPUJson(virJSONValue *content, virDomainDef *vmdef)
>      return -1;
>  }
>  
> +static int
> +virCHMonitorBuildPTYJson(virJSONValue *content, virDomainDef *vmdef)
> +{
> +    virJSONValue *ptyc = virJSONValueNewObject();
> +    virJSONValue *ptys = virJSONValueNewObject();
> +
> +    if (vmdef->nconsoles || vmdef->nserials) {

This ^^^ looks redundant since you explicitly check for problematic
configuration below.

> +        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"));
> +            goto cleanup;
> +        } else if (vmdef->nconsoles > 1) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                           _("Only a single console can be configured for this domain"));
> +            goto cleanup;
> +        } else if (vmdef->nserials > 1) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                           _("Only a single serial can be configured for this domain"));
> +            goto cleanup;
> +        }
> +    }
> +
> +    if (vmdef->nconsoles) {
> +        if (vmdef->consoles[0]->source->type == VIR_DOMAIN_CHR_TYPE_PTY) {
> +            if (virJSONValueObjectAppendString(ptyc, "mode", "Pty") < 0)
> +                goto cleanup;
> +            if (virJSONValueObjectAppend(content, "console", &ptyc) < 0)
> +                goto cleanup;
> +        } else {
> +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                           _("Console can only be enabled for a PTY"));
> +            goto cleanup;
> +        }
> +    }
> +
> +    if (vmdef->nserials) {
> +        if (vmdef->serials[0]->source->type == VIR_DOMAIN_CHR_TYPE_PTY) {
> +            if (virJSONValueObjectAppendString(ptys, "mode", "Pty") < 0)
> +                goto cleanup;
> +            if (virJSONValueObjectAppend(content, "serial", &ptys) < 0)
> +                goto cleanup;
> +        } else {
> +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                           _("Serial can only be enabled for a PTY"));
> +            goto cleanup;
> +        }
> +    }
> +
> +    return 0;
> +
> + cleanup:
> +    virJSONValueFree(ptyc);
> +    virJSONValueFree(ptys);

You can avoid these explicit calls to virJSONValueFree() if you declare
those variables as g_autoptr() like this:

g_autoptr(virJSONValue) ptyc = NULL;
g_autoptr(virJSONValue) pyts = NULL;

and you can even do that in the inner most blocks where the vars are used.

> +    return -1;

Then, this return -1 can be at all those places where 'goto cleanup' is.

> +}
> +
>  static int
>  virCHMonitorBuildKernelRelatedJson(virJSONValue *content, virDomainDef *vmdef)
>  {
> 

Michal