some comments on the LVM source

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

Andrew,

thanks for providing your work results.

A general recommendation:
please provide a patch and a shorter explanation on what you want to do change with it and why.

Yep, makes sense.

We are about to split the library up into a directory hirarchy and therefor infividual .h files are likely anyway.

See above.

That's not true.
It was designed exactly the other way.

The only functions which are "bigger" than 100 lines netto are:

- pv_release_pe()
- vg_setup_for_extend()
- vg_read_with_pv_and_lv()
- pv_read_all_pv()
- vg_cfgrestore()
- pv_read_all_pv_of_vg()
- lv_setup_for_extend()
- vg_setup_for_split()
- lv_setup_for_create()
- lvm_error()
- vg_cfgbackup()
- pv_move_pe()
- pv_move_pes()

Please remember: there are about *200* functions in the library.

50 lines == large function?

Why?
The code is small already.

Anyway: I'm open for cleaner solutions and appreciate them.

Could you provide a patch against LVM 0.9.1 Beta 7 in order to ease integration?

That's an individual opinion on an individual scale ;-)

You've found a tiny notion bit.

Regards,
Heinz    -- The LVM Guy --

*** Software bugs are stupid.
    Nevertheless it needs not so stupid people to solve them ***

=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-

Heinz Mauelshagen                                 Sistina Software Inc.
Senior Consultant/Developer                       Am Sonnenhang 11
                                                  56242 Marienrachdorf
                                                  Germany
[email protected]                           +49 2626 141200
                                                       FAX 924446
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-