FreeLists / openbeosstorage / [openbeosstorage] Re: DiskDevice API 2.2 remarks

["Ingo Weinhold" <bonefish@xxxxxxxxxxxxxxx>]

Tyler Dauwalder <tyler@xxxxxxxxxxxxx> wrote:
[...]
> > > BDiskDisk:
> > > ---------
> > > 
> > > * What meaning has the `checkOnly' flag of SupportsRepairing()=3F 
> > > Same
> > > in
> > > BPartition::[Can]Repair().
> 
> Well, I was thinking of offering the ability to make read-only repair 
> runs, i.e. verification runs. I'm not sure that's really necessary 
> though.

Ah, OK. Doesn't harm at least. BTW, the idea is to send encountered 
errors via notifications? I hope that doesn't have impact on 
performance. Although the FS could try to bundle subsequent errors 
messages in one notification message (like `Blocks 123456 to 198235 
contain a funny potpourri of garbage. Better start searching your 
backup.').

> > > BPartition:
> > > ----------
> > > 
> > > * VisitSubtree(): I thought, we agreed on VisitEachDescendent()=
> > > 3F
> 
> I think I was playing around with that after taking out 
> CountDescendents(), and forgot about it. You like it less, I take it?

Yes. Since we have VisitEachChild(), VisitEachDescendant() would be 
consequent.

[...]
> > > * CreateChild(): I'd add a `BPartition **child' parameter for
> > > convenience.
> 
> Hmmmm, okay.

We can make it a `= NULL' default parameter, so one doesn't have to 
supply it. It's just, that you can't be sure, at which index the new 
partition ends up.

> > I wonder, why parameters aren't needed anymore.
> 
> Because CreateChild() always creates uninitialized partitions.

Right, but nevertheless the parameters the parent disk system 
associates with the partition (as opposed to the content parameters, 
which are indeed not needed when creating an uninitialized partition) 
are needed. I would allow NULL as argument, in which case the 
partitioning system has to use defaults.

[...]
> > > It may be better to introduce
> > > [Can]Delete() instead (or additionally).
> 
> Well, I had those in there, but I felt they fell under the same 
> argument as the one use for eliminating 
> BPartitionableSpace::CreatePartition(), i.e. the BPartition is 
> suddenly 
> going to disappear or at least be invalid.

Although at least the name Delete() should be hint enough for the user 
to not be too surprised. ;-) However, since one can only delete child 
partitions (not devices) DeleteChild() is probably better. Maybe we 
should add an additional DeleteChild(BPartition *child) version.

> > > Now, I finally know, what bothered me a bit. A partition actually 
> > > has
> > > two types, an outer and inner type. The former one is assigned by 
> > > the
> > > parent partition, while the latter one describes the contents. 
> > > The
> > > same
> > > holds, of course, for the parameters. I wonder, if we should make
> > > that
> > > explicit in the API and have Type() and ContentType(). That way 
> > > we
> > > could also drop the BDiskDevice::DeviceType(). 
> > > CanEditParameters(),
> > > GetParameterEditor() and SetParameters() would each get a second,
> > > `Content', version.
> > > What do you think?
> 
> I'm not sure yet. :-) I think I was hoping to hide most of the 
> parent-level type info away, but I didn't know and still don't know 
> that that's feasible. What's your take on that?

Actually, I don't think it is. Same with parameters + content 
parameters.

> If not, I do really 
> like the Type() and ContentType() route.

OK, then let's go for it.

CU, Ingo