On 2/27/26 1:07 PM, Jonathon Jongsma via Devel wrote:
The current error message results in something like the following when
running `virsh define` for an existing domain:
`domain Domain already exists with UUID '$UUID' exists already`
Improve the error message and make it behave like the esx driver and
indicate that we do not yet support redefining existing domains in hyperv.
Also avoid using the public LookupByUUID() API to check for existance,
which requires unnecessarily allocating and de-allocating a virDomainPtr
object.
Signed-off-by: Jonathon Jongsma <[email protected]>
The optimization makes sense, and the old error message was pretty
bad[*] but I think it would make sense to still have the domain name and
uuid included in the error log, rather than just a generic message with
no identifying info.
(forgive any naivety in the following - this is the first time in all
these years I've taken a close look at virerror.c - before this I always
just used it)
[*] The horrible grammar of the existing error message is really caused
by the fact that VIR_ERR_DOM_EXIST (which has been in the list of error
types since the beginning, but is only used by the hyperv driver, and
hasn't ever been used by *any other driver* ever since support for
DomainDefineXML was added to the Xen driver in 2006!!) expects that the
string passed into the error logger is just the name of the domain (not
an entire descriptive text), and that string is embedded into the string
"domain %1$s exists already". This is one of only 4 VIR_ERR_* types that
embeds the string from the caller into the *middle* of an existing
string (rather than at the end), and in 3 of the 4 cases the error
reporting uses it incorrectly (ie assuming they should send an entire
sentence fragment), resulting in similarly awkward error messages (only
the uses of VIR_ERR_STORAGE_VOL_EXIST properly pass only the name of the
storage volume rather than a complete string, resulting in a coherent
message). The fact that nearly all existing usages of the VIR_ERR_* type
that put the passed argument into the middle of the
virErrorMsgTuple.msginfo string are doing it wrong makes me wonder if we
should change this to *always* be
[error-type-specific-string]: [reported-error-string]
(i.e. append the passed argument at the end of the msginfo string after
a ": ", since that's what everyone assumes will happen anyway). Does
anyone have an opinion about this? (For that matter, just how useful is
the error type anyway? It looks like it's only use is 1) to determine
the "error-type-specific-string" (which is almost always a preamble of
the string that is logged) and 2) if an "ErrorLogPriorityFunc" is set
(with virSetErrorLogPriorityFunc()) then the error type can be used to
modify the error level. (2) appears to only be done by remote_daemon.c
to reduce the level of many error types to DEBUG (so they don't 'clog up
the log'), but seems to be otherwise unset. Am I missing something here?
Otherwise it all seems to be a lot of extra complication for little
extra utility (especially since, as evidenced by the disuse of
VIR_ERR_DOM_EXIST as an example, the error types may give someone who
does find a use for the error types a false sense that they carry
accurate information).
---
src/hyperv/hyperv_driver.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c
index b01b4919fe..6e9917f92a 100644
--- a/src/hyperv/hyperv_driver.c
+++ b/src/hyperv/hyperv_driver.c
@@ -2933,6 +2933,8 @@ hypervDomainDefineXML(virConnectPtr conn, const char *xml)
virDomainPtr domain = NULL;
g_autoptr(hypervInvokeParamsList) params = NULL;
g_autoptr(GHashTable) defineSystemParam = NULL;
+ g_autoptr(Msvm_ComputerSystem) existing = NULL;
+ char uuid_string[VIR_UUID_STRING_BUFLEN];
size_t i = 0;
/* parse xml */
@@ -2943,13 +2945,10 @@ hypervDomainDefineXML(virConnectPtr conn, const char
*xml)
goto error;
/* abort if a domain with this UUID already exists */
- if ((domain = hypervDomainLookupByUUID(conn, def->uuid))) {
- char uuid_string[VIR_UUID_STRING_BUFLEN];
- virUUIDFormat(domain->uuid, uuid_string);
- virReportError(VIR_ERR_DOM_EXIST, _("Domain already exists with UUID
'%1$s'"), uuid_string);
-
- // Don't use the 'exit' label, since we don't want to delete the
existing domain.
- virObjectUnref(domain);
+ virUUIDFormat(def->uuid, uuid_string);
+ if (hypervMsvmComputerSystemFromUUID(priv, uuid_string, &existing) == 0 &&
existing != NULL) {
+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+ _("Domain already exists, editing existing domains is not
supported yet"));
return NULL;
}
