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

[Hitoshi Mitake]

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?

> } 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.

Thanks,
Hitoshi