Re: [libosinfo PATCH] tree: cleanup non-fatal errors

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 <fabiano@xxxxxxxxxxxx>
> ---
>  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");

Attachment: signature.asc
Description: PGP signature

_______________________________________________
Libosinfo mailing list
Libosinfo@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libosinfo

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Fedora Users]     [Fedora Maintainers]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]

  Powered by Linux