True testing of hard link capability in setting Timeshift location

[prl]

I've started implementing Enhancement request #364: Timeshift code should check for hardlink feature and as an initial step in doing that, I've been looking at removing the huge amount of copypasta in Screens.Timeshift.TimeshiftSettings methods checkReadWriteDir(), dirnameSelected() and keySave(). The idea is to avoid the copypasta in the new code.

In keySave() there is this odd code:

Code: Select all

	def keySave(self):
		...
		if candidates:  # prl - the list of mountpoints with whitelisted filesystem types is non-empty
		...
		else:
			if int(config.timeshift.startdelay.value) > 0:
				self.session.open(
					MessageBox,
					_("The directory %s is not a EXT2, EXT3, EXT4 or NFS partition.\nMake sure you select a valid partition type.") % config.usage.timeshift_path.value,
					type=MessageBox.TYPE_ERROR
				)
			else:
				config.timeshift.startdelay.value = "0"
				self.saveAll()
				self.close()

That code is odd, because the code sets config.timeshift.startdelay to 0 and saves it only if its value is already 0.

Surely it should be:

Code: Select all

	def keySave(self):
		...
		if candidates:  # prl - the list of mountpoints with whitelisted filesystem types is non-empty
		...
		else:
			if int(config.timeshift.startdelay.value) > 0:
				self.session.open(
					MessageBox,
					_("The directory %s is not a EXT2, EXT3, EXT4 or NFS partition.\nMake sure you select a valid partition type.") % config.usage.timeshift_path.value,
					type=MessageBox.TYPE_ERROR
				)
				config.timeshift.startdelay.value = "0"
				self.saveAll()
				self.close()

Anyone got any ideas about what it's actually supposed to do?

[MrQuade]

That code is odd, because the code sets config.timeshift.startdelay to 0 and saves it only if its value is already 0.

Or negative. I guess that is a tidy up for an invalid value?

[IanSav]

Hi Prl,

This is the new version of Timeshift.py that I developed as part of the new Setup.py redevelopment. Perhaps you can incorporate some of my planned changes.

Regards,
Ian.

[IanSav]

Hi Prl,

This is the enhanced setup.xml that goes with the above Timeshift.py. I can probably give you the patches to the current setup code that will make the new Timeshift.py fully functional on the current firmware. Do you want the changes to run my code as is?

By the way, there is also a new Recordings.py that has similar improvements and error checking.

Regards,
Ian.

[prl]

This is the enhanced setup.xml that goes with the above Timeshift.py.

Missing attachment?

[IanSav]

Hi Prl,

Damn this forum software. Hopefully here now. (Rename the attachment to setup.xml.)

Regards,
Ian.

[prl]

This is the enhanced setup.xml that goes with the above Timeshift.py.

Is there a reason why you don't just submit it?

[IanSav]

Hi Prl,

This code was ready 18 months ago but PeterU refuses to accept these sort of UI change pull requests from me. It was not worth all the effort or heartache trying to get this accepted.

I couldn't even get a simple ActionMap change accepted after over four months of solid effort! Comparatively this would be a much larger change. What do you think my chances would be of getting this accepted by PeterU?

Regards,
Ian.

[prl]

Why would mine be better?

[IanSav]

Hi Prl,

It is not a matter of being better. It is just that the odds are that if you submitted this sort of change it would be accepted by PeterU without question or issue.

I am happy to create the pull requests but only if they stand a chance of being accepted. PeterU has stated a number of times that (my) changes will not be accepted until a new branch of Beyonwiz firmware is created. There has been no specification of when or if this will ever happen.

Do you want me to create a pull request with the changes to Timeshift.py and the equivalent changes to Recordings.py? If you like I can try and tweak the code to be functional without the full Setup change so it can be tested as a standalone change before I create a pull request. Once I create a pull request I expect it to be merged in a timely manner without all the rebases, delays, etc that normally accompany my requests. If I am stuffed around I will just close the request and keep moving forward with the complete set of these changes for OpenPLi and OpenViX.

Regards,
Ian.

[IanSav]

Hi Prl,

I have updated the Timeshift.pyand Recordings.py code to more contemporary standards. If you would like to test it out just let me know and I will post the updated files here. I have changed the code such that only small changes should be required to Setup.py to make them work.

Regards,
Ian.

[prl]

It is not a matter of being better. It is just that the odds are that if you submitted this sort of change it would be accepted by PeterU without question or issue.

I was asking why by chances would be better, not about the quality of the code. I can't think of any reason why, if I submitted the same code, I'd be more likely to have the changes accepted.

[prl]

I have updated the Timeshift.pyand Recordings.py code to more contemporary standards. If you would like to test it out just let me know and I will post the updated files here. I have changed the code such that only small changes should be required to Setup.py to make them work.

There are some aspects of the way you implement the link test that I'll use because they're better than what I've done, but I intend to keep the basic framework of TimeshiftSettings unchanged.

[prl]

Hi, Ian

Is it the intention of your TimeshiftSettings code that the timeshift directory must be on the same volume as the default movie directory? It appears that it implements that restriction.

[IanSav]

Hi Prl,

Yes, I have that restriction because, from memory, the code tries to turn timeshift buffers into recordings by using links. (Or that is what I think used to happen or what I was going to make happen.) Now that the original intent is lost to the ravages of time I am happy to adjust the code as you suggest.

Regards,
Ian.

[prl]

Yes, I have that restriction because, from memory, the code tries to turn timeshift buffers into recordings by using links. (Or that is what I think used to happen or what I was going to make happen.) Now that the original intent is lost to the ravages of time I am happy to adjust the code as you suggest.

The timeshift save code tries to use links, and if that fails, it copies. It's been like that for a long time, since at least 2013, when the code was moved out of InfoBarGenerics. I haven't looked at the log for InfoBarGenerics to see how the timeshift code was when it was there.

