[Qemu-devel] r6677 broke access to physical FDD on Win32

[Qemu-devel] r6677 broke access to physical FDD on Win32

[Robert Riebisch]

Hi!

If I run QEMU on Win32 with option "-fda a:", then I'm unable to access
physical floppies from guest OS. "Physical" also includes virtual drives
created by <http://chitchat.at.infoseek.co.jp/vmware/vfd.html>.

I'm also unable to boot such floppies ("-boot a"). Error message is:
"Boot failed: not a bootable floppy disk"

By doing some builds I narrowed the problem down to
<http://svn.savannah.gnu.org/viewvc?view=rev&root=qemu&revision=6677> by
Anthony Liguori.

Robert Riebisch
-- 
BTTR Software
http://www.bttr-software.de/

Re: [Qemu-devel] r6677 broke access to physical FDD on Win32

[Anthony Liguori]

Robert Riebisch wrote:
> Hi!
>
> If I run QEMU on Win32 with option "-fda a:", then I'm unable to access
> physical floppies from guest OS. "Physical" also includes virtual drives
> created by <http://chitchat.at.infoseek.co.jp/vmware/vfd.html>.
>
> I'm also unable to boot such floppies ("-boot a"). Error message is:
> "Boot failed: not a bootable floppy disk"
>
> By doing some builds I narrowed the problem down to
> <http://svn.savannah.gnu.org/viewvc?view=rev&root=qemu&revision=6677> by
> Anthony Liguori.
>   

I suspect this code:

block-raw-win32.c:raw_getlength():

    case FTYPE_HARDDISK:
        status = DeviceIoControl(s->hfile, IOCTL_DISK_GET_DRIVE_GEOMETRY_EX,
                                 NULL, 0, &dg, sizeof(dg), &count, NULL);
        if (status != 0) {
            l = dg.DiskSize;
        }
        break;

Is not doing the correct thing.

Looking at the code, perhaps you should be saying -fda //./a.  I don't 
know a lot about windows but that sure does look weird to me.

Regards,

Anthony Liguori

> Robert Riebisch
>   

Re: [Qemu-devel] r6677 broke access to physical FDD on Win32

[Robert Riebisch]

Anthony Liguori wrote:

> block-raw-win32.c:raw_getlength():
> 
>     case FTYPE_HARDDISK:

A floppy disk drive is hard disk?

>         status = DeviceIoControl(s->hfile, IOCTL_DISK_GET_DRIVE_GEOMETRY_EX,
>                                  NULL, 0, &dg, sizeof(dg), &count, NULL);
>         if (status != 0) {
>             l = dg.DiskSize;
>         }
>         break;

> Looking at the code, perhaps you should be saying -fda //./a.

"-fda //./a" fails, but "-fda //./a:" works for the stable branch. It
does NOT work for original qemu-0.10.0.tar.gz.

Now we "just" need to allow the old syntax. :-)

Robert Riebisch
-- 
BTTR Software
http://www.bttr-software.de/

Re: [Qemu-devel] r6677 broke access to physical FDD on Win32

[Anthony Liguori]

Robert Riebisch wrote:
> Anthony Liguori wrote:
>
>   
>> block-raw-win32.c:raw_getlength():
>>
>>     case FTYPE_HARDDISK:
>>     
>
> A floppy disk drive is hard disk?
>   

I didn't write the code, but yeah, that confused me too.

>> Looking at the code, perhaps you should be saying -fda //./a.
>>     
>
> "-fda //./a" fails, but "-fda //./a:" works for the stable branch. It
> does NOT work for original qemu-0.10.0.tar.gz.
>
> Now we "just" need to allow the old syntax. :-)
>   

The patch that I wrote fixed a bug where a guest could send IO requests 
to a guest backend that were greater than the size of the device.  The 
size of the device is bdrv_getlength().  In this case, when you specify 
"a:" on Windows, it doesn't correctly detect that it is a physical 
device.  It cannot get the size of the device correctly because of 
this.  This wasn't an issue before because of the aforementioned bug.

