GNOME Bugzilla – Bug 730640
Add initrd injection support for debian based express installation
Last modified: 2016-03-31 13:22:07 UTC
These patches add the initrd injection support. This has the following advantages: a) If libosinfo gets some desktop installation scripts now, we are able to use them b) If libosinfo gets these scripts without these patches they would be treated like any other express installation script and the user would wonder why it does not work. (Before this every desktop script is just inserted via floppy.) I tested them with a debian (XFCE) iso as well as an ubuntu 12.04 alternate iso (which needs other kernel and initrd paths than the ones given in libosinfo but that's an own issue). This is my first patch series, I hope I got it right. Zeenix: I stripped away unneeded functions from my libarchive wrapper.
Created attachment 277065 [details] [review] unattended-installer: Move boot file extraction This moves the extraction of the boot files to be before the injection of the automated installation scripts. This is necessary since in some cases these scripts will be placed within these boot files.
Created attachment 277066 [details] [review] Add libarchive dependency Libarchive will be used for archive handling in the future.
Created attachment 277067 [details] [review] Add archive-utils for archive handling
Created attachment 277068 [details] [review] Add wrapper for libarchives Archive.Write
Created attachment 277069 [details] [review] Add wrapper for libarchives Archive.Read Be aware that this is merely a stripped version containing only the code needed by Boxes.
Created attachment 277070 [details] [review] Add archivist for easy archive handling This is merely a stripped version containing only the code needed by Boxes.
Created attachment 277071 [details] [review] unattended-file: Handle injection methods Prior to this, injection methods were just ignored, meaning that every desktop express installation script would be treated like a Fedora kickstart script. This will now be differentiated. This also adds the ability to do initrd injections e.g. for debian systems.
Note: This patch isn't really good testable until libosinfo gets desktop installation scripts for an initrd injection based distribution. I'll do this soon and put another comment here when it's ready.
With the patch posted here: https://www.redhat.com/archives/libosinfo/2014-May/msg00061.html on libosinfo, the debian express installation is testable. 1. Apply the patches from this bug to Boxes 2. Apply the patch from the libosinfo link above to libosinfo 3. Recompile everything 4. The normal express installation process should work with debian wheezy isos Note: * you need to have an internet connection or there may be questions asked * as DE gnome will be installed independently from the ISO you use
I forgot one thing: The default disk space settings are too low for a debian installation. I don't know the exact size, but I would recommend to take 20 GiB or so. (I got an error with 10.)
(In reply to comment #9) > Note: > * you need to have an internet connection or there may be questions asked Any idea why (what kind of questions are they?) and if there's a way to avoid that? > I don't know the exact size, but I would recommend to take 20 GiB or so. (I got > an error with 10.) Hmm 10GB already seems like quite a bit of space, it's a bit surprising that it's not enough. https://www.debian.org/releases/stable/amd64/ch03s04.html lists some smaller figures
(In reply to comment #11) > (In reply to comment #9) > > Note: > > * you need to have an internet connection or there may be questions asked > > Any idea why (what kind of questions are they?) and if there's a way to avoid > that? I think it is possible to avoid it. This requires more testing which I am currently doing. > > I don't know the exact size, but I would recommend to take 20 GiB or so. (I got > > an error with 10.) > > Hmm 10GB already seems like quite a bit of space, it's a bit surprising that > it's not enough. https://www.debian.org/releases/stable/amd64/ch03s04.html > lists some smaller figures The error I got was "not enough disk space" in the log when using it with about 10 GiB. I think this is pretty clear, maybe the debian documentation is a bit old?
Review of attachment 277065 [details] [review]: Patch is good but log needs improvements. Shortlog doesn't mean anything at all. Move it where? Another file? Another function? I don't think the 'move' part is important here so I'll suggest: Extract boot files before script injection. I'd suggest adding '(e.g Debian-based OSs) after 'cases' in the log.
Review of attachment 277066 [details] [review]: Again, patch is good but log could be better: * 'Add libarchive dependency' => "Require libarchive 3.0.0" IMHO * 'in the future' is very vague, better be more specific: We'll need this in the following patches where we add support for express installation of OSs that require installation script to be injected in the initrd file.
Review of attachment 277067 [details] [review]: If you want to keep this patch separate, the natural order for it would be after adding the archive handling API.
Review of attachment 277067 [details] [review]: The utils are used by the classes of the following commits meaning the build would fail if this is not applied first.
Review of attachment 277068 [details] [review]: * If this patch depends on the new vapi, you either need to bump vala dependency or include modified vapi in a patch that comes *before* this patch. * 'libarchives ' is redundant in log I think but if you disagree, you want to write: libarchive's rather than libarchives. * A bit more info in the log would be highly recommended. ::: src/archive-writer.vala @@ +5,3 @@ + private Archive.Write archive; + + public ArchiveWriter.to_file (string filename, I think there is no reason to name this contruction method. It can be just the main construction method that takes filename. Besides it seems ArchiveWriter.from_raw_read_archive also takes a filename. @@ +7,3 @@ + public ArchiveWriter.to_file (string filename, + Archive.Format format, + GLib.List<Archive.Filter>? filters = null) * I couldn't find any reference to Archive.Filter on valadocs.org nor could i find any 'fitler' reference in libarchive manpage. What do they do? * Do we really need filters in Boxes? @@ +8,3 @@ + Archive.Format format, + GLib.List<Archive.Filter>? filters = null) + throws Util.ArchiveError { * alignment is one space off. * I pointed out in my first review that coding style is to align the arguments' identifiers too. @@ +21,3 @@ + archive = new Archive.Write (); + if (read_archive.next_header (out iterator) != Archive.Result.OK) { + // its empty or something went wrong - throw exception '- throw exception' part is redundant here. Its pretty obvious we are throwing exception below. @@ +30,3 @@ + + do { + bool omit = false; Use 'var' where possible, especially if type is obvious to user from initialization. @@ +38,3 @@ + } + + if (omit == false) { Booleans don't need comparisons. Use '!omit' here. @@ +42,3 @@ + var buf = new uint8[len]; + insert_entry (iterator); + if (len > 0) This should be checked before creating entry and buffer. I'd do: if (len <= 0) continue; @@ +45,3 @@ + insert_data (buf, read_archive.read_data (buf, (size_t) len)); + } else { + debug ("Omitting file '%s' on archive recreation.", iterator.pathname ()); * code is more readable when there is less nesting. I'd write this logic as: if (omit) { debug (.. continue; } // Code here that assume no ommiting. * Debug sentence seems gramatically incorrect. How about "Not packing '%s' in archive '%s.'? @@ +52,3 @@ + private void prepare_archive (Archive.Format format, GLib.List<Archive.Filter>? filters = null) + throws Util.ArchiveError { + if (archive.set_format (format) != Archive.Result.OK) { RETRY status needs special handling here as pointed out below. @@ +69,3 @@ + + ~WriteArchive () { + archive.close (); Have you checked if this is really needed and its not implicit in losing last reference to it? @@ +91,3 @@ + insert_data ((uint8[]) buf, len); + } catch (GLib.Error e) { + throw new Util.ArchiveError.FILE_OPERATION_ERROR ("Error reading from source file '%s'. Message: '%s'.", I don't think there is any need to translate all errors into one error. Even if there is, GIO already has many error codes under GIOError that are appropriate for most I/O errors. @@ +97,3 @@ + + public void add_filters (GLib.List<Archive.Filter> filters) throws Util.ArchiveError { + stdout.printf ("Adding filters...\n"); We don't want printf's in a GUI app. :) All output is either debug, warning, error or message. @@ +107,3 @@ + + private void insert_entry (Archive.Entry entry) throws Util.ArchiveError { + // write header Typical case of over-documentation: The function says write_header so this comment doesn't add anything for reader. @@ +109,3 @@ + // write header + if (archive.write_header (entry) != Archive.Result.OK) + throw new Util.ArchiveError.FILE_OPERATION_ERROR ("Failed writing header to archive. Message: '%s'.", * G_IO_ERROR_FAILED would do fine for Archive.Result.FAILED :) * You are translating RETRY to an error too. The docs say the operation might succeed if retried. @@ +114,3 @@ + + private void insert_data (void* data, int64 len) throws Util.ArchiveError { + // write data another redundant comment.
Review of attachment 277067 [details] [review]: Ah ok but I don't think these belong in a separate file. Each module can define it's own error domain, even if redudant. Besides (see my review on Writer patch) I don't think we need new error codes. I need to see the usage of Util.ArchiveAccess to be sure but I have a feeling this is not needed either
Review of attachment 277069 [details] [review]: * The warning in the log is redundant. The code is going into Boxes as part of it so it just makes sense not to add any API that is not needed by Boxes. Besides you don't mention 'stripped down version' of what? As you saw in Bastien's blog, we could use a GIO/Vala friendly library to handle all this for us but that will have to be in C, unless you want to fight the whole world for rest of your life. :) * I'd think its natural to first add reading API rather than writing. :) * Not a useful API since it doesn't seem to provide a way to extract files from archive. I'd imagine that to be the main functionality of this class and therefore part of this patch. * Some (if not many) comments I made on Writer patch apply here. ::: src/archive-reader.vala @@ +2,3 @@ + +class Util.ArchiveReader : GLib.Object { + // This is the example block size from the libarchive website IMHO better write: This is the block size used by example code on libarchive website @@ +7,3 @@ + private string filename; + private Archive.Format? format = null; + private GLib.List<Archive.Filter>? filters = null; Hmm.. so in case of writing, the above two are passed around as params but here they are kept as instance members? Rationale? :)
Review of attachment 277068 [details] [review]: > * If this patch depends on the new vapi, you either need to bump vala dependency or include modified vapi in a patch that comes *before* this patch. The new vapi is already in the vala directories. I assume I need to "bump vala dependency" even though I don't quite understand what you mean with that. > * 'libarchives ' is redundant in log I think but if you disagree, you want to write: libarchive's rather than libarchives. No, thats ok with me. > * A bit more info in the log would be highly recommended. No problem. ::: src/archive-writer.vala @@ +5,3 @@ + private Archive.Write archive; + + public ArchiveWriter.to_file (string filename, Speaking of the code needed for Boxes _right_now_ this may be right. The thought behind is generality: libarchive supports some archive types that write to memory and such things. Keeping this in mind I wanted the developer to consciously choose the right constructor. @@ +7,3 @@ + public ArchiveWriter.to_file (string filename, + Archive.Format format, + GLib.List<Archive.Filter>? filters = null) Take a look at http://valadoc.org/#!api=libarchive/Archive.Compression Filters are the same but better. Archive.Compression is deprecated since libarchive 3; I did update the vapi so that there are these Filters. (Valadoc is out of date.) @@ +8,3 @@ + Archive.Format format, + GLib.List<Archive.Filter>? filters = null) + throws Util.ArchiveError { Sorry, forgot this. @@ +21,3 @@ + archive = new Archive.Write (); + if (read_archive.next_header (out iterator) != Archive.Result.OK) { + // its empty or something went wrong - throw exception Correct. @@ +30,3 @@ + + do { + bool omit = false; Ok. @@ +38,3 @@ + } + + if (omit == false) { Sounds better indeed. @@ +42,3 @@ + var buf = new uint8[len]; + insert_entry (iterator); + if (len > 0) Wrong. The hardlinks only have an header but no body (so len is 0). So the entry needs to be written! However the allocation should indeed be shifted into the condition. @@ +45,3 @@ + insert_data (buf, read_archive.read_data (buf, (size_t) len)); + } else { + debug ("Omitting file '%s' on archive recreation.", iterator.pathname ()); I like my sentence better (omit vs. not packing - latter sounds ugly in my eyes) but its your choice. @@ +69,3 @@ + + ~WriteArchive () { + archive.close (); It's implicit. So given that vala is deterministic one could indeed omit this, it just >feels< wrong ;) I'll remove it. @@ +91,3 @@ + insert_data ((uint8[]) buf, len); + } catch (GLib.Error e) { + throw new Util.ArchiveError.FILE_OPERATION_ERROR ("Error reading from source file '%s'. Message: '%s'.", Okay, I'll spend some time transforming the errors. @@ +97,3 @@ + + public void add_filters (GLib.List<Archive.Filter> filters) throws Util.ArchiveError { + stdout.printf ("Adding filters...\n"); Sorry, forgot transformation. @@ +107,3 @@ + + private void insert_entry (Archive.Entry entry) throws Util.ArchiveError { + // write header Correct. (In a previous version of the wrapper that was not so easy to see.) @@ +109,3 @@ + // write header + if (archive.write_header (entry) != Archive.Result.OK) + throw new Util.ArchiveError.FILE_OPERATION_ERROR ("Failed writing header to archive. Message: '%s'.", As above. Ok. @@ +114,3 @@ + + private void insert_data (void* data, int64 len) throws Util.ArchiveError { + // write data Like above.
Review of attachment 277069 [details] [review]: > * The warning in the log is redundant. The code is going into Boxes as part of it so it just makes sense not to add any API that is not needed by Boxes. Besides you don't mention 'stripped down version' of what? As you saw in Bastien's blog, we could use a GIO/Vala friendly library to handle all this for us but that will have to be in C, unless you want to fight the whole world for rest of your life. :) Ok- > * I'd think its natural to first add reading API rather than writing. :) Sounds good on first sight. But if you look deeper you see that the read API depends on the write API. So if you add read first the commit is not compilable. > * Not a useful API since it doesn't seem to provide a way to extract files from archive. I'd imagine that to be the main functionality of this class and therefore part of this patch. I stripped this away since it is not needed. You said you don't want to have any unneeded code. However we'll need it when replacing the iso-read dependency with libarchive when the libosinfo patch is through. > * Some (if not many) comments I made on Writer patch apply here. I'll look into it. ::: src/archive-reader.vala @@ +2,3 @@ + +class Util.ArchiveReader : GLib.Object { + // This is the example block size from the libarchive website Ok. @@ +7,3 @@ + private string filename; + private Archive.Format? format = null; + private GLib.List<Archive.Filter>? filters = null; None :) Thats why we have reviews.
Review of attachment 277070 [details] [review]: Some of the comments I made in previous reviews apply here too. ::: src/archivist.vala @@ +4,3 @@ + + +class Util.Archivist : GLib.Object { * Thats a weird name. * As I pointed out before, the namespace should be Boxes here, unless its a small utility class that can be put in Utils module. * Seems like ArchiveReader doesn't actually provide reading functionality but leaves it all to this class that doesn't even derive from it (and it can't as it also abstracts ArchiveWriter). I think thats not a good API. If you move reading to Reader (as you should), there isn't much this class actually provides. I also see that the constuction method of this class is complicated and looking at all this, I think this class only makes API and code complicated. Lets just have a class for reading and a class for writing to archives. Another idea would be to move both reading and writing code here and just have one class deal with both but I don't like that idea and I would imagine you wouldn't either. @@ +21,3 @@ + GLib.List<Archive.Filter>? filters = null) + throws Util.ArchiveError + requires ((access & 0x3) != 0) You want to use the enum you defined here rather than hardcoding the value. :) @@ +66,3 @@ + public bool writable () { + return (access & ArchiveAccess.WRITE) != 0; + } These methods will be redundant if don't have this class.
Review of attachment 277070 [details] [review]: ::: src/archivist.vala @@ +4,3 @@ + + +class Util.Archivist : GLib.Object { > * Thats a weird name. I had this name in the first proposal and asked about anyone complaining because I didn't like it. Then I changed it to RWArchive and you complained about my class names so I changed it back - I am open for proposals. > * As I pointed out before, the namespace should be Boxes here, unless its a small utility class that can be put in Utils module. Quote from https://bugzilla.gnome.org/show_bug.cgi?id=729655#c7 > > * When you move the wrapper to Boxes, I assume the namespace will be changed > > from Utils to Boxes? > > It seemed to me like a general utility, so Utils did make sense for me. However > it doesn't make that difference so I'll go with your preference. We do have a 'Util' namespace so you can use that instead. (End quote) * Seems like ArchiveReader doesn't actually provide reading functionality but leaves it all to this class that doesn't even derive from it (and it can't as it also abstracts ArchiveWriter). I think thats not a good API. If you move reading to Reader (as you should), there isn't much this class actually provides. 1. I'll add the reading functionality shortly since we decided in Bug#720741 that we'll need this. The required libosinfo patch is already submitted on the mailing list. 2. This class provides abstraction. You can open an archive and then write to it as if it were modifyable - which it isn't. @@ +21,3 @@ + GLib.List<Archive.Filter>? filters = null) + throws Util.ArchiveError + requires ((access & 0x3) != 0) Correct.
Review of attachment 277071 [details] [review]: * Fedora -> All OSs other than Debian-based ones :) * I think more important would be to specify which injection method(s) was assumed. * Lets have a separate patch for: * Add handling of injection method * Handle initrd injection method ::: src/unattended-file.vala @@ +44,3 @@ public string dest_name { get; set; } public string src_path { get; set; } + public string? initrd_path { get; set; default = null; } * this should be passed at construction time. * No reason for specifying default value if its null. @@ +55,3 @@ + this.installer = installer; + this.script = script; + this.dest_name = script.get_expected_filename (); * Removal of dest_name argument and moving its initialization here should be split into a separate patch. * Also if we are going to directly get it from script when needed, there is no need to keep it in the instance and fetching this already. @@ +60,3 @@ + + public async virtual void copy (Cancellable? cancellable) throws GLib.Error { + if ( (this.injection_methods & InstallScriptInjectionMethod.FLOPPY) != 0 ) { * redundant spaces here: '( (' and '0 )'. * Vala has a pretty syntax for this: InstallScriptInjectionMethod.FLOPPY in this.injection_methods. :) * this code (should) handles 'disk' method and in fact, in case of Fedora, its the 'disk' method we use. @@ +76,3 @@ + var archive = new Util.Archivist.from_file (initrd_path, Util.ArchiveAccess.READWRITE); + archive.insert_file (source_file.get_path (), dest_name); + archive = null; this is redundant. Vala will unref the object once its out of scope. @@ +80,3 @@ + debug ("Copied unattended file '%s' into initrd '%s'", dest_name, initrd_path); + } else { + throw new GLib.OptionError.UNKNOWN_OPTION ("Unknown injection method!"); * this is a programmer error. * Its not a case of 'unknown method' but more like 'no supported injection method available'.
Review of attachment 277071 [details] [review]: ::: src/unattended-file.vala @@ +55,3 @@ + this.installer = installer; + this.script = script; + this.dest_name = script.get_expected_filename (); > * Removal of dest_name argument and moving its initialization here should be split into a separate patch. Ok. > * Also if we are going to directly get it from script when needed, there is no need to keep it in the instance and fetching this already. I have something in mind that makes this more modular, lets take a look at this issue in the next patch iteration. The copy method in general is a bit dirty in this patch since it duplicates some code, I have a solution for that now. @@ +60,3 @@ + + public async virtual void copy (Cancellable? cancellable) throws GLib.Error { + if ( (this.injection_methods & InstallScriptInjectionMethod.FLOPPY) != 0 ) { > * redundant spaces here: '( (' and '0 )'. Yes, decreases readability for me but its your style and I sould've obeyed it. > * Vala has a pretty syntax for this: InstallScriptInjectionMethod.FLOPPY in this.injection_methods. :) That's great! Thanks :) > * this code (should) handles 'disk' method and in fact, in case of Fedora, its the 'disk' method we use. Ok. @@ +76,3 @@ + var archive = new Util.Archivist.from_file (initrd_path, Util.ArchiveAccess.READWRITE); + archive.insert_file (source_file.get_path (), dest_name); + archive = null; The debug message in line 80 is wrong without this statement. @@ +80,3 @@ + debug ("Copied unattended file '%s' into initrd '%s'", dest_name, initrd_path); + } else { + throw new GLib.OptionError.UNKNOWN_OPTION ("Unknown injection method!"); OK, I'll make it an assertion.
Review of attachment 277069 [details] [review]: >> * I'd think its natural to first add reading API rather than writing. :) >Sounds good on first sight. But if you look deeper you see that the read API depends on the write API. So if you add read first the commit is not compilable. Thats because you wrote it this way. :) 28 create_writable (string filename, string[] omit_files) throws Util.ArchiveError { 29 return new ArchiveWriter.from_raw_read_archive (archive, filename, omit_files); 30 } Firstly this is not a very useful API as its a very slim wrapper around ArchiveWriter constructor. If this is needed, you either want to improve the ArchiveWriter constructor or add another one that does exactly what this method is doing. >> * Not a useful API since it doesn't seem to provide a way to extract files from archive. I'd imagine that to be the main functionality of this class and therefore part of this patch. >I stripped this away since it is not needed. You said you don't want to have any unneeded code. And you put the code into another class and that is not what I meant. :) > However we'll need it when replacing the iso-read dependency with libarchive when the libosinfo >patch is through. I have a better idea. See my comment on Archivist patch.
Review of attachment 277070 [details] [review]: ::: src/archivist.vala @@ +4,3 @@ + + +class Util.Archivist : GLib.Object { >> * Thats a weird name. >I had this name in the first proposal and asked about anyone complaining because I didn't like it. Then I changed it to RWArchive and you complained about my class names so. I recall "complaining" about ReadArchive and WriteArchive, not RWArchive. RWArchive is much better but I think us not coming up with a good name for it is another hint that this class really shouldn't exist. >We do have a 'Util' namespace so you can use that instead. (End quote) I don't know what you mean here. My comment still stands. >>* Seems like ArchiveReader doesn't actually provide reading functionality but leaves it all to this class that doesn't even derive from it (and it can't as it also abstracts ArchiveWriter). I think thats not a good API. If you move reading to Reader (as you should), there isn't much this class actually provides. >1. I'll add the reading functionality shortly since we decided in Bug#720741 that we'll need this. The required libosinfo patch is already submitted on the mailing list. I don't think this is relevant to what I wrote above. >>2. This class provides abstraction. You can open an archive and then write to it as if it were modifyable - which it isn't. You can always create abstractions over abstractions (, ...) but you need to make sure its actually useful. Seems to me that its not the case here. If you had managed to hide ArchiveReader and ArchiverWriter API behind this, I could have been convinced but thats not the case either. You want to read files from an archive, sure ArchiveReader is your friend. You want to write to an archive, ArchiveWriter is then your friend. You can be friends with both too if you like but there is no need for to introduce a third friend just to deal wit these two friends for you. :)
Review of attachment 277071 [details] [review]: ::: src/unattended-file.vala @@ +76,3 @@ + var archive = new Util.Archivist.from_file (initrd_path, Util.ArchiveAccess.READWRITE); + archive.insert_file (source_file.get_path (), dest_name); + archive = null; you mean the file hasn't actually been copied? insert_file should ensure that or you need another method for that here.
Review of attachment 277071 [details] [review]: ::: src/unattended-file.vala @@ +76,3 @@ + var archive = new Util.Archivist.from_file (initrd_path, Util.ArchiveAccess.READWRITE); + archive.insert_file (source_file.get_path (), dest_name); + archive = null; The file has been copied but the archive has not been closed so it is not a valid archive at this point. After testing the wrapper with Ubuntu I realized that an inserted file may replace an existing file. This requires the Archivist to be changed a bit so that every inserted file will be omitted when the ArchiveWriter is created out of the Archive.Read object. This implies that this is done on destruction, after all files are inserted. This is also necessary to deploy some deleteFile() methods (which would have to add the filename to the omittion list). You really dont want all these abstractions in a class? I don't think it's good to throw away the archivist because that makes things more complicated for the users (that have to user two objects [ArchiveReader and ArchiveWriter] and worry about all these problems related to non modifyable archives). This object improves code understanding, readability (a lot I think) and code reusability without harming anyone. When I read this debug message I do expect there to be a valid archive.
Review of attachment 277071 [details] [review]: ::: src/unattended-file.vala @@ +76,3 @@ + var archive = new Util.Archivist.from_file (initrd_path, Util.ArchiveAccess.READWRITE); + archive.insert_file (source_file.get_path (), dest_name); + archive = null; * You put answer to two different comments in here. :( > The file has been copied but the archive has not been closed so it is not a valid archive at this point. Ah ok, the debug statement doesn't exactly have to be 100% accurate. :) You can move it outside the try block btw (the throw in 'else' will ensure that its not executed in case of error if you want it to be 100% accurate. About the 'inserted file may replace an existing file.', I'm afraid I couldn't quite understand how that is going to be done but when we need that code, we can talk about abstractions that will be good for that. No need to create a complete class now for that. I'm talking about Archivist as it is right now in your patch and as it is now, it does not provide any useful functions. About problems related to modifiablility, that can and should be dealt by the actual class that deals with writing archives. The interface would be pretty simple with just ArchiveReader and ArchiveWriter already. There is no usecase where using code needs to do archive handling and it doesn't know if its reading or writing. It always knows that so you are not going to be actually abstracting them from anything with this class. @@ +80,3 @@ + debug ("Copied unattended file '%s' into initrd '%s'", dest_name, initrd_path); + } else { + throw new GLib.OptionError.UNKNOWN_OPTION ("Unknown injection method!"); So sorry, I think you were correct here. We could easily hit this in future and its not programmer error. However I'd prefer the message I wrote in the previous comment.
Created attachment 277644 [details] [review] unattended-installer: Extract boot files before injection This moves the extraction of the boot files to be before the injection of the automated installation scripts. This is necessary since in some cases (e.g. Debian-based OSes) these scripts will be placed within these boot files.
Created attachment 277645 [details] [review] Require libarchive 3 Libarchive will be used for archive handling for the express installation feature. E.g. debian based OSes require the installation script to be injected into the initrd archive. This will be applied in the following patches.
Created attachment 277646 [details] [review] Add ArchiveHelper for libarchive error handling The ArchiveHelper will be used by the following patches to introduce a libarchive wrapper. The helper provides the capability to handle the Archive.Result type.
Created attachment 277647 [details] [review] Add wrapper for libarchives Archive.Read The ArchiveReader is capable of reading archives through libarchive. It primarily provides an file extraction functionality. I don't know if we need the get_file_list function in the future. I don't think we'll need it for the iso-read bug or for the initrd injection, so I can strip it off if its not needed elsewhere.
Created attachment 277648 [details] [review] Add wrapper for libarchives Archive.Write The ArchiveWriter provides the functionality of creating archives (either from existing ones or new) and writing files to it. Similar as the ArchiveReader: I dont know if we need insert_files. Depends on the following patches, which I'll do shortly. In case I'll resubmit this patch without the convenience wrapper.
Created attachment 277649 [details] [review] Add wrapper for Archive.Read The ArchiveReader is capable of reading archives through libarchive. It primarily provides an file extraction functionality. Sorry, forgot to adjust commit summary.
Created attachment 277650 [details] [review] Add wrapper for Archive.Write The ArchiveWriter provides the functionality of creating archives (either from existing ones or new) and writing files to it. Same here.
Review of attachment 277650 [details] [review]: ::: src/archive-writer.vala @@ +60,3 @@ + + if (omit) { + if (omit_hardlinked_files || iterator.nlink () == 1 || iterator.hardlink () == null) s/iterator.hardlink () == null/iterator.hardlink () != null/ (If the file itself is a hardlink its safe to omit it.) @@ +90,3 @@ + var entry = get_entry_for_file (src, dst); + if (entry.hardlink () != null && entry.size () == 0) + throw new GLib.IOError.NOT_SUPPORTED ("Inserting hardlinks is currently not supported."); This hit me over night: This doesn't make sense since hardlinks on the real filesystem are indistinguishable from the real file. What makes more sense would be asking FileUtils.test for IS_SYMLINK and IS_DIR (throw NOT_SUPPORTED in this case, IS_REGULAR does not do the trick according to valadoc) (Inserting directories would need an own method that is not needed by Boxes.)
Review of attachment 277646 [details] [review]: I know you don't want to have an extra file for archive utility classes but I am not a fan of duplicating functions. Possible alternatives: - put the class in archive-reader.vala because it makes sense to look into the reader first and the writer uses the ArchiveReader anyway (when importing format, filters (=Compressions) and contents from it) - duplicating the functions (I hate that) - other proposals? I didnt find anything like the variadic templates in C++ in Vala so I had to go with this and the closures for the handle_errors function.
Created attachment 277687 [details] [review] unattended-file: get archive path from derived classes This provides the possibility to decide over the archive name in the derived classes of UnattendedFile. This will be needed for supporting different injection methods for express installation.
Created attachment 277688 [details] [review] unattended-file: use libarchive for copying If libarchive is not able to handle the archive, fall back to mcopy.
Created attachment 277689 [details] [review] unattended-file: handle injection methods Prior to this, injection methods were just assumed to be DISK. This patch adds handling of the DISK and INITRD injection method.
Created attachment 277690 [details] [review] unattended-file: get archive path from derived classes This provides the possibility to decide over the archive name in the derived classes of UnattendedFile. This will be needed for supporting different injection methods for express installation.
Created attachment 277691 [details] [review] unattended-file: use libarchive for copying If libarchive is not able to handle the archive, fall back to mcopy.
Created attachment 277692 [details] [review] unattended-file: handle injection methods Prior to this, injection methods were just assumed to be DISK. This patch adds handling of the DISK and INITRD injection method.
Created attachment 277693 [details] [review] unattended-file: handle injection methods Sorry for all the noise. I got a bit confused with all the patches, branches and git bz. I never worked this way before so things go wrong.
Review of attachment 277644 [details] [review]: ack
Review of attachment 277645 [details] [review]: ack
Review of attachment 277646 [details] [review]: I'm not a fan of duplicating functions either, especially if they aren't very small. :) You must have misunderstood me or I might have been intoxicated if I said otherwise. :) ::: src/archive-utils.vala @@ +10,3 @@ + out unowned Archive.Entry iterator, + uint retry = 1) + throws GLib.IOError { This function seems to be mostly doing exactly what handle_error is doing. You can reduce this function to a few LOC by passing a lambda function to handle_error. Then you'd see that this function isn't really helping much with avoiding duplication. If we only have handle_errors, it can just go into util.vala (with appropriate renaming of course). @@ +32,3 @@ + } + + throw new GLib.IOError.FAILED ("Failed to retrieve header. Message was: '%s'.", ' . Message was" is redundant and IMO sounds weird. At least its inconsistent with how we present error messages. @@ +33,3 @@ + + throw new GLib.IOError.FAILED ("Failed to retrieve header. Message was: '%s'.", + archive.error_string ()); We are actually handling a know value of Archive.Result aren't we? If so, this should go in a case statement and 'default' should probably return_val_if_reached if we are handling all cases here. @@ +36,3 @@ + } + + public delegate Archive.Result libarchive_function (); Convention is to use CamelCase for types, even functions. E.g GLib.HashFunc. @@ +61,3 @@ + } + + throw new GLib.IOError.FAILED ("%s", archive.error_string ()); Same comment about handling of cases above.
Review of attachment 277646 [details] [review]: ::: src/archive-utils.vala @@ +10,3 @@ + out unowned Archive.Entry iterator, + uint retry = 1) + throws GLib.IOError { The main difference to handle_errors is that it doesnt throw an exception on EOF and returns a bool. This EOF result (and the bool return value) is only relevant for getting the next header so this is not a generic error handling thing. @@ +32,3 @@ + } + + throw new GLib.IOError.FAILED ("Failed to retrieve header. Message was: '%s'.", Good hint, I'll change that. @@ +33,3 @@ + + throw new GLib.IOError.FAILED ("Failed to retrieve header. Message was: '%s'.", + archive.error_string ()); I don't know if I'm understanding you right: We are not handling all cases here. Archive.Result.FAILED is not handled and if the maximum number of retries is reached this line will also be reached. @@ +36,3 @@ + } + + public delegate Archive.Result libarchive_function (); Right. @@ +61,3 @@ + } + + throw new GLib.IOError.FAILED ("%s", archive.error_string ()); Same answer as above.
Review of attachment 277649 [details] [review]: Close :) an -> a in the log. ::: src/archive-reader.vala @@ +4,3 @@ + // This is the block size used by example code on the libarchive website + private static const int BLOCK_SIZE = 10240; + public Archive.Read archive; Newline above and below this public field to visually separate it from private and static fields would be nice. @@ +7,3 @@ + private string filename; + private Archive.Format? format = null; + private GLib.List<Archive.Filter>? filters = null; No need to initialize complex types to 'null' as vala (and gtype system too) does that for you. @@ +39,3 @@ + } + + // src_dst is a hash table while the key is the relative path in the archive and the val the path to extract to while -> where val -> value @@ +40,3 @@ + + // src_dst is a hash table while the key is the relative path in the archive and the val the path to extract to + public void extract_files (string[] src, I'd call it src_list @@ +41,3 @@ + // src_dst is a hash table while the key is the relative path in the archive and the val the path to extract to + public void extract_files (string[] src, + string[] dsts, dst isn't a known abbreviation. I'd name it 'dest_list'. @@ +43,3 @@ + string[] dsts, + bool override_if_necessary = false, + uint follow_hardlinks = 1) I don't think this is very much needed. Lets hardcode it to 1. Otherwise 'hardlinks_depth' would be a better name. @@ +54,3 @@ + string[] hardlink_dst = {}; + while (ArchiveHelper.get_next_header (archive, out iterator) && (i < src.length)) { + string dst = null; Let not shorten more than needed: dest. @@ +55,3 @@ + while (ArchiveHelper.get_next_header (archive, out iterator) && (i < src.length)) { + string dst = null; + for (uint j = 0; j < src.length; j++) { Wouldn't the code be simpler/more readable if this was the outer loop? @@ +120,3 @@ + ArchiveHelper.handle_errors (archive, archive.support_filter_all); + else + set_filter_stack (); its a very small function and not called by anything else so I think it'd be more readable if the code was put here rather.
Review of attachment 277690 [details] [review]: "First line (the brief description) must only be one sentence and should start with a capital letter unless it starts with a lowercase symbol or identifier." from https://wiki.gnome.org/Git/CommitMessages . I think other patches also had the same "issue". ::: src/unattended-file.vala @@ +40,3 @@ } + + protected string get_disk_file () { * Always prefer field or prop over getter methods in vala as vala translates read access to getter function and write access to setter method for you so there is no performance penalty and props/fields are more readable IMO. * Why are you redefining it all all subclasses?
Review of attachment 277690 [details] [review]: I'll correct the message(s). ::: src/unattended-file.vala @@ +40,3 @@ } + + protected string get_disk_file () { * Always prefer field or prop over getter methods in vala as vala translates read access to getter function and write access to setter method for you so there is no performance penalty and props/fields are more readable IMO. I have to get accustomed to that... * Why are you redefining it all all subclasses? Because I stripped away the installer prop from the UnattendedFile class. I'll follow your style and unstrip it.
Review of attachment 277691 [details] [review]: ::: src/unattended-file.vala @@ +15,3 @@ + try { + var reader = new ArchiveReader (disk_file); + var writer = new ArchiveWriter.from_archive_reader (reader, disk_file + "~", false); Whats this "~" about? Something specific to libarchive I assume? If so, ArchiveWriter should abstract that. @@ +20,3 @@ + writer.insert_file (source_file.get_path (), dest_name); + + // close files for moving How about "Force vala to close the files already before we move them to any errors from libarchive"? @@ +26,3 @@ + var src = GLib.File.new_for_path (disk_file + "~"); + var dst = GLib.File.new_for_path (disk_file); + src.move (dst, FileCopyFlags.OVERWRITE); I don't get it. What do we need to move files? Didn't we already put the files into the archive above? @@ +28,3 @@ + src.move (dst, FileCopyFlags.OVERWRITE); + } catch (GLib.IOError e) { + if (e.message == "Unrecognized archive format") { yikes, we really should look at the error code here. Messages are not supposed to be for the code. Also for the error code, you want to have a separate catch block rather than having to compare with 'if'. @@ +30,3 @@ + if (e.message == "Unrecognized archive format") { + debug ("Falling back to mcopy..."); + string[] argv = {"mcopy", Unless we know of cases where libarchive fails, I don't think this is worth it.
Review of attachment 277693 [details] [review]: While you addressed other comments from last review of this, you missed one: * Lets have a separate patch for: * Add handling of injection method * Handle DISK & INITRD initrd injection methods ::: src/unattended-file.vala @@ +107,3 @@ + protected string get_disk_file () throws GLib.Error { + if (InstallScriptInjectionMethod.DISK in script.get_injection_methods ()) would be nice to put result of script.get_injection_methods () in a local variable @@ +112,3 @@ + return installer.initrd_file.get_path (); + else + throw new GLib.OptionError.UNKNOWN_OPTION ("No supported injection method available."); We are not doing any option parsing here and OptionError seems to be very specific for that. There are loads of IOError codes and they are generic.
Review of attachment 277691 [details] [review]: ::: src/unattended-file.vala @@ +15,3 @@ + try { + var reader = new ArchiveReader (disk_file); + var writer = new ArchiveWriter.from_archive_reader (reader, disk_file + "~", false); When using libarchive you can't read one file and at the same time write to it. So you need to open the original file for reading, create a temporary file for writing and in the end replace the original file with the temporary one. The "~" postfix is stolen from gedit and could be anything. @@ +20,3 @@ + writer.insert_file (source_file.get_path (), dest_name); + + // close files for moving You seem to have understood my comment. I don't understand your suggested replacement... (therefore mine seems clearer since we both understood it? :)) @@ +26,3 @@ + var src = GLib.File.new_for_path (disk_file + "~"); + var dst = GLib.File.new_for_path (disk_file); + src.move (dst, FileCopyFlags.OVERWRITE); see comment on line 17 @@ +28,3 @@ + src.move (dst, FileCopyFlags.OVERWRITE); + } catch (GLib.IOError e) { + if (e.message == "Unrecognized archive format") { this is a GLib.IOError.FAILED. It is thrown in archive-utils.vala and the message comes from libarchive. So we have only the message to catch the right error. @@ +30,3 @@ + if (e.message == "Unrecognized archive format") { + debug ("Falling back to mcopy..."); + string[] argv = {"mcopy", We do know such cases. It occurs e.g. when express installing fedora: (gnome-boxes:13959): Boxes-DEBUG: unattended-file.vala:13: Copying unattended file 'fedora.ks' into disk drive/image '/home/lasse_backup/.cache/gnome-boxes/fedora20-6-unattended.img' (gnome-boxes:13959): Boxes-DEBUG: unattended-file.vala:31: Falling back to mcopy... (gnome-boxes:13959): Boxes-DEBUG: unattended-file.vala:45: Copied unattended file 'fedora.ks' into disk drive/image '/home/lasse_backup/.cache/gnome-boxes/fedora20-6-unattended.img'
Review of attachment 277649 [details] [review]: Very monotonic answer: ok. ::: src/archive-reader.vala @@ +4,3 @@ + // This is the block size used by example code on the libarchive website + private static const int BLOCK_SIZE = 10240; + public Archive.Read archive; ok @@ +10,3 @@ + + public ArchiveReader (string filename, + Archive.Format? format = null, ok @@ +39,3 @@ + } + + // src_dst is a hash table while the key is the relative path in the archive and the val the path to extract to ok @@ +40,3 @@ + + // src_dst is a hash table while the key is the relative path in the archive and the val the path to extract to + public void extract_files (string[] src, ok @@ +41,3 @@ + // src_dst is a hash table while the key is the relative path in the archive and the val the path to extract to + public void extract_files (string[] src, + string[] dsts, ok @@ +43,3 @@ + string[] dsts, + bool override_if_necessary = false, + uint follow_hardlinks = 1) So you'd go with a bool? If you take a bool or an int isnt that much difference. I'd go for the more flexible solution and take hardlinks_depth. @@ +54,3 @@ + string[] hardlink_dst = {}; + while (ArchiveHelper.get_next_header (archive, out iterator) && (i < src.length)) { + string dst = null; ok @@ +55,3 @@ + while (ArchiveHelper.get_next_header (archive, out iterator) && (i < src.length)) { + string dst = null; + for (uint j = 0; j < src.length; j++) { Then you'd have to iterate through the whole archive several times. I'd want to avoid that as you have to reopen the archive for that every time. I think this has a better performance. @@ +120,3 @@ + ArchiveHelper.handle_errors (archive, archive.support_filter_all); + else + set_filter_stack (); ok
Review of attachment 277693 [details] [review]: ok to all comments.
Review of attachment 277646 [details] [review]: ::: src/archive-utils.vala @@ +9,3 @@ + public static bool get_next_header (Archive.Read archive, + out unowned Archive.Entry iterator, + uint retry = 1) Perhaps 'num_retries' would be a better name? @@ +10,3 @@ + out unowned Archive.Entry iterator, + uint retry = 1) + throws GLib.IOError { Ah ok but it can still be defined based on handle_error. Just that you'll have to through a specific error for EOF from handle_error and catch that in here.
Review of attachment 277646 [details] [review]: ::: src/archive-utils.vala @@ +38,3 @@ + public delegate Archive.Result libarchive_function (); + + public static void handle_errors (Archive.Archive archive, Its name makes it sound like it handles errors while it only translates libarchive function results to errors. Perhaps 'call_libarchive_function' would be a better name?
Review of attachment 277690 [details] [review]: ::: src/unattended-file.vala @@ +40,3 @@ } + + protected string get_disk_file () { > * Why are you redefining it all all subclasses? > Because I stripped away the installer prop from the UnattendedFile class. Ah I had forgotten that UnattendedFile is an interface, not a class.
Review of attachment 277691 [details] [review]: ::: src/unattended-file.vala @@ +15,3 @@ + try { + var reader = new ArchiveReader (disk_file); + var writer = new ArchiveWriter.from_archive_reader (reader, disk_file + "~", false); Ah ok. I think you should add a comment explaining whats going on for stupid people like me. :) @@ +28,3 @@ + src.move (dst, FileCopyFlags.OVERWRITE); + } catch (GLib.IOError e) { + if (e.message == "Unrecognized archive format") { Thats libarchive-specific knowledge and therefore code assuming that doesn't belong here. ArchivWriter should abstract us from that and translate to specific GLib.Error for us to catch here. @@ +30,3 @@ + if (e.message == "Unrecognized archive format") { + debug ("Falling back to mcopy..."); + string[] argv = {"mcopy", I thought the issue with Fedora was only hardlinks. What other issues are there?
Review of attachment 277649 [details] [review]: ::: src/archive-reader.vala @@ +43,3 @@ + string[] dsts, + bool override_if_necessary = false, + uint follow_hardlinks = 1) No, I'd go with no param and hardcode it as I suggested above. No need to add features we are unlikely to need. @@ +55,3 @@ + while (ArchiveHelper.get_next_header (archive, out iterator) && (i < src.length)) { + string dst = null; + for (uint j = 0; j < src.length; j++) { Ah ok. Maybe a comment here saying that would be nice.
Review of attachment 277649 [details] [review]: ::: src/archive-reader.vala @@ +43,3 @@ + string[] dsts, + bool override_if_necessary = false, + uint follow_hardlinks = 1) How would you hardcode it without a param? I don't see how to do this without copying a lot of code, you know there's a recursive invokation down there changing this param, do you?
Review of attachment 277649 [details] [review]: ::: src/archive-reader.vala @@ +43,3 @@ + string[] dsts, + bool override_if_necessary = false, + uint follow_hardlinks = 1) There are several ways: 1. you can put most of this code in another private function where you can have this param. 2. you can use a loop instead (Not my favorite since I'm a fan of functional programming :)). The main point is that public API be as simple and slim as possible.
Review of attachment 277690 [details] [review]: ::: src/unattended-file.vala @@ +40,3 @@ } + + protected string get_disk_file () { * Always prefer field or prop over getter methods in vala as vala translates read access to getter function and write access to setter method for you so there is no performance penalty and props/fields are more readable IMO. I just remembered the reason why this is not a prop: it throws an exception. Props can't do that.
Review of attachment 277691 [details] [review]: ::: src/unattended-file.vala @@ +30,3 @@ + if (e.message == "Unrecognized archive format") { + debug ("Falling back to mcopy..."); + string[] argv = {"mcopy", $ file fedora20-unattended.img fedora20-unattended.img: DOS floppy 1440k, DOS/MBR hard disk boot sector This has nothing to do with hardlinks. Its just that libarchive handle that kind of archives.
Created attachment 279681 [details] [review] Require libarchive 3 Libarchive will be used for archive handling for the express installation feature. E.g. debian based OSes require the installation script to be injected into the initrd archive. This will be applied in the following patches.
Created attachment 279682 [details] [review] util: Add libarchive helper functions These functions will be used by the following patches to introduce a libarchive wrapper. They provide the capability to handle the Archive.Result type.
Created attachment 279683 [details] [review] Add wrapper for Archive.Read The ArchiveReader is capable of reading archives through libarchive. It primarily provides an file extraction functionality.
Created attachment 279684 [details] [review] Add wrapper for Archive.Write The ArchiveWriter provides the functionality of creating archives (either from existing ones or new) and writing files to it.
Created attachment 279685 [details] [review] unattended-file: Get archive path from derived classes This provides the possibility to decide over the archive name in the derived classes of UnattendedFile. This will be needed for supporting different injection methods for express installation.
Created attachment 279686 [details] [review] unattended-file: Use libarchive for copying If libarchive is not able to handle the archive, fall back to mcopy.
Created attachment 279687 [details] [review] unattended-file: Handle injection methods Prior to this, injection methods were just assumed to be DISK.
Created attachment 279688 [details] [review] unattended-file: Handle INITRD injection method
Review of attachment 279681 [details] [review]: ::: configure.ac @@ +96,3 @@ glib-2.0 >= $GLIB_MIN_VERSION gio-2.0 >= $GLIB_MIN_VERSION + libarchive >= $LIBARCHIVE_MIN_VERSION You don't have to recheck the whole patch, this is the only line that was added in comparision to the one you accepted. Reason for the change of this accepted patch is the move of the libarchive helper functions to src/util.vala which is compiled into this common library.
Review of attachment 277690 [details] [review]: ::: src/unattended-file.vala @@ +40,3 @@ } + + protected string get_disk_file () { Huh? Where? protected string get_disk_file () { I don't see any error throwing declaration.
Review of attachment 277691 [details] [review]: ::: src/unattended-file.vala @@ +30,3 @@ + if (e.message == "Unrecognized archive format") { + debug ("Falling back to mcopy..."); + string[] argv = {"mcopy", Can you be a bit specific please. What kind of archives it can and can not handle?
Review of attachment 277690 [details] [review]: ::: src/unattended-file.vala @@ +40,3 @@ } + + protected string get_disk_file () { Sorry, I was editing all of these patches so I wasnt aware that this is not the case in this patch YET. However it is needed later: https://bugzilla.gnome.org/review?bug=730640&attachment=279687
Review of attachment 277691 [details] [review]: ::: src/unattended-file.vala @@ +30,3 @@ + if (e.message == "Unrecognized archive format") { + debug ("Falling back to mcopy..."); + string[] argv = {"mcopy", Libarchive: * Reads a variety of formats, including tar, pax, cpio, zip, xar, lha, ar, cab, mtree, rar, and ISO images. * Writes tar, pax, cpio, zip, xar, ar, ISO, mtree, and shar archives. Source: http://libarchive.org/, paragraph 3 IIUC for this DISK insertion you create these DOS floppy disk files. These are not supported. I don't think this affects other files, ISOs, INITRDs and kernels should be handled by libarchive.
Review of attachment 277690 [details] [review]: ::: src/unattended-file.vala @@ +40,3 @@ } + + protected string get_disk_file () { Ah ok. I also now realized why you mentioned "Because I stripped away the installer prop from the UnattendedFile class." as rationale for redefining this in all classes. Yeah, I think thats not a good idea to strip that prop away. Since we want this as method rather, we should take advantage of it and define it once in the interface (as 'virtual') and then only redefine it the class we need to redefine it in.
Review of attachment 277691 [details] [review]: ::: src/unattended-file.vala @@ +30,3 @@ + if (e.message == "Unrecognized archive format") { + debug ("Falling back to mcopy..."); + string[] argv = {"mcopy", Here we are specifically putting files into DOS floppy disk so if libarchive can't handle that, this change is quite useless.
Review of attachment 279685 [details] [review]: TODO: dont strip installer away, make disk_file a prop with virtual getter
(In reply to comment #84) > Review of attachment 279685 [details] [review]: > > TODO: dont strip installer away, make disk_file a prop with virtual getter Thats not what I said. You can't have a prop defined in interface, only declared and used by methods.
Review of attachment 279685 [details] [review]: Correction: Provide a (defined) virtual method get_disk_file ()
Review of attachment 277691 [details] [review]: ::: src/unattended-file.vala @@ +30,3 @@ + if (e.message == "Unrecognized archive format") { + debug ("Falling back to mcopy..."); + string[] argv = {"mcopy", Well its needed later when the initrd injection method is added. How would you handle this?
Review of attachment 277691 [details] [review]: ::: src/unattended-file.vala @@ +30,3 @@ + if (e.message == "Unrecognized archive format") { + debug ("Falling back to mcopy..."); + string[] argv = {"mcopy", Ah ok but if we know already which case libarchive can be used for and which not, we should decide to use the method according to the injection method then. There is no point in trying out something that we know will not work.
Review of attachment 279684 [details] [review]: ::: src/archive-writer.vala @@ +94,3 @@ + throw new GLib.IOError.NOT_FOUND ("Source file '%s' cannot be injected. File not found.", src); + + if (!FileUtils.test (src, FileTest.IS_SYMLINK)) s/!//
Created attachment 280508 [details] [review] Add wrapper for Archive.Write The ArchiveWriter provides the functionality of creating archives (either from existing ones or new) and writing files to it.
Created attachment 280509 [details] [review] unattended-file: Introduce get_disk_file() This provides the possibility to decide over the archive name in the derived classes of UnattendedFile. This will be needed for supporting different injection methods for express installation.
Created attachment 280510 [details] [review] unattended-file: Correct spacing
Created attachment 280511 [details] [review] unattended-file: Use libarchive for non-dos images If libarchive is not able to handle the archive, use mcopy.
Created attachment 280512 [details] [review] unattended-file: Handle injection methods Prior to this, injection methods were just assumed to be DISK.
Created attachment 280513 [details] [review] unattended-file: Handle INITRD injection method
please do not review these patches yet.
Created attachment 280515 [details] [review] unattended-file: Handle injection methods Prior to this, injection methods were just assumed to be DISK.
Created attachment 280516 [details] [review] unattended-file: Handle INITRD injection method
now you're free to go ;)
Review of attachment 279681 [details] [review]: ::: configure.ac @@ +79,3 @@ uuid >= $UUID_REQUIRED libsoup-2.4 >= $LIBSOUP_REQUIRED + libarchive >= $LIBARCHIVE_MIN_VERSION This becomes redundant then since Boxes common is linked.
Review of attachment 279682 [details] [review]: ack
Review of attachment 279683 [details] [review]: ::: src/archive-reader.vala @@ +61,3 @@ + string[] hardlink_src = {}; + string[] hardlink_dest = {}; + while (get_next_header (archive, out iterator) && (i < src_list.length)) { I have a big problem reading through the loop about what exact it does. Can we really not divide this big block into multiple functions? If not, can we add some comments explaining whats going on? @@ +78,3 @@ + + if (iterator.hardlink () != null && iterator.size () == 0) { + debug ("Following hardlink of '%s' to '%s'.\n", iterator.pathname (), iterator.hardlink ()); \n is redundant in debug/warning/log/message/ etc
Review of attachment 280508 [details] [review]: Looks good otherwise. ::: src/archive-writer.vala @@ +108,3 @@ + } + + public void add_filters () throws GLib.IOError { why is this public? Since filters are already passed as construct param, I don't think user should need to call this method for setting them up. Also the name is misleading as the filters are already added to Writer and this just adds to underlying archive (an internal detail). @@ +120,3 @@ + } + + private void get_filters (Archive.Read read_archive) { The name makes it sound like it returns filters rather than initializing object field/property. I'd suggest 'copy_filters_from_read_archive'.
Review of attachment 280509 [details] [review]: ack
Review of attachment 280510 [details] [review]: ack
Review of attachment 280511 [details] [review]: ::: src/unattended-file.vala @@ +15,3 @@ debug ("Copying unattended file '%s' into disk drive/image '%s'", dest_name, disk_file); + + if (!disk_is_dos_image ()) { maybe it'd be better to put each copy implementation in its own function. @@ +50,3 @@ } + + protected virtual bool disk_is_dos_image () { I don't think its specific to type of image or is it? @@ +51,3 @@ + + protected virtual bool disk_is_dos_image () { + return true; I don't think this is specific to subclasses but rather the file. You want to check the filetype here. 'application/x-raw-disk-image' is what you get.
Review of attachment 280515 [details] [review]: ::: src/unattended-file.vala @@ +107,3 @@ } + + protected bool disk_is_dos_image () { We shoudln't need to override this here as per comment on previous patch. @@ +114,3 @@ + protected string get_disk_file () throws GLib.Error { + var avail_methods = script.get_injection_methods (); + if (InstallScriptInjectionMethod.DISK in avail_methods) It seems very wrong to check if our insertion method is available or not in this getter for disk file path. We chould check this and error out in the construction time e.g.
Review of attachment 280516 [details] [review]: ::: src/unattended-file.vala @@ +117,3 @@ return installer.disk_file.get_path (); + else if (InstallScriptInjectionMethod.INITRD in avail_methods) + return installer.initrd_file.get_path (); Same here. Perhaps it'd be better to go though available methods at the beginning and put the one of our choice into a field/property.
Review of attachment 279681 [details] [review]: ::: configure.ac @@ +79,3 @@ uuid >= $UUID_REQUIRED libsoup-2.4 >= $LIBSOUP_REQUIRED + libarchive >= $LIBARCHIVE_MIN_VERSION why do you need glib and gio there then? And why doesnt it compile if I remove the line?
Review of attachment 279681 [details] [review]: ::: configure.ac @@ +79,3 @@ uuid >= $UUID_REQUIRED libsoup-2.4 >= $LIBSOUP_REQUIRED + libarchive >= $LIBARCHIVE_MIN_VERSION ah, nm you are right. If compilation fails if you remove it from either here or below, it means you need it in both. :)
Review of attachment 280508 [details] [review]: ::: src/archive-writer.vala @@ +108,3 @@ + } + + public void add_filters () throws GLib.IOError { this function isnt really useful, its short and a two-liner so I'll just inline it.
Review of attachment 280511 [details] [review]: ::: src/unattended-file.vala @@ +51,3 @@ + + protected virtual bool disk_is_dos_image () { + return true; IIUC I'd need to load the contents of the file into the memory to check it with g_content_type_guess. I'm not really a fan of this since this is basically what happened before with some disadvantages: In the initial solution we let libarchive decide whether it supports the archive type or not. Thats a good abstraction in my eyes and is great since if the libarchive guys are adding archive formats we'll automatically use this. What happens on the low level is basically the same as what you are proposing just with less code and more generic: libarchive looks at the file header. Libarchive provides this, why not use it? While I agree that handling that via exceptions was probably not the _best_ thing I am more a fan of that than making a non-abstract type recognition around it while we provide the unsupported formats via hardcoding them. Can we maybe find a better solution like having a static function: format_supported () in the reader and the writer which tries to open the file in libarchive and reports false if it fails?
Review of attachment 280511 [details] [review]: ::: src/unattended-file.vala @@ +51,3 @@ + + protected virtual bool disk_is_dos_image () { + return true; some things you are missing: * libarchive failing does not necessarily mean that we are dealing with something mcopy can deal with. * you don't need to load contents to memory for g_content_type_guess to work. It loads the necessary amount of data from file for detection on its own.
Review of attachment 280516 [details] [review]: ::: src/unattended-file.vala @@ +117,3 @@ return installer.disk_file.get_path (); + else if (InstallScriptInjectionMethod.INITRD in avail_methods) + return installer.initrd_file.get_path (); Quite true but the initrd_file will be set after the construction so if you store it into a member here it'll be null. How about leaving the geter this way and checking if a injection method is available in the constructor?
Review of attachment 280516 [details] [review]: ::: src/unattended-file.vala @@ +117,3 @@ return installer.disk_file.get_path (); + else if (InstallScriptInjectionMethod.INITRD in avail_methods) + return installer.initrd_file.get_path (); No the initrd file but the available method. i-e at construction, choose one (or error out if we don't support any of the methods) and then store it in a field/property.
Created attachment 281408 [details] [review] Add wrapper for Archive.Read The ArchiveReader is capable of reading archives through libarchive. It primarily provides an file extraction functionality.
Created attachment 281409 [details] [review] Add wrapper for Archive.Write The ArchiveWriter provides the functionality of creating archives (either from existing ones or new) and writing files to it.
Created attachment 281410 [details] [review] unattended-file: Introduce get_disk_file() This provides the possibility to decide over the archive name in the derived classes of UnattendedFile. This will be needed for supporting different injection methods for express installation.
Created attachment 281411 [details] [review] unattended-file: Correct spacing
Created attachment 281412 [details] [review] unattended-file: Use libarchive for non-dos images In the following patches Boxes will learn how to handle non dos images. For these new formats we need libarchive for copying.
Created attachment 281413 [details] [review] media-manager: Use media if UnattendedInstaller fails If the UnattendedInstaller cannot be created, fallback to the usual installation media.
Created attachment 281414 [details] [review] unattended-installer: Ignore unsupported scripts In the following patch the constructor of UnattendedSetupFile will check if the script has an unsupported injection method. Lets use only supported scripts and throw an error if there are no such scripts.
Created attachment 281415 [details] [review] unattended-file: Handle injection methods Prior to this, injection methods were just assumed to be DISK.
Created attachment 281416 [details] [review] unattended-file: Handle INITRD injection method
Review of attachment 281415 [details] [review]: ::: src/unattended-file.vala @@ +131,3 @@ + } else + throw new GLib.IOError.NOT_SUPPORTED ("No supported injection method available."); + } I think throwing an error here is more secure. It is good for code reusage and doesnt rely on assumptions (e.g. the invariant "noone changes injection_methods after construction").
Review of attachment 281415 [details] [review]: ::: src/unattended-file.vala @@ +131,3 @@ + } else + throw new GLib.IOError.NOT_SUPPORTED ("No supported injection method available."); + } ? Does injection methods change after construction? libosinfo db is only loaded once so its almost impossible that they'll change in future.
Review of attachment 281415 [details] [review]: ::: src/unattended-file.vala @@ +131,3 @@ + } else + throw new GLib.IOError.NOT_SUPPORTED ("No supported injection method available."); + } currently they dont change after construction but you never know what happens. The main reason throwing the exception here was for me code reusability. (I can invoke it in constructor to get the exception.)
Created attachment 281935 [details] [review] unattended-file: Introduce disk_file prop This provides the possibility to decide over the archive name in the derived classes of UnattendedFile. This will be needed for supporting different injection methods for express installation.
Created attachment 281936 [details] [review] unattended-file: Correct spacing
Created attachment 281937 [details] [review] unattended-file: Use libarchive for non-dos images In the following patches Boxes will learn how to handle non dos images. For these new formats we need libarchive for copying.
Created attachment 281938 [details] [review] media-manager: Use media if UnattendedInstaller fails If the UnattendedInstaller cannot be created, fallback to the usual installation media.
Created attachment 281939 [details] [review] unattended-installer: Ignore unsupported scripts In the following patch the constructor of UnattendedSetupFile will check if the script has an unsupported injection method. Lets use only supported scripts and throw an error if there are no such scripts.
Created attachment 281940 [details] [review] unattended-file: Handle injection methods Prior to this, injection methods were just assumed to be DISK.
Created attachment 281941 [details] [review] unattended-file: Handle INITRD injection method
Review of attachment 281408 [details] [review]: ::: src/archive-reader.vala @@ +67,3 @@ + string[] hardlink_dest = {}; + while (get_next_header (archive, out iterator) && (i < src_list.length)) { + var dest = get_dest_file (src_list, dest_list, iterator.pathname ()); From zeeshan: get_dest_file is an improvement to readability but we really should divide this block inside while loop, more. For now i guess a FIXME comment would suffice.
Created attachment 282014 [details] [review] Add wrapper for Archive.Read The ArchiveReader is capable of reading archives through libarchive. It primarily provides an file extraction functionality.
Created attachment 282015 [details] [review] Add wrapper for Archive.Write The ArchiveWriter provides the functionality of creating archives (either from existing ones or new) and writing files to it.
Created attachment 282016 [details] [review] unattended-file: Introduce disk_file prop This provides the possibility to decide over the archive name in the derived classes of UnattendedFile. This will be needed for supporting different injection methods for express installation.
Created attachment 282017 [details] [review] unattended-file: Correct spacing
Created attachment 282018 [details] [review] unattended-file: Use libarchive for non-dos images In the following patches Boxes will learn how to handle non dos images. For these new formats we need libarchive for copying.
Created attachment 282019 [details] [review] media-manager: Use media if UnattendedInstaller fails If the UnattendedInstaller cannot be created, fallback to the usual installation media.
Created attachment 282020 [details] [review] unattended-installer: Ignore unsupported scripts In the following patch the constructor of UnattendedSetupFile will check if the script has an unsupported injection method. Lets use only supported scripts and throw an error if there are no such scripts.
Created attachment 282021 [details] [review] unattended-file: Handle injection methods Prior to this, injection methods were just assumed to be DISK.
Created attachment 282022 [details] [review] unattended-file: Handle INITRD injection method
Created attachment 282023 [details] [review] unattended-file: Use libarchive for non-dos images In the following patches Boxes will learn how to handle non dos images. For these new formats we need libarchive for copying. As libarchive and its wrapper only have a non-async API we need to run it in an own thread.
Created attachment 282024 [details] [review] media-manager: Use media if UnattendedInstaller fails If the UnattendedInstaller cannot be created, fallback to the usual installation media.
Created attachment 282025 [details] [review] unattended-installer: Ignore unsupported scripts In the following patch the constructor of UnattendedSetupFile will check if the script has an unsupported injection method. Lets use only supported scripts and throw an error if there are no such scripts.
Created attachment 282026 [details] [review] unattended-file: Handle injection methods Prior to this, injection methods were just assumed to be DISK.
Created attachment 282027 [details] [review] unattended-file: Handle INITRD injection method
Review of attachment 282014 [details] [review]: ACK
Review of attachment 282015 [details] [review]: ack
Created attachment 282155 [details] [review] unattended-installer: Extract boot files before injection This moves the extraction of the boot files to be before the injection of the automated installation scripts. This is necessary since in some cases (e.g. Debian-based OSes) these scripts will be placed within these boot files.
Created attachment 282156 [details] [review] Require libarchive 3 Libarchive will be used for archive handling for the express installation feature. E.g. debian based OSes require the installation script to be injected into the initrd archive. This will be applied in the following patches.
Created attachment 282157 [details] [review] util: Add libarchive helper functions These functions will be used by the following patches to introduce a libarchive wrapper. They provide the capability to handle the Archive.Result type.
Created attachment 282158 [details] [review] Add wrapper for Archive.Read The ArchiveReader is capable of reading archives through libarchive. It primarily provides an file extraction functionality.
Created attachment 282159 [details] [review] Add wrapper for Archive.Write The ArchiveWriter provides the functionality of creating archives (either from existing ones or new) and writing files to it.
Created attachment 282160 [details] [review] unattended-file: Introduce disk_file prop This provides the possibility to override the disk file in the derived classes of UnattendedFile to copy to initrd file rather. This will be needed for supporting initrd injection method for express installation.
Created attachment 282161 [details] [review] unattended-file: Correct spacing
Created attachment 282162 [details] [review] unattended-file: Use libarchive for non-DOS images In the following patches Boxes will learn how to handle non-DOS images. For these new formats we need libarchive for copying. As libarchive and its wrapper only have a sync API we need to run it in an own thread.
Created attachment 282163 [details] [review] media-manager: Use original media if unattended setup fails If the UnattendedInstaller instance cannot be created, fallback to the original installation media.
Created attachment 282164 [details] [review] unattended-file: Ensure DISK injection method is supported Prior to this, injection methods were just assumed to be DISK. We should check if it actually is supported before using it.
Created attachment 282165 [details] [review] unattended-file: Handle INITRD injection method https://bugzilla.gnome.org/show_bug.cgi?id=730640 Conflicts: src/unattended-file.vala
Comment on attachment 282025 [details] [review] unattended-installer: Ignore unsupported scripts nak
Attachment 282155 [details] pushed as d7bbc26 - unattended-installer: Extract boot files before injection Attachment 282156 [details] pushed as ce69528 - Require libarchive 3 Attachment 282157 [details] pushed as fa2a409 - util: Add libarchive helper functions Attachment 282158 [details] pushed as b4f851f - Add wrapper for Archive.Read Attachment 282159 [details] pushed as 06aecbd - Add wrapper for Archive.Write Attachment 282160 [details] pushed as 510f4ff - unattended-file: Introduce disk_file prop Attachment 282161 [details] pushed as 198eadd - unattended-file: Correct spacing Attachment 282162 [details] pushed as 686e2d5 - unattended-file: Use libarchive for non-DOS images Attachment 282163 [details] pushed as 9e1e875 - media-manager: Use original media if unattended setup fails Attachment 282164 [details] pushed as 789600c - unattended-file: Ensure DISK injection method is supported Attachment 282165 [details] pushed as 22c329c - unattended-file: Handle INITRD injection method