On Wed, 2005-01-26 15:47:00 +0100, Juliusz Chroboczek <jch@xxxxxxxxxxxxxx>
wrote in message <tpsm4ob63f.fsf@xxxxxxxxxxxxxxxxxxxxxxx>:
> If you've got time, could you please have a look at maybe_relink at
> the end of compat.c? I've only added it last week, and this function,
> if it's wrong, could cause massive data loss.
Well, I think I've now basically understood what this function tries to
do. Here are some ad-hoc comments, maybe I'll send a patch at some time:
-----------------------------
if(srcstat.st_dev != dststat.st_dev) {
return 0;
}
-----------------------------
This part looks clearly wrong to me. A return value of zero is ment to
mean "source and destination are already linked", but a hard link can
never ever spawn a st_dev (since st_dev does have filesystem meaning
here, not physical storage as in raid/lvm). So I'd return -2, "files
differ".
Another thing that we need to keep an eye on is the linking part:
-----------------------------
rc = link(src, tempname);
if(rc < 0) {
free(tempname);
return -1;
}
-----------------------------
As stated, link() is atomic even on NFS, but that's only half of the
truth. Indeed, link() *is* atomic on the server [1], but what happens if
server's positive reply isn't received because of a shortish network
outage? You'll get a -1, probably with EIO. Because I don't yet know
enough about darcs internals, I need to ask how darcs will react if the
file shows up later on, because network is available again. (I've spent
lots of time doing transaction-like things over network filesystems that
don't have them at all, thus I'm hyper-paranoid on that :-)
And the least part I need to think hard about it:
-----------------------------
while(i < tempstat.st_size) {
rc1 = read(fd1, buf1, RELINK_BUFFER_SIZE);
if(rc1 < 0) { close(fd1); close(fd2); goto fail; }
rc2 = read(fd2, buf2, RELINK_BUFFER_SIZE);
if(rc2 < 0) { close(fd1); close(fd2); goto fail; }
if(rc1 == 0 || rc1 != rc2 || memcmp(buf1, buf2, rc1) != 0) {
close(fd1); close(fd2);
unlink(tempname);
free(tempname);
return -2;
}
i += rc1;
}
-----------------------------
Comparing two files sounds easy, but really, it isn't :-) This loop is
missing EINTR and short-read handling. I don't know if it's worth
implementing these (since they're quite rare), but if this is mission
critical, I guess giving up with -2 (files differ) only because one of
the reads were short reads isn't neccessary. (I'd figure out what
higher-level Haskall sources will do in this case, though...)
MfG, JBG
[1] I seem to recall that you may get into trouble if your server only
re-exports a different server's filesystem... Also, some network
filesystems may not provide an atomic link at all, SMB with Unix
extensions would be a good candidate, but I'd need to check...
--
Jan-Benedict Glaw jbglaw@xxxxxxxxxx . +49-172-7608481 _ O _
"Eine Freie Meinung in einem Freien Kopf | Gegen Zensur | Gegen Krieg _ _ O
fuer einen Freien Staat voll Freier Bürger" | im Internet! | im Irak! O O O
ret = do_actions((curr | FREE_SPEECH) & ~(NEW_COPYRIGHT_LAW | DRM | TCPA));
signature.asc
Description: Digital signature
_______________________________________________
darcs-devel mailing list
darcs-devel@xxxxxxxxx
http://www.abridgegame.org/cgi-bin/mailman/listinfo/darcs-devel
|
