[lvm-devel] [PATCH] handle transient errors in lvconvert --repair

[Petr Rockai]

Takahiro Yasui <tyasui at redhat.com> writes:

>>+++ cvs-upstream/tools/lvconvert.c	2010-05-19 20:18:40.000000000 +0200
>>@@ -1225,11 +1225,11 @@
>> 		if (!lv_remove_mirrors(cmd, lv, failed_mirrors, new_log_count,
>> 				       _is_partial_lv, NULL, 0))
>> 			return 0;
>
> As I posted in the previous mail, the argument, new_log_count, is
> correct here? The argument is used to check if a log should be removed.
> I think that failed_log comes here instead of new_log_count.

Oh *jeez*.

int lv_remove_mirrors(struct cmd_context *cmd __attribute((unused)),
		      struct logical_volume *lv,
		      uint32_t mirrors, uint32_t log_count,
		      int (*is_removable)(struct logical_volume *, void *),
		      void *removable_baton,
		      uint64_t status_mask)

Notice how the parameter is named "log_count" -- I have assumed, given
that elsewhere it is called "remove_log" that this has a reverse
meaning. Wrong, it actually means the same thing as remove_log.

Thanks for noticing this. So yes, it should spell

		if (!lv_remove_mirrors(cmd, lv, failed_mirrors, failed_log,
				       _is_partial_lv, NULL, 0))

... now this was probably not being noticed, since in most cases, the
mistakenly removed log would be re-created right away by later
code. I'll fix this in my local patch.

> Or the value such as nlc in _lvconvert_mirrors_aux() comes.
>
>                 uint32_t nlc = (!new_log_count || lp->mirrors == 1) ? 1U : 0U;
> 		...
> 		} else if (!lv_remove_mirrors(cmd, lv, nmc, nlc,
>                                               is_mirror_image_removable, operable_pvs, 0))

Ugh, nlc sounds like short for new_log_count to me, which is *extremely
confusing*, since it's actually quite opposite: nlc = 1 means remove
log, nlc = 0 means retain the log. Is that right?

Yours,
   Petr.