Board.KolibriOS.org

Официальный форум KolibriOS
Текущее время: Пн сен 25, 2017 1:59 am

Часовой пояс: UTC+03:00




Начать новую тему  Ответить на тему  [ 78 сообщений ]  На страницу Пред. 1 2 3 4 5 6 След.
Автор Сообщение
 Заголовок сообщения: Re: ext2
СообщениеДобавлено: Ср сен 25, 2013 1:26 pm 
Не в сети
Public Relations
Аватара пользователя

Зарегистрирован: Пн июн 07, 2010 12:01 pm
Сообщения: 1879
Shikhin писал(а):
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 писал(а):
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
СообщениеДобавлено: Сб сен 28, 2013 2:23 pm 
Не в сети
KSoC/GSoC Student

Зарегистрирован: Ср июн 05, 2013 4:22 pm
Сообщения: 17
Hi,

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

Regards,
Shikhin


Вернуться к началу
 Заголовок сообщения: Re: ext2
СообщениеДобавлено: Сб сен 28, 2013 8:20 pm 
Не в сети
Mentor/Kernel Developer
Аватара пользователя

Зарегистрирован: Пт июн 30, 2006 9:01 am
Сообщения: 1224
Shikhin писал(а):
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:
Код:
        add     eax, 3054539008                         ; (369 * 365 + 89) * 24 * 3600

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


Вернуться к началу
 Заголовок сообщения: Re: ext2
СообщениеДобавлено: Сб сен 28, 2013 9:56 pm 
Не в сети
KSoC/GSoC Student

Зарегистрирован: Ср июн 05, 2013 4:22 pm
Сообщения: 17
Hi,

hidnplayr писал(а):
Well, I've just had a look and everything looks neat and well organized, so kudos for that!


Thanks!

hidnplayr писал(а):
Some small remarks:
In ext2.asm, I found this:
Код:
        add     eax, 3054539008                         ; (369 * 365 + 89) * 24 * 3600

First, why did you calculate this number? The following code perfectly works in FASM too:
Код:
        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 писал(а):
At the bottom of the same file I found this:
Код:
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:

Код:
self_link:
        db ".", 0


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

hidnplayr писал(а):
Keep up the good work! ;)


Thanks! :)

Regards,
Shikhin


Вернуться к началу
 Заголовок сообщения: Re: ext2
СообщениеДобавлено: Вс сен 29, 2013 11:02 am 
Не в сети
Mentor/Kernel Developer
Аватара пользователя

Зарегистрирован: Пт июн 30, 2006 9:01 am
Сообщения: 1224
Shikhin писал(а):
Hrm. Is this correct:

Код:
self_link:
        db ".", 0


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


What I suggest is simply this:
Код:
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


Вернуться к началу
 Заголовок сообщения: Re: ext2
СообщениеДобавлено: Вс сен 29, 2013 2:37 pm 
Не в сети
Public Relations
Аватара пользователя

Зарегистрирован: Пн июн 07, 2010 12:01 pm
Сообщения: 1879
Shikhin писал(а):
(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:
Код:
/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
СообщениеДобавлено: Вс сен 29, 2013 6:23 pm 
Не в сети

Зарегистрирован: Чт фев 19, 2009 12:57 pm
Сообщения: 69
Some corrections.
yogev_ezra писал(а):
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:
Код:
label self_link
    db ".", 0

while self_link db ".", 0 is equivalent to
Код:
label self_link byte
    db ".", 0


Вернуться к началу
 Заголовок сообщения: Re: ext2
СообщениеДобавлено: Вс сен 29, 2013 6:31 pm 
Не в сети
KSoC/GSoC Student

Зарегистрирован: Ср июн 05, 2013 4:22 pm
Сообщения: 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 писал(а):
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:
Код:
/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
СообщениеДобавлено: Вс сен 29, 2013 7:51 pm 
Не в сети
Mentor/Kernel Developer
Аватара пользователя

Зарегистрирован: Пт июн 30, 2006 9:01 am
Сообщения: 1224
Shikhin писал(а):
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


Вернуться к началу
 Заголовок сообщения: Re: ext2
СообщениеДобавлено: Вс окт 06, 2013 5:45 pm 
Не в сети
Public Relations
Аватара пользователя

Зарегистрирован: Пн июн 07, 2010 12:01 pm
Сообщения: 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?


Вернуться к началу
 Заголовок сообщения: Re: ext2
СообщениеДобавлено: Пн окт 07, 2013 12:28 pm 
Не в сети
KSoC/GSoC Student

Зарегистрирован: Ср июн 05, 2013 4:22 pm
Сообщения: 17
Hi,

I've been struggling with a heisenbug in the Write syscall, so can't commit some stable code yet.

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?


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
СообщениеДобавлено: Пт окт 18, 2013 9:22 pm 
Не в сети
KSoC/GSoC Student

Зарегистрирован: Ср июн 05, 2013 4:22 pm
Сообщения: 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.


Вернуться к началу
 Заголовок сообщения: ext2 write
СообщениеДобавлено: Пн окт 21, 2013 1:20 pm 
Не в сети
KSoC/GSoC Student

Зарегистрирован: Ср июн 05, 2013 4:22 pm
Сообщения: 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


Последний раз редактировалось Shikhin Пн окт 21, 2013 5:18 pm, всего редактировалось 1 раз.

Вернуться к началу
 Заголовок сообщения: Re: ext2 write
СообщениеДобавлено: Пн окт 21, 2013 2:01 pm 
Не в сети
Public Relations
Аватара пользователя

Зарегистрирован: Пн июн 07, 2010 12:01 pm
Сообщения: 1879
Shikhin писал(а):
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 писал(а):
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
СообщениеДобавлено: Вт окт 22, 2013 5:09 pm 
Не в сети
Public Relations
Аватара пользователя

Зарегистрирован: Пн июн 07, 2010 12:01 pm
Сообщения: 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...)


Вернуться к началу
Показать сообщения за:  Поле сортировки  
Начать новую тему  Ответить на тему  [ 78 сообщений ]  На страницу Пред. 1 2 3 4 5 6 След.

Часовой пояс: UTC+03:00


Кто сейчас на конференции

Сейчас этот форум просматривают: нет зарегистрированных пользователей и 1 гость


Вы не можете начинать темы
Вы не можете отвечать на сообщения
Вы не можете редактировать свои сообщения
Вы не можете удалять свои сообщения
Вы не можете добавлять вложения

Найти:
Перейти:  
Создано на основе phpBB® Forum Software © phpBB Limited
Русская поддержка phpBB