Re: [hatari-devel] Duplicate drive image file issue

[ Thread Index | Date Index | More lists.tuxfamily.org/hatari-devel Archives ]

To: hatari-devel@xxxxxxxxxxxxxxxxxxx
Subject: Re: [hatari-devel] Duplicate drive image file issue
From: Eero Tamminen <oak@xxxxxxxxxxxxxx>
Date: Tue, 8 Dec 2020 01:57:49 +0200

Hi,

On 12/6/20 10:23 PM, Uwe Seimet wrote:

Yes, your patches appear to work. I'd like to suggest to be a bit more
precise in the error message. The message should not just say that locking
failed, but it should say what this means in practice, i.e. the respective
drive will not be available.


First I thought that because those messages are
similar to the other Hatari error dialogs i.e. as
the indicated functionality failed, user shouldn't
expect it to work.

Then I realized that Hatari dialogs don't show
the log level i.e. is the dialog message just
info, warning or an error.

Attached patch would do that, but I'm not sure
is that enough...

Do you have some exact suggestion what the error
dialog text should be in the lock failure case?


And after that I realized read-only warning:
"ACSI HD file is read-only, no writes will go through <filename>"

IS not shown in dialog, because by default
only fatal & error levels go to dialogs, rest
are shown only in console by Hatari nowadays.

Any comments on that?


	- Eero

Hi Uwe,
On 11/24/20 6:45 PM, Uwe Seimet wrote:

Before applying the patch I wanted to check the current behavior, and it
looks as if something else is (also) wrong: In TT mode when I use the same
image for IDE HD 0 and ACSI HD 0 there is no warning. But if I use the same
image for ACSI HD 0 and SCSI ID 0 there is a warning about Hatari not being
able to lock the file.


My patchset should have fixed that too.

Earlier IDE code printed that error only to
console, my patch changes it to be an alert
dialog, same as with ACSI/SCSI.

This is inconsistent, because whenever I try to use the same image more than
once there should be a warning, irrespective of the hard disk interface.

Maybe you can have a look at this first, your patches might change based on
this observation.


Attached is slightly updated patchset.

(I've fixed few typos in the messages and
improved commit descriptions.)

Could you verify whether they fix both of
the issues you noticed?

(I'm hoping to include them to Hatari v2.3.1
bugfix release.)


	- Eero

Hi Uwe,

I've tested the attached patch(es) to fix
the issue you found.

Could you verify?