Is "a:" a valid filename in Windows?  Is there a way in Windows to 
detect that a given file is actually a physical device?

Regards,

Anthony Liguori

> Robert Riebisch
>   

Re: [Qemu-devel] r6677 broke access to physical FDD on Win32

[Luca Tettamanti]

On Sat, Mar 21, 2009 at 12:02 AM, Anthony Liguori <anthony@codemonkey.ws> wrote:
> Is "a:" a valid filename in Windows?

No, ":" is not a valid char for a file name.

> Is there a way in Windows to detect
> that a given file is actually a physical device?

: is used for delimiting the mount point; actually the internals are a
bit more complicated (see [1]), stuff like A: or C: are reserved
special names, the "real" device names are \\.\A: or \\.\C: (where
\\.\ is the namespace used for devices).

Luca
http://msdn.microsoft.com/en-us/library/aa365247.aspx#path_names_and_namespaces

Re: [Qemu-devel] r6677 broke access to physical FDD on Win32

[Anthony Liguori]

Luca Tettamanti wrote:
> On Sat, Mar 21, 2009 at 12:02 AM, Anthony Liguori <anthony@codemonkey.ws> wrote:
>   
>> Is "a:" a valid filename in Windows?
>>     
>
> No, ":" is not a valid char for a file name.
>
>   
>>  Is there a way in Windows to detect
>> that a given file is actually a physical device?
>>     
>
> : is used for delimiting the mount point; actually the internals are a
> bit more complicated (see [1]), stuff like A: or C: are reserved
> special names, the "real" device names are \\.\A: or \\.\C: (where
> \\.\ is the namespace used for devices).
>   

Looks like the code is doing the right thing.  Someone running windows 
is going to have to debug this because a: should be transformed to 
\\.\a: as far as I can read the code.

Regards,

Anthony Liguori

> Luca
> http://msdn.microsoft.com/en-us/library/aa365247.aspx#path_names_and_namespaces
>
>
>   

Re: [Qemu-devel] r6677 broke access to physical FDD on Win32

[Luca Tettamanti]

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

On Fri, Mar 20, 2009 at 11:01 PM, Anthony Liguori <anthony@codemonkey.ws> wrote:
> Robert Riebisch wrote:
>>
>> Hi!
>>
>> If I run QEMU on Win32 with option "-fda a:", then I'm unable to access
>> physical floppies from guest OS. "Physical" also includes virtual drives
>> created by <http://chitchat.at.infoseek.co.jp/vmware/vfd.html>.
>>
>> I'm also unable to boot such floppies ("-boot a"). Error message is:
>> "Boot failed: not a bootable floppy disk"
>>
>> By doing some builds I narrowed the problem down to
>> <http://svn.savannah.gnu.org/viewvc?view=rev&root=qemu&revision=6677> by
>> Anthony Liguori.
>>
>
> I suspect this code:
>
> block-raw-win32.c:raw_getlength():
>
>   case FTYPE_HARDDISK:
>       status = DeviceIoControl(s->hfile, IOCTL_DISK_GET_DRIVE_GEOMETRY_EX,
>                                NULL, 0, &dg, sizeof(dg), &count, NULL);
>       if (status != 0) {
>           l = dg.DiskSize;
>       }
>       break;
>
> Is not doing the correct thing.

Sort of; the real problem is in find_device_type(), it thinks that the
device is a file :S

The bug is here:
  type = GetDriveType(s->drive_path);
  if (type == DRIVE_CDROM)
    return FTYPE_CD;
  else
    return FTYPE_FILE;

GetDriveType("a:") returns DRIVE_REMOVABLE, which according to MSDN
means "The drive has removable media; for example, a floppy drive,
thumb drive, or flash card reader."; the code is not expecting such a
value so it sets the type to FTYPE_FILE; raw_getlength() then uses
GetFileSize() which of course fails...

