From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=34416 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PgDsX-00086E-2Q for qemu-devel@nongnu.org; Fri, 21 Jan 2011 05:06:03 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PgDsV-0007oc-K2 for qemu-devel@nongnu.org; Fri, 21 Jan 2011 05:06:00 -0500 Received: from mx1.redhat.com ([209.132.183.28]:41970) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PgDsV-0007oY-8s for qemu-devel@nongnu.org; Fri, 21 Jan 2011 05:05:59 -0500 Message-ID: <4D395ADF.5060102@redhat.com> Date: Fri, 21 Jan 2011 11:07:27 +0100 From: Kevin Wolf MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH 3/3] make path_combine() especially for filenames, not URLs References: <1294829822-27938-1-git-send-email-mjt@tls.msk.ru> <1294829822-27938-4-git-send-email-mjt@msgid.tls.msk.ru> In-Reply-To: <1294829822-27938-4-git-send-email-mjt@msgid.tls.msk.ru> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Michael Tokarev Cc: qemu-devel@nongnu.org Am 12.01.2011 11:57, schrieb Michael Tokarev: > Currently the two routines tries to "understand" and skip : > prefix in path arguments are path_combine() and path_is_absolute() > (the latter isn't used anywhere but in the former). This is wrong, > since notion of absolute path is, at least, protocol-specific. > > The implementation is more wrong on windows where even non-absolute > paths but with drive name (d:foo) should be treated as absolute, as > in, one can't combine, say, c:\bar with d:foo forming c:\foo as > path_combine() currently does. > > Introduce isslash() macro that works correctly on both windows and > unix, use it in is_windows_drive_prefix() (since any form of slash > can be used in constructs like //./), You're saying that \/.\ is a valid format for it? Wow... > remove path_is_absolute() and > hardcode the trivial (but right) condition in path_combine(), and > simplify path_combine() further by removing : handling > and unifying shash searching. > > Signed-off-by: Michael Tokarev > --- > block.c | 72 +++++++++++++++++++++----------------------------------------- > 1 files changed, 25 insertions(+), 47 deletions(-) > > diff --git a/block.c b/block.c > index 42d6ff1..31a821d 100644 > --- a/block.c > +++ b/block.c > @@ -71,6 +71,9 @@ static BlockDriverState *bs_snapshots; > static int use_bdrv_whitelist; > > #ifdef _WIN32 > + > +#define isslash(c) ((c) == '/' || (c) == '\\') > + > static int is_windows_drive_prefix(const char *filename) > { > return (((filename[0] >= 'a' && filename[0] <= 'z') || > @@ -83,11 +86,17 @@ int is_windows_drive(const char *filename) > if (is_windows_drive_prefix(filename) && > filename[2] == '\0') > return 1; > - if (strstart(filename, "\\\\.\\", NULL) || > - strstart(filename, "//./", NULL)) > - return 1; > + if (isslash(filename[0] && isslash(filename[1]) && Missing bracket, doesn't even compile. > + filename[2] == '.' && isslash(filename[3])) > + return 1; /* special case: windows device "//./" */ Please add curly braces. > return 0; > } > + > +#else > + > +#define isslash(c) ((c) == '/') > +#define is_windows_drive_prefix(filename) (0) Please make this a function, for consistency and type checking. The compiler is going to inline it anyway. > + > #endif > > /* check if the path starts with ":" > @@ -115,61 +124,30 @@ static char *path_has_protocol(const char *path) > (char*)p : NULL; > } > > -int path_is_absolute(const char *path) > -{ > - const char *p; > -#ifdef _WIN32 > - /* specific case for names like: "\\.\d:" */ > - if (*path == '/' || *path == '\\') > - return 1; > -#endif > - p = strchr(path, ':'); > - if (p) > - p++; > - else > - p = path; > -#ifdef _WIN32 > - return (*p == '/' || *p == '\\'); > -#else > - return (*p == '/'); > -#endif > -} > - > /* if filename is absolute, just copy it to dest. Otherwise, build a > - path to it by considering it is relative to base_path. URL are > - supported. */ > + path to it by considering it is relative to base_path. > + This is really about filenames not URLs - we don't support > + : prefix in filename since it makes no sense, especially > + if the protocol in base_path is not the same as in filename. > + */ > void path_combine(char *dest, int dest_size, > const char *base_path, > const char *filename) > { > - const char *p, *p1; > + const char *p; > int len; > > if (dest_size <= 0) > return; > - if (path_is_absolute(filename)) { > + /* on windows, d:filename should be treated as absolute too */ > + if (isslash(filename[0]) || is_windows_drive_prefix(filename)) { > pstrcpy(dest, dest_size, filename); > } else { > - p = strchr(base_path, ':'); > - if (p) > - p++; > - else > - p = base_path; > - p1 = strrchr(base_path, '/'); > -#ifdef _WIN32 > - { > - const char *p2; > - p2 = strrchr(base_path, '\\'); > - if (!p1 || p2 > p1) > - p1 = p2; > - } > -#endif > - if (p1) > - p1++; > - else > - p1 = base_path; > - if (p1 > p) > - p = p1; > + /* find last slash */ > + p = base_path + strlen(base_path); > + while(p >= base_path && !isslash(*p)) > + --p; > + p++; Please add braces. > len = p - base_path; > if (len > dest_size - 1) > len = dest_size - 1; This changes the semantics to throw away the protocol. For example fat:foo combined with bar would have resulted in fat:bar previously and results in bar now. Probably not a problem. path_combine gets even worse if filename has a protocol. It's completely unclear what it's supposed to do with protocols anyway. Kevin