Closed t-w closed 1 year ago
kyz
commented
1 year ago I'm OK with this, however what I would recommend is a review of the API documentation in doc.
Specifically for adfFileOpen(), write down what the function does today. I don't even think there's a need to remove "a" mode, just write what it does today... but personally I'd add a note saying that this function is likely to change in the next release?
kyz
commented
1 year ago Another thing I'd suggest is to review the debian packaging. I haven't looked at their standards for years, so I don't know what is modern packaging practise, but from what I can read, it does not let the build system do an install step and take from the install destination, instead it plucks its own choice of files from the build tree?
Also, it chooses the generic driver which does nothing. Even though it is experimental, shouldn't we include the new native linux driver?
kyz
commented
1 year ago My suggested changes to debian could either follow after https://github.com/lclevy/ADFlib/pull/33 or be done as part of it, if you are willing to make some commits to that branch, as I don't have the expertise to change the debian packaging
lclevy
commented
1 year ago 👍
t-w
commented
1 year ago I'm OK with this, however what I would recommend is a review of the API documentation in
doc.Sure. Added to the list.
Specifically for adfFileOpen(), write down what the function does today. I don't even think there's a need to remove "a" mode, just write what it does today...
Well, there was so much about it that it seemed like a major issue - so now too late... ;-) Or you can revert it - this time up to you. Both ways are fine for me.
but personally I'd add a note saying that this function is likely to change in the next release?
Sure, why not. Though, at the same time, it should not suggest that everything else will not change... It might - if any further improvements are going to happen.
t-w
commented
1 year ago Another thing I'd suggest is to review the debian packaging. I haven't looked at their standards for years, so I don't know what is modern packaging practise, but from what I can read, it does not let the build system do an install step and take from the install destination, instead it plucks its own choice of files from the build tree?
For me it is important to have working deb packages (which, potentially, can be useful for any deb-based distro).
I do not know details of Debian policies and do not think that we immediately have to follow them 100% strictly. If needed/requested, we can improve it. If it will go to Debian, we can improve it in coordination with the maintainer.
Normally, lintian reports things to correct. Currently, there is couple of warnings, we can clean-up some of them if it is something worth the time (eg. add man pages for the utilities).
Also, it chooses the generic driver which does nothing. Even though it is experimental, shouldn't we include the new native linux driver?
Experimental code, untested, dealing with physical storage devices? I think it can do more harm than good... If anyone wants to experiment, can rebuild (it is not hard and well documented). But, IMHO, users should not be by default exposed to such a dangerous piece of code.
t-w
commented
1 year ago My suggested changes to debian could either follow after #33 or be done as part of it, if you are willing to make some commits to that branch, as I don't have the expertise to change the debian packaging
Yes, Debian packaging should go along with #33, as it is using autotools which you modify.
I am not an expert, either. But I can have a look at that. Let me know when it will be ready, I will check the debian stuff.
kyz
commented
1 year ago Well, there was so much about it that it seemed like a major issue - so now too late... ;-) Or you can revert it - this time up to you. Both ways are fine for me.
To me, it is a major issue if we produce a library with a function that looks like it does one thing, but actually does another, and I'd still like to get it fixed. We really should fix it before release. I have a proposal for how to fix it as a PR. It can remain open, it does not affect any release process.
If you want torelease without fixing it, I don't want to get in your way, given how much you've already written, but I'd like us to do all we can to signal to users that this function is not doing what you'd expect, and we plan to change it. We already changed the API heavily, so anyone using this library now has to make changes to keep using it, but we should signal there are more changes to come.
lclevy
commented
1 year ago What about another "open" name for this function with a different behaviour? Adfopen2 or openposix?
kyz
commented
1 year ago It would be fine to have a second file open function, named appropriately - ideally there is both an open() style API and fopen() style API. The latter is implemented in a few lines translating the call to the former.
Prior to today, there was one function for opening files, adfOpenFile(), and it only accepted "r" as the mode.
From this release, it is replaced with adfFileOpen(), which does support writing files. I would not like this scenario:
What I would like:
kyz
commented
1 year ago add man pages for the utilities
I added the unadf manpage from Debian and updated it. Perhaps you could do the manpages for the new utilities?
t-w
commented
1 year ago It would be fine to have a second file open function, named appropriately - ideally there is both an open() style API and fopen() style API. The latter is implemented in a few lines translating the call to the former.
Such new functions require also changes in other functions (write/truncate/flush), which behaviour would change in different modes. And that is not trivial and would have to be done very carefully and well tested.
Prior to today, there was one function for opening files, adfOpenFile(), and it only accepted "r" as the mode.
This is simply not true:
struct File* adfOpenFile(struct Volume *vol, char* name, char *mode) { [...] write=( strcmp("w",mode)==0 || strcmp("a",mode)==0 );
if (write && vol->dev->readOnly) {
(*adfEnv.wFct)("adfFileOpen : device is mounted 'read only'");
return NULL;
}[...] if (write && nSect!=-1) { (*adfEnv.wFct)("adfFileOpen : file already exists"); return NULL; } [...] if (strcmp("w",mode)==0) { memset(file->fileHdr,0,512); adfCreateFile(vol,vol->curDirPtr,name,file->fileHdr); file->eof = TRUE; } else if (strcmp("a",mode)==0) { memcpy(file->fileHdr,&entry,sizeof(struct bFileHeaderBlock)); file->eof = TRUE; adfFileSeek(file, file->fileHdr->byteSize); } else if (strcmp("r",mode)==0) { memcpy(file->fileHdr,&entry,sizeof(struct bFileHeaderBlock)); file->eof = FALSE; }
This is from `archive/adflib-0.7.12.tar.bz2` (see also [tagged source](https://github.com/lclevy/ADFlib/blob/f3af9e019ff55537f990fb02f3ca0c3febc7b28c/src/adf_file.c#L273)), so the functionality/API was there for a long time, it is not introduced by my updates. It was partially implemented, eg. file could be written only sequentially, "a" was just not checking if the file exists, but assuming it does... And "w" was not allowing to overwrite an existing file (implementation for it was not there). Seek was not working for many of the cases... but, from API side, it was all there.
> From this release, it is replaced with adfFileOpen(), which does support writing files.
It is not _replaced_, it is _completed_ (and improved by a bit of refactoring). Changing the name is not replacing functionality...
> I would not like this scenario:
>
> 1. adfFileOpen() where "w" doesn't truncate files and "a" doesn't create files and generally _looks_ just like the fopen() API but doesn't _behave_ like it
>
> 2. adfFileOpenC89() where it both looks like it and behaves like it?
>
> 3. adfFileOpen2() where it looks and behaves like open() ?
>
>
> What I would like:
>
> 1. adfFileOpen() where it looks like and behaves like fopen()
>
> 2. adfFileOpen2() (or a better name) where it looks and behaves like open()
Yeah, sure.... Half of the library will be implementation of different ways of opening the file, so that anyone can choose. Let me ask you a question: how about write? Which one:ssize_t write ( int fd, const void *buf, size_t count );
size_t fwrite ( const void ptr, size_t size, size_t nmemb, FILE stream );
The current one in ADFlib doesn't match any... moreover - it is confusing as it is actually similar to the first one (`write()`) but with _swapped_ parameters:uint32_t adfFileWrite ( struct AdfFile const file, const uint32_t n, const uint8_t const buffer );
So, do we implement 2 or 3 versions for clients to choose as well? For read, too?
As for me: **NO, I DO NOT AGREE WITH THIS!**
So far, ADFlib's contains, in general, _low level functionality for accessing/manipulating the Amiga filesystem_. And what is there and is _working_ should remain _possibly_ as is (at least until some major rewrite) - and that is what I was trying to preserve. The existing file API function calls are simple, flexible, and can cover at least basic/essential functionality of any file API style you want.
So, since you both brought the additional functions to the table, I propose something that I thought about since the beginning:
**A strictly standard-compliant/style API**, as you expect to have (which is OK., myself, I wanted POSIX-like stuff from the start - but it was just not feasible to change complex existing code to achieve that) **should be done as _a wrapper over the existing low-level functions_**.
So, for POSIX/low level style (open/close + file descriptors):
- a new module `adf_file_posix.h/c` with
- functions (with standard-compliant parameters): `adfFile_open` (with modes as bitmasks), `adfFile_close`, `adfFile_read`, `adfFile_write`, `adfFile_truncate` which will use existing low-level `adfFile[...]` functions
- descriptor management
- user should get only an `int` (descriptor/id of the opened file), everything else should be hidden
- probably more things (like directory management API) would have to be done in a similar way, otherwise users may be surprised...
(naming is up to debate, but it _has_ to be visually separated/different from the low-level part)
This approach will definitely stabilize the file API according to any chosen standard and, also, allow to evolve the lower level part as needed, without disturbing the clients.
If you want also/instead POSIX/stdlib (higher-level style), too - a new module `adf_file_stdlib.h/c`.
For me, this kind of wrapper can be just done in the _client code_. That is what I have done myself in `fuseadf` - which is a (simple) _filesystem_ implementation. If something changes in the next version of the lib - I will update my wrapper code and update dependency to the newer lib. I do not see any problem with that. I prefer that the lib gets improved in a significant way than having a frozen, standard-like API making it difficult to make any changes (to a library that, clearly, may require many more improvements). Even Linux kernel, having a bit more users than ADFlib, have an evolving API, occasionally disturbing 3rd party drivers (I remember very well correcting glue code to build NVIDIA drivers for an updated kernel...).
Wrappers with a standard-inspired API for the essential API can address that.
Other way is to change all the functions to "unsurprising" versions. If so - for which release?
If 0.8.0, we just postpone it.
My proposition is to focus on completing the remaining things (this issue is dedicated to that), and leave this discussion to #38 and #42 where is belongs (and post 0.8.0 releases).
For 0.8.0, we can annotate that the API is not stable and very likely will change in the next release. Do we need to do more for this?Well, there was so much about it that it seemed like a major issue - so now too late... ;-) Or you can revert it - this time up to you. Both ways are fine for me.
To me, it is a major issue if we produce a library with a function that looks like it does one thing, but actually does another,
That's your interpretation, which, as you argumented, was based on "a" not creating the file. Or there is some other problem? With "w"? or "r"?.
Edit: Oh, yes, I forgot: "w" is not truncating... But if this "w" truncates, how can we overwrite?
and I'd still like to get it fixed. We really should fix it before release.
Now it is gone (no "a"). So what else is here to fix (urgently)? Improvements in the API could be plenty (starting with proper prefixing in all subsystems / modules - this is A LOT, it will annoy users more and one option of one function...).
I have a proposal for how to fix it as a PR. It can remain open, it does not affect any release process.
If you want to release without fixing it, I don't want to get in your way, given how much you've already written, but I'd like us to do all we can to signal to users that this function is not doing what you'd expect,
Not anymore (no "a").
and we plan to change it. We already changed the API heavily, so anyone using this library now has to make changes to keep using it, but we should signal there are more changes to come.
Change in the version signaling API changes + release notes / ChangeLog + stating that the current API in not stable and will likely change. What else?
Otherwise, if it will be decided that we start fixing the API (plenty to do), fine. But for me, we should mark a milestone with 0.8.0 where we have things working (so that it can be used (even if it will require some changes in the code later), tested and maybe also result in some feedback, also regarding the API), and work on polishing the API later. The current file API (without "a") is minimal, can only be extended so I do not see how it will make trouble.
t-w
commented
1 year ago To remove the confusion with the last thing ("w") - maybe we can change the modes from "r" and "w" to something like:
ADFLIB_FILE_MODE_READ 1
ADFLIB_FILE_MODE_READWRITE 2so that we would have:
adfFileOpen ( , ADFLIB_FILE_MODE_READ ) - for reading only
adfFileOpen ( , ADFLIB_FILE_MODE_READWRITE ) - for read-write onlyLike this, we change only the adfFileOpen() signature, we do not have to change anything else (except for the existing calls to adfFileOpen() in tests, utils etc.).
The naming is library-specific, not suggesting any standard or other library (I think...), it should not cause trouble. Would this be acceptable? This is the simplest thing we can do.
What do you think? Would this be an acceptable fix?
kyz
commented
1 year ago This is simply not true:
OK, I stand corrected.
Yeah, sure.... Half of the library will be implementation of different ways of opening the file, so that anyone can choose. Let me ask you a question: how about write? Which one:
The reason I care, specifically about adfFileOpen(fname, "w"), goes back to the aCropalypse bug, where programmers see ... open ... "w" ... and think it truncates, because that's what most APIs do, but this one doesn't. That's a footgun. It's a footgun that continues to cause problems today in Android.
The same footgun doesn't exist for adfFileWrite vs write.
A strictly standard-compliant/style API ... as a wrapper over the existing low-level functions
I'm completely OK with this. And I'm also happy for that to be in the 0.9.0+ future. My concern for 0.8.0 is exclusively the footgun of "w" not truncating.
Edit: Oh, yes, I forgot: "w" is not truncating...
Indeed, it's also an Android bug caused by human factors, which lost millions of people their privacy, which is why I care about it.
But if this "w" truncates, how can we overwrite?
r+ Open for reading and writing. The stream is positioned at the
beginning of the file.maybe we can change the modes from "r" and "w"
That's very much what I have been saying, although perhaps I haven't been saying it the right way round. It sounds like you're understanding me now.
It is not that we must follow the C89 stdio behaviour, although that would be nice, and open() even better. It's that adfFileOpen() looks like fopen(), specifically the use of a string for the file mode, specifically because the accepted strings for the file mode match fopen()... the API style came from_ fopen()... but fails to do what anyone knowing fopen() would expect.
So either we make it behave like fopen() to match that it looks like fopen()... or we make it not look like fopen()
adfFileOpen ( , ADFLIB_FILE_MODE_READ ) - for reading only adfFileOpen ( , ADFLIB_FILE_MODE_READWRITE ) - for read-write only
Yes, this is ideal. Allow me to propose one step further: AmigaDOS
/* Mode parameter to Open() */
#define MODE_OLDFILE 1005 /* Open existing file read/write
* positioned at beginning of file. */
#define MODE_NEWFILE 1006 /* Open freshly created file (delete
* old file) read/write, exclusive lock. */
#define MODE_READWRITE 1004 /* Open old file w/shared lock,
* creates file if doesn't exist. */What do you think? Would this be an acceptable fix?
Yes. And document what adfFileOpen() does do, does not do, in the API docs.
lclevy
commented
1 year ago I really think we must decide minimal set of changes for 0.8.0. Priority must be bug and build fixes. Not everything has to be perfect as a first shot
t-w
commented
1 year ago Closing (see the 0.8.0 release).
I guess we need to clarify what we do for the 0.8.0, prioritize a bit and close what really matters now, while leave other things for later.
I'd see the following priorities:
adfFileOpen()(edit: done in 75004da)unadf(#39)No other major or breaking changes that will take time to implement/fix (ie. no bumping requirements regarding build system software versions).
Let me know what you think.