Fixes for label #17 bugs

[andrewbird]

I've written some tests for label operations in Dosemu2's test suite, which can now pass, but I wonder if you can review the patch set here as I'm a little out of my comfort zone in FAT manipulation? In particular patch 2 Add special case for label creation uses a dos_findfirst() which may be a layering violation?

Dosemu2 tests are in a topic branch here https://github.com/andrewbird/dosemu2/tree/ci-01, but not submitted for PR until FDPP is updated with a similar patch to this.

[PerditionC]

I will review this this weekend. I have a few other changes to the kernel to try integrate this weekend as well.

[andrewbird]

Hello @PerditionC , @xakon, I pushed a new version that should address the concern you raised.

Thanks.

[andrewbird]

Hello @PerditionC, sorry to trouble you but I wondered whether you managed to consider if my use of dos_findfirst() was valid?

Thanks!

[PerditionC]

Not yet, I got sidetracked playing with GPT support.

[stsp]

Hello @PerditionC, sorry to trouble you but I wondered whether you managed to consider if my use of
dos_findfirst() was valid?

Please look into find_fname().

[andrewbird]

Please look into find_fname()

Thanks, I couldn't use it as is as I don't know the name of the file I'm looking for and it doesn't take wildcards, but it made a good basis for a new function that finds by attribute.

[stsp]

If using find_fname(), we'd be only interested
in a directory. Maybe the only thing you need to
do is to replace
if (fcbmatch(fnp->f_dir.dir_name, fnp->f_dmp->dm_name_pat))
with
if (!fnp->f_dmp->dm_name_pat[0] || fcbmatch(fnp->f_dir.dir_name, fnp->f_dmp->dm_name_pat))
?

[stsp]

It doesn't look like dir_read() looks
into dm_name_pat so that should work.
Alternatively there is fcmp_wild() but for
such a small task IMO just an observation
that dir_read() doesn't use dm_name_pat
is enough.

[andrewbird]

That almost works but find_fname() uses split_path() to open the directory and that fails if the filename is empty

  if (path[strlen(path) - 1] == '\\')                                           
    return (f_node_ptr) 0;                                                      

[stsp]

OK but I think its wrong to pass fnp
to your new find_fattr() because that
fnp was obtained with sft_to_fnode(fd).
So is obviously not for that search.
If you want a new func, then I think
you need to make it so that it creates
fnp internally from given path.

[stsp]

In fact it seems fnp is mostly used
as an output parameter here. So I
guess you just shouldn't skip find_fname()
with your goto doit;. find_fname()
should fill fnp after your manipulations.

[stsp]

What if you still have a normal
file with that name?
I think that block should be removed.

[andrewbird]

What if you still have a normal
file with that name?
I think that block should be removed.

The point of this patch is that a file or directory can coexist with a label of the same name.

[stsp]

Ah perhaps you explicitly don't care
about any other file (to allow duplicates)
and re-fill fnp with alloc_find_free().
I see.

[stsp]

OK in this case the only mod I can
think of is that you should pass dir to
find_fattr(), not path. As currently
you pass the path of a new vol file,
which makes no sense.
So I suggest to just move split_path()
to the caller, and pass only attr and
fnp to find_fattr() (or just inline it
entirely, as then it isn't worth being
a function)

[andrewbird]

Sorry for the delay, I've been away from my computer. Thanks for your suggestions, I'll study them a little later.

[stsp]

I pushed a few patches to fdpp from
which is should be clear how I see it
implemented (in fdpp, freedos is free
to choose any other impl).

[stsp]

So in fdpp you can now do:

fnp = split_path(path, fnp);
fnp->f_dmp->dm_name_pat[0] = '\0'; // drop filename, only dir remains
rc = find_in_dir(D_VOLID, D_VOLID, fnp);

[stsp]

Also I wonder if its intentional to
search for vol in cur dir rather than
in a root dir. If so, have you tested
what happens when vol is attempted
not in a root dir?

Or if you want to look only in a root
dir, then you can do:

char root[4];
memcpy(root, path, 3);  // "C:\"
root[3] = '\0';
fnp = dir_open(root, FALSE, fnp);
rc = find_in_dir(D_VOLID, D_VOLID, fnp);

Though I am not sure if dir_open()
resets dm_name_pat properly. It calls
dir_init_fnode() but I don't see it clearing
dm_name_pat. This have to be checked/added.

[stsp]

I checked that dm_name_pat was actually
never initialized, and added a patch to change
that. Waiting for your test-suit to run, as its
not impossible for some code to rely on a
prev behavior...

[andrewbird]

Test suit running now on branch ci-01, but expect the recent tests for label and command.com/copy to fail as those were added for FreeDOS and I didn't get around to proposing some patches for FDPP yet. https://github.com/andrewbird/dosemu2/actions/runs/3446152758/jobs/5750707847

[andrewbird]

So yes there are some unexplained failures. These might be ones to look at first
test/test_dos.py PPDOSGITTestCase.test_fat12_img_d_writable
test/test_dos.py PPDOSGITTestCase.test_fat_ds2_find_simple
test/test_dos.py PPDOSGITTestCase.test_fat_ds2_rename_file

[stsp]

Thanks, changed so dm_name_pat is
only cleared on dir_open().

[andrewbird]

Cool, thanks. Rerunning now https://github.com/andrewbird/dosemu2/actions/runs/3446152758

[andrewbird]

Great, that looks much as I expect.

[stsp]

But insufficient this time...
There was another dir_init_fnode() call
in dir_open() where dm_name_pat was
not being cleared.
Covered now.
These are safe changes, should not regress.
Only clearing dm_name_pat directly in
dir_init_fnode() was an ask for troubles.

[andrewbird]

Thanks for your changes to help. I pushed my first modified patch over at dosemu2/fdpp#202. It's probably best if we continue discussion in that PR and return here later.