[sheepdog] [PATCH] journal: refactor the journaling subsystem

[Kai Zhang]

Hitoshi Mitake <mitake.hitoshi at gmail.com> writes:

> At Mon, 24 Jun 2013 01:31:41 -0700,
> Kai Zhang wrote:
>>
>> Hitoshi Mitake <mitake.hitoshi at lab.ntt.co.jp> writes:
>>
>> > Current journaling subsystem has some dirty parts because of its old
>> > journal descriptor types (epoch and config). This patch cleans them.
>> >
>> > Signed-off-by: Hitoshi Mitake <mitake.hitoshi at lab.ntt.co.jp>
>> > ---
>> >  sheep/journal.c |   31 +++++++++++++++----------------
>> >  1 files changed, 15 insertions(+), 16 deletions(-)
>> >
>> > diff --git a/sheep/journal.c b/sheep/journal.c
>> > index 50783f2..f1f94da 100644
>> > --- a/sheep/journal.c
>> > +++ b/sheep/journal.c
>> > @@ -137,21 +137,18 @@ static bool journal_entry_full_write(struct journal_descriptor *jd)
>> >
>> >  static void journal_get_path(struct journal_descriptor *jd, char *path)
>> >  {
>> > -	switch (jd->flag) {
>> > -	case JF_STORE:
>> > -	case JF_REMOVE_OBJ:
>> > -		snprintf(path, PATH_MAX, "%s/%016"PRIx64,
>> > -			 md_get_object_path(jd->oid), jd->oid);
>> > -		if (jd->flag == JF_STORE)
>> > -			sd_iprintf("%s, size %"PRIu64", off %"PRIu64", %d",
>> > -				path, jd->size, jd->offset, jd->create);
>> > -		else		/* JF_REMOVE_OBJ */
>> > -			sd_iprintf("%s (remove)", path);
>> > -		break;
>> > -	default:
>> > -		panic("unknown type of journal flag: %d", jd->flag);
>> > -		break;
>> > -	}
>> > +	snprintf(path, PATH_MAX, "%s/%016"PRIx64,
>> > +		md_get_object_path(jd->oid), jd->oid);
>> > +
>> > +	if (jd->flag == JF_STORE)
>> > +		sd_iprintf("%s, size %"PRIu64", off %"PRIu64", %d",
>> > +			path, jd->size, jd->offset, jd->create);
>> > +	else if (jd->flag == JF_REMOVE_OBJ)
>> > +		sd_iprintf("%s (remove)", path);
>> > +	else
>> > +		panic("unknown type of journal flag,"
>> > +			" the journal flag seems to be corrupted: %d",
>> > +			jd->flag);
>> >  }
>> >
>> >  static int replay_journal_entry(struct journal_descriptor *jd)
>> > @@ -168,6 +165,8 @@ static int replay_journal_entry(struct journal_descriptor *jd)
>> >		return 0;
>> >	}
>> >
>> > +	assert(jd->flag == JF_STORE);
>> > +
>>
>> Because journal_get_path(jd, patch) is needed whenever jd->flag is
>> JF_REMOVE_OBJ or JF_STORE, I think we can put it to the top to clean the code.
>> And because the value of jd->flag will be checked by both journal_get_path(),
>> and this assert, so it would be better to check it in one place.
>> The modifide code would be like:
>>
>> journal_get_path(jd, path); // no check
>> if (jd->flag == JF_REMOVE_OBJ) {
>>    unlink(path);
>>    return 0;
>> } else if (jd->flag == JF_RESTORE) {
>>    /* restore logic here */
>
> Do you mean STORE?
>

Sorry, my mistake. It should be STORE.

>> } else {
>>    panic(...);
>> }
>>
>> A switch case is also a good choice.
>
> Because current journal entry has only two types, the above
> indentation of /* restore logic here */ or switch statements would be
> too flowery.
>

How about a helper function to wrap them up?
Anyway, maybe this is not a big issue.
But what I stressed is that journal_get_path() and the logic of checking
jd->flag were duplicated. And there is a way to clean them.

Thanks,
Kyle