Opened 4 years ago

Closed 4 years ago

#137 closed defect (fixed)

QDirModel shows phantom drive B:

Reported by: rudi Owned by:
Priority: blocker Milestone: Qt 4.6.2
Component: QtCore Version: 4.5.1 GA
Severity: highest Keywords:
Cc:

Description

Using a QDirModel to list all drives shows an empty line (apparently a placeholder for floppy drive B:). See attachment...

Attachments (1)

qdirmodel.jpg (26.3 KB) - added by rudi 4 years ago.

Download all attachments as: .zip

Change History (21)

Changed 4 years ago by rudi

comment:1 Changed 4 years ago by dmik

Are you sure it's 4.5.1 GA? I see something similar here with the trunk (i.e. 4.6.1) but 4.5.1 GA definitely doesn't show this behavior.

4.5.1 did show this problem in the past though but I specifically fixed it and it may be a 4.6.1 regression: they probably changed something at the same place so that my fix was effectively discarded.

comment:2 Changed 4 years ago by dmik

The similar defect from Qt 4.5.1 is #109.

comment:3 Changed 4 years ago by rudi

Are you sure it's 4.5.1 GA?

Yes. However, it's on a Warp4 non-LVM system. Furthermore, it takes about 8 seconds to populate the box for the first time. On a similar Windows machine this happens in 2 seconds. The long delay happens when the top-level drive information is queried.

comment:4 Changed 4 years ago by rudi

Maybe I should open another ticket that deals with these delays. Setting "lazyChildCount" to true makes the performance about the same I have in Windows. But without that property set, it can become completely inacceptable (more than half a minute in case of NetDrive? installed).

comment:5 Changed 4 years ago by dmik

I will have to check it again. Bad thing is that I can't reproduce it here and this is a way too complex code (my opinion is that the complexity is totally inadequate to the amount of work the file dialog actually does). Probably I will have to ask you to try the debug build with some additional debug statements. It will definitely take some time. I will report back here.

No need for another ticket but refer to #6 too (and partly to #124).

comment:6 Changed 4 years ago by rudi

  • Component changed from General to QtCore

I think I have a solution, which also reduces - if not avoids - the error popups for not-ready drives. In \src\corelib\io\qfsfileengine_os2.cpp, method doStat() try to replace:

BYTE drv[3] = { filePath.at(0).cell(), ':', '\0' };
BYTE buf[sizeof(FSQBUFFER2) + (3 * CCHMAXPATH)] = {0};
ULONG bufSize = sizeof(buf);
PFSQBUFFER2 pfsq = (PFSQBUFFER2) buf;
APIRET arc = DosQueryFSAttach(drv, 0, FSAIL_QUERYNAME, pfsq,
                              &bufSize);
