Hey, On Sat, Dec 01, 2018 at 09:54:21AM +0100, Fabiano Fidêncio wrote: > There are errors which are not fatal and just ignored in load_keyinfo. > However, as those have not been cleaned up, we could see messages like: > (lt-osinfo-detect:20658): GLib-WARNING **: GError set over the top of a > previous GError or uninitialized memory. > This indicates a bug in someone's code. You must ensure an error is NULL > before it's set. > The overwriting error message was: Key file does not have key “boot.iso” > in group “images-x86_64” > > In order to avoid this, let's just call g_clear_error() after an > situations where an error may have been set but it can just be ignored. > > Signed-off-by: Fabiano Fidêncio <[email protected]> > --- > osinfo/osinfo_tree.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/osinfo/osinfo_tree.c b/osinfo/osinfo_tree.c > index da01c8b..3082eab 100644 > --- a/osinfo/osinfo_tree.c > +++ b/osinfo/osinfo_tree.c > @@ -491,26 +491,31 @@ static OsinfoTree *load_keyinfo(const gchar *location, > if (!g_key_file_load_from_data(file, content, length, > G_KEY_FILE_NONE, error)) > goto cleanup; > + g_clear_error(error);
I don't think we'll be getting a non-NULL *error when
g_key_file_load_from_data() returns success, so this g_clear_error() is
a bit odd. I'd remove it, or use a g_warn_if_fail() if this could really
happen.
>
> if (!(family = g_key_file_get_string(file, "general", "family", error))
> &&
> (*error && (*error)->code != G_KEY_FILE_ERROR_KEY_NOT_FOUND &&
> (*error)->code != G_KEY_FILE_ERROR_GROUP_NOT_FOUND))
> goto cleanup;
> + g_clear_error(error);
3 slight imperfections with that code (all theoritical issues):
- if g_key_file_get_string() is sucessful, we still attempt to clean-up
the GError, I don't think this should ever happen
- the existing code is not checking the error domain, so if
g_key_file_get_string was returning errors from different domains, we
could mistakenly ignore errors
- if family is NULL but *error is NULL too, we'd ignore the failure, I
think this is an oversight in the way the code is written
Maybe something like this patch would be better:
(potentially split in a preparatory one + your patch on top)
diff --git a/osinfo/osinfo_tree.c b/osinfo/osinfo_tree.c
index da01c8ba..e1d86caa 100644
--- a/osinfo/osinfo_tree.c
+++ b/osinfo/osinfo_tree.c
@@ -472,6 +472,12 @@ static gboolean is_str_empty(const gchar *str) {
}
+static gboolean is_unknown_group_or_key_error(const GError *error)
+{
+ return (g_error_matches(error, G_KEY_FILE_ERROR,
G_KEY_FILE_ERROR_KEY_NOT_FOUND) ||
+ g_error_matches(error, G_KEY_FILE_ERROR,
G_KEY_FILE_ERROR_GROUP_NOT_FOUND));
+}
+
static OsinfoTree *load_keyinfo(const gchar *location,
const gchar *content,
gsize length,
@@ -488,48 +494,63 @@ static OsinfoTree *load_keyinfo(const gchar *location,
gchar *bootiso = NULL;
gchar *group = NULL;
+ g_return_val_if_fail(error != NULL, NULL);
+
if (!g_key_file_load_from_data(file, content, length,
G_KEY_FILE_NONE, error))
goto cleanup;
- if (!(family = g_key_file_get_string(file, "general", "family", error)) &&
- (*error && (*error)->code != G_KEY_FILE_ERROR_KEY_NOT_FOUND &&
- (*error)->code != G_KEY_FILE_ERROR_GROUP_NOT_FOUND))
- goto cleanup;
+ if (!(family = g_key_file_get_string(file, "general", "family", error))) {
+ if (!is_unknown_group_or_key_error(*error))
+ goto cleanup;
+ else
+ g_clear_error(error);
+ }
- if (!(variant = g_key_file_get_string(file, "general", "variant", error))
&&
- (*error && (*error)->code != G_KEY_FILE_ERROR_KEY_NOT_FOUND &&
- (*error)->code != G_KEY_FILE_ERROR_GROUP_NOT_FOUND))
- goto cleanup;
+ if (!(variant = g_key_file_get_string(file, "general", "variant", error)))
{
+ if (!is_unknown_group_or_key_error(*error))
+ goto cleanup;
+ else
+ g_clear_error(error);
+ }
- if (!(version = g_key_file_get_string(file, "general", "version", error))
&&
- (*error && (*error)->code != G_KEY_FILE_ERROR_KEY_NOT_FOUND &&
- (*error)->code != G_KEY_FILE_ERROR_GROUP_NOT_FOUND))
- goto cleanup;
-
- if (!(arch = g_key_file_get_string(file, "general", "arch", error)) &&
- (*error && (*error)->code != G_KEY_FILE_ERROR_KEY_NOT_FOUND &&
- (*error)->code != G_KEY_FILE_ERROR_GROUP_NOT_FOUND))
- goto cleanup;
+ if (!(version = g_key_file_get_string(file, "general", "version", error)))
{
+ if (!is_unknown_group_or_key_error(*error))
+ goto cleanup;
+ else
+ g_clear_error(error);
+ }
+ if (!(arch = g_key_file_get_string(file, "general", "arch", error))) {
+ if (!is_unknown_group_or_key_error(*error))
+ goto cleanup;
+ else
+ g_clear_error(error);
+ }
if (arch) {
group = g_strdup_printf("images-%s", arch);
- if (!(kernel = g_key_file_get_string(file, group, "kernel", error)) &&
- (*error && (*error)->code != G_KEY_FILE_ERROR_KEY_NOT_FOUND &&
- (*error)->code != G_KEY_FILE_ERROR_GROUP_NOT_FOUND))
- goto cleanup;
+ if (!(kernel = g_key_file_get_string(file, group, "kernel", error))) {
+ if (!is_unknown_group_or_key_error(*error))
+ goto cleanup;
+ else
+ g_clear_error(error);
+ }
- if (!(initrd = g_key_file_get_string(file, group, "initrd", error)) &&
- (*error && (*error)->code != G_KEY_FILE_ERROR_KEY_NOT_FOUND &&
- (*error)->code != G_KEY_FILE_ERROR_GROUP_NOT_FOUND))
- goto cleanup;
+ if (!(initrd = g_key_file_get_string(file, group, "initrd", error))) {
+ if (!is_unknown_group_or_key_error(*error))
+ goto cleanup;
+ else
+ g_clear_error(error);
+ }
- if (!(bootiso = g_key_file_get_string(file, group, "boot.iso", error))
&&
- (*error && (*error)->code != G_KEY_FILE_ERROR_KEY_NOT_FOUND &&
- (*error)->code != G_KEY_FILE_ERROR_GROUP_NOT_FOUND))
- goto cleanup;
+ if (!(bootiso = g_key_file_get_string(file, group, "boot.iso",
error))) {
+ if (!is_unknown_group_or_key_error(*error))
+ goto cleanup;
+ else
+ g_clear_error(error);
+ }
}
tree = osinfo_tree_new(location, arch ? arch : "i386");
signature.asc
Description: PGP signature
_______________________________________________ Libosinfo mailing list [email protected] https://www.redhat.com/mailman/listinfo/libosinfo
