[PATCH] 1332-Abort unit test when open() fails

[Ariel Barria]

https://fedorahosted.org/sssd/ticket/1332

[Jan Zelený]

> https://fedorahosted.org/sssd/ticket/1332

Obvious Ack,

Jan

[Jakub Hrozek]

On Sun, May 13, 2012 at 04:18:39PM -0500, Ariel Barria wrote:
> https://fedorahosted.org/sssd/ticket/1332
> if ( fd < 0 ){

Thanks for the patch Ariel, but would you also mind amending the
whitespace? We don't put whitespace between brackets and the expression
inside, but we do put a space after a normal bracket and a curly one. So
the "if" should read:

+ if (fd < 0) {
>   fail("Cannot open /dev/zero");
> }else{

Similar here, space between curly bracket and else on both sides.

The same applies to the second change.

I was also thinking we may want to introduce a new macro that would mark
the test as failed and quit the test at the same time, something like
quit_if and equivalent quit_unless. These would be just simple wrappers
around existing fail_if/unless. Along the lines of (untested):

#define quit_if(expr, ...) do {    \
    fail_if(expr, ## __VA_ARGS__); \
    if (expr) {                    \
        return;                    \
    }                              \
} while(0)

But perhaps that's outside the scope of this ticket..

[Jan Zelený]

> On Sun, May 13, 2012 at 04:18:39PM -0500, Ariel Barria wrote:
> > https://fedorahosted.org/sssd/ticket/1332
> > if ( fd < 0 ){
>
> Thanks for the patch Ariel, but would you also mind amending the
> whitespace? We don't put whitespace between brackets and the expression
> inside, but we do put a space after a normal bracket and a curly one. So
> the "if" should read:
> if (fd < 0) {
> >   fail("Cannot open /dev/zero");
> > }else{
>
> Similar here, space between curly bracket and else on both sides.
>
> The same applies to the second change.

Since we are nitpicking, I have one comment as well:

+       fail_unless(numread == bufsize,
                 "Read %d bytes expected %d\n", numread, bufsize);
-    close(fd);

The string in the middle should be moved to be aligned with the rest of 
arguments. There are two occurences of this in the patch.
> I was also thinking we may want to introduce a new macro that would mark
> the test as failed and quit the test at the same time, something like
> quit_if and equivalent quit_unless. These would be just simple wrappers
> around existing fail_if/unless. Along the lines of (untested):
>
> #define quit_if(expr, ...) do {    \
>     fail_if(expr, ## __VA_ARGS__); \
>     if (expr) {                    \
>         return;                    \
>     }                              \
> } while(0)
>
> But perhaps that's outside the scope of this ticket..

I agree that we should do such macro, but it should be used on much more 
places where the failed check should fail the test otherwise segfault follows. 
However this should be done as more extensive effort and all places with 
potential for this kind of error should be changed. Therefore new ticket seems 
more appropriate.

Thanks
Jan

[Stephen Gallagher]

On Mon, 2012-05-14 at 10:22 +0200, Jakub Hrozek wrote:
> On Sun, May 13, 2012 at 04:18:39PM -0500, Ariel Barria wrote:
> > https://fedorahosted.org/sssd/ticket/1332
> > if ( fd < 0 ){
>
> Thanks for the patch Ariel, but would you also mind amending the
> whitespace? We don't put whitespace between brackets and the expression
> inside, but we do put a space after a normal bracket and a curly one. So
> the "if" should read:
> if (fd < 0) {
> >   fail("Cannot open /dev/zero");
> > }else{
>
> Similar here, space between curly bracket and else on both sides.
>
> The same applies to the second change.
>
> I was also thinking we may want to introduce a new macro that would mark
> the test as failed and quit the test at the same time, something like
> quit_if and equivalent quit_unless. These would be just simple wrappers
> around existing fail_if/unless. Along the lines of (untested):
>
> #define quit_if(expr, ...) do {    \
>     fail_if(expr, ## __VA_ARGS__); \
>     if (expr) {                    \
>         return;                    \
>     }                              \
> } while(0)
>
> But perhaps that's outside the scope of this ticket..

This is the definition of _fail_unless() (which the fail_unless and
fail_if macros wrap):

void _fail_unless (int result, const char *file,
                   int line, const char *expr, ...)
{
  const char *msg;

send_loc_info (file, line);
  if (!result) {
    va_list ap;
    char buf[BUFSIZ];

va_start(ap,expr);
    msg = (const char*)va_arg(ap, char *);
    if (msg == NULL)
      msg = expr;
    vsnprintf(buf, BUFSIZ, msg, ap);
    va_end(ap);
    send_failure_info (buf);
    if (cur_fork_status() == CK_FORK) {
#ifdef _POSIX_VERSION
      exit(1);
#endif /* _POSIX_VERSION */
    }
  }
}

So basically, as long as we're running with CK_FORK=yes (the default
mode), failing a test is already guaranteed to exit.

We might want to propose a patch upstream that exit(1) should not be
contingent upon CK_FORK, since I would be willing to bet that in most
programs, continuing on would result only in chaining failures.

I do understand, however, that their goal here is to allow running ALL
the tests so they can display only the failures at the end.

Also keep in mind that CK_FORK=no is intended only as a debugging mode
(so it's easier to get into gdb/valgrind). All of the check tests are
written with CK_FORK=yes in mind, because exiting the child process
guarantees cleanup, where if you are using CK_FORK=no, your state may
not be stable between individual tests.

[Ariel Barria]

Thanks to comments

From: [e-mail]
To: [e-mail]
Date: Mon, 14 May 2012 07:22:16 -0400
Subject: Re: [SSSD] [PATCH] 1332-Abort unit test when open() fails

On Mon, 2012-05-14 at 10:22 +0200, Jakub Hrozek wrote:
> On Sun, May 13, 2012 at 04:18:39PM -0500, Ariel Barria wrote:
> > https://fedorahosted.org/sssd/ticket/1332
> > if ( fd < 0 ){
>
> Thanks for the patch Ariel, but would you also mind amending the
> whitespace? We don't put whitespace between brackets and the expression
> inside, but we do put a space after a normal bracket and a curly one. So
> the "if" should read:
> if (fd < 0) {
> >   fail("Cannot open /dev/zero");
> > }else{
>
> Similar here, space between curly bracket and else on both sides.
>
> The same applies to the second change.
>
> I was also thinking we may want to introduce a new macro that would mark
> the test as failed and quit the test at the same time, something like
> quit_if and equivalent quit_unless. These would be just simple wrappers
> around existing fail_if/unless. Along the lines of (untested):
>
> #define quit_if(expr, ...) do {    \
>     fail_if(expr, ## __VA_ARGS__); \
>     if (expr) {                    \
>         return;                    \
>     }                              \
> } while(0)
>
> But perhaps that's outside the scope of this ticket..

This is the definition of _fail_unless() (which the fail_unless and
fail_if macros wrap):

void _fail_unless (int result, const char *file,
                   int line, const char *expr, ...)
{
  const char *msg;

send_loc_info (file, line);
  if (!result) {
    va_list ap;
    char buf[BUFSIZ];

va_start(ap,expr);
    msg = (const char*)va_arg(ap, char *);
    if (msg == NULL)
      msg = expr;
    vsnprintf(buf, BUFSIZ, msg, ap);
    va_end(ap);
    send_failure_info (buf);
    if (cur_fork_status() == CK_FORK) {
#ifdef _POSIX_VERSION
      exit(1);
#endif /* _POSIX_VERSION */
    }
  }
}

So basically, as long as we're running with CK_FORK=yes (the default
mode), failing a test is already guaranteed to exit.

We might want to propose a patch upstream that exit(1) should not be
contingent upon CK_FORK, since I would be willing to bet that in most
programs, continuing on would result only in chaining failures.

I do understand, however, that their goal here is to allow running ALL
the tests so they can display only the failures at the end.

Also keep in mind that CK_FORK=no is intended only as a debugging mode
(so it's easier to get into gdb/valgrind). All of the check tests are
written with CK_FORK=yes in mind, because exiting the child process
guarantees cleanup, where if you are using CK_FORK=no, your state may
not be stable between individual tests.

_______________________________________________
sssd-devel mailing list
[e-mail]
https://fedorahosted.org/mailman/listinfo/sssd-devel

[Jakub Hrozek]

On Mon, May 14, 2012 at 11:42:31PM -0500, Ariel Barria wrote:
> Thanks to comments

Sorry Ariel, I must have confused you -- as Stephen explained, the
existing fail_if macros already exit if you're running in the default
forking more. I didn't notice this because my .bashrc contains
CK_FORK=no.

So all that is needed is to fix the code style in your first patch.
Sorry again for the confusion.