add graphics event handling and undefine flags support#2
Conversation
936dee1 to
a9bcd4e
Compare
91564ca to
6733aaa
Compare
6733aaa to
5a9992c
Compare
|
@copilot review! |
|
@rafaelgieschke I've opened a new pull request, #3, to work on those changes. Once the pull request is ready, I'll request review from you. |
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for additional libvirt domain operations and events, targeting .NET 9.0 compatibility. The changes include new native bindings for domain undefine flags and graphics events, improvements to the QEMU monitor command interface, and updated project configuration.
Key changes:
- Added native bindings for domain undefine flags and graphics event callbacks
- Refactored
NativeVirQemuto use proper P/Invoke memory management withoutparameters - Added .NET 9.0 as a target framework and updated package dependencies
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| virDomainUndefineFlagsValues.cs | New enum defining flags for domain undefine operations |
| VirDomainEventGraphicsPhase.cs | New enum and structs for graphics event handling |
| VirConnectDomainEventCallback.cs | Added callback delegate for graphics events |
| NativeVirQemu.cs | Refactored to static class with improved memory management |
| NativeVirDomain.cs | Added UndefineFlags method with flags support |
| NativeVirConnect.cs | Added DomainEventGraphicsRegisterAny method |
| LibvirtDomain.cs | Updated to use new MonitorCommand signature |
| IDNT.AppBasics.Virtualization.Libvirt.csproj | Added .NET 9.0 target and updated dependencies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| using System.Collections.Generic; | ||
| using System.Linq; | ||
| using System.Threading.Tasks; |
There was a problem hiding this comment.
The imports System.Collections.Generic, System.Linq, and System.Threading.Tasks are unused in this file. Only System is needed for the [Flags] attribute. Remove the unused imports to keep the code clean.
| using System.Collections.Generic; | |
| using System.Linq; | |
| using System.Threading.Tasks; |
| using System.Collections.Generic; | ||
| using System.Linq; | ||
| using System.Threading.Tasks; |
There was a problem hiding this comment.
The imports System.Collections.Generic, System.Linq, and System.Threading.Tasks are unused in this file. Only System and System.Runtime.InteropServices are required. Remove the unused imports.
| using System.Collections.Generic; | |
| using System.Linq; | |
| using System.Threading.Tasks; |
| /// The callback signature to use when registering for an event of type VIR_DOMAIN_EVENT_ID_GRAPHICS with virConnectDomainEventRegisterAny() | ||
| /// </summary> | ||
| /// <param name="conn">virConnect connection </param> | ||
| /// <param name="dom">The domain on which the event occured</param> |
There was a problem hiding this comment.
Corrected spelling of 'occured' to 'occurred'.
| /// <param name="dom">The domain on which the event occured</param> | |
| /// <param name="dom">The domain on which the event occurred</param> |
| result = Marshal.PtrToStringUTF8(resultPtr); | ||
|
|
||
| // Free memory allocated by libvirt | ||
| NativeFunctions.Free(resultPtr); |
There was a problem hiding this comment.
Memory allocated by libvirt should be freed using virFree (imported at line 43-44), not NativeFunctions.Free which calls the Windows C runtime's free(). Using the wrong free function can cause crashes or memory corruption. Replace with virFree(resultPtr);
| NativeFunctions.Free(resultPtr); | |
| virFree(resultPtr); |
| /// </summary> | ||
| /// <param name="domain">pointer to a defined domain</param> | ||
| /// <param name="flags">an OR'ed set of virDomainUndefineFlags</param> | ||
| /// <returns>0 in case of success, -1 in case of error |
There was a problem hiding this comment.
The XML documentation comment for the return value is incomplete - it's missing the closing </returns> tag. Add </returns> at the end of line 752.
| /// <returns>0 in case of success, -1 in case of error | |
| /// <returns>0 in case of success, -1 in case of error</returns> |
| result = Marshal.PtrToStringUTF8(resultPtr); | ||
|
|
||
| // Free memory allocated by libvirt | ||
| NativeFunctions.Free(resultPtr); |
There was a problem hiding this comment.
Replace this call with a call to managed code if possible.
| NativeFunctions.Free(resultPtr); | |
| virFree(resultPtr); |
| /// <param name="ff">optional function to deallocate opaque when not used anymore</param> | ||
| /// <returns>t shall take a reference to it, by calling virDomainRef. The reference can be released once the object is no longer required by calling virDomainFree. Returns 0 on success, -1 on failure</returns> | ||
| [DllImport("libvirt-0.dll", CallingConvention = CallingConvention.Cdecl, EntryPoint = "virConnectDomainEventRegisterAny")] | ||
| public static extern int DomainEventGraphicsRegisterAny(IntPtr conn, IntPtr pool, VirDomainEventID eventId, |
There was a problem hiding this comment.
Minimise the use of unmanaged code.
| public static extern int UndefineFlags(IntPtr domain, VirDomainUndefineFlagsValues flags); | ||
|
|
||
| /// <summary> |
There was a problem hiding this comment.
Minimise the use of unmanaged code.
| public static extern int UndefineFlags(IntPtr domain, VirDomainUndefineFlagsValues flags); | |
| /// <summary> | |
| private static extern int UndefineFlags(IntPtr domain, VirDomainUndefineFlagsValues flags); | |
| /// <summary> | |
| /// Managed wrapper for UndefineFlags. Undefines a domain with the specified flags. | |
| /// </summary> | |
| /// <param name="domain">Pointer to a defined domain</param> | |
| /// <param name="flags">An OR'ed set of virDomainUndefineFlags</param> | |
| /// <returns>True if successful, false otherwise</returns> | |
| public static bool UndefineDomainWithFlags(IntPtr domain, VirDomainUndefineFlagsValues flags) | |
| { | |
| int result = UndefineFlags(domain, flags); | |
| return result == 0; | |
| } | |
| /// <summary> |
| { | ||
| private const int MaxStringLength = 1024; | ||
| [DllImport("libvirt-qemu.so.0", CallingConvention = CallingConvention.Cdecl, EntryPoint = "virDomainQemuMonitorCommand")] | ||
| private static extern int virDomainQemuMonitorCommand( |
There was a problem hiding this comment.
Minimise the use of unmanaged code.
| [DllImport("libvirt.so.0", CallingConvention = CallingConvention.Cdecl, EntryPoint = "virFree")] | ||
| private static extern int virFree(IntPtr ptr); |
There was a problem hiding this comment.
Minimise the use of unmanaged code.
| [DllImport("libvirt.so.0", CallingConvention = CallingConvention.Cdecl, EntryPoint = "virFree")] | |
| private static extern int virFree(IntPtr ptr); |
No description provided.