> [...] > > > > > > As written in KDiskDeviceJob.h, I'm not sure if we want to > > > > > > add a > > > > > > Cancel() method to cancel a job in progress. Scheduled jobs > > > > > > can > > > > > > easily > > > > > > be canceled by removing them from the job queue. I just > > > > > > realize, > > > > > > that > > > > > > we don't provide any methods in the userland API for > > > > > > canceling a > > > > > > job. > > > > > Yes, I think we should support cancel, for things like > > > > > defragging > > > > > that > > > > > can be interrupted. But we should also allow the job to > > > > > specify > > > > > if > > > > > it > > > > > allows cancelling or not. > > > > > > Don't know, if that's necessary. Jobs, that can't be cancelled, > > > will > > > simply finish their task. > > > > So are you proposing we just always display cancellation as an > > option, > > and if the task doesn't support it, things just keep quietly > > plugging > > away when you click the "Cancel" button? I think we should offer the > > user some indication of which tasks can be canceled gracefully and > > which can't. > > OK, why not. So, how about adding a uint32 > BDiskDeviceJob::CancelProperties(), which would return a combination > of > these: > > enum { > B_DISK_DEVICE_JOB_CAN_CANCEL = 0x01, > B_DISK_DEVICE_JOB_STOP_ON_CANCEL = 0x02, > B_DISK_DEVICE_JOB_REVERSE_ON_CANCEL = 0x04, > }; > > The cancel method would have parameters `bool force' and `bool > reverse'. It wouldn't have any effect, when > B_DISK_DEVICE_JOB_CAN_CANCEL is not set. If `force' is true the > `reverse' parameter is ignored and the job is told to quit as soon as > possible, even, if that means, that the partition will be left in an > invalid state. Otherwise `reverse' indicates whether the effect of the > job shall be undone. > > CancelProperties() would be live. It could change during the execution > of the job (e.g. repairing might have a read-only analysis phase that > can simply be canceled, while canceling the repairing phase might > leave > the partition in an invalid state). Okay, works for me. > > commit_disk_device_modifications(device, ...); > > // this will finally trigger the modifications to be made > > // and unlock the API - or will it first process all jobs > > // and the unlock the API? > > It will compare the original with the shadow hierarchy and create and > schedule the jobs needed to transform the original hierarchy > respectively. The shadow hierarchy is deleted, and all partitions that > might be affected by any scheduled job are marked `busy', all > ancestors > of `busy' partitions are marked `descendant-busy'. When the syscall > returns, a subsequent prepare_disk_device_modifications() on that disk > device will succeed -- only partitions that are not marked `*busy' are > allowed to be modified then, though. > > > Where do we need the shadow partition name anyway? It doesn't seem > > to > > be part of the user API. > > You mean KPartition::Name()? If one sets the name of a partition via > the userland API, it should also be reflected in the partition data, I > think. We currently don't have the functionality to set the name of a > partition in the userland API. We may want to add it, though. Maybe we > also want to discriminate between Name() and ContentName() (e.g. > partition name vs. FS/volume name). Yes, we probably should allow both, since they both have accessors already. > > So the user will never > > come across all those user_ prefixes - because if he would, I would > > consider dumping them. > > I introduced the prefix for partition_data and disk_device_data, > because there're equally named structures for the partition module/FS > add-on interfaces and KPartition/KDiskDevice will probably have to > deal > with both. For consistency I added he prefix to the other userland > structures as well. > > > And if we have all these int32 IDs we could think about adding a new > > type for them, like partition_id. > > Good idea. What about IDs for disk systems and disk device jobs? > disk_system_id, disk_device_job_id? Too long? How about disk_job_id? Or just job_id, unless you're worried that's too general. > > We might also add a "name" field to initialize_partition() since > > almost > > all disk systems share this property (IIRC only Intel style > > partitioning doesn't have it). > > If we do that, we should also make the functionality to set the name > explicit (or even `name' and `content name'). Might be a good idea, since it's a pretty ubiquitous concept. And particularly since Tracker lets you do this by just renaming a volume. > > As you mentioned, the only problem which we would still have is with > > the image files. Though I would really like us to be able to simply > > register/publish them as a device, it would also be nice to create a > > file system on them without having to register them. > > For example, that would be the use for the fs_shell, but perhaps > > also > > other things, although I don't have a good idea right now. > > Or to say it this way: I like to have no real differentiation > > between > > block devices and files - they are almost the same from the OS point > > of > > view, why shouldn't we keep this? Registering a file as device would > > be > > certainly a step into the right direction - but it would be even > > nicer > > if that wouldn't be necessary at all. > > We would also need to differentiate between file system images and > > disk > > images - but I guess we're doing this anyway. > > > > Well, I think it would be okay to not consider the fs_shell case, > > but > > to have mkfs register files automatically - the only thing we should > > support, though, is that we should continue to be able to address > > this > > file using its standard path. I.e. something like: > > $ mkfs test.image > > $ makebootable test.image > > > > Shouldn't fail even if "makebootable" needs a device to operate on. > > Dunno how we should do that right now, though. Maybe it's not even a > > top priority - but it'd be nice to have it. > > OK, then how about the following additions to BDiskDeviceRoster? > > partition_id RegisterDeviceFile(const char *filename); > // publishes: /dev/disk/virtual/files/<disk device ID>/raw > status_t UnregisterDeviceFile(const char *filename); > status_t UnregisterDeviceFile(partition_id device); > > partition_id GetDeviceForPath(const char *filename, BDiskDevice * > device, > bool registerIfFile = false); > partition_id GetPartitionForPath(const char *filename, BDiskDevice * > device, > BPartition **partition, > bool registerIfFile = false); > > mkfs (or fs_initialize_volume(), if we implement it using the > DiskDevice API) would only have to call one method to get hold of the > BPartition in questions. So, that should be rather convenient, I > suppose. Seems okay to me. :-) > > > > > It could be nice to have this, and it would also be a bit > > > > > faster. > > > > > I > > > > > am not sure if it's rendering the API inconsistent there, > > > > > though. > > > > > One > > > > > could argue that the partition related functions (not > > > > > initialize()) > > > > > are a bit separated from the rest anyway. > > > > Either way would work for me. So, if you have any preferences... > > > I prefer passing the device. :-) > > > > Okay, so let's do that, then :-) > > So, then it's a FD plus the ID of the partition (something is needed > to > identify it), I suppose. Yes to the former, and yes to the latter where it's needed (the scanning functions could get away with the the FD, as the partition scanning functions do, couldn't they?). -Tyler