Page 2 of 7

Re: ext2

Posted: Wed Sep 25, 2013 1:26 pm
by yogev_ezra
Shikhin wrote:P.S. @yogev_ezra -- since my account had an influx of money from you, I was hoping I could get some sort of a statement saying "We gave $2250 to Shikhin Sethi, amounting to 130600INR, on <date-you-send-payment>, for the Summer of Code 2013" or something similar, so that I can avoid problems with the tax department?
Sorry, I am on 10-day vacation abroad right now, so can't really help you until I return. I will do that ASAP when I am back home.
Shikhin wrote:Following my report from yesterday: I've put up 25.patch and 25.img. 25.img is the kolibri.img with the latest kernel build; note that CreateFolder now works too, so have fun with testing it. Rewrite should be easy to do now, but I think it requires Write too, so I'll do those together. I'd love all sort of code reviews, so do look at everything.
Unless anyone publicly expresses an objection within 72 hours from now, I think you should commit all your code to trunk - that seems the easiest way to force people do testing in KolibriOS :-)

Re: ext2

Posted: Sat Sep 28, 2013 2:23 pm
by Shikhin
Hi,

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

Regards,
Shikhin

Re: ext2

Posted: Sat Sep 28, 2013 8:20 pm
by hidnplayr
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! ;)

Re: ext2

Posted: Sat Sep 28, 2013 9:56 pm
by Shikhin
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

Re: ext2

Posted: Sun Sep 29, 2013 11:02 am
by hidnplayr
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

Re: ext2

Posted: Sun Sep 29, 2013 2:37 pm
by yogev_ezra
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

Re: ext2

Posted: Sun Sep 29, 2013 6:23 pm
by Insolor
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

Re: ext2

Posted: Sun Sep 29, 2013 6:31 pm
by Shikhin
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

Re: ext2

Posted: Sun Sep 29, 2013 7:51 pm
by hidnplayr
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...

Re: ext2

Posted: Sun Oct 06, 2013 5:45 pm
by yogev_ezra
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?

Re: ext2

Posted: Mon Oct 07, 2013 12:28 pm
by Shikhin
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

Re: ext2

Posted: Fri Oct 18, 2013 9:22 pm
by 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.

ext2 write

Posted: Mon Oct 21, 2013 1:20 pm
by Shikhin
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

Re: ext2 write

Posted: Mon Oct 21, 2013 2:01 pm
by yogev_ezra
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.

Re: ext2

Posted: Tue Oct 22, 2013 5:09 pm
by yogev_ezra
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...)