Re: [PATCH] vmx: Check serialX.vspc before serialX.fileName

On Thu, Apr 25, 2024 at 14:12:54 +0200, Martin Kletzander wrote: > When using vSPC (Virtual Serial Port Concentrator) in vSphere the actual > address for it is saved in serialX.vspc in which case the > serialX.fileName is most probably something we can't get any useful > information from and we also fail during the parsing rendering any > dumpxml and similar tries unsuccessful. > > Instead of parsing the vspc URL with something along the lines of > `virURIParse(vspc ? vspc : fileName)`, which could lead to us reporting > information that is very prune to misuse (the vSPC seemingly has a > protocol on top of the telnet connection; redefining the domain would > change the behaviour; the URL might have a fragment we are not saving; > etc.) or adding more XML knobs to indicate vSPC usage (which we would > not be able to configure; we'd have to properly error out everywhere; > etc.) let's just report dummy serial port that leads to nowhere. > > Resolves: https://issues.redhat.com/browse/RHEL-32182 > Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; > --- > src/vmx/vmx.c | 12 +++ > tests/vmx2xmldata/esx-in-the-wild-13.vmx | 97 ++++++++++++++++++++++++ > tests/vmx2xmldata/esx-in-the-wild-13.xml | 55 ++++++++++++++ > tests/vmx2xmltest.c | 1 + > 4 files changed, 165 insertions(+) > create mode 100644 tests/vmx2xmldata/esx-in-the-wild-13.vmx > create mode 100644 tests/vmx2xmldata/esx-in-the-wild-13.xml > > diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c > index 5da67aae60d9..32074f62e14c 100644 > --- a/src/vmx/vmx.c > +++ b/src/vmx/vmx.c > @@ -2975,6 +2975,9 @@ virVMXParseSerial(virVMXContext *ctx, virConf *conf, int port, > char fileName_name[48] = ""; > g_autofree char *fileName = NULL; > > + char vspc_name[48] = ""; > + g_autofree char *vspc = NULL; > + > char network_endPoint_name[48] = ""; > g_autofree char *network_endPoint = NULL; > > @@ -2997,6 +3000,7 @@ virVMXParseSerial(virVMXContext *ctx, virConf *conf, int port, > VMX_BUILD_NAME(startConnected); > VMX_BUILD_NAME(fileType); > VMX_BUILD_NAME(fileName); > + VMX_BUILD_NAME(vspc); > VMX_BUILD_NAME_EXTRA(network_endPoint, "network.endPoint"); > > /* vmx:present */ > @@ -3026,6 +3030,10 @@ virVMXParseSerial(virVMXContext *ctx, virConf *conf, int port, > if (virVMXGetConfigString(conf, fileName_name, &fileName, true) < 0) > goto cleanup; > > + /* vmx:fileName -> def:data.file.path */ > + if (virVMXGetConfigString(conf, vspc_name, &vspc, true) < 0) > + goto cleanup; > + > /* vmx:network.endPoint -> def:data.tcp.listen */ > if (virVMXGetConfigString(conf, network_endPoint_name, &network_endPoint, > true) < 0) { > @@ -3057,6 +3065,10 @@ virVMXParseSerial(virVMXContext *ctx, virConf *conf, int port, > (*def)->target.port = port; > (*def)->source->type = VIR_DOMAIN_CHR_TYPE_PIPE; > (*def)->source->data.file.path = g_steal_pointer(&fileName); > + } else if (STRCASEEQ(fileType, "network") && vspc) { > + (*def)->target.port = port; > + (*def)->source->type = VIR_DOMAIN_CHR_TYPE_FILE; Wouldn't a _TYPE_NULL be more reasonable in this case? Or is the intention to actually have a path?