Re: [Qemu-devel] [PATCH V5 3/6] block: Add WIN32 platform support for backing_file_loop_check()

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2949 bytes --]

On 08/02/2013 03:02 AM, Xu Wang wrote:
> From: Xu Wang <cngesaint@gmail.com>
> 
> Method of get_inode is different between Linux and WIN32 plateform.

s/plateform/platform/g (3 instances in the commit message)

> This patch added inode caculate method on Windows plateform so that

s/added/adds an/
s/caculate/calculation/

> backing file check could work on Windows plateform.
> 
> Signed-off-by: Xu Wang <cngesaint@gmail.com>
> ---
>  block.c | 156 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 148 insertions(+), 8 deletions(-)
> 

>  }
>  
> +#ifdef _WIN32
> +static int get_lnk_target_file(const char *lnk_file, char *filepath)
> +{
> +    unsigned int flag, offset;
> +    unsigned int sflag;
> +    char uch;
> +    int i = 0;
> +
> +    FILE *fd = fopen(lnk_file, "rb");
> +    if (!fd) {
> +        error_report("Open file %s failed.", lnk_file);

Error messages should not include '.'; also, when it is due to a system
call failure, you should include strerror() information explaining the
failure.

> +        return -1;
> +    }
> +
> +    /* Check if the file is shortcuts by reading first 4 bytes if it's 0x4c */
> +    if (fread(&flag, 4, 1, fd) != 1) {
> +        error_report("Read 0x4c field of %s failed.", lnk_file);

I don't know WIN32 programming well enough to know if this really how
you should be checking for infinite loops.  But how is this supposed to
work?  In POSIX, fopen() of a symlink opens its destination; to read a
symlink's contents, you have to use readlink() (that is, the API used to
read file contents is NOT the API used to chase link resolution).  This
whole function feels like a horrible hack, unlikely to do what you meant.


> +
> +static long get_inode(const char *filename)

Again, 'long' and 'ino_t' are not necessarily compatible types.  Use
ino_t.  And again, 'ino_t' alone does not uniquely designate a file; it
is the combination of device and inode numbers together that give
uniqueness.

> +{
> +    #ifdef _WIN32

We usually anchor # in the first column.

> +    char pbuf[PATH_MAX + 1], *p;
> +    long inode;
> +    struct stat sbuf;
> +    char path[PATH_MAX + 1];
> +    int len;
> +
> +    /* If filename contains .lnk, it's a shortcuts. Target file
> +     * need to be parsed.

How does the rest of the qemu code base handle .lnk files?  Are we
trying to dereference the target file automatically?  If so, is there
already code that does that, which we can reuse here?  It just seems
awkward that you are implementing this from scratch - either we support
reading through windows links (and should reuse that code) or we don't
(and hence it doesn't matter if the user passes us a .lnk file, we
aren't treating it any different than any other data file).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]