On 29/11/2010 11:28, Corinna Vinschen wrote:
> On Nov 26 13:48, Jon TURNEY wrote:
>> At the moment io_stream::exists() returns FALSE for file:// paths
>> which refer to an existing directory. Inconsistently, for
>> cygfile:// paths which refer to an existing directory, it returns
>> TRUE.
>>
>> Return a new state, IO_STREAM_EXISTS_DIRECTORY, to indicate if
>> pathname exists as a directory and update all uses appropriately
>>
>> Not sure if the use of access() in the legacy branch of
>> io_stream_cygfile::exists() is correct, looks like it's inverted
>
> Indeed.
It's somewhat academic as I think io_stream_cygfile::exists() is never used at
the moment.
>> Not sure if current exists() implementation deals correctly when other
>> attributes are set for a file, e.g. FILE_ATTRIBUTE_COMPRESSED or
>> FILE_ATTRIBUTE_ENCRYPTED, since it checks attributes against an
>> expected value rather than checking for bits being set?
>
> No, the original implementation certainly doesn't look quite right.
>
>> @@ -196,11 +197,21 @@ io_stream_cygfile::exists (const std::string& path)
>> mklongpath (wname, cygpath (normalise(path)).c_str (), len);
>> DWORD attr = GetFileAttributesW (wname);
>> if (attr != INVALID_FILE_ATTRIBUTES)
>> - return 1;
>> + return (attr & FILE_ATTRIBUTE_DIRECTORY) ?
>> IO_STREAM_EXISTS_DIRECTORY : IO_STREAM_EXISTS_FILE;
>> }
>> else if (_access (cygpath (normalise(path)).c_str (), 0))
>> - return 1;
>> - return 0;
>> + {
>> + struct _stat s;
>> + if (!_stat (cygpath (normalise(path)).c_str (), &s))
>> + {
>> + if (s.st_mode & S_IFDIR)
>> + return IO_STREAM_EXISTS_DIRECTORY;
>> +
>> + if (s.st_mode & S_IFREG)
>> + return IO_STREAM_EXISTS_FILE;
>> + }
>> + }
>> + return IO_STREAM_EXISTS_NO;
>> }
>
> I would prefer if you would use GetFileAttributesA here, just like the
> io_stream_file::exists method. This also unifies testing the
> attributes.
Oh, I used _stat() in the !IsWindowsNT() case as MSDN tells me that
GetFileAttributes() isn't available prior to Win2K. I guess I've been misled
:-)
Updated patch attached
From 13e4288208580a18544c27c9df1cd2a0c5ea40ed Mon Sep 17 00:00:00 2001
From: Jon TURNEY <jon.tur...@dronecode.org.uk>
Date: Thu, 25 Nov 2010 23:29:11 +0000
Subject: [PATCH] Make io_stream::exists() directory aware
At the moment io_stream::exists() returns FALSE for file:// paths
which refer to an existing directory. Inconsistently, for
cygfile:// paths which refer to an existing directory, it returns
TRUE.
Return a new state, IO_STREAM_EXISTS_DIRECTORY, to indicate if
pathname exists as a directory and update all uses appropriately
Change io_stream_cygfile::exists() to use GetFileAttributesA()
in the legacy branch and unify attribute testing.
(Also make exists() implementations deal correctly with other
attributes being set for a file, e.g. FILE_ATTRIBUTE_COMPRESSED or
FILE_ATTRIBUTE_ENCRYPTED, by checking attributes for bits being
set rather than against an expected value)
2010-12-16 Jon TURNEY <jon.tur...@dronecode.org.uk>
* io_stream.h (io_stream_exists_t, io_stream::exists): Change
io_stream::exists
return type to new enum io_stream_exists_t.
* io_stream.cc (exists): Ditto.
* IOStreamProvider.h (IOStreamProvider): Ditto.
* io_stream_file.h (io_stream): Ditto.
* io_stream_file.cc (IOStreamProvider, exists): Ditto.
* io_stream_cygfile.h (io_stream): Ditto.
* io_stream_cygfile.cc (IOStreamProvider, exists): Ditto. Also use
GetFileAttributesA()
and unify attribute testing.
* script.cc (try_run_script): Update use of io_stream::exists().
* install.cc (installOne): Ditto.
* download.cc (check_for_cached): Ditto.
---
IOStreamProvider.h | 2 +-
download.cc | 4 ++--
install.cc | 2 +-
io_stream.cc | 2 +-
io_stream.h | 10 +++++++++-
io_stream_cygfile.cc | 24 +++++++++++++++---------
io_stream_cygfile.h | 2 +-
io_stream_file.cc | 15 ++++++++++-----
io_stream_file.h | 2 +-
script.cc | 2 +-
10 files changed, 42 insertions(+), 23 deletions(-)
diff --git a/IOStreamProvider.h b/IOStreamProvider.h
index b30362e..1c12f42 100644
--- a/IOStreamProvider.h
+++ b/IOStreamProvider.h
@@ -25,7 +25,7 @@
class IOStreamProvider
{
public:
- virtual int exists (const std::string& ) const = 0;
+ virtual io_stream_exists_t exists (const std::string& ) const = 0;
virtual int remove (const std::string& ) const = 0;
virtual int mklink (const std::string&, const std::string&,
io_stream_link_t) const = 0;
diff --git a/download.cc b/download.cc
index 25260fd..4a3a4e4 100644
--- a/download.cc
+++ b/download.cc
@@ -91,7 +91,7 @@ check_for_cached (packagesource & pkgsource)
* std::string-ified, and doesn't use overcomplex semantics. */
std::string fullname = prefix +
(pkgsource.Canonical() ? pkgsource.Canonical() : "");
- if (io_stream::exists(fullname))
+ if (io_stream::exists(fullname) == IO_STREAM_EXISTS_FILE)
{
if (validateCachedPackage (fullname, pkgsource))
pkgsource.set_cached (fullname);
@@ -110,7 +110,7 @@ check_for_cached (packagesource & pkgsource)
{
std::string fullname = prefix + rfc1738_escape_part (n->key) + "/" +
pkgsource.Canonical ();
- if (io_stream::exists(fullname))
+ if (io_stream::exists(fullname) == IO_STREAM_EXISTS_FILE)
{
if (validateCachedPackage (fullname, pkgsource))
pkgsource.set_cached (fullname);
diff --git a/install.cc b/install.cc
index 6891fec..6508ffb 100644
--- a/install.cc
+++ b/install.cc
@@ -261,7 +261,7 @@ Installer::installOne (packagemeta &pkgm, const
packageversion &ver,
io_stream *pkgfile = NULL;
- if (!source.Cached() || !io_stream::exists (source.Cached ())
+ if (!source.Cached() || (io_stream::exists (source.Cached ()) !=
IO_STREAM_EXISTS_FILE)
|| !(pkgfile = io_stream::open (source.Cached (), "rb", 0)))
{
note (NULL, IDS_ERR_OPEN_READ, source.Cached (), "No such file");
diff --git a/io_stream.cc b/io_stream.cc
index 08ecae4..23c7e36 100644
--- a/io_stream.cc
+++ b/io_stream.cc
@@ -241,7 +241,7 @@ io_stream::gets (char *buffer, size_t length)
return buffer;
}
-int
+io_stream_exists_t
io_stream::exists (const std::string& name)
{
IOStreamProvider const *p = findProvider (name);
diff --git a/io_stream.h b/io_stream.h
index 3968acc..97528a8 100644
--- a/io_stream.h
+++ b/io_stream.h
@@ -74,6 +74,14 @@ typedef enum
}
io_stream_seek_t;
+typedef enum
+{
+ IO_STREAM_EXISTS_NO = 0,
+ IO_STREAM_EXISTS_FILE,
+ IO_STREAM_EXISTS_DIRECTORY,
+}
+io_stream_exists_t;
+
class io_stream
{
public:
@@ -103,7 +111,7 @@ public:
*/
static io_stream *open (const std::string&, const std::string&, mode_t);
static int remove (const std::string& );
- static int exists (const std::string& );
+ static io_stream_exists_t exists (const std::string& );
/* moves physical stream source to dest. A copy will be attempted if a
* pointer flip fails.
*/
diff --git a/io_stream_cygfile.cc b/io_stream_cygfile.cc
index 8da97e7..60f48aa 100644
--- a/io_stream_cygfile.cc
+++ b/io_stream_cygfile.cc
@@ -40,7 +40,7 @@ static const char *cvsid =
class CygFileProvider : public IOStreamProvider
{
public:
- int exists (const std::string& path) const
+ io_stream_exists_t exists (const std::string& path) const
{return io_stream_cygfile::exists(path);}
int remove (const std::string& path) const
{return io_stream_cygfile::remove(path);}
@@ -182,25 +182,31 @@ io_stream_cygfile::~io_stream_cygfile ()
}
/* Static members */
-int
+io_stream_exists_t
io_stream_cygfile::exists (const std::string& path)
{
get_root_dir_now ();
if (!get_root_dir ().size())
- return 0;
+ return IO_STREAM_EXISTS_NO;
+
+ DWORD attr;
if (IsWindowsNT ())
{
size_t len = cygpath (normalise(path)).size () + 7;
WCHAR wname[len];
mklongpath (wname, cygpath (normalise(path)).c_str (), len);
- DWORD attr = GetFileAttributesW (wname);
- if (attr != INVALID_FILE_ATTRIBUTES)
- return 1;
+ attr = GetFileAttributesW (wname);
}
- else if (_access (cygpath (normalise(path)).c_str (), 0))
- return 1;
- return 0;
+ else
+ {
+ attr = GetFileAttributesA (cygpath (normalise(path)).c_str ());
+ }
+
+ if (attr != INVALID_FILE_ATTRIBUTES && !(attr & FILE_ATTRIBUTE_DEVICE))
+ return (attr & FILE_ATTRIBUTE_DIRECTORY) ? IO_STREAM_EXISTS_DIRECTORY :
IO_STREAM_EXISTS_FILE;
+
+ return IO_STREAM_EXISTS_NO;
}
int
diff --git a/io_stream_cygfile.h b/io_stream_cygfile.h
index 1ece242..d0a6bcb 100644
--- a/io_stream_cygfile.h
+++ b/io_stream_cygfile.h
@@ -28,7 +28,7 @@ extern int cygmkdir_p (path_type_t isadir, const std::string&
path, mode_t mode)
class io_stream_cygfile:public io_stream
{
public:
- static int exists (const std::string& );
+ static io_stream_exists_t exists (const std::string& );
static int remove (const std::string& );
static int mklink (const std::string& , const std::string& ,
io_stream_link_t);
io_stream_cygfile (const std::string&, const std::string&, mode_t);
diff --git a/io_stream_file.cc b/io_stream_file.cc
index 91f9afa..422f081 100644
--- a/io_stream_file.cc
+++ b/io_stream_file.cc
@@ -46,7 +46,7 @@ using namespace std;
class FileProvider : public IOStreamProvider
{
public:
- int exists (const std::string& path) const
+ io_stream_exists_t exists (const std::string& path) const
{return io_stream_file::exists(path);}
int remove (const std::string& path) const
{return io_stream_file::remove(path);}
@@ -114,7 +114,7 @@ io_stream_file::~io_stream_file ()
delete [] wname;
}
-int
+io_stream_exists_t
io_stream_file::exists (const std::string& path)
{
DWORD attr;
@@ -127,9 +127,14 @@ io_stream_file::exists (const std::string& path)
mklongpath (wname, path.c_str (), len);
attr = GetFileAttributesW (wname);
}
- return attr != INVALID_FILE_ATTRIBUTES
- && attr != FILE_ATTRIBUTE_DIRECTORY
- && attr != FILE_ATTRIBUTE_DEVICE;
+
+ if (attr != INVALID_FILE_ATTRIBUTES
+ && !(attr & FILE_ATTRIBUTE_DEVICE))
+ {
+ return (attr & FILE_ATTRIBUTE_DIRECTORY) ? IO_STREAM_EXISTS_DIRECTORY :
IO_STREAM_EXISTS_FILE;
+ }
+
+ return IO_STREAM_EXISTS_NO;
}
int
diff --git a/io_stream_file.h b/io_stream_file.h
index b1d7f2e..2ec56a3 100644
--- a/io_stream_file.h
+++ b/io_stream_file.h
@@ -25,7 +25,7 @@
class io_stream_file:public io_stream
{
public:
- static int exists (const std::string& );
+ static io_stream_exists_t exists (const std::string& );
static int remove (const std::string& );
static int mklink (const std::string& , const std::string& ,
io_stream_link_t);
io_stream_file (const std::string&, const std::string&, mode_t);
diff --git a/script.cc b/script.cc
index ec363d8..f64fecf 100644
--- a/script.cc
+++ b/script.cc
@@ -305,7 +305,7 @@ try_run_script (const std::string& dir,
const std::string& fname,
const std::string& ext)
{
- if (io_stream::exists ("cygfile://" + dir + fname + ext))
+ if (io_stream::exists ("cygfile://" + dir + fname + ext) ==
IO_STREAM_EXISTS_FILE)
return Script (dir + fname + ext).run ();
return NO_ERROR;
}
--
1.7.2.3
