1. Patchew
  2. Libvirt
  3. View series

[libvirt PATCH] qemu_monitor_json: fix JSON generator for VC chardev

Pavel Hrdina posted 1 patch 3 years, 2 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/f7f4cc75f92876fef0ebf9ab9edfab3d146546e5.1612375264.git.phrdina@redhat.com
src/qemu/qemu_monitor_json.c | 5 ++++-
tests/qemumonitorjsontest.c  | 2 +-
2 files changed, 5 insertions(+), 2 deletions(-)
[libvirt PATCH] qemu_monitor_json: fix JSON generator for VC chardev
Posted by Pavel Hrdina 3 years, 2 months ago 
The correct backend type is 'vc', same as in qemuBuildChrChardevStr()
where we generate qemu command line.

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 src/qemu/qemu_monitor_json.c | 5 ++++-
 tests/qemumonitorjsontest.c  | 2 +-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 97c5e5b36c..231f0ded32 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -7438,10 +7438,13 @@ qemuMonitorJSONAttachCharDevCommand(const char *chrID,
 
     switch ((virDomainChrType)chr->type) {
     case VIR_DOMAIN_CHR_TYPE_NULL:
-    case VIR_DOMAIN_CHR_TYPE_VC:
         backend_type = "null";
         break;
 
+    case VIR_DOMAIN_CHR_TYPE_VC:
+        backend_type = "vc";
+        break;
+
     case VIR_DOMAIN_CHR_TYPE_PTY:
         backend_type = "pty";
         break;
diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c
index 29c396891b..956423f10f 100644
--- a/tests/qemumonitorjsontest.c
+++ b/tests/qemumonitorjsontest.c
@@ -718,7 +718,7 @@ qemuMonitorJSONTestAttachChardev(virDomainXMLOptionPtr xmlopt,
 
     chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_VC };
     CHECK("vc", false,
-          "{'id':'alias','backend':{'type':'null','data':{}}}");
+          "{'id':'alias','backend':{'type':'vc','data':{}}}");
 
     chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_PTY };
     if (qemuMonitorJSONTestAttachOneChardev(xmlopt, schema, "pty", &chr,
-- 
2.29.2
Re: [libvirt PATCH] qemu_monitor_json: fix JSON generator for VC chardev
Posted by Michal Privoznik 3 years, 2 months ago 
On 2/3/21 7:01 PM, Pavel Hrdina wrote:
> The correct backend type is 'vc', same as in qemuBuildChrChardevStr()
> where we generate qemu command line.
> 
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>   src/qemu/qemu_monitor_json.c | 5 ++++-
>   tests/qemumonitorjsontest.c  | 2 +-
>   2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 97c5e5b36c..231f0ded32 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -7438,10 +7438,13 @@ qemuMonitorJSONAttachCharDevCommand(const char *chrID,
>   
>       switch ((virDomainChrType)chr->type) {
>       case VIR_DOMAIN_CHR_TYPE_NULL:
> -    case VIR_DOMAIN_CHR_TYPE_VC:
>           backend_type = "null";
>           break;
>   
> +    case VIR_DOMAIN_CHR_TYPE_VC:
> +        backend_type = "vc";
> +        break;
> +
>       case VIR_DOMAIN_CHR_TYPE_PTY:
>           backend_type = "pty";
>           break;
> diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c
> index 29c396891b..956423f10f 100644
> --- a/tests/qemumonitorjsontest.c
> +++ b/tests/qemumonitorjsontest.c
> @@ -718,7 +718,7 @@ qemuMonitorJSONTestAttachChardev(virDomainXMLOptionPtr xmlopt,
>   
>       chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_VC };
>       CHECK("vc", false,
> -          "{'id':'alias','backend':{'type':'null','data':{}}}");
> +          "{'id':'alias','backend':{'type':'vc','data':{}}}");
>   
>       chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_PTY };
>       if (qemuMonitorJSONTestAttachOneChardev(xmlopt, schema, "pty", &chr,
> 

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

But maybe we could deduplicate some code? I mean, 
qemuBuildChrChardevStr() and some parts of 
qemuMonitorJSONAttachCharDevCommand() look very similar and we could use 
  virQEMUBuildCommandLineJSON() or something. But merging the patch as 
is is okay too.

Michal
Re: [libvirt PATCH] qemu_monitor_json: fix JSON generator for VC chardev
Posted by Pavel Hrdina 3 years, 2 months ago 
On Thu, Feb 04, 2021 at 01:00:51PM +0100, Michal Privoznik wrote:
> On 2/3/21 7:01 PM, Pavel Hrdina wrote:
> > The correct backend type is 'vc', same as in qemuBuildChrChardevStr()
> > where we generate qemu command line.
> > 
> > Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> > ---
> >   src/qemu/qemu_monitor_json.c | 5 ++++-
> >   tests/qemumonitorjsontest.c  | 2 +-
> >   2 files changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> > index 97c5e5b36c..231f0ded32 100644
> > --- a/src/qemu/qemu_monitor_json.c
> > +++ b/src/qemu/qemu_monitor_json.c
> > @@ -7438,10 +7438,13 @@ qemuMonitorJSONAttachCharDevCommand(const char *chrID,
> >       switch ((virDomainChrType)chr->type) {
> >       case VIR_DOMAIN_CHR_TYPE_NULL:
> > -    case VIR_DOMAIN_CHR_TYPE_VC:
> >           backend_type = "null";
> >           break;
> > +    case VIR_DOMAIN_CHR_TYPE_VC:
> > +        backend_type = "vc";
> > +        break;
> > +
> >       case VIR_DOMAIN_CHR_TYPE_PTY:
> >           backend_type = "pty";
> >           break;
> > diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c
> > index 29c396891b..956423f10f 100644
> > --- a/tests/qemumonitorjsontest.c
> > +++ b/tests/qemumonitorjsontest.c
> > @@ -718,7 +718,7 @@ qemuMonitorJSONTestAttachChardev(virDomainXMLOptionPtr xmlopt,
> >       chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_VC };
> >       CHECK("vc", false,
> > -          "{'id':'alias','backend':{'type':'null','data':{}}}");
> > +          "{'id':'alias','backend':{'type':'vc','data':{}}}");
> >       chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_PTY };
> >       if (qemuMonitorJSONTestAttachOneChardev(xmlopt, schema, "pty", &chr,
> > 
> 
> Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

Thanks

> But maybe we could deduplicate some code? I mean, qemuBuildChrChardevStr()
> and some parts of qemuMonitorJSONAttachCharDevCommand() look very similar
> and we could use  virQEMUBuildCommandLineJSON() or something. But merging
> the patch as is is okay too.

That was a plan but it is not that trivial as the JSON and cmdline
syntax are different.

Pavel
Re: [libvirt PATCH] qemu_monitor_json: fix JSON generator for VC chardev
Posted by Michal Privoznik 3 years, 2 months ago 
On 2/4/21 1:06 PM, Pavel Hrdina wrote:
> On Thu, Feb 04, 2021 at 01:00:51PM +0100, Michal Privoznik wrote:
>> On 2/3/21 7:01 PM, Pavel Hrdina wrote:
>>> The correct backend type is 'vc', same as in qemuBuildChrChardevStr()
>>> where we generate qemu command line.
>>>
>>> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
>>> ---
>>>    src/qemu/qemu_monitor_json.c | 5 ++++-
>>>    tests/qemumonitorjsontest.c  | 2 +-
>>>    2 files changed, 5 insertions(+), 2 deletions(-)
>>>

>> But maybe we could deduplicate some code? I mean, qemuBuildChrChardevStr()
>> and some parts of qemuMonitorJSONAttachCharDevCommand() look very similar
>> and we could use  virQEMUBuildCommandLineJSON() or something. But merging
>> the patch as is is okay too.
> 
> That was a plan but it is not that trivial as the JSON and cmdline
> syntax are different.

Oh, are they? I only quickly skimmed through both functions and it 
appeared similar. If it isn't then it may be more confusing to merge 
them together.

Michal

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.