One particular usage case that your code would block would be the default movie directory on a NAS with the timeshift on internal disk.

[IanSav]

Hi Prl,

The timeshift save code tries to use links, and if that fails, it copies. It's been like that for a long time, since at least 2013, when the code was moved out of InfoBarGenerics. I haven't looked at the log for InfoBarGenerics to see how the timeshift code was when it was there.

One particular usage case that your code would block would be the default movie directory on a NAS with the timeshift on internal disk.

From memory the code I refactored kept all the functionality of the original. Perhaps the requirement for links was my equivalent for trying to identify the type of file system upon which the files were stored. It really was a long time ago. Does the current code have this limitation?

Regards,
Ian.

[IanSav]

Hi Prl,

I just checked my archives and the code upon which I based my refactor has not changed. There is either a reason for the new code or there was an error in my design. Given that PeterU rejected this proposal a long time ago it never got fully tested.

I would be very interested in your suggestions to correct any issues with the current code as I hope to soon bring it into active use.

Regards,
Ian.

[prl]

I just checked my archives and the code upon which I based my refactor has not changed. There is either a reason for the new code or there was an error in my design. Given that PeterU rejected this proposal a long time ago it never got fully tested.

The current source for Screens/Timeshift.py doesn't reference config.usage.default_path, and it doesn't appear any where in the change log (git log -p) for Screens/Timeshift.py. That log goes back to April 2015.

The tests for suitability of a directory for timeshifting in the current code are:

• Directory is on a filesystem type in the file system whitelist ('ext4', 'ext3', 'ext2', 'nfs'), and
• Directory is writable

I would be very interested in your suggestions to correct any issues with the current code as I hope to soon bring it into active use.

Instead of:

Code: Select all

			tmpfile = tempfile.NamedTemporaryFile(suffix='', prefix='tmp', dir=path, delete=False)
		...
		srcname = tmpfile.name
		...
		dstname = config.usage.default_path.value + srcname[-9:]

I'd do something like:

Code: Select all

			tmpfile = tempfile.NamedTemporaryFile(suffix='.file', prefix='tmp', dir=path, delete=False)
		...
		srcname = tmpfile.name
		...
		dstname = os.path.splitext(tmpfile.name)[0] + ".link"

I'd also wondered whether it's possible for tempfile.NamedTemporaryFile() to throw exceptions other than OSError (IOError, for example). The documentation is silent on the matter.

The timeshift directory cleanup code in Components.Timeshift.InfoBarTimeshift.ptsCleanTimeshiftFolder() should probably be extended to clean up any stray test files generated by the TimeshiftSettings code, if that hasn't been done already.

[IanSav]

Hi Prl,

Your code improvement suggestions have been implemented.

The current source for Screens/Timeshift.py doesn't reference config.usage.default_path, and it doesn't appear any where in the change log (git log -p) for Screens/Timeshift.py. That log goes back to April 2015.

I don't understand this comment.

The tests for suitability of a directory for timeshifting in the current code are:

• Directory is on a filesystem type in the file system whitelist ('ext4', 'ext3', 'ext2', 'nfs'), and
• Directory is writable

I used the tests for links as a comparable test of the filesystem capabilities. NTFS should also be a valid filesystem and is acceptable to my test code.

I'd also wondered whether it's possible for tempfile.NamedTemporaryFile() to throw exceptions other than OSError (IOError, for example). The documentation is silent on the matter.

If creating a temporary file fails we are likely to be in big trouble regardless of the error condition returned.

The timeshift directory cleanup code in Components.Timeshift.InfoBarTimeshift.ptsCleanTimeshiftFolder() should probably be extended to clean up any stray test files generated by the TimeshiftSettings code, if that hasn't been done already.

The main thrust of my efforts were related to cleaning up Setup related code from a UI perspective. I haven't considered the Components part of the code.

Regards,
Ian.

[prl]

Hi Prl,

Your code improvement suggestions have been implemented.

Thanks.

The current source for Screens/Timeshift.py doesn't reference config.usage.default_path, and it doesn't appear any where in the change log (git log -p) for Screens/Timeshift.py. That log goes back to April 2015.

I don't understand this comment.

It was in reply to your:

I just checked my archives and the code upon which I based my refactor has not changed. There is either a reason for the new code or there was an error in my design. Given that PeterU rejected this proposal a long time ago it never got fully tested.

Your TimeshiftSetup code creates a test hard link from the directory in config.usage.default_path to the proposed new timeshift directory. That config variable doesn't appear to have been used in the distributed TimeshiftSetup code at any time going back to Apr 2015.

The tests for suitability of a directory for timeshifting in the current code are:

• Directory is on a filesystem type in the file system whitelist ('ext4', 'ext3', 'ext2', 'nfs'), and
• Directory is writable

I used the tests for links as a comparable test of the filesystem capabilities. NTFS should also be a valid filesystem and is acceptable to my test code.

Yes, but the original code only tested (via an incomplete whitelist) for whether links were supported on the device that contained the proposed timeshift directory. You code required both that the timeshift directory and the default movie direcory were on the same device (which was not a requirement in the original code) and that hard links were supported on that device via a proper test of the capability.

I'd also wondered whether it's possible for tempfile.NamedTemporaryFile() to throw exceptions other than OSError (IOError, for example). The documentation is silent on the matter.

If creating a temporary file fails we are likely to be in big trouble regardless of the error condition returned.

Not really - after all, you handle an OSError exception from tempfile.NamedTemporaryFile(). While the code is structured so that you'd not expect that there would be an exception (the directory exists because it was selected from the file system and the directory has already been tested for writability). I was just wondering whether the function could throw other exceptions.