ext2

Drive subsystem, filesystem drivers
  • Hi,

    I've committed all code till the last ftp update I made; intermediate code isn't stable.

    Regards,
    Shikhin
  • Shikhin wrote: I'd love all sort of code reviews, so do look at everything.
    Well, I've just had a look and everything looks neat and well organized, so kudos for that!

    Some small remarks:
    In ext2.asm, I found this:

    Code: Select all

            add     eax, 3054539008                         ; (369 * 365 + 89) * 24 * 3600
    First, why did you calculate this number? The following code perfectly works in FASM too:

    Code: Select all

            add     eax, (369 * 365 + 89) * 24 * 3600
    (Let FASM's pre-processor do the dirty work for you)
    Then, I noticed that you used this number in some more places, maybe it's better to make it a constant then.

    At the bottom of the same file I found this:

    Code: Select all

    self_link:   db ".", 0   
    The code formatting guidelines asks us not to place anything after a label. Just loose the semicolon.

    Keep up the good work! ;)
    "Any intelligent fool can make things bigger, more complex, and more violent. It takes a touch of genius -- and a lot of courage -- to move in the opposite direction." Albert Einstein
  • Hi,
    hidnplayr wrote:Well, I've just had a look and everything looks neat and well organized, so kudos for that!
    Thanks!
    hidnplayr wrote:Some small remarks:
    In ext2.asm, I found this:

    Code: Select all

            add     eax, 3054539008                         ; (369 * 365 + 89) * 24 * 3600
    First, why did you calculate this number? The following code perfectly works in FASM too:

    Code: Select all

            add     eax, (369 * 365 + 89) * 24 * 3600
    (Let FASM's pre-processor do the dirty work for you)
    Then, I noticed that you used this number in some more places, maybe it's better to make it a constant then.
    That part was inherited from the existing driver; I think I added the comment to justify the magic constant, but since it was used in the ntfs and FAT driver as well, I didn't think of replacing it. You're right though, and I should do it.
    hidnplayr wrote:At the bottom of the same file I found this:

    Code: Select all

    self_link:   db ".", 0   
    The code formatting guidelines asks us not to place anything after a label. Just loose the semicolon.
    Hrm. Is this correct:

    Code: Select all

    self_link:
            db ".", 0
    (not sure what you mean by "lose the semicolon")
    hidnplayr wrote:Keep up the good work! ;)
    Thanks! :)

    Regards,
    Shikhin
  • Shikhin wrote: Hrm. Is this correct:

    Code: Select all

    self_link:
            db ".", 0
    (not sure what you mean by "lose the semicolon")
    What I suggest is simply this:

    Code: Select all

    self_link     db ".", 0
    "Any intelligent fool can make things bigger, more complex, and more violent. It takes a touch of genius -- and a lot of courage -- to move in the opposite direction." Albert Einstein
  • Shikhin wrote:(not sure what you mean by "lose the semicolon")
    What he means is:
    self_link: is a label, used for JMP instructions
    self_link is a constant/variable
    Judging from db ".", 0 after self_link:, here you wanted it to be a constant/variable, so it has to be without semicolon.

    EDIT:
    Another comment - we usually use file extension ".inc" for included files, and file extension ".asm" only for the main file. It doesn't change anything, just makes it easier to locate the main file in the folder. So in your case, it would be probably better to have the following file names:

    Code: Select all

    /kernel/trunk/fs/ext2/ext2.asm
    /kernel/trunk/fs/ext2/blocks.inc
    /kernel/trunk/fs/ext2/ext2.inc
    /kernel/trunk/fs/ext2/inode.inc
    /kernel/trunk/fs/ext2/resource.inc
  • Some corrections.
    yogev_ezra wrote: self_link: is a label, used for JMP instructions
    self_link is a constant/variable
    self_link in self_link: db ".", 0 and in self_link db ".", 0 both are labels. But from self_link: the complier cannot recognize unit (byte, word, dword etc.) of a following data. The equivalent code is:

    Code: Select all

    label self_link
        db ".", 0
    while self_link db ".", 0 is equivalent to

    Code: Select all

    label self_link byte
        db ".", 0
  • Hi,

    D'oh. I think confusion on my part stemmed from the fact that you all referred to the colon as the semicolon. Anyway, I'll be sure to fix that.
    yogev_ezra wrote:Another comment - we usually use file extension ".inc" for included files, and file extension ".asm" only for the main file. It doesn't change anything, just makes it easier to locate the main file in the folder. So in your case, it would be probably better to have the following file names:

    Code: Select all

    /kernel/trunk/fs/ext2/ext2.asm
    /kernel/trunk/fs/ext2/blocks.inc
    /kernel/trunk/fs/ext2/ext2.inc
    /kernel/trunk/fs/ext2/inode.inc
    /kernel/trunk/fs/ext2/resource.inc
    I generally use .asm with anything that contains code, and .inc for all include files, but if that's the style KolibriOS follows, I'll fix it. :)

    Regards,
    Shikhin
  • Shikhin wrote:D'oh. I think confusion on my part stemmed from the fact that you all referred to the colon as the semicolon. Anyway, I'll be sure to fix that.
    My bad, looks like my english needs some work...
    "Any intelligent fool can make things bigger, more complex, and more violent. It takes a touch of genius -- and a lot of courage -- to move in the opposite direction." Albert Einstein
  • People found a bug (not sure whether it's part of your driver or not, but it is related to ext2/3/4 so I'll list it here): Linux SWAP partition is listed now, while it wasn't listed before. Our suggestion is not to list it at all, because you cannot (and should not be able to) access it anyway, so listing an inaccessible partition just confuses people. What do you think?
  • Hi,

    I've been struggling with a heisenbug in the Write syscall, so can't commit some stable code yet.
    yogev_ezra wrote:People found a bug (not sure whether it's part of your driver or not, but it is related to ext2/3/4 so I'll list it here): Linux SWAP partition is listed now, while it wasn't listed before. Our suggestion is not to list it at all, because you cannot (and should not be able to) access it anyway, so listing an inaccessible partition just confuses people. What do you think?
    AFAIK, SWAP partitions had a different type of partition type; that means they shouldn't have gone through. Of course, I'm not entirely sure; I'll look into this. Thanks! :)

    Regards,
    Shikhin
  • Hi,

    I'm struggling with a particularly irritating bug, which means that write gets corrupted. I'll tarball the code and upload to ftp soon, and try to fix this soon so I can put up my final report.

    Regards,
    Shikhin

    EDIT: since this is some last minute rushing, I thought I'd provide some updates. I think I've zeroed it down to the part where you get the block ID from the index in a inode's i_block array -- this part comes from before my time, so bah.

    EDIT: I fixed the problem in that function, and I think I even pushed the relevant part to ftp, but the bug is still there. Investigating, still.
  • Hi,

    Cheers to all!

    I was working on ext2{3,4} write support, for the KolibriOS Summer of Code. I'm pleased to announce that I've pushed all the code upstream. What I've achieved is refactoring all the ext2 code, splitting it up, adding "needs-own-module" code to kernel/trunk/fs/ext2.inc, working ext2 write support, merging of CleverMouse's change to use the new interface (that took some time, as it happened mid-way through my work), and bug fixes in the read driver. I had promised better ext{3,4} support, but, unfortunately, couldn't deliver much on that front (although, with a very limited feature-set of either, you could write still; more advanced features couldn't find proper support, as of now). I've added a TODO, as per the suggestion of yogev_ezra, in kernel/trunk/fs/ext2.inc.

    As for known issues, I have two, both of which I believe aren't directly related to my code:
    • SWAP partitions also get listed. Previously, with the old device-interface, there'd be a partition type, which, AFAIK, was gathered from the MBR. This was matched up against the ext2-partition type, and thus, a SWAP partition wouldn't get listed. With CleverMouse's changes, however, I saw that variable altogether removed. I couldn't figure out how to get the partition type, and, since the partition is a valid ext2 partition, it would correctly list it. I think CleverMouse would be able to handle this better than me.
    • While trying to copy, say, kolibri.img, a file new_kolibri.img is generated. This happens successfully. If you try to copy kolibri.img again, I see GetFileInfo getting a bogus request for a kolibri.lbl file, to which it returns "file not found." Eolite hence displays a small error, although, the write still takes perfectly fine. I couldn't find any such source in my code; I think the Eolite developer(s) can better handle this. I've left certain DEBUGF instructions, commented out, in kernel/trunk/fs/ext2.asm. Uncomment them, and you should get better information about how each syscall returns (and see the bogus request).

    The KolibriOS Summer of Code provided quite some experience to me, especially about participation in a big-scale open source project. I'd like to thank yogev_ezra, who guided me well, teaching me more about proper conversations with the team. As a minor list of acknowledgements, seeing as it calls for it, I'd like to thank my dad (who let me take part), and my friends nortti and Rishabh, whom I constantly bugged (pun intended), when I was trying to fix a bug (the latter of whom probably didn't understand much). I'd also like to thank sortiecat for helping me out with bugs. :)

    Regards,
    Shikhin
    Last edited by Shikhin on Mon Oct 21, 2013 5:18 pm, edited 1 time in total.
  • Shikhin wrote:SWAP partitions also get listed. Previously, with the old device-interface, there'd be a partition type, which, AFAIK, was gathered from the MBR. This was matched up against the ext2-partition type, and thus, a SWAP partition wouldn't get listed. With CleverMouse's changes, however, I saw that variable altogether removed. I couldn't figure out how to get the partition type, and, since the partition is a valid ext2 partition, it would correctly list it. I think CleverMouse would be able to handle this better than me.
    As per CleverMouse, this is not a bug, and it's up to each file manager to filter this disk out (or not). I don't completely agree with her - her statement is correct that it's not a bug, but KolibriOS can do nothing with this partition and trying to open it will always cause error, so it has to be handled somehow (maybe add a new kernel FS error message - "Error - cannot open Linux SWAP partition" or something like that).
    Shikhin wrote:While trying to copy, say, kolibri.img, a file new_kolibri.img is generated. This happens successfully. If you try to copy kolibri.img again, I see GetFileInfo getting a bogus request for a kolibri.lbl file, to which it returns "file not found." Eolite hence displays a small error, although, the write still takes perfectly fine. I couldn't find any such source in my code; I think the Eolite developer(s) can better handle this. I've left certain DEBUGF instructions, commented out, in kernel/trunk/fs/ext2.asm. Uncomment them, and you should get better information about how each syscall returns (and see the bogus request).
    This seems like a bug related to Mario's code: viewtopic.php?f=35&t=2319 . I have let him known in the chat about this. Let's see what he says.
  • By the way, a question: under which Linux user will KolibriOS access the disk (read/write/delete)? Is it "root" or what?
    (I just don't have a Linux machine right now to test it, so I thought it would be easier to ask you...)
  • Who is online

    Users browsing this forum: No registered users and 3 guests