<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><div>On Jun 24, 2013, at 11:54 PM, Hitoshi Mitake <<a href="mailto:mitake.hitoshi@gmail.com">mitake.hitoshi@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><span style="font-family: Helvetica; font-size: medium; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; display: inline !important; float: none; ">At Mon, 24 Jun 2013 02:20:59 -0700,</span><br style="font-family: Helvetica; font-size: medium; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; "><span style="font-family: Helvetica; font-size: medium; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; display: inline !important; float: none; ">Kai Zhang wrote:</span><br style="font-family: Helvetica; font-size: medium; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; "><blockquote type="cite" style="font-family: Helvetica; font-size: medium; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; "><br>Hitoshi Mitake <<a href="mailto:mitake.hitoshi@gmail.com">mitake.hitoshi@gmail.com</a>> writes:<br><br><blockquote type="cite">At Mon, 24 Jun 2013 01:31:41 -0700,<br>Kai Zhang wrote:<br><blockquote type="cite"><br>Hitoshi Mitake <<a href="mailto:mitake.hitoshi@lab.ntt.co.jp">mitake.hitoshi@lab.ntt.co.jp</a>> writes:<br><br><blockquote type="cite">Current journaling subsystem has some dirty parts because of its old<br>journal descriptor types (epoch and config). This patch cleans them.<br><br>Signed-off-by: Hitoshi Mitake <<a href="mailto:mitake.hitoshi@lab.ntt.co.jp">mitake.hitoshi@lab.ntt.co.jp</a>><br>---<br>sheep/journal.c | 31 +++++++++++++++----------------<br>1 files changed, 15 insertions(+), 16 deletions(-)<br><br>diff --git a/sheep/journal.c b/sheep/journal.c<br>index 50783f2..f1f94da 100644<br>--- a/sheep/journal.c<br>+++ b/sheep/journal.c<br>@@ -137,21 +137,18 @@ static bool journal_entry_full_write(struct journal_descriptor *jd)<br><br>static void journal_get_path(struct journal_descriptor *jd, char *path)<br>{<br>-<span class="Apple-tab-span" style="white-space: pre; "> </span>switch (jd->flag) {<br>-<span class="Apple-tab-span" style="white-space: pre; "> </span>case JF_STORE:<br>-<span class="Apple-tab-span" style="white-space: pre; "> </span>case JF_REMOVE_OBJ:<br>-<span class="Apple-tab-span" style="white-space: pre; "> </span><span class="Apple-tab-span" style="white-space: pre; "> </span>snprintf(path, PATH_MAX, "%s/%016"PRIx64,<br>-<span class="Apple-tab-span" style="white-space: pre; "> </span><span class="Apple-tab-span" style="white-space: pre; "> </span><span class="Apple-tab-span" style="white-space: pre; "> </span><span class="Apple-converted-space"> </span>md_get_object_path(jd->oid), jd->oid);<br>-<span class="Apple-tab-span" style="white-space: pre; "> </span><span class="Apple-tab-span" style="white-space: pre; "> </span>if (jd->flag == JF_STORE)<br>-<span class="Apple-tab-span" style="white-space: pre; "> </span><span class="Apple-tab-span" style="white-space: pre; "> </span><span class="Apple-tab-span" style="white-space: pre; "> </span>sd_iprintf("%s, size %"PRIu64", off %"PRIu64", %d",<br>-<span class="Apple-tab-span" style="white-space: pre; "> </span><span class="Apple-tab-span" style="white-space: pre; "> </span><span class="Apple-tab-span" style="white-space: pre; "> </span><span class="Apple-tab-span" style="white-space: pre; "> </span>path, jd->size, jd->offset, jd->create);<br>-<span class="Apple-tab-span" style="white-space: pre; "> </span><span class="Apple-tab-span" style="white-space: pre; "> </span>else<span class="Apple-tab-span" style="white-space: pre; "> </span><span class="Apple-tab-span" style="white-space: pre; "> </span>/* JF_REMOVE_OBJ */<br>-<span class="Apple-tab-span" style="white-space: pre; "> </span><span class="Apple-tab-span" style="white-space: pre; "> </span><span class="Apple-tab-span" style="white-space: pre; "> </span>sd_iprintf("%s (remove)", path);<br>-<span class="Apple-tab-span" style="white-space: pre; "> </span><span class="Apple-tab-span" style="white-space: pre; "> </span>break;<br>-<span class="Apple-tab-span" style="white-space: pre; "> </span>default:<br>-<span class="Apple-tab-span" style="white-space: pre; "> </span><span class="Apple-tab-span" style="white-space: pre; "> </span>panic("unknown type of journal flag: %d", jd->flag);<br>-<span class="Apple-tab-span" style="white-space: pre; "> </span><span class="Apple-tab-span" style="white-space: pre; "> </span>break;<br>-<span class="Apple-tab-span" style="white-space: pre; "> </span>}<br>+<span class="Apple-tab-span" style="white-space: pre; "> </span>snprintf(path, PATH_MAX, "%s/%016"PRIx64,<br>+<span class="Apple-tab-span" style="white-space: pre; "> </span><span class="Apple-tab-span" style="white-space: pre; "> </span>md_get_object_path(jd->oid), jd->oid);<br>+<br>+<span class="Apple-tab-span" style="white-space: pre; "> </span>if (jd->flag == JF_STORE)<br>+<span class="Apple-tab-span" style="white-space: pre; "> </span><span class="Apple-tab-span" style="white-space: pre; "> </span>sd_iprintf("%s, size %"PRIu64", off %"PRIu64", %d",<br>+<span class="Apple-tab-span" style="white-space: pre; "> </span><span class="Apple-tab-span" style="white-space: pre; "> </span><span class="Apple-tab-span" style="white-space: pre; "> </span>path, jd->size, jd->offset, jd->create);<br>+<span class="Apple-tab-span" style="white-space: pre; "> </span>else if (jd->flag == JF_REMOVE_OBJ)<br>+<span class="Apple-tab-span" style="white-space: pre; "> </span><span class="Apple-tab-span" style="white-space: pre; "> </span>sd_iprintf("%s (remove)", path);<br>+<span class="Apple-tab-span" style="white-space: pre; "> </span>else<br>+<span class="Apple-tab-span" style="white-space: pre; "> </span><span class="Apple-tab-span" style="white-space: pre; "> </span>panic("unknown type of journal flag,"<br>+<span class="Apple-tab-span" style="white-space: pre; "> </span><span class="Apple-tab-span" style="white-space: pre; "> </span><span class="Apple-tab-span" style="white-space: pre; "> </span>" the journal flag seems to be corrupted: %d",<br>+<span class="Apple-tab-span" style="white-space: pre; "> </span><span class="Apple-tab-span" style="white-space: pre; "> </span><span class="Apple-tab-span" style="white-space: pre; "> </span>jd->flag);<br>}<br><br>static int replay_journal_entry(struct journal_descriptor *jd)<br>@@ -168,6 +165,8 @@ static int replay_journal_entry(struct journal_descriptor *jd)<br><span class="Apple-tab-span" style="white-space: pre; "> </span><span class="Apple-tab-span" style="white-space: pre; "> </span>return 0;<br><span class="Apple-tab-span" style="white-space: pre; "> </span>}<br><br>+<span class="Apple-tab-span" style="white-space: pre; "> </span>assert(jd->flag == JF_STORE);<br>+<br></blockquote><br>Because journal_get_path(jd, patch) is needed whenever jd->flag is<br>JF_REMOVE_OBJ or JF_STORE, I think we can put it to the top to clean the code.<br>And because the value of jd->flag will be checked by both journal_get_path(),<br>and this assert, so it would be better to check it in one place.<br>The modifide code would be like:<br><br>journal_get_path(jd, path); // no check<br>if (jd->flag == JF_REMOVE_OBJ) {<br> unlink(path);<br> return 0;<br>} else if (jd->flag == JF_RESTORE) {<br> /* restore logic here */<br></blockquote><br>Do you mean STORE?<br><br></blockquote><br>Sorry, my mistake. It should be STORE.<br><br><blockquote type="cite"><blockquote type="cite">} else {<br> panic(...);<br>}<br><br>A switch case is also a good choice.<br></blockquote><br>Because current journal entry has only two types, the above<br>indentation of /* restore logic here */ or switch statements would be<br>too flowery.<br><br></blockquote><br>How about a helper function to wrap them up?<br>Anyway, maybe this is not a big issue.<br></blockquote><br style="font-family: Helvetica; font-size: medium; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; "><span style="font-family: Helvetica; font-size: medium; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; display: inline !important; float: none; ">Do you mean implementing functions for each types of journal entry? I</span><br style="font-family: Helvetica; font-size: medium; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; "><span style="font-family: Helvetica; font-size: medium; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; display: inline !important; float: none; ">feel it is an over abstraction.</span><br style="font-family: Helvetica; font-size: medium; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; "><br style="font-family: Helvetica; font-size: medium; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; "><blockquote type="cite" style="font-family: Helvetica; font-size: medium; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; ">But what I stressed is that journal_get_path() and the logic of checking<br>jd->flag were duplicated. And there is a way to clean them.<br><br></blockquote><br style="font-family: Helvetica; font-size: medium; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; "><span style="font-family: Helvetica; font-size: medium; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; display: inline !important; float: none; ">IIUC, you think that the assert(jd->flag == JF_STORE); is</span><br style="font-family: Helvetica; font-size: medium; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; "><span style="font-family: Helvetica; font-size: medium; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; display: inline !important; float: none; ">redundant. Of course we can remove the assert practically. But I added</span><br style="font-family: Helvetica; font-size: medium; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; "><span style="font-family: Helvetica; font-size: medium; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; display: inline !important; float: none; ">the assert mainly for documentation. And it produces no runtime</span><br style="font-family: Helvetica; font-size: medium; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; "><span style="font-family: Helvetica; font-size: medium; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; display: inline !important; float: none; ">overhead. I believe this would be helpful for understanding of other</span><br style="font-family: Helvetica; font-size: medium; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; "><span style="font-family: Helvetica; font-size: medium; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; display: inline !important; float: none; ">developers.</span></blockquote></div><br><div><br></div><div>I see.</div><div>Maybe it's just a matter of taste.</div><div>And because I'm not a maintainer currently, please ignore my comments and</div><div>wait for Yuan or Kazutaka to merge this patch.</div><div><br></div><div>Thanks,</div><div>Kyle</div></body></html>