Re: [hatari-devel] Hatari GEMDOS drive volume name

[Uwe Seimet <Uwe.Seimet@xxxxxxxxx>]

Hi Eero,

Thank you. With this patch my code works as expected.

Best regards

Uwe

> Hi Uwe,
> 
> On 8.9.2024 11.45, Uwe Seimet wrote:
> > if(!Fsfirst("C:\*.*", FA_VOLUME)) {
> >    DTA *dta = Fgetdta();
> > 
> >    if(dta->d_attrib == 0x08) {
> >      // Unexpected: d_attrib is $00 here, shouldn't it be 0x08?
> >      // d_fname is "EMULATED.002"
> >    }
> > }
> 
> Thanks for reporting!
> 
> Fix for that is one-liner, but the attached patch should fix also few 
> other GEMDOS HD attribute handling corner-cases I noticed while 
> investigating this.
> 
> => It's good if you could test that it does not regress anything else 
> before I commit it.
> 
> (Hatari GEMDOS HD unit test are very basic, as is my setup & manual 
> testing, so you're likely to catch things I do not.)
> 
> 
> Thomas / Nicolas, does the patch look OK?
> 
> 
> 	- Eero

> From 75a33df5b164d965540058e35eb6fcc7f3496357 Mon Sep 17 00:00:00 2001
> From: Eero Tamminen <oak@xxxxxxxxxxxxxx>
> Date: Sun, 8 Sep 2024 16:17:01 +0300
> Subject: [PATCH] GEMDOS HD: fixes for attribute handling corner-cases
> 
> Instead of using in Fsnext() a (global) attribute value set by
> previous Fsfirst() call, store it to internal DTA entry.  This fixes
> case where program first Fgetdata()s multiple DTAs with _different_
> attributes from Fsfirst() calls, and then does Fsnext() calls while
> switching between those earlier acquired DTAs with Fsetdta().
> 
> Support volume labels also in case when archive and/or read-only
> attributes are specified, with the new IS_VOLUME_LABEL() macro.
> 
> Use correct attribute value also for volume labels returned from Fsfirst().
> 
> Improve related code comments & warning messages and update docs.
> ---
>  doc/manual.html               |  2 ++
>  doc/release-notes.txt         |  3 +++
>  src/gemdos.c                  | 47 +++++++++++++++++++++--------------
>  src/includes/gemdos_defines.h |  3 ++-
>  4 files changed, 35 insertions(+), 20 deletions(-)
> 
> diff --git a/doc/manual.html b/doc/manual.html
> index 2e26c4bd..4c137ab9 100644
> --- a/doc/manual.html
> +++ b/doc/manual.html
> @@ -2707,6 +2707,8 @@ have no files.</li>
>  handles used with GEMDOS <em>file</em> functions, such as Fwrite() to standard
>  output that is redirected to a file.  File redirection is NOT supported
>  e.g. for GEMDOS Ccon* console functions.</li>
> +<li>Only FsFirst() / FsNext() calls support drive volume labels, other
> +GEMDOS calls return errors when volume label attribute is specified
>  <li>Host and emulation have their own clocks, which will drift apart
>  after emulation is started, especially when emulation is
>  fast-forwarded or paused.  Because of this, anything inside emulation
> diff --git a/doc/release-notes.txt b/doc/release-notes.txt
> index e072fafb..17fc1758 100644
> --- a/doc/release-notes.txt
> +++ b/doc/release-notes.txt
> @@ -46,6 +46,9 @@ Emulator improvements:
>    - Fix: segfault with negative joystick indexes
>  - GEMDOS HD:
>    - Fix: Fattrib() call on a directory
> +  - Fix: Return attrib for volume labels in FsFirst()
> +  - Fix: Fsnext() calls with DTAs having different attribs,
> +    where DTA is not from preceeding Fsfirst() but earlier one
>    - Fix: Handle additional '*' chars at end of file mask
>  - SCC:
>    - Fix: invalid channel B file close on uninit
> diff --git a/src/gemdos.c b/src/gemdos.c
> index 8bbf1dc1..7a840fd7 100644
> --- a/src/gemdos.c
> +++ b/src/gemdos.c
> @@ -141,6 +141,7 @@ typedef struct
>  	char szActualName[MAX_GEMDOS_PATH];        /* used by F_DATIME (0x57) */
>  } FILE_HANDLE;
>  
> +/* stored FsFirst() information */
>  typedef struct
>  {
>  	bool bUsed;
> @@ -148,16 +149,20 @@ typedef struct
>  	int  nentries;                      /* number of entries in fs directory */
>  	int  centry;                        /* current entry # */
>  	struct dirent **found;              /* legal files */
> -	char path[MAX_GEMDOS_PATH];                /* sfirst path */
> +	char path[MAX_GEMDOS_PATH];
> +	char dta_attrib;
>  } INTERNAL_DTA;
>  
> +/* DTA population ignores archive & read-only attributes */
> +#define IGNORED_FILE_ATTRIBS (GEMDOS_FILE_ATTRIB_WRITECLOSE|GEMDOS_FILE_ATTRIB_READONLY)
> +#define IS_VOLUME_LABEL(x) (((x) & ~(IGNORED_FILE_ATTRIBS)) == GEMDOS_FILE_ATTRIB_VOLUME_LABEL)
> +
>  static FILE_HANDLE  FileHandles[MAX_FILE_HANDLES];
>  static INTERNAL_DTA *InternalDTAs;
>  static int DTACount;        /* Current DTA cache size */
>  static uint16_t DTAIndex;     /* Circular index into above */
>  static uint16_t CurrentDrive; /* Current drive (0=A,1=B,2=C etc...) */
>  static uint32_t act_pd;       /* Used to get a pointer to the current basepage */
> -static uint16_t nAttrSFirst;  /* File attribute for SFirst/Snext */
>  static uint32_t CallingPC;    /* Program counter from caller */
>  
>  static uint32_t nSavedPexecParams;
> @@ -287,7 +292,7 @@ static uint8_t GemDOS_ConvertAttribute(mode_t mode, const char *path)
>  
>  	/* TODO, Other attributes:
>  	 * - GEMDOS_FILE_ATTRIB_HIDDEN (file not visible on desktop/fsel)
> -	 * - GEMDOS_FILE_ATTRIB_ARCHIVE (file written after being backed up)
> +	 * - GEMDOS_FILE_ATTRIB_WRITECLOSE (arcive bit, file written after being backed up)
>  	 * ?
>  	 */
>  	return Attrib;
> @@ -299,7 +304,7 @@ static uint8_t GemDOS_ConvertAttribute(mode_t mode, const char *path)
>   * Populate the DTA buffer with file info.
>   * @return   DTA_OK if entry is ok, DTA_SKIP if it should be skipped, DTA_ERR on errors
>   */
> -static dta_ret_t PopulateDTA(char *path, struct dirent *file, DTA *pDTA, uint32_t DTA_Gemdos)
> +static dta_ret_t PopulateDTA(INTERNAL_DTA *iDTA, struct dirent *file, DTA *pDTA, uint32_t DTA_Gemdos)
>  {
>  	/* TODO: host file path can be longer than MAX_GEMDOS_PATH */
>  	char tempstr[MAX_GEMDOS_PATH];
> @@ -307,8 +312,8 @@ static dta_ret_t PopulateDTA(char *path, struct dirent *file, DTA *pDTA, uint32_
>  	DATETIME DateTime;
>  	int nFileAttr, nAttrMask;
>  
> -	if (snprintf(tempstr, sizeof(tempstr), "%s%c%s",
> -	             path, PATHSEP, file->d_name) >= (int)sizeof(tempstr))
> +	if (snprintf(tempstr, sizeof(tempstr), "%s%c%s", iDTA->path,
> +	             PATHSEP, file->d_name) >= (int)sizeof(tempstr))
>  	{
>  		Log_Printf(LOG_ERROR, "PopulateDTA: path is too long.\n");
>  		return DTA_ERR;
> @@ -327,7 +332,7 @@ static dta_ret_t PopulateDTA(char *path, struct dirent *file, DTA *pDTA, uint32_
>  
>  	/* Check file attributes (check is done according to the Profibuch) */
>  	nFileAttr = GemDOS_ConvertAttribute(filestat.st_mode, tempstr);
> -	nAttrMask = nAttrSFirst|GEMDOS_FILE_ATTRIB_WRITECLOSE|GEMDOS_FILE_ATTRIB_READONLY;
> +	nAttrMask = iDTA->dta_attrib | IGNORED_FILE_ATTRIBS;
>  	if (nFileAttr != 0 && !(nAttrMask & nFileAttr))
>  		return DTA_SKIP;
>  
> @@ -1978,10 +1983,9 @@ static bool GemDOS_Create(uint32_t Params)
>  		return redirect_to_TOS();
>  	}
>  
> -	if (Mode == GEMDOS_FILE_ATTRIB_VOLUME_LABEL)
> +	if (IS_VOLUME_LABEL(Mode))
>  	{
> -		Log_Printf(LOG_WARN, "Warning: Hatari doesn't support GEMDOS volume"
> -			   " label setting\n(for '%s')\n", pszFileName);
> +		Log_Printf(LOG_WARN, "volume label creation not supported on GEMDOS HD\n");
>  		Regs[REG_D0] = GEMDOS_EFILNF;         /* File not found */
>  		return true;
>  	}
> @@ -2606,9 +2610,9 @@ static bool GemDOS_Fattrib(uint32_t Params)
>  	GemDOS_CreateHardDriveFileName(nDrive, psFileName,
>  	                              sActualFileName, sizeof(sActualFileName));
>  
> -	if (nAttrib == GEMDOS_FILE_ATTRIB_VOLUME_LABEL)
> +	if (IS_VOLUME_LABEL(nAttrib))
>  	{
> -		Log_Printf(LOG_WARN, "Hatari doesn't support GEMDOS volume label setting\n(for '%s')\n", sActualFileName);
> +		Log_Printf(LOG_WARN, "volume label Fattrib() not supported on GEMDOS HD\n");
>  		Regs[REG_D0] = GEMDOS_EFILNF;         /* File not found */
>  		return true;
>  	}
> @@ -2914,7 +2918,7 @@ static bool GemDOS_SNext(void)
>  		return false;
>  	}
>  
> -	if (nAttrSFirst == GEMDOS_FILE_ATTRIB_VOLUME_LABEL)
> +	if (IS_VOLUME_LABEL(pDTA->dta_attrib))
>  	{
>  		/* Volume label was given already in Sfirst() */
>  		Regs[REG_D0] = GEMDOS_ENMFIL;
> @@ -2946,7 +2950,7 @@ static bool GemDOS_SNext(void)
>  			return true;
>  		}
>  
> -		ret = PopulateDTA(InternalDTAs[Index].path,
> +		ret = PopulateDTA(&InternalDTAs[Index],
>  				  temp[InternalDTAs[Index].centry++],
>  				  pDTA, DTA_Gemdos);
>  	} while (ret == DTA_SKIP);
> @@ -2981,17 +2985,18 @@ static bool GemDOS_SFirst(uint32_t Params)
>  	DTA *pDTA;
>  	uint32_t DTA_Gemdos;
>  	uint16_t useidx;
> +	uint16_t attrib;
>  
> -	nAttrSFirst = STMemory_ReadWord(Params+SIZE_LONG);
> +	attrib = STMemory_ReadWord(Params+SIZE_LONG);
>  	pszFileName = STMemory_GetStringPointer(STMemory_ReadLong(Params));
>  	if (!pszFileName)
>  	{
>  		LOG_TRACE(TRACE_OS_GEMDOS, "GEMDOS 0x4E bad Fsfirst(0x%X, 0x%x) at PC 0x%X\n",
> -		          STMemory_ReadLong(Params), nAttrSFirst, CallingPC);
> +		          STMemory_ReadLong(Params), attrib, CallingPC);
>  		return false;
>  	}
>  
> -	LOG_TRACE(TRACE_OS_GEMDOS, "GEMDOS 0x4E Fsfirst(\"%s\", 0x%x) at PC 0x%X\n", pszFileName, nAttrSFirst,
> +	LOG_TRACE(TRACE_OS_GEMDOS, "GEMDOS 0x4E Fsfirst(\"%s\", 0x%x) at PC 0x%X\n", pszFileName, attrib,
>  		  CallingPC);
>  
>  	Drive = GemDOS_FileName2HardDriveID(pszFileName);
> @@ -3045,13 +3050,17 @@ static bool GemDOS_SFirst(uint32_t Params)
>  	}
>  	InternalDTAs[useidx].bUsed = true;
>  	InternalDTAs[useidx].addr = DTA_Gemdos;
> +	InternalDTAs[useidx].dta_attrib = attrib;
>  
> -	/* Were we looking for the volume label? */
> -	if (nAttrSFirst == GEMDOS_FILE_ATTRIB_VOLUME_LABEL)
> +	/* volume labels are supported only when no other
> +	 * other file types are specified in same query
> +	 */
> +	if (IS_VOLUME_LABEL(attrib))
>  	{
>  		/* Volume name */
>  		strcpy(pDTA->dta_name,"EMULATED.001");
>  		pDTA->dta_name[11] = '0' + Drive;
> +		pDTA->dta_attrib = GEMDOS_FILE_ATTRIB_VOLUME_LABEL;
>  		Regs[REG_D0] = GEMDOS_EOK;          /* Got volume */
>  		return true;
>  	}
> diff --git a/src/includes/gemdos_defines.h b/src/includes/gemdos_defines.h
> index 53352925..80fcb126 100644
> --- a/src/includes/gemdos_defines.h
> +++ b/src/includes/gemdos_defines.h
> @@ -53,7 +53,8 @@
>  #define GEMDOS_FILE_ATTRIB_SYSTEM_FILE   0x04
>  #define GEMDOS_FILE_ATTRIB_VOLUME_LABEL  0x08
>  #define GEMDOS_FILE_ATTRIB_SUBDIRECTORY  0x10
> -/* file was written and closed correctly (used automatically on gemdos >=0.15) */
> +/* archive bit, file was written and closed correctly
> + * (used automatically on gemdos >=0.15) */
>  #define GEMDOS_FILE_ATTRIB_WRITECLOSE    0x20
>  
>  #endif /* HATARI_GEMDOS_DEFINES_H */
> -- 
> 2.39.2
>