Re: [patch] ped_device_from_store

[Andrew Clausen]

On Thu, Aug 16, 2001 at 02:48:50PM +0200, Neal H Walfield wrote:
> > > If so, then I fail to see the continued utility
> > > of ped_device_open/ped_device_close.  Actually, as far as I can tell,
> > > right now, they really are not open and close meathods, but rather, a
> > > reference counting system that you use to safely free nonvolatile
> > > resources (e.g. you can get another file descriptor since you save the
> > > path to the block device).
> > 
> > Correct.  It allows us to free resources, if they aren't being used
> > by libparted.  However, we can't free these special resources (and
> > get them back), so they should only be destroyed on ped_device_destroy().
> 
> Free resources, what does this mean?  You are trying to do reference
> counting and yet, you are calling it opening.

It's not ref-counting, it's use-counting.  This isn't really the
same thing as opening, agreed.

> Look at the currnt scheme
> in do_select, you do a ped_device_new (via command_line_get_device).A

ped_device_get() (minor detail...)

> Next, you explicitly add a reference (i.e. open_status ++) using
> ped_device_open.  You then ped_device_close the old device (to drop the
> user reference).  There is no complement to ped_device_new.

You mean to ped_device_get().  This is intentional.  I wanted there
to be a list of all devices available.

> And this in
> the only time that you use ped_device_open (except in _choose_device,
> but that is just a modified do_select); this is really an internal
> function.  In fact, you do not even need to call ped_device_open here,
> this is only a speed optimization and, therefore, a hack -- you are
> using the internal reference counting system.

Hmmm... maybe it should be moved into do_*(), and I should make
all parted commands atomic.  (I.e. re-read partition table, flush
cache, etc.)

> In my opinion, what you
> should really be doing is something like the following:
> 
> do_select ()
> {
>       ...
> 
>       new = command_line_get_device () /* i.e. ped_device_new wrapper */
>       if (! new)
>               return 0;
>       
>       ped_device_destroy (*dev);
>       *dev = new;
> 
>       ...
> }
> 
> ped_device_destroy would then drop the _user_ reference (i.e. not the
> libparted reference) to 0.  Maybe names such as ped_device_use and
> ped_device_done would make the intent clearer.

I have no objections to that.

> Either way, I really think that you are trying to implement unix style
> vtables and are just missing this because there is no (or easy
> globalized) peropen state.

Nah... peropen state is bad IMHO.  It is a vtable, but not unix style,
intentionally.

> Consider the following:  you are using a private name space that is
> maintained via the _device_register and _device_unregister functions
> (which turn system filenames into local inodes).  You access these
> filenames using ped_device_{get,new}.  The PedDevice names the resource.
> And then, you stop there and allow the kernel space (libparted) and user
> space (parted) resources to mingle and confuse the issue.

I'm not trying to model Unix here.  We don't need per-open metadata.

> Allow me to propose to you the following:  I want a peropen hook for
> whatever reason (and I think that there are a variety of good reasons to
> do this).  I would like to be able to do something like this:
> 
>       PedDeviceName *dev;
>       PedDeviceOpen *po;
> 
>       /* Map filename from global to local namespace.  */
>       dev = ped_device_get ("/dev/hda");
>       if (! dev)
>               return NULL;
> 
>       /* Open it so that we can call functions  */
>       po = ped_device_open (dev);
>       if (! po)
>               return NULL;
>       
>       /* Set local state.  */
>       po->hook = ...
> 
> Now, PedDeviceName is completely opaque (current public data should now
> be accessed via meathods).  Then, PedDeviceOpen might look like this:
> 
>       struct PedDeviceOpen
>       {
>               PedDeviceName *dev;
>               void *hook;
>       };
> 
> And PedDeviceName can do user reference counting, i.e. the number of
> user opens.  Additionally, this scheme  makes libparted reference
> counting (mostly) obsolete because, you cannot pass a PedDeviceOpen that
> you have called ped_device_close on.  But maybe, we do not want to do
> that and allow a user to open a resource, launch a thread, and then
> close the resource and let the thread continue.  This would be like
> creating a file, opening and deleteing the file.  The vnode still
> exists, however, the file is now gone.

What's the motivation for all this?  What's wrong with ped_device_open()
as it stands?  Do you actually plan to use PedDeviceOpen.hook?

> > Anyway, I like ped_device_close() being equivalent to ped_device_destroy()
> > is an even worse change (read: violation) of semantics.
> 
> Huh?

s/^Anyway, I/Anyway, I don't/.  Sorry.

Andrew