Re: [PATCH hurd] rumpdisk: do not open device if block size is 0
Flávio Cruz, le jeu. 29 févr. 2024 22:01:41 -0500, a ecrit: > On Thu, Feb 29, 2024 at 7:29 PM Samuel Thibault <[1]samuel.thiba...@gnu.org> > wrote: > > I could be wrong but if you look at this build log > [3]https://buildd.debian.org > /status/fetch.php?pkg=perl&arch=hurd-i386&ver=5.38.2-3&stamp=1705087520&raw=0 > it fails on t/op/stat: > > > t/op/stat # Failed > test 36 - ls and -b agreeing on /dev (0 310) at op/stat.t line 322 > # got "0" > # expected "310" > # = before > # total 1912 > > I did run the suite myself and it did seem to get stuck at listing /dev when > it > attempted to open /dev/cd0. Oh? In my memory the issue I was seeing with op/stat was different. Anyway, we will see, if it's solved then great :) Samuel
[PATCH hurd] libps: update ps_emit_nice_size_t to handle arbitrarily large size_t
Update argument types for sprint_frac_value to reflect how big they
actually are so that GCC doesn't think it needs a larger buffer than
necessary.
---
libps/spec.c | 22 +-
1 file changed, 13 insertions(+), 9 deletions(-)
diff --git a/libps/spec.c b/libps/spec.c
index 9f64703..60ae7fb 100644
--- a/libps/spec.c
+++ b/libps/spec.c
@@ -19,6 +19,7 @@
Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA. */
#include
+#include
#include
#include
#include
@@ -437,12 +438,12 @@ ps_emit_num_blocks (struct proc_stat *ps, struct
ps_fmt_field *field,
size_t
sprint_frac_value (char *buf,
- size_t value, int min_value_len,
- size_t frac, int frac_scale,
- int width)
+ uint16_t value, uint8_t min_value_len,
+ uint16_t frac, uint8_t frac_scale,
+ uint8_t width)
{
- int value_len = 0;
- int frac_len = 0;
+ uint8_t value_len = 0;
+ uint8_t frac_len = 0;
if (value >= 1000)/* the integer part */
value_len = 4; /* values 1000-1023 */
@@ -462,9 +463,9 @@ sprint_frac_value (char *buf,
frac /= 10;
if (frac_len > 0)
-sprintf (buf, "%zd.%0*zd", value, frac_len, frac);
+sprintf (buf, "%" PRIu16 ".%0*" PRIu16, value, frac_len, frac);
else
-sprintf (buf, "%zd", value);
+sprintf (buf, "%" PRIu16, value);
return strlen (buf);
}
@@ -492,11 +493,14 @@ error_t
ps_emit_nice_size_t (struct proc_stat *ps, struct ps_fmt_field *field,
struct ps_stream *stream)
{
- char buf[21];
+ char buf[20];
size_t value = FG_PROC_STAT (field, size_t)(ps);
- char *sfx = " KMG";
+ char *sfx = " KMGTPE";
size_t frac = 0;
+ _Static_assert (sizeof (size_t) <= 8,
+ "ps_emit_nice_size_t can only emit size_t up to 8 bytes long.");
+
while (value >= 1024)
{
frac = ((value & 0x3FF) * 1000) >> 10;
--
2.39.2
Re: [PATCH hurd] rumpdisk: do not open device if block size is 0
On Sun, Mar 3, 2024 at 10:39 AM Samuel Thibault
wrote:
> Flávio Cruz, le jeu. 29 févr. 2024 22:01:41 -0500, a ecrit:
> > On Thu, Feb 29, 2024 at 7:29 PM Samuel Thibault <[1]
> samuel.thiba...@gnu.org>
> > wrote:
> >
> > I could be wrong but if you look at this build log [3]
> https://buildd.debian.org
> >
> /status/fetch.php?pkg=perl&arch=hurd-i386&ver=5.38.2-3&stamp=1705087520&raw=0
> > it fails on t/op/stat:
> >
> >
> > t/op/stat #
> Failed test 36 - ls and -b agreeing on /dev (0 310) at op/stat.t line 322
> > # got "0"
> > # expected "310"
> > # = before
> > # total 1912
> >
> > I did run the suite myself and it did seem to get stuck at listing /dev
> when it
> > attempted to open /dev/cd0.
>
> Oh? In my memory the issue I was seeing with op/stat was different.
> Anyway, we will see, if it's solved then great :)
>
I just gave it a try with a patched system and indeed it doesn't seem to
solve this problem.
However, I also tested the vim testsuite and the existing hang
VIMRUNTIME=../../runtime ../vim -f -u unix.vim --gui-dialog-file
guidialog -U NONE --noplugin --not-a-term -S runtest.vim test_stat.vim
--cmd 'au SwapExists * let v:swapchoice = "e"' | LC_ALL=C LANG=C
LANGUAGE=C awk '/Executing Test_/{match($0, "([0-9][0-9]:[0-9][0-9]
*)?Executing Test_[^\\)]*\\)"); print substr($0, RSTART, RLENGTH)
"\r"; fflush()}'
E: Build killed with signal TERM after 180 minutes of inactivity
no longer happens. The test passes but the test suite goes on to fail on
later tests.
> Samuel
>
Re: [PATCH hurd] libps: update ps_emit_nice_size_t to handle arbitrarily large size_t
Applied, thanks!
Flavio Cruz, le dim. 03 mars 2024 12:38:19 -0500, a ecrit:
> Update argument types for sprint_frac_value to reflect how big they
> actually are so that GCC doesn't think it needs a larger buffer than
> necessary.
> ---
> libps/spec.c | 22 +-
> 1 file changed, 13 insertions(+), 9 deletions(-)
>
> diff --git a/libps/spec.c b/libps/spec.c
> index 9f64703..60ae7fb 100644
> --- a/libps/spec.c
> +++ b/libps/spec.c
> @@ -19,6 +19,7 @@
> Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA. */
>
> #include
> +#include
> #include
> #include
> #include
> @@ -437,12 +438,12 @@ ps_emit_num_blocks (struct proc_stat *ps, struct
> ps_fmt_field *field,
>
> size_t
> sprint_frac_value (char *buf,
> - size_t value, int min_value_len,
> - size_t frac, int frac_scale,
> - int width)
> + uint16_t value, uint8_t min_value_len,
> + uint16_t frac, uint8_t frac_scale,
> + uint8_t width)
> {
> - int value_len = 0;
> - int frac_len = 0;
> + uint8_t value_len = 0;
> + uint8_t frac_len = 0;
>
>if (value >= 1000)/* the integer part */
> value_len = 4; /* values 1000-1023 */
> @@ -462,9 +463,9 @@ sprint_frac_value (char *buf,
> frac /= 10;
>
>if (frac_len > 0)
> -sprintf (buf, "%zd.%0*zd", value, frac_len, frac);
> +sprintf (buf, "%" PRIu16 ".%0*" PRIu16, value, frac_len, frac);
>else
> -sprintf (buf, "%zd", value);
> +sprintf (buf, "%" PRIu16, value);
>
>return strlen (buf);
> }
> @@ -492,11 +493,14 @@ error_t
> ps_emit_nice_size_t (struct proc_stat *ps, struct ps_fmt_field *field,
>struct ps_stream *stream)
> {
> - char buf[21];
> + char buf[20];
>size_t value = FG_PROC_STAT (field, size_t)(ps);
> - char *sfx = " KMG";
> + char *sfx = " KMGTPE";
>size_t frac = 0;
>
> + _Static_assert (sizeof (size_t) <= 8,
> + "ps_emit_nice_size_t can only emit size_t up to 8 bytes long.");
> +
>while (value >= 1024)
> {
>frac = ((value & 0x3FF) * 1000) >> 10;
> --
> 2.39.2
>
>
--
Samuel
---
Pour une évaluation indépendante, transparente et rigoureuse !
Je soutiens la Commission d'Évaluation de l'Inria.
Re: [PATCH 1/1 hurd] ext2fs: New default: use xattrs to store translator records
Damien Zammit, le dim. 03 mars 2024 01:00:53 +, a ecrit: > - Legacy records still work with either setting. Just to confirm: did you try to make an ext2fs built before this use the xattr record? I.e. we can go back&forth between previous hurd package and next hurd package? Also, if (sblock->s_creator_os != htole32 (EXT2_OS_HURD)) return EOPNOTSUPP; We don't want to require a hurd-created filesystem any more: - if the creator os is hurd, then we can try to read di->i_translator - if the xattr feature is enabled on the fs, then we can try to read the gnu.translator xattr It's only when the creator os is not hurd and the xattr feature is not enabled on the fs, that we should return EOPNOTSUPP. Samuel
Re: [PATCH 0/1 hurd] ext2fs - new translator entries in xattrs
Damien Zammit, le dim. 03 mars 2024 01:00:48 +, a ecrit: > Do we allow a soft migration where translator entries are kept as is, > but new ones are migrated only upon creation. That will be fine for now, as we test it. > Or do we write a script that will migrate all translator entries so > users don't end up with a mish-mash of old and new ones on their > systems? We can write such script at some point yes. Samuel
Re: [PATCH v7 1/2 hurd] libirqhelp: Add library
Damien Zammit, le sam. 02 mars 2024 10:31:50 +, a ecrit:
> +static error_t
> +get_acpi(void)
> +{
> + error_t err = 0;
> + mach_port_t tryacpi, device_master;
> +
> + acpidev = MACH_PORT_NULL;
This looks odd. If we had a previous port in acpidev, we want to
deallocate it, not forget it.
> + err = get_privileged_ports (0, &device_master);
> + if (!err)
> +{
> + err = device_open (device_master, D_READ, "acpi", &tryacpi);
> + mach_port_deallocate (mach_task_self (), device_master);
> + if (!err)
> +{
> + acpidev = tryacpi;
> + return 0;
> +}
> +}
> +
> + tryacpi = file_name_lookup (_SERVERS_ACPI, O_RDONLY, 0);
> + if (tryacpi == MACH_PORT_NULL)
> +return ENODEV;
> +
> + acpidev = tryacpi;
> + return 0;
> +}
> +
> +static error_t
> +get_irq(void)
> +{
> + error_t err = 0;
> + mach_port_t tryirq, device_master;
> +
> + irqdev = MACH_PORT_NULL;
Same here.
> + err = get_privileged_ports (0, &device_master);
> + if (err)
> +return err;
> +
> + err = device_open (device_master, D_READ|D_WRITE, "irq", &tryirq);
> + mach_port_deallocate (mach_task_self (), device_master);
> + if (err)
> +return err;
> +
> + irqdev = tryirq;
> + return err;
> +}
> +
> +error_t
> +irqhelp_init(void)
> +{
> + static bool inited = false;
> + error_t err;
> +
> + if (inited)
> +{
> + log_error("already inited\n");
> + return 0;
But we already have a guard against several initializations. So I don't
see the point of setting the ports to MACH_PORT_NULL? Or else you meant
to statically initalize them to MACH_PORT_NULL? (which will be much more
efficient).
> +}
> +
> + err = get_irq();
> + if (err)
> +{
> + log_error("cannot grab irq device\n");
> + return err;
> +}
> +
> + err = get_acpi();
> + if (err)
> +{
> + log_error("cannot grab acpi device\n");
> + return err;
> +}
> +
> + inited = true;
> + return 0;
> +}
> +
> +void *
> +irqhelp_server_loop(void *arg)
> +{
> + struct irq *irq = (struct irq *)arg;
> + mach_port_t master_host;
> + mach_port_t pset, psetcntl;
> + error_t err;
> +
> + if (!irq)
> +{
> + log_error("cannot start this irq thread\n");
> + return NULL;
> +}
> +
> + err = get_privileged_ports (&master_host, 0);
> + if (err)
> +{
> + log_error("cannot get master_host port\n");
> + return NULL;
> +}
> +
> + err = thread_get_assignment (mach_thread_self (), &pset);
> + if (err)
> +goto fail;
> +
> + err = host_processor_set_priv (master_host, pset, &psetcntl);
> + if (err)
> +goto fail;
> +
> + thread_max_priority (mach_thread_self (), psetcntl, 0);
> + err = thread_priority (mach_thread_self (), IRQ_THREAD_PRIORITY, 0);
> + if (err)
> +goto fail;
> +
> + mach_port_deallocate (mach_task_self (), master_host);
> +
> + int interrupt_demuxer (mach_msg_header_t *inp,
> + mach_msg_header_t *outp)
> + {
> +static bool printed0 = false;
> +static bool printed1 = false;
Better put them in their respective blocks.
> +device_intr_notification_t *n = (device_intr_notification_t *) inp;
> +
> +((mig_reply_header_t *) outp)->RetCode = MIG_NO_REPLY;
> +if (n->intr_header.msgh_id != DEVICE_INTR_NOTIFY)
> + {
> + if (!printed0)
> + {
> + log_error("msg received is not an interrupt\n");
> +printed0 = true;
> + }
> + return 0;
> + }
> +
> +/* FIXME: id <-> gsi now has an indirection, assuming 1:1 */
> +if (n->id != irq->gsi)
> + {
> + if (!printed1)
> + {
> + log_error("interrupt expected on irq %d arrived on irq %d\n",
> irq->gsi, n->id);
> + printed1 = true;
> + }
> + return 0;
> + }
> +
> +/* wait if irq disabled */
> +pthread_mutex_lock (&irq->irqlock);
> +while (!irq->enabled)
> + pthread_cond_wait (&irq->irqcond, &irq->irqlock);
> +pthread_mutex_unlock (&irq->irqlock);
> +
> +/* call handler */
> +irq->handler(irq->context);
You probably want to add an if (!irq->shutdown) before this? Otherwise
after the application called irqhelp_remove_interrupt_handler, the
handler will still be called once.
> +/* ACK interrupt */
> +device_intr_ack (irqdev, irq->port, MACH_MSG_TYPE_MAKE_SEND);
This might be spurious as well in the irq->shutdown case?
> +if (irq->shutdown)
> + mach_port_deallocate (mach_task_self (), irq->port);
> +
> +return 1;
> + }
> +
> + /* Server loop */
> + mach_msg_server (interrupt_demuxer, 0, irq->port);
> + goto done;
> +
> +fail:
> + log_error("failed to register irq %d\n", irq->gsi);
> +
> +done:
> + pthread_cond_destroy(&irq->irqcond);
> + pthread_mutex_destroy(&irq->irqlock);
> + free(irq);
> + return NULL;
> +}
