FIXMEs

[treveradams]

svf-vfs.h:1056 /* FIXME: Return immediately if !(flags & O_CREAT) && errno != ENOENT? */ I do not think returning immediately is correct. This module is stackable, while I cannot think of any other reason than possibly audit modules, I think it would be best for svf to avoid any more processing of its own on the file, but not stop any modules this may be stacked above from functioning.

svf-vfs.h:1155 /* Return immediately if close_result == -1, and close_errno == EBADF */ if (close_result == -1 && close_errno == EBADF) goto svf_vfs_close_fail; I have already modified this comment, with the above code (untested). Why try to run the virus scanner if the file is not valid? (Sorry for not posting a pull request, I am not able to access any of my machines where my git configuration is located at the moment.)

svf-vfs.h:1152 /* FIXME: Must close after scan? */ I am not a Samba internals expert, but no where do I see a reason that it must. The virus scanning is done external to the module, so it doesn't need to remain open. The moving of the file is done by name not filedescriptor.

The rest that I see are partially implemented features for scanners other than clamav, which I do not have access to, or are things I haven't dug into yet.

Remove two of them, change the other with the two lines of code (corrected if appropriate) would be my recommendation.

What else is needed to get this ready for inclusion in Samba? Or is that still a goal?

[fumiyas]

svf-vfs.h:1056 /* FIXME: Return immediately if !(flags & O_CREAT) && errno != ENOENT? */ I do not think returning immediately is correct. This module is stackable, while I cannot think of any other reason than possibly audit modules, I think it would be best for svf to avoid any more processing of its own on the file, but not stop any modules this may be stacked above from functioning.

OK.

svf-vfs.h:1155 /* Return immediately if close_result == -1, and close_errno == EBADF */ if (close_result == -1 && close_errno == EBADF) goto svf_vfs_close_fail; I have already modified this comment, with the above code (untested). Why try to run the virus scanner if the file is not valid? (Sorry for not posting a pull request, I am not able to access any of my machines where my git configuration is located at the moment.)

SMB_VFS_NEXT_CLOSE() fails with EBADF when the specified file descriptor in the file_struct is already closed (or is an invalid number). I think svf_vfs_close() should return in that case, but should not remove a cache entry.

svf-vfs.h:1152 /* FIXME: Must close after scan? */ I am not a Samba internals expert, but no where do I see a reason that it must. The virus scanning is done external to the module, so it doesn't need to remain open. The moving of the file is done by name not filedescriptor.

OK.

[treveradams]

SMB_VFS_NEXT_CLOSE() fails with EBADF when the specified file descriptor in the file_struct is already closed (or is an invalid number). I think svf_vfs_close() should return in that case, but should not remove a cache entry.

When I was doing this work, I looked at the manpages for close for several systems. EBADF can be returned for two conditions that are not readily apparent from Linux/glibc manpages (and may not apply to Linux/glibc). First, if a file is removed on some file systems, close will fail with EBADF. The second is related, and that is if the file is moved in a way that amounts to a removal (on Linux this is moving it from one file system to another, other systems it may include moving within the file system). This was the rationale behind removing the cache entry. If the file has moved at all, the cache entry is no longer useful.

Are you certain that my choice to remove the cache entry is incorrect?

[fumiyas]

On POSIX OSes and its filesystems, close(2) never fails whether the opened file is removed (i.e., unlinked) or not. On the other hand, on Windows OS and its filesystems or CIFS, opened files can NOT be removed.

But Samba is implemented on POSIX(-ish) OSes, thus the opened files can be removed by other UNIX processes...

[treveradams]

Ok. Maybe I was remembering one of the other conditions. However, we must handle the situation where the close fails but the file is modified. Does this meet your requirements:

// Return immediately if close_result == -1, and close_errno == EBADF.
// If close failed, file likely doesn't exist, do not try to scan.
if (close_result == -1 && close_errno == EBADF)
{
    if (fsp->modified)
    {
        DEBUG(10, ("Removing cache entry (if existant): "
            "fname: %s\n", fname));
        virusfilter_cache_remove(virusfilter_h->cache_h,
            fname);
    }
    goto virusfilter_vfs_close_fail;
}

[fumiyas]

Closes with #23. Thanks!