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]

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 https://github.com/dosemu2/fdpp/pull/202. It's probably best if we continue discussion in that PR and return here later.

[andrewbird]

Closing this here as it's outdated, the patches having been superseded by those in https://github.com/dosemu2/fdpp/pull/202