Re: [patch] ped_device_from_store

[Neal H Walfield]

> > My rational is that the resource is already open and, as of right now,
> > the resource is destoryed when the open_count goes to 0.
> 
> Well, this can be changed.

This would be good.

> > Since the
> > resource is consumable, the other option will destroy it even before it
> > leaves ped_device_new_from_store.
> 
> Unless you change it.

You mean the code?

> > 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.  Look at the currnt scheme
in do_select, you do a ped_device_new (via command_line_get_device).
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.  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.  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.

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.

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.

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.

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

Huh?

pgpdBVaRWFnAz.pgp
Description: PGP signature