[PATCH] libxl: Implement domainGetMessages API

Jim Fehlig posted 1 patch 2 years, 3 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20211214003536.32731-1-jfehlig@suse.com
src/libxl/libxl_driver.c | 24 ++++++++++++++++++++++++
1 file changed, 24 insertions(+)
[PATCH] libxl: Implement domainGetMessages API
Posted by Jim Fehlig 2 years, 3 months ago
Since commit 46783e6307a, the 'virsh dominfo' command calls
virDomainGetMessages to report any messages from the domain.
Hypervisors not implementing the API now get the following
log message when clients invoke 'virsh dominfo'

this function is not supported by the connection driver: virDomainGetMessages

Although libxl currently does not support any tainting or
deprecation messages, provide an implementation to squelch
the previously unseen error message when collecting dominfo.

Signed-off-by: Jim Fehlig <jfehlig@suse.com>
---
 src/libxl/libxl_driver.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index bc8598ea96..2d9385654c 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -6385,6 +6385,29 @@ libxlDomainGetMetadata(virDomainPtr dom,
     return ret;
 }
 
+static int
+libxlDomainGetMessages(virDomainPtr dom,
+                      char ***msgs,
+                      unsigned int flags)
+{
+    virDomainObj *vm = NULL;
+    int ret = -1;
+
+    virCheckFlags(0, -1);
+
+    if (!(vm = libxlDomObjFromDomain(dom)))
+        return -1;
+
+    if (virDomainGetMessagesEnsureACL(dom->conn, vm->def) < 0)
+        goto cleanup;
+
+    ret = virDomainObjGetMessages(vm, msgs, flags);
+
+ cleanup:
+    virDomainObjEndAPI(&vm);
+    return ret;
+}
+
 static virHypervisorDriver libxlHypervisorDriver = {
     .name = LIBXL_DRIVER_EXTERNAL_NAME,
     .connectURIProbe = libxlConnectURIProbe,
@@ -6498,6 +6521,7 @@ static virHypervisorDriver libxlHypervisorDriver = {
     .connectBaselineCPU = libxlConnectBaselineCPU, /* 2.3.0 */
     .domainSetMetadata = libxlDomainSetMetadata, /* 5.7.0 */
     .domainGetMetadata = libxlDomainGetMetadata, /* 5.7.0 */
+    .domainGetMessages = libxlDomainGetMessages, /* 8.0.0 */
 
 };
 
-- 
2.34.1


Re: [PATCH] libxl: Implement domainGetMessages API
Posted by Michal Prívozník 2 years, 3 months ago
On 12/14/21 01:35, Jim Fehlig wrote:
> Since commit 46783e6307a, the 'virsh dominfo' command calls
> virDomainGetMessages to report any messages from the domain.
> Hypervisors not implementing the API now get the following
> log message when clients invoke 'virsh dominfo'
> 
> this function is not supported by the connection driver: virDomainGetMessages
> 
> Although libxl currently does not support any tainting or
> deprecation messages, provide an implementation to squelch
> the previously unseen error message when collecting dominfo.
> 
> Signed-off-by: Jim Fehlig <jfehlig@suse.com>
> ---
>  src/libxl/libxl_driver.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

Michal

Re: [PATCH] libxl: Implement domainGetMessages API
Posted by Peter Krempa 2 years, 3 months ago
On Mon, Dec 13, 2021 at 17:35:36 -0700, Jim Fehlig wrote:
> Since commit 46783e6307a, the 'virsh dominfo' command calls
> virDomainGetMessages to report any messages from the domain.
> Hypervisors not implementing the API now get the following
> log message when clients invoke 'virsh dominfo'
> 
> this function is not supported by the connection driver: virDomainGetMessages
> 
> Although libxl currently does not support any tainting or
> deprecation messages, provide an implementation to squelch
> the previously unseen error message when collecting dominfo.

So you are fixing a symptom, but IMO the proper fix is to just make
virsh ignore errors when virDomainGetMessages is not supported.

I have nothing against the code itself, but I wouldn't describe it as a
fix for virsh showing an error.

> 
> Signed-off-by: Jim Fehlig <jfehlig@suse.com>
> ---
>  src/libxl/libxl_driver.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)

Re: [PATCH] libxl: Implement domainGetMessages API
Posted by Peter Krempa 2 years, 3 months ago
On Tue, Dec 14, 2021 at 10:05:51 +0100, Peter Krempa wrote:
> On Mon, Dec 13, 2021 at 17:35:36 -0700, Jim Fehlig wrote:
> > Since commit 46783e6307a, the 'virsh dominfo' command calls
> > virDomainGetMessages to report any messages from the domain.
> > Hypervisors not implementing the API now get the following
> > log message when clients invoke 'virsh dominfo'
> > 
> > this function is not supported by the connection driver: virDomainGetMessages
> > 
> > Although libxl currently does not support any tainting or
> > deprecation messages, provide an implementation to squelch
> > the previously unseen error message when collecting dominfo.
> 
> So you are fixing a symptom, but IMO the proper fix is to just make
> virsh ignore errors when virDomainGetMessages is not supported.

