Re: CVS commit: src/sys/dev

[Nathanial Sloss]

On Sat, 4 Nov 2017 09:46:03 matthew green wrote:
> > Modified Files:
> > src/sys/dev: audio.c
> > 
> > Log Message:
> > Blocksizes sould be rounded to a power of 2 as OSS applications need
> > this.
> 

Not sure how hard it would be to move it to ossaudio.  I'll try to modify 
ossaudio.c in the kernel and libossaudio and see how it goes.


> sounds like a limitation that belongs in ossaudio not audio.
> is it hard to move there instead?
> 
> 
> mrg.

re: CVS commit: src/sys/dev

[matthew green]

> Modified Files:
>   src/sys/dev: audio.c
> 
> Log Message:
> Blocksizes sould be rounded to a power of 2 as OSS applications need this.

sounds like a limitation that belongs in ossaudio not audio.
is it hard to move there instead?


.mrg.

Re: CVS commit: src/sys/dev

[Robert Swindells]

Michael van Elst  wrote:
>On Fri, Nov 03, 2017 at 09:22:36AM +0900, Takeshi Nakayama wrote:
>> errno 5 is EIO.
>> 
>> This is why we check WDF_LOADED is not set and !RAW_PART or !S_IFCHR
>> in wdopen().  WDF_LOADED is set only in wd_fistopen(), so we cannot
>> open disks unless open with RAW_PART and S_IFCHR in the first time.
>> 
>> Possible fix is remove useless check as follows or introduce
>> WDF_OPEN if we want to mimic the sd(4).
>
>Please try
>
>http://ftp.netbsd.org/pub/NetBSD/misc/mlelstv/wd.diff
>
>this is the "introduce WDF_OPEN" that is missing.

With the patch my test machine now boots.

Thanks,

Robert Swindells

Re: CVS commit: src/sys/dev

[Takeshi Nakayama]

>>> Michael van Elst  wrote

> On Fri, Nov 03, 2017 at 09:22:36AM +0900, Takeshi Nakayama wrote:
> > errno 5 is EIO.
> > 
> > This is why we check WDF_LOADED is not set and !RAW_PART or !S_IFCHR
> > in wdopen().  WDF_LOADED is set only in wd_fistopen(), so we cannot
> > open disks unless open with RAW_PART and S_IFCHR in the first time.
> > 
> > Possible fix is remove useless check as follows or introduce
> > WDF_OPEN if we want to mimic the sd(4).
> 
> Please try
> 
> http://ftp.netbsd.org/pub/NetBSD/misc/mlelstv/wd.diff
> 
> this is the "introduce WDF_OPEN" that is missing.

I just confirmed that works well.  Thank you for fixing.

-- Takehi Nakayama

re: CVS commit: src

[Paul Goyette]

On Fri, 3 Nov 2017, matthew green wrote:


sorry, i forgot the sysctl interface was new.  this is OK, but
still likely overblown adding a version -- as mentioned, the
ABI should never change in an incompatible way (we do have
ways that grow structures, but never modify existing parts),
so having a version is not necessary - the version could be
considered the size of the structure.


OK, no problem.

I'll be happy to remove the version stuff.  Probably tomorrow, 
definitely before the weekend is over.




