Sorry for missing to read the review email from Jan.
I've already post PATCHv3 about this issue to ltp-list.
Thanks for Jan's review, and I appreciate it very much.
Best regards,
--
George Wang 王旭
Kernel Quantity Engineer
Red Hat Software (Beijing) Co.,Ltd
IRC:xuw
Tel:+86-010-62608041
Phone:15901231579
9/F, Tower C, Raycom
----- Original Message -----
From: "Stanislav Kholmanskikh" <stanislav.kholmansk...@oracle.com>
To: x...@redhat.com
Cc: "Jan Stancek" <jstan...@redhat.com>, ltp-list@lists.sourceforge.net
Sent: Tuesday, December 16, 2014 6:57:43 PM
Subject: Re: [LTP] [PATCH V2] library: add tst_system for wrapper system(3)
without SIGCHLD
Hi, George.
I've just found that I'm also motivated to have this bug fixed :)
Are you planning to send an updated version including Jan's comments?
Thanks.
PS: I also tested it with access06 test case:
without the patch:
[root@kholmanskikh access]# LTP_DEV_FS_TYPE=btrfs ./access06
access06 0 TINFO : Found free device '/dev/loop0'
access06 1 TBROK : tst_sig.c:233: unexpected signal
SIGCHLD/SIGCLD(17) received (pid = 10671).
access06 2 TBROK : tst_sig.c:233: Remaining cases broken
with:
[root@kholmanskikh access]# LTP_DEV_FS_TYPE=btrfs ./access06
access06 0 TINFO : Found free device '/dev/loop0'
access06 0 TINFO : Appending '-f' flag to mkfs.btrfs
access06 0 TINFO : Formatting /dev/loop0 with btrfs extra opts=''
access06 1 TPASS : access failed as expected:
TEST_ERRNO=EROFS(30): Read-only file system
On 11/20/2014 01:57 PM, Jan Stancek wrote:
>
>
> ----- Original Message -----
>> From: x...@redhat.com
>> To: ltp-list@lists.sourceforge.net
>> Sent: Monday, 17 November, 2014 5:04:39 PM
>> Subject: [LTP] [PATCH V2] library: add tst_system for wrapper system(3)
>> without SIGCHLD
>>
>> From: George Wang <x...@redhat.com>
>
> Hi,
>
> Looks good to me, I tested only on single testcase, it work
> as advertised. Couple nits to comments/commit message:
>
>>
>> system(3) will raise SIGCHLD signal to parent process, and most
>> test cases will call tst_sig to poison all signals including the
>> SIGCHLD. So add system(3) wrapper function to temporarily disable
>> the poisoned handler for SIGCHLD, which will make the test cases
>> happy.
>> Replace directly calling system(3) with tst_systm on behalf of
> ^^ tst_system
>> ignorcing SIGCHLG signal posioned handler.
> ^^ ignoring ^^ poisoned
> I don't get the "on behalf of" part of that sentence. Maybe:
> "Replace system(3) with tst_system() to ignore SIGCHLG
> signal handler poisoned earlier."
>
>>
>> Signed-off-by: George Wang <x...@redhat.com>
>> ---
>> include/test.h | 5 +++++
>> lib/tst_mkfs.c | 2 +-
>> lib/tst_run_cmd.c | 17 +++++++++++++++++
>> 3 files changed, 23 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/test.h b/include/test.h
>> index 775ba39..326b686 100644
>> --- a/include/test.h
>> +++ b/include/test.h
>> @@ -300,6 +300,11 @@ void tst_run_cmd(void (cleanup_fn)(void),
>> const char *stdout_path,
>> const char *stderr_path);
>>
>> +/* Wrapper function for system(3), ignorcing SIGCLD signal.
> ^^ ignoring
>
>> + * @command: the command to be run.
>> + */
>> +int tst_system(const char *command);
>> +
>> /* lib/tst_mkfs.c
>> *
>> * @dev: path to a device
>> diff --git a/lib/tst_mkfs.c b/lib/tst_mkfs.c
>> index 5eb3392..173347a 100644
>> --- a/lib/tst_mkfs.c
>> +++ b/lib/tst_mkfs.c
>> @@ -45,7 +45,7 @@ void tst_mkfs(void (cleanup_fn)(void), const char *dev,
>> /*
>> * The -f option was added to btrfs-progs v3.12
>> */
>> - if (system("mkfs.btrfs 2>&1 | grep '\\-f ' >/dev/null") == 0) {
>> + if (tst_system("mkfs.btrfs 2>&1 | grep '\\-f ' >/dev/null") ==
>> 0) {
>
> This line is now too long.
>
> Regards,
> Jan
>
>> tst_resm(TINFO, "Appending '-f' flag to mkfs.%s",
>> fs_type);
>> argv[pos++] = "-f";
>> diff --git a/lib/tst_run_cmd.c b/lib/tst_run_cmd.c
>> index 4eb32e6..5a02db0 100644
>> --- a/lib/tst_run_cmd.c
>> +++ b/lib/tst_run_cmd.c
>> @@ -125,3 +125,20 @@ void tst_run_cmd(void (cleanup_fn)(void),
>> "close() on %s failed at %s:%d",
>> stderr_path, __FILE__, __LINE__);
>> }
>> +
>> +int tst_system(const char *command)
>> +{
>> + int ret = 0;
>> +
>> + /*
>> + *Temporarily disable SIGCHLD of user defined handler, so the
>> + *system(3) function will not cause unexpected SIGCHLD signal
>> + *callback function for test cases.
>> + */
>> + void *old_handler = signal(SIGCHLD, SIG_DFL);
>> +
>> + ret = system(command);
>> +
>> + signal(SIGCHLD, old_handler);
>> + return ret;
>> +}
>> --
>> 1.8.4.2
>>
>>
>> ------------------------------------------------------------------------------
>> Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
>> from Actuate! Instantly Supercharge Your Business Reports and Dashboards
>> with Interactivity, Sharing, Native Excel Exports, App Integration & more
>> Get technology previously reserved for billion-dollar corporations, FREE
>> http://pubads.g.doubleclick.net/gampad/clk?id=157005751&iu=/4140/ostg.clktrk
>> _______________________________________________
>> Ltp-list mailing list
>> Ltp-list@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/ltp-list
>>
>
> ------------------------------------------------------------------------------
> Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
> from Actuate! Instantly Supercharge Your Business Reports and Dashboards
> with Interactivity, Sharing, Native Excel Exports, App Integration & more
> Get technology previously reserved for billion-dollar corporations, FREE
> http://pubads.g.doubleclick.net/gampad/clk?id=157005751&iu=/4140/ostg.clktrk
> _______________________________________________
> Ltp-list mailing list
> Ltp-list@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/ltp-list
>
------------------------------------------------------------------------------
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration & more
Get technology previously reserved for billion-dollar corporations, FREE
http://pubads.g.doubleclick.net/gampad/clk?id=164703151&iu=/4140/ostg.clktrk
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list
