Re: [PATCH] Cygwin: expose all windows volume mount points.
On Feb 7 20:38, Jeremy Drake via Cygwin-patches wrote: > They are exposed via the getmntent API and proc filesystem entries > dealing with mounts. This allows things like `df` to show volumes > that are only mounted on directories, not on drive letters. > > Addresses: https://cygwin.com/pipermail/cygwin/2025-February/257251.html > Signed-off-by: Jeremy Drake > --- > Rather amazingly, this seemed to work as I expected. Kind of gross due to > keeping state in the _mytls.locals struct, but it seems to do the job. > Does this approach make sense, or is there something I'm missing? The approach makes sense, but... On Feb 8 10:27, Jeremy Drake via Cygwin wrote: > On Sat, 8 Feb 2025, Corinna Vinschen via Cygwin wrote: > > Go for it. There's already matching logic in fhandler/proc.cc, > > function format_proc_partitions() for the "win-mounts" column > > of /proc/partitions. Probably this can be reused. > > Actually closer to the dos_drive_mappings at the end of mount.cc, except > that only looks at the first mount point. But, reusing anything might be > difficult due to having to save state and resume iteration on subsequent > calls. Open to suggestions on > https://cygwin.com/pipermail/cygwin-patches/2025q1/013353.html. Yes, dos_drive_mappings() is what I really meant, thanks for pointing it out. So I wonder why not include your additional requirements into the dos_drive_mappings class and just use it to iterate over the mount points. AFAICS there are only two things missing in dos_drive_mappings: - looking for all mount points, not just the first one, and - bookkeeping over getmntent calls If you add a state pointer (pointing to the current mapping) to dos_drive_mappings, you only need a single slot in the TLS, holding a pointer to your dos_drive_mappings instance. [...time passes...] Hang on, there might be a bug here... [...time passes...] Why do we keep the cygdrive state in cygtls anyway? From history I see that this has been the case since at least 2003. Per definition, getmntent isn't thread-safe at all, and getmntent_r is only thread-safe in that it keeps the data in a buffer provided by the caller. However, the state information is process-wide: if you call getmntent alternating in two different threads, they don't see the same set of drives, but only every second drive. At least this is the case on Linux. Don't we subvert expectations by handling getmentent thread-local? If I'm thinking to much outside the box, feel free to kick me. Thanks, Corinna
Re: [PATCH] Cygwin: expose all windows volume mount points.
Used wrong from address... :( On Mon, 10 Feb 2025, Jeremy Drake wrote: > On Mon, 10 Feb 2025, Corinna Vinschen wrote: > > > Yes, dos_drive_mappings() is what I really meant, thanks for pointing > > it out. > > > > So I wonder why not include your additional requirements into the > > dos_drive_mappings class and just use it to iterate over the mount > > points. AFAICS there are only two things missing in dos_drive_mappings: > > > > - looking for all mount points, not just the first one, and > > - bookkeeping over getmntent calls > > > > If you add a state pointer (pointing to the current mapping) to > > dos_drive_mappings, you only need a single slot in the TLS, holding a > > pointer to your dos_drive_mappings instance. > > This would be much cleaner, I think. I'll look at this. > > > [...time passes...] > > > > Hang on, there might be a bug here... > > > > [...time passes...] > > > > Why do we keep the cygdrive state in cygtls anyway? From history I see > > that this has been the case since at least 2003. > > > > Per definition, getmntent isn't thread-safe at all, and getmntent_r is > > only thread-safe in that it keeps the data in a buffer provided by the > > caller. > > However, the state information is process-wide: if you call getmntent > > alternating in two different threads, they don't see the same set of > > drives, but only every second drive. > > > > At least this is the case on Linux. Don't we subvert expectations > > by handling getmentent thread-local? > > > > If I'm thinking to much outside the box, feel free to kick me. > > That's well outside my scope ;). I think 1) getmentent is pretty well > deprecated, and 2) if something isn't defined to be thread-safe, pretty > much anything it happens to do in the face of multiple threads using it is > fine (to me, we're in the realms of UB here). >
Re: Subject: [PATCH v2 2/2] Cygwin: expose all windows volume mount points.
Realized an oversight (besides the messed-up subject!) after sending:
> @@ -1943,14 +1961,6 @@ extern "C" FILE *
> setmntent (const char *filep, const char *)
> {
>_my_tls.locals.iteration = 0;
> - _my_tls.locals.available_drives = GetLogicalDrives ();
> - /* Filter floppy drives on A: and B: */
> - if ((_my_tls.locals.available_drives & 1)
> - && get_disk_type (L"A:") == DT_FLOPPY)
> -_my_tls.locals.available_drives &= ~1;
> - if ((_my_tls.locals.available_drives & 2)
> - && get_disk_type (L"B:") == DT_FLOPPY)
> -_my_tls.locals.available_drives &= ~2;
should have something like
+ if (_my_tls.locals.drivemappings)
+{
+ delete _my_tls.locals.drivemappings;
+ _my_tls.locals.drivemappings = NULL;
+}
here
[PATCH v2 1/2] Cygwin: make list of mounts for a volume in dos_drive_mappings
make mappings linked list in order rather than reverse order.
Signed-off-by: Jeremy Drake
---
winsup/cygwin/local_includes/mount.h | 11 +---
winsup/cygwin/mount.cc | 39
2 files changed, 35 insertions(+), 15 deletions(-)
diff --git a/winsup/cygwin/local_includes/mount.h
b/winsup/cygwin/local_includes/mount.h
index b2acdf08b4..c96b34781e 100644
--- a/winsup/cygwin/local_includes/mount.h
+++ b/winsup/cygwin/local_includes/mount.h
@@ -223,12 +223,15 @@ class dos_drive_mappings
struct mapping
{
mapping *next;
-size_t doslen;
size_t ntlen;
-wchar_t *dospath;
wchar_t *ntdevpath;
- };
- mapping *mappings;
+struct dosmount
+{
+ dosmount *next;
+ wchar_t *path;
+ size_t len;
+} dos;
+ } *mappings;
public:
dos_drive_mappings ();
diff --git a/winsup/cygwin/mount.cc b/winsup/cygwin/mount.cc
index bf26c4af3e..82ed4259f7 100644
--- a/winsup/cygwin/mount.cc
+++ b/winsup/cygwin/mount.cc
@@ -2009,6 +2009,7 @@ dos_drive_mappings::dos_drive_mappings ()
if (sh == INVALID_HANDLE_VALUE)
debug_printf ("FindFirstVolumeW, %E");
else {
+mapping **nextm = &mappings;
do
{
/* Skip drives which are not mounted. */
@@ -2047,20 +2048,30 @@ dos_drive_mappings::dos_drive_mappings ()
mapping *m = new mapping ();
if (m)
{
- m->dospath = wcsdup (mounts);
+ if ((m->dos.path = (wchar_t *) malloc (len * sizeof (WCHAR
+ memcpy (m->dos.path, mounts, len * sizeof (WCHAR));
m->ntdevpath = wcsdup (devpath);
- if (!m->dospath || !m->ntdevpath)
+ if (!m->dos.path || !m->ntdevpath)
{
- free (m->dospath);
+ free (m->dos.path);
free (m->ntdevpath);
delete m;
continue;
}
- m->doslen = wcslen (m->dospath);
- m->dospath[--m->doslen] = L'\0'; /* Drop trailing backslash */
+ mapping::dosmount *dos = &m->dos;
+ for (wchar_t *mount = m->dos.path;
+ dos;
+ mount += dos->len + 2,
+ dos->next = mount[0] ? new mapping::dosmount () : NULL,
+ dos = dos->next)
+ {
+ dos->path = mount;
+ dos->len = wcslen (dos->path);
+ dos->path[--dos->len] = L'\0'; /* Drop trailing backslash */
+ }
m->ntlen = wcslen (m->ntdevpath);
- m->next = mappings;
- mappings = m;
+ *nextm = m;
+ nextm = &m->next;
}
}
else
@@ -2088,11 +2099,11 @@ dos_drive_mappings::fixup_if_match (wchar_t *path)
{
wchar_t *tmppath;
- if (m->ntlen > m->doslen)
- wcsncpy (path += m->ntlen - m->doslen, m->dospath, m->doslen);
+ if (m->ntlen > m->dos.len)
+ wcsncpy (path += m->ntlen - m->dos.len, m->dos.path, m->dos.len);
else if ((tmppath = wcsdup (path + m->ntlen)) != NULL)
{
- wcpcpy (wcpcpy (path, m->dospath), tmppath);
+ wcpcpy (wcpcpy (path, m->dos.path), tmppath);
free (tmppath);
}
break;
@@ -2106,8 +2117,14 @@ dos_drive_mappings::~dos_drive_mappings ()
for (mapping *m = mappings; m; m = n)
{
n = m->next;
- free (m->dospath);
+ free (m->dos.path);
free (m->ntdevpath);
+ mapping::dosmount *dn;
+ for (mapping::dosmount *dm = m->dos.next; dm; dm = dn)
+ {
+ dn = dm->next;
+ delete dm;
+ }
delete m;
}
}
--
2.47.1.windows.2
Subject: [PATCH v2 2/2] Cygwin: expose all windows volume mount points.
They are exposed via the getmntent API and proc filesystem entries
dealing with mounts. This allows things like `df` to show volumes
that are only mounted on directories, not on drive letters.
Addresses: https://cygwin.com/pipermail/cygwin/2025-February/257251.html
Signed-off-by: Jeremy Drake
---
winsup/cygwin/cygtls.cc | 6 ++
winsup/cygwin/fhandler/process.cc | 6 +-
winsup/cygwin/local_includes/cygtls.h | 6 +-
winsup/cygwin/local_includes/mount.h | 5 +-
winsup/cygwin/mount.cc| 102 +-
5 files changed, 82 insertions(+), 43 deletions(-)
diff --git a/winsup/cygwin/cygtls.cc b/winsup/cygwin/cygtls.cc
index 1134adc3e2..13d133f47c 100644
--- a/winsup/cygwin/cygtls.cc
+++ b/winsup/cygwin/cygtls.cc
@@ -121,6 +121,12 @@ _cygtls::remove (DWORD wait)
CloseHandle (h);
}
+ if (locals.drivemappings)
+{
+ delete locals.drivemappings;
+ locals.drivemappings = NULL;
+}
+
/* Close handle and free memory used by select. */
if (locals.select.sockevt)
{
diff --git a/winsup/cygwin/fhandler/process.cc
b/winsup/cygwin/fhandler/process.cc
index f779028116..76056f4983 100644
--- a/winsup/cygwin/fhandler/process.cc
+++ b/winsup/cygwin/fhandler/process.cc
@@ -1397,7 +1397,7 @@ format_process_mountstuff (void *data, char *&destbuf,
bool mountinfo)
/* Store old value of _my_tls.locals here. */
int iteration = _my_tls.locals.iteration;
- unsigned available_drives = _my_tls.locals.available_drives;
+ class dos_drive_mappings *drivemappings = _my_tls.locals.drivemappings;
/* This reinitializes the above values in _my_tls. */
setmntent (NULL, NULL);
/* Restore iteration immediately since it's not used below. We use the
@@ -1470,8 +1470,8 @@ format_process_mountstuff (void *data, char *&destbuf,
bool mountinfo)
}
}
- /* Restore available_drives */
- _my_tls.locals.available_drives = available_drives;
+ /* Restore old values of _my_tls.locals here. */
+ _my_tls.locals.drivemappings = drivemappings;
if (u_hdl) /* Only not-NULL if open_shared has been called. */
{
diff --git a/winsup/cygwin/local_includes/cygtls.h
b/winsup/cygwin/local_includes/cygtls.h
index d814814b19..dfd3198435 100644
--- a/winsup/cygwin/local_includes/cygtls.h
+++ b/winsup/cygwin/local_includes/cygtls.h
@@ -93,10 +93,10 @@ struct _local_storage
int dl_error;
char dl_buffer[256];
- /* path.cc */
+ /* mount.cc */
struct mntent mntbuf;
int iteration;
- unsigned available_drives;
+ class dos_drive_mappings *drivemappings;
char mnt_type[80];
char mnt_opts[80];
char mnt_fsname[CYG_MAX_PATH];
@@ -181,7 +181,7 @@ public: /* Do NOT remove this public: line, it's a marker
for gentls_offsets. */
siginfo_t *sigwait_info;
HANDLE signal_arrived;
bool will_wait_for_signal;
-#if 1
+#if 0
long __align;/* Needed to align context to 16 byte.
*/
#endif
/* context MUST be aligned to 16 byte, otherwise RtlCaptureContext fails.
diff --git a/winsup/cygwin/local_includes/mount.h
b/winsup/cygwin/local_includes/mount.h
index c96b34781e..8f2c225f53 100644
--- a/winsup/cygwin/local_includes/mount.h
+++ b/winsup/cygwin/local_includes/mount.h
@@ -216,6 +216,7 @@ class mount_info
bool from_fstab (bool user, WCHAR [], PWCHAR);
int cygdrive_win32_path (const char *src, char *dst, int& unit);
+ struct mntent *cygdrive_getmntent ();
};
class dos_drive_mappings
@@ -231,11 +232,13 @@ class dos_drive_mappings
wchar_t *path;
size_t len;
} dos;
- } *mappings;
+ } *mappings, *cur_mapping;
+ mapping::dosmount *cur_dos;
public:
dos_drive_mappings ();
~dos_drive_mappings ();
wchar_t *fixup_if_match (wchar_t *path);
+ wchar_t *next_dos_mount ();
};
#endif
diff --git a/winsup/cygwin/mount.cc b/winsup/cygwin/mount.cc
index 82ed4259f7..135f230d22 100644
--- a/winsup/cygwin/mount.cc
+++ b/winsup/cygwin/mount.cc
@@ -1645,14 +1645,8 @@ fillout_mntent (const char *native_path, const char
*posix_path, unsigned flags)
struct mntent& ret=_my_tls.locals.mntbuf;
bool append_bs = false;
- /* Remove drivenum from list if we see a x: style path */
if (strlen (native_path) == 2 && native_path[1] == ':')
-{
- int drivenum = cyg_tolower (native_path[0]) - 'a';
- if (drivenum >= 0 && drivenum <= 31)
- _my_tls.locals.available_drives &= ~(1 << drivenum);
append_bs = true;
-}
/* Pass back pointers to mount_table strings reserved for use by
getmntent rather than pointers to strings in the internal mount
@@ -1744,41 +1738,65 @@ mount_item::getmntent ()
return fillout_mntent (native_path, posix_path, flags);
}
-static struct mntent *
-cygdrive_getmntent ()
+struct mntent *
+mount_info::cygdrive_getmntent ()
{
- char native_path[4];
- char posix_path[CYG_MAX_PATH];
- DWORD mask = 1, drive = 'a';
- struct mntent *ret = NULL;
+ tmp_pathbuf tp;
+ wchar_t *wide_path;
+ char *win32_path, *posix_path;