+--+--++
| Paul Goyette | PGP Key fingerprint: | E-mail addresses:  |
| (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee dot com   |
| Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd dot org |
+--+--++

re: CVS commit: src

[matthew green]

sorry, i forgot the sysctl interface was new.  this is OK, but
still likely overblown adding a version -- as mentioned, the
ABI should never change in an incompatible way (we do have
ways that grow structures, but never modify existing parts),
so having a version is not necessary - the version could be
considered the size of the structure.

thanks.


.mrg.

Re: CVS commit: src/sys/dev

[Michael van Elst]

On Fri, Nov 03, 2017 at 09:22:36AM +0900, Takeshi Nakayama wrote:
> errno 5 is EIO.
> 
> This is why we check WDF_LOADED is not set and !RAW_PART or !S_IFCHR
> in wdopen().  WDF_LOADED is set only in wd_fistopen(), so we cannot
> open disks unless open with RAW_PART and S_IFCHR in the first time.
> 
> Possible fix is remove useless check as follows or introduce
> WDF_OPEN if we want to mimic the sd(4).

Please try

http://ftp.netbsd.org/pub/NetBSD/misc/mlelstv/wd.diff

this is the "introduce WDF_OPEN" that is missing.


Greetings,
-- 
Michael van Elst
Internet: mlel...@serpens.de
"A potential Snark may lurk in every tree."

re: CVS commit: src

[Paul Goyette]

On Fri, 3 Nov 2017, matthew green wrote:


thanks for working on this.


* Update the data structures used for exporting kernel history data to
  include a version number as well as the length of history arguments.


so, old vmstat breaks for no reason?  nothing else changed, right?


Unfortunately, yes, old vmstat breaks.  But other stuff _did_ change, 
notably the change that all four of the printf-args are explicitly cast 
to uintmax_t before being stored in the kernel and before being 
"exported" to userland.


It was because of this breakage that I introduced a version-and-size 
check, to _cleanly_ detect any such mismatch in the future.


Note that vmstat was already quite broken on architectures such as arm, 
where passing a 64-bit value to printf(3) was processed as two separate 
32-bit args.



* All [2] existing users of kernhist(9) have had instances of "%s" or
  "%c" format strings replaced with numeric formats; several instances
  of mis-match between format string and argument list have been fixed.


i wonder if we could have an audit mode.  it would actually
pass the arguments to a printf()-like function so the compiler
would do checking.


That would have made the task much simpler.


* vmstat(1) now checks the version and argument length included in the
  data exported via sysctl(9) and exits if they do not match the values
  with which vmstat was built.


that's not how we do sysctl.  if we were just going to do this
we may as well just do kvm.  sysctl should be ABI stable.


As far as I know, the sysctl we're discussing is newly-introduced for 
NetBSD-8, so it has _never_ been part of any release.  The version and 
length checks can easily be removed - they were intended only for the 
convenience of those who follow -current.



please revert all the ABI changes.  they're not necessary and
they're just not how sysctl is done.


Well, since "all the ABI changes" includes the sizes of the members of 
the data structures, that would mean reverting _all_ the recent changes, 
and going back to the broken state (at least for some architectures).
(It isn't clear - to me, at least - if this "please revert" also refers 
to the original sysctl-ification that was done so many months ago.)


If you can be more explicit and detailed about exactly what you want 
done, I will look into it further.  Please address each of:


- version-and-length check
- other changes to the data structures, resulting in the
  conversion of printf()'s arguments to uintmax_t in both
  in-kernel and export structures (as opposed to ulong and
  uint64_t respectively)
- the whole sysctl-ification





+--+--++
| Paul Goyette | PGP Key fingerprint: | E-mail addresses:  |
| (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee dot com   |
| Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd dot org |
+--+--++

re: CVS commit: src

[matthew green]

thanks for working on this.

> * Update the data structures used for exporting kernel history data to
>   include a version number as well as the length of history arguments.

so, old vmstat breaks for no reason?  nothing else changed, right?

> * All [2] existing users of kernhist(9) have had instances of "%s" or
>   "%c" format strings replaced with numeric formats; several instances
>   of mis-match between format string and argument list have been fixed.

i wonder if we could have an audit mode.  it would actually
pass the arguments to a printf()-like function so the compiler
would do checking.

> * vmstat(1) now checks the version and argument length included in the
>   data exported via sysctl(9) and exits if they do not match the values
>   with which vmstat was built.

that's not how we do sysctl.  if we were just going to do this
we may as well just do kvm.  sysctl should be ABI stable.

please revert all the ABI changes.  they're not necessary and
they're just not how sysctl is done.

thanks.


.mrg.