Okay, so I misread that because the error message you've posted here
doesn't look like message from the log. Spamming logs is not good, so
as said it's okay.

Re: [PATCH] libxl: Implement domainGetMessages API
Posted by Daniel P. Berrangé 2 years, 3 months ago
On Tue, Dec 14, 2021 at 10:05:26AM +0100, Peter Krempa wrote:
> On Mon, Dec 13, 2021 at 17:35:36 -0700, Jim Fehlig wrote:
> > Since commit 46783e6307a, the 'virsh dominfo' command calls
> > virDomainGetMessages to report any messages from the domain.
> > Hypervisors not implementing the API now get the following
> > log message when clients invoke 'virsh dominfo'
> > 
> > this function is not supported by the connection driver: virDomainGetMessages
> > 
> > Although libxl currently does not support any tainting or
> > deprecation messages, provide an implementation to squelch
> > the previously unseen error message when collecting dominfo.
> 
> So you are fixing a symptom, but IMO the proper fix is to just make
> virsh ignore errors when virDomainGetMessages is not supported.
> 
> I have nothing against the code itself, but I wouldn't describe it as a
> fix for virsh showing an error.

I interpreted it as meaning that message appears in logs on
libvirtd side ?

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH] libxl: Implement domainGetMessages API
Posted by Peter Krempa 2 years, 3 months ago
On Tue, Dec 14, 2021 at 09:14:38 +0000, Daniel P. Berrangé wrote:
> On Tue, Dec 14, 2021 at 10:05:26AM +0100, Peter Krempa wrote:
> > On Mon, Dec 13, 2021 at 17:35:36 -0700, Jim Fehlig wrote:
> > > Since commit 46783e6307a, the 'virsh dominfo' command calls
> > > virDomainGetMessages to report any messages from the domain.
> > > Hypervisors not implementing the API now get the following
> > > log message when clients invoke 'virsh dominfo'
> > > 
> > > this function is not supported by the connection driver: virDomainGetMessages
> > > 
> > > Although libxl currently does not support any tainting or
> > > deprecation messages, provide an implementation to squelch
> > > the previously unseen error message when collecting dominfo.
> > 
> > So you are fixing a symptom, but IMO the proper fix is to just make
> > virsh ignore errors when virDomainGetMessages is not supported.
> > 
> > I have nothing against the code itself, but I wouldn't describe it as a
> > fix for virsh showing an error.
> 
> I interpreted it as meaning that message appears in logs on
> libvirtd side ?

Yeah, I got to that on a second read which I did after sending the
original reply and looking at the code. The commit message put too much
emphasis on virsh and mentions 'log' only once so that's why I missed
it.

Re: [PATCH] libxl: Implement domainGetMessages API
Posted by Jim Fehlig 2 years, 3 months ago
On 12/14/21 02:18, Peter Krempa wrote:
> On Tue, Dec 14, 2021 at 09:14:38 +0000, Daniel P. Berrangé wrote:
>> On Tue, Dec 14, 2021 at 10:05:26AM +0100, Peter Krempa wrote:
>>> On Mon, Dec 13, 2021 at 17:35:36 -0700, Jim Fehlig wrote:
>>>> Since commit 46783e6307a, the 'virsh dominfo' command calls
>>>> virDomainGetMessages to report any messages from the domain.
>>>> Hypervisors not implementing the API now get the following
>>>> log message when clients invoke 'virsh dominfo'
>>>>
>>>> this function is not supported by the connection driver: virDomainGetMessages
>>>>
>>>> Although libxl currently does not support any tainting or
>>>> deprecation messages, provide an implementation to squelch
>>>> the previously unseen error message when collecting dominfo.
>>>
>>> So you are fixing a symptom, but IMO the proper fix is to just make
>>> virsh ignore errors when virDomainGetMessages is not supported.
>>>
>>> I have nothing against the code itself, but I wouldn't describe it as a
>>> fix for virsh showing an error.
>>
>> I interpreted it as meaning that message appears in logs on
>> libvirtd side ?
> 
> Yeah, I got to that on a second read which I did after sending the
> original reply and looking at the code. The commit message put too much
> emphasis on virsh and mentions 'log' only once so that's why I missed
> it.

Yep, message appears in the libvirtd log. Should I make that clearer before 
pushing? E.g. change the last sentence of the opening paragraph to

"Hypervisors not implementing the API now get the following libvirtd log message 
when clients invoke 'virsh dominfo'."

Regards,
Jim