procsfdisk.awk "original_variable" calulation?

[H3Krn]

@sebastian-roth I won’t claim to completely understand the procsfdisk code, but I’m currently suspecting the “if” clause on line 436.

436                if (match(fixedList, regex)) {
437                    continue;
438                }

When encountering a extended partion, I think it’s size should never be taken into account when trying to resize variable primary or logical partitions. So the for loop should break there in whatever case.

I’ve removed that “if” clause, leaving the “continue;” in place. This gives me:

./procsfdisk.awk -v SECTOR_SIZE=512 -v CHUNK_SIZE=512 -v MIN_START=2048 -v action=filldisk -v target=/dev/sda -v sizePos=134217728 -v diskSize=134217728 -v fixedList=1: d1.minimum.partitions d1.partitions 
# Partition table is consistent.
label: dos
label-id: 0x964ef18c
device: /dev/sda
unit: sectors
sector-size: 512

/dev/sda1 : start=        2048, size=     1048576, type=b, bootable
/dev/sda2 : start=     1052670, size=   133162498, type=5
/dev/sda5 : start=     1052672, size=   133162496, type=83

To me this seems a much more believable layout. So I’ve patched my init.xz, copying in the new procsfdisk.awk script using the postinitscript method. This seems to solve the issue for me. (… after deleting the d1p2.* files from my image as they overwrite the new layout of the extended partition.)

[rodluz]

@h3krn & @Sebastian-Roth I may be completely wrong about this one (I haven’t really taken a deep dive into this file) but looking at the code, is the issue in the if statement in line 457? I only say this because the comment contradicts the statement. Should the ! be removed from the if statement?

456        # If fixed, add the partition size.
457        if (!match(fixedList, regex)) {
458            # Increment the variable value.
459            original_variable += p_size;
460            continue;
461        }

[Tom Elliott]

@rodluz It’s been a very long time since I’ve had to work on this file.

I suspect the comment is correct though, even if the if statement is odd (but I’d have to review the whole of procsfdisk.awk for full understanding. A few lines out of the whole isn’t generally enough of an overview, but that doesn’t mean we didn’t make a mistake either.

[H3Krn]

@rodluz allthough that comment did confuse me. I don’t think it is related to my issue with calculation of the extended partition. Currently what I know of the filldisk functions is:

• It sums the sizes of all fixed partitions from the image in “original_fixed”
• It sums the sizes of all variable partitions from the image in “original_variable”
• It calculates “new_available” (new storage available for variable size partitions) by subtracting “original_fixed” from the new disk size.
• It calculates a factor to resize all variable partitions by dividing “new_variable” by “original_variable”.

My specific problem is that the size of the extended partition is added to “original_variable” whereas it this should be completely ignored in the resize factoring. (only variable logical partitions should be added).

All in all, I think when a variable partition is encountered, that iteration of the for loop should never arrive at lines 456.

On a separate note, it now occurs to me that a fixed extended partition might also not be properly handled. If the extended partition is marked as fixed, I think that “fixed” flag should be inherited to the logical partitions. As you simply can’t resize logical partitions to outside of the extended one.

Hope this all makes sense?

[Tom Elliott]

@rodluz To your point I re-evaluated the file and again, you took these lines out of context:
The full scope of it is:

        # If fixed, add the partition size.
        if (!match(fixedList, regex)) {
            # Increment the variable value.
            original_variable += p_size;
            continue;
        }
        original_fixed += p_size;
        partitions[pName, "orig_size"] = p_size;

Essentially my approach to scripting is to do reverse style coding. What do I mean by this:

Do things that I expect to work a certain way after checking the reverse.

This allows less nesting statements (which makes managing the code a little easier in my eyes.)

So while you’re right that the comment doesn’t appear to match, we have it setting original_variable, and if it is in the fixed list, it gets put into original_fixed.

Specifically the continue says okay well that’s not true, so put it in this point and skip the rest of the loop it’s in. If that’s false, it will add it to the fixed list.

[Tom Elliott]

@h3krn I believe 436 thru 438 is safe to remove, but I’m trying to grapple my mind in why that was added.

I suspect the mentality was if it’s fixed skip trying to do anything resizing but would skip everything below it. I’ll try to think about what I was thinking here, but for now I believe it’s safe to just comment these 3 lines out.

These may have also been for debugging and I just forgot to remove them lol.

[H3Krn]

@tom-elliott I did remove the “if” clause on lines 436 and 438. But I left the continue; statement at line 437. That way the size of the extended partition is left out of scope for the resize variables starting line 457

See here: https://github.com/FOGProject/fos/pull/52

(Feel free to reject or disregard that PR if you feel it could have been done better.)

[Sebastian Roth]

@H3Krn Still have not found enough time to look into this thoroughly. Will do so soon. Thanks for your PR. We’ll discuss further details there.

Edit: Just found the other topic I was talking about before: https://forums.fogproject.org/topic/16106/resizing-extfs-volume-done-but-system-partition-not-expanded-to-disk-size

[H3Krn]

@sebastian-roth, no worries. Thnx for the update. I had a quick glance at the other topic but I’ll need to read I properly to make any accurate observations

Current causes I found for my own case:

• The bug in the procsfdisk.awk resize script (fixed for myself with the PR)
• After resizing partitions during a restore task the contents of the extended partition table (d1p2 in my case) is written to the disk. This contains the layout of the original small logical partitions. (solved by deleting d1p2.ebr &d1p2.img from the image)

For now I have my setup working. Just hoping I can help you guys with it.

[H3Krn]

Hi all,

been A while. I guess we’ve all been busy. Just wanted to check back in if there’s anything I can still help with on this topic?

Grtz, Harm

[Sebastian Roth]

@H3Krn Thanks for checking up in this. I’ve got it on my list but don’t seem to find the time.

@rodluz Do you want to take a look at this?