Re: [PATCH] gfxterm: check elements' properties and hadle errors.

Re: [PATCH] gfxterm: check elements' properties and hadle errors.

[Vladimir Testov]

[-- Attachment #1: Type: text/plain, Size: 682 bytes --]

>This is not a correct error handling. You should never return an error
>without calling grub_error. Also in all these contexts you handle the
>cases of unknown property name. I don't feel like refusing to load a
>theme because of unknown property is a good idea. This would i.a.
>exclude older GRUB versions for newer themes even if property in
>question is minor in meaning. Compare this to a browser refusing to show
>a page because of unknown tag attribute. Also GRUB_ERR_IO hardly
>describes a condition of unknown property.
Thanks for the reply. :) This subject is closed.
-- 
With best regards,
_______________________________
Vladimir Testov, ROSA Laboratory.
www.rosalab.ru

[-- Attachment #2: Type: text/html, Size: 3551 bytes --]

Re: [PATCH] gfxterm: check elements' properties and hadle errors.

[Vladimir Testov]

[-- Attachment #1: Type: text/plain, Size: 2027 bytes --]

>Again - what problem are you trying to solve? I added unknown option
>to theme element and it was loaded and displayed just fine. Why
>aborting theme loading in this case is better?

There is possibility that we can misprint some option. Or write some wrong value for some existing option.
I think it's better to know about it - so we can quickly find the place where we are wrong.

>May be you should be less hostile to simple questions? I take it you
>are expert in GRUB2 themes. I'm not and I'm trying to learn. OK?

Sorry, if I've insulted you. That wasn't my intension.

>You conveniently ignored comment about losing original error.
The original error will be displayed anyway, because it's called via grub_error function.
grub_errno in that case is only an indicator that everything went all right or something went wrong.
Returning error state (from component's modules) doesn't change grub_errno until we call grub_error in theme_loader.
In current state return value isn't analized. And I think it's not good.
(if theme_loaders checks global options and component's names - why shouldn't it also check for component's options)

I could think about more complicated hadling of return value.
If it is GRUB_ERR_IO then either information message about the error has already been shown
(don't see the possibility of the fact right now)
or we've faced some non-existing option (or option with a misprint)
So I could check for other types of error and hadle them more properly.

grub_errno in that case means error code, returned after trying to set a particular value to a particular option.
so if return code is GRUB_ERR_IO then there is a problem with option name
otherwise there is a problem with a value (e.g. we tried to set integer value to a option, which accepts string value.)

If you concerns are about right that - I can slightly remake the patch to show more detailed information about the error.

Cheers.
-- 
With best regards,
_______________________________
Vladimir Testov, ROSA Laboratory.
www.rosalab.ru

[-- Attachment #2: Type: text/html, Size: 9626 bytes --]

Re: [PATCH] gfxterm: check elements' properties and hadle errors.

[Vladimir Testov]

[-- Attachment #1: Type: text/plain, Size: 1014 bytes --]

>This loses original OOM error if it was returned. May be make it return
>true/false and check if grub_errno is set on false. Not sure.
>
>Do you have any real problem to solve? I wonder if these errors are
>visible to user at all; if not, setting them does not really buy
>anything.

The original error is not beeing processed at all in the current state.
So if we have error in global option or in component's name - the error is displayed.
But if we have error in component's option - (in current state) nothing is displayed and the option is just beeing ignored.
With this patch the error will be displayed in all cases.

Andrey, maybe you should try to compile GRUB with the patch or read source 
code more carefully before making conclusions? Or should I sent you screenshot 
with displaying of the error? Or you just don't know how the theme file is 
beeing parsed?

Please, try to be more concrete. :)

-- 
With best regards,
_______________________________
Vladimir Testov, ROSA Laboratory.
www.rosalab.ru

[-- Attachment #2: Type: text/html, Size: 4976 bytes --]

Re: [PATCH] gfxterm: check elements' properties and hadle errors.

[Andrey Borzenkov]

On Thu, Mar 7, 2013 at 8:22 AM, Vladimir Testov
<vladimir.testov@rosalab.ru> wrote:
>>This loses original OOM error if it was returned. May be make it return
>
>>true/false and check if grub_errno is set on false. Not sure.
>
>>
>
>>Do you have any real problem to solve? I wonder if these errors are
>
>>visible to user at all; if not, setting them does not really buy
>
>>anything.
>
>
> The original error is not beeing processed at all in the current state.
>
> So if we have error in global option or in component's name - the error is
> displayed.
>
> But if we have error in component's option - (in current state) nothing is
> displayed and the option is just beeing ignored.
>
> With this patch the error will be displayed in all cases.
>

Again - what problem are you trying to solve? I added unknown option
to theme element and it was loaded and displayed just fine. Why
aborting theme loading in this case is better?

>
>
> Andrey, maybe you should try to compile GRUB with the patch or read source
> code more carefully before making conclusions? Or should I sent you
> screenshot with displaying of the error? Or you just don't know how the
> theme file is beeing parsed?
>

May be you should be less hostile to simple questions? I take it you
are expert in GRUB2 themes. I'm not and I'm trying to learn. OK?

>
> Please, try to be more concrete. :)
>

You conveniently ignored comment about losing original error.

[PATCH] gfxterm: check elements' properties and hadle errors.

[Vladimir Testov]

[-- Attachment #1: Type: text/plain, Size: 278 bytes --]

GRUB does not show errors about non-existing properties or wrong values.
property name and error hadling works only for global properties

This patch fixes this misbehavior.

-- 
With best regards,
_______________________________
Vladimir Testov, ROSA Laboratory.
www.rosalab.ru

[-- Attachment #2: grub-2.00-check-properties.patch --]
[-- Type: text/x-patch, Size: 3928 bytes --]

diff -Naur grub-2.00/grub-core/gfxmenu/gui_box.c grub-patch/grub-core/gfxmenu/gui_box.c
--- grub-2.00/grub-core/gfxmenu/gui_box.c	2010-12-01 17:45:43.000000000 +0300
+++ grub-patch/grub-core/gfxmenu/gui_box.c	2013-03-04 14:17:51.423471466 +0400
@@ -300,6 +300,10 @@
       else
         self->id = 0;
     }
+  else
+    {
+      return GRUB_ERR_IO;
+    }
 
   return grub_errno;
 }
diff -Naur grub-2.00/grub-core/gfxmenu/gui_canvas.c grub-patch/grub-core/gfxmenu/gui_canvas.c
--- grub-2.00/grub-core/gfxmenu/gui_canvas.c	2010-12-01 17:45:43.000000000 +0300
+++ grub-patch/grub-core/gfxmenu/gui_canvas.c	2013-03-04 14:16:32.200770137 +0400
@@ -186,6 +186,10 @@
       else
         self->id = 0;
     }
+  else
+    {
+      return GRUB_ERR_IO;
+    }
   return grub_errno;
 }
 
diff -Naur grub-2.00/grub-core/gfxmenu/gui_circular_progress.c grub-patch/grub-core/gfxmenu/gui_circular_progress.c
--- grub-2.00/grub-core/gfxmenu/gui_circular_progress.c	2010-12-01 17:45:43.000000000 +0300
+++ grub-patch/grub-core/gfxmenu/gui_circular_progress.c	2013-03-04 14:15:57.352997914 +0400
@@ -270,6 +270,10 @@
 	grub_gfxmenu_timeout_register ((grub_gui_component_t) self,
 				       circprog_set_state);
     }
+  else
+    {
+      return GRUB_ERR_IO;
+    }
   return grub_errno;
 }
 
diff -Naur grub-2.00/grub-core/gfxmenu/gui_image.c grub-patch/grub-core/gfxmenu/gui_image.c
--- grub-2.00/grub-core/gfxmenu/gui_image.c	2012-02-08 16:41:16.000000000 +0400
+++ grub-patch/grub-core/gfxmenu/gui_image.c	2013-03-04 14:15:17.657499911 +0400
@@ -237,6 +237,10 @@
       else
         self->id = 0;
     }
+  else
+    {
+      return GRUB_ERR_IO;
+    }
   return grub_errno;
 }
 
