#66 nolocks still tries to lock. - patch attached

closed-rejected
5
2006-11-18
2006-11-09
Anonymous
No

--- davfs2-1.1.3/src/webdav.c 2006-11-08
02:44:22.000000000 +0800
+++ davfs2-1.1.3_fixed/src/webdav.c 2006-11-09
14:43:24.000000000 +0800
@@ -109,7 +109,7 @@
From: alan@akbkhome.com

/* Lock store, lock owner and lock timeout for session.
Will be set by dav_init_webdav(). */
-static ne_lock_store *locks;
+static ne_lock_store *locks = NULL;
static char *owner;
static time_t lock_timeout;

@@ -901,7 +901,7 @@

if (locks == NULL) {
*expire = 0;
- return -1;
+ return 0;
}

DBG1(" LOCKDISCOVER %s", spath);
@@ -980,7 +980,9 @@
if (date != NULL)
free(date);
ne_request *req = ne_request_create(session,
"PUT", spath);
- ne_lock_using_resource(req, spath, 0);
+ if (locks) {
+ ne_lock_using_resource(req, spath, 0);
+ }
if (ne_set_request_body_fd(req, fd) != 0) {
ne_request_destroy(req);
return EIO;

Discussion

  • Werner Baumann

    Werner Baumann - 2006-11-09

    Logged In: YES
    user_id=1260327

    I can not reproduce this bug. I checked the server logs and
    watched the HTTP-traffic. There are no LOCK requests, when I
    set 'use_locks 0' in the configuration file, or set the
    deprecated 'nolocks' option in fstab.
    How did you configure davfs2 to not use locks?
    How did you verify that mount.davfs nevertheless sends LOCK
    request?

    Im sorry, but your patch is buggy:

    if (locks == NULL) {
    *expire = 0;
    - return -1;
    + return 0;
    }

    This badly breaks the interface of function lock_discover().
    The interface description cleary defines, that the return
    value will be -1 if *no* matching lock is found, and 0 when
    a matching lock is found. You return 0 allthough no matching
    lock is found. But all calls to lock_discover() rely on the
    correct return value.

    There is no need to explicitly initialize global variables
    with NULL, this is done automatically.

    There is no need to prevent the call of
    ne_lock_using_resource() when no locks are used. It will do
    nothing. Standard is to use locks. Not to use locks is the
    exeption. So I prefer code that is more simple and better
    matches the standard case. One unecessary, but harmless,
    function call in case of the exeption does not matter much.

    Greetings
    Werner

     
  • Werner Baumann

    Werner Baumann - 2006-11-09
    • status: open --> open-rejected
     
  • Werner Baumann

    Werner Baumann - 2006-11-09
    • assigned_to: nobody --> wbaumann
     
  • Nobody/Anonymous

    Logged In: NO

    Almost all the neon calls are conditional on lock_discover()
    returning 0:

    eg. dav_delete:
    if (ret == EACCES && lock_discover(suri->path, expire) == 0) {
    ret = ne_delete(session, suri->path);
    ret = get_error(ret, "DELETE");
    }
    when lock's are off-> lock_discover returns -1 - and the
    file is never deleted.

    same for put_file:
    if (ret == EACCES && lock_discover(spath, expire) == 0) {
    ...
    ne_request *req = ne_request_create(session, "PUT", spath);
    ...
    }

    unless I'm missing something else..

    Regards
    Alan
    alan@akbkhome.com

     
  • Werner Baumann

    Werner Baumann - 2006-11-10

    Logged In: YES
    user_id=1260327

    Hello Alan,

    you missed the *first* request. The general structure is:

    Send the request to the server (including our lock tocken,
    if there is one in the lock store).

    *If* the server denies access because the resource is
    locked by another lock:
    Try to get that lock token from the server
    and test whether we are the owner of this
    lock.
    If we can get and use this lock token:
    Send the request *again*.

    Please have a look at e.g. dav_delete() in full.

    To see whether there is really a 'nolocks still tries to
    lock'-bug, and if so how to fix it, I need some more
    information.

    Greetings
    Werner

     
  • Nobody/Anonymous

    Logged In: NO

    Ah Sorry,.. - just read the code again - I see it's doing
    as you described.

    The bug I experienced is detailed below. - I suspect the
    cause is caused by mismatched server times, and the
    timestamp checking in PUT.
    I'm going to revert to a clean source and see what is going
    on again.

    I suspect the code in dav_put() that does pre-checks before
    actually writing, is the cause of the problem.

    Regards
    Alan

    The bug title is probably wrong
    - I have a subversion server, and just recently upgraded
    from my original nolocks patch to the 2 year old davfs2, as
    when I set it up originally, subversion did not support locks.
    Anyway I've never bothered using locks on any of the
    servers. So I thought I'd try out the new davfs2, with
    /secrets and davfs2.conf etc. and discovered this:

    = With locks on at the client (and not configured at the server)
    * ~ 4 http request calls where getting done per save / etc.
    - this was problematic as I work in HK, and one of my svn
    servers is in the UK.. - a wee bit slow.
    * Frequently, the save was failing, or not happening on the
    server (locks where sent, but PUT never got sent)
    * Occasionally it was replacing a file on the server with a
    0 byte file
    (luckly it's all on a rev server)

    Anyway, I tried with locks turned off in davfs2.conf
    From what I remember
    * Sometimes The server stoped recieving any requests. when
    saving etc.
    * it occasionally wrote 0 byte files.

     
  • Werner Baumann

    Werner Baumann - 2006-11-12
    • status: open-rejected --> open
     
  • Werner Baumann

    Werner Baumann - 2006-11-12

    Logged In: YES
    user_id=1260327

    Hello Alan,

    I am not familiar with subversion. But here are some remarks
    about how davfs2 is working, that might be useful for debugging.

    0-Byte-files:
    -------------
    When a new file is created, davfs2 will first lock it on the
    server. There are currently two different reactions of
    servers, when a lock for a nonexisting file is requested:
    - old style: the server locks the name. When no file is
    uploaded and the lock is released, the entry will disappear.
    - new style: the server creates an empty file. If no file is
    uploaded and the lock is released, the empty file will stay
    on the server.

    PUT
    ---
    davfs2 indeed does some prechecking, to avoid the 'Lost
    Update Problem'. It will
    - check for the Last-Modified-Date and Etag of the file on
    the server, to make sure it has not been changed by somebody
    else in between. This *should* not happen when locks are
    use, but nevertheless it may happen.
    - it uploads the file
    - it retrieves the new values of Last-Modified and Etag.
    It would be best to do this in one request. HTTP allows for
    this. But unfortunately limitations and bugs in real servers
    make it impossible:
    - the most popular apache mod_dav has a bug and can not
    handle conditional uploads (PUT If-Not-Modified). So we have
    to do a HEAD request before the upload.
    - servers could include the new values of Last-Modified and
    Etag in the response to the PUT request. But almost no
    server does. So we have to do an additional HEAD after the
    upload.

    Currently davfs2 checks the Last-Modified-Date *and* the
    Etag value, to see whether the file has been changed
    remotely. The Etag value is more reliable. But as not all
    servers support it, davfs2 checks both. This may cause
    trouble when the clocks ar not synchronized. So I think I
    should change the code and test Last_modified only when no
    Etag is available. (I put it on my TODO list). If non
    synchronized clocks are the problem, and your server
    supports Etag, you might remove the Last-Modified related
    code in the first section of dav_put(); the part before
    'put_file(suri->path, fd, expire, mtime)'.

    Backups
    -------
    If for any reason davfs can not upload files that are
    created or changed locally, it will create a backup in the
    directory lost+found. You should check this directory
    regularly. If there are backups frequently this indicates
    serious problems with uploads.

    Greatings
    Werner

     
  • Nobody/Anonymous

    Logged In: NO

    I've reverted to 0.2 for the time being - this was taking up far to much time to debug.

    While I think the caching is very nice for Read-only webdav, it adds a layer of complexity to the code, that makes it very difficult to work with when debugging write issues. Being able to compile without caching/ or turn it off so reads/writes actually do what they are intended to, would make debugging alot easier.

    Thanks for all your input, but I probably need a good break from davfs2 hacking to regain my sanity ;)

     
  • Werner Baumann

    Werner Baumann - 2006-11-15

    Logged In: YES
    user_id=1260327
    Originator: NO

    > While I think the caching is very nice for Read-only webdav,
    > it adds a layer of complexity to the code, that makes it
    > very difficult to work with when debugging write issues.
    Caching in davfs2 was introduced to avoid unecessary traffic and to make the file system more responsive. This is for writing just as important as for reading, but of course there are more problems to be solved.

    > Being able to compile without caching/ or turn it off so
    > reads/writes actually do what they are intended to, would
    > make debugging alot easier.
    The non cachin version is still available.

    > I probably need a good break from davfs2
    > hacking to regain my sanity
    Maybe hacking davfs2 was the wrong way. A better description what you use davfs2 for and what goes on would have been more effecitve. As far as I can see now there are two main problems why the caching version of cafs2 does not fit your needs:

    - the delayed write back of changed files. davfs2 changes the mtime at this occasion and this will not work with your editor. To solve this, not delaying the write back is not an option, as this would cause a lot of traffic and delay with applications that use temporary files (and many applications use them). Your editor might use another strategy. Or we have to solve the mtime problem. I think your suggestion
    > The only other value that could be correct, would be an mtime that
    > is generated by the local write time, and stored in the cache
    > (and protected from being updated by PROPFIND's etc.)
    is the way to go. It is not 'whole can of worms' but just the problem to get the details right.

    - Using davfs2 with subversion (I do not really understand why many people want to use davfs2 instead of a proper subversion client). davfs2 and subversion do not fit well. That is because subversion creates a new version for every file you save (I just assume you use this feature of subversion). So locking as well as checking for remote changes before uploading a file, features that are really important with 'normal' webdav servers, are not only useless, but they might even interfere with what subversion is doing.

    Greetings
    Werner

    P.S.: There is an idiotic comment on bug '[ 1596105 ] make install overwrites existing config files'. It goes 'haha and who read's README's :)'. I hope it's not from you.

     
  • Werner Baumann

    Werner Baumann - 2006-11-18
    • status: open --> open-rejected
     
  • Werner Baumann

    Werner Baumann - 2006-11-18
    • status: open-rejected --> closed-rejected
     

Log in to post a comment.

Get latest updates about Open Source Projects, Conferences and News.

Sign up for the SourceForge newsletter:





No, thanks