Here's a patch for properly handling the return value of GetDriveType
(sorry for the attachment, but I'm currently using a remote VM over
RDP and don't have a decent mailer installed).

Luca

[-- Attachment #2: fix-fda.diff --]
[-- Type: application/octet-stream, Size: 856 bytes --]

Fix find_device_type() to correctly identify floppy disk devices;
they are reported as DRIVE_REMOVABLE by win32.

Signed-off-by: Luca Tettamanti

diff --git a/block-raw-win32.c b/block-raw-win32.c
index d034b4a..ba9b0cf 100644
--- a/block-raw-win32.c
+++ b/block-raw-win32.c
@@ -408,10 +408,15 @@ static int find_device_type(BlockDriverState *bs, const char *filename)
             return FTYPE_HARDDISK;
         snprintf(s->drive_path, sizeof(s->drive_path), "%c:\\", p[0]);
         type = GetDriveType(s->drive_path);
-        if (type == DRIVE_CDROM)
+        switch (type) {
+        case DRIVE_REMOVABLE:
+        case DRIVE_FIXED:
+            return FTYPE_HARDDISK;
+        case DRIVE_CDROM:
             return FTYPE_CD;
-        else
+        default:
             return FTYPE_FILE;
+        }
     } else {
         return FTYPE_FILE;
     }

Re: [Qemu-devel] r6677 broke access to physical FDD on Win32

[Robert Riebisch]

Anthony Liguori wrote:

> Looks like the code is doing the right thing.  Someone running windows 
> is going to have to debug this because a: should be transformed to 
> \\.\a: as far as I can read the code.

Japheth posted the following proposal to my forum, where I advertize
running DOS in QEMU
(<http://www.bttr-software.de/forum/forum_entry.php?id=6249>):

I didn't test so far, because I was AFK for this weekend.

--- block-raw-win32.co	Wed Mar  4 22:54:44 2009
+++ block-raw-win32.c	Sat Mar 21 05:51:34 2009
@@ -32,6 +32,7 @@
 #define FTYPE_FILE 0
 #define FTYPE_CD     1
 #define FTYPE_HARDDISK 2
+#define FTYPE_DISK 3

 typedef struct BDRVRawState {
     HANDLE hfile;
@@ -305,6 +306,7 @@
         if (l.LowPart == 0xffffffffUL && GetLastError() != NO_ERROR)
             return -EIO;
         break;
+    case FTYPE_DISK:
     case FTYPE_CD:
         if (!GetDiskFreeSpaceEx(s->drive_path, &available, &total,
&total_free))
             return -EIO;
@@ -402,7 +404,7 @@
         if (type == DRIVE_CDROM)
             return FTYPE_CD;
         else
-            return FTYPE_FILE;
+            return FTYPE_DISK;
     } else {
         return FTYPE_FILE;
     }
@@ -411,7 +413,7 @@
 static int hdev_open(BlockDriverState *bs, const char *filename, int flags)
 {
     BDRVRawState *s = bs->opaque;
-    int access_flags, create_flags;
+    int access_flags, create_flags, share_flags;
     DWORD overlapped;
     char device_name[64];

@@ -429,6 +431,10 @@
         }
     }
     s->type = find_device_type(bs, filename);
+    if (s->type == FTYPE_DISK)
+        share_flags = FILE_SHARE_READ | FILE_SHARE_WRITE;
+    else
+        share_flags = FILE_SHARE_READ;

     if ((flags & BDRV_O_ACCESS) == O_RDWR) {
         access_flags = GENERIC_READ | GENERIC_WRITE;
@@ -447,7 +453,7 @@
     else if (!(flags & BDRV_O_CACHE_WB))
         overlapped |= FILE_FLAG_WRITE_THROUGH;
     s->hfile = CreateFile(filename, access_flags,
-                          FILE_SHARE_READ, NULL,
+                          share_flags, NULL,
                           create_flags, overlapped, NULL);
     if (s->hfile == INVALID_HANDLE_VALUE) {
         int err = GetLastError();

Robert Riebisch
-- 
BTTR Software
http://www.bttr-software.de/

Re: [Qemu-devel] r6677 broke access to physical FDD on Win32

[Anthony Liguori]

Robert Riebisch wrote:
> Anthony Liguori wrote:
>
>   
>> Looks like the code is doing the right thing.  Someone running windows 
>> is going to have to debug this because a: should be transformed to 
>> \\.\a: as far as I can read the code.
>>     
>
> Japheth posted the following proposal to my forum, where I advertize
> running DOS in QEMU
> (<http://www.bttr-software.de/forum/forum_entry.php?id=6249>):
>
> I didn't test so far, because I was AFK for this weekend.
>   

Needs a signed-off-by.

Regards,

Anthony Liguori

> --- block-raw-win32.co	Wed Mar  4 22:54:44 2009
> +++ block-raw-win32.c	Sat Mar 21 05:51:34 2009
> @@ -32,6 +32,7 @@
>  #define FTYPE_FILE 0
>  #define FTYPE_CD     1
>  #define FTYPE_HARDDISK 2
> +#define FTYPE_DISK 3
>
>  typedef struct BDRVRawState {
>      HANDLE hfile;
> @@ -305,6 +306,7 @@
>          if (l.LowPart == 0xffffffffUL && GetLastError() != NO_ERROR)
>              return -EIO;
>          break;
> +    case FTYPE_DISK:
>      case FTYPE_CD:
>          if (!GetDiskFreeSpaceEx(s->drive_path, &available, &total,
> &total_free))
>              return -EIO;
> @@ -402,7 +404,7 @@
>          if (type == DRIVE_CDROM)
>              return FTYPE_CD;
>          else
> -            return FTYPE_FILE;
> +            return FTYPE_DISK;
>      } else {
>          return FTYPE_FILE;
>      }
> @@ -411,7 +413,7 @@
>  static int hdev_open(BlockDriverState *bs, const char *filename, int flags)
>  {
>      BDRVRawState *s = bs->opaque;
> -    int access_flags, create_flags;
> +    int access_flags, create_flags, share_flags;
>      DWORD overlapped;
>      char device_name[64];
>
> @@ -429,6 +431,10 @@
>          }
>      }
>      s->type = find_device_type(bs, filename);
> +    if (s->type == FTYPE_DISK)
> +        share_flags = FILE_SHARE_READ | FILE_SHARE_WRITE;
> +    else
> +        share_flags = FILE_SHARE_READ;
>
>      if ((flags & BDRV_O_ACCESS) == O_RDWR) {
>          access_flags = GENERIC_READ | GENERIC_WRITE;
> @@ -447,7 +453,7 @@
>      else if (!(flags & BDRV_O_CACHE_WB))
>          overlapped |= FILE_FLAG_WRITE_THROUGH;
>      s->hfile = CreateFile(filename, access_flags,
> -                          FILE_SHARE_READ, NULL,
> +                          share_flags, NULL,
>                            create_flags, overlapped, NULL);
>      if (s->hfile == INVALID_HANDLE_VALUE) {
>          int err = GetLastError();
>
> Robert Riebisch
>   

Re: [Qemu-devel] r6677 broke access to physical FDD on Win32

[Luca Tettamanti]

On Sun, Mar 22, 2009 at 9:49 PM, Robert Riebisch <rr@bttr-software.de> wrote:
> --- block-raw-win32.co  Wed Mar  4 22:54:44 2009
> +++ block-raw-win32.c   Sat Mar 21 05:51:34 2009
> @@ -429,6 +431,10 @@
>         }
>     }
>     s->type = find_device_type(bs, filename);
> +    if (s->type == FTYPE_DISK)
> +        share_flags = FILE_SHARE_READ | FILE_SHARE_WRITE;
> +    else
> +        share_flags = FILE_SHARE_READ;

This look dangerous (as in "the guest might corrupt the fs"), what it
the rationale behind this change?

Luca

Re: [Qemu-devel] r6677 broke access to physical FDD on Win32

[Japheth]

> This look dangerous (as in "the guest might corrupt the fs"), what it
> the rationale behind this change?

there's a sentence in MSDN about the Win32 CreateFile() API for "disk devices":

------------------------------------------------------------------------
When opening a floppy disk or a partition on a hard disk, you must set the 
FILE_SHARE_WRITE flag in the dwShareMode parameter.
------------------------------------------------------------------------

Re: [Qemu-devel] r6677 broke access to physical FDD on Win32

[Robert Riebisch]

Any news on this issue?

Robert Riebisch
-- 
BTTR Software
http://www.bttr-software.de/

Re: [Qemu-devel] r6677 broke access to physical FDD on Win32

[Anthony Liguori]

Luca Tettamanti wrote:
> On Fri, Mar 20, 2009 at 11:01 PM, Anthony Liguori <anthony@codemonkey.ws> wrote:
>   
>> Robert Riebisch wrote:
>>     
>>> Hi!
>>>
>>> If I run QEMU on Win32 with option "-fda a:", then I'm unable to access
>>> physical floppies from guest OS. "Physical" also includes virtual drives
>>> created by <http://chitchat.at.infoseek.co.jp/vmware/vfd.html>.
>>>
>>> I'm also unable to boot such floppies ("-boot a"). Error message is:
>>> "Boot failed: not a bootable floppy disk"
>>>
>>> By doing some builds I narrowed the problem down to
>>> <http://svn.savannah.gnu.org/viewvc?view=rev&root=qemu&revision=6677> by
>>> Anthony Liguori.
>>>
>>>       
>> I suspect this code:
>>
>> block-raw-win32.c:raw_getlength():
>>
>>   case FTYPE_HARDDISK:
>>       status = DeviceIoControl(s->hfile, IOCTL_DISK_GET_DRIVE_GEOMETRY_EX,
>>                                NULL, 0, &dg, sizeof(dg), &count, NULL);
>>       if (status != 0) {
>>           l = dg.DiskSize;
>>       }
>>       break;
>>
>> Is not doing the correct thing.
>>     
>
> Sort of; the real problem is in find_device_type(), it thinks that the
> device is a file :S
>
> The bug is here:
>   type = GetDriveType(s->drive_path);
>   if (type == DRIVE_CDROM)
>     return FTYPE_CD;
>   else
>     return FTYPE_FILE;
>
> GetDriveType("a:") returns DRIVE_REMOVABLE, which according to MSDN
> means "The drive has removable media; for example, a floppy drive,
> thumb drive, or flash card reader."; the code is not expecting such a
> value so it sets the type to FTYPE_FILE; raw_getlength() then uses
> GetFileSize() which of course fails...
>
> Here's a patch for properly handling the return value of GetDriveType
> (sorry for the attachment, but I'm currently using a remote VM over
> RDP and don't have a decent mailer installed).
>
> Luca
>   
Applied.  Thanks.

-- 
Regards,

Anthony Liguori

Re: [Qemu-devel] r6677 broke access to physical FDD on Win32

[Luca Tettamanti]

On Tue, Apr 7, 2009 at 3:46 AM, Anthony Liguori <aliguori@us.ibm.com> wrote:
> Luca Tettamanti wrote:
>> The bug is here:
>>  type = GetDriveType(s->drive_path);
>>  if (type == DRIVE_CDROM)
>>    return FTYPE_CD;
>>  else
>>    return FTYPE_FILE;
>>
>> GetDriveType("a:") returns DRIVE_REMOVABLE, which according to MSDN
>> means "The drive has removable media; for example, a floppy drive,
>> thumb drive, or flash card reader."; the code is not expecting such a
>> value so it sets the type to FTYPE_FILE; raw_getlength() then uses
>> GetFileSize() which of course fails...
>>
>> Here's a patch for properly handling the return value of GetDriveType
>> (sorry for the attachment, but I'm currently using a remote VM over
>> RDP and don't have a decent mailer installed).
>>
>
> Applied.  Thanks.

There is also this patch floating around:
http://thread.gmane.org/gmane.comp.emulators.qemu/39851

which also addresses the lack of FILE_SHARE_WRITE flag.

Luca