Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 src/conf/domain_conf.c | 39 ++++++++++++++++++++++++++-------------
 1 file changed, 26 insertions(+), 13 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index bb4be5d1cd..8fe79f70bf 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -10885,6 +10885,30 @@ virDomainChrDefParseTargetXML(virDomainChrDefPtr def,
     return ret;
 }
 
+
+static int
+virDomainChrSourceDefParseProtocol(virDomainChrSourceDefPtr def,
+                                   xmlNodePtr protocol)
+{
+    char *prot = NULL;
+
+    if (def->type != VIR_DOMAIN_CHR_TYPE_TCP)
+        return 0;
+
+    if ((prot = virXMLPropString(protocol, "type")) &&
+        (def->data.tcp.protocol =
+         virDomainChrTcpProtocolTypeFromString(prot)) < 0) {
+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                       _("Unknown protocol '%s'"), prot);
+        VIR_FREE(prot);
+        return -1;
+    }
+
+    VIR_FREE(prot);
+    return 0;
+}
+
+
 #define SERIAL_CHANNEL_NAME_CHARS \
     "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_-."
 
@@ -10910,7 +10934,6 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
     char *logfile = NULL;
     char *logappend = NULL;
     char *mode = NULL;
-    char *protocol = NULL;
     char *channel = NULL;
     char *master = NULL;
     char *slave = NULL;
@@ -11040,7 +11063,8 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
                 goto error;
             }
             protocolParsed = true;
-            protocol = virXMLPropString(cur, "type");
+            if (virDomainChrSourceDefParseProtocol(def, cur) < 0)
+                goto error;
         }
     }
 
@@ -11151,16 +11175,6 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
             }
             def->data.tcp.tlsFromConfig = !!tmp;
         }
-
-        if (!protocol)
-            def->data.tcp.protocol = VIR_DOMAIN_CHR_TCP_PROTOCOL_RAW;
-        else if ((def->data.tcp.protocol =
-                  virDomainChrTcpProtocolTypeFromString(protocol)) < 0) {
-            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-                           _("Unknown protocol '%s'"), protocol);
-            goto error;
-        }
-
         break;
 
     case VIR_DOMAIN_CHR_TYPE_UDP:
@@ -11227,7 +11241,6 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
     ret = 0;
  cleanup:
     VIR_FREE(mode);
-    VIR_FREE(protocol);
     VIR_FREE(bindHost);
     VIR_FREE(bindService);
     VIR_FREE(connectHost);
-- 
2.13.5

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Mon, Aug 21, 2017 at 10:07:08AM +0200, Pavel Hrdina wrote:
>Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
>---
> src/conf/domain_conf.c | 39 ++++++++++++++++++++++++++-------------
> 1 file changed, 26 insertions(+), 13 deletions(-)
>
>diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>index bb4be5d1cd..8fe79f70bf 100644
>--- a/src/conf/domain_conf.c
>+++ b/src/conf/domain_conf.c
>@@ -11040,7 +11063,8 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
>                 goto error;
>             }
>             protocolParsed = true;
>-            protocol = virXMLPropString(cur, "type");
>+            if (virDomainChrSourceDefParseProtocol(def, cur) < 0)
>+                goto error;
>         }
>     }
>
>@@ -11151,16 +11175,6 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
>             }
>             def->data.tcp.tlsFromConfig = !!tmp;
>         }
>-
>-        if (!protocol)
>-            def->data.tcp.protocol = VIR_DOMAIN_CHR_TCP_PROTOCOL_RAW;

This removes the explicit assignment of VIR_DOMAIN_CHR_TCP_PROTOCOL_RAW
if no protocol node has been seen.

The most direct equivalent would be:
    if (!protocolParsed)
but I would also be okay with (in the order of preference)
1. initializing it before we start parsing the node
2. adding a _DEFAULT enum value and changing it in PostParse
3. explicitly assigning VIR_DOMAIN_CHR_TCP_PROTOCOL_RAW = 0
and letting calloc do the initialization

ACK with that fixed

Jan

>-        else if ((def->data.tcp.protocol =
>-                  virDomainChrTcpProtocolTypeFromString(protocol)) < 0) {
>-            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>-                           _("Unknown protocol '%s'"), protocol);
>-            goto error;
>-        }
>-
>         break;
>
>     case VIR_DOMAIN_CHR_TYPE_UDP:
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Tue, Aug 22, 2017 at 03:59:01PM +0200, Ján Tomko wrote:
> On Mon, Aug 21, 2017 at 10:07:08AM +0200, Pavel Hrdina wrote:
> > Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> > ---
> > src/conf/domain_conf.c | 39 ++++++++++++++++++++++++++-------------
> > 1 file changed, 26 insertions(+), 13 deletions(-)
> > 
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index bb4be5d1cd..8fe79f70bf 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> > @@ -11040,7 +11063,8 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
> >                 goto error;
> >             }
> >             protocolParsed = true;
> > -            protocol = virXMLPropString(cur, "type");
> > +            if (virDomainChrSourceDefParseProtocol(def, cur) < 0)
> > +                goto error;
> >         }
> >     }
> > 
> > @@ -11151,16 +11175,6 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
> >             }
> >             def->data.tcp.tlsFromConfig = !!tmp;
> >         }
> > -
> > -        if (!protocol)
> > -            def->data.tcp.protocol = VIR_DOMAIN_CHR_TCP_PROTOCOL_RAW;
> 
> This removes the explicit assignment of VIR_DOMAIN_CHR_TCP_PROTOCOL_RAW
> if no protocol node has been seen.
> 
> The most direct equivalent would be:
>    if (!protocolParsed)
> but I would also be okay with (in the order of preference)
> 1. initializing it before we start parsing the node

This would require to check for the chardev type and I'm not a fan of
that.

> 2. adding a _DEFAULT enum value and changing it in PostParse

This is probably for a follow-up patch and the preferred way.

> 3. explicitly assigning VIR_DOMAIN_CHR_TCP_PROTOCOL_RAW = 0
> and letting calloc do the initialization

I'll go with this option since it's the easiest one :)

> ACK with that fixed

Thanks, I'll push the series shortly.

Pavel
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list