if (arc == ERROR_NOT_READY) {
    // it must be a removable drive with no media in it
...

by

HFILE  h;
ULONG  a;
APIRET arc;
BYTE   drv[3] = { filePath.at(0).cell(), ':', '\0' };

arc = DosOpen(drv, &h, &a, 0, FILE_NORMAL, FILE_OPEN,
	      OPEN_ACCESS_READONLY | OPEN_SHARE_DENYNONE |
	      OPEN_FLAGS_DASD | OPEN_FLAGS_FAIL_ON_ERROR, NULL);

if (arc == NO_ERROR) {
    DosClose(h);
} else if (arc == ERROR_NOT_SUPPORTED ||
	   arc == ERROR_NETWORK_ACCESS_DENIED ) {
    BYTE buf[sizeof(FSQBUFFER2) + (3 * CCHMAXPATH)] = {0};
    ULONG bufSize = sizeof(buf);
    PFSQBUFFER2 pfsq = (PFSQBUFFER2) buf;
    arc = DosQueryFSAttach(drv, 0, FSAIL_QUERYNAME, pfsq, &bufSize);
}

if (arc == ERROR_NOT_READY || arc == ERROR_DISK_CHANGE) {
    // it must be a removable drive with no media in it

...

comment:7 Changed 4 years ago by rudi

To add: the above does not avoid the error popups completely, but reduces the number of them. The situation can be improved a bit more by changing the method isSymLink() to not call QT_FSTAT for root drives (assuming that they are never symlinks in the sense of KLIBC). The remaining popups seem to originate from opendir() in qsfileengine_iterator_os2.cpp...

comment:8 Changed 4 years ago by dmik

Rudi, thanks for the patch. Don't get me wrong but if your complete point is to get rid of these hardware error popups, then I don't think that putting the efforts in finding and patching all the responsible code is worth it, provided that we can solve it at once by simply adding AUTOFAIL=YES to CONFIG.SYS or even further by doing #131.

OTOH, if your patch helps to minimize the delay when populating the removable drives when the file dialog is opened for the first time, then I will surely apply it. The problem here is that I run OS/2 in a virtual environment herer and therefore I don't have real hardware delays. I also don't have any real floppy hardware so that I could test it on a real OS/2 installation even if I managed to install it.

So, to summarize, answer these questions please (I assume that you have OS/2 running on the real hardware and you have a real floppy drive):

  1. Does the current implementation (without your patch form commment:6) show a significant delay when you first open the file dialog, before you explicitly click Computer to enumerate all drives?
  2. If yes, is this delay caused by the floppy drive moving its gear (and making a well known noice) in attempt to read the missing medium?
  3. If yes, does your patch from comment:6 eliminate this delay?

I assume that using DosQueryFSAttach() removed the behavior described in 1. that we had in early Qt4 builds. Correct me if I'm wrong.

comment:9 Changed 4 years ago by dmik

  • Milestone changed from Qt Enhanced to Qt 4.6.1

comment:10 Changed 4 years ago by dmik

  • Priority changed from major to blocker
  • Severity changed from low to highest

comment:11 Changed 4 years ago by rudi

Dmik,

Q1: No. The delay is about half a second, if not less.
Q2..Q3: n/a.

However, with my patches (see summary later) applied, the time needed to populate the box with drives (i.e. after clicking on "Computer") is cut by half at least. This can also be observed with the dirview example. Also B: is shown as regular drive in dirview. Note that the standard file open box seems to handle this different, since it doesn't show the B: drive at all. As a side effect, no error popups appear when opening a file box / QDirView (except you manage to change the "ready" state of a drive while information is queried).

There is still some noise from the floppy when the file dialogs open, but it seems to happen less often.

Summary of changes

qfsfileengine_os2.cpp / QFSFileEnginePrivate::doStat()

Removed DosQueryFSAttach() completely. Note that DosOpen? will also fail on network drives (here even with different error codes depending on the type of network !). I'm not sure if it makes sense to try DosQueryFSAttach() in thats situation to obtain "ready" information for a network drive...

    if (isDriveRootPath(filePath)) {
        not_ready = 0;

	HFILE  h;		
	ULONG  a;		
	APIRET arc;		
	BYTE   drv[3] = { filePath.at(0).cell(), ':', '\0' };

	arc = DosOpen(drv, &h, &a, 0, FILE_NORMAL, FILE_OPEN,
		      OPEN_ACCESS_READONLY | OPEN_SHARE_DENYNONE |
		      OPEN_FLAGS_DASD | OPEN_FLAGS_FAIL_ON_ERROR, NULL);
	if (arc == NO_ERROR) {
	    DosClose(h);
	}

	if (arc == ERROR_NOT_READY || arc == ERROR_DISK_CHANGE) {
            // it must be a removable drive with no media in it
            memset(&st, 0, sizeof(st));
            st.st_mode = S_IFDIR | 0777;
            not_ready = 1;
            could_stat = true;
        } else if (arc == ERROR_INVALID_DRIVE) {
            could_stat = false;
        } else {
            could_stat = (QT_STAT(nativeFilePath.constData(), &st) == 0);
        }
    }

    ...

qfsfileengine_os2.cpp / QFSFileEnginePrivate::isSymlink()

I assume that a drive root is never a symlink. Is it ?

    if (need_lstat) {
        QFSFileEnginePrivate *that = const_cast<QFSFileEnginePrivate *>(this);
        that->need_lstat = false;

        if (!isDriveRootPath(nativeFilePath.constData())) {
           QT_STATBUF st;          // don't clobber our main one
           that->is_link = (QT_LSTAT(nativeFilePath.constData(), &st) == 0) ? S_ISLNK(st.st_mode) : false;
	}
    }

qfsfileengine_iterator_os2.cpp / QFSFileEngineIterator::hasNext()

opendir is causing three error popups. Thus I assume, that it tries to access the drive 3 times. Therefore doing a check before should reduce the time in the not-ready case. Note that you also need to add:

#define INCL_DOSERRORS
#include <os2.h>

    if (!path.endsWith(QLatin1Char('/')))
        path.append(QLatin1Char('/'));

	HFILE  h;	
	ULONG  a;
	APIRET arc;
	BYTE   drv[3] = { path.at(0).cell(), ':', '\0' };

	arc = DosOpen(drv, &h, &a, 0, FILE_NORMAL, FILE_OPEN,
		      OPEN_ACCESS_READONLY | OPEN_SHARE_DENYNONE |
		      OPEN_FLAGS_DASD | OPEN_FLAGS_FAIL_ON_ERROR, NULL);
	if (arc == NO_ERROR) {
	    DosClose(h);
	}

	if (arc == ERROR_NOT_READY || arc == ERROR_DISK_CHANGE || arc == ERROR_INVALID_DRIVE) {
	    that->platform->dir = 0; that->platform->done = true;
	} else if ((that->platform->dir = ::opendir(QFile::encodeName(path).data())) == 0) {
            that->platform->done = true;
        } else {

      ...

comment:12 Changed 4 years ago by dmik

I will be checking all that right now but a quick question: do you want to say that DosQueryFSAttach() causes more noise on the floppy drive with no disk in than DosOpen?("A:")? Do you also want to say that DosQueryFSAttach() causes the error popup to show up while DosOpen("A:") doesn't?

Regarding A: vs B:. I recall from the DOS times, that it's a trick DOS uses on systems with one real floppy drive (to make e.g. copying from one floppy to another possible): when B: is accessed, it asks you to insert another disk and then attaches letter B: to the same floppy where A: was attached and detaches A: from it. The only thing I wonder is how the standard file dialog does that switch (DosFSAttach() perhaps). I don't think we need to bother supporting it in Qt though since there is no such a concept in there it seems so that it will require at least one new message box etc.

comment:13 Changed 4 years ago by rudi

will be checking all that right now but a quick question: do you want to say that DosQueryFSAttach() causes more noise on the floppy drive with no disk in than DosOpen??("A:")?

No, it's the same.

Do you also want to say that DosQueryFSAttach() causes the error popup to show up while DosOpen?("A:") doesn't?

Yes. This is due to OPEN_FLAGS_FAIL_ON_ERROR.

to make e.g. copying from one floppy to another possible

That is, where it comes from. I( don't think we should really care about that. Just make the "B:" drive look like "A:" would be sufficient.

BTW, I made a mistake in QFSFileEnginePrivate::isSymlink():

We should use:

     !isDriveRootPath(filePath)

instead of:

     !isDriveRootPath(nativeFilePath.constData())

I still have a performance issue with NetDrive?/SMB. Especially when the whole network neighbourhood is mapped to a single drive letter. Not sure, how this can be solved. Netdrive (or the SMB plugin) doesn't seem to be 100% stable as well.

comment:14 Changed 4 years ago by dmik

So, there are two differences between DosQueryFSAttach() and DosOpen?():

  1. DosOpen?() can be instructed to suppress the error popup regardless of the AUTOFAIL or DosError?() setting.
  2. DosOpen?() always fails on network drives.
  1. is, still, irrelevant since I'm going to implement #131 next which will effectively disable popups for the whole Qt process using DosError?(), including both DosOpen?(), DosQueryFSAttach() and all other relevant APIs.
  1. is actually a disadvantage of DosOpen?(). So DosQueryFSAttach() is preferable here since it lets us know that the drive can be safely stat()'ed, including the network ones.

Next, in r612, I dropped the whole not_ready thing from the implementation. Any error from DosQueryFSFileAttach() will now cause doStat() to return false which will in turn make QFileInfo::exists() return false for all such drives, while both QFileInfo::isDir() and QFileInfo::isRoot() will always return true for all drives. The latter fixes the original problem with dirview/QDirModel you reported in this defect BTW.

Another effect of these changes is that CD drives with no disks don't show up in QFileDialog any more (because QFileInfo::exists() is false for them now too), but in r613 I added support for watching the drive list to the polling file system watcher so that the drive letters for CD and floppy will instantly appear in My Computer as soon as the media is inserted. This includes USB sticks too. This improvement is experimental though since it causes all drives to be accessed each 3 seconds to see if there are any changes and I'm not sure how annoying/slow/noisy it will be on a real system. Under VBox, it's quite acceptable. If it's too bad (and also if the native file watcher is not capable of watch for drive changes) I will have to remove this feature.

Your patch to isSymLink() is indeed a nice one and I applied it in r612, thanks for the hint. The patch to the iterator (slightly changed, according to the intention to implement #131) is also applied in r614.

Again, thank you for your contribution! Please check if the original problem is solved and if the current trunk is as fast as you could get with your original patches and generally satisfies you (with AUTOFAIL=YES so far of course).

comment:15 Changed 4 years ago by dmik

No AUTOFAIL=YES should be necessary any more since I did #131.

comment:16 Changed 4 years ago by rudi

Hi Dmik,

I finally managed to tested the latest changes and for me it's O.K. Guess this ticket can be closed.

comment:17 Changed 4 years ago by dmik

Hey Rudi, are you sure that it's fine now? Re-asking because I wonder that DosQeryFSAttach() on each drive every 3 seconds doesn't cause any noise. If it's really so then I will do the same for the new XWP-based watcher implementation. So could you do the following:

  1. Remove media from the floppy and CD-ROM drives.
  2. Start any app that has a file open dialog, for example, demos/textedit.
  3. Open the file open dialog, select "Computer". There should be no floppy and CD-ROM drives in the list. Wait for some time.
  4. Insert the diskette in the floppy drive. "A:" should appear in the list within some seconds. Wait for some time.
  5. Remove the diskette. "A:" should disappear.
  6. Repeat the same for the CD-ROM drive
  7. Report the observations.

comment:18 Changed 4 years ago by rudi

Dmik,

sorry I was not listening carefully enough to the drive. The whole computer is located in a closet... So yes, there is a cyclic noise (head movement) from the floppy as soon as "Computer" is selected. Also the drive light stays on all the time. The floppy drive icon did appear and disappear as you described. The same for the CD.

However, after a few floppy insert/remove cycles, the floppy icon did not show up anymore. I had a situation, where it wouldn't come up again after closing/restarting the application, but after a few retries it worked again. I also had a situation, where the floppy icon did not disappear when removed. In this case the disk was inserted before when the file dialog was launched.

So I'd say I was too fast with my positive feedback...

comment:19 Changed 4 years ago by dmik

Okay, I feared that. Thank you for testing. I rolled back r613.

Instead, I had to apply a fix to QFileSystemModel in r627 which prevents drives with no media from being removed from the QFileDialog listing under the "Computer" node.

Note that this actually applies to the Windows platform as well (and can be treated as a vendor bug then) but they solved the problem differently on Windows: they forcefully return "true" in response QFileInfo("A:/").exists() (which makes A: appear in the list) but QFileInfo("A:").exists() will return "false". This looks completely weird to me so I had to find a better solution.

Note that I also enabled the native XWP-based file watcher in r628.

I think we may close this defect. Rudi, please check again and feel free to create a new defect if you find any problem.

comment:20 Changed 4 years ago by dmik

  • Resolution set to fixed
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.