Re: Disk Device Manager quo vadis?

On 2007-07-22 at 21:34:13 [+0200], jan matějek <[email protected]> wrote:
> Ingo Weinhold napsal(a):
[...]
> >> In other news, i am now sick and in bed, so i have about a week's worth
> >> of free time. I could try and find out if i can build a gui partitioning
> >> client in that time.
> >
> > That would of course be nice, but don't sweat it. Rather get well. :-)
> 
> i've done a little bit of something, i'll clean it up a little and post
> it here or on haiku-development (as it isn't really kernel related, and
> i am trying to solving some usability problems)

Nice, thanks.

> > I had a first look through the patch. As our coding style police already
> > mentioned, there are some incompatibilities with our coding style. I will
> > clean that up while committing the code.
> >   
> i wonder, what kind of incompatibilities? we mostly tried to adhere to
> the coding style, i'd like to know what went wrong.
> (i know about problems in the client app, but we thought the kernel is
> "clean")

Some examples:

KSetNameJob::KSetNameJob(partition_id parentID, partition_id partitionID, 
const char * name, 
						 const char * contentName)
	: KDiskDeviceJob(B_DISK_DEVICE_JOB_SET_NAME, partitionID, parentID),
	fName( !name ? NULL : strcpy( new char[strlen(name)+1], name ) ),
	fContentName( !contentName ? NULL : strcpy( new char[strlen(contentName)+1], 
contentName ) )
{
	SetDescription( "setting name for the partition or its content" );
}

* The line length is limited to 80 columns. Our style guide is a bit vague in 
this respect as it says something about 80 - 90 columns, but in doubt a limit 
of 80 columns should be respected.

* Wrapped function parameter lists (formal and actual) shall be indented just 
one additional tab. In case of the constructor a second tab is acceptable for 
a better visual separation from the initializer list.

* There's no case where we use spaces to separate parentheses from their 
contents.

* Generally binary operators shall be separated from their operands by spaces.

* Not style related: "strcpy( new char[strlen(name)+1], name )": There's 
strdup(), which can be used in such a case. Furthermore this is bad, since it 
will crash when the memory could not be allocated.

status_t KSetNameJob::Do() {
	KDiskDeviceManager * manager = KDiskDeviceManager::Default();
	
[...]
			status_t set_cont_name_result = diskSystem->SetContentName(
					partition, fContentName, this);
			
			if( set_cont_name_result != B_OK ) {
				SetErrorMessage( "Setting name of the partition's content failed!" );
			}
			return set_cont_name_result;
			
		}
		
		
	} else {
		SetErrorMessage( "Couldn't find partition!" );
		return B_ENTRY_NOT_FOUND;
	}
			
			
}

bool KDiskDeviceJob::isPartitionNotBusy( KPartition * partition ) {
	if( !partition ) {
		return false; 
	}
	
	return partition->VisitEachDescendant(&fNotBusyVisitor);
}

* In a function definition the return type + modifiers shall be on a separate 
line, or in other words the fully annotated function name shall begin on a 
new line.

* The name of a public/protected function starts with an upper case 
character. For private functions an underscore ("_") is prepended.

* In a function definition the brace ("{") opening the function block lives 
on a separate line. This generally also holds true for blocks not belonging 
to an "if", "for", "while", "do" construct, i.e. for those that are merely 
being used for variable scoping.

* Don't use uncommon abbreviations in identifier names (e.g. "cont" in 
"set_cont_name_result").

* Local variables are camel case, e.g. "setContentNameResult".

* "if", "for", and "while" are separated by space from the following 
parenthesis. The spacing within the parentheses is as written above.

* Don't overdo line spacing. There are generally no blank lines before a "}". 
Exceptions can be made where it improves readability, e.g. when one has 
several "else if" blocks and wants to separate them visually.

> > Some files are missing copyright headers, other contain "Copyright: See
> > COPYING file that comes with this distribution", which, I assume, means 
> > the
> > MIT license we use for the project. I'd like to add/change the copyright
> > headers so that they comply with our standard header. It would be nice, if
> > you could tell me who was responsible for implementing which part, so that
> > I can credit the authorship appropriately, where things are unclear.
> >   
> Tomáš Kučera (Tomas Kucera) is exclusively responsible for the
> partitioning add-on. He also documented the addon interface class in
> DiskDeviceManager
> Luboš Kulič (Lubos Kulic) is the author of new jobs and the client app.
> i, Jan Matějek (Jan Matejek), did the rest - necessary changes in
> DiskDeviceManager and related classes, tinkering with new and existing
> jobs, overview documentation.

Thanks.

> > I'd also like to add you guys to the contributors list in our AboutSystem
> > application, unless you object, of course.
> >   
> i don't have any problem with that. i'll ask my colleagues but i doubt
> they would object

OK.

> > Apart from that, I've seen some issues in the DDM (jobs) code, that I'd
> > like to solve differently. I'll have to re-familiarize myself with the DDM
> > and look into the patch more thouroughly first, though.
> >   
> please let me know about that, i am still working on it (as i wrote,
> there are things still missing)

I think the two major issues are:

* When a partition is busy, a job for it should not just fail, but wait until 
the partition becomes unbusy again.

* The notification mechanism for kernel and userland is missing. And the DDM 
needs to listen for certain events itself (device added/removed, disk system 
added/removed).

Please keep me posted, what you're working on.

> > I will commit things in smaller chunks over the next days -- I'll see when
> > I find time. If you want me to refrain from adjusting the coding style of
> > the partitioning app, so you don't get unnecessary conflicts while you're
> > working on it, I can as well commit it verbatim for the time being.
> >   
> i am dropping the existing application entirely, as it is no good, and
> starting from scratch. maybe it isn't necessary to commit the existing
> code at all.

OK, I'll commit the other stuff first. Then we'll see how far you got.

CU, Ingo

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >>  http://get.splunk.com/
_______________________________________________
Open-beos-kernel-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/open-beos-kernel-devel