On Wed, Jan 07, 2009 at 11:41:48PM +0530, Sudhir Kumar wrote: > On Wed, Jan 07, 2009 at 11:28:57PM +0530, Dhaval Giani wrote: > > On Wed, Jan 07, 2009 at 10:20:33PM +0530, Sudhir Kumar wrote: > > > A lack of proper initial thought let me use some variables globaly, which > > > is not a good idea at all. However fixing this issue at this stage is not > > > very good from the point that it changes variables for a lot of functions. > > > But still it has to be done for a better and reusable code. This patch > > > does > > > that while putting related variables in meaningful structs. > > > Thanks to Balbir for a good suggestion. > > > > > > TODO: > > > Fill these data structures from a configuration file after analyzing > > > requirements from different test functions. > > > > > > Signed-off-by: Sudhir Kumar <[email protected]> > > > Acked-by: Dhaval Giani <[email protected]> > > > Acked-by: Balbir Singh <[email protected]> > > > > > > --- > > > tests/libcgrouptest.h | 54 ++++++++------ > > > tests/libcgrouptest01.c | 177 > > > ++++++++++++++++++++++++++++-------------------- > > > 2 files changed, 138 insertions(+), 93 deletions(-) > > > > > > Index: trunk/tests/libcgrouptest.h > > > =================================================================== > > > --- trunk.orig/tests/libcgrouptest.h > > > +++ trunk/tests/libcgrouptest.h > > > @@ -34,8 +34,6 @@ > > > #define PASS 1 /* test passed */ > > > #define FAIL 0 /* test failed */ > > > > > > -int cpu = 0, memory = 0; > > > - > > > enum cgroup_mount_t { > > > FS_NOT_MOUNTED, > > > FS_MOUNTED, > > > @@ -78,26 +76,36 @@ enum info_message_t { > > > NOMESSAGE, > > > }; > > > > > > +/* Keep a single struct of all ids */ > > > +struct uid_gid_t { > > > + uid_t control_uid; > > > + gid_t control_gid; > > > + uid_t tasks_uid; > > > + gid_t tasks_gid; > > > +}; > > > + > > > +/* Keep a single struct of all control values */ > > > +struct cntl_val_t { > > > + int64_t val_int64; > > > + u_int64_t val_uint64; > > > + bool val_bool; > > > + /* size worth of 100 digit num is fair enough */ > > > + char val_string[100]; /* string value of control parameter */ > > > +}; > > > + > > > +extern int cpu, memory; > > > + > > > /* The set of verbose messages useful to the user */ > > > extern char info[NUM_MSGS][SIZE]; > > > > > > > hmm, > > > > extern char info[][]; > > > > ? > A global readonly variable. Probably I would have put it const char. > > > > > -int64_t val_int64; > > > -u_int64_t val_uint64; > > > -bool val_bool; > > > -/* Doubt: size of following string. is'nt this wrong ?*/ > > > -char val_string[FILENAME_MAX]; /* string value of control parameter */ > > > -uid_t control_uid; > > > -gid_t control_gid; > > > -uid_t tasks_uid; > > > -gid_t tasks_gid; > > > /* this variable is never modified */ > > > -int fs_mounted; > > > +extern int fs_mounted; > > > > > > -/* The mountpoints as received from script */ > > > -char mountpoint[FILENAME_MAX], mountpoint2[FILENAME_MAX]; > > > - > > > -/* No extra message unless specified */ > > > -char extra[SIZE] = "\n"; > > > +/* The mountpoints as received from script > > > + * We use mountpoint for single mount. > > > + * For multimount we use mountpoint and mountpoint2. > > > + */ > > > +extern char mountpoint[FILENAME_MAX], mountpoint2[FILENAME_MAX]; > > > > > > > similarly here. > > > > (This is because, if you change the value elsewhere, you will need to > > change it everywhere. bad for maintainance) > The above one and these two are constant variables that never change in > the tests. They deserved a const qualifier actually. > >
True, but my point is that you should not give the size in multiple places. Just when you declare it. -- regards, Dhaval ------------------------------------------------------------------------------ Check out the new SourceForge.net Marketplace. It is the best place to buy or sell services for just about anything Open Source. http://p.sf.net/sfu/Xq1LFB _______________________________________________ Libcg-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/libcg-devel
