Add removable_storage option to rsync syncer

[map7]

This is without the mounted? method for now, just checking for the sub directory on the removable media

[map7]

Just added the code to rsync syncer which will check the mount point instead of the backup path

[map7]

With the checking of the mount point if the path is a sub directory of the removable_storage mount point then it won't work. EG:
Assuming the mount point is /home/map7/usb the following will not work
path = /home/map7/usb/test
This is because of the way we check the mount point. I'm thinking of introducing another variable called remote_storage_mount_point which we could set to '/home/map7/usb' in this situation and it would work.

[tombruijn]

@map7 I thought the mount? check would fix the problem that File.exists? had? So it could check if /home/map7/usb was a mounted device?

[map7]

This current solution does check if the path == your mount point, so if you mount your usb in /home/map7/usb and that is your destination path then it will work. The problem happens when you have your usb mount point as /home/map7/usb and your path set to /home/map7/usb/servers, this will not work at all. I'm trying to work around this problem.

Maybe we need an extra field called remote_storage_mount_point or something?
What do you think?

[tombruijn]

@map7 I woudn't want to introduce a new configuration option. Then I would need a separate class as there will be too much specific configuration.

Couldn't we check if the path you are specifying is part of a mounted path?

if mount_points.select { |mount_point| remote_path.include? mount_point  }.length > 0
  # ...
end

Because the root harddisk is also a mounted point they should probably be filtered from the mount_points return value.

mount_points.reject { |x| %w(/ /dev).include? x }

What do you think?

[map7]

Yes I agree, but we need to reject more than just / /dev, I've added this to the new commit.

[tombruijn]

@map7 Ah I see, but instead wouldn't a .grep(/\/dev/) fix that on map7@15c0a80#diff-447febccf1e28b3b863e194edb5d53a7R55? You only select the lines that are actually contain /dev? Now a check is only done on it containing dev.

Would that work as well? Then you don't have to add the removable_mount_points method?

[map7]

Yeah you could I just like keeping my methods short

On 3 October 2014 20:58:26 GMT+10:00, Tom de Bruijn notifications@github.com wrote:

@map7 Ah I see, but instead wouldn't a .grep(/\/dev/) fix that on
map7@15c0a80#diff-447febccf1e28b3b863e194edb5d53a7R55?
You only select the lines that are actually contain /dev? Now a check
is only done on it containing dev.

Would that work as well? Then you don't have to add the
removable_mount_points method?

---

Reply to this email directly or view it on GitHub:
#590 (comment)

[tombruijn]

@map7 I understand that, but now you have 2 methods, one of which (mount_points) isn't used by anything else but removable_mount_points. Plus, the other mount point checks you do in removable_mount_points will not even be returned from mount_points as it only returns lines with the string dev in them. So things like /home won't even be returned.

I was thinking of maybe shortening it even more in just one method with just this:

def mount_points
  `mount`.scan(/^[^ ]+ on (\/dev[^ ]+)/).flatten
end

What that does is finding every match of "on /dev" on every line and return every mount point in an array. This way we don't have to split, grep, map and value check the array of mount points.

It will return:

["/dev", "/dev/foo", "/dev/bar"]

Would you agree that this is a simpler solution? Other than this I have no other issues with this PR. Do you still want to support this in the Local storage as well?

---

I understand I might be one of those crazies that somewhat understands regex, so I'll post a short explanation as well.

/^[^ ]+ on (\/dev[^ ]+)/

• /^ Start of regex, begin this search on the start of every line.
• [^ ]+ find every character that's not a space, and check for one or more occurrences.
• on find the word "on" surrounded by spaces
• (\/dev find the string "/dev" and start a regex match with (.
• [^ ]+) find every character that's not a space again and end the regex match.
• / end regex

[map7]

I don't think this is simpler, but I do agree with combining both methods into one. Also I found that when I run mount.scan(/^[^ ]+ on (/dev[^ ]+)/).flatten it only returns ["/dev/pts"].

I did play around with the regex a little and managed to get
mount.scan(//dev/[^ ]+ on [^ ]+/)
=> ["/dev/mapper/vg.lvm.sol-root on /", "/dev/md0 on /boot", "/dev/mapper/vg.lvm.sol-home on /home", "/dev/sdd1 on /media/data", "/dev/sdc1 on /media/backup", "/dev/sde1 on /home/map7/usb"]

which includes my 'usb' mount in there, but I would still have to map & split. I've done regex before heaps of times but I usually leave it to a minimum because it's harder to read. Even though split, grep, map & split are longer I can see straight away what it's doing.

I'll tidy it up a little in this next commit

[map7]

Yes this should be in local as well, let me add the new code to that

[map7]

Just fixed up the storage local & rsync as well as their related tests

[tombruijn]

I feel like this rename doesn't cover what is happening. It's not just checking if the path is mounted, but it's checking if the path is available for writing and that's not what happening anymore when you read it.

[tombruijn]

@map7 I've thought about it, and unfortunately thanks to not having a lot of free time, only now came to a decision. I decided not to merge this.

It adds some complexity not everyone needs and I can't really think of a good way to implement it right now in the core. I fear that it will get in the way of the normal behavior. I've added it to the backup-features issue list for now backup/backup-features#19. (It might not look active right now because it's more of a todo list for v5 of the utility. And development on v5 is done in private for now.)

Thanks for the work you put in, this case will certainly stay on our minds. I hope you can use a workaround in the mean time.