diff -Naur grub-2.00/grub-core/gfxmenu/gui_label.c grub-patch/grub-core/gfxmenu/gui_label.c
--- grub-2.00/grub-core/gfxmenu/gui_label.c	2012-03-03 16:00:50.000000000 +0400
+++ grub-patch/grub-core/gfxmenu/gui_label.c	2013-03-04 14:14:53.121066492 +0400
@@ -231,6 +231,10 @@
 	grub_gfxmenu_timeout_register ((grub_gui_component_t) self,
 				       label_set_state);
     }
+  else
+    {
+      return GRUB_ERR_IO;
+    }
   return GRUB_ERR_NONE;
 }
 
diff -Naur grub-2.00/grub-core/gfxmenu/gui_list.c grub-patch/grub-core/gfxmenu/gui_list.c
--- grub-2.00/grub-core/gfxmenu/gui_list.c	2011-12-14 14:36:07.000000000 +0400
+++ grub-patch/grub-core/gfxmenu/gui_list.c	2013-03-04 12:54:31.311035573 +0400
@@ -527,6 +527,10 @@
       else
         self->id = 0;
     }
+  else
+    {
+      return GRUB_ERR_IO;
+    }
   return grub_errno;
 }
 
diff -Naur grub-2.00/grub-core/gfxmenu/gui_progress_bar.c grub-patch/grub-core/gfxmenu/gui_progress_bar.c
--- grub-2.00/grub-core/gfxmenu/gui_progress_bar.c	2012-03-10 22:37:48.000000000 +0400
+++ grub-patch/grub-core/gfxmenu/gui_progress_bar.c	2013-03-04 14:16:57.169163072 +0400
@@ -345,6 +345,10 @@
 	grub_gfxmenu_timeout_register ((grub_gui_component_t) self,
 				       progress_bar_set_state);
     }
