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

On Thu, Apr 25, 2024 at 05:30:24PM +0200, Peter Krempa wrote: >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? > It is not the intention, I simply haven't thought of a NULL type. I'll try to figure out what's the best way of keeping the serial in a way that it does work seamlessly with the main (only?) consumer - virt-v2v.