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

Jan Zelený jzeleny at redhat.com
Mon May 14 08:32:27 UTC 2012


> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120514/fa489215/attachment.sig>


More information about the sssd-devel mailing list