[linux-lvm] some comments on the LVM source
Andrew Clausen
clausen at gnu.org
Mon Apr 30 23:06:03 UTC 2001
Hi,
I'm mainly looking at the userspace stuff here, because
that's what I'm interested in ;-)
Comments:
* liblvm.h is one monster file. It would be nice to separate
it up into smaller pieces. Say, one (or more!) files for
VG stuff, etc.
* the .c files are in one directory. They should be split
into subdirectories... makes it easier to find stuff
* lots of monster functions. Monster functions are harder to
understand and maintain.
I'll have a look at one function for you, so I can go a bit
more in depth:
lvm_add_dir_cache() in LVM/0.9.1_beta7/tools/lib/lvm_dir_cache.c
The function is about 50 lines, which is on the large side.
It is doing things on two layers of abstraction:
* high layer: calling high level things like
lvm_dir_cache_hit(),
* lower layer: reallocating memory for the cache,
updating lower level variables, etc.
It is usually a bad thing to do things. The fact that the
function is also fairly large should have lead to a decision
to split it up, by putting the lower level stuff into (static)
helper functions.
The 4 layers of nesting also gives you a hint that you're
handling too many levels of abstraction ;-)
IMHO, the function should be roughly 10 lines, and the
lower level code taken out into helper functions.
Also, you have an inconsistent interface to lvm_dir_cache().
Most of the functions related to the lvm_dir_cache() are
named lvm_dir_cache_XXX(), but you have lvm_add_dir_cache().
Also, some functions ask for a "dircache" parameter, and others
just use the global variable (since there is exactly one
cache anyway).
Make up your mind! In my example "cleaned version", I
chose to use the global variables. I guess it doesn't
make a big difference (some ppl would argue global
variables evil blah blah blah...)
So, something along the lines of:
static int _dir_cache_init_entry (dir_cache_t *entry,
char *dev_name) {
struct stat stat_b;
if (stat (dev_name, &stat_b) == -1)
return FALSE;
if (lvm_check_dev (&stat_b, TRUE) == FALSE)
return FALSE;
entry->dev_name = strdup (dev_name);
if (!entry->dev_name) {
fprintf (stderr, "malloc error in %s [line %d]\n",
__FILE__, __LINE__);
return FALSE;
}
entry->st_rdev = stat_b.st_rdev;
entry->st_mode = stat_b.st_mode;
return TRUE;
}
static int _dir_cache_set_size (int size) {
dir_cache_t *dir_cache_sav = dir_cache;
dir_cache = realloc (dir_cache,
size * sizeof (dir_cache_t));
if (dir_cache) {
cache_size = size;
return TRUE;
} else {
fprintf (stderr, "realloc error in %s [line %d]\n",
__FILE__, __LINE__);
return FALSE;
}
}
static int _dir_cache_set_entry (int num, dir_cache_t *entry) {
memcpy (&dir_cache [num], entry, sizeof (dir_cache_t));
return TRUE;
}
static int _dir_cache_append (dir_cache_t *entry) {
if (_dir_cache_set_size (cache_size + 1) == FALSE)
return FALSE;
return _dir_cache_set_entry (cache_size - 1, entry);
}
int lvm_dir_cache_add (char *dev_name) {
dir_cache_t entry;
if (_dir_cache_init_entry (&entry, dev_name) == FALSE)
return FALSE;
if (lvm_dir_cache_hit (entry.st_rdev) == FALSE)
return _dir_cache_append (&entry);
return TRUE;
}
/* below: need to change the lvm_dir_cache_hit() interface */
Obviously, a lot of this is personal preference. But, I think
the code here is much easier to understand, because each function
operates on exactly one level of abstraction, and the style
is consistent (we never pass around the dir_cache and cache_size
variables... they are global).
Further things to improve;
* more error handling could be added (why does stat fail?
etc.)
* common error handling - you probably want to have a more
convienient how to report errors than fprintf (stderr, ...).
You might be interested in the exception system in libparted
(it's very very small ;-)
* function entry / exit messages could be done with macros:
#define ENTER_FUNCTION \
debug_enter (__FUNCTION__ "() -- ENTERING\n");
#define LEAVE_FUNCTION(ret) \
do { \
debug_leave (__FUNCTION__ \
"() -- LEAVING with ret: %d\n", ret); \
return ret; \
} while (0);
/dev/clausen
More information about the linux-lvm
mailing list