[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: GNU Mach / Debian patches
|
From: |
Thomas Schwinge |
|
Subject: |
Re: GNU Mach / Debian patches |
|
Date: |
Thu, 7 Apr 2005 12:11:56 +0200 |
|
User-agent: |
Mutt/1.4.2.1i |
[ CCed to the patch's author, Alexey Dejneka. ]
On Tue, Apr 05, 2005 at 06:15:07PM +0200, Alfred M. Szmidt wrote:
> > 10_cdromlock.patch
>
> <URL:http://lists.gnu.org/archive/html/bug-hurd/2000-11/msg00006.html>
>
> Could you provide a ChangeLog for this one?
Here we go:
2005-04-05 Thomas Schwinge <schwinge@nic-nac-project.de>
* linux/dev/glue/block.c
(device_close_forced): Renamed from device_close.
(device_close): New function.
(device_no_senders): New function.
(device_open): Do the right thing.
linux_block_emulation_ops: Use (device_no_senders).
* i386/i386at/i386at_ds_routines.c (ds_notify): Do the right thing.
Patch by Alexey Dejneka <adejneka@comail.ru>.
What shall I do with the 'Do the right thing.' parts? I do not (yet)
fully understand what this changes are about, so I can hardly document
them.
I attached the related mail correspondece including Alexey's proposed
patch (in case someone want's to review that) and the current (to be
applied) Debian patch (10_cdromlock.patch).
Comparing the 2004-12-04 release of the GNU system (which does not
contain the patch--correct Alfred?--at least 'nm /boot/gnumach | grep
device_no_senders' returned nothing) to Debian GNU/Hurd K8 which includes
the patch ('nm /boot/gnumach-dbg | grep device_no_senders' gives output)
I was able to verify that the patch actually works.
(I did these tests in Qemu).
> PS, Could you fix your MTA to be sane so one can simply remove any
> cruft when one wants to do a private reply
I could try to fix my *MUA* if you told me what was wrong with it.
> (which is what I wanted to
> do, but since I can't figure out your mail address, I can't)
Huh?
#v+
[...]
From: Thomas Schwinge <schwinge-lists-gnu.org-bug-hurd@nic-nac-project.de>
[...]
#v-
Regards,
Thomas
10_cdromlock.patch
Description: Text document
--- Begin Message ---
|
Subject: |
CDROM lock |
|
Date: |
Sat, 14 Oct 2000 16:27:38 +0400 |
devprobe hd2
dd if=/dev/hd2 of=/dev/null bs=1k count=1
etc. lock CD (hd2): devprobe and storeio call device_open, which
calls cdrom_lock_door, but they don't call device_close, which unlocks
CD! (Should Mach call device_close, when there are no send rights to
the device port?)
--- End Message ---
--- Begin Message ---
|
Subject: |
Re: CDROM lock |
|
Date: |
Mon, 16 Oct 2000 21:06:47 +0200 |
|
User-agent: |
Mutt/1.1.4i |
On Sat, Oct 14, 2000 at 04:27:38PM +0400, Alexey Dejneka wrote:
> devprobe hd2
> dd if=/dev/hd2 of=/dev/null bs=1k count=1
> etc. lock CD (hd2): devprobe and storeio call device_open, which
> calls cdrom_lock_door, but they don't call device_close, which unlocks
> CD! (Should Mach call device_close, when there are no send rights to
> the device port?)
I think libstoreio and devprobe should close the device. Thanks for
reporting this, I will look into it (I think with what you said, the fix is
pretty straight forward, but I want to test anyway).
One problem is certainly if you kill the task that opened the device without
giving it a chance for proper shut down. Then the door remains locked. I
don't know if Mach can detect it, but it would be a good idea. If anybody
can work on that, do it!
Thanks,
Marcus
--
`Rhubarb is no Egyptian god.' Debian http://www.debian.org Check Key server
Marcus Brinkmann GNU http://www.gnu.org for public PGP Key
Marcus.Brinkmann@ruhr-uni-bochum.de, marcus@gnu.org PGP Key ID 36E7CD09
http://homepage.ruhr-uni-bochum.de/Marcus.Brinkmann/ brinkmd@debian.org
--- End Message ---
--- Begin Message ---
|
Subject: |
storeio (in)activation (was: Re: CDROM lock |
|
Date: |
Mon, 16 Oct 2000 23:38:31 +0200 |
|
User-agent: |
Mutt/1.1.4i |
On Mon, Oct 16, 2000 at 09:06:47PM +0200, Marcus Brinkmann wrote:
> I think libstoreio and devprobe should close the device. Thanks for
> reporting this, I will look into it (I think with what you said, the fix is
> pretty straight forward, but I want to test anyway).
ok, for devprobe this is pretty easy, but for libstore not so.
The problem is that the responsibility is not entirely clear to me.
isofs does use storeio on /dev/hd3, so for CD changes to work, clearing the
active isofs translator should free the device. This means, a close on an
open_hook in storeio should set the INACTIVE flag in the store if it is the
last one. Also, the right thing has to be done when the translator should go
away, and this even if nosync is true.
That's a bit involved, so it needs some careful combing of the relevant
sources, so we don't break something else. There are several locations
involved:
libstore/make.c (store_free):
which calls store->class->cleanup, which probably needs to inactivate
the device.
libstore/device.c (dclose):
which needs to call device_close (that's easy).
storeio/storeio.c (close_hook) (or storeio/open.c (open_free))
inactivate the device, but only if it is the last user.
and what about other users of storeio?
Mmmh.
Thanks,
Marcus
--
`Rhubarb is no Egyptian god.' Debian http://www.debian.org Check Key server
Marcus Brinkmann GNU http://www.gnu.org for public PGP Key
Marcus.Brinkmann@ruhr-uni-bochum.de, marcus@gnu.org PGP Key ID 36E7CD09
http://homepage.ruhr-uni-bochum.de/Marcus.Brinkmann/ brinkmd@debian.org
--- End Message ---
--- Begin Message ---
|
Subject: |
Re: storeio (in)activation (was: Re: CDROM lock |
|
Date: |
Wed, 18 Oct 2000 01:52:25 -0400 (EDT) |
In oskit-mach, device_close is a no-op and releasing the last send right to
the port returned by device_open is what closes the device. Then there is
nothing extra to worry about, even in case of untimely death of translator.
So there.
--- End Message ---
--- Begin Message ---
|
Subject: |
Re: storeio (in)activation (was: Re: CDROM lock |
|
Date: |
Wed, 18 Oct 2000 17:27:40 +0200 |
|
User-agent: |
Mutt/1.1.4i |
On Wed, Oct 18, 2000 at 01:52:25AM -0400, Roland McGrath wrote:
> In oskit-mach, device_close is a no-op and releasing the last send right to
> the port returned by device_open is what closes the device. Then there is
> nothing extra to worry about, even in case of untimely death of translator.
> So there.
That's great. So should we just wait for this bug to vanish, or consider
doing it semantically correct anyway? [The answer might be the same as to
the question "does anybody worry enough to provide a patch?"]
Marcus
--
`Rhubarb is no Egyptian god.' Debian http://www.debian.org Check Key server
Marcus Brinkmann GNU http://www.gnu.org for public PGP Key
Marcus.Brinkmann@ruhr-uni-bochum.de, marcus@gnu.org PGP Key ID 36E7CD09
http://homepage.ruhr-uni-bochum.de/Marcus.Brinkmann/ brinkmd@debian.org
--- End Message ---
--- Begin Message ---
|
Subject: |
Bug#78011: storeio (in)activation (was: Re: CDROM lock) |
|
Date: |
Sun, 26 Nov 2000 04:15:32 +0100 |
|
User-agent: |
Mutt/1.1.4i |
Package: hurd
Severity: wishlist
On Wed, Oct 18, 2000 at 01:52:25AM -0400, Roland McGrath wrote:
> In oskit-mach, device_close is a no-op and releasing the last send right to
> the port returned by device_open is what closes the device. Then there is
> nothing extra to worry about, even in case of untimely death of translator.
> So there.
This is not good enough, IMHO. Alexey Dejneka sent in a patch for gnumach
that does the same, and it works just fine. But after doing:
settrans -ag /cdrom
you have to do
settrans -ag /dev/hd3
(or whatever your CD ROM device is) as well, or the CD ROM remains busy.
I consider this to be a bit inconvenient, and also repetitive. In fact, we
don't handle removeable media well currently (CD ROM, ZIP).
I think that storeio should release such ressources it doesn't need anymore.
Thanks,
Marcus
I wrote (just to log it in the BTS):
> The problem is that the responsibility is not entirely clear to me.
> isofs does use storeio on /dev/hd3, so for CD changes to work, clearing the
> active isofs translator should free the device. This means, a close on an
> open_hook in storeio should set the INACTIVE flag in the store if it is the
> last one. Also, the right thing has to be done when the translator should go
> away, and this even if nosync is true.
--
`Rhubarb is no Egyptian god.' Debian http://www.debian.org brinkmd@debian.org
Marcus Brinkmann GNU http://www.gnu.org marcus@gnu.org
Marcus.Brinkmann@ruhr-uni-bochum.de
http://www.marcus-brinkmann.de
--- End Message ---
--- Begin Message ---
|
Subject: |
Bug#78011: storeio (in)activation (was: Re: CDROM lock) |
|
Date: |
Sun, 26 Nov 2000 17:16:51 -0500 (EST) |
> This is not good enough, IMHO. Alexey Dejneka sent in a patch for gnumach
> that does the same, and it works just fine. But after doing:
>
> settrans -ag /cdrom
>
> you have to do
>
> settrans -ag /dev/hd3
>
> (or whatever your CD ROM device is) as well, or the CD ROM remains busy.
Ok. I think there are two lines of inquiry here. Firstly, storeio
definitely needs to be fixed; this is a bug. What storeio should do is
keep track of the number of peropens and when there are no live peropens
for the device, it should deactivate the store. (Incidentally, I think
that the changes I made to close and reopen the store should instead just
clear/set the STORE_INACTIVE flag after opening with STORE_INACTIVE.)
That will make things work right when everything is well-behaved.
For another level of robustness, we might want to think about having a
revoke-like call at the kernel device level.
--- End Message ---
--- Begin Message ---
|
Subject: |
Bug#78011: marked as done (storeio (in)activation (was: Re: CDROM lock)) |
|
Date: |
Sat, 26 May 2001 21:04:47 -0500 |
Your message dated Sun, 27 May 2001 02:58:31 +0200
with message-id <20010527025826.C577@212.23.136.22>
and subject line That was fixed, too.
has caused the attached Bug report to be marked as done.
This means that you claim that the problem has been dealt with.
If this is not the case it is now your responsibility to reopen the
Bug report if necessary, and/or fix the problem forthwith.
(NB: If you are a system administrator and have no idea what I am
talking about this indicates a serious mail system misconfiguration
somewhere. Please contact me immediately.)
Darren Benham
(administrator, Debian Bugs database)
--------------------------------------
Received: (at submit) by bugs.debian.org; 26 Nov 2000 03:14:02 +0000
>From Marcus.Brinkmann@ruhr-uni-bochum.de Sat Nov 25 21:14:02 2000
Return-path: <Marcus.Brinkmann@ruhr-uni-bochum.de>
Received: from (c3po.terralink.de) [194.97.37.23] (qmailr)
by master.debian.org with smtp (Exim 3.12 1 (Debian))
id 13zsGP-0002xU-00; Sat, 25 Nov 2000 21:14:01 -0600
Received: (qmail 31987 invoked from network); 26 Nov 2000 03:13:59 -0000
Received: from 213?21?40?184.surf-callino.de (HELO localhost)
(mail@213.21.40.184)
by c3po.t-link.de with SMTP; 26 Nov 2000 03:13:59 -0000
Received: from marcus by localhost with local (Exim 3.16 #1 (Debian))
id 13zsHs-00005O-00; Sun, 26 Nov 2000 04:15:32 +0100
Date: Sun, 26 Nov 2000 04:15:32 +0100
From: Marcus Brinkmann <Marcus.Brinkmann@ruhr-uni-bochum.de>
To: Roland McGrath <roland@frob.com>
Cc: submit@bugs.debian.org
Subject: storeio (in)activation (was: Re: CDROM lock)
Message-ID: <20001126041532.A283@ulysses.dhis.net>
References: <20001016233831.A469@ulysses.dhis.net>
<200010180552.e9I5qPX12162@neuralgia.linnaean.org>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
User-Agent: Mutt/1.1.4i
In-Reply-To: <200010180552.e9I5qPX12162@neuralgia.linnaean.org>; from
roland@frob.com on Wed, Oct 18, 2000 at 01:52:25AM -0400
Organization: Marcus Brinkmann's Home
Delivered-To: submit@bugs.debian.org
Package: hurd
Severity: wishlist
On Wed, Oct 18, 2000 at 01:52:25AM -0400, Roland McGrath wrote:
> In oskit-mach, device_close is a no-op and releasing the last send right to
> the port returned by device_open is what closes the device. Then there is
> nothing extra to worry about, even in case of untimely death of translator.
> So there.
This is not good enough, IMHO. Alexey Dejneka sent in a patch for gnumach
that does the same, and it works just fine. But after doing:
settrans -ag /cdrom
you have to do
settrans -ag /dev/hd3
(or whatever your CD ROM device is) as well, or the CD ROM remains busy.
I consider this to be a bit inconvenient, and also repetitive. In fact, we
don't handle removeable media well currently (CD ROM, ZIP).
I think that storeio should release such ressources it doesn't need anymore.
Thanks,
Marcus
I wrote (just to log it in the BTS):
> The problem is that the responsibility is not entirely clear to me.
> isofs does use storeio on /dev/hd3, so for CD changes to work, clearing the
> active isofs translator should free the device. This means, a close on an
> open_hook in storeio should set the INACTIVE flag in the store if it is the
> last one. Also, the right thing has to be done when the translator should go
> away, and this even if nosync is true.
--
`Rhubarb is no Egyptian god.' Debian http://www.debian.org brinkmd@debian.org
Marcus Brinkmann GNU http://www.gnu.org marcus@gnu.org
Marcus.Brinkmann@ruhr-uni-bochum.de
http://www.marcus-brinkmann.de
---------------------------------------
Received: (at 78011-done) by bugs.debian.org; 27 May 2001 00:58:33 +0000
>From Marcus.Brinkmann@ruhr-uni-bochum.de Sat May 26 19:58:33 2001
Return-path: <Marcus.Brinkmann@ruhr-uni-bochum.de>
Received: from (localhost) [212.23.136.22] (mail)
by master.debian.org with esmtp (Exim 3.12 1 (Debian))
id 153ot7-0003NY-00; Sat, 26 May 2001 19:58:33 -0500
Received: from marcus by localhost with local (Exim 3.22 #1 (Debian))
id 153ot5-0005nc-00
for <78011-done@bugs.debian.org>; Sun, 27 May 2001 02:58:31 +0200
Date: Sun, 27 May 2001 02:58:31 +0200
From: Marcus Brinkmann <Marcus.Brinkmann@ruhr-uni-bochum.de>
To: 78011-done@bugs.debian.org
Subject: That was fixed, too.
Message-ID: <20010527025826.C577@212.23.136.22>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
User-Agent: Mutt/1.3.15i
Sender: Marcus Brinkmann <Marcus.Brinkmann@ruhr-uni-bochum.de>
Delivered-To: 78011-done@bugs.debian.org
Fixed since some time. The revocation call at kernel level is missing,
though.
Marcus
--
`Rhubarb is no Egyptian god.' Debian http://www.debian.org brinkmd@debian.org
Marcus Brinkmann GNU http://www.gnu.org marcus@gnu.org
Marcus.Brinkmann@ruhr-uni-bochum.de
http://www.marcus-brinkmann.de
--- End Message ---
--- Begin Message ---
|
Subject: |
CD-ROM lock |
|
Date: |
Wed, 08 Nov 2000 11:33:42 +0300 |
Gnumach doesn't close device when last send right is released. If you do
dd if=/dev/hd2 of=/dev/null bs=1k count=1
kill /* pid of /hurd/storeio hd2 */
cdrom (hd2) door remains locked.
Patch:
-----------cut here-------
diff -r -C 2 --exclude configure
gnumach/i386/i386at/i386at_ds_routines.c
gnumach-new/i386/i386at/i386at_ds_routines.c
*** gnumach/i386/i386at/i386at_ds_routines.c Mon Apr 26 09:30:00 1999
--- gnumach-new/i386/i386at/i386at_ds_routines.c Wed Nov 8
09:08:04 2000
***************
*** 230,234 ****
ns = (mach_no_senders_notification_t *) msg;
! dev = (device_t) ns->not_header.msgh_remote_port;
if (dev->emul_ops->no_senders)
(*dev->emul_ops->no_senders) (ns);
--- 230,235 ----
ns = (mach_no_senders_notification_t *) msg;
! dev = (device_t) ((ipc_port_t) ns->not_header.msgh_remote_port)
! -> ip_kobject;
if (dev->emul_ops->no_senders)
(*dev->emul_ops->no_senders) (ns);
diff -r -C 2 --exclude configure gnumach/linux/dev/glue/block.c
gnumach-new/linux/dev/glue/block.c
*** gnumach/linux/dev/glue/block.c Sun Sep 24 11:51:12 2000
--- gnumach-new/linux/dev/glue/block.c Wed Nov 8 12:20:47 2000
***************
*** 798,801 ****
--- 798,802 ----
static io_return_t device_close (void *);
+ static io_return_t device_close_forced (void *, int);
/* Return a send right for block device BD. */
***************
*** 1173,1176 ****
--- 1174,1178 ----
ipc_kobject_set (bd->port, IKO_NULL, IKOT_NONE);
ipc_port_dealloc_kernel (bd->port);
+ *devp = IP_NULL;
}
kfree ((vm_offset_t) bd, sizeof (struct block_data));
***************
*** 1183,1198 ****
bd->next = open_list;
open_list = bd;
}
! if (IP_VALID (reply_port))
! ds_device_open_reply (reply_port, reply_port_type, err,
dev_to_port (bd));
! else if (! err)
device_close (bd);
!
! return MIG_NO_REPLY;
}
static io_return_t
! device_close (void *d)
{
struct block_data *bd = d, *bdp, **prev;
--- 1185,1198 ----
bd->next = open_list;
open_list = bd;
+ *devp = &bd -> device;
}
! if (!IP_VALID (reply_port) && ! err)
device_close (bd);
! return err;
}
static io_return_t
! device_close_forced (void *d, int force)
{
struct block_data *bd = d, *bdp, **prev;
***************
*** 1211,1215 ****
ds->busy = 1;
! if (--bd->open_count == 0)
{
/* Wait for pending I/O to complete. */
--- 1211,1215 ----
ds->busy = 1;
! if (force || --bd->open_count == 0)
{
/* Wait for pending I/O to complete. */
***************
*** 1254,1257 ****
--- 1254,1264 ----
}
+ static io_return_t
+ device_close (void *d)
+ {
+ return device_close_forced (d, 0);
+ }
+
+
#define MAX_COPY (VM_MAP_COPY_PAGE_LIST_MAX << PAGE_SHIFT)
***************
*** 1675,1678 ****
--- 1682,1695 ----
}
+
+ static void
+ device_no_senders (mach_no_senders_notification_t *ns)
+ {
+ device_t dev;
+
+ dev = (device_t) ((ipc_port_t) ns->not_header.msgh_remote_port) ->
ip_kobject;
+ device_close_forced (dev->emul_data, 1);
+ }
+
struct device_emulation_ops linux_block_emulation_ops =
{
***************
*** 1690,1694 ****
NULL,
NULL,
! NULL,
NULL,
NULL
--- 1707,1711 ----
NULL,
NULL,
! device_no_senders,
NULL,
NULL
--------------------cut here--------------
--- End Message ---
--- Begin Message ---
|
Subject: |
CDROM lock |
|
Date: |
Wed, 08 Nov 2000 11:38:54 +0300 |
Gnumach doesn't close device when last send right is released. If you do
dd if=/dev/hd2 of=/dev/null bs=1k count=1
kill /* pid of /hurd/storeio hd2 */
cdrom (hd2) door remains locked.
Patch:
-----------cut here-------
diff -r -C 2 --exclude configure
gnumach/i386/i386at/i386at_ds_routines.c
gnumach-new/i386/i386at/i386at_ds_routines.c
*** gnumach/i386/i386at/i386at_ds_routines.c Mon Apr 26 09:30:00 1999
--- gnumach-new/i386/i386at/i386at_ds_routines.c Wed Nov 8
09:08:04 2000
***************
*** 230,234 ****
ns = (mach_no_senders_notification_t *) msg;
! dev = (device_t) ns->not_header.msgh_remote_port;
if (dev->emul_ops->no_senders)
(*dev->emul_ops->no_senders) (ns);
--- 230,235 ----
ns = (mach_no_senders_notification_t *) msg;
! dev = (device_t) ((ipc_port_t) ns->not_header.msgh_remote_port)
! -> ip_kobject;
if (dev->emul_ops->no_senders)
(*dev->emul_ops->no_senders) (ns);
diff -r -C 2 --exclude configure gnumach/linux/dev/glue/block.c
gnumach-new/linux/dev/glue/block.c
*** gnumach/linux/dev/glue/block.c Sun Sep 24 11:51:12 2000
--- gnumach-new/linux/dev/glue/block.c Wed Nov 8 12:20:47 2000
***************
*** 798,801 ****
--- 798,802 ----
static io_return_t device_close (void *);
+ static io_return_t device_close_forced (void *, int);
/* Return a send right for block device BD. */
***************
*** 1173,1176 ****
--- 1174,1178 ----
ipc_kobject_set (bd->port, IKO_NULL, IKOT_NONE);
ipc_port_dealloc_kernel (bd->port);
+ *devp = IP_NULL;
}
kfree ((vm_offset_t) bd, sizeof (struct block_data));
***************
*** 1183,1198 ****
bd->next = open_list;
open_list = bd;
}
! if (IP_VALID (reply_port))
! ds_device_open_reply (reply_port, reply_port_type, err,
dev_to_port (bd));
! else if (! err)
device_close (bd);
!
! return MIG_NO_REPLY;
}
static io_return_t
! device_close (void *d)
{
struct block_data *bd = d, *bdp, **prev;
--- 1185,1198 ----
bd->next = open_list;
open_list = bd;
+ *devp = &bd -> device;
}
! if (!IP_VALID (reply_port) && ! err)
device_close (bd);
! return err;
}
static io_return_t
! device_close_forced (void *d, int force)
{
struct block_data *bd = d, *bdp, **prev;
***************
*** 1211,1215 ****
ds->busy = 1;
! if (--bd->open_count == 0)
{
/* Wait for pending I/O to complete. */
--- 1211,1215 ----
ds->busy = 1;
! if (force || --bd->open_count == 0)
{
/* Wait for pending I/O to complete. */
***************
*** 1254,1257 ****
--- 1254,1264 ----
}
+ static io_return_t
+ device_close (void *d)
+ {
+ return device_close_forced (d, 0);
+ }
+
+
#define MAX_COPY (VM_MAP_COPY_PAGE_LIST_MAX << PAGE_SHIFT)
***************
*** 1675,1678 ****
--- 1682,1695 ----
}
+
+ static void
+ device_no_senders (mach_no_senders_notification_t *ns)
+ {
+ device_t dev;
+
+ dev = (device_t) ((ipc_port_t) ns->not_header.msgh_remote_port) ->
ip_kobject;
+ device_close_forced (dev->emul_data, 1);
+ }
+
struct device_emulation_ops linux_block_emulation_ops =
{
***************
*** 1690,1694 ****
NULL,
NULL,
! NULL,
NULL,
NULL
--- 1707,1711 ----
NULL,
NULL,
! device_no_senders,
NULL,
NULL
--------------------cut here--------------
--- End Message ---
--- Begin Message ---
|
Subject: |
Bug#78011: storeio (in)activation (was: Re: CDROM lock) |
|
Date: |
Sun, 18 Feb 2001 21:35:39 +0100 |
|
User-agent: |
Mutt/1.1.4i |
Hi,
On Sun, Nov 26, 2000 at 05:16:51PM -0500, Roland McGrath wrote:
> > This is not good enough, IMHO. Alexey Dejneka sent in a patch for gnumach
> > that does the same, and it works just fine. But after doing:
> >
> > settrans -ag /cdrom
> >
> > you have to do
> >
> > settrans -ag /dev/hd3
> >
> > (or whatever your CD ROM device is) as well, or the CD ROM remains busy.
>
> Ok. I think there are two lines of inquiry here. Firstly, storeio
> definitely needs to be fixed; this is a bug. What storeio should do is
> keep track of the number of peropens and when there are no live peropens
> for the device, it should deactivate the store. (Incidentally, I think
> that the changes I made to close and reopen the store should instead just
> clear/set the STORE_INACTIVE flag after opening with STORE_INACTIVE.)
> That will make things work right when everything is well-behaved.
Well, the patch below does just that, but it doesn't work. It seems that
opening with STORE_INACTIVE is not supported. I can imagine several
approaches to this:
1. In storeio, the first time open the store active (and skip the clear
inactive in open_hook). After that, proceed as normal (activating/inactivating).
2. In libstore, implement support for STORE_INACTIVE in store opens.
3. In storeio, use open/close instead activation flag.
4. In storeio, open the store active, but inactivate it immediately. Doh!
I think 2. is the only proper solution to this, and not hard at all (it
might be the simplest amont all. But we need to do it for every store
type seperately, I think (at least for those were the active flag has
any meaning at all).
> For another level of robustness, we might want to think about having a
> revoke-like call at the kernel device level.
That's for later.
Marcus
diff -ru storeio.pri/ChangeLog storeio/ChangeLog
--- storeio.pri/ChangeLog Tue Jan 30 00:43:26 2001
+++ storeio/ChangeLog Sun Feb 18 20:57:20 2001
@@ -1,3 +1,15 @@
+2001-02-18 Marcus Brinkmann <marcus@gnu.org>
+
+ * storeio.c (global_lock): New global variable.
+ (nperopens): Likewise.
+ (main): Initialize global_lock.
+ (open_hook): Hold global_lock and check if this is the first open.
+ If yes, activate the store.
+ (close_hook): Hold global_lock and check if this was the last open.
+ If yes, inactivate the store.
+ * dev.c (dev_open): Open the store with STORE_INACTIVE
+ (in store_parsed_open as well as in store_create).
+
2001-01-17 Roland McGrath <roland@frob.com>
* dev.c (dev_buf_discard): Don't check AMOUNT if store_write failed.
diff -ru storeio.pri/dev.c storeio/dev.c
--- storeio.pri/dev.c Tue Jan 30 00:43:26 2001
+++ storeio/dev.c Sun Feb 18 20:53:59 2001
@@ -145,14 +145,15 @@
/* This means we had no store arguments.
We are to operate on our underlying node. */
err = store_create (storeio_fsys->underlying,
- dev->readonly ? STORE_READONLY : 0,
+ STORE_INACTIVE | (dev->readonly ? STORE_READONLY : 0),
0, &dev->store);
}
else
/* Open based on the previously parsed store arguments. */
err = store_parsed_open (dev->store_name,
- dev->readonly ? STORE_READONLY : 0,
+ STORE_INACTIVE
+ | (dev->readonly ? STORE_READONLY : 0),
&dev->store);
if (err)
return err;
diff -ru storeio.pri/storeio.c storeio/storeio.c
--- storeio.pri/storeio.c Tue Jan 16 17:34:12 2001
+++ storeio/storeio.c Sun Feb 18 20:59:03 2001
@@ -32,6 +32,12 @@
#include "open.h"
#include "dev.h"
+/* Global lock. */
+struct mutex global_lock;
+
+/* Count of active opens. */
+int nperopens;
+
static struct argp_option options[] =
{
{"readonly", 'r', 0, 0,"Disallow writing"},
@@ -124,6 +130,8 @@
struct dev device;
struct storeio_argp_params params;
+ mutex_init (&global_lock);
+
bzero (&device, sizeof device);
mutex_init (&device.lock);
@@ -233,18 +241,34 @@
static error_t
open_hook (struct trivfs_peropen *peropen)
{
+ error_t err = 0;
struct dev *const dev = peropen->cntl->hook;
+
if (dev->store)
- return open_create (dev, (struct open **)&peropen->hook);
- else
- return 0;
+ {
+ mutex_lock (&global_lock);
+ if (nperopens++ == 0)
+ err = store_clear_flags (dev->store, STORE_INACTIVE);
+ mutex_unlock (&global_lock);
+ if (!err)
+ err = open_create (dev, (struct open **)&peropen->hook);
+ }
+ return err;
}
static void
close_hook (struct trivfs_peropen *peropen)
{
+ struct dev *const dev = peropen->cntl->hook;
+
if (peropen->hook)
- open_free (peropen->hook);
+ {
+ mutex_lock (&global_lock);
+ if (--nperopens == 0)
+ store_set_flags (dev->store, STORE_INACTIVE);
+ mutex_unlock (&global_lock);
+ open_free (peropen->hook);
+ }
}
/* ---------------------------------------------------------------- */
--
`Rhubarb is no Egyptian god.' Debian http://www.debian.org brinkmd@debian.org
Marcus Brinkmann GNU http://www.gnu.org marcus@gnu.org
Marcus.Brinkmann@ruhr-uni-bochum.de
http://www.marcus-brinkmann.de
--- End Message ---
--- Begin Message ---
|
Subject: |
Bug#78011: storeio (in)activation (was: Re: CDROM lock) |
|
Date: |
Sun, 18 Feb 2001 18:50:30 -0500 (EST) |
> Well, the patch below does just that, but it doesn't work. It seems that
> opening with STORE_INACTIVE is not supported.
Ok. Well, if the changes to storeio succeed in making that libstore calls
we think are right (and in particular deactivating the store when peropens
goes to zero on a close), then you can check those changes in before we
worry about how libstore should be fixed. The count you added should be in
struct dev, not a global variable, and locked by the existing mutex there.
But other than that, it looks good so you can check it in with that cleanup.
I think I agree with you about the libstore changes. But it is a decent
interim solution to just leave libstore as it is and change storeio as you
have. Then the store gets activated when storeio first starts up, but is
properly deactivated after the first open+close.
--- End Message ---
--- Begin Message ---
|
Subject: |
Bug#78011: storeio (in)activation (was: Re: CDROM lock) |
|
Date: |
Mon, 19 Feb 2001 12:11:50 +0100 |
|
User-agent: |
Mutt/1.3.12i |
On Sun, Feb 18, 2001 at 06:50:30PM -0500, Roland McGrath wrote:
> > Well, the patch below does just that, but it doesn't work. It seems that
> > opening with STORE_INACTIVE is not supported.
>
> Ok. Well, if the changes to storeio succeed in making that libstore calls
> we think are right (and in particular deactivating the store when peropens
> goes to zero on a close), then you can check those changes in before we
> worry about how libstore should be fixed. The count you added should be in
> struct dev, not a global variable, and locked by the existing mutex there.
> But other than that, it looks good so you can check it in with that cleanup.
Ah yes, that's much better.
> I think I agree with you about the libstore changes. But it is a decent
> interim solution to just leave libstore as it is and change storeio as you
> have. Then the store gets activated when storeio first starts up, but is
> properly deactivated after the first open+close.
It gets, but because the way the code is written, dopen in libstore will be
run twice, and one port to the device gets leaked. This lost port will block
the drive until storeio is killed. (A workaround might be to check in dopen
if the drive was already opened, or to check in store_set_flags if the flag
is already set, likewise for clear).
Thanks,
Marcus
--- End Message ---
--- Begin Message ---
|
Subject: |
Bug#78011: storeio (in)activation (was: Re: CDROM lock) |
|
Date: |
Mon, 19 Feb 2001 15:02:07 -0500 (EST) |
> It gets, but because the way the code is written, dopen in libstore will be
> run twice, and one port to the device gets leaked. This lost port will block
> the drive until storeio is killed. (A workaround might be to check in dopen
> if the drive was already opened, or to check in store_set_flags if the flag
> is already set, likewise for clear).
As I read store_{set,clear}_flags, they will not make any function calls if
no backend flag bits are changing. The store types' *_set_flags functions
should definitely not do anything stupid if called with STORE_INACTIVE
already in the same state, since there can be such calls for other flags
changing.
--- End Message ---