On 2/27/26 1:07 PM, Jonathon Jongsma via Devel wrote:

Right now if we run `virsh define domain.xml` multiple times, it will result in multiple domains being defined with the same name. This violates libvirt assumptions about name uniqueness, so prevent this from happening by returning an error.

There's not much we can do about vms that may have been created outside of libvirt that might have the same name (unless we switch to using something like the UUID as the name for hyperv domains, which would not be very user-friendly), but at least we can not contribute to the problem.

Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/hyperv/hyperv_driver.c | 7 +++++++ 1 file changed, 7 insertions(+)

diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c index 6e9917f92a..f717499ba9 100644 --- a/src/hyperv/hyperv_driver.c +++ b/src/hyperv/hyperv_driver.c @@ -2952,6 +2952,13 @@ hypervDomainDefineXML(virConnectPtr conn, const char *xml) return NULL; }

+ /* abort if a domain with this name already exists */ + if (hypervGetVirtualSystemByName(priv, def->name, &existing) == 0 && + existing != NULL) { + virReportError(VIR_ERR_DOM_EXIST, "%s", def->name);

HA!! So in your new use of VIR_ERR_DOM_EXIST you've used it correctly (just sending the name of the domain in the string). But now you've got two cases where the error condition is that a conflicting domain already exists, but one uses the specific error code and the other uses a generic error code. So what's the use of the error code then, if some cases of a "domain already exists" error don't use it? Anyway, that's a more bike shed discussion, and your patch does fix the problem at hand with the provided tools, so: Reviewed-by: Laine Stump <laine@redhat.com> (I do think the whole VIR_ERR_* thing should be revisited at some point though)

+ return NULL; + } + /* prepare params: only set the VM's name for now */ params = hypervCreateInvokeParamsList("DefineSystem", MSVM_VIRTUALSYSTEMMANAGEMENTSERVICE_SELECTOR,

On 3/1/26 10:27 PM, Laine Stump via Devel wrote:

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 <jjongsma@redhat.com>

BTW, as long as you add the domain name and uuid back to the error message: Reviewed-by: Laine Stump <laine@redhat.com> (all the rest was just me ranting about legacy cruft)

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;       }