[openbeosstorage] Re: BDiskSystem::Supports*()/Validate*()

On Sat, 26 Jul 2003 12:37:48 -0700 Tyler Dauwalder <tyler@xxxxxxxxxxxxx
> wrote:
> On 2003-07-26 at 07:16:41 [-0700], you wrote:
> > "Ingo Weinhold" <bonefish@xxxxxxxxxxxxxxx> wrote:
> > > And I also wouldn't like to have e.g. a 1 GB FS residing on a 2 
> > > GB
> > > partition without being able to get that info. What we could do, 
> > > is
> > > to
> > > add a BPartition::ContentSize(). Then DriveSetup can give the 
> > > user a
> > > very good picture of the situation. Thoughts?
> > 
> > Might be a good idea, yes. OTOH some file systems might have size
> > alignment requirements and cannot use the whole space - and if it
> > always says something about 0.3% free on that partition it could be
> > confusing for the user.
> > Also, that case should only very rarely happen, so it could be too 
> > much
> > of a good thing as well.
> 
> We could decide to only display the content size if it differs by an 
> empirically chosen significant amount. Even if it's theoretically 
> only 
> rarely needed, I think it would be a good idea to have the 
> ContentSize() feature in there if we're going to support 
> non-destructive enlarging even when the contents of the partition 
> cannot (and are not) enlarged.

OK, it's no problem to add it. The question is then, whether we want to 
allow resizing the contents individually 
(BPartition::ResizeContents(off_t)), or at least allow to pass both 
size and content size to Resize():

        status_t Resize(off_t size, off_t contentSize);

CanResize() and ValidateResize() would need to be adjusted accordingly. 
Against separating the features speaks, that it's more work for the API 
user, since they must take care of the order of applying the resize 
operations (contents, partition when shrinking, the other way around 
when enlarging).

> > > > >   bool CanResize(bool *canResizeContents, bool *whileMounted 
> > > > > =
> > > > > NULL);
> > > > 
> > > > I am not sure if I like that way of method calling, even if I 
> > > > might
> > > > be a bit late :)
> > > > I think it's strange that a CanResize() call needs and changes
> > > > boolean values.
> > > At least for all operations applicable on file systems the 
> > > respective
> > > Can*() methods takes a `whileMounted' parameter. Not required, 
> > > but
> > > optional. I think `canResizeContents' is not optional only for 
> > > the
> > > reason, that I can hardly imagine, why one would not be 
> > > interested 
> > > in
> > > that info.
> > 
> > True enough, but it would always create the need to have another
> > variable and check, even if what I want to know would be this:
> > bool canResizeContents;
> > if (CanResize(&canResizeContents) && canResizeContents) {
> >     ...
> > }
> > 
> > I think it would be clearer to write:
> > if ((ResizeCapabilities() & (CAN_RESIZE | CAN_RESIZE_CONTENTS)) != 
> > 0) 
> > {
> >     ...
> > }

Unfortunately not having the same semantics though. :-P
It would correctly read:
if (!(~ResizeCapabilities() & (CAN_RESIZE | CAN_RESIZE_CONTENTS))) {
        ...
}
(significantly longer constant names BTW ;-)

Looking less sexy I have to admit.

> > But if the contents of the partition are supposed to be going away
> > anyway, you could also just check:
> > if (CanResize()) {
> >     ...
> > }
> > without having to define an extra boolean parameter.

That would work with the current version as well, when providing a 
default parameter for `canResizeContents'. I didn't, because I assumed 
that one would always be interested in the full truth, but it's no big 
deal to change that.

> > OTOH for DriveSetup its use would probably nice, I have to admit:
> > 
> > bool canResizeContents;
> > if (CanResize(&canResizeContents)) {
> >     if (!canResizeContents)
> >         warnUserAndAbortIfHeWantsTo();
> > 
> >     // resize the partition
> >     ...
> > }

Yep.

> > > > I would understand a:
> > > > enum resize_capabilities {
> > > >     B_CAN_RESIZE_ITSELF = 1,
> > > >     B_CAN_RESIZE_CONTENTS = 2,
> > > >     B_RESIZE_WHILE_MOUNTED = 4
> > > > };
> > > > and:
> > > > bool CanResize();
> > > > resize_capabilities ResizeCapabilities();
> > > > 
> > > > better (or something to that extend). The functionality itself
> > > > looks
> > > > good, though :)
> > > In fact, CanResize() wouldn't be needed at all, since all 
> > > information
> > > would be provided by the second method. It is even less clear, 
> > > what
> > > it
> > > would return here -- whether the partition can be resized, or 
> > > it's
> > > contents, or both?
> > 
> > True, CanResize() wouldn't be needed anymore - yet the value it 
> > returns
> > is as confusing as in the current solution (IMO).
> 
> If we don't need CanResize(), then why not ditch it? The only problem 
> I 
> can see with ResizeCapabilities is that is doesn't match the CanXYZ() 
> naming scheme.

Yes, that's pretty much what I think too.

> > > > >   bool CanMove(bool *canMoveContents, bool *whileMounted = 
> > > > >   NULL);
> > > > 
> > > > same as above.
> > > 
> > > Now the method looks even as scary as this:
> > > 
> > >     bool CanMove(BObjectList<BPartition> *unmovableDescendants,
> > >                  BObjectList<BPartition> *movableOnlyIfUnmounted)
> > > const;
> > > 
> > > So you would think of a `bool CanMove() const' and a second 
> > > method
> > > (status_t GetMovingProperies(...)).
> > > 
> > > Looks OK in principle. I'm a bit undecided though. Tyler?
> > 
> > Oh, didn't know it's already that complicated :-)
> > But in which case does CanMove() return true? Only if
> > unmovableDescendants is empty? In my understanding, you cannot move 
> > a
> > partition when there is an unmovable descendant.
> 
> IIRC, the idea was to return whether or not the partition itself 
> could 
> be moved, separate from whether any of its children could be moved. 
> That information would be returned in the two BObjectList parameters. 
> The user then could choose to move only if all descendents allowed 
> moving as well, or to move regardless and destroy the data in any 
> child 
> partitions that do not allow moving. 
> 
> I'm not sure I see what would be gained by splitting CanMove() into 
> two 
> functions, though. If you really want to be able to get just the info 
> returned by CanMove(), why not allow NULL to be passed for the 
> BObjectLists and provide default parameters?

Same here, no big deal to do that.

CU, Ingo

Other related posts: