[haiku-commits] Re: r34967 - in haiku/trunk: headers/private/kernel/fs src/system/kernel/device_manager src/system/kernel/disk_device_manager src/tests/add-ons/kernel/kernelland_emu
- From: Ingo Weinhold <ingo_weinhold@xxxxxx>
- To: haiku-commits@xxxxxxxxxxxxx
- Date: Sat, 09 Jan 2010 22:09:35 +0100
On 2010-01-09 at 13:53:22 [+0100], Michael Lotz <mmlr@xxxxxxxx> wrote:
> > On 2010-01-09 at 04:55:39 [+0100], mmlr@xxxxxxxx wrote:
> >> Author: mmlr
> >> Date: 2010-01-09 04:55:38 +0100 (Sat, 09 Jan 2010)
> >> New Revision: 34967
> >> Changeset: http://dev.haiku-os.org/changeset/34967/haiku
> >> Ticket: http://dev.haiku-os.org/ticket/4587
> >>
> >> Modified:
> >> haiku/trunk/headers/private/kernel/fs/devfs.h
> >> haiku/trunk/src/system/kernel/device_manager/devfs.cpp
> >> haiku/trunk/src/system/kernel/disk_device_manager/KPartition.cpp
> >> haiku/trunk/src/tests/add-
> > > ons/kernel/kernelland_emu/device_manager.cpp
> >> Log:
> >> ..
> > > of these left behind nodes, causing bug #4587. Seeing that this
> > > code is more
> > > compact and straight forward anyway I don't quite see why it was
> > > changed in
> > > the
> > > first place.
> >
> > Well more compact in the devfs, less compact in KPartition. Most
> > importantly
> > though, UnpublishPartition() wouldn't need memory and thus be fail-
> > safe,
> > which is always a good thing in code that frees resources.
>
> I understand that this is desireable, but I don't see how this could
> work otherwise. It requires the raw device node, which is the one being
> removed first. Of course logic could be implemented to unpublish the
> other nodes manually before the raw node is removed, but I think that's
> not really nicer than this.
No, you're right, that's not so nice, and the previous implementation
hasn't ever worked for removable devices. It was written from the disk
device API user perspective only (like removing partitions).
Anyway, devfs_unpublish_partition() could still be implemented with the
previous signature -- thus not requiring the caller to use operations that
can fail. Since the KDiskDevice is write locked at that point, it might be
possible to (somewhat hackily) turn the device path temporarily into the
directory path, so that even directory path + entry name could be provided
instead.
Alternatively KPartition could store the full path under which it is
published instead of just the name, but that wastes quite a bit of memory.
> > [...]
> > > Modified:
> > > haiku/trunk/src/system/kernel/disk_device_manager/KPartition.cpp
> > > ===================================================================
> > > --- haiku/trunk/src/system/kernel/disk_device_manager/KPartition.
> > > cpp
> >> 2010-01-09 02:21:22 UTC (rev 34966)
> >> +++ haiku/trunk/src/system/kernel/disk_device_manager/KPartition.cpp
> >> 2010-01-09 03:55:38 UTC (rev 34967)
> >> @@ -232,9 +232,16 @@
> >> if (!fPublishedName)
> >> return B_OK;
> >>
> >> - status_t error = devfs_unpublish_partition(Device()->Path(),
> >> - fPublishedName);
> >> + // get the path
> >> + KPath path;
> >> + status_t error = GetPath(&path);
> >
> > This could fail due to memory shortage, and...
> >
> >> if (error != B_OK) {
> >> + dprintf("KPartition::UnpublishDevice(): Failed to get path
> > > for "
> >> + "partition %ld: %s\n", ID(), strerror(error));
> >> + }
> >> +
> >> + error = devfs_unpublish_partition(path.Path());
> >
> > ... this would be called with an empty path.
>
> Yup, missing return in the error check. Going to fix that.
Well, the main problem is that KPartition is not really able to handle
failure to unpublish the device. Besides the devfs node also
KPartition::fPublishedName will be leaked.
CU, Ingo
Other related posts:
