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

[Uwe Seimet <Uwe.Seimet@xxxxxxxxx>]

Hi,

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.
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.

Best regards

Uwe

> 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
>