4 patches for ms-sys
Brought to you by:
henca
Menu
▾
▴
#5 4 patches for ms-sys
Hi Henrik,
As the author of Rufus (https://github.com/pbatard/rufus), which relies extensivelyon ms-sys for its boot record installation, I have been maintaining my own version with some additional features and fixes, which I hope can be seen as useful.
As such, I would appreciate if they could be considered for integration into ms-sys.
Rather than attach a set of patch, and since I have my own github version of ms-sys, where I have applied the proposed set (https://github.com/pbatard/ms-sys/commits/rufus), I hope it is okay to just reference them here. The patches are:
1) Various fixes and improvements (web diff: https://github.com/pbatard/ms-sys/commit/45410b221fe34ffd858a7d67e68565b108b455ec, actual patch: https://github.com/pbatard/ms-sys/commit/45410b221fe34ffd858a7d67e68565b108b455ec.patch)
2) Experimental support for large blocks (web diff: https://github.com/pbatard/ms-sys/commit/fc773613c381c27b82191210fd99306cf3354853, actual patch: https://github.com/pbatard/ms-sys/commit/fc773613c381c27b82191210fd99306cf3354853)
3) Add support for ReactOS and Rufus boot records (web diff: https://github.com/pbatard/ms-sys/commit/3212c2f5ac1cb62c040e800d2aa518e9f672e755, actual patch: https://github.com/pbatard/ms-sys/commit/3212c2f5ac1cb62c040e800d2aa518e9f672e755.patch)
4) Add support for KolibriOS, Grub4DOS and GRUB 2 boot records (web diff: https://github.com/pbatard/ms-sys/commit/a6f5fe3c25a28a27490c44edec9f65394577b910, actual patch: https://github.com/pbatard/ms-sys/commit/a6f5fe3c25a28a27490c44edec9f65394577b910.patch).
These 4 patches should apply without issue, in the order above, on top of 2.5.1.
Now with some more remarks as to their content:
1a) I think the multiplication of fat16## and fat32## files makes maintenance a bit more difficult than it should, so I moved all the functions targeted at these FS's into a single source.
1b) The disk signature was part of the zero boot record - I don't think this should be the case
1c) FAT16 and FAT32 actually have the drive ID at a different location, so the existing write_partition_physical_disk_drive_id() was only valid for FAT32. From what I could tell, this call was only used for FAT32 anyway, but I made it more explicit that difference exists by having 2 separate calls (even if the FAT16 one is not used for now)
1d) Typos, whitespace issues (that mix of tabs and spaces is killing me with Visual Studio, but I still tried to keep the existing scheme) as well as some compilation warnings on non-Linux were addressed.
2) From what I could see, and even if I have yet to see a system that actually boots from large sectors, the current agreement for boot marker (0x55, 0xAA) is that they should be placed at every 0x200-2 in the boot sector if it is larger than 0x200 bytes. So I added a call to do just that, and used this opportunity to simplify the handling of boot markers. Now, because we can't get the sector size from the fd, I also had to add a new option to main (-B / --bps) to specify the number of bytes per sector, as well as code to ensure that it would always be between 512 and 64K bytes. Ideally, we'd also make sure it's a power of 2, but I didn't want to overcomplexify things, especially since I doubt there's any system out there that will boot MBR with something like 4K sectors...
3a) I think ReactOS is good to have. It requires an MBR (new option -0) as well as FAT16/FAT32 boot records (-o/O). ReactOS, along with its boot records, is GPL, so it is redisributable.
3b) Of course I'm a bit biased here, but I'd also like to see the dedicated MBR I install with Rufus for most Windows MBR installations to be added as well (with new option -r). Basically this MBR duplicates the "Press any key to boot from CD..." from Windows installation disc media into a "Press any key to boot from USB..." for disk media, which allows for unattended Windows installations. The problem here is that, during a Windows install, the system will reboot to perform the second part of the installation, and if you use the standard MBR, unless you unplug the drive, you will boot into media install again. With the Rufus MBR, this can be avoided, as it will boot the HDD if no key is pressed. Just like ReactOS, that MBR is GPL, and if you really want to know everything about it, you can read its primer at https://github.com/pbatard/rufus/tree/master/res/mbr (as well as its assembly source).
4a) I'm not sure about the demand for KolibriOS might be (it's a very lightweight and fast OS - http://kolibrios.org/en/), but it was easy to add in Rufus, so here it is. I couldn't be bothered to add FAT16 support for it, so only MBR (new option -k) and FAT32 (new option -K) are needed.
4b) Grub4DOS and Grub 2.0 are probably something a few more people will want. No FAT16/FAT32 needed for those, as they both boil down to an extra loader which either comes as a 'grldr' file (eg. https://rufus.akeo.ie/files/grub4dos-0.4.6a/) for Grub4DOS, that simply needs to be copied as a regular file on the first partition, or a 'core.img' sector dump (eg. https://rufus.akeo.ie/files/grub-2.02~beta2/) for GRUB 2, that usually gets copied after the boot record and before the first partition. The Grub 2 MBR installation on its own may not as helpful as the Grub4DOS one, but I think having support for both will be useful.
I appreciate that these are a lot of changes to take in, but I hope you will see them as useful.
Regards,
/Pete
Thanks alot for your new contributions! I hope I will get the time to dig into this between christmas and new year. Unfortunately, even though your patches apply clean against latest release I have some development also in CVS. There will be some conflicts against other new upcoming features, for example the switch -O is used to write OEM ID string...
1a) I agree that most source and include files are smaller than what would be perfect for easy reading and understanding of code structure. On the other hand, it does make kind of sense to have one write_*br function in each (non master) boot record file. It is also easy to follow if each such file includes its corresponding br*_0x*.h even if some of those files might get duplicated contents.
1b) I have no strong opinions myself about how a boot record should be zeroed. Maybe the first 512 bytes should be wiped (but that would erase the partition table), maybe the partition table should be kept and only 446 bytes should be zeroed as today, maybe both the partition table and the disk signature should be kept and only 440 bytes zeroed. You seem to prefer to keep the disk signature, Andrew Yeomans who once contributed the mbr_zero functionality did prefer to zero out also the disk signature. I would prefer to get an OK from Andrew before changing the functionality he provided. But my attempt doing so would be to reply to an email which is more than 10 years old. Before I would try that, do you really think it is that important to keep the disk signature?
1c) Thanks for the fat16 disk ID code! I will consider to add that code and to add a call to the code. I will probably not add the code unless I make a call to it as I don't like to have dead unused code among the sources.
1d) Thanks for the cleanup! I also prefer to have spaces only as indentation in my code and I avoid rows longer than 80 chars. However, as I didn't write all code myself other ways to indent might have gotten in the code. The problem now is that I can't apply your patches on the current code. Some kind of manual work will be needed and maybe some of those whitespace cleanups might get hard to spot then.
2) Thanks for the large sector functionality!
3) Thanks for ReactOS and Rufus boot records! I might change the ms-sys switches for ReactOS as -O now is used for writing OEM ID.
4) Thanks for KolibriOS and Grub boot records!
I appreciate your contributions and I hope I will get the time to get a new release out.
regards Henrik
Hi Henrik,
Thanks for reviewing the patches. Here are my comments on your points:
1a) Well, it's your repository, but as far as I'm concerned, when integrating a lot of code from external projects, as I do in Rufus, it makes life A LOT easier not to have a myriad of sources with only a handful line of codes in them. It also makes troubleshooting/debugging easier, when you don't have to have 10 awfully similar files open in the IDE. And avoiding duplicated code is something one learns the hard way, the day they find they have to modify the same duplicated data 10 times, and have introduced a hard to debug typo in one of the duplicates...
Again, it's up to you, but this divergence of source organization will definitely make my life more difficult when trying to push ms-sys patches your way, as I am not planning to split all of these back again in my source...
1b) I think not having the signature is important for detection of whether the drive has a zero MBR. IMO, a drive that has zeros up to the signature should be reported as having a zeroed MBR. This is actually why I removed the signature, as I found that, otherwise, I would almost never be able to detect zeroed MBRs on Windows...
1c) I can live with that. I don't use the fat16 part either, so if you remove it, I will follow suit.
1d) I'm not sure why you can't apply the cleanup. Do you have additional changes that are not yet in the public repo? Or did I miss something in my branch so that it can apply cleanly? I though I had rebased on the latest, but if that's not the case, let me know, as I may be able to rework these patches so that you don't have to rework them.
2) I will stress out again that it's very experimental. In Rufus, I've only been using to try large sector support with Syslinux... but I still have to find a system where that seemed to work.
3) Feel free to change the switches. Obviously, I don't use any of these parameters in Rufus, so changing these letters is no issue. I would advise you to look into using GNU getopt to parse options at some stage. It's only a couple of files (e.g. https://github.com/pbatard/rufus/tree/master/src/getopt) and it makes handling of parameters a lot easier.
Regards,
/Pete
Thanks for your clarifications! Some more questions/comments:
1a) I don't see those duplicated lines in br_*_0x*.h as code, I would rather see those lines as data. If some of those files have identical contents it is because some boot records look identical, at least in part. There is no need to debug those files as they contain no code being executed by ms-sys.
1b) From your last comment I get the impression that you saw a need to zero out some more bytes? But from your patch it seems as if you needed to zero out less bytes, leaving the disk signature as is also on a zeroed mbr?
1c) I hope I will add a call to your new code.
1d) My latest changes are in the public repo,
https://sourceforge.net/p/ms-sys/code/ has instructions on how to download a copy using CVS. There is also a web based repository viewer at
http://ms-sys.cvs.sourceforge.net/viewvc/ms-sys/
You did rebase from the latest released version, but since that release I have started working on a new requested feature. Because of my new work your patches does not apply cleanly and there will be some conflicts on how parameter switches are used.
2) It is OK to have some experimental functionality. Also the next version of ms-sys will be a development version.
3) Thanks for your pointer to gnu getopt, one day I might start using that in ms-sys.
regards Henrik
1a) I was talking about the splitting of these various similar calls in fat32##.h/.c. Someone might be troubleshooting or modifying these calls. The only reason I also mentioned duplicated data, which is always a bad idea, even if it's zeroed data (forn instance, how about you suddenly find that you don't need to include as much zeroed data, as in the case of the zeroed MBR. Now you have to modify AND VALIDATE tens of locations in your code, instead of just one), is that you also mentioned that in your comment as well.
1b) You are misreading my comment. I have seen other applications, that will zero an MBR and then apply a disk signature, or just apply a disk signature regardless of the MBR, which may have been zeroed before. It also make sense to consider that while a partitioning software may not change the lower MBR data, it may introduce a singnature. In that case, ms-sys will wrongfully report that what is essentially a zeroed MBR isn't a zeroed MBR. So, what I am saying is: logically, the signature should be left out of the zeored MBR always, so as not to change the disk signature if you write to it, and properly detect a zeroed MBR if you read it.
1d) I will rebase my changes against the latest from CVS, so that the patches apply cleanly. With git, rebasing is usually very easy to do. I will post links to the new patches here once I'm done.
Here are the new patches rebased on latest CVS. The new branch is at https://github.com/pbatard/ms-sys/commits/rufus2
1) https://github.com/pbatard/ms-sys/commit/2cd8b8d72471564c41d6897b2817fe436546b164.patch
2) https://github.com/pbatard/ms-sys/commit/3d34fba4058e22193f6d48319fa25162a8037a31.patch
3) https://github.com/pbatard/ms-sys/commit/f46bdcb649328c0367dc6ac1905bd3662c738ea9.patch
4) https://github.com/pbatard/ms-sys/commit/78bf57493f4879c091acb273aeb6157a498ac036.patch
NB: If you want the webdiff, just remove the '.patch', and if you want an actual diff, just replace '.patch' with '.diff'.
Thanks for your new patches which did apply cleanly on latest CVS! I haven't yet commited those patches to CVS, first I want to:
a) Check with Andrew Yeomans what he thinks about zeroed MBRs.
b) Rearrange parameter switches. Right now -O is used both for writing OEM ID and FAT32 ReactOS. Previously, I have also used capital letters only for switches which require a parameter. Maybe that luxury can no longer be affored if we are running out of lowercase letters.
c) I might also rewrite the global variable ulBytesPerSector to become a static file global only variable in br.c. If so I will write some kind of set- and maybe also a get-function of that variable. I don't like global variables, but I prefer a file global variable instead of a program global variable.
regards Henrik
I have now discussed the switch -z with Andrew and added some new functionality that I hope will make ms-sys the most useful for everyone.
Some examples:
The switch -f is used in my examples above only because I play with a file instead of a real disk device.
Pete, do you think this is OK when it comes to -z and Windows Disk Signature?
If you want to play with this new functionality it has been checked in to CVS.
If you think this is OK I will start working on updating man pages and translation files, then a new release will be made.
regards Henrik
Hi Henrik,
Thanks for applying the patches. I've fetched the latest from CVS and been starting working on integrating these changes in Rufus. So far things look good, and I'm happy that it should help keep both your master codebase and the one I use in Rufus a lot more in sync.
I have no issues with having -z and -S switch for zeroing/writing the disk signature.
However, one function I am really interested in having is a call that will tell me if an MBR is zeroed up, excluding the disk signature, and I don't think the newly introduced is_zero_mbr_with_other_windows_disk_signature() call fits that role, because the first part will fail if the signature is also zeroed). Either that or I'm not sure I understand the logic for that function.
With the current source, the only way I see to tell if an MBR is zeroed up to the signature is to call both is_zero_mbr() and is_zero_mbr_with_other_windows_disk_signature() which isn't really convenient, even more so as it requires to read the same MBR 4 times behind the scenes (!). Granted, with caching and all, this may not be much of an issue, but I can't help but feel that the inclusion of the disk signature in the MBR makes the whole thing convoluted and hard to understand for anyone tasked to maintain the code, or even make sense of what is really trying to be accomplished here.
If it was me, I'd have set mbr_zero_0x0 to be only the first 440 bytes (i.e. disk signature excluded) and have used something like this:
Then you can use these 2 very straightforward calls in any operation where you need to find out which exactly of the MBR and the disk signature are zeroed out. If needed, you could even add something like:
To give you the equivalent of your current is_zero_mbr().
At this stage, i think I will still go with a 440 bytes zeroed MBR and my own is_zero_mbr() in Rufus, rather than use the latest ms-sys, because it just doesn't make sense to me to have to read the MBR 4 times to figure out if the least amount of data I need is zeroed.
This being said, thanks for integrating my patches - much appreciated!
Thanks for taking your time to review this!
I was hoping that the logic of is_zero_mbr_with_other_windows_disk_signature() would be clear from its function name. It returns true if and only if the MBR is zeroed like is_zero_mbr() but the windows disk signature has some other value than zero.
So if you don't care about the Windows disk signature but want to know if only the first 440 bytes are zeroed you can call (is_zero_mbr_with_other_windows_disk_signature() || is_zero_mbr()).
The fact that is_zero_mbr_with_other_windows_disk_signature() reads the MBR more than once is no problem when in comes to performance. In fact, ms-sys doing a diagnosis today reads the MBR at least once for every known BR. Howver, as those reads are done using fread they will most likely be buffered meaning that we only copy and compare rather small amounts of RAM.
Your proposed is_zero_mbr_excluding_disk_signature() and is_zero_disk_signature() will not give very much gain when it comes to performance and some more function would be needed to also test the bytes between the Windows Disk Signature and the partition table.
If you in your code still prefer to write only 440 bytes when zeroing an MBR, please feel free to do so, but I prefer to by default also zeroing the Windows Disk Signature and also also zeroing the bytes between the Windows Disk Signature and the partition table. I also prefer to leave the functionality of the existing switch -z as is, that to make sure not breaking any script out there relying on the current implementation of write_zero_mbr in ms-sys.
Unless you have any big objections I feel rather ready to prepare the next relase. Updating the man-pages will be rather quick, unfortunately updating the translation files usually needs a few hours. I can't say for sure when I will get that time, I only know it will not be within the next weekend.
Last edit: Henrik Carlqvist 2016-01-06
That's what I assumed I would need to do, and what I am unhappy with.
My problem is that, if you do that, what happens behind the scenes is that if the signature is not zero, contains_data(fp, 0x0, mbr_zero_0x0, sizeof(mbr_zero_0x0)) will always be called twice (and the MBR may be read up to 4 times). This just to test whether a continuous block of data is zero. Irrespective of performance, this just doesn't sound elegant, and wouldn't be something something I'd want to see anywhere in well maintained source.
I did specifcally mention that, with caching, performance was not an issue so my comment was never about performance. Rather, it was about improving the code logic and maintainability for whoever is going to handle ms-sys source, by making sure things are logically broken down as they should.
Also, I am not asking you to change the -z option at all. I thought it was implicit that what the new breakdown would do, was split things behind the scenes, for logical coherence, but without changing the user-facing functionality one. So all I am saying is that you could easily achieve what you need with much easier to read functions, that are split to deal with the actual lowest number of bytes one needs, for each of the logical parts to test, and that you can then combined, to test for a larger amount of data where needed. This is even more true as the application itself is definitely trying to process them as separate entities (so that it can report the signature). So I just don't get why one wouldn't always want to split the disk signature from the rest of the MBR payload...
According to Wikipedia, this is actually part of the extended disk signature (the complete disk signature is 6 bytes, not 4), and these may not always be zero if the MBR is write protected. I don't think I've ever seen that applied in practice, but I haven't looked at it that closely. But even then, adding a logically split call, that specifically handles the write protection marker sounds to me as a more maintainable thing to do then just consider that this goes with the restof the payload.
At the very least, could I please ask you to add an is_zero_mbr_something() function (suffix doesn't matter) that only checks if the first 0x1b8 bytes are zeroed? This is really all I need to be able to use br.c/.h as is in my app, rather than rewrite these files. If you did that, I think I can overlook the logical breakdown of the rest of the is_zero_mbr_XXXX calls...
Ok, I will add a function named something like is_zero_mbr_not_including_disk_signature_or_copy_protect. I hope to get the time to commit that function to CVS within a few weeks. Maybe I will also add some function to check whether the copy protect bytes are 5A5A.
Sounds good. I went ahead and applied the latest from ms-sys onto my internal version, and added the proposed is_zero_mbr_not_including_disk_signature_or_copy_protect().
If it helps, this is how I modified br.c/.h for that last part:
https://github.com/pbatard/rufus/blob/b77561bb9bbda5263c8cf031a9455b72eee52f89/src/ms-sys/br.c#L203-L209
https://github.com/pbatard/rufus/blob/b77561bb9bbda5263c8cf031a9455b72eee52f89/src/ms-sys/inc/br.h#L81
Your is_zero_mbr_not_including_disk_signature_or_copy_protect() has now been commited to CVS. As I don't like having unused (and thereby also untested) code among my sources I have also replaced the previous call to the now removed function is_zero_mbr_with_other_windows_disk_signature() with a call to your function.
To not miss if the copy protect bytes are non zero I have also added new functions read_mbr_copy_protect_bytes() and read_mbr_copy_protect_bytes_explained(). At the time of this writing those new functions are only used when detecting an "almost zero-mbr". Maybe they will be more used in the future.
Version 2.5.2 of ms-sys is now released!
regards Henrik