+  else
+    {
+      return GRUB_ERR_IO;
+    }
   return grub_errno;
 }
 
diff -Naur grub-2.00/grub-core/gfxmenu/theme_loader.c grub-patch/grub-core/gfxmenu/theme_loader.c
--- grub-2.00/grub-core/gfxmenu/theme_loader.c	2012-02-24 14:19:45.000000000 +0400
+++ grub-patch/grub-core/gfxmenu/theme_loader.c	2013-03-04 15:01:36.112753989 +0400
@@ -557,7 +557,16 @@
 	parse_proportional_spec (value, &component->h, &component->hfrac);
       else
 	/* General property handling.  */
-	component->ops->set_property (component, property, value);
+        {
+          grub_err_t error = component->ops->set_property (component, property, value);
+          if (error != GRUB_ERR_NONE)
+            {
+              grub_error (GRUB_ERR_IO,
+                          "%s:%d:%d unknown property or wrong value `%s'=`%s'",
+                          p->filename, p->line_num, p->col_num,
+                          property, value);
+            }
+        }
 
       grub_free (value);
       grub_free (property);

Re: [PATCH] gfxterm: check elements' properties and hadle errors.

[Andrey Borzenkov]

В Mon, 04 Mar 2013 15:27:51 +0400
Vladimir Testov <vladimir.testov@rosalab.ru> пишет:

> GRUB does not show errors about non-existing properties or wrong values.
> property name and error hadling works only for global properties
> 
> This patch fixes this misbehavior.
> 

I find diff -p... generally more readable by providing context around
changes. May be my personal preference.

> --- grub-2.00/grub-core/gfxmenu/theme_loader.c	2012-02-24 14:19:45.000000000 +0400
> +++ grub-patch/grub-core/gfxmenu/theme_loader.c	2013-03-04 15:01:36.112753989 +0400
> @@ -557,7 +557,16 @@
>  	parse_proportional_spec (value, &component->h, &component->hfrac);
>        else
>  	/* General property handling.  */
> -	component->ops->set_property (component, property, value);
> +        {
> +          grub_err_t error = component->ops->set_property (component, property, value);
> +          if (error != GRUB_ERR_NONE)
> +            {
> +              grub_error (GRUB_ERR_IO,
> +                          "%s:%d:%d unknown property or wrong value `%s'=`%s'",
> +                          p->filename, p->line_num, p->col_num,
> +                          property, value);
> +            }
> +        }

This loses original OOM error if it was returned. May be make it return
true/false and check if grub_errno is set on false. Not sure.

Do you have any real problem to solve? I wonder if these errors are
visible to user at all; if not, setting them does not really buy
anything.

Re: [PATCH] gfxterm: check elements' properties and hadle errors.

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 642 bytes --]

On 04.03.2013 12:27, Vladimir Testov wrote:

> +  else
> +    {
> +      return GRUB_ERR_IO;
> +    }

This is not a correct error handling. You should never return an error
without calling grub_error. Also in all these contexts you handle the
cases of unknown property name. I don't feel like refusing to load a
theme because of unknown property is a good idea. This would i.a.
exclude older GRUB versions for newer themes even if property in
question is minor in meaning. Compare this to a browser refusing to show
a page because of unknown tag attribute. Also GRUB_ERR_IO hardly
describes a condition of unknown property.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]