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

[Jasper Hartline]

On Wed, Aug 18, 2010 at 6:51 AM, Richard Shaw <hobbes1069 at gmail.com> wrote:
>>    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.

True. If it becomes more complicated python-magic
will check teh file magic.


>>            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.

I understand. I think I wrote it in the foresight that we would possibly only
call it once with any options. Therefore, when a reader is reading it
and they see if options: They expect options are expected.
If I write if not options:
Also this is just semantical I think.
If I write the upper blocks to be code we don't use much..
I suppesa options would work too. I will go back through it once I get
the base case working.

>>        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.

test
    return
return

Is easy to read.
Python is based in it's blocks on spacing. So we tend to lean towards
the indentation to read Python more or less.

> 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.

I was hoping not to have to test for too many errors.
The only ones I can think of would be
You are not root. You can't mount.
All of your loop devices are already used, can't get one.
The files you provided on the command line don't exist.

So far anyhow.


>>    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.

Hm.
we divide (x86size + x64size) by block_size -> 2048 to get count for
passing to dd.
The way I understand it, mkisofs pads the ISO up to the last 2048
divisible block.
There shouldn't be a reason why a ISO file made with mkisofs is not
divisible by 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.

Yeah I noticed that. I want to set it, but not test for it and produce
an error or usage.

>> 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...

Yeah right now we want to keep it simpler until we get the base case working.
2 ISO files, one target which is multiarch.

>>    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?

Yes, I did this on purpose, in the foresight to be run from cron or as
a nightly operation.


>> 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.

Hm. Right. so no string replace operations on a list, bummer.
But, we could create a new list out of the old.

Read data we want from the old list,
create a new list, with the new inputs and write it to the file.

As far as keeping it simpler for now, you can see I've done that
by saying we need to partition the lodevivce type 0x83
We can expand this later.