Date: Tue, 26 Dec 2000 10:51:45 +0200 (IST) From: Eli Zaretskii X-Sender: eliz AT is To: Tim Van Holder cc: djgpp-workers AT delorie DOT com Subject: Re: Draft patch for opendir() extension In-Reply-To: Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Reply-To: djgpp-workers AT delorie DOT com Errors-To: nobody AT delorie DOT com X-Mailing-List: djgpp-workers AT delorie DOT com X-Unsubscribes-To: listserv AT delorie DOT com Precedence: bulk On Mon, 25 Dec 2000, Tim Van Holder wrote: > Below is a patch to extend opendir() et al to support 'virtual > files' introduced by the fsext mechanism (eg /dev/zero). The > idea is that fsext modules register and deregister any files > they handle. Thanks for working on this. > The registering/deregistering isn't yet implemented in this > draft, as this probably requires more thought: i.e. do fsext > functions know the name of the file they're working on after > the initial open()? I send a few comments about the code below, but I suggest first to invest the thought you mention above, before plunging into the implementation details. It is hard to reflect on code before dust settles on important API and usage considerations. Here are some issues I suggest to discuss: - Why would an application want to fake directory entries, and for what kind of functionality? Until now, we have been talking about /dev/* pseudo-files only, which don't need such a general-purpose mechanism. - In what kind of data structure(s) will the fake entries be stored? - How would an application register and deregister entries in practice? In what directory (or directories) will those entries be visible, and what API does an application need to control this? - Can fake entries be registered before an opendir call for their parent directory? Can they be deregistered before closedir is called? - Will the fake entries appear in the beginning or the end of the real entries? - Why do we need to invent a special mechanism? Isn't it enough to define an __FSEXT_dir hook that would be called by all these functions, in the same manner as __FSEXT_open hook is called by _open? > This patch also fixes a bug in telldir/seekdir. > telldir() returns the number of actual entries read and > seekdir() calls readdir() that many times after doing a > rewinddir(). But if the DIR has simulated dots (or with this > patch, other simulated entries), those numbers do not match. Please don't mix patches for different problems together. Please send the telldir patches separately. Anyway, I'm not sure what is the bug you are trying to solve. Can you give an example of a series of calls to opendir, readdir, telldir, and seekdir where the current implementation fails? > + char** > + __fsext_entries (const char* d) > + { > + return NULL; > + } It is not clear why do you need this function. Since the fake entries are stored in an array accessible from the DIR structure, what do you need __fsext_entries for? > + int exists; > + char **fsext_entries; > + char **fsext_entry; > }; Please explain the need for the `exists' member. I also don't understand why do you need both fsext_entries and fsext_entry, since they both are used to index into the same array. Isn't num_read enough for that? > ! /* Ensure that directory to be accessed exists; only matters if there are > ! * no entries faked by the fsext system. */ > if (access(dir->name, D_OK)) > { > ! if (dir->fsext_entries == NULL) > ! { > ! free(dir->name); > ! free(dir); > ! return 0; > ! } > ! else > ! { > ! dir->exists = 0; > ! dir->need_fake_dot_dotdot = 2; > ! return dir; > ! } I don't understand this: why do you keep reporting data to the application if the directory doesn't exist? This code is about fake directory entries, not about fake directories, isn't it? Also, how is the fact that dir->fsext_entries is non-NULL relevant to whether the directory may or may not be non-existent? > ! if (dir->exists == 2) > done = findnext(&dir->ff); > else > { > *************** > *** 39,44 **** > --- 53,59 ---- > if (dir->flags & __OPENDIR_FIND_LABEL) > ff_flags |= FA_LABEL; > done = findfirst(dir->name, &dir->ff, ff_flags); > + dir->exists = 2; What is the special meaning of dir->exists == 2? If this is not a simple boolean variable, I think the name `exists' is inappropriate.