(I think I'll push it only after release though.)


	- Eero

On 10/30/20 9:55 AM, Uwe Seimet wrote:

Any news on this?

Best regards

Uwe


No, I'm afraid nothing has changed, the issue is still there.

Best regards

Uwe

>>

On 11/3/19 9:33 PM, Uwe Seimet wrote:> Hi Roger,

On 3 Nov 2019 at 8:17, Uwe Seimet wrote:


For IDE HD 0 I have configured a drive image to be used. When I select
the same image file for ACSI HD 0 and reset Hatari, Hatari reports
"Locking HD file for writing failed" and TOS only sees IDE HD 0, but not
HD ACSI 0. So far, so good.

>>>

Now I eject the file for IDE HD 0, so that the image file is configured
for ACSI HD 0 only. After rebooting there is no HD available for TOS
at all, even though ACSI HD 0 is the only drive configured with an image
file. Looks as if Hatari has not noticed that there is no image file
conflict anymore but still ignores the (only) assigned image.

Are you sure this is not a byte-swapping problem?  My ACSI image and IDE image
are byte-swapped with respect to each other.

    >
    > Yes, I'm sure, because if I avoid the locking issue by applying the
    > change (i.e. using the image file for ACSI instead of IDE) in a single
    > step in the UI, everything is fine.

I think I see the problem.  In:
	change.c::Change_CopyChangedParamsToConfiguration()

UnInit for IDE & ACSI/SCSI drives is called only if the status of drives
is changed *and* at least on of the changed drives is still used,
i.e. they're unlocked only if IDE or ACSI needs to re-initialized.

Attached quick patch does unInit separately for them.  Does that fix
the problem?

>From 392c4bc4d1de15bd0d0ba9c29ff41704aa703100 Mon Sep 17 00:00:00 2001
From: Eero Tamminen <oak@xxxxxxxxxxxxxx>
Date: Sun, 22 Nov 2020 00:04:20 +0200
Subject: [PATCH 1/3] Tell user which HD type (interface) is in question on HD
   errors

E.g. if user selects same HD file for multiple interfaces, it's
impossible for user to know on which interface the file got rejected
(due to file lock failing) unless Hatari tells it.
---
   src/hdc.c          | 33 ++++++++++++++++++---------------
   src/ide.c          |  4 ++--
   src/includes/hdc.h |  4 ++--
   src/ncr5380.c      |  2 +-
   4 files changed, 23 insertions(+), 20 deletions(-)

diff --git a/src/hdc.c b/src/hdc.c
index f0c728ce..509e6a36 100644
--- a/src/hdc.c
+++ b/src/hdc.c
@@ -774,7 +774,7 @@ int HDC_PartitionCount(FILE *fp, const Uint64 tracelevel, int *pIsByteSwapped)
    * Check file size for sane values (non-zero, multiple of 512),
    * and return the size
    */
-off_t HDC_CheckAndGetSize(const char *filename, unsigned long blockSize)
+off_t HDC_CheckAndGetSize(const char *hdtype, const char *filename, unsigned long blockSize)
   {
   	off_t filesize;
   	char shortname[48];
@@ -784,8 +784,8 @@ off_t HDC_CheckAndGetSize(const char *filename, unsigned long blockSize)
   	filesize = File_Length(filename);
   	if (filesize < 0)
   	{
-		Log_AlertDlg(LOG_ERROR, "Unable to get size of HD image file\n'%s'!",
-		             shortname);
+		Log_AlertDlg(LOG_ERROR, "Unable to get size of %s HD image file\n'%s'!",
+		             hdtype, shortname);
   		if (sizeof(off_t) < 8)
   		{
   			Log_Printf(LOG_ERROR, "Note: This version of Hatari has been built"
@@ -796,16 +796,16 @@ off_t HDC_CheckAndGetSize(const char *filename, unsigned long blockSize)
   	}
   	if (filesize == 0)
   	{
-		Log_AlertDlg(LOG_ERROR, "Can not use HD image file\n'%s'\n"
+		Log_AlertDlg(LOG_ERROR, "Can not use %s HD image file\n'%s'\n"
   		                        "since the file is empty.",
-		             shortname);
+		             hdtype, shortname);
   		return -EINVAL;
   	}
   	if ((filesize & (blockSize - 1)) != 0)
   	{
-		Log_AlertDlg(LOG_ERROR, "Can not use the hard disk image file\n"
+		Log_AlertDlg(LOG_ERROR, "Can not use the %s HD image file\n"
   		                        "'%s'\nsince its size is not a multiple"
-		                        " of %ld.", shortname, blockSize);
+		                        " of %ld.", hdtype, shortname, blockSize);
   		return -EINVAL;
   	}

@@ -815,16 +815,16 @@ off_t HDC_CheckAndGetSize(const char *filename, unsigned long blockSize)

   /**
    * Open a disk image file
    */
-int HDC_InitDevice(SCSI_DEV *dev, char *filename, unsigned long blockSize)
+int HDC_InitDevice(const char *hdtype, SCSI_DEV *dev, char *filename, unsigned long blockSize)
   {
   	off_t filesize;
   	FILE *fp;

dev->enabled = false;

-	Log_Printf(LOG_INFO, "Mounting hard drive image '%s'\n", filename);
+	Log_Printf(LOG_INFO, "Mounting %s HD image '%s'\n", hdtype, filename);

/* Check size for sanity */

-	filesize = HDC_CheckAndGetSize(filename, blockSize);
+	filesize = HDC_CheckAndGetSize(hdtype, filename, blockSize);
   	if (filesize < 0)
   		return filesize;

@@ -832,14 +832,17 @@ int HDC_InitDevice(SCSI_DEV *dev, char *filename, unsigned long blockSize)

   	{
   		if (!(fp = fopen(filename, "rb")))
   		{
-			Log_AlertDlg(LOG_ERROR, "Cannot open HD file for reading\n'%s'!\n", filename);
+			Log_AlertDlg(LOG_ERROR, "Cannot open %s HD file for reading\n'%s'!\n",
+				     hdtype, filename);
   			return -ENOENT;
   		}
-		Log_AlertDlg(LOG_WARN, "HD file is read-only, no writes will go through\n'%s'.\n", filename);
+		Log_AlertDlg(LOG_WARN, "%s HD file is read-only, no writes will go through\n'%s'.\n",
+			     hdtype, filename);
   	}
   	else if (!File_Lock(fp))
   	{
-		Log_AlertDlg(LOG_ERROR, "Locking HD file for writing failed\n'%s'!\n", filename);
+		Log_AlertDlg(LOG_ERROR, "Locking %s HD file for writing failed\n'%s'!\n",
+			     hdtype, filename);
   		fclose(fp);
   		return -ENOLCK;
   	}
@@ -875,7 +878,7 @@ bool HDC_Init(void)
   	{
   		if (!ConfigureParams.Acsi[i].bUseDevice)
   			continue;
-		if (HDC_InitDevice(&AcsiBus.devs[i], ConfigureParams.Acsi[i].sDeviceFile, ConfigureParams.Acsi[i].nBlockSize) == 0)
+		if (HDC_InitDevice("ACSI", &AcsiBus.devs[i], ConfigureParams.Acsi[i].sDeviceFile, ConfigureParams.Acsi[i].nBlockSize) == 0)
   			nAcsiPartitions += HDC_PartitionCount(AcsiBus.devs[i].image_file, TRACE_SCSI_CMD, NULL);
   	}
   	/* set total number of partitions */
@@ -1027,7 +1030,7 @@ static void Acsi_DmaTransfer(void)
   			int wlen = fwrite(&STRam[nDmaAddr], 1, AcsiBus.data_len, AcsiBus.dmawrite_to_fh);
   			if (wlen != AcsiBus.data_len)
   			{
-				Log_Printf(LOG_ERROR, "Could not write all bytes to hard disk image.\n");
+				Log_Printf(LOG_ERROR, "Could not write all bytes to ACSI HD image.\n");
   				AcsiBus.status = HD_STATUS_ERROR;
   			}
   #endif
diff --git a/src/ide.c b/src/ide.c
index b1955d21..55a75710 100644
--- a/src/ide.c
+++ b/src/ide.c
@@ -560,7 +560,7 @@ static int bdrv_open(BlockDriverState *bs, const char *filename, unsigned long b
   	Log_Printf(LOG_INFO, "Mounting IDE hard drive image %s\n", filename);

bs->read_only = 0;

-	bs->file_size = HDC_CheckAndGetSize(filename, blockSize);
+	bs->file_size = HDC_CheckAndGetSize("IDE", filename, blockSize);
   	if (bs->file_size <= 0)
   		return -1;
   	if (bs->file_size < 2 * 16 * 63 * bs->sector_size)
@@ -581,7 +581,7 @@ static int bdrv_open(BlockDriverState *bs, const char *filename, unsigned long b
   	}
   	else if (!File_Lock(bs->fhndl))
   	{
-		Log_Printf(LOG_ERROR, "Cannot lock HD file for writing!\n");
+		Log_Printf(LOG_ERROR, "Cannot lock IDE HD file for writing!\n");
   		fclose(bs->fhndl);
   		bs->fhndl = NULL;
   	}
diff --git a/src/includes/hdc.h b/src/includes/hdc.h
index df4445dc..201aa380 100644
--- a/src/includes/hdc.h
+++ b/src/includes/hdc.h
@@ -94,12 +94,12 @@ extern bool bAcsiEmuOn;
    */
   extern bool HDC_Init(void);
   extern void HDC_UnInit(void);
-extern int HDC_InitDevice(SCSI_DEV *dev, char *filename, unsigned long blockSize);
+extern int HDC_InitDevice(const char *hdtype, SCSI_DEV *dev, char *filename, unsigned long blockSize);
   extern void HDC_ResetCommandStatus(void);
   extern short int HDC_ReadCommandByte(int addr);
   extern void HDC_WriteCommandByte(int addr, Uint8 byte);
   extern int HDC_PartitionCount(FILE *fp, const Uint64 tracelevel, int *pIsByteSwapped);
-extern off_t HDC_CheckAndGetSize(const char *filename, unsigned long blockSize);
+extern off_t HDC_CheckAndGetSize(const char *hdtype, const char *filename, unsigned long blockSize);
   extern bool HDC_WriteCommandPacket(SCSI_CTRLR *ctr, Uint8 b);
   extern void HDC_DmaTransfer(void);

diff --git a/src/ncr5380.c b/src/ncr5380.c

index 44e6eaf7..aa8ec992 100644
--- a/src/ncr5380.c
+++ b/src/ncr5380.c
@@ -1036,7 +1036,7 @@ bool Ncr5380_Init(void)
   	{
   		if (!ConfigureParams.Scsi[i].bUseDevice)
   			continue;
-		if (HDC_InitDevice(&ScsiBus.devs[i], ConfigureParams.Scsi[i].sDeviceFile, ConfigureParams.Scsi[i].nBlockSize) == 0)
+		if (HDC_InitDevice("SCSI", &ScsiBus.devs[i], ConfigureParams.Scsi[i].sDeviceFile, ConfigureParams.Scsi[i].nBlockSize) == 0)
   			nScsiPartitions += HDC_PartitionCount(ScsiBus.devs[i].image_file, TRACE_SCSI_CMD, NULL);
   	}
   	nNumDrives += nScsiPartitions;
--
2.20.1

>From 023f8f20c01c81f1205b18dd0502ad28214d63d4 Mon Sep 17 00:00:00 2001
From: Eero Tamminen <oak@xxxxxxxxxxxxxx>
Date: Sun, 22 Nov 2020 01:10:57 +0200
Subject: [PATCH 3/3] Treat IDE HD image errors similarly to ACSI/SCSI errors

---
   src/ide.c | 9 ++++++++-
   1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/src/ide.c b/src/ide.c
index c66ebcc5..44dc770b 100644
--- a/src/ide.c
+++ b/src/ide.c
@@ -576,14 +576,21 @@ static int bdrv_open(BlockDriverState *bs, const char *filename, unsigned long b
   		/* Maybe the file is read-only? */
   		bs->fhndl = fopen(filename, "rb");
   		if (!bs->fhndl)
+		{
   			perror("bdrv_open");
+			Log_AlertDlg(LOG_WARN, "IDE HD file open failed\n'%s'.\n", filename);
+			return -1;
+		}
+		Log_AlertDlg(LOG_WARN, "IDE HD file is read-only, no writes will go through\n'%s'.\n",
+			     filename);
   		bs->read_only = 1;
   	}
   	else if (!File_Lock(bs->fhndl))
   	{
-		Log_Printf(LOG_ERROR, "Cannot lock IDE HD file for writing!\n");
+		Log_AlertDlg(LOG_ERROR, "Locking ID HD file for writing failed\n'%s'!\n", filename);
   		fclose(bs->fhndl);
   		bs->fhndl = NULL;
+		return -1;
   	}

/* call the change callback */

--
2.20.1

>From 033e33310ec78f0390d151a5bbffa1ff1593704a Mon Sep 17 00:00:00 2001
From: Eero Tamminen <oak@xxxxxxxxxxxxxx>
Date: Sun, 22 Nov 2020 00:48:38 +0200
Subject: [PATCH 2/3] Disable failing HD images

Fixes following issue:
* User starts hatari with IDE: hatari --ide-master disk.img
* User selects same image also for another interface in GUI,
    e.g. for ACSI, and proceeds
    -> Hatari ACSI file locking fails, and it's not mounted
* User ejects IDE image from GUI, leaves ACSI image as-is and proceeds

=> Hatari won't (re-)initialize ACSI as its config did not change, and
     as result ACSI drive won't be mounted to emulation, although it's
     still visible in the GUI as being mounted.

After the change, ACSI image name isn't anymore shown in the GUI after
failure, as it's now disabled.  When user browses it again, he can
just OK the old name, one doesn't need to search for it.
---
   src/hdc.c     | 2 ++
   src/ide.c     | 6 +++++-
   src/ncr5380.c | 2 ++
   3 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/src/hdc.c b/src/hdc.c
index 509e6a36..8989add2 100644
--- a/src/hdc.c
+++ b/src/hdc.c
@@ -880,6 +880,8 @@ bool HDC_Init(void)
   			continue;
   		if (HDC_InitDevice("ACSI", &AcsiBus.devs[i], ConfigureParams.Acsi[i].sDeviceFile, ConfigureParams.Acsi[i].nBlockSize) == 0)
   			nAcsiPartitions += HDC_PartitionCount(AcsiBus.devs[i].image_file, TRACE_SCSI_CMD, NULL);
+		else
+			ConfigureParams.Acsi[i].bUseDevice = false;
   	}
   	/* set total number of partitions */
   	nNumDrives += nAcsiPartitions;
diff --git a/src/ide.c b/src/ide.c
index 55a75710..c66ebcc5 100644
--- a/src/ide.c
+++ b/src/ide.c
@@ -2733,7 +2733,11 @@ void Ide_Init(void)
   		if (ConfigureParams.Ide[i].bUseDevice)
   		{
   			int is_byteswap;
-			bdrv_open(hd_table[i], ConfigureParams.Ide[i].sDeviceFile, ConfigureParams.Ide[i].nBlockSize, 0);
+			if (bdrv_open(hd_table[i], ConfigureParams.Ide[i].sDeviceFile, ConfigureParams.Ide[i].nBlockSize, 0) < 0)
+			{
+				ConfigureParams.Ide[i].bUseDevice = false;
+				continue;
+			}
   			nIDEPartitions += HDC_PartitionCount(hd_table[i]->fhndl, TRACE_IDE, &is_byteswap);
   			/* Our IDE implementation is little endian by default,
   			 * so we need to byteswap if the image is not swapped! */
diff --git a/src/ncr5380.c b/src/ncr5380.c
index aa8ec992..4c6b7f39 100644
--- a/src/ncr5380.c
+++ b/src/ncr5380.c
@@ -1038,6 +1038,8 @@ bool Ncr5380_Init(void)
   			continue;
   		if (HDC_InitDevice("SCSI", &ScsiBus.devs[i], ConfigureParams.Scsi[i].sDeviceFile, ConfigureParams.Scsi[i].nBlockSize) == 0)
   			nScsiPartitions += HDC_PartitionCount(ScsiBus.devs[i].image_file, TRACE_SCSI_CMD, NULL);
+		else
+			ConfigureParams.Scsi[i].bUseDevice = false;
   	}
   	nNumDrives += nScsiPartitions;
   	if (nScsiPartitions)
--
2.20.1

>From 5296d0123b4b0822d5563cf28fbd2d64122b9c21 Mon Sep 17 00:00:00 2001
From: Eero Tamminen <oak@xxxxxxxxxxxxxx>
Date: Sun, 22 Nov 2020 00:48:38 +0200
Subject: [PATCH 3/3] Disable failing HD images

Fixes following issue:
* User starts hatari with IDE: hatari --ide-master disk.img
* User selects same image also for another interface in GUI,
   e.g. for ACSI, and proceeds
   -> Hatari ACSI file locking fails, and it's not mounted
* User ejects IDE image from GUI, leaves ACSI image as-is and proceeds

=> Hatari won't (re-)initialize ACSI as its config did not change, and
    as result ACSI drive won't be mounted to emulation, although it's
    still visible in the GUI as being mounted.

After the change, ACSI image name isn't anymore shown in the GUI after
failure, as it's now disabled.  When user browses it again, he can
just OK the old name, one doesn't need to search for it.
---
  src/hdc.c     | 2 ++
  src/ide.c     | 6 +++++-
  src/ncr5380.c | 2 ++
  3 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/src/hdc.c b/src/hdc.c
index edf9421a..e6f3488c 100644
--- a/src/hdc.c
+++ b/src/hdc.c
@@ -883,6 +883,8 @@ bool HDC_Init(void)
  			nAcsiPartitions += HDC_PartitionCount(AcsiBus.devs[i].image_file, TRACE_SCSI_CMD, NULL);
  			bAcsiEmuOn = true;
  		}
+		else
+			ConfigureParams.Acsi[i].bUseDevice = false;
  	}
  	/* set total number of partitions */
  	nNumDrives += nAcsiPartitions;
diff --git a/src/ide.c b/src/ide.c
index 61d4cf69..1563fd89 100644
--- a/src/ide.c
+++ b/src/ide.c
@@ -2740,7 +2740,11 @@ void Ide_Init(void)
  		if (ConfigureParams.Ide[i].bUseDevice)
  		{
  			int is_byteswap;
-			bdrv_open(hd_table[i], ConfigureParams.Ide[i].sDeviceFile, ConfigureParams.Ide[i].nBlockSize, 0);
+			if (bdrv_open(hd_table[i], ConfigureParams.Ide[i].sDeviceFile, ConfigureParams.Ide[i].nBlockSize, 0) < 0)
+			{
+				ConfigureParams.Ide[i].bUseDevice = false;
+				continue;
+			}
  			nIDEPartitions += HDC_PartitionCount(hd_table[i]->fhndl, TRACE_IDE, &is_byteswap);
  			/* Our IDE implementation is little endian by default,
  			 * so we need to byteswap if the image is not swapped! */
diff --git a/src/ncr5380.c b/src/ncr5380.c
index 4d901ff6..f36c3de5 100644
--- a/src/ncr5380.c
+++ b/src/ncr5380.c
@@ -1041,6 +1041,8 @@ bool Ncr5380_Init(void)
  			nScsiPartitions += HDC_PartitionCount(ScsiBus.devs[i].image_file, TRACE_SCSI_CMD, NULL);
  			bScsiEmuOn = true;
  		}
+		else
+			ConfigureParams.Scsi[i].bUseDevice = false;
  	}
  	nNumDrives += nScsiPartitions;
  #endif
--
2.20.1

>From 3846b08206a59471169f799824483ec4d28c7af9 Mon Sep 17 00:00:00 2001
From: Eero Tamminen <oak@xxxxxxxxxxxxxx>
Date: Sun, 22 Nov 2020 01:10:57 +0200
Subject: [PATCH 2/3] Fail IDE on IDE disk open errors and tell user about it

Makes IDE image opening related errors work similarly to how they're
handled with ACSI/SCSI.
---
  src/ide.c | 9 ++++++++-
  1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/src/ide.c b/src/ide.c
index 55a75710..61d4cf69 100644
--- a/src/ide.c
+++ b/src/ide.c
@@ -576,14 +576,21 @@ static int bdrv_open(BlockDriverState *bs, const char *filename, unsigned long b
  		/* Maybe the file is read-only? */
  		bs->fhndl = fopen(filename, "rb");
  		if (!bs->fhndl)
+		{
  			perror("bdrv_open");
+			Log_AlertDlg(LOG_ERROR, "Cannot open IDE HD for reading\n'%s'.\n", filename);
+			return -1;
+		}
+		Log_AlertDlg(LOG_WARN, "IDE HD file is read-only, no writes will go through\n'%s'.\n",
+			     filename);
  		bs->read_only = 1;
  	}
  	else if (!File_Lock(bs->fhndl))
  	{
-		Log_Printf(LOG_ERROR, "Cannot lock IDE HD file for writing!\n");
+		Log_AlertDlg(LOG_ERROR, "Locking IDE HD file for writing failed\n'%s'!\n", filename);
  		fclose(bs->fhndl);
  		bs->fhndl = NULL;
+		return -1;
  	}