From: Cyril Hrubis <chrubis@suse.cz>
To: Zeng Linggang <zenglg.jy@cn.fujitsu.com>
Cc: ltp-list <ltp-list@lists.sourceforge.net>
Subject: Re: [LTP] [PATCH] stress_cd: Cleanup
Date: Tue, 11 Nov 2014 16:08:36 +0100 [thread overview]
Message-ID: <20141111150836.GC32196@rei> (raw)
In-Reply-To: <1413554791.5750.27.camel@G08JYZSD130126.localdomain>
Hi!
> diff --git a/testcases/kernel/io/stress_cd/stress_cd.c b/testcases/kernel/io/stress_cd/stress_cd.c
> index 0209a1f..6e13afa 100644
> --- a/testcases/kernel/io/stress_cd/stress_cd.c
> +++ b/testcases/kernel/io/stress_cd/stress_cd.c
> @@ -1,6 +1,7 @@
> /*
> - *
> * Copyright (c) International Business Machines Corp., 2001
> + * 06/20/2001 Robbie Williamson (robbiew@us.ibm.com)
> + * 11/08/2001 Manoj Iyer (manjo@austin.ibm.com)
> *
> * This program is free software; you can redistribute it and/or modify
> * it under the terms of the GNU General Public License as published by
> @@ -13,37 +14,19 @@
> * the GNU General Public License for more details.
> *
> * You should have received a copy of the GNU General Public License
> - * along with this program; if not, write to the Free Software
> - * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + * along with this program; if not, write to the Free Software Foundation,
> + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> */
>
> /*
> - * FILE : stress_cd.c
> - * DESCRIPTION : creates multiple read threads on the cdrom device.
> - * HISTORY:
> - * 06/20/2001 Robbie Williamson (robbiew@us.ibm.com)
> - * -Ported
> - * 11/08/2001 Manoj Iyer (manjo@austin.ibm.com)
> - * - Modified.
> - * - removed compiler warnings.
> - * - Added #include <sys/types.h>, #include <unistd.h> and
> - * #include <string.h>
> - * - print unsigned long correctly in printf() use "lx" instead of "x"
> - * - added missing parameter in usage message.
> - *
> -+--------------------------------------------------------------------+
> -| |
> -| Usage: cdtest [-n n] [-f file] [-m xx] [-d] |
> -| |
> -| where: |
> -| -n n Number of threads to create |
> -| -f file File or device to read from |
> -| -m xx Number of MB to read from file |
> -| -b xx Number of bytes to read from file |
> -| -d Enable debugging messages |
> -| |
> -| |
> -+-------------------------------------------------------------------*/
> + * Usage: stress_cd [-n n] [-f file] [-m xx] [-d]
> + * where:
> + * -n n Number of threads to create
> + * -f file File or device to read from
> + * -m xx Number of MB to read from file
> + * -b xx Number of bytes to read from file
> + * -d Enable debugging messages
> + */
>
> #include <pthread.h>
> #include <fcntl.h>
> @@ -55,46 +38,21 @@
> #include <unistd.h>
> #include <string.h>
>
> -/* Defines
> - *
> - * DEFAULT_NUM_THREADS: Default number of threads to create,
> - * user can specifiy with [-n] command line option.
> - *
> - * USAGE: usage statement
> - */
> -#define DEFAULT_NUM_THREADS 10
> -#define DEFAULT_NUM_BYTES 1024*1024*100 /* 100Mb */
> -#define DEFAULT_FILE "/dev/cdrom"
> +#define DEFAULT_NUM_THREADS 10
> +#define DEFAULT_NUM_BYTES (1024*1024*100) /* 100Mb */
> +#define DEFAULT_FILE "/dev/cdrom"
>
> -/*
> - * Function prototypes
> - *
> - * sys_error (): System error message function
> - * error (): Error message function
> - * parse_args (): Parses command line arguments
> - */
> static void sys_error(const char *, int);
> -static void error(const char *, int);
> static void parse_args(int, char **);
> -void *thread(int *);
> -int read_data(int, unsigned long);
> +static void *thread(int *);
> +static int read_data(int, unsigned long *);
> +
> +static int num_threads = DEFAULT_NUM_THREADS;
> +static int num_bytes = DEFAULT_NUM_BYTES;
> +static char *file = DEFAULT_FILE;
> +static unsigned long checksum;
> +static int debug;
>
> -/*
> - * Global Variables
> - */
> -int num_threads = DEFAULT_NUM_THREADS;
> -int num_bytes = DEFAULT_NUM_BYTES;
> -char *file = DEFAULT_FILE;
> -unsigned long checksum = 0;
> -int debug = 0;
> -
> -/*-------------------------------------------------------------------+
> -| main () |
> -| ================================================================== |
> -| |
> -| Function: Main program (see prolog for more details) |
> -| |
> -+-------------------------------------------------------------------*/
> int main(int argc, char **argv)
> {
> pthread_attr_t attr;
> @@ -106,17 +64,16 @@ int main(int argc, char **argv)
> parse_args(argc, argv);
>
> /* Read data from CDROM & compute checksum */
> - read_data(0, checksum);
> + read_data(0, &checksum);
Nice catch :)
So the testcases wasn't checking the checksums at all...
> if (debug)
> - printf("Thread [main] checksum: %-#12lx \n", checksum);
> + printf("\tThread [main] checksum: %-#12lx\n", checksum);
What is the use for the '\t' here? As far as I can see the printed
strings in the testcase ends up with '\n' anyway and so the printed
statement starts at first column anyway.
> if (pthread_attr_init(&attr))
> sys_error("pthread_attr_init failed", __LINE__);
> if (pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_JOINABLE))
> sys_error("pthread_attr_setdetachstate failed", __LINE__);
>
> - /* Create num_thread threads... */
> - printf("\tThread [main] Creating %d threads\n", num_threads);
> + printf("Thread [main] Creating %d threads\n", num_threads);
>
> array = malloc(sizeof(pthread_t) * num_threads);
> arg = malloc(sizeof(int) * num_threads);
Can we also get rid of allocations and decleare the arrays as:
pthread_t array[num_threads];
instead?
> @@ -126,15 +83,15 @@ int main(int argc, char **argv)
> if (debug)
> printf("\tThread [main]: creating thread %d\n", i + 1);
> arg[i] = i + 1;
> - if (pthread_create
> - ((pthread_t *) & array[i], &attr, (void *)thread,
> - (void *)&arg[i])) {
> - if (errno == EAGAIN)
> + if (pthread_create((pthread_t *)&array[i], &attr,
> + (void *)thread, (void *)&arg[i])) {
> + if (errno == EAGAIN) {
> fprintf(stderr,
> - "\tThread [main]: unable to create thread %d\n",
> - i);
> - else
> + "\tThread [main]: unable to create "
> + "thread %d\n", i);
> + } else {
> sys_error("pthread_create failed", __LINE__);
> + }
> }
> if (debug)
> printf("\tThread [main]: created thread %d\n", i + 1);
> @@ -144,8 +101,7 @@ int main(int argc, char **argv)
>
> for (i = 0; i < num_threads; i++) {
> void *exit_value;
> - printf("\tThread [main]: waiting for thread: %d\n", i + 1);
> - /*if (pthread_join ((pthread_t*) array [i], (void **) &exit_value)) */
> + printf("Thread [main]: waiting for thread: %d\n", i + 1);
> if (pthread_join(array[i], &exit_value))
> sys_error("pthread_join failed", __LINE__);
>
> @@ -157,25 +113,16 @@ int main(int argc, char **argv)
> free(array);
> free(arg);
...
> - /* One or more of the threads did not complete sucessfully! */
> if (rc != 0) {
> printf("test failed!\n");
> exit(-1);
> + } else {
> + printf("Thread [main] All threads completed successfully...\n");
> + exit(0);
> }
> -
> - /* Program completed successfully... */
> - printf("\tThread [main] All threads completed successfully...\n");
> - exit(0);
There is no need for this particular change.
If we enter the if block we will exit at exit(-1) otherwise we continue
to the end of the function.
> }
>
> -/*-------------------------------------------------------------------+
> -| thread () |
> -| ================================================================== |
> -| |
> -| Function: ... |
> -| |
> -+-------------------------------------------------------------------*/
> -void *thread(int *parm)
> +static void *thread(int *parm)
> {
> int num = *parm;
> unsigned long cksum = 0;
> @@ -183,7 +130,7 @@ void *thread(int *parm)
> if (debug)
> printf("\tThread [%d]: begin\n", num);
>
> - read_data(num, cksum);
> + read_data(num, &cksum);
> if (checksum != cksum) {
> fprintf(stderr, "\tThread [%d]: checksum mismatch!\n", num);
> pthread_exit((void *)-1);
> @@ -193,17 +140,9 @@ void *thread(int *parm)
> printf("\tThread [%d]: done\n", num);
>
> pthread_exit(NULL);
> - return (NULL);
> }
>
> -/*-------------------------------------------------------------------+
> -| read_data () |
> -| ================================================================== |
> -| |
> -| Function: Reads data from the CDROM |
> -| |
> -+-------------------------------------------------------------------*/
> -int read_data(int num, unsigned long cksum)
> +static int read_data(int num, unsigned long *cksum)
> {
> int fd;
> const int bufSize = 1024;
> @@ -223,23 +162,25 @@ int read_data(int num, unsigned long cksum)
>
> lseek(fd, 1024 * 36, SEEK_SET);
> while (bytes_read < num_bytes) {
> - if ((n = read(fd, buffer, bufSize)) < 0)
> + n = read(fd, buffer, bufSize);
> + if (n < 0)
> sys_error("read failed", __LINE__);
> + else if (n == 0)
> + sys_error("End of file", __LINE__);
> bytes_read += n;
>
> for (p = buffer; p < buffer + n; p++)
> - cksum += *p;
> + *cksum += *p;
>
> if (debug)
> - printf
> - ("\tThread [%d] bytes read: %5d checksum: %-#12lx\n",
> - num, bytes_read, cksum);
> + printf("\tThread [%d] bytes read: %5d checksum: "
> + "%-#12lx\n", num, bytes_read, *cksum);
> }
> free(buffer);
>
> if (debug)
> printf("\tThread [%d] bytes read: %5d checksum: %-#12lx\n",
> - num, bytes_read, cksum);
> + num, bytes_read, *cksum);
>
> if (close(fd) < 0)
> sys_error("close failed", __LINE__);
> @@ -250,20 +191,6 @@ int read_data(int num, unsigned long cksum)
> return (0);
> }
>
> -/*-------------------------------------------------------------------+
> -| parse_args () |
> -| ================================================================== |
> -| |
> -| Function: Parse the command line arguments & initialize global |
> -| variables. |
> -| |
> -| Updates: (command line options) |
> -| |
> -| [-n] num number of threads to create |
> -| |
> -| [-d] enable debugging messages |
> -| |
> -+-------------------------------------------------------------------*/
> static void parse_args(int argc, char **argv)
> {
> int i;
> @@ -297,6 +224,10 @@ static void parse_args(int argc, char **argv)
> errflag++;
> fprintf(stderr, "ERROR: num_bytes must be greater than 0");
> }
> + if (num_threads < 0) {
> + errflag++;
> + fprintf(stderr, "ERROR: num_threads must be greater than 0");
> + }
>
> if (errflag) {
> fprintf(stderr, "\nUsage: %s"
> @@ -311,30 +242,8 @@ static void parse_args(int argc, char **argv)
> }
> }
>
> -/*-------------------------------------------------------------------+
> -| sys_error () |
> -| ================================================================== |
> -| |
> -| Function: Creates system error message and calls error () |
> -| |
> -+-------------------------------------------------------------------*/
> static void sys_error(const char *msg, int line)
> {
> - char syserr_msg[256];
> -
> - sprintf(syserr_msg, "%s: %s\n", msg, strerror(errno));
> - error(syserr_msg, line);
> -}
> -
> -/*-------------------------------------------------------------------+
> -| error () |
> -| ================================================================== |
> -| |
> -| Function: Prints out message and exits... |
> -| |
> -+-------------------------------------------------------------------*/
> -static void error(const char *msg, int line)
> -{
> - fprintf(stderr, "ERROR [line: %s] \n", msg);
> + fprintf(stderr, "ERROR [%d: %s: %s]\n", line, msg, strerror(errno));
> exit(-1);
> }
> --
> 1.9.3
>
>
>
>
> ------------------------------------------------------------------------------
> Comprehensive Server Monitoring with Site24x7.
> Monitor 10 servers for $9/Month.
> Get alerted through email, SMS, voice calls or mobile push notifications.
> Take corrective actions from your mobile device.
> http://p.sf.net/sfu/Zoho
> _______________________________________________
> Ltp-list mailing list
> Ltp-list@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/ltp-list
--
Cyril Hrubis
chrubis@suse.cz
------------------------------------------------------------------------------
Comprehensive Server Monitoring with Site24x7.
Monitor 10 servers for $9/Month.
Get alerted through email, SMS, voice calls or mobile push notifications.
Take corrective actions from your mobile device.
http://pubads.g.doubleclick.net/gampad/clk?id=154624111&iu=/4140/ostg.clktrk
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list
next prev parent reply other threads:[~2014-11-11 15:09 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-10-17 14:06 [LTP] [PATCH] stress_cd: Cleanup Zeng Linggang
2014-11-11 15:08 ` Cyril Hrubis [this message]
[not found] ` <1415773510.3552.12.camel@G08JYZSD130126.localdomain>
2014-12-02 16:10 ` [LTP] [PATCH v2] " Cyril Hrubis
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20141111150836.GC32196@rei \
--to=chrubis@suse.cz \
--cc=ltp-list@lists.sourceforge.net \
--cc=zenglg.jy@cn.fujitsu.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.