Board.KolibriOS.org

Official KolibriOS board
It is currently Fri Jul 19, 2019 12:39 am

All times are UTC+03:00




Post new topic  Reply to topic  [ 92 posts ]  Go to page Previous 1 2 3 4 57 Next
Author Message
 Post subject: Re: ext2
PostPosted: Wed Sep 25, 2013 1:26 pm 
Offline
Public Relations
User avatar

Joined: Mon Jun 07, 2010 12:01 pm
Posts: 1879
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 :-)


Top
   
 Post subject: Re: ext2
PostPosted: Sat Sep 28, 2013 2:23 pm 
Offline
KSoC/GSoC Student

Joined: Wed Jun 05, 2013 4:22 pm
Posts: 17
Hi,

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

Regards,
Shikhin


Top
   
 Post subject: Re: ext2
PostPosted: Sat Sep 28, 2013 8:20 pm 
Offline
Mentor/Kernel Developer
User avatar

Joined: Fri Jun 30, 2006 9:01 am
Posts: 1247
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:
        add     eax, 3054539008                         ; (369 * 365 + 89) * 24 * 3600

First, why did you calculate this number? The following code perfectly works in FASM too:
Code:
        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:
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


Top
   
 Post subject: Re: ext2
PostPosted: Sat Sep 28, 2013 9:56 pm 
Offline
KSoC/GSoC Student

Joined: Wed Jun 05, 2013 4:22 pm
Posts: 17
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:
        add     eax, 3054539008                         ; (369 * 365 + 89) * 24 * 3600

First, why did you calculate this number? The following code perfectly works in FASM too:
Code:
        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:
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:
self_link:
        db ".", 0


(not sure what you mean by "lose the semicolon")

hidnplayr wrote:
Keep up the good work! ;)


Thanks! :)

Regards,
Shikhin


Top
   
 Post subject: Re: ext2
PostPosted: Sun Sep 29, 2013 11:02 am 
Offline
Mentor/Kernel Developer
User avatar

Joined: Fri Jun 30, 2006 9:01 am
Posts: 1247
Shikhin wrote:
Hrm. Is this correct:

Code:
self_link:
        db ".", 0


(not sure what you mean by "lose the semicolon")


What I suggest is simply this:
Code:
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


Top
   
 Post subject: Re: ext2
PostPosted: Sun Sep 29, 2013 2:37 pm 
Offline
Public Relations
User avatar

Joined: Mon Jun 07, 2010 12:01 pm
Posts: 1879
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:
/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


Top
   
 Post subject: Re: ext2
PostPosted: Sun Sep 29, 2013 6:23 pm 
Offline

Joined: Thu Feb 19, 2009 12:57 pm
Posts: 68
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:
label self_link
    db ".", 0

while self_link db ".", 0 is equivalent to
Code:
label self_link byte
    db ".", 0


Top
   
 Post subject: Re: ext2
PostPosted: Sun Sep 29, 2013 6:31 pm 
Offline
KSoC/GSoC Student

Joined: Wed Jun 05, 2013 4:22 pm
Posts: 17
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:
/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


Top
   
 Post subject: Re: ext2
PostPosted: Sun Sep 29, 2013 7:51 pm 
Offline
Mentor/Kernel Developer
User avatar

Joined: Fri Jun 30, 2006 9:01 am
Posts: 1247
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


Top
   
 Post subject: Re: ext2
PostPosted: Sun Oct 06, 2013 5:45 pm 
Offline
Public Relations
User avatar

Joined: Mon Jun 07, 2010 12:01 pm
Posts: 1879
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?


Top
   
 Post subject: Re: ext2
PostPosted: Mon Oct 07, 2013 12:28 pm 
Offline
KSoC/GSoC Student

Joined: Wed Jun 05, 2013 4:22 pm
Posts: 17
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


Top
   
 Post subject: Re: ext2
PostPosted: Fri Oct 18, 2013 9:22 pm 
Offline
KSoC/GSoC Student

Joined: Wed Jun 05, 2013 4:22 pm
Posts: 17
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.


Top
   
 Post subject: ext2 write
PostPosted: Mon Oct 21, 2013 1:20 pm 
Offline
KSoC/GSoC Student

Joined: Wed Jun 05, 2013 4:22 pm
Posts: 17
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.

Top
   
 Post subject: Re: ext2 write
PostPosted: Mon Oct 21, 2013 2:01 pm 
Offline
Public Relations
User avatar

Joined: Mon Jun 07, 2010 12:01 pm
Posts: 1879
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.


Top
   
 Post subject: Re: ext2
PostPosted: Tue Oct 22, 2013 5:09 pm 
Offline
Public Relations
User avatar

Joined: Mon Jun 07, 2010 12:01 pm
Posts: 1879
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...)


Top
   
Display posts from previous:  Sort by  
Post new topic  Reply to topic  [ 92 posts ]  Go to page Previous 1 2 3 4 57 Next

All times are UTC+03:00


Who is online

Users browsing this forum: No registered users and 1 guest


You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot post attachments in this forum

Search for:
Jump to:  
Powered by phpBB® Forum Software © phpBB Limited