Use syslog for device self-test errors

Submitted by Giuseppe Bilotta on Sept. 27, 2015, 11:46 a.m.

Details

Message ID 1443354390-3205-1-git-send-email-giuseppe.bilotta@gmail.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Giuseppe Bilotta Sept. 27, 2015, 11:46 a.m.
---
 src/cl_device_id.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/cl_device_id.c b/src/cl_device_id.c
index 78d2cf4..47bc772 100644
--- a/src/cl_device_id.c
+++ b/src/cl_device_id.c
@@ -36,6 +36,8 @@ 
 #include <stdlib.h>
 #include <sys/sysinfo.h>
 
+#include <syslog.h>
+
 #ifndef CL_VERSION_1_2
 #define CL_DEVICE_BUILT_IN_KERNELS 0x103F
 #endif
@@ -611,7 +613,7 @@  cl_self_test(cl_device_id device, cl_self_test_res atomic_in_l3_flag)
                       ret = SELF_TEST_PASS;
                     } else {
                       ret = SELF_TEST_SLM_FAIL;
-                      printf("Beignet: self-test failed: (3, 7, 5) + (5, 7, 3) returned (%i, %i, %i)\n"
+                      syslog(LOG_ERR, "Beignet: self-test failed: (3, 7, 5) + (5, 7, 3) returned (%i, %i, %i)\n"
                              "See README.md or http://www.freedesktop.org/wiki/Software/Beignet/\n",
                              test_data[0], test_data[1], test_data[2]);
 
@@ -646,6 +648,8 @@  cl_get_device_ids(cl_platform_id    platform,
 {
   cl_device_id device;
 
+  openlog("beignet", LOG_CONS, LOG_USER);
+
   /* Do we have a usable device? */
   device = cl_get_gt_device();
   if (device) {
@@ -653,7 +657,7 @@  cl_get_device_ids(cl_platform_id    platform,
     if (ret == SELF_TEST_ATOMIC_FAIL) {
       device->atomic_test_result = ret;
       ret = cl_self_test(device, ret);
-      printf("Beignet: warning - disable atomic in L3 feature.\n");
+      syslog(LOG_WARNING, "Beignet: Warning - disable atomic in L3 feature.\n");
     }
 
     if(ret == SELF_TEST_SLM_FAIL) {
@@ -664,13 +668,16 @@  cl_get_device_ids(cl_platform_id    platform,
         sscanf(env, "%i", &disable_self_test);
       }
       if (disable_self_test) {
-        printf("Beignet: Warning - overriding self-test failure\n");
+        syslog(LOG_WARNING, "Beignet: Warning - overriding self-test failure\n");
       } else {
-        printf("Beignet: disabling non-working device\n");
+        syslog(LOG_ERR, "Beignet: disabling non-working device\n");
         device = 0;
       }
     }
   }
+
+  closelog();
+
   if (!device) {
     if (num_devices)
       *num_devices = 0;

Comments

It is good idea print warnings to syslog avoid application confuse.
But for errors, I think also need print to stdout, give the clear hint to application.

Any other consideration? 

> -----Original Message-----

> From: Beignet [mailto:beignet-bounces@lists.freedesktop.org] On Behalf Of

> Giuseppe Bilotta

> Sent: Sunday, September 27, 2015 19:47

> To: beignet@lists.freedesktop.org

> Cc: Giuseppe Bilotta

> Subject: [Beignet] [PATCH] Use syslog for device self-test errors

> 

> ---

>  src/cl_device_id.c | 15 +++++++++++----

>  1 file changed, 11 insertions(+), 4 deletions(-)

> 

> diff --git a/src/cl_device_id.c b/src/cl_device_id.c index 78d2cf4..47bc772

> 100644

> --- a/src/cl_device_id.c

> +++ b/src/cl_device_id.c

> @@ -36,6 +36,8 @@

>  #include <stdlib.h>

>  #include <sys/sysinfo.h>

> 

> +#include <syslog.h>

> +

>  #ifndef CL_VERSION_1_2

>  #define CL_DEVICE_BUILT_IN_KERNELS 0x103F  #endif @@ -611,7 +613,7

> @@ cl_self_test(cl_device_id device, cl_self_test_res atomic_in_l3_flag)

>                        ret = SELF_TEST_PASS;

>                      } else {

>                        ret = SELF_TEST_SLM_FAIL;

> -                      printf("Beignet: self-test failed: (3, 7, 5) + (5, 7, 3) returned

> (%i, %i, %i)\n"

> +                      syslog(LOG_ERR, "Beignet: self-test failed: (3, 7, 5) + (5, 7, 3)

> returned (%i, %i, %i)\n"

>                               "See README.md or

> http://www.freedesktop.org/wiki/Software/Beignet/\n",

>                               test_data[0], test_data[1], test_data[2]);

> 

> @@ -646,6 +648,8 @@ cl_get_device_ids(cl_platform_id    platform,

>  {

>    cl_device_id device;

> 

> +  openlog("beignet", LOG_CONS, LOG_USER);

> +

>    /* Do we have a usable device? */

>    device = cl_get_gt_device();

>    if (device) {

> @@ -653,7 +657,7 @@ cl_get_device_ids(cl_platform_id    platform,

>      if (ret == SELF_TEST_ATOMIC_FAIL) {

>        device->atomic_test_result = ret;

>        ret = cl_self_test(device, ret);

> -      printf("Beignet: warning - disable atomic in L3 feature.\n");

> +      syslog(LOG_WARNING, "Beignet: Warning - disable atomic in L3

> + feature.\n");

>      }

> 

>      if(ret == SELF_TEST_SLM_FAIL) {

> @@ -664,13 +668,16 @@ cl_get_device_ids(cl_platform_id    platform,

>          sscanf(env, "%i", &disable_self_test);

>        }

>        if (disable_self_test) {

> -        printf("Beignet: Warning - overriding self-test failure\n");

> +        syslog(LOG_WARNING, "Beignet: Warning - overriding self-test

> + failure\n");

>        } else {

> -        printf("Beignet: disabling non-working device\n");

> +        syslog(LOG_ERR, "Beignet: disabling non-working device\n");

>          device = 0;

>        }

>      }

>    }

> +

> +  closelog();

> +

>    if (!device) {

>      if (num_devices)

>        *num_devices = 0;

> --

> 2.6.0.rc2.233.g6dd8a9a.dirty

> 

> _______________________________________________

> Beignet mailing list

> Beignet@lists.freedesktop.org

> http://lists.freedesktop.org/mailman/listinfo/beignet
On Mon, Oct 12, 2015 at 3:43 AM, Yang, Rong R <rong.r.yang@intel.com> wrote:
> It is good idea print warnings to syslog avoid application confuse.
> But for errors, I think also need print to stdout, give the clear hint to application.
>
> Any other consideration?

In my opinion a library should never produce (console) output unless
explicitly requested to do so, so I think using syslog in any case is
the correct approach. Maybe have some environment variable
(OCL_VERBOSE or something like that) to control output to console?

(In any case, even if the printf is preserved, it should at the very
least be an fprintf(stderr, ...) instead)

>> -----Original Message-----
>> From: Beignet [mailto:beignet-bounces@lists.freedesktop.org] On Behalf Of
>> Giuseppe Bilotta
>> Sent: Sunday, September 27, 2015 19:47
>> To: beignet@lists.freedesktop.org
>> Cc: Giuseppe Bilotta
>> Subject: [Beignet] [PATCH] Use syslog for device self-test errors
>>
>> ---
>>  src/cl_device_id.c | 15 +++++++++++----
>>  1 file changed, 11 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/cl_device_id.c b/src/cl_device_id.c index 78d2cf4..47bc772
>> 100644
>> --- a/src/cl_device_id.c
>> +++ b/src/cl_device_id.c
>> @@ -36,6 +36,8 @@
>>  #include <stdlib.h>
>>  #include <sys/sysinfo.h>
>>
>> +#include <syslog.h>
>> +
>>  #ifndef CL_VERSION_1_2
>>  #define CL_DEVICE_BUILT_IN_KERNELS 0x103F  #endif @@ -611,7 +613,7
>> @@ cl_self_test(cl_device_id device, cl_self_test_res atomic_in_l3_flag)
>>                        ret = SELF_TEST_PASS;
>>                      } else {
>>                        ret = SELF_TEST_SLM_FAIL;
>> -                      printf("Beignet: self-test failed: (3, 7, 5) + (5, 7, 3) returned
>> (%i, %i, %i)\n"
>> +                      syslog(LOG_ERR, "Beignet: self-test failed: (3, 7, 5) + (5, 7, 3)
>> returned (%i, %i, %i)\n"
>>                               "See README.md or
>> http://www.freedesktop.org/wiki/Software/Beignet/\n",
>>                               test_data[0], test_data[1], test_data[2]);
>>
>> @@ -646,6 +648,8 @@ cl_get_device_ids(cl_platform_id    platform,
>>  {
>>    cl_device_id device;
>>
>> +  openlog("beignet", LOG_CONS, LOG_USER);
>> +
>>    /* Do we have a usable device? */
>>    device = cl_get_gt_device();
>>    if (device) {
>> @@ -653,7 +657,7 @@ cl_get_device_ids(cl_platform_id    platform,
>>      if (ret == SELF_TEST_ATOMIC_FAIL) {
>>        device->atomic_test_result = ret;
>>        ret = cl_self_test(device, ret);
>> -      printf("Beignet: warning - disable atomic in L3 feature.\n");
>> +      syslog(LOG_WARNING, "Beignet: Warning - disable atomic in L3
>> + feature.\n");
>>      }
>>
>>      if(ret == SELF_TEST_SLM_FAIL) {
>> @@ -664,13 +668,16 @@ cl_get_device_ids(cl_platform_id    platform,
>>          sscanf(env, "%i", &disable_self_test);
>>        }
>>        if (disable_self_test) {
>> -        printf("Beignet: Warning - overriding self-test failure\n");
>> +        syslog(LOG_WARNING, "Beignet: Warning - overriding self-test
>> + failure\n");
>>        } else {
>> -        printf("Beignet: disabling non-working device\n");
>> +        syslog(LOG_ERR, "Beignet: disabling non-working device\n");
>>          device = 0;
>>        }
>>      }
>>    }
>> +
>> +  closelog();
>> +
>>    if (!device) {
>>      if (num_devices)
>>        *num_devices = 0;
>> --
>> 2.6.0.rc2.233.g6dd8a9a.dirty
>>
>> _______________________________________________
>> Beignet mailing list
>> Beignet@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/beignet
> -----Original Message-----

> From: Giuseppe Bilotta [mailto:giuseppe.bilotta@gmail.com]

> Sent: Monday, October 12, 2015 16:38

> To: Yang, Rong R

> Cc: beignet@lists.freedesktop.org

> Subject: Re: [Beignet] [PATCH] Use syslog for device self-test errors

> 

> On Mon, Oct 12, 2015 at 3:43 AM, Yang, Rong R <rong.r.yang@intel.com>

> wrote:

> > It is good idea print warnings to syslog avoid application confuse.

> > But for errors, I think also need print to stdout, give the clear hint to

> application.

> >

> > Any other consideration?

> 

> In my opinion a library should never produce (console) output unless

> explicitly requested to do so, so I think using syslog in any case is the correct

> approach. Maybe have some environment variable (OCL_VERBOSE or

> something like that) to control output to console?


I think it is worthy to produce some output if catch the known issues. It is helpful for application to locate the problem.
Or Maybe print in debug build only?

> 

> (In any case, even if the printf is preserved, it should at the very least be an

> fprintf(stderr, ...) instead)

Yes, agree with you.

> 

> >> -----Original Message-----

> >> From: Beignet [mailto:beignet-bounces@lists.freedesktop.org] On

> >> Behalf Of Giuseppe Bilotta

> >> Sent: Sunday, September 27, 2015 19:47

> >> To: beignet@lists.freedesktop.org

> >> Cc: Giuseppe Bilotta

> >> Subject: [Beignet] [PATCH] Use syslog for device self-test errors

> >>

> >> ---

> >>  src/cl_device_id.c | 15 +++++++++++----

> >>  1 file changed, 11 insertions(+), 4 deletions(-)

> >>

> >> diff --git a/src/cl_device_id.c b/src/cl_device_id.c index

> >> 78d2cf4..47bc772

> >> 100644

> >> --- a/src/cl_device_id.c

> >> +++ b/src/cl_device_id.c

> >> @@ -36,6 +36,8 @@

> >>  #include <stdlib.h>

> >>  #include <sys/sysinfo.h>

> >>

> >> +#include <syslog.h>

> >> +

> >>  #ifndef CL_VERSION_1_2

> >>  #define CL_DEVICE_BUILT_IN_KERNELS 0x103F  #endif @@ -611,7 +613,7

> >> @@ cl_self_test(cl_device_id device, cl_self_test_res atomic_in_l3_flag)

> >>                        ret = SELF_TEST_PASS;

> >>                      } else {

> >>                        ret = SELF_TEST_SLM_FAIL;

> >> -                      printf("Beignet: self-test failed: (3, 7, 5) + (5, 7, 3) returned

> >> (%i, %i, %i)\n"

> >> +                      syslog(LOG_ERR, "Beignet: self-test failed:

> >> + (3, 7, 5) + (5, 7, 3)

> >> returned (%i, %i, %i)\n"

> >>                               "See README.md or

> >> http://www.freedesktop.org/wiki/Software/Beignet/\n",

> >>                               test_data[0], test_data[1],

> >> test_data[2]);

> >>

> >> @@ -646,6 +648,8 @@ cl_get_device_ids(cl_platform_id    platform,

> >>  {

> >>    cl_device_id device;

> >>

> >> +  openlog("beignet", LOG_CONS, LOG_USER);

> >> +

> >>    /* Do we have a usable device? */

> >>    device = cl_get_gt_device();

> >>    if (device) {

> >> @@ -653,7 +657,7 @@ cl_get_device_ids(cl_platform_id    platform,

> >>      if (ret == SELF_TEST_ATOMIC_FAIL) {

> >>        device->atomic_test_result = ret;

> >>        ret = cl_self_test(device, ret);

> >> -      printf("Beignet: warning - disable atomic in L3 feature.\n");

> >> +      syslog(LOG_WARNING, "Beignet: Warning - disable atomic in L3

> >> + feature.\n");

> >>      }

> >>

> >>      if(ret == SELF_TEST_SLM_FAIL) {

> >> @@ -664,13 +668,16 @@ cl_get_device_ids(cl_platform_id    platform,

> >>          sscanf(env, "%i", &disable_self_test);

> >>        }

> >>        if (disable_self_test) {

> >> -        printf("Beignet: Warning - overriding self-test failure\n");

> >> +        syslog(LOG_WARNING, "Beignet: Warning - overriding self-test

> >> + failure\n");

> >>        } else {

> >> -        printf("Beignet: disabling non-working device\n");

> >> +        syslog(LOG_ERR, "Beignet: disabling non-working device\n");

> >>          device = 0;

> >>        }

> >>      }

> >>    }

> >> +

> >> +  closelog();

> >> +

> >>    if (!device) {

> >>      if (num_devices)

> >>        *num_devices = 0;

> >> --

> >> 2.6.0.rc2.233.g6dd8a9a.dirty

> >>

> >> _______________________________________________

> >> Beignet mailing list

> >> Beignet@lists.freedesktop.org

> >> http://lists.freedesktop.org/mailman/listinfo/beignet

> 

> 

> 

> --

> Giuseppe "Oblomov" Bilotta