[Fedora-livecd-list] List of previous issues with livecd-tools which should be addressed

[Richard Shaw]

On Wed, Aug 18, 2010 at 12:04 AM, Jasper Hartline
<jasper.hartline at gmail.com> wrote:
> On Tue, Aug 17, 2010 at 8:39 PM, Richard Shaw <hobbes1069 at gmail.com> wrote:
>> I put in a bunch of inline comments. I decided it was better to
>> communicate my changes there instead of in the email. You can comment
>> back here or there or delete them if you understand what I was trying
>> to do and agree. I probably should have made a diff patch as well but
>> I didn't save a copy of the original and I'm to lazy to do it now :) I
>> don't think most of it was drastic.
>
>    def mount(src, dst, options=None):
>        if os.path.exists(src):         # Is src the lodev device?
>
> mount is a generic mount procedure, src can be a file, a loop device..
> and ISO, a disk. Anything really.

Ok, so maybe later we'll need to know what type of file/device it is
and we can add tests for it if necessary.

>
>            if options:                 # None is the same as Null so
> you can test for existence instead.
>
> I'm pretty sure if options: without making options=None will cause
> and IndexErroor. So I probably should just test for thing: when I set
> offset=None or
> options=None in the function.

Sorry, I may have confused you with my shorthand comment. Yes, we'll
still need the "None" default in the function definition. I just
changed the logic so instead of testing "if not" (i.e. False)
condition  I'm testing for the if True condition.

I try to stick with 'if <True>' conditions if you're going to do
something one way or the other. I use 'if not <True>' when if
something is False then I do something and if it's True I don't do
anything. Does that make sense? I'm not sure I'm explaining my logic
well.

>        else:                           # Let's make sure only one
> return statement get's triggered.
>            return False                # Might as well return
> something useful so we can test for failure.
>
> This is sort of debateable.
> I mean, if the mount operation fails, we can't really do anything to correct it.
> In other words, it should succeed, else return nothing.
> I am pretty sure return will just return nothing.

Yeah, I did some research last night and there are people on both
sides of the "one return statement" or "multiple return statements in
a function" camps. Also now that I re-read your comment I don't think
you were arguing that :) Sorry, sick kid == little sleep.

To answer what I think you're commenting about, yes, I agree. So since
python is going to see the same thing (None=False), I think the
question is, if there's a chance of others (hopefully several others)
helping with this, does the explicit False statement aid readability?

Now that I think about it, we should probably figure out what the best
practice is for returning an error if the function is not successful.

>        else:
>            print "Something is wrong, %s has no size." % ent
>            return False
>
> I think this is good, you never know when the file you are trying to
> use may not be the right one.
>
>
>    def blocks(block_size, size):
>        # Round up to nearest block
>        # Make sure floating math, not integer math or we don't get remainders.
>        # Turn back into an integer for return statement.
>        return int(math.ceil(size / float(block_size)))
>
> I'm not sure why we would need this.
> Any idea?

I have some experience in this area as I'm now the discspan (check it
out on freshmeat or sourceforge) maintainer. ISO 9660 also has 2k
sectors so in order to properly estimate the size you have to round up
to the nearest sector per file. I'm guessing we're only doing this for
the whole image, but it's the same idea. No matter how small a file is
it at least has to take up one sector (optical media) or one block
(file systems).

Integer math will drop remainders (it doesn't even round) so for 11 /
3 the result would be 3, not 3.83 or 4. Making any one of the
arguments a floating point number trips the whole thing into floating
point math. Since we're stuck allocating a whole block we can't round
down so that's where the ceiling function comes in.

>    def setup(x86, x64, multi):
>        # Reworked the logic a bit.
>        block_size = 2048               # Should this be a global
> constant instead?
>        sz = size(x86) + size(x64)
>        count = blocks(block_size, sz)
>
>        multi = mkimage(str(blsz), count)
>        losetup(multi, lo())
>
> block size is a constant, in the scope of setup() which is about the only place
> we would need to use it. Unless that changes.
> I'm using "2048"

In that case we should probably just capitalize the variable (per the
PEP) so: BLOCK_SIZE = 2048...

>    def parse(x86, x64, multi):
>        for file in x86, x64, multi:    # Do we expect that multi exists yet?
>            if os.path.exists(file):    # Should we test for existance
> or isfile?
>                pass
>            else:
>                parser.error("One of the images does not exist.")
>        setup(x86, x64, multi)
>
> multi is our end-result target file.
> This begins with a temporary file, that is then partitioned and formatted.
> Copied to, and closed up eventually, then shutil.move(tmpfile, multi)

Ok, obviously the code isn't fully flushed out but it looks like
currently the test for existence happens before it's created.

> Maybe both. if os.path.exixts() and if os.path.isfile()
> Then again, there shouldn't be a reason we should disallow use of
> hardlinked ISO image files or soft linked ones. Have any logic behind
> preventing use of the files if they are links and not files?

As long as they're usable/accessible I guess it shouldn't matter, but
if a ISO is referenced we can feel pretty good that it's going to
work, if it's something else, like the image mounted on /dev/sr0, does
that change how we do anything? For one, the files will be already
accessible unlike an ISO which we would have to mount first.

I think depends on how far we want to get in the first "release". It
would be OK with me to require the sources be ISO's at this point for
simplicity sake...

>    try:                                # Any reason this would fail?
>        parse(args[0], args[1], args[2])
>    except:
>        parser.error("Something failed. Better luck next time!")
>        sys.exit(1)
>
> Yes, when the files don't exist it will fail with an IndexError
> on sys.argv[]

Well I'm already testing that there are exactly 3 arguments during
option parsing so parse() itself should never fail, now since it calls
other functions that may fail it kinda make sense. I am concerned
though that we're essentially wrapping the entire program in a 'try'
statement, which I'm guessing is probably not a good programming
practice. It could also mask problems we should be handling, right?

> I think I might also do something different with the syslinux.cfg stuff.
> Instead of having the huge text data as a content "" and filled.
> Instead read the ISOLINUX.CFG and use re.search and re.match
> To find what we need, then concatenate using line indexes
> from readlines().
>
> Can contents = fd.readlines() be written to?
> Do you know? Is the content of contents static?

I think so as it returns a list. I just made sure it doesn't return a generator.

> Still looking at the module investigator text you sent.
> It is hard to tell how to do simple things with that module.
>
> All we need to do is, partition it.. type 0x83
> Start cylinder 1 or 63, and end, up to the end of the file.
> Then, write that partition table.
> Format the partition.
> Move to mounting our ISO files and doing extraction of the UUID
> and other things. Copy the files to the partition.
>
> Close it up.
> --
> livecd mailing list
> livecd at lists.fedoraproject.org
> https://admin.fedoraproject.org/mailman/listinfo/livecd
>