On Wed, Oct 17, 2012 at 08:17:17PM +0200, Miloslav Trmač wrote:
> When logging an error, don't throw away the detailed information.
> Example record when using the journald output:
>
> MESSAGE=Domain not found
> PRIORITY=4
> LIBVIRT_SOURCE=error
> CODE_FILE=../../src/test/test_driver.c
> CODE_LINE=1405
> CODE_FUNC=testLookupDomainByName
> DOMAIN=12
> CODE=42
> STR1=Domain not found
> STR2=
>
> The format used in other output destinations (e.g. "syslog", "file") is
> still unchanged.
>
> The "domain" and "code" numbers are part of the libvirt ABI in
> <libvirt/virterror.h>; therefore log processing tools can rely on them,
> unlike the text log string (which is translated depending on locale,
> and may be modified for other reasons as well).
>
> Alternatively, the "domain" and "code" fields could contain strings
> instead of numbers, but it's not clear that it's worth it:
> Advantages of numbers:
> * the numbers are shorter
> * the ABI guarantees that the numbers won't change
> Disadvantages of strings:
> * adding a ABI-stable string mapping for virErrorNumber would result
> in additional work each time a new error number is added
> (note that virErrorMsg cannot be used for this because it is
> translated)
> * a change in the string mapping would be less likely to be noticed
> The advantage of using strings is more readability, but note that the
> "msg" field above already contains a readable description of the
> error.
Agreed, the numeric values are the only part we want to consider
ABI stable. We have often changed the string values, even for
the error codes.
> @@ -676,10 +678,39 @@ virRaiseErrorFull(const char *filename ATTRIBUTE_UNUSED,
> priority = virErrorLevelPriority(level);
> if (virErrorLogPriorityFilter)
> priority = virErrorLogPriorityFilter(to, priority);
> +
> + i = 0;
> +#define META_ADD_STRING(KEY, VALUE) \
> + do { \
> + meta[i].key = (KEY); \
> + meta[i].s = (VALUE); \
> + i++; \
> + } while (0)
> +#define META_ADD_INT(KEY, VALUE) \
> + do { \
> + meta[i].key = (KEY); \
> + meta[i].s = NULL; \
> + meta[i].i = (VALUE); \
> + i++; \
> + } while (0)
> +
> + META_ADD_INT("DOMAIN", domain);
> + META_ADD_INT("CODE", code);
> + if (str1 != NULL)
> + META_ADD_STRING("STR1", str1);
> + if (str2 != NULL)
> + META_ADD_STRING("STR2", str2);
> + if (str3 != NULL)
> + META_ADD_STRING("STR3", str3);
> + if (int1 != -1)
> + META_ADD_INT("INT1", int1);
> + if (int2 != -1)
> + META_ADD_INT("INT2", int2);
At this point in time, I would like us to leave out the str1, str2,
str3, int1 and int2 values. These are a badly defined part of our
error reporting system. My goal is that for each error code, we
will define explicit semantics for the str1/str2/str3/int1/int2
fields (or defined them to be unused).
For example, with VIR_ERR_SYSTEM_ERROR, we'll declare that 'int1'
is always the errno values. Once we have this kind of stuff defined,
then we can make the structured error reports contain the appropriate
extra fields for the code in question. So for VIR_ERR_SYSTEM_ERROR,
the structured error log would contain an 'errno' field instead
of an 'int1' field.
Can you re-submit, just this patch in the series, with those parts
commented out or removed.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list