bolthole / zrep

ZREP ZFS based replication and failover script from bolthole.com
Other
251 stars 57 forks source link

_refreshpull(): don't set sent senttime if sync failed #156

Closed wxcafe closed 4 years ago

ppbrown commented 4 years ago

Hello, and thank you for your interest in zrep! I especially appreciate users who are willing to dig into the code and explore for bugs.

In this case, however, it seems to me that you are "barking up the wrong tree", as the expression goes.

the zfs set $senttime line should already not be called on error.. because

    if [[ $? -ne 0 ]] ; then
            zrep_errquit Unforseen error pulling snapshot $newsnap from $srchost
    fi

actually completely exits zrep. Further code in that function should not be reached.

zrep_errquit(){
        _errprint Error: "$@"
         ....
        exit 1
}
wxcafe commented 4 years ago

woops, I should have checked in more details what zrep_errquit does! It's weird that getlastsnapsent() doesn't check for the sent time, then :)

Anyway, this PR is useless then, I'll close it

ppbrown commented 4 years ago

Perhaps you mean "its weird that getlastsnap() doesnt check for sent time". Because getlastsnapsent DOES check it. There's a separate function, "getlastsnap". That checks for last snap in the sequence. whereas getlastsnapsent, checks for actually sent.

After 8 years, and 2700 lines of code.. zrep has acquired a certain level of complexity, even for a shellscript ;)

On Mon, Apr 27, 2020 at 12:40 PM wxcafé notifications@github.com wrote:

woops, I should have checked in more details what zrep_errquit does! It's weird that getlastsnapsent() doesn't check for the sent time, then :)

Anyway, this PR is useless then, I'll close it

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/bolthole/zrep/pull/156#issuecomment-620192613, or unsubscribe https://github.com/notifications/unsubscribe-auth/AANEV6PIRJPHZ7TZQMLD2CTROXNTDANCNFSM4MSFV6CQ .

wxcafe commented 4 years ago

yeah, you're absolutely right, that's what I meant! And that's probably what should be used in #157 too instead of getlastsnapsent