Am Mittwoch, 14. Juni 2006 15:23 schrieb Gilles Chanteperdrix: > Niklaus Giger wrote: > > Is it okay to submit such a patch, which mixes a different test cases > > or shall I submit the patches in smaller pieces? Should the test that I > > put into the procedure msgDuringInt better be inlined? Or should I add a > > new file for these tests? > > I think it is a good idea to add some more tests to the message queues > tests. I also agree with the idea of documenting the differences instead > of being bug compatible. However, I would apply a few cosmetic changes > to the patch before including it. Comments follow, please tell me if you > agree with these changes. > > > - TEST_ASSERT(taskIdVerify(0) == ERROR && errno == > > S_objLib_OBJ_ID_ERROR); + errno = 0; > > + TEST_ASSERT(taskIdVerify(0) == ERROR && errno == 0); > > This is weird, it means that taskIdVerify does not set errno when passed > a null argument, but returns ERROR. Anyway, and this goes for all other > checks, I do not like errno = 0. If errno is not set, let us not check > it. Okay. That's probably safer. > > > + errno = 0; > > + TEST_ASSERT(taskIdVerify(malloc(20)) == ERROR && errno == > > S_objLib_OBJ_ID_ERROR); > > You should memset the new malloced block (or use calloc instead of > malloc), in order to be sure that it is invalid. And free it after use. I didn't bother about the memory leak, as the testsuite was not meant to live a long time. And it kept the changes smaller. But I would never accept this kind of code in a production system. So we better clean it up. > > + TEST_ASSERT_OK(rc); > > + TEST_ASSERT(errno == 0); > > There is no guarantee that the syscalls will not set errno when > returning OK. I would remove every TEST_ASSERT(errno == 0). Agreed. > > + TEST_ASSERT(rc == ERROR); > > + TEST_ASSERT(errno == S_msgQLib_NON_ZERO_TIMEOUT_AT_INT_LEVEL); > > Here, and in several other places, I would use only one assert, as it is > done in other tests. Fine for me. Just while developing test cases it was more convenient to know exactly which condition lead to an error. > > + errno = 0; > > qid = msgQCreate(NMESSAGES,sizeof(int),0xffff); > > - TEST_ASSERT(qid == 0 && errno == S_msgQLib_INVALID_QUEUE_TYPE); > > + TEST_ASSERT(qid != 0 && errno == 0); > > + if (qid) msgQDelete(qid); > > Here, it appears that vxworks and xenomai vxworks skin are incompatible, > changing the test code will make the test fail for xenomai. Would not it > be a better idea to document this difference ? After all, users code is > supposed to never use erroneous arguments, and if it does, it is better > to learn it when porting code to xenomai than to keep ignoring it > silently. Agreed. Do you agree with the following sentence for vxworks-skin.txt?
Passing a argument to options with non defined bits for msgQCreate will return 0 under the xenomai skin. VxWorks returns a valid msqId. > > - rc = msgQSend(qid,(char > > *)&message_list,0,WAIT_FOREVER,MSG_PRI_NORMAL); + errno = 0; > > + rc = msgQSend(qid,(char *)&message_list,2*message_list, > > WAIT_FOREVER,MSG_PRI_NORMAL); > > > > TEST_ASSERT(rc == ERROR && errno == S_msgQLib_INVALID_MSG_LENGTH); > > Accepting null length messages may be a feature, does a receiver receive > the null length message with vxworks ? I will check and give you an answer about this later (probably before Friday evening). > > - rc = msgQReceive(qid,(char *)&msg,0,NO_WAIT); > > - TEST_ASSERT(rc == ERROR && errno == S_msgQLib_INVALID_MSG_LENGTH); > > Null length message again, does this work with the real vxworks ? Same here. > > + rc = msgQNumMsgs(qid); > > You do not use this value. Missing TEST_ASSERT ? No. Bad cleanup before sending. I had some redundant tests after this position. Just kill this line. You may either check in a modified version of this patch or wait till I send you one which incorporates your suggestions. Best regards -- Niklaus Giger _______________________________________________ Xenomai-core mailing list Xenomaiemail@example.com https://mail.gna.org/listinfo/xenomai-core