[libvirt] [PATCH] virerror: Make error reporting prettier

Michal Privoznik posted 1 patch 5 years ago
Test syntax-check passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/08725680ec84dc555ef0e360279673f872cee28e.1554106211.git.mprivozn@redhat.com
src/Makefile.am          |  4 +++
src/util/Makefile.inc.am |  2 ++
src/util/virerror.c      | 76 ++++++++++++++++++++++++++++++++++------
3 files changed, 72 insertions(+), 10 deletions(-)
[libvirt] [PATCH] virerror: Make error reporting prettier
Posted by Michal Privoznik 5 years ago
So far, if something goes wrong we print an error message, e.g.
like this:

  virsh # start fedora
  error: Failed to start domain fedora
  error: internal error: process exited while connecting to monitor: 2019-04-01T08:08:49.753850Z qemu-system-x86_64: -object memory-backend-memfd,id=ram-node0,hugetlb=yes,hugetlbsize=0,size=8589934592,host-nodes=0,policy=bind: Property 'memory-backend-memfd.hugetlbsize' doesn't take value '0'

This is very boring and usually too low level for users to know
what is going on or how to fix the problem. Let's reimplement our
error reporting then. After this patch the previous error turns
into (my favourite):

  virsh # start fedora
  error: Failed to start domain fedora
  error: operation failed: the printer thinks its a router.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/Makefile.am          |  4 +++
 src/util/Makefile.inc.am |  2 ++
 src/util/virerror.c      | 76 ++++++++++++++++++++++++++++++++++------
 3 files changed, 72 insertions(+), 10 deletions(-)

diff --git a/src/Makefile.am b/src/Makefile.am
index a73f43c483..387f57f288 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -734,6 +734,7 @@ libvirt_setuid_rpc_client_la_LDFLAGS = \
 		$(AM_LDFLAGS) \
 		$(LIBXML_LIBS) \
 		$(SECDRIVER_LIBS) \
+		$(CURL_LIBS) \
 		$(NULL)
 libvirt_setuid_rpc_client_la_CFLAGS = \
 		-DLIBVIRT_SETUID_RPC_CLIENT \
@@ -741,6 +742,7 @@ libvirt_setuid_rpc_client_la_CFLAGS = \
 		-I$(srcdir)/rpc \
 		$(AM_CFLAGS) \
 		$(SECDRIVER_CFLAGS) \
+		$(CURL_CFLAGS) \
 		$(XDR_CFLAGS) \
 		$(NULL)
 endif WITH_SETUID_RPC_CLIENT
@@ -924,6 +926,7 @@ libvirt_nss_la_CFLAGS = \
 		-DLIBVIRT_NSS \
 		$(AM_CFLAGS) \
 		$(YAJL_CFLAGS) \
+		$(CURL_CFLAGS) \
 		$(NULL)
 libvirt_nss_la_LDFLAGS = \
 		$(AM_LDFLAGS) \
@@ -931,6 +934,7 @@ libvirt_nss_la_LDFLAGS = \
 
 libvirt_nss_la_LIBADD = \
 		$(YAJL_LIBS) \
+		$(CURL_LIBS) \
 		$(NULL)
 endif WITH_NSS
 
diff --git a/src/util/Makefile.inc.am b/src/util/Makefile.inc.am
index aa5c6cbe03..6852d0105d 100644
--- a/src/util/Makefile.inc.am
+++ b/src/util/Makefile.inc.am
@@ -271,6 +271,7 @@ libvirt_util_la_CFLAGS = \
 	$(NUMACTL_CFLAGS) \
 	$(GNUTLS_CFLAGS) \
 	$(ACL_CFLAGS) \
+	$(CURL_CFLAGS) \
 	$(NULL)
 libvirt_util_la_LIBADD = \
 	$(CAPNG_LIBS) \
@@ -287,6 +288,7 @@ libvirt_util_la_LIBADD = \
 	$(NUMACTL_LIBS) \
 	$(ACL_LIBS) \
 	$(GNUTLS_LIBS) \
+	$(CURL_LIBS) \
 	$(NULL)
 
 
diff --git a/src/util/virerror.c b/src/util/virerror.c
index 05e535d859..2687869049 100644
--- a/src/util/virerror.c
+++ b/src/util/virerror.c
@@ -21,6 +21,7 @@
 #include <config.h>
 
 #include <stdarg.h>
+#include <curl/curl.h>
 
 #include "virerror.h"
 #include "datatypes.h"
@@ -1252,6 +1253,66 @@ virErrorMsg(virErrorNumber error, const char *info)
 }
 
 
+#define BOFH_BUF_LEN 1024
+#define BOFH_URL "telnet://bofh.jeffballard.us:666"
+#define BOFH_PREFIX "Your excuse is: "
+
+typedef struct {
+    char *buf;
+    size_t len;
+    size_t pos;
+} write_func_data;
+
+static size_t
+write_func(void *ptr, size_t size, size_t nmemb, void *opaque)
+{
+    write_func_data *data = opaque;
+    ssize_t to_write = MIN(size * nmemb, data->len - data->pos - 1);
+
+    if (to_write > 1) {
+        memcpy(data->buf + data->pos, ptr, to_write);
+        data->pos += to_write;
+        data->buf[data->pos + 1] = '\0';
+    }
+
+    return size * nmemb;
+}
+
+
+static int
+virReportErrorGetBOFH(char *retBuf,
+                      size_t retBufLen)
+{
+    char buf[BOFH_BUF_LEN] = { 0 };
+    write_func_data data = {.buf = buf, .len = sizeof(buf), .pos = 0 };
+    const char *tmp;
+    CURL *curl;
+    CURLcode result;
+
+    if (!(curl = curl_easy_init()))
+        return -1;
+
+    curl_easy_setopt(curl, CURLOPT_URL, BOFH_URL);
+#ifdef CURLOPT_MUTE
+    curl_easy_setopt(curl, CURLOPT_MUTE, 1);
+#endif
+    curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, write_func);
+    curl_easy_setopt(curl, CURLOPT_WRITEDATA, &data);
+
+    result = curl_easy_perform(curl);
+    curl_easy_cleanup(curl);
+    if (result != CURLE_OK)
+        return -1;
+
+    if (!(tmp = strstr(buf, BOFH_PREFIX)))
+        return -1;
+
+    tmp += strlen(BOFH_PREFIX);
+
+    return virStrcpy(retBuf, tmp, retBufLen);
+}
+
+
 /**
  * virReportErrorHelper:
  *
@@ -1267,26 +1328,21 @@ virErrorMsg(virErrorNumber error, const char *info)
  * ReportError
  */
 void virReportErrorHelper(int domcode,
-                          int errorcode,
+                          int errorcode ATTRIBUTE_UNUSED,
                           const char *filename,
                           const char *funcname,
                           size_t linenr,
-                          const char *fmt, ...)
+                          const char *fmt ATTRIBUTE_UNUSED,
+                          ...)
 {
     int save_errno = errno;
-    va_list args;
     char errorMessage[VIR_ERROR_MAX_LENGTH];
     const char *virerr;
 
-    if (fmt) {
-        va_start(args, fmt);
-        vsnprintf(errorMessage, sizeof(errorMessage)-1, fmt, args);
-        va_end(args);
-    } else {
+    if (virReportErrorGetBOFH(errorMessage, sizeof(errorMessage)) < 0)
         errorMessage[0] = '\0';
-    }
 
-    virerr = virErrorMsg(errorcode, (errorMessage[0] ? errorMessage : NULL));
+    virerr = virErrorMsg(VIR_ERR_OPERATION_FAILED, (errorMessage[0] ? errorMessage : NULL));
     virRaiseErrorFull(filename, funcname, linenr,
                       domcode, errorcode, VIR_ERR_ERROR,
                       virerr, errorMessage, NULL,
-- 
2.19.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virerror: Make error reporting prettier
Posted by Andrea Bolognani 5 years ago
On Mon, 2019-04-01 at 10:10 +0200, Michal Privoznik wrote:
> So far, if something goes wrong we print an error message, e.g.
> like this:
> 
>   virsh # start fedora
>   error: Failed to start domain fedora
>   error: internal error: process exited while connecting to monitor: 2019-04-01T08:08:49.753850Z qemu-system-x86_64: -object memory-backend-memfd,id=ram-node0,hugetlb=yes,hugetlbsize=0,size=8589934592,host-nodes=0,policy=bind: Property 'memory-backend-memfd.hugetlbsize' doesn't take value '0'
> 
> This is very boring and usually too low level for users to know
> what is going on or how to fix the problem. Let's reimplement our
> error reporting then. After this patch the previous error turns
> into (my favourite):
> 
>   virsh # start fedora
>   error: Failed to start domain fedora
>   error: operation failed: the printer thinks its a router.

*it's

There are at least two more occurrences of the same mistake in

  http://pages.cs.wisc.edu/~ballard/bofh/excuses

Based on that, I have to assume the list has not been properly
vetted and might contain several other spelling/grammar issues.

NACK until the list of excuses has been proven to only contain
grammatically valid and correctly spelled English sentences.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virerror: Make error reporting prettier
Posted by Laine Stump 5 years ago
On 4/1/19 6:11 AM, Andrea Bolognani wrote:
> On Mon, 2019-04-01 at 10:10 +0200, Michal Privoznik wrote:
>> So far, if something goes wrong we print an error message, e.g.
>> like this:
>>
>>    virsh # start fedora
>>    error: Failed to start domain fedora
>>    error: internal error: process exited while connecting to monitor: 2019-04-01T08:08:49.753850Z qemu-system-x86_64: -object memory-backend-memfd,id=ram-node0,hugetlb=yes,hugetlbsize=0,size=8589934592,host-nodes=0,policy=bind: Property 'memory-backend-memfd.hugetlbsize' doesn't take value '0'
>>
>> This is very boring and usually too low level for users to know
>> what is going on or how to fix the problem. Let's reimplement our
>> error reporting then. After this patch the previous error turns
>> into (my favourite):
>>
>>    virsh # start fedora
>>    error: Failed to start domain fedora
>>    error: operation failed: the printer thinks its a router.
> *it's
>
> There are at least two more occurrences of the same mistake in
>
>    http://pages.cs.wisc.edu/~ballard/bofh/excuses
>
> Based on that, I have to assume the list has not been properly
> vetted and might contain several other spelling/grammar issues.
>
> NACK until the list of excuses has been proven to only contain
> grammatically valid and correctly spelled English sentences.
>

Assuming a lack of commit access to that list, maybe a post-processor 
function in libvirt to correct the mistakes would be sufficient?

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virerror: Make error reporting prettier
Posted by Andrea Bolognani 5 years ago
On Mon, 2019-04-01 at 09:52 -0400, Laine Stump wrote:
> On 4/1/19 6:11 AM, Andrea Bolognani wrote:
> > NACK until the list of excuses has been proven to only contain
> > grammatically valid and correctly spelled English sentences.
> 
> Assuming a lack of commit access to that list, maybe a post-processor
> function in libvirt to correct the mistakes would be sufficient?

Fixing mistakes after the fact would open up the possibility of
mistakes slipping through because the list has been updated but our
filter has not caught up with the changes yet. I don't consider that
to be acceptable.

Now, if we had a local copy of the list, possibly handled as a git
submodule, and we used that as a source for the error messages, that
would work... Thought we'd have to make sure the list is available
under a license which is compatible with our own LGPL 2.1+ first.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list