search

Ferk / udev-media-automount

A simple automount mechanism using udev rules
Other
125 stars 59 forks source link

Mount UUID #14

Closed ahostetler300 closed 1 year ago

Ferk commented 1 year ago

Hi, Thanks for the PR!

I'm not sure about wanting to merge this as is, some notes:

  1. Adding an uninstall goal to the Makefile is something I wanted to do, the problem is that it will also delete the possibly user-modified configuration in /etc/media-automount.d which might be a problem when reinstalling. Perhaps there should be a prompt asking the user whether they want to remove that one (and by default, don't remove any /etc/* files).

  2. I'm not sure if using the UUID for the directory name by default is a good idea.

Could you elaborate on why you want this? The use of the label (with the device name as fallback) is meant to make it more identifiable for the end user. I don't think it would be very user friendly to use the UUID code as a folder name.

We could compromise by having the script not override the AUTOMOUNT_DIR environment variable. So that it would be possible for anyone to customize the directory name using, for example, /etc/media-automount.d/auto, or to use special names for a particular FS TYPE.

Either way, I think the check for conflicting names should stay, since we never know what other mounts might the user have. It'd be weird to have a mount with exactly the same name as the UUID, but it's not impossible, so I'd rather keep it, since in normal operation that check does not hurt and it can potentially help identify problems.

  1. Is the 5 times retry necessary? Have you encountered a situation where it fixed a problem? note that this would make every repeatable mount failure take always 25 seconds long. Would it cause issues on quick plug/unplug? It's accessing the same resources (and creating/removing folders) so I'm wondering if a long pending retry could lead to concurrency issues.

For detachable storage, if the user wants to retry they can always do it themselves re-plugging it. An error when mounting might be a sign that the storage wasn't properly attached, or some other reason that I'm not convinced it'll be fixed by just retrying the mount call.

ahostetler300 commented 1 year ago

Thanks for the reply. Apologies, but I'm new to GitHub, and didn't realize I had initiated a PR to the main project, I had intended to keep my changes in my local branch.

That said, I'm happy to explain the reasoning behind my changes.

In general, I'm using this code for an edge video recorder, and typically this device runs unattended, so my needs are likely unique.

  1. In my use case, we will always use the same configuration across multiple devices, so I expect the standard configuration to be cloned to each device, I really just needed an easy way to wipe uninstall and reinstall when there are code changes
  2. The issue I had with the dev label is that this can change after a reboot, and I need a mount point that doesn't change for a given device, UUID met that requirement. In my use case, we only use one or two drive max for the life of the device, so I didn't feel conflicting names would be an issue.
  3. That might be overkill, but I did have issues with the drive mounting consistently at boot up, and in my use case, I need to ensure the drive mount any time the device loses power and reboots. When reviewing the logs, after adding my code, I believe most thing mounted by the 2nd attempt, and I recall a few instances where the device mounted after the 3rd attempt. Keep in mind, this is for an edge use case, so our testing includes lots of power interruptions.
Ferk commented 1 year ago

I'm closing the pull request, since this was not intended to be posted and was meant for a local branch with a specific usecase, as you clearly explained.

Thanks for taking the time to clarify.