Re: cksum.1: use imperative tense

[patrick keshishian]

On Wed, Aug 16, 2023 at 7:02 PM Klemens Nanni  wrote:
>
> Consistent with other options and our manuals in general.
>
> Index: cksum.1
> ===
> RCS file: /cvs/src/bin/md5/cksum.1,v
> retrieving revision 1.40
> diff -u -p -r1.40 cksum.1
> --- cksum.1 4 Aug 2022 06:20:24 -   1.40
> +++ cksum.1 17 Aug 2023 01:31:19 -
> @@ -131,7 +131,7 @@ Place the checksum into
>  .Ar hashfile
>  instead of stdout.
>  .It Fl p
> -Echoes stdin to stdout and appends the
> +Echo stdin to stdout and append the
>  checksum to stdout.

That sounds awkward. I'd rephrase it as "Echo stdin and append the
checksum to stdout"
Same below with md5.1 instance.

--patrick


>  .It Fl q
>  Only print the checksum (quiet mode) or if used in conjunction with the
> @@ -141,16 +141,16 @@ flag, only print the failed cases.
>  Reverse the format of the hash algorithm output, making
>  it match the checksum output format.
>  .It Fl s Ar string
> -Prints a checksum of the given
> +Print a checksum of the given
>  .Ar string .
>  .It Fl t
> -Runs a built-in time trial.
> +Run a built-in time trial.
>  Specifying
>  .Fl t
>  multiple times results in the number of rounds being multiplied
>  by 10 for each additional flag.
>  .It Fl x
> -Runs a built-in test script.
> +Run a built-in test script.
>  .El
>  .Pp
>  The default CRC used is based on the polynomial used for CRC error checking
> Index: md5.1
> ===
> RCS file: /cvs/src/bin/md5/md5.1,v
> retrieving revision 1.48
> diff -u -p -r1.48 md5.1
> --- md5.1   25 Jan 2019 00:19:25 -  1.48
> +++ md5.1   17 Aug 2023 01:31:54 -
> @@ -89,7 +89,7 @@ Place the checksum into
>  .Ar hashfile
>  instead of stdout.
>  .It Fl p
> -Echoes stdin to stdout and appends the
> +Echo stdin to stdout and append the
>  checksum to stdout.
>  .It Fl q
>  Only print the checksum (quiet mode) or if used in conjunction with the
> @@ -100,16 +100,16 @@ Reverse the format of the hash algorithm
>  it match the output format used by
>  .Xr cksum 1 .
>  .It Fl s Ar string
> -Prints a checksum of the given
> +Print a checksum of the given
>  .Ar string .
>  .It Fl t
> -Runs a built-in time trial.
> +Run a built-in time trial.
>  Specifying
>  .Fl t
>  multiple times results in the number of rounds being multiplied
>  by 10 for each additional flag.
>  .It Fl x
> -Runs a built-in test script.
> +Run a built-in test script.
>  .El
>  .Sh EXIT STATUS
>  These utilities exit 0 on success,
>

Re: Mention Smart Battery Data Spec in smbus.h

[patrick keshishian]

On Sat, Mar 05, 2022 at 09:37:32PM +1100, Damien Miller wrote:
> without commenting on the substance of this change, it should definitely
> not be added to the copyright block

Good point.

I thought adding reference to SMBus spec and mention of ACPI
section where SMBus register offsets are sourced would be
beneficial.

How about either of the following to variants.

Version 1:  Indicate reference material at relevant section.
Version 2:  List references material at top in one comment block.

Best,
--patrick


> On Fri, 4 Mar 2022, patrick keshishian wrote:
> 
> > Hello,
> > 
> > I took a wrong turn, and got interested in where the SMBATT_CMD_*
> > defines were sourced.
> > 
> > Adding a reference to Smart Battery Data Spec might save someone
> > else the time searching through ACPI spec, then SMBus spec, to
> > finally arriving at the answer.
> > 
> > Is the following diff acceptable?
> > I believe this is the correct/definitive source.
> > 
> > Thanks,
> > --patrick


=== VERSION 1 =

Index: smbus.h
===
RCS file: /cvs/obsd/src/sys/dev/acpi/smbus.h,v
retrieving revision 1.1
diff -u -p -u -p -r1.1 smbus.h
--- smbus.h 22 Feb 2017 16:39:56 -  1.1
+++ smbus.h 5 Mar 2022 18:37:55 -
@@ -29,6 +29,9 @@
 #ifndef _ACPI_SMBUS_H_
 #define _ACPI_SMBUS_H_
 
+/*
+ * ACPI Section 12.9.2 Protocol Description
+ */
 enum {
 SMBUS_WRITE_QUICK = 2,
 SMBUS_READ_QUICK = 3,
@@ -60,11 +63,18 @@ enum {
 
 /*
  * Smart-Battery commands and definitions
+ *
+ * SMBus Specification Appendix C
+ * http://smbus.org/specs/SMBus_3_2_20220112.pdf
  */
 
 /* Base address */
 #define SMBATT_ADDRESS 0x16
 
+/*
+ * Smart Battery Data Specification Section 5
+ * http://sbs-forum.org/specs/sbdat110.pdf
+ */
 
 /* access: READ WRITE WORD */
 #define SMBATT_CMD_MANUFACTURER_ACCESS 0



=== VERSION 2 =

Index: smbus.h
===
RCS file: /cvs/obsd/src/sys/dev/acpi/smbus.h,v
retrieving revision 1.1
diff -u -p -u -p -r1.1 smbus.h
--- smbus.h 22 Feb 2017 16:39:56 -  1.1
+++ smbus.h 5 Mar 2022 18:45:17 -
@@ -26,6 +26,16 @@
  * $FreeBSD$
  */
 
+/*
+ * ACPI Section 12.9.2 Protocol Description
+ *
+ * SMBus Specification Appendix C
+ * http://smbus.org/specs/SMBus_3_2_20220112.pdf
+ *
+ * Smart Battery Data Specification Section 5
+ * http://sbs-forum.org/specs/sbdat110.pdf
+ */
+
 #ifndef _ACPI_SMBUS_H_
 #define _ACPI_SMBUS_H_
 

Mention Smart Battery Data Spec in smbus.h

[patrick keshishian]

Hello,

I took a wrong turn, and got interested in where the SMBATT_CMD_*
defines were sourced.

Adding a reference to Smart Battery Data Spec might save someone
else the time searching through ACPI spec, then SMBus spec, to
finally arriving at the answer.

Is the following diff acceptable?
I believe this is the correct/definitive source.

Thanks,
--patrick


Index: smbus.h
===
RCS file: /cvs/obsd/src/sys/dev/acpi/smbus.h,v
retrieving revision 1.1
diff -u -p -u -p -r1.1 smbus.h
--- smbus.h 22 Feb 2017 16:39:56 -  1.1
+++ smbus.h 5 Mar 2022 07:37:06 -
@@ -2,6 +2,9 @@
  * Copyright (c) 2005 Hans Petter Selasky
  * All rights reserved.
  *
+ * Smart Battery Data Specification
+ * http://sbs-forum.org/specs/sbdat110.pdf
+ *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:

Re: dhcpleased(8): ignore servers / parts of lease

[patrick keshishian]

On Sun, Aug 08, 2021 at 12:37:54PM +0200, Florian Obser wrote:
> This implements ignoring of nameservers and / or routes in leases as
> well as completely ignoring servers (you cannot block rogue DHCP servers
> in pf because bpf sees packets before pf).
> 
> Various people voiced the need for these features.
> Tests, OKs?
> 
> diff --git dhcpleased.c dhcpleased.c
> index 36a4a21c21a..e005d7de9ae 100644
> --- dhcpleased.c
> +++ dhcpleased.c
> @@ -569,6 +569,20 @@ main_dispatch_engine(int fd, short event, void *bula)
>   sizeof(imsg_interface.if_index));
>   break;
>   }
> + case IMSG_WITHDRAW_ROUTES: {
> + struct imsg_configure_interface imsg_interface;
> + if (IMSG_DATA_SIZE(imsg) != sizeof(imsg_interface))
> + fatalx("%s: IMSG_CONFIGURE_INTERFACE wrong "
> + "length: %lu", __func__,
> + IMSG_DATA_SIZE(imsg));
> + memcpy(&imsg_interface, imsg.data,
> + sizeof(imsg_interface));
> + if (imsg_interface.routes_len >= MAX_DHCP_ROUTES)
> + fatalx("%s: too many routes in imsg", __func__);
> + if (imsg_interface.routes_len > 0)
> + configure_routes(RTM_DELETE, &imsg_interface);
> + break;
> + }
>   case IMSG_PROPOSE_RDNS: {
>   struct imsg_propose_rdns rdns;
>   if (IMSG_DATA_SIZE(imsg) != sizeof(rdns))
> diff --git dhcpleased.conf.5 dhcpleased.conf.5
> index 9e6846f899e..b856113bac1 100644
> --- dhcpleased.conf.5
> +++ dhcpleased.conf.5
> @@ -57,6 +57,17 @@ A list of interfaces to overwrite defaults:
>  .Ic interface
>  options are as follows:
>  .Bl -tag -width Ds
> +.It Ic ignore dns
> +Ignore nameservers from leases on this interface.
> +The default is to not ignore nameservers.
> +.It Ic ignore routes
> +Ignore routes from leases on this interface.
> +The default is to not ignore routes.
> +.It Ic ignore Ar server-ip
> +Ignore leases from
> +.Ar server-ip .
> +This option can be listed multiple times.
> +The default is to not ignore servers.
>  .It Ic send client id Ar client-id
>  Send the dhcp client identifier option with a value of
>  .Ar client-id .
> diff --git dhcpleased.h dhcpleased.h
> index 7f6ec87ad19..1d20b77dbd3 100644
> --- dhcpleased.h
> +++ dhcpleased.h
> @@ -151,6 +151,12 @@
>  #define  DHCPRELEASE 7
>  #define  DHCPINFORM  8
>  
> +/* Ignore parts of DHCP lease */
> +#define  IGN_ROUTES  1
> +#define  IGN_DNS 2
> +
> +#define  MAX_SERVERS 16  /* max servers that can be ignores per 
> if */

Typo in comment: ignored (not ignores)

Should there be a mention of a limitation in the man page where
it states the option can be listed multiple times?

--patrick


> +
>  #define  IMSG_DATA_SIZE(imsg)((imsg).hdr.len - IMSG_HEADER_SIZE)
>  #define  DHCP_SNAME_LEN  64
>  #define  DHCP_FILE_LEN   128
> @@ -216,6 +222,7 @@ enum imsg_type {
>   IMSG_DECONFIGURE_INTERFACE,
>   IMSG_PROPOSE_RDNS,
>   IMSG_WITHDRAW_RDNS,
> + IMSG_WITHDRAW_ROUTES,
>   IMSG_REPROPOSE_RDNS,
>   IMSG_REQUEST_REBOOT,
>  };
> @@ -246,6 +253,9 @@ struct iface_conf {
>   int  vc_id_len;
>   uint8_t *c_id;
>   int  c_id_len;
> + int  ignore;
> + struct in_addr   ignore_servers[MAX_SERVERS];
> + int  ignore_servers_len;
>  };
>  
>  struct dhcpleased_conf {
> @@ -304,6 +314,8 @@ const char*sin_to_str(struct sockaddr_in *);
>  
>  /* frontend.c */
>  struct iface_conf*find_iface_conf(struct iface_conf_head *, char *);
> +int  *changed_ifaces(struct dhcpleased_conf *, struct
> +  dhcpleased_conf *);
>  
>  /* printconf.c */
>  void print_config(struct dhcpleased_conf *);
> diff --git engine.c engine.c
> index 076a57e9ba6..67693c358ee 100644
> --- engine.c
> +++ engine.c
> @@ -139,6 +139,7 @@ void   
> send_configure_interface(struct dhcpleased_iface *);
>  void  send_rdns_proposal(struct dhcpleased_iface *);
>  void  send_deconfigure_interface(struct dhcpleased_iface *);
>  void  send_rdns_withdraw(struct dhcpleased_iface *);
> +void  send_routes_withdraw(struct dhcpleased_iface *);
>  void  parse_lease(struct dhcpleased_iface *,
>struct imsg_ifinfo *);
>  int   engine_imsg_compose_main(int, pid_t, void *, uint16_t);
> @@ -506,13 +507,37 @@ engine_dispatch_main(int fd, short event, void *bula)
>   IMSG_DATA_SIZE(imsg));
>  

Re: awk FS behaviour change

[patrick keshishian]

On Sat, Jun 27, 2020 at 06:50:39AM +0100, Jason McIntyre wrote:
> On Fri, Jun 26, 2020 at 09:28:00PM -0600, Todd C. Miller wrote:
> > On Fri, 26 Jun 2020 23:56:23 +0200, Klemens Nanni wrote:
> > 
> > > How about adding something like "Therefore, FS should be set with -F or
> > > in a BEGIN block before input is read." as second sentence in this
> > > paragraph?
> > 
> > That whole section is missing important details.  I've tried to add
> > the missing info without being too repetitive.
> > 
> >  - todd
> > 
> > Index: usr.bin/awk/awk.1
> > ===
> > RCS file: /cvs/src/usr.bin/awk/awk.1,v
> > retrieving revision 1.54
> > diff -u -p -u -r1.54 awk.1
> > --- usr.bin/awk/awk.1   26 Jun 2020 21:50:06 -  1.54
> > +++ usr.bin/awk/awk.1   27 Jun 2020 03:25:48 -
> > @@ -129,27 +129,25 @@ and newlines are used as field separator
> >  .Va FS ) .
> >  This is convenient when working with multi-line records.
> >  .Pp
> > -An input line is normally made up of fields separated by whitespace,
> > -or by the regular expression
> > -.Va FS .
> > +An input line is normally made up of fields split based on the value
> > +of the field separator
> > +.Va FS
> > +at the time the line is read.
> 
> i'm not sure it reads better when we switch the emphasis from whitespace
> to FS. i think it's better that people see how it normally works, then
> the gories about FS. so i'd have kept the first part of the sentence,
> but maybe reworked the FS bit.
> 
> >  The fields are denoted
> >  .Va $1 , $2 , ... ,
> >  while
> >  .Va $0
> >  refers to the entire line.
> > -If
> >  .Va FS
> > -is null, the input line is split into one field per character.
> > -Lines are split into fields using the value of
> > +may be set to either a single character or a regular expression.
> > +As as special case, if
> >  .Va FS
> > -at the time the line is read.
> > -Because of this,
> > +is a single space
> > +.Pq the default ,
> > +fields will be split by one or more whitespace characters.
> > +If
> >  .Va FS
> > -is usually set via the
> > -.Fl F
> > -option or inside of a
> > -.Ic BEGIN
> > -block.
> > +is null, the input line is split into one field per character.
> >  .Pp
> >  Normally, any number of blanks separate fields.
> >  In order to set the field separator to a single blank, use the
> > @@ -171,6 +169,11 @@ as the field separator, use the
> >  .Fl F
> >  option with a value of
> >  .Sq [t] .
> > +The field separator is usually set via the
> > +.Fl F
> > +option or from inside of a
> 
> that sounds odd, but it may be a US/UK thing: i would say either "from
> inside a block" or "from the inside of a block".

Maybe "... from inside of the" rather than "... from inside of a"

--patrick

> 
> jmc
> 
> > +.Ic BEGIN
> > +block so that it takes effect before the input is read.
> >  .Pp
> >  A pattern-action statement has the form:
> >  .Pp
> > @@ -407,9 +410,9 @@ The name of the current input file.
> >  .It Va FNR
> >  Ordinal number of the current record in the current file.
> >  .It Va FS
> > -Regular expression used to separate fields; also settable
> > -by option
> > -.Fl F Ar fs .
> > +Regular expression used to separate fields (default whitespace);
> > +also settable by option
> > +.Fl F Ar fs
> >  .It Va NF
> >  Number of fields in the current record.
> >  .Va $NF
> > 
> 

Re: ipmi(4): don't block interrupts/systq long time

[patrick keshishian]

On Fri, Jun 07, 2019 at 07:29:19PM +0900, Naoki Fukaumi wrote:
> hi tech@,
> 
> here is another patch for another issue for ipmi(4).
> 
> ipmi_sendcmd() and ipmi_recvcmd() are always called in order/pairs as a
> single task of a single-threaded taskq, remove mutex from there. this
> avoids interrupts blocked long time.
> 
> task for watchdog can be long lived, use own taskq instead of systq.
> 
> --
> FUKAUMI Naoki
> 
> Index: sys/dev/ipmi.c
> ===
> RCS file: /cvs/src/sys/dev/ipmi.c,v
> retrieving revision 1.102
> diff -u -p -r1.102 ipmi.c
> --- sys/dev/ipmi.c15 Jun 2018 12:21:41 -  1.102
> +++ sys/dev/ipmi.c7 Jun 2019 10:00:10 -
> @@ -1037,14 +1037,10 @@ ipmi_cmd(struct ipmi_cmd *c)
>  void
>  ipmi_cmd_poll(struct ipmi_cmd *c)
>  {
> - mtx_enter(&c->c_sc->sc_cmd_mtx);
> -
>   if ((c->c_ccode = ipmi_sendcmd(c)))
>   printf("%s: sendcmd fails\n", DEVNAME(c->c_sc));
>   else
>   c->c_ccode = ipmi_recvcmd(c);
> -
> - mtx_leave(&c->c_sc->sc_cmd_mtx);
>  }
>  
>  void
> @@ -1653,7 +1649,6 @@ ipmi_match(struct device *parent, void *
>  
>   /* XXX local softc is wrong wrong wrong */
>   sc = malloc(sizeof(*sc), M_TEMP, M_WAITOK | M_ZERO);
> - mtx_init(&sc->sc_cmd_mtx, IPL_MPFLOOR);
>   strlcpy(sc->sc_dev.dv_xname, "ipmi0", sizeof(sc->sc_dev.dv_xname));
>  
>   /* Map registers */
> @@ -1693,15 +1688,31 @@ ipmi_attach(struct device *parent, struc
>   /* Map registers */
>   ipmi_map_regs(sc, ia);
>  
> + /* Setup taskqs */
> + sc->sc_cmd_taskq = taskq_create("ipmicmd", 1, IPL_NONE, TASKQ_MPSAFE);
> + if (sc->sc_cmd_taskq == NULL) {
> + printf(": unable to allocate cmd taskq\n");
> + return;
> + }
> + sc->sc_wdog_taskq = taskq_create("ipmiwdog", 1, IPL_SOFTCLOCK, 0);
> + if (sc->sc_cmd_taskq == NULL) {

paste-o?

> + taskq_destroy(sc->sc_cmd_taskq);
> + printf(": unable to allocate wdog taskq\n");
> + return;
> + }
> + task_set(&sc->sc_wdog_tickle_task, ipmi_watchdog_tickle, sc);
> +
> + /* Setup thread */
>   sc->sc_thread = malloc(sizeof(struct ipmi_thread), M_DEVBUF, M_NOWAIT);
>   if (sc->sc_thread == NULL) {
> + taskq_destroy(sc->sc_cmd_taskq);
> + taskq_destroy(sc->sc_wdog_taskq);
>   printf(": unable to allocate thread\n");
>   return;
>   }
>   sc->sc_thread->sc = sc;
>   sc->sc_thread->running = 1;
>  
> - /* Setup threads */
>   kthread_create_deferred(ipmi_create_thread, sc);
>  
>   printf(": version %d.%d interface %s %sbase 0x%x/%x spacing %d",
> @@ -1717,16 +1728,12 @@ ipmi_attach(struct device *parent, struc
>  
>   /* Setup Watchdog timer */
>   sc->sc_wdog_period = 0;
> - task_set(&sc->sc_wdog_tickle_task, ipmi_watchdog_tickle, sc);
>   wdog_register(ipmi_watchdog, sc);
>  
>   rw_init(&sc->sc_ioctl.lock, DEVNAME(sc));
>   sc->sc_ioctl.req.msgid = -1;
>   c->c_sc = sc;
>   c->c_ccode = -1;
> -
> - sc->sc_cmd_taskq = taskq_create("ipmicmd", 1, IPL_NONE, TASKQ_MPSAFE);
> - mtx_init(&sc->sc_cmd_mtx, IPL_MPFLOOR);
>  }
>  
>  int
> @@ -1896,8 +1903,8 @@ ipmi_watchdog(void *arg, int period)
>   int res;
>  
>   t = &sc->sc_wdog_tickle_task;
> - (void)task_del(systq, t);
> - res = task_add(systq, t);
> + (void)task_del(sc->sc_wdog_taskq, t);
> + res = task_add(sc->sc_wdog_taskq, t);
>   KASSERT(res == 1);
>   }
>   return (period);
> Index: sys/dev/ipmivar.h
> ===
> RCS file: /cvs/src/sys/dev/ipmivar.h,v
> retrieving revision 1.28
> diff -u -p -r1.28 ipmivar.h
> --- sys/dev/ipmivar.h 5 Feb 2016 06:29:01 -   1.28
> +++ sys/dev/ipmivar.h 7 Jun 2019 10:00:10 -
> @@ -112,7 +112,6 @@ struct ipmi_softc {
>   int sc_btseq;
>   u_int8_tsc_buf[IPMI_MAX_RX + 16];
>   struct taskq*sc_cmd_taskq;
> - struct mutexsc_cmd_mtx;
>  
>   struct ipmi_ioctl {
>   struct rwlock   lock;
> @@ -122,6 +121,7 @@ struct ipmi_softc {
>   } sc_ioctl;
>  
>   int sc_wdog_period;
> + struct taskq*sc_wdog_taskq;
>   struct task sc_wdog_tickle_task;
>  
>   struct ipmi_thread  *sc_thread;
> 

Re: sed(1) make -i behave a little nicer

[patrick keshishian]

On Fri, Dec 07, 2018 at 08:41:21AM +0100, Martijn van Duren wrote:
> I ran into a few minor nuisances with sed's -i mode, which are mostly 
> compatible with gnu sed, but I reckon we should address.
> 
> The problem is sed works by writing the output to a second file in the
> same directory as the original and after completion renaming the file
> to the original. This has two disadvantages:
> 1) The inode number changes, resulting in loss of carefully crafted
>hardlinks.
> 2) We require to have write permission in the same directory as the
>original file, even if we don't want to have a backup file.
> 
> Diff below tries to solve this by doing the following.
> Copy the file to the backup location (/tmp/sedXX if no
> extension) and use the backup as the infile and the original as the
> outfile.
> 
> Furthermore I changed the lstat to fstat, so we can edit symlinks (gsed
> supports symlinks by replacing the symlink by a new real file, which is
> also fun), and I extended the warning messages in process to show the
> backup file if we crash during operation, so people will always know
> where to recover the file in case of disaster.
> 
> Because process also error(FATAL, ...)s during process and we always
> have a backup file I don't think the warning in sed.1 is worth keeping.
> 
> The only downside to this new approach (that I can think of) is that we
> now temporarily have a file that is in an inconsistent state, but that's
> not much different to writing a file with any other editor.
> 
> $ echo test > /usr/obj/test && dd if=/dev/zero of=/usr/obj/bloat
> /usr/obj: write failed, file system is full
> dd: /usr/obj/bloat: No space left on device
> 614113+0 records in
> 614112+0 records out
> 314425344 bytes transferred in 2.206 secs (142470196 bytes/sec)
> $ ./obj/sed -i -f /tmp/test.sed /usr/obj/test 
>  
> 
> /usr/obj: write failed, file system is full
> sed: /usr/obj/test: No space left on device (backup at /tmp/sedgRPSLImG9N)
> $ cat /tmp/test.sed
> s/test/aaa/
> $ cat /tmp/sedgRPSLImG9N 
> test
> $ ls -i /tmp/test
> 104 /tmp/test
> $ sed -i s/test/foo/ /tmp/test
> $ ls -i /tmp/test 
> 104 /tmp/test
> $ doas touch /etc/test
> $ doas chown martijn /etc/test
> $ echo test > /etc/test
> $ sed -i s/test/foo/ /etc/test
> $ cat /etc/test 
> foo
> 
> The diff does change quite a few mechanics, so some scrutiny is welcome.
> 
> martijn@
> 
> Index: main.c
> ===
> RCS file: /cvs/src/usr.bin/sed/main.c,v
> retrieving revision 1.39
> diff -u -p -r1.39 main.c
> --- main.c6 Dec 2018 20:16:04 -   1.39
> +++ main.c7 Dec 2018 07:31:54 -
> @@ -35,6 +35,7 @@
>  
>  #include 
>  #include 
> +#include 
>  #include 
>  
>  #include 
> @@ -94,13 +95,13 @@ static int rval;  /* Exit status */
>   */
>  const char *fname;   /* File name. */
>  const char *outfname;/* Output file name */
> -static char oldfname[PATH_MAX];  /* Old file name (for in-place editing) 
> */
> -static char tmpfname[PATH_MAX];  /* Temporary file name (for in-place 
> editing) */
> +char oldfname[PATH_MAX]; /* Old file name (for in-place editing) */
>  char *inplace;   /* Inplace edit file extension */
>  u_long linenum;
>  
>  static void add_compunit(enum e_cut, char *);
>  static void add_file(char *);
> +int copy(FILE *, FILE *);
>  static int next_files_have_lines(void);
>  
>  int termwidth;
> @@ -310,26 +311,46 @@ again:
>   return (NULL);
>  }
>  
> +int
> +copy(FILE *src, FILE *dst)
> +{
> + unsigned char buf[MAXBSIZE];
> + size_t r, w, tw;
> +
> + while(1) {
> + if ((r = fread(buf, sizeof(*buf), sizeof(buf), src)) == 0) {
> + if (feof(src)) {
> + if (fflush(dst) == EOF)
> + return 0;
> + return 1;
> + }
> + if (errno != EINTR)
> + return 0;
> + continue;
> + }
> + tw = 0;
> + while(tw < r) {
> + w = fwrite(buf + tw, sizeof(*buf), r - tw, dst);
> + if (w == 0) {
> + if (errno != EINTR)
> + return 0;
> + continue;
> + }
> + tw += w;
> + }
> + }
> +}
> +
>  void
>  finish_file(void)
>  {
>   if (infile != NULL) {
>   fclose(infile);
> - if (*oldfname != '\0') {
> - if (rename(fname, oldfname) != 0) {
> - warning("rename()");
> - unlink(tmpfname);
> - exit(1);
> - }
> + if (inplace != NULL) {
> +  

Re: mktemp: Clarify error message

[patrick keshishian]

On 12/25/17, Otto Moerbeek  wrote:
> On Tue, Dec 26, 2017 at 12:31:02AM +0100, Klemens Nanni wrote:
>
>> On Mon, Dec 25, 2017 at 03:57:00PM -0700, Theo de Raadt wrote:
>> > I think this is a silly solution, and the documentation is clear
>> > enough.
>> The manual page certainly is clear enough but the current error message
>> is logically wrong, as there are sufficient Xs *in* `XXs' but just
>> not at the end of it, call it nitpicking if you will.
>>
>> > How did this happen to you?  Show the place where it happened to you.
>> > Would the text you propose actually have saved you 1 second of time
>> > to help you realize what was wrong?  I don't think so.
>> Just a typo really making me think "this could be clearer". So yes, I
>> find telling this way actually saves time understanding the error, even
>> if so little.
>>
>> > If you weren't familiar that the template has to be minimum 6 XX at
>> > end of the string, then you hadn't achieved familiarity of the
>> > subject matter yet.
>> I agree that knowing one from the manual implies knowing the other as
>> well, but it doesn't seem reason enough to keep the error message as is,
>> hence the diff.
>
> I disagree. An error message does not need to document everything, An
> erro message should short and clear enough together with the doumentation.

all well and good, but let's not drop words and letters in pursuit of brevity.

-pk

> This reminds me of the old IRIX compiler, that would cite complete
> parapgraphs of the C standard in error mesasges. Of course logically
> it was all correct, but it lead to long and unreadable error messages
> that filled up disks with build logs.
>
>   -Otto
>
>
>
>

Re: enum unsigned or not?

[patrick keshishian]

On Thu, Aug 31, 2017 at 12:13:19PM -0700, William Ahern wrote:
> On Thu, Aug 31, 2017 at 02:08:07PM +0200, Otto Moerbeek wrote:
> > Hi,
> > 
> > /usr/src/usr.sbin/sasyncd/carp.c:157:12: warning: comparison of
> > unsigned enum expression < 0 is always false [-Wtautological-compare]
> > if (state < 0 || state > FAIL)
> > ~ ^ ~
> > /usr/src/usr.sbin/sasyncd/carp.c:166:20: warning: comparison of
> > unsigned enum expression < 0 is always false [-Wtautological-compare]
> > if (current_state < 0 || current_state > FAIL) {
> > ~ ^ ~

if (!(state >= INIT && state <= FAIL))
state = FAIL;
and
if (!(current_sate >= INIT && current_state <= FAIL) {
log_err ...
...
return;
}

More better?

--patrick

> > 
> > this warning is a tiny bit interesting. A compiler is free to choose
> > the type of the enum, as long as it can represent all given values.
> > So another compiler might choose not to make it unsigned. So I came up
> > with this fix that is not depending on the signedness of the type. But
> > most of the time avoiding enum is better, I suppose.
> 
> It's free to choose the integer type of the enum, but enumeration members
> (i.e. the constant identifiers) have int type.
> 
>   The identifiers in an enumerator list are declared as constants that have
>   type int and may appear wherever such are permitted.
> 
>   C11 (N1570) 6.7.2.2p3.
> 
> Furthermore, the defining expression of the constant must be representable
> as an int. 6.7.2.2p2.
> 
> I've always vascillated about which operand to cast, and to which type, when
> silencing compilers' annoying warnings. But now that I've read the section
> more closely I think I'll just cast the operand with enum type to int, all
> things being equal.
> 

Re: [PATCH] rm(1): add -v option for verbosity

[patrick keshishian]

On Sun, Jun 25, 2017 at 11:16:00PM +0200, Ingo Schwarze wrote:
> Hi,
> 
> Paul de Weerd wrote on Sun, Jun 25, 2017 at 10:46:15PM +0200:
> > On Sun, Jun 25, 2017 at 03:12:26PM +0200, Ingo Schwarze wrote:
> 
> > | If you are really unsure, study the output of
> > |   $ find *
> > | first, before typing
> > |   $ rm -rf *
> > | No non-standard option is needed at all for this.
> 
> > Very bad example.  You study the output of find * and another process
> > writes a new file in the mean time.  With rm -v, you would've known.
> 
> I don't buy the talk about race conditions at all.
> 
> If other processes may be writing important files there, then for
> god's sake don't rm -rf it.  Besides, the rm -v output arrives too
> late, when the data is already gone.
> 
> If the directory is full of transient files and removing them is OK,
> than it obviously doesn't matter what exactly got removed.
> 
> Seriously, files not important enough to be kept, but so important
> that you need an audit trail concerning their removal?  That sounds
> like a very unusual use case at best.
> 
> 
> Landry's argument about the progress-meter has some merit, but it
> is clearly more important for copying than for removing, and it is
> clearly more important for copying over the network than for plain
> cp(1), so i'm not sure it is sufficient justification.

scp -v?

$ mkdir ../abc
$ scp -v ./*pdf ../abc/
Executing: cp -- ./iso-prog_lang_C-WG14-N869.pdf ../abc/
Executing: cp -- ./iso-prog_lang_C.WG14-N897.pdf ../abc/
Executing: cp -- ./n1124.pdf ../abc/
Executing: cp -- ./n1256.pdf ../abc/
Executing: cp -- ./n1570.pdf ../abc/
...
$ scp -v n1570.pdf ../abc/single_file.pdf
Executing: cp -- n1570.pdf ../abc/single_file.pdf

-pk

> Anyway, the whole thread looks awfully bikeshedish, so i guess
> i should set a better example and shut up...
> 
> Yours,
>   Ingo
> 
> 
> P.S.
> On the humorous side:
> 
> Landry mentioned about rsync:
> 
> > Technically it could be possible to replicate mv with rsync
> > --remove-source-files ... :)
> 
> Talk about the kitchen sink...
> 
> And i heard rumours that some rsync(1) command line options
> are so long that they don't fit into the command line length
> limit of some shells.  ;-)
> 

Re: detect post threaded condition

[patrick keshishian]

On Tue, May 30, 2017 at 03:48:06AM -0400, Ted Unangst wrote:
> talking to stsp, he reminded me of a problematic bug that took some time to
> track down in some desktop software that shall not be named. after a program
> calls fork(), the child has only a single thread. per the standard, it needs
> to get to exec() as quickly as possible. per the quality standards of modern
> software, this doesn't always happen, and then strangeness happens.
> 
> this adds a quick check for this error condition. after a fork, we set a new
> variable in the child that we are post threaded. this can be checked elsewhere
> in the library. i added a check to pthread_join because that's a notable
> problem function.
> 
> 
> Index: rthread.c
> ===
> RCS file: /cvs/src/lib/librthread/rthread.c,v
> retrieving revision 1.94
> diff -u -p -r1.94 rthread.c
> --- rthread.c 4 Sep 2016 10:13:35 -   1.94
> +++ rthread.c 30 May 2017 07:43:12 -
> @@ -64,6 +64,7 @@ REDIRECT_SYSCALL(thrkill);
>  static int concurrency_level;/* not used */
>  
>  int _threads_ready;
> +int _post_threaded;
>  size_t _thread_pagesize;
>  struct listhead _thread_list = LIST_HEAD_INITIALIZER(_thread_list);
>  _atomic_lock_t _thread_lock = _SPINLOCK_UNLOCKED;
> @@ -358,6 +359,11 @@ pthread_join(pthread_t thread, void **re
>   pthread_t self;
>   PREP_CANCEL_POINT(tib);
>  
> + if (_post_threaded) {
> +#define GREATSCOTT "great scott! serious repurcussions on future events!\n"
> + write(2, GREATSCOTT, sizeof(GREATSCOTT));
 ^^
maybe sizeof(GREATSCOTT)-1

-pk

> + abort();
> + }
>   if (!_threads_ready)
>   _rthread_init();
>   self = tib->tib_thread;
> Index: rthread_fork.c
> ===
> RCS file: /cvs/src/lib/librthread/rthread_fork.c,v
> retrieving revision 1.19
> diff -u -p -r1.19 rthread_fork.c
> --- rthread_fork.c4 Sep 2016 10:13:35 -   1.19
> +++ rthread_fork.c30 May 2017 07:41:29 -
> @@ -58,6 +58,7 @@ _dofork(pid_t (*sys_fork)(void))
>   pthread_t me;
>   pid_t newid;
>   int i;
> + extern int _post_threaded;
>  
>   if (!_threads_ready)
>   return sys_fork();
> @@ -110,6 +111,7 @@ _dofork(pid_t (*sys_fork)(void))
>  
>   /* single threaded now */
>   __isthreaded = 0;
> + _post_threaded = 1;
>   }
>  #ifndef NO_PIC
>   else if (_DYNAMIC)
> 

Re: ulpt read interface

[patrick keshishian]

Ping?


On Thu, May 18, 2017 at 07:35:31PM -0700, patrick keshishian wrote:
> Hello,
> 
> I would like to propose adding a ulptread call to the ulpt(4).
> 
> This diff adds ulptread and ulpt_do_read functions, similar to
> ultpwrite and ulpt_do_write (essentially copy/paste/edit), as well
> as looking at ugen_do_read (for BULK).
> 
> 
> One thing to note is the current ultp driver runs two continuous
> transfers and stashes any data into sc_junk. This was added in:
> 
> | revision 1.7
> | date: 2001/05/03 02:20:34;  author: aaron;  state: Exp;  lines: +165 -40;
> | branches:  1.7.2;
> | Sync with NetBSD. Tested with a USB keyboard, USB mouse, and three different
> | kue(4) Ethernet devices.
> 
> I think this matches revision 1.42 on NetBSD.
> 
> I am not sure of the original reason for this continuous read.
> Therefore, to minimize any potential fallout, this diff checks
> the flag to ulptopen call for FREAD, and if set, it doesn't
> start the continuous transfers to sc_junk.
> 
> 
> Looking at NetBSD source history now, they added read to their driver
> in revision 1.60:
> 
> | Several changes:
> | * Implement read for ulpt.
> | * If the device is not opened for reading, occasionally drain any
> |   data the printer might have (but don't hammer the printer with reads).
> | * Lower the buffer size to one page.
> |   The driver seems to work with more printers now.
> 
> 
> They similarly check for FREAD in their open, and if "it's not
> opened for read the [sic] set up a reader."
> 
> 
> The reason for the proposal is that I have a USB thermal printer
> which attaches to ulpt and for me to "drive" the printer, I need
> the status messages the printer sends back.
> 
> I initially got the device working with ulpt disabled and using
> libusb1 from ports. It would be much more convenient not to bother
> with disabling ulpt.
> 
> 
> Comments?
> 
> --patrick
> 
> 
> 
> 
> Index: sys/conf.h
> ===
> RCS file: /cvs/obsd/src/sys/sys/conf.h,v
> retrieving revision 1.143
> diff -u -p -u -p -r1.143 conf.h
> --- sys/conf.h4 Sep 2016 10:51:24 -   1.143
> +++ sys/conf.h19 May 2017 01:39:18 -
> @@ -356,9 +356,9 @@ extern struct cdevsw cdevsw[];
>   (dev_type_stop((*))) enodev, 0, selfalse, \
>   (dev_type_mmap((*))) enodev }
>  
> -/* open, close, write */
> +/* open, close, read, write */
>  #define cdev_ulpt_init(c,n) { \
> - dev_init(c,n,open), dev_init(c,n,close), (dev_type_read((*))) enodev, \
> + dev_init(c,n,open), dev_init(c,n,close), dev_init(c,n,read), \
>   dev_init(c,n,write), (dev_type_ioctl((*))) enodev, \
>   (dev_type_stop((*))) enodev, 0, selfalse, (dev_type_mmap((*))) enodev }
>  
> Index: dev/usb/ulpt.c
> ===
> RCS file: /cvs/obsd/src/sys/dev/usb/ulpt.c,v
> retrieving revision 1.54
> diff -u -p -u -p -r1.54 ulpt.c
> --- dev/usb/ulpt.c26 Mar 2017 15:31:15 -  1.54
> +++ dev/usb/ulpt.c19 May 2017 01:39:18 -
> @@ -42,6 +42,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -486,7 +487,7 @@ ulptopen(dev_t dev, int flag, int mode, 
>   if (usbd_is_dying(sc->sc_udev)) {
>   error = ENXIO;
>   sc->sc_state = 0;
> - goto done;
> + goto bad;
>   }
>   }
>  
> @@ -494,18 +495,20 @@ ulptopen(dev_t dev, int flag, int mode, 
>   if (err) {
>   sc->sc_state = 0;
>   error = EIO;
> - goto done;
> + goto bad;
>   }
>   if (ulptusein && sc->sc_in != -1) {
> - DPRINTF(("ulpt_open: open input pipe\n"));
> + DPRINTF(("ulptopen: open input pipe\n"));
>   err = usbd_open_pipe(sc->sc_iface, sc->sc_in,0,&sc->sc_in_pipe);
>   if (err) {
>   error = EIO;
>   usbd_close_pipe(sc->sc_out_pipe);
>   sc->sc_out_pipe = NULL;
>   sc->sc_state = 0;
> - goto done;
> + goto bad;
>   }
> + if ((flag & FREAD) == FREAD)
> + goto done;
>   sc->sc_in_xfer1 = usbd_alloc_xfer(sc->sc_udev);
>   sc->sc_in_xfer2 = usbd_alloc_xfer(sc->sc_udev);
>   if (sc->sc_in_xfer1 == NULL || sc->sc_in_xfer2 == NULL) {
> @@ 

ulpt read interface

[patrick keshishian]

Hello,

I would like to propose adding a ulptread call to the ulpt(4).

This diff adds ulptread and ulpt_do_read functions, similar to
ultpwrite and ulpt_do_write (essentially copy/paste/edit), as well
as looking at ugen_do_read (for BULK).


One thing to note is the current ultp driver runs two continuous
transfers and stashes any data into sc_junk. This was added in:

| revision 1.7
| date: 2001/05/03 02:20:34;  author: aaron;  state: Exp;  lines: +165 -40;
| branches:  1.7.2;
| Sync with NetBSD. Tested with a USB keyboard, USB mouse, and three different
| kue(4) Ethernet devices.

I think this matches revision 1.42 on NetBSD.

I am not sure of the original reason for this continuous read.
Therefore, to minimize any potential fallout, this diff checks
the flag to ulptopen call for FREAD, and if set, it doesn't
start the continuous transfers to sc_junk.


Looking at NetBSD source history now, they added read to their driver
in revision 1.60:

| Several changes:
| * Implement read for ulpt.
| * If the device is not opened for reading, occasionally drain any
|   data the printer might have (but don't hammer the printer with reads).
| * Lower the buffer size to one page.
|   The driver seems to work with more printers now.


They similarly check for FREAD in their open, and if "it's not
opened for read the [sic] set up a reader."


The reason for the proposal is that I have a USB thermal printer
which attaches to ulpt and for me to "drive" the printer, I need
the status messages the printer sends back.

I initially got the device working with ulpt disabled and using
libusb1 from ports. It would be much more convenient not to bother
with disabling ulpt.


Comments?

--patrick




Index: sys/conf.h
===
RCS file: /cvs/obsd/src/sys/sys/conf.h,v
retrieving revision 1.143
diff -u -p -u -p -r1.143 conf.h
--- sys/conf.h  4 Sep 2016 10:51:24 -   1.143
+++ sys/conf.h  19 May 2017 01:39:18 -
@@ -356,9 +356,9 @@ extern struct cdevsw cdevsw[];
(dev_type_stop((*))) enodev, 0, selfalse, \
(dev_type_mmap((*))) enodev }
 
-/* open, close, write */
+/* open, close, read, write */
 #define cdev_ulpt_init(c,n) { \
-   dev_init(c,n,open), dev_init(c,n,close), (dev_type_read((*))) enodev, \
+   dev_init(c,n,open), dev_init(c,n,close), dev_init(c,n,read), \
dev_init(c,n,write), (dev_type_ioctl((*))) enodev, \
(dev_type_stop((*))) enodev, 0, selfalse, (dev_type_mmap((*))) enodev }
 
Index: dev/usb/ulpt.c
===
RCS file: /cvs/obsd/src/sys/dev/usb/ulpt.c,v
retrieving revision 1.54
diff -u -p -u -p -r1.54 ulpt.c
--- dev/usb/ulpt.c  26 Mar 2017 15:31:15 -  1.54
+++ dev/usb/ulpt.c  19 May 2017 01:39:18 -
@@ -42,6 +42,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -486,7 +487,7 @@ ulptopen(dev_t dev, int flag, int mode, 
if (usbd_is_dying(sc->sc_udev)) {
error = ENXIO;
sc->sc_state = 0;
-   goto done;
+   goto bad;
}
}
 
@@ -494,18 +495,20 @@ ulptopen(dev_t dev, int flag, int mode, 
if (err) {
sc->sc_state = 0;
error = EIO;
-   goto done;
+   goto bad;
}
if (ulptusein && sc->sc_in != -1) {
-   DPRINTF(("ulpt_open: open input pipe\n"));
+   DPRINTF(("ulptopen: open input pipe\n"));
err = usbd_open_pipe(sc->sc_iface, sc->sc_in,0,&sc->sc_in_pipe);
if (err) {
error = EIO;
usbd_close_pipe(sc->sc_out_pipe);
sc->sc_out_pipe = NULL;
sc->sc_state = 0;
-   goto done;
+   goto bad;
}
+   if ((flag & FREAD) == FREAD)
+   goto done;
sc->sc_in_xfer1 = usbd_alloc_xfer(sc->sc_udev);
sc->sc_in_xfer2 = usbd_alloc_xfer(sc->sc_udev);
if (sc->sc_in_xfer1 == NULL || sc->sc_in_xfer2 == NULL) {
@@ -534,9 +537,10 @@ ulptopen(dev_t dev, int flag, int mode, 
usbd_transfer(sc->sc_in_xfer1); /* ignore failed start */
}
 
+ done:
sc->sc_state = ULPT_OPEN;
 
- done:
+ bad:
if (--sc->sc_refcnt < 0)
usb_detach_wakeup(&sc->sc_dev);
 
@@ -596,6 +600,71 @@ ulptclose(dev_t dev, int flag, int mode,
 
DPRINTF(("ulptclose: closed\n"));
return (0);
+}
+
+int
+ulpt_do_read(struct ulpt_softc *sc, struct uio *uio, int flags)
+{
+   size_t n;
+   int error = 0;
+   uint32_t bytes;
+   void *bufp;
+   struct usbd_xfer *xfer;
+   usbd_status err;
+
+   DPRINTF(("ulptread\n"));
+   xfer = usbd_alloc_xfer(sc->sc_udev);
+   if (xfer == NULL)
+   retur

Re: unmount mountpoints

[patrick keshishian]

my initial attempt to send a response is not moving out of the
queue...so here is a second attempt.

On 1/10/17, Alexander Bluhm  wrote:
> Hi,
>
> When I force to unmount a filesystem where another mountpoint is
> located, an unlinked mountpoint will remain.  I have not found a
> way to restore the kernel to a sane state.
>
> # mount /dev/vnd0a /mnt
> # mkdir /mnt/mnt
> # mount /dev/vnd1a /mnt/mnt
> # mount
> /dev/sd0a on / type ffs (local)
> /dev/vnd0a on /mnt type ffs (local)
> /dev/vnd1a on /mnt/mnt type ffs (local)
> # umount /mnt
> umount: /mnt: Device busy
> # umount -f /mnt
> # mount
> /dev/sd0a on / type ffs (local)
> /dev/vnd1a on /mnt/mnt type ffs (local)
> # umount /mnt/mnt
> umount: /mnt/mnt: No such file or directory

does

# umount /dev/vnd1a

not do the trick?

--patrick


> # mkdir /mnt/mnt
> # umount /mnt/mnt
> umount: /mnt/mnt: Invalid argument
> # vnconfig -u vnd1
> vnconfig: VNDIOCCLR: Device busy
>
> The fix could be to unmount recursively.
>
> ok?
>
> bluhm
>
> Index: kern/vfs_syscalls.c
> ===
> RCS file: /cvs/src/sys/kern/vfs_syscalls.c,v
> retrieving revision 1.267
> diff -u -p -r1.267 vfs_syscalls.c
> --- kern/vfs_syscalls.c   10 Jan 2017 20:13:17 -  1.267
> +++ kern/vfs_syscalls.c   10 Jan 2017 20:36:05 -
> @@ -88,6 +88,7 @@ int domkdirat(struct proc *, int, const
>  int doutimensat(struct proc *, int, const char *, struct timespec [2],
> int);
>  int dovutimens(struct proc *, struct vnode *, struct timespec [2]);
>  int dofutimens(struct proc *, int, struct timespec [2]);
> +int unmount_vnode(struct vnode *, void *);
>
>  /*
>   * Virtual File System System Calls
> @@ -368,15 +369,56 @@ sys_unmount(struct proc *p, void *v, reg
>   return (dounmount(mp, SCARG(uap, flags) & MNT_FORCE, p));
>  }
>
> +struct unmount_args {
> + struct proc *ua_proc;
> + int  ua_flags;
> + int  ua_error;
> +};
> +
> +int
> +unmount_vnode(struct vnode *vp, void *args)
> +{
> + struct unmount_args *ua = args;
> + struct mount *mp;
> + int error;
> +
> + if (vp->v_type != VDIR)
> + return (0);
> + if ((mp = vp->v_mountedhere) == NULL)
> + return (0);
> + if (!(ua->ua_flags & MNT_FORCE)) {
> + ua->ua_error = EBUSY;
> + return (EBUSY);
> + }
> + if (vfs_busy(mp, VB_WRITE|VB_WAIT)) {
> + ua->ua_error = EBUSY;
> + return (0);
> + }
> + error = dounmount(mp, ua->ua_flags, ua->ua_proc);
> + if (error)
> + ua->ua_error = error;
> + return (0);
> +}
> +
>  /*
>   * Do the actual file system unmount.
>   */
>  int
>  dounmount(struct mount *mp, int flags, struct proc *p)
>  {
> + struct unmount_args ua;
>   struct vnode *coveredvp;
>   int error;
>   int hadsyncer = 0;
> +
> + ua.ua_proc = p;
> + ua.ua_flags = flags;
> + ua.ua_error = 0;
> + vfs_mount_foreach_vnode(mp, unmount_vnode, &ua);
> + if (ua.ua_error && !(flags & MNT_DOOMED)) {
> + vfs_unbusy(mp);
> + return (ua.ua_error);
> + }
>
>   mp->mnt_flag &=~ MNT_ASYNC;
>   cache_purgevfs(mp); /* remove cache entries for this file sys */
>
>

Re: paste-o fix in comment + load average question

[patrick keshishian]

Ahh... seems the culprit is softclock_thread added 2016/09/22
(kern/kern_timeout.c mpi@).

I guess next question is whether OpenBSD will live (and document)
this change (increase) in load average, or some sort of counter
measure will be taken in uvm_loadav() to suppress the extra bump
in the value?

Cheers,
--patrick


On Fri, Nov 11, 2016 at 01:23:26PM -0800, patrick keshishian wrote:
> Hi,
> 
> For a long time I was running an amd64 snapshot from 2016/03/30,
> then I installed a snapshot from 2016/10/09 on four machines
> (two laptops, a desktop and a 1U supermicro).
> 
> One apparent difference was that the load average on all four
> machines idled at slightly above 1 (1.12, 1.18, ...), about 1
> more than previous snapshots (up and including March 30th).
> 
> Initially I assumed maybe something in the snapshot(s), which
> would eventually go away with future updates.
> 
> I built a couple of kernels from source since 2016/10/09 snapshot
> and each still runs at load avg of 1 more than expected when the
> box is completely idle.
> 
> I briefly looked at cvs history (cvs log -d '2016/03/30<') and
> failed to see any obvious change that would attribute to this
> apparent display/reporting "glitch?"
> 
> Can someone enlighten me on this issue?
> 
> Thanks in advance,
> 
> --patrick
> 
> 
> 
> 
> Index: kern_pledge.c
> ===
> RCS file: /cvs/obsd/src/sys/kern/kern_pledge.c,v
> retrieving revision 1.187
> diff -u -p -u -p -r1.187 kern_pledge.c
> --- kern_pledge.c 27 Oct 2016 10:48:25 -  1.187
> +++ kern_pledge.c 11 Nov 2016 20:56:21 -
> @@ -1046,7 +1046,7 @@ pledge_sysctl(struct proc *p, int miblen
>   if (miblen == 2 &&  /* hw.ncpu */
>   mib[0] == CTL_HW && mib[1] == HW_NCPU)
>   return (0);
> - if (miblen == 2 &&  /* kern.loadavg / getloadavg(3) */
> + if (miblen == 2 &&  /* vm.loadavg / getloadavg(3) */
>   mib[0] == CTL_VM && mib[1] == VM_LOADAVG)
>   return (0);

paste-o fix in comment + load average question

[patrick keshishian]

Hi,

For a long time I was running an amd64 snapshot from 2016/03/30,
then I installed a snapshot from 2016/10/09 on four machines
(two laptops, a desktop and a 1U supermicro).

One apparent difference was that the load average on all four
machines idled at slightly above 1 (1.12, 1.18, ...), about 1
more than previous snapshots (up and including March 30th).

Initially I assumed maybe something in the snapshot(s), which
would eventually go away with future updates.

I built a couple of kernels from source since 2016/10/09 snapshot
and each still runs at load avg of 1 more than expected when the
box is completely idle.

I briefly looked at cvs history (cvs log -d '2016/03/30<') and
failed to see any obvious change that would attribute to this
apparent display/reporting "glitch?"

Can someone enlighten me on this issue?

Thanks in advance,

--patrick




Index: kern_pledge.c
===
RCS file: /cvs/obsd/src/sys/kern/kern_pledge.c,v
retrieving revision 1.187
diff -u -p -u -p -r1.187 kern_pledge.c
--- kern_pledge.c   27 Oct 2016 10:48:25 -  1.187
+++ kern_pledge.c   11 Nov 2016 20:56:21 -
@@ -1046,7 +1046,7 @@ pledge_sysctl(struct proc *p, int miblen
if (miblen == 2 &&  /* hw.ncpu */
mib[0] == CTL_HW && mib[1] == HW_NCPU)
return (0);
-   if (miblen == 2 &&  /* kern.loadavg / getloadavg(3) */
+   if (miblen == 2 &&  /* vm.loadavg / getloadavg(3) */
mib[0] == CTL_VM && mib[1] == VM_LOADAVG)
return (0);
 



OpenBSD 6.0-current (GENERIC.MP) #2553: Sun Oct  9 20:50:35 MDT 2016
dera...@amd64.openbsd.org:/usr/src/sys/arch/amd64/compile/GENERIC.MP
real mem = 4277665792 (4079MB)
avail mem = 4143493120 (3951MB)
mpath0 at root
scsibus0 at mpath0: 256 targets
mainbus0 at root
bios0 at mainbus0: SMBIOS rev. 2.6 @ 0x9f000 (19 entries)
bios0: vendor American Megatrends Inc. version "1.1a" date 12/17/10
bios0: Supermicro X7SPA-HF
acpi0 at bios0: rev 2
acpi0: sleep states S0 S1 S4 S5
acpi0: tables DSDT FACP APIC MCFG OEMB HPET EINJ BERT ERST HEST
acpi0: wakeup devices P0P1(S4) USB0(S4) USB1(S4) USB2(S4) USB5(S4) EUSB(S4) 
USB3(S4) USB4(S4) USB6(S4) USBE(S4) P0P4(S4) P0P5(S4) P0P6(S4) P0P7(S4) 
P0P8(S4) P0P9(S4) [...]
acpitimer0 at acpi0: 3579545 Hz, 24 bits
acpimadt0 at acpi0 addr 0xfee0: PC-AT compat
cpu0 at mainbus0: apid 0 (boot processor)
cpu0: Intel(R) Atom(TM) CPU D525 @ 1.80GHz, 1800.23 MHz
cpu0: 
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,DTES64,MWAIT,DS-CPL,TM2,SSSE3,CX16,xTPR,PDCM,MOVBE,NXE,LONG,LAHF,PERF,SENSOR
cpu0: 512KB 64b/line 8-way L2 cache
cpu0: smt 0, core 0, package 0
mtrr: Pentium Pro MTRR support, 8 var ranges, 88 fixed ranges
cpu0: apic clock running at 199MHz
cpu0: mwait min=64, max=64, C-substates=0.1, IBE
cpu1 at mainbus0: apid 2 (application processor)
cpu1: Intel(R) Atom(TM) CPU D525 @ 1.80GHz, 1800.00 MHz
cpu1: 
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,DTES64,MWAIT,DS-CPL,TM2,SSSE3,CX16,xTPR,PDCM,MOVBE,NXE,LONG,LAHF,PERF,SENSOR
cpu1: 512KB 64b/line 8-way L2 cache
cpu1: smt 0, core 1, package 0
cpu2 at mainbus0: apid 1 (application processor)
cpu2: Intel(R) Atom(TM) CPU D525 @ 1.80GHz, 1800.00 MHz
cpu2: 
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,DTES64,MWAIT,DS-CPL,TM2,SSSE3,CX16,xTPR,PDCM,MOVBE,NXE,LONG,LAHF,PERF,SENSOR
cpu2: 512KB 64b/line 8-way L2 cache
cpu2: smt 1, core 0, package 0
cpu3 at mainbus0: apid 3 (application processor)
cpu3: Intel(R) Atom(TM) CPU D525 @ 1.80GHz, 1800.00 MHz
cpu3: 
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,DTES64,MWAIT,DS-CPL,TM2,SSSE3,CX16,xTPR,PDCM,MOVBE,NXE,LONG,LAHF,PERF,SENSOR
cpu3: 512KB 64b/line 8-way L2 cache
cpu3: smt 1, core 1, package 0
ioapic0 at mainbus0: apid 4 pa 0xfec0, version 20, 24 pins
acpimcfg0 at acpi0 addr 0xe000, bus 0-255
acpihpet0 at acpi0: 14318179 Hz
acpiprt0 at acpi0: bus 0 (PCI0)
acpiprt1 at acpi0: bus 4 (P0P1)
acpiprt2 at acpi0: bus 1 (P0P4)
acpiprt3 at acpi0: bus -1 (P0P5)
acpiprt4 at acpi0: bus -1 (P0P6)
acpiprt5 at acpi0: bus -1 (P0P7)
acpiprt6 at acpi0: bus 2 (P0P8)
acpiprt7 at acpi0: bus 3 (P0P9)
acpicpu0 at acpi0: C1(@1 halt!)
acpicpu1 at acpi0: C1(@1 halt!)
acpicpu2 at acpi0: C1(@1 halt!)
acpicpu3 at acpi0: C1(@1 halt!)
"PNP0501" at acpi0 not configured
"PNP0501" at acpi0 not configured
acpibtn0 at acpi0: PWRB
ipmi at mainbus0 not configured
pci0 at mainbus0 bus 0
pchb0 at pci0 dev 0 function 0 "Intel Pineview DMI" rev 0x02
uhci0 at pci0 dev 26 function 0 "Intel 82801I USB" rev 0x02: apic 4 int 16
uhci1 at pci0 dev 26 function 1 "Intel 82801I USB" rev 0x02: apic 4 int 21
uhci2 at pci0 dev 26 function 2 "Inte

step missing from "2016/10/14" followig -current?

[patrick keshishian]

Instructions say:

$ cd /sys
$ rm -r arch/*/compile/[GR]*
$ rm arch/*/compile/.cvsignore
$ cvs up

The new way of configuring, building and installing a kernel is:

$ cd /sys/arch/$(machine)/compile/GENERIC.MP
$ doas make obj
$ make config
$ make
$ doas make install

However, the GENERIC.MP directory was just removed (above) and cvs up
didn't create it.

Is the step to create the GENERIC.MP missing?

TIA,
--patrick

Re: xdm halt & reboot buttons

[patrick keshishian]

On 7/1/16, Matthieu Herrb  wrote:
> On Fri, Jul 01, 2016 at 02:22:57PM +0100, Craig Skinner wrote:
>> Hi Alexander,
>>
>> On 2016-06-30 Thu 20:58 PM |, Alexander Hall wrote:
>> >
>> > 1. xmessage has an sometimes useful, but in general awful, interface.
>>
>> Aye,... simple, in base & functional.
>>
>> The prettiness factor can be cosmetically altered with Xmessage* lines
>> in /etc/X11/xdm/Xresources. See '! xmessage resources' mid way down:
>> http://cafim.sssup.it/~giulio/other/Customization_XDM.html
>>
>> >
>> > Maybe just add a button or two to the xdm login panel instead?
>> >
>>
>> Good idea. I'll try to alter that instead Suggestions welcome!
>
> I'm not sure it's a good idea. The Login widget in xdm is ugly old
> code. You will loose hair or your mind trying to hack there, unless
> you decide to first clean it up completely.
>
> If you spend some time providing a set of nice resouces for xmessages
> and keep it minimalistic, I think we can include your suggestion. I've
> been waiting for other comments.


Only comment I (as a user) have, has to do with liking the
default look as-is. I like the idea of this being put in a
separate script/program (suggested by Alexander Hall); that
way, it can easily be disabled (reverted to original look)
with a one-line comment.

cheers,
--patrick

Re: splraise for sparc

[patrick keshishian]

On Mon, Jun 13, 2016 at 11:29:13AM +1000, David Gwynne wrote:
> this is kind of like the change i just made to sparc64.
> 
> sparc created instances of inline functions for each of the splfoo
> calls. this provides an splraise call and turns splfoo into a macro
> to splraise(IPL_FOO).
> 
> the spl code is quite long so i turned it into functions instead
> of inlines.
> 
> could someone test this? i haven't got a running (walking?) sparc
> anymore.

OK getting further with this diff[1]. Going to let it run
overnight.

but one more nit below:

> Index: sparc/intr.c
> ===
> RCS file: /cvs/src/sys/arch/sparc/sparc/intr.c,v
> retrieving revision 1.43
> diff -u -p -r1.43 intr.c
> --- sparc/intr.c  10 Dec 2015 19:48:04 -  1.43
> +++ sparc/intr.c  13 Jun 2016 01:27:01 -
> @@ -544,3 +544,70 @@ splassert_check(int wantipl, const char 
>   }
>  }
>  #endif
> +
> +int
> +spl0(void)
> +{
> + int psr, oldipl;
> +
> + /*
> +  * wrpsr xors two values: we choose old psr and old ipl here,
> +  * which gives us the same value as the old psr but with all
> +  * the old PIL bits turned off.
> +  */
> + __asm volatile("rd %%psr,%0" : "=r" (psr));
> + oldipl = psr & PSR_PIL;
> + __asm volatile("wr %0,%1,%%psr" : : "r" (psr), "r" (oldipl));
> +
> + /*
> +  * Three instructions must execute before we can depend
> +  * on the bits to be changed.
> +  */
> + __asm volatile("nop; nop; nop");
> + return (oldipl);
> +}
> +
> +int
> +splraise(int newipl)
> +{
> + int psr, oldipl;
> +
> + newipl <<= 8;
> +
> + __asm volatile("rd %%psr,%0" : "=r" (psr));
> + oldipl = psr & PSR_PIL;
> + if (newipl <= oldipl)
> + return oldipl;
> +
> + psr &= ~oldipl;
> + __asm volatile("wr %0,%1,%%psr" : : "r" (psr), "n" (newipl));
> + __asm volatile("nop; nop; nop");
> + __asm volatile("":::"memory");  /* protect from reordering */ \

leftover back-slash in above line :-)


> +
> + return (oldipl);
> +}
> +
> +int
> +splhigh(void)
> +{
> + int psr, oldipl;
> +
> + __asm volatile("rd %%psr,%0" : "=r" (psr));
> + __asm volatile("wr %0,0,%%psr" : : "r" (psr | PSR_PIL));
> + __asm volatile("and %1,%2,%0; nop; nop" : "=r" (oldipl) :
> + "r" (psr), "n" (PSR_PIL));
> + __asm volatile("":::"memory");  /* protect from reordering */
> + return (oldipl);
> +}
> +
> +void
> +splx(int newipl)
> +{
> + int psr;
> +
> + __asm volatile("":::"memory");  /* protect from reordering */
> + __asm volatile("rd %%psr,%0" : "=r" (psr));
> + __asm volatile("wr %0,%1,%%psr" : :
> + "r" (psr & ~PSR_PIL), "rn" (newipl));
> + __asm volatile("nop; nop; nop");
> +}
> 


[1] My grep may be defective but I couldn't find anything
for sparc using:

-SPLHOLD(splausoft, IPL_AUSOFT)
-SPLHOLD(splfdsoft, IPL_FDSOFT)
-SPLHOLD(splaudio, IPL_AUHARD)

so not included.


Index: include/psl.h
===
RCS file: /cvs/obsd/src/sys/arch/sparc/include/psl.h,v
retrieving revision 1.28
diff -u -p -u -p -r1.28 psl.h
--- include/psl.h   29 Mar 2014 18:09:30 -  1.28
+++ include/psl.h   14 Jun 2016 07:51:32 -
@@ -106,9 +106,6 @@
 
 static __inline int getpsr(void);
 static __inline void setpsr(int);
-static __inline int spl0(void);
-static __inline int splhigh(void);
-static __inline void splx(int);
 static __inline int getmid(void);
 
 /*
@@ -142,28 +139,6 @@ setpsr(newpsr)
__asm volatile("nop");
 }
 
-static __inline int
-spl0()
-{
-   int psr, oldipl;
-
-   /*
-* wrpsr xors two values: we choose old psr and old ipl here,
-* which gives us the same value as the old psr but with all
-* the old PIL bits turned off.
-*/
-   __asm volatile("rd %%psr,%0" : "=r" (psr));
-   oldipl = psr & PSR_PIL;
-   __asm volatile("wr %0,%1,%%psr" : : "r" (psr), "r" (oldipl));
-
-   /*
-* Three instructions must execute before we can depend
-* on the bits to be changed.
-*/
-   __asm volatile("nop; nop; nop");
-   return (oldipl);
-}
-
 #ifdef DIAGNOSTIC
 /*
  * Although this function is implemented in MI code, it must be in this MD
@@ -183,83 +158,24 @@ void splassert_check(int, const char *);
 #define splsoftassert(wantipl) do { /* nada */ } while (0)
 #endif
 
-/*
- * PIL 1 through 14 can use this macro.
- * (spl0 and splhigh are special since they put all 0s or all 1s
- * into the ipl field.)
- */
-#defineSPL(name, newipl) \
-static __inline int name(void); \
-static __inline int name() \
-{ \
-   int psr, oldipl; \
-   __asm volatile("rd %%psr,%0" : "=r" (psr)); \
-   oldipl = psr & PSR_PIL; \
-   psr &= ~oldipl; \
-   __asm volatile("wr %0,%1,%%psr" : : \
-   "r" (psr), "n" ((newipl) << 8)); \
-   __asm volatile("nop; nop; nop"); \
-   __asm volatile(""

Re: utvfu driver port

[patrick keshishian]

On Wed, Jun 01, 2016 at 12:04:38PM +0200, Stefan Sperling wrote:
> On Wed, Jun 01, 2016 at 01:40:14AM -0700, patrick keshishian wrote:
> > Here it is...
> 
> Thanks patrick!
> 
> As far as I can see we don't have a man page for this driver yet.
> Could you provide one?

Hi Stefan,

I expected the need for one. I'll look at some examples in
the tree and come up with something.

One thing I forgot to mention last night (it was late and I
also didn't expect the import to happen so quickly), the
copyright text from the original Linux driver was updated
per OpenBSD's request by Lubomir Rintel and also approved
by Federico Simoncelli (the copyright owners/authors).

Just wanted to thank them again for their work and making
it a BSD friendly driver source, and of course their prompt
reply to requested text changes.

Cheers,
--patrick

Re: utvfu driver port

[patrick keshishian]

On Wed, Jun 01, 2016 at 12:24:43AM -0700, patrick keshishian wrote:
> On Wed, Jun 01, 2016 at 08:58:21AM +0200, Marcus Glocker wrote:
> > On Wed, Jun 01, 2016 at 12:51:22AM -0400, Ted Unangst wrote:
> > 
> > > Marcus Glocker wrote:
> > > > Me too.  Would it be ok to merge utvfu.c and utvfu_ops.c by including
> > > > both Copyrights in this file?  Should it be
> > > > 
> > > > [Copyright 1]
> > > > [Code 1]
> > > > [Copyright 2]
> > > > [Code 2]
> > > > 
> > > > or
> > > > 
> > > > [Copyright 1]
> > > > [Copyright 2]
> > > > [Code 1]
> > > > [Code 2]
> > > > 
> > > 
> > > Historically, the second has been popular, but that's also when the new 
> > > code is
> > > mixed in with old.
> > > 
> > > If it's entirely new code, I think the top option is better because it 
> > > allows
> > > separation later. We have some files where all the [code 1] gets deleted, 
> > > but
> > > the copyright remains, somewhat dubious.
> > 
> > Thanks for the feedback Ted.  I guess in this case we should go with option
> > 1 then.
> > 
> > Patrick, can you please merge utvfu_ops.c into utvfu.c in your new diff
> > this way?
> 
> OK. Will send it out in a few.

Here it is...

Cheers,
--patrick

Index: arch/amd64/conf/GENERIC
===
RCS file: /cvs/obsd/src/sys/arch/amd64/conf/GENERIC,v
retrieving revision 1.418
diff -u -p -u -p -r1.418 GENERIC
--- arch/amd64/conf/GENERIC 7 May 2016 23:10:50 -   1.418
+++ arch/amd64/conf/GENERIC 1 Jun 2016 08:33:40 -
@@ -290,6 +290,9 @@ uoakv*  at uhidev?  # Toradex OAK 10V sen
 onewire* at uow?
 uvideo*at uhub?# USB Video
 video* at uvideo?
+utvfu* at uhub?# Fushicai Audio-Video Grabber
+video* at utvfu?
+audio* at utvfu?
 udl*   at uhub?# DisplayLink USB displays
 wsdisplay* at udl?
 
Index: arch/macppc/conf/GENERIC
===
RCS file: /cvs/obsd/src/sys/arch/macppc/conf/GENERIC,v
retrieving revision 1.256
diff -u -p -u -p -r1.256 GENERIC
--- arch/macppc/conf/GENERIC24 Apr 2016 17:30:31 -  1.256
+++ arch/macppc/conf/GENERIC1 Jun 2016 08:33:40 -
@@ -302,6 +302,10 @@ onewire* at uow?
 uvideo* at uhub?
 video*  at uvideo?
 
+utvfu* at uhub?# Fushicai Audio-Video Grabber
+video* at utvfu?
+audio* at utvfu?
+
 udl*   at uhub?
 wsdisplay* at udl?
 
Index: dev/audio.c
===
RCS file: /cvs/obsd/src/sys/dev/audio.c,v
retrieving revision 1.147
diff -u -p -u -p -r1.147 audio.c
--- dev/audio.c 11 May 2016 16:16:58 -  1.147
+++ dev/audio.c 1 Jun 2016 08:33:40 -
@@ -1344,6 +1344,14 @@ audio_detach(struct device *self, int fl
return 0;
 }
 
+int
+audio_submatch(struct device *parent, void *match, void *aux)
+{
+struct cfdata *cf = match;
+
+   return (cf->cf_driver == &audio_cd);
+}
+
 struct device *
 audio_attach_mi(struct audio_hw_if *ops, void *arg, struct device *dev)
 {
@@ -1357,7 +1365,7 @@ audio_attach_mi(struct audio_hw_if *ops,
 * attach this driver to the caller (hardware driver), this
 * checks the kernel config and possibly calls audio_attach()
 */
-   return config_found(dev, &aa, audioprint);
+   return config_found_sm(dev, &aa, audioprint, audio_submatch);
 }
 
 int
Index: dev/video.c
===
RCS file: /cvs/obsd/src/sys/dev/video.c,v
retrieving revision 1.38
diff -u -p -u -p -r1.38 video.c
--- dev/video.c 8 Feb 2016 17:21:10 -   1.38
+++ dev/video.c 1 Jun 2016 08:33:40 -
@@ -392,6 +392,14 @@ videommap(dev_t dev, off_t off, int prot
return (pa);
 }
 
+int
+video_submatch(struct device *parent, void *match, void *aux)
+{
+struct cfdata *cf = match;
+
+   return (cf->cf_driver == &video_cd);
+}
+
 /*
  * Called from hardware driver. This is where the MI video driver gets
  * probed/attached to the hardware driver
@@ -403,7 +411,7 @@ video_attach_mi(struct video_hw_if *rhwp
 
arg.hwif = rhwp;
arg.hdl = hdlp;
-   return (config_found(dev, &arg, videoprint));
+   return (config_found_sm(dev, &arg, videoprint, video_submatch));
 }
 
 void
Index: dev/usb/usbdevs
===
RCS file: /cvs/obsd/src/sys/dev/usb/usbdevs,v
retrieving revision 1.664
diff -u -p -u -p -r1.664 usbdevs
--- dev/usb/usbdevs 20 May 2016 02:04:24 -

Re: utvfu driver port

[patrick keshishian]

On Wed, Jun 01, 2016 at 08:58:21AM +0200, Marcus Glocker wrote:
> On Wed, Jun 01, 2016 at 12:51:22AM -0400, Ted Unangst wrote:
> 
> > Marcus Glocker wrote:
> > > Me too.  Would it be ok to merge utvfu.c and utvfu_ops.c by including
> > > both Copyrights in this file?  Should it be
> > > 
> > >   [Copyright 1]
> > >   [Code 1]
> > >   [Copyright 2]
> > >   [Code 2]
> > > 
> > > or
> > > 
> > >   [Copyright 1]
> > >   [Copyright 2]
> > >   [Code 1]
> > >   [Code 2]
> > > 
> > 
> > Historically, the second has been popular, but that's also when the new 
> > code is
> > mixed in with old.
> > 
> > If it's entirely new code, I think the top option is better because it 
> > allows
> > separation later. We have some files where all the [code 1] gets deleted, 
> > but
> > the copyright remains, somewhat dubious.
> 
> Thanks for the feedback Ted.  I guess in this case we should go with option
> 1 then.
> 
> Patrick, can you please merge utvfu_ops.c into utvfu.c in your new diff
> this way?

OK. Will send it out in a few.

--patrick

Re: utvfu driver port

[patrick keshishian]

Hello,

On Mon, May 30, 2016 at 04:22:56PM +0200, Martin Pieuchot wrote:
> On 26/05/16(Thu) 16:09, patrick keshishian wrote:
> > 
> > Included is my initial effort to port the dual-licensed driver
> > for Fushicai Audio-Video Grabber (vendor 0x1b71 product 0x3002).
> 
> Nice.  Did you test both bulk and iso? Some comments inline.

As far as I can tell, this device's video streaming endpoint
is isochronous.

I think I have addressed most of the points you highlighted.
I'll reply inline to each. New diff is inline at the end. I
had to add a rwlock as I hit a race condition testing, where
the bulk thread was accessing variables, as they are being
cleared when the audio client was closing the device.


> > Index: dev/usb/files.usb
> > ===
> > RCS file: /cvs/obsd/src/sys/dev/usb/files.usb,v
> > retrieving revision 1.126
> > diff -u -p -u -p -r1.126 files.usb
> > --- dev/usb/files.usb   8 Jan 2016 15:54:13 -   1.126
> > +++ dev/usb/files.usb   26 May 2016 22:34:38 -
> > @@ -35,6 +35,12 @@ device   uvideo: video, firmload
> >  attach uvideo at uhub
> >  file   dev/usb/uvideo.cuvideo
> >  
> > +# USBTV007 devices
> > +device utvfu: video, audio
> > +attach utvfu at uhub
> > +file   dev/usb/utvfu.c utvfu
> > +file   dev/usb/utvfu_ops.c utvfu
> 
> I'd appreciate if we can keep the driver in one file, when the license
> issue is solved, this is coherent with the rest of our tree.

*cringe* ... I'll let the more experienced decide how this is
handled. I don't have strong feelings on this subject, whichever
way it is decided. Just that, one of the licenses is slightly
more restrictive.

> > +#ifndef _UTVFU_H_
> > +#define _UTVFU_H_
> 
> Is there any hardware reference for this device or everything is reverse
> engineered?  I mean the values below are specified somewhere?

None which I am aware of. The original driver mentions reverse
engineering it.

> > +#defineUTVFU_COMPOSITE_INPUT   0
> > +#defineUTVFU_SVIDEO_INPUT  1
> ^^
> We try to avoid mixing space and tab after "#define"

I fixed this and two others. Hopefully that was all there was.

> > +/* parameters for supported TV norms */
> > +struct utvfu_norm_params {
> > +   v4l2_std_id norm;
> > +   int cap_width,
> > +   cap_height,
> > +   frame_len;
> 
> Please one type per line.

Fixed.

> > +struct utvfu_isoc_xfer {
> > +   struct utvfu_softc  *sc;
> > +   struct usbd_xfer*xfer;
> > +   void*buf;
> > +   uint16_tsize[UTVFU_NFRAMES_MAX];
> > +};
> > +
> > +struct utvfu_bulk_xfer {
> > +   struct usbd_xfer*xfer;
> > +   void*buf;
> 
> I'd use the simpler approach of using KERNADDR(&xfer->dmabuf, 0) instead
> of defining another structure to abstract an USB xfer.  You can look at
> uhidev_get_report_async_cb() for an example.

I had to scratch my head a few times then look in the usb
sources to see exactly what was going on. So yes. I agree
and have made the change.

> > +#define UTVFU_DEBUG
> > +#ifdef UTVFU_DEBUG
> > +extern int utvfu_debug;
> > +#define DPRINTF(l, x...) do { if ((l) <= utvfu_debug) printf(x); } while 
> > (0)
> 
> You mostly have 1 level, so do you really want a `D'PRINTF()?  By the
> way, we try to remove debug printf before committing code.  So if you
> think some of the printfs you added won't help later, just get rid of
> them.

This came over from uvideo.c. I have gotten rid of a few in
this revision. As I shuffle around code, if I break things,
they sometimes are helpful. They can be trimmed more later.

> > +   for (i = 0; i < size; i++) {
> > +   USETW(req.wIndex, regs[i][0]);
> > +   USETW(req.wValue, regs[i][1]);
> > +
> > +   error = usbd_do_request(sc->sc_udev, &req, NULL);
> > +   if (USBD_NORMAL_COMPLETION != error) {
> 
> This way of checking for error value might be clever, but I IMHO being
> coherent with the rest of the code base is more important.  So I would
> stick to the "if (variable == constant)" style.

OK.

> > +   DPRINTF(1, "%s: %s: calling usbd_set_interface(ifaceh, 0)\n",
> > +   DEVNAME(sc), __func__);
> > +   /* default video stream interface */
> > +   error = usbd_set_interface(sc->sc_uifaceh, 0);
> 
> Could you add a define for `0' and `1` below.  This would aut

Re: utvfu driver port

[patrick keshishian]

On Fri, May 27, 2016 at 08:41:33PM +0200, Marcus Glocker wrote:
> Hello Patrick,
> 
> On Thu, May 26, 2016 at 04:09:26PM -0700, patrick keshishian wrote:
> 
> > Hi,
> > 
> > Included is my initial effort to port the dual-licensed driver
> > for Fushicai Audio-Video Grabber (vendor 0x1b71 product 0x3002).
> > 
> > As I mentioned in previous email it is "mostly working". I have
> > tested it on amd64 and macppc. The macppc audio issue mentioned
> > got resolved thanks to a hint from Alexandre Ratchov; I needed
> > some education on how to use sndiod :-) Thanks Alex.
> > 
> > I realize not many of these devices are out in the hands of
> > people who could test this code, but there are some obvious
> > bits (especially in the audio ops) that I am "winging", partly
> > because, the audio interface required them.
> > 
> > There is .h and two .c files; as mentioned, I kept the original
> > driver source bits and what I borrowed from uvideo.c separate.
> > If they must be combine into one .c file, I leave that part to
> > Marcus Glocker and company.
> > 
> > My main testing tool has been video(1). I had to modify it for
> > two reasons: 1. To test the mmap() interface and 2. because
> > to determine format description video(1) does a strcmp() rather
> > than use the pixelformat member of the v4l2_fmtdesc struct.
> > Patch at the very end. Maybe upstream might be interested in
> > the mmap change?
> > 
> > Cheers,
> > --patrick
> 
> Ok, lets work together to get this driver imported.
> 
> First thing to sort out are the Copyrights:
> 
> - Please change the uvideo Copyright to your own and add a sentence like
>   "Based on the uvideo driver".  You can find some good examples e.g.
>   here: cd /usr/src/sys/dev/usb ; grep -i based *.c
> 
> - Can you please get in touch with the original authors of the other
>   code and ask them if they could fix their Copyright to make it valid?
>   Theo did also point out about a valid Copyright format:
>   1. Copyright ...
>   2. [rights granted]
>   3. [legacy disclaimer, not really required]
>   4. [extra text]
>   5. [source]
>   Important is that the Copyright must be followed by the rights.

Thanks. I'll get on this.

--patrick

> In the meantime I give the driver spin.  Maybe I can find such a device
> somewhere at one point.
> 

Re: uvideo patches: Overview [0/4]

[patrick keshishian]

On Thu, May 26, 2016 at 10:55:47PM +0200, Marcus Glocker wrote:
> On Tue, May 17, 2016 at 05:15:51PM -0700, patrick keshishian wrote:
> 
> > Greetings,
> > 
> > I have been looking at uvideo trying to model a new driver I'm
> > attempting to port over and found a few issues (or what I precive
> > as issues).
> > 
> > Since the list likes separate diffs for easier discussion, Here
> > is my attempt to break them up in four emails. I think, with
> > exception of one, all should apply and compile individually.
> > 
> > Here are description of patches in decreasing order of my
> > confidence in proposing them:
> > 
> > 1/4: Incorrect enum used for v4l2_buf.flags.
> >  This is a paste error I believe. Very simple diff
> > 
> > 2/4: Assumption on endpoint index to use in uvideo_vs_open() vs
> >  actual saved endpoint address.
> 
> Makes sense.  I also would change the following DPRINTF text to
> remove 'sc->sc_vs_cur->endpoint' since this will be same as
> 'ed->bEndpointAddress' and also remove 'sc->sc_vs_cur->psize'
> since this information is already printed in uvideo_vs_set_alt().

As long as it is understood that the actually packet size is
not the literal value of wMaxPacketSize, rather:

psize = UGETW(ed->wMaxPacketSize);
psize = UE_GET_SIZE(psize) * (1 + UE_GET_TRANS(psize));

Fancy stuff this USB.

--patrick

> I think this check was in from the very beginning before we had
> implemented a proper set alternate interface function.
> 
> 
> Index: uvideo.c
> ===
> RCS file: /cvs/src/sys/dev/usb/uvideo.c,v
> retrieving revision 1.187
> diff -u -p -u -p -r1.187 uvideo.c
> --- uvideo.c  26 May 2016 04:47:08 -  1.187
> +++ uvideo.c  26 May 2016 20:38:16 -
> @@ -1817,18 +1817,17 @@ uvideo_vs_open(struct uvideo_softc *sc)
>   return (error);
>   }
>  
> - ed = usbd_interface2endpoint_descriptor(sc->sc_vs_cur->ifaceh, 0);
> + ed = usbd_get_endpoint_descriptor(sc->sc_vs_cur->ifaceh,
> + sc->sc_vs_cur->endpoint);
>   if (ed == NULL) {
>   printf("%s: no endpoint descriptor for VS iface\n",
>   DEVNAME(sc));
>   return (USBD_INVAL);
>   }
>   DPRINTF(1, "%s: open pipe for ", DEVNAME(sc));
> - DPRINTF(1, "bEndpointAddress=0x%02x (0x%02x), wMaxPacketSize=%d (%d)\n",
> + DPRINTF(1, "bEndpointAddress=0x%02x, wMaxPacketSize=%d\n",
>   ed->bEndpointAddress,
> - sc->sc_vs_cur->endpoint,
> - UGETW(ed->wMaxPacketSize),
> - sc->sc_vs_cur->psize);
> + UGETW(ed->wMaxPacketSize));
>  
>   error = usbd_open_pipe(
>   sc->sc_vs_cur->ifaceh,
> 

utvfu driver port

[patrick keshishian]

Hi,

Included is my initial effort to port the dual-licensed driver
for Fushicai Audio-Video Grabber (vendor 0x1b71 product 0x3002).

As I mentioned in previous email it is "mostly working". I have
tested it on amd64 and macppc. The macppc audio issue mentioned
got resolved thanks to a hint from Alexandre Ratchov; I needed
some education on how to use sndiod :-) Thanks Alex.

I realize not many of these devices are out in the hands of
people who could test this code, but there are some obvious
bits (especially in the audio ops) that I am "winging", partly
because, the audio interface required them.

There is .h and two .c files; as mentioned, I kept the original
driver source bits and what I borrowed from uvideo.c separate.
If they must be combine into one .c file, I leave that part to
Marcus Glocker and company.

My main testing tool has been video(1). I had to modify it for
two reasons: 1. To test the mmap() interface and 2. because
to determine format description video(1) does a strcmp() rather
than use the pixelformat member of the v4l2_fmtdesc struct.
Patch at the very end. Maybe upstream might be interested in
the mmap change?

Cheers,
--patrick



Index: arch/amd64/conf/GENERIC
===
RCS file: /cvs/obsd/src/sys/arch/amd64/conf/GENERIC,v
retrieving revision 1.415
diff -u -p -u -p -r1.415 GENERIC
--- arch/amd64/conf/GENERIC 28 Mar 2016 17:54:37 -  1.415
+++ arch/amd64/conf/GENERIC 26 May 2016 22:34:38 -
@@ -289,6 +289,9 @@ uoakv*  at uhidev?  # Toradex OAK 10V sen
 onewire* at uow?
 uvideo*at uhub?# USB Video
 video* at uvideo?
+utvfu* at uhub?# Fushicai Audio-Video Grabber
+video* at utvfu?
+audio* at utvfu?
 udl*   at uhub?# DisplayLink USB displays
 wsdisplay* at udl?
 
Index: dev/video.c
===
RCS file: /cvs/obsd/src/sys/dev/video.c,v
retrieving revision 1.38
diff -u -p -u -p -r1.38 video.c
--- dev/video.c 8 Feb 2016 17:21:10 -   1.38
+++ dev/video.c 26 May 2016 22:34:38 -
@@ -392,6 +392,14 @@ videommap(dev_t dev, off_t off, int prot
return (pa);
 }
 
+int
+video_submatch(struct device *parent, void *match, void *aux)
+{
+struct cfdata *cf = match;
+
+   return (cf->cf_driver == &video_cd);
+}
+
 /*
  * Called from hardware driver. This is where the MI video driver gets
  * probed/attached to the hardware driver
@@ -403,7 +411,7 @@ video_attach_mi(struct video_hw_if *rhwp
 
arg.hwif = rhwp;
arg.hdl = hdlp;
-   return (config_found(dev, &arg, videoprint));
+   return (config_found_sm(dev, &arg, videoprint, video_submatch));
 }
 
 void
Index: dev/audio.c
===
RCS file: /cvs/obsd/src/sys/dev/audio.c,v
retrieving revision 1.145
diff -u -p -u -p -r1.145 audio.c
--- dev/audio.c 16 Mar 2016 06:46:39 -  1.145
+++ dev/audio.c 26 May 2016 22:34:38 -
@@ -1335,6 +1335,14 @@ audio_detach(struct device *self, int fl
return 0;
 }
 
+int
+audio_submatch(struct device *parent, void *match, void *aux)
+{
+struct cfdata *cf = match;
+
+   return (cf->cf_driver == &audio_cd);
+}
+
 struct device *
 audio_attach_mi(struct audio_hw_if *ops, void *arg, struct device *dev)
 {
@@ -1348,7 +1356,7 @@ audio_attach_mi(struct audio_hw_if *ops,
 * attach this driver to the caller (hardware driver), this
 * checks the kernel config and possibly calls audio_attach()
 */
-   return config_found(dev, &aa, audioprint);
+   return config_found_sm(dev, &aa, audioprint, audio_submatch);
 }
 
 int
Index: dev/usb/usbdevs
===
RCS file: /cvs/obsd/src/sys/dev/usb/usbdevs,v
retrieving revision 1.663
diff -u -p -u -p -r1.663 usbdevs
--- dev/usb/usbdevs 31 Mar 2016 12:27:48 -  1.663
+++ dev/usb/usbdevs 26 May 2016 22:34:38 -
@@ -591,6 +591,7 @@ vendor SILABS5  0x1ba4  Silicon Labs
 vendor CORSAIR 0x1b1c  Corsair
 vendor MATRIXORB   0x1b3d  Matrix Orbital
 vendor TORADEX 0x1b67  Toradex inc.
+vendor FUSHICAI0x1b71  Fushicai
 vendor OVISLINK0x1b75  OvisLink
 vendor TML 0x1b91  The Mobility Lab
 vendor TCTMOBILE   0x1bbb  TCT Mobile
@@ -1995,6 +1996,9 @@ product FUJITSUCOMP VIRTETH   0xa4a2  Virtu
 
 /* Fujitsu Siemens Computers products */
 product FSC E5400  0x1009  PrismGT USB 2.0 WLAN
+
+/* Fushicai */
+product FUSHICAI USBTV007  0x3002  Fushicai Audio-Video Grabber
 
 /* G.Mate */
 productGMATE YP3X000x1001  YP3X00 PDA
Index: dev/usb/files.usb
===
RCS file: /cvs/obsd/src/sys/dev/usb/files.usb,v
retrieving revision 1.126
diff -u -p -u -p -r1.126 files.usb
--- dev/usb/files.usb   8 Jan 201

Re: usbtv driver port (questions)

[patrick keshishian]

On Wed, May 25, 2016 at 01:57:36PM +0200, Marcus Glocker wrote:
> On Tue, May 24, 2016 at 04:46:57PM -0700, patrick keshishian wrote:
> 
> > Hi,
> > 
> > I have a "mostly working" driver port for Fushicai Audio-Video
> > Grabber (vendor 0x1b71 product 0x3002).
> > 
> > I've had the video bit working for a few days on amd64 and macppc.
> > I finally managed to get the audio bit working this morning on
> > amd64, but still an encoding issue (LE vs BE) on macppc.
> > 
> > I'd like to "polish up" the code for a post here, or off-list if
> > that is preferred, for review and ridicule. So I have some
> > questions which I don't think style(9) answers.
> > 
> > 1. Types: Original source used a few instances of "u16" types,
> >but I think OpenBSD prefers "u_int16_t" and "uint16_t".
> >Which to pick?
> 
> I vote for uint*_t.
>  
> > 2. Return values inside parenthesis or not?
> 
> The ongoing religious question :-)  Some people say return and sizeof
> shouldn't be used with parenthesis because they are no functions.  I
> still find it more readable with parenthesis.  We have both variations
> in the tree.  Your choice.
> 
> > 3. Original source filenames were hyphenated (e.g., usbtv-video.c).
> >OK to use underscore instead? I think it is preferred.
> >(Next question may trump this one)
> 
> How's about something a bit shorter, like uvcap.c (USB Video Capture)
> and then also call the driver uvcap?  Anyway just a proposal.  Otherwise
> yes, replace the '-' by '_' in the filename.
> 
> > 4. Original source has three .c and one .h file. They are dual-
> >licensed Linux sources: BSD and GPL.
> > 
> >I've borrowed from uvideo.c to get this driver working,
> >introducing a fourth .c.
> > 
> >I'm pretty sure OBSD doesn't want so many files introduced.
> >Should I combine the original .c files into one .c file, keep
> >the .h and the new .c file I introduced? Maintaining copyright
> >notices as-is (the original and one from uvideo.c).
> 
> For such a kind of driver I also would perfer one .c and one .h file.

Yes. Only concern I have is the slight variations in license
text. The original code is a two-clause BSD[0] while the uvideo.c
is the far simpler one.

> > 5. If tech@ is not the place to post an initial port effort,
> >especially from a first-timer, do let me know.
> 
> Well, I think tech@ is the right place.  Maybe we can take some stuff
> off-list.  Though myself has no such device yet to test further.

It is a low-end item, so I'm not sure if you really want one, but
would be glad to send one your way; just let me know where to ship
it to.

Also thanks for the suggestions!

--patrick

[0] https://github.com/torvalds/linux/tree/master/drivers/media/usb/usbtv

Re: usbtv driver port (questions)

[patrick keshishian]

On Wed, May 25, 2016 at 08:45:29AM -0400, Ian Darwin wrote:
> On 2016-05-25 7:57 AM, Marcus Glocker wrote:
> >On Tue, May 24, 2016 at 04:46:57PM -0700, patrick keshishian wrote:
> >
> >>Hi,
> >>
> >>I have a "mostly working" driver port for Fushicai Audio-Video
> >>Grabber (vendor 0x1b71 product 0x3002).
> >>
> >>I've had the video bit working for a few days on amd64 and macppc.
> >>I finally managed to get the audio bit working this morning on
> >>amd64, but still an encoding issue (LE vs BE) on macppc.
> >>
> >>...
> >>2. Return values inside parenthesis or not?
> >The ongoing religious question :-)  Some people say return and sizeof
> >shouldn't be used with parenthesis because they are not functions.  I
> >still find it more readable with parenthesis.  We have both variations
> >in the tree.  Your choice.
> "Obviously without" :-)
> >
> >>3. Original source filenames were hyphenated (e.g., usbtv-video.c).
> >>OK to use underscore instead? I think it is preferred.
> >>(Next question may trump this one)
> >How's about something a bit shorter, like uvcap.c (USB Video Capture)
> >and then also call the driver uvcap?  Anyway just a proposal.  Otherwise
> >yes, replace the '-' by '_' in the filename.
> >
> >
> There are (at least) three other popular usb video capture devices with
> totally different chipsets (Syntek, Empia, Somagic, besides this Fushicai
> one, "usbtv007"), so calling one of them uvcap might be annoying, like if
> one of our many network drivers were called "network". See
> https://linuxtv.org/wiki/index.php/Easycap.
> Does it make sense to start a naming convention like: utvsyn, utvemp,
> utvsom, utvfu, ...?

I like this scheme and utvfu.

Thanks,
--patrick

usbtv driver port (questions)

[patrick keshishian]

Hi,

I have a "mostly working" driver port for Fushicai Audio-Video
Grabber (vendor 0x1b71 product 0x3002).

I've had the video bit working for a few days on amd64 and macppc.
I finally managed to get the audio bit working this morning on
amd64, but still an encoding issue (LE vs BE) on macppc.

I'd like to "polish up" the code for a post here, or off-list if
that is preferred, for review and ridicule. So I have some
questions which I don't think style(9) answers.

1. Types: Original source used a few instances of "u16" types,
   but I think OpenBSD prefers "u_int16_t" and "uint16_t".
   Which to pick?

2. Return values inside parenthesis or not?

3. Original source filenames were hyphenated (e.g., usbtv-video.c).
   OK to use underscore instead? I think it is preferred.
   (Next question may trump this one)

4. Original source has three .c and one .h file. They are dual-
   licensed Linux sources: BSD and GPL.

   I've borrowed from uvideo.c to get this driver working,
   introducing a fourth .c.

   I'm pretty sure OBSD doesn't want so many files introduced.
   Should I combine the original .c files into one .c file, keep
   the .h and the new .c file I introduced? Maintaining copyright
   notices as-is (the original and one from uvideo.c).

5. If tech@ is not the place to post an initial port effort,
   especially from a first-timer, do let me know.

Thanks,
--patrick

uvideo patches: V4L2_BUF_FLAG_{DONE,QUEUED} [4/4]

[patrick keshishian]

4/4: I don't believe V4L2_BUF_FLAG_QUEUED and V4L2_BUF_FLAG_DONE
 flags are handled correctly in our uvideo driver.

According to linuxtv.org Buffers Chap 3. Input/Output Table 3.4
Buffer Flags:

https://www.linuxtv.org/downloads/legacy/video4linux/API/V4L2_API/spec-single/v4l2.html#buffer-flags

V4L2_BUF_FLAG_QUEUED
[The buffer] automatically moves to the outgoing queue
after the buffer has been filled or displayed. Drivers
set or clear this flag when the _QUERYBUF ioctl is
called.
After (successful) calling the _QBUF ioctl it is always
set and after _DQBUF alwyas cleared

V4L2_BUF_FLAG_DONE
When this flag is set, the buffer is currently on the
outgoing queue, ready to be dequeued from the driver.
Drivers set or clear this flag when the _QUERYBUF ioctl
is called.
After calling the _QBUF or _DQBUF it is always cleared.
Of course a buffer cannot be on both queues at the same
time, the _QUEUED and _DONE flag are mutually exclusive.
They can be both cleared however, then the buffer is in
"dequeued" state, in the application domain so to say.

This patch I'm a bit lukewarm on.

It gets rid of sc_mmap_cur used mainly in uvideo_mmap_queue()
looking to find a buffer which can be "queued".

Since the _QUEUE flag is never cleared in this driver, is it
potentially reusing a buffer which the client/consumer may not
yet be done with?

Also, I couldn't convince myself that the while()-loop searching
for this buffer, once the _QUEUE flag clearing is fixed, would
not end up in a situation where it would equal to sc_mmap_count
and never get past returning ENOMEM.

I think (but am not sure) the intent of sc_mmap_cur is to give
each buffer a fair chance at getting used. Where, w/o it, and
a fast client/consumer, it is possible that only the first few
buffers would get all the attention.

Or maybe I'm reading all this incorrectly.

Thoughts?


Index: uvideo.c
===
RCS file: /cvs/obsd/src/sys/dev/usb/uvideo.c,v
retrieving revision 1.185
diff -u -p -u -p -r1.185 uvideo.c
--- uvideo.c17 May 2016 08:27:17 -  1.185
+++ uvideo.c17 May 2016 23:08:32 -
@@ -78,7 +78,6 @@ struct uvideo_softc {
size_t   sc_mmap_buffer_size;
q_mmap   sc_mmap_q;
int  sc_mmap_count;
-   int  sc_mmap_cur;
int  sc_mmap_flag;
 
struct vnode*sc_vp;
@@ -590,7 +589,6 @@ uvideo_attach_hook(struct device *self)
 
/* init mmap queue */
SIMPLEQ_INIT(&sc->sc_mmap_q);
-   sc->sc_mmap_cur = -1;
sc->sc_mmap_count = 0;
 
DPRINTF(1, "uvideo_attach: doing video_attach_mi\n");
@@ -1695,7 +1693,6 @@ uvideo_vs_free_frame(struct uvideo_softc
while (!SIMPLEQ_EMPTY(&sc->sc_mmap_q))
SIMPLEQ_REMOVE_HEAD(&sc->sc_mmap_q, q_frames);
 
-   sc->sc_mmap_cur = -1;
sc->sc_mmap_count = 0;
 }
 
@@ -2208,44 +2205,45 @@ uvideo_vs_decode_stream_header_isight(st
 }
 
 int
+uvideo_find_queued(struct uvideo_softc *sc)
+{
+   int i;
+   for (i = 0; i < sc->sc_mmap_count; ++i) {
+   if (sc->sc_mmap[i].v4l2_buf.flags & V4L2_BUF_FLAG_DONE)
+   continue;
+   if (sc->sc_mmap[i].v4l2_buf.flags & V4L2_BUF_FLAG_QUEUED)
+   break;
+   }
+   return (i == sc->sc_mmap_count) ? -1 : i;
+}
+
+int
 uvideo_mmap_queue(struct uvideo_softc *sc, uint8_t *buf, int len)
 {
-   if (sc->sc_mmap_cur < 0 || sc->sc_mmap_count == 0 ||
-   sc->sc_mmap_buffer == NULL)
+   int i;
+
+   if (sc->sc_mmap_count == 0 || sc->sc_mmap_buffer == NULL)
panic("%s: mmap buffers not allocated", __func__);
 
/* find a buffer which is ready for queueing */
-   while (sc->sc_mmap_cur < sc->sc_mmap_count) {
-   if (sc->sc_mmap[sc->sc_mmap_cur].v4l2_buf.flags &
-   V4L2_BUF_FLAG_QUEUED)
-   break;
-   /* not ready for queueing, try next */
-   sc->sc_mmap_cur++;
-   }
-   if (sc->sc_mmap_cur == sc->sc_mmap_count) {
-   DPRINTF(1, "%s: %s: mmap queue is full!",
-   DEVNAME(sc), __func__);
+   if (-1 == (i = uvideo_find_queued(sc)))
return ENOMEM;
-   }
 
/* copy frame to mmap buffer and report length */
-   bcopy(buf, sc->sc_mmap[sc->sc_mmap_cur].buf, len);
-   sc->sc_mmap[sc->sc_mmap_cur].v4l2_buf.bytesused = len;
+   bcopy(buf, sc->sc_mmap[i].buf, len);
+   sc->sc_mmap[i].v4l2_buf.bytesused = len;
 
/* timestamp it */
-   getmicrotime(&sc->sc_mmap[sc->sc_mmap_cur].v4l2_buf.timestamp);
+   getmicrotime(&sc->sc_mmap[i].v4l2_buf.ti

uvideo patches: endpoint address vs index 0 [2/4]

[patrick keshishian]

2/4: Assumption on endpoint index to use in uvideo_vs_open() vs
 actual saved endpoint address.

Index: uvideo.c
===
RCS file: /cvs/obsd/src/sys/dev/usb/uvideo.c,v
retrieving revision 1.185
diff -u -p -u -p -r1.185 uvideo.c
--- uvideo.c17 May 2016 08:27:17 -  1.185
+++ uvideo.c17 May 2016 22:53:11 -
@@ -1819,7 +1819,8 @@ uvideo_vs_open(struct uvideo_softc *sc)
return (error);
}
 
-   ed = usbd_interface2endpoint_descriptor(sc->sc_vs_cur->ifaceh, 0);
+   ed = usbd_get_endpoint_descriptor(sc->sc_vs_cur->ifaceh,
+   sc->sc_vs_cur->endpoint);
if (ed == NULL) {
printf("%s: no endpoint descriptor for VS iface\n",
DEVNAME(sc));

uvideo patches: Alternate Setting [3/4]

[patrick keshishian]

3/4: In uvideo_vs_set_alt(), according to the comment within
 while()-loop searches for an endpoint with requested
 bandwidth, or best match. An iterator index (int i) is used
 in the while()-loop, and eventually its value is used in
 usbd_set_interface().

 Is the "matched" interface's bAlternateSetting not the
 correct value to be used?

Index: uvideo.c
===
RCS file: /cvs/obsd/src/sys/dev/usb/uvideo.c,v
retrieving revision 1.185
diff -u -p -u -p -r1.185 uvideo.c
--- uvideo.c17 May 2016 08:27:17 -  1.185
+++ uvideo.c17 May 2016 22:55:34 -
@@ -1223,11 +1223,10 @@ uvideo_vs_set_alt(struct uvideo_softc *s
const usb_descriptor_t *desc;
usb_interface_descriptor_t *id;
usb_endpoint_descriptor_t *ed;
-   int i, diff, best_diff = INT_MAX;
+   int diff, best_diff = INT_MAX;
usbd_status error;
uint32_t psize;
 
-   i = 0;
usbd_desc_iter_init(sc->sc_udev, &iter);
desc = usbd_desc_iter_next(&iter);
while (desc) {
@@ -1245,7 +1244,6 @@ uvideo_vs_set_alt(struct uvideo_softc *s
if (desc->bDescriptorType != UDESC_ENDPOINT)
goto next;
ed = (usb_endpoint_descriptor_t *)(uint8_t *)desc;
-   i++;
 
/* save endpoint with requested bandwidth */
psize = UGETW(ed->wMaxPacketSize);
@@ -1271,10 +1269,10 @@ next:
sc->sc_vs_cur->curalt, sc->sc_vs_cur->psize, max_packet_size);
 
/* set alternate video stream interface */
-   error = usbd_set_interface(ifaceh, i);
+   error = usbd_set_interface(ifaceh, sc->sc_vs_cur->curalt);
if (error) {
printf("%s: could not set alternate interface %d!\n",
-   DEVNAME(sc), i);
+   DEVNAME(sc), sc->sc_vs_cur->curalt);
return (USBD_INVAL);
}
 

uvideo patches: Overview [0/4]

[patrick keshishian]

Greetings,

I have been looking at uvideo trying to model a new driver I'm
attempting to port over and found a few issues (or what I precive
as issues).

Since the list likes separate diffs for easier discussion, Here
is my attempt to break them up in four emails. I think, with
exception of one, all should apply and compile individually.

Here are description of patches in decreasing order of my
confidence in proposing them:

1/4: Incorrect enum used for v4l2_buf.flags.
 This is a paste error I believe. Very simple diff

2/4: Assumption on endpoint index to use in uvideo_vs_open() vs
 actual saved endpoint address.

3/4: In uvideo_vs_set_alt(), according to the comment within
 while()-loop searches for an endpoint with requested
 bandwidth, or best match. An iterator index (int i) is used
 in the while()-loop, and eventually its value is used in
 usbd_set_interface().

 Is the "matched" interface's bAlternateSetting not the
 correct value to be used?

4/4: I don't believe V4L2_BUF_FLAG_QUEUED and V4L2_BUF_FLAG_DONE
 flags are handled correctly in our uvideo driver.

uvideo patches: u4l2_buf.flags [1/4]

[patrick keshishian]

1/4: Incorrect enum used for v4l2_buf.flags.
 This is a paste error I believe. Very simple diff

Index: uvideo.c
===
RCS file: /cvs/obsd/src/sys/dev/usb/uvideo.c,v
retrieving revision 1.185
diff -u -p -u -p -r1.185 uvideo.c
--- uvideo.c17 May 2016 08:27:17 -  1.185
+++ uvideo.c17 May 2016 22:51:49 -
@@ -3202,7 +3202,7 @@ uvideo_reqbufs(void *v, struct v4l2_requ
sc->sc_mmap[i].v4l2_buf.sequence = 0;
sc->sc_mmap[i].v4l2_buf.field = V4L2_FIELD_NONE;
sc->sc_mmap[i].v4l2_buf.memory = V4L2_MEMORY_MMAP;
-   sc->sc_mmap[i].v4l2_buf.flags = V4L2_MEMORY_MMAP;
+   sc->sc_mmap[i].v4l2_buf.flags = V4L2_BUF_FLAG_MAPPED;
 
DPRINTF(1, "%s: %s: index=%d, offset=%d, length=%d\n",
DEVNAME(sc), __func__,

Re: [patch] theo.c [was: TLS now supported on openbsd.org?]

[patrick keshishian]

I must be missing something. I thought that was a funny comment from Theo.

--patrick


On 5/9/16, Bob Beck  wrote:
> I am gonna say no.  i am the bigger holdout for this particular issue than
> theo.  he has been working on me quietly behind the scenes to soften me up
> about it while maintaing a stern face publicly to protect his people.  you
> know what, ill happily pick on theo for eccenticities, and when he is a
> dick. but no. unless you start a beck.c there aint no. way this is fair,
> sorry. You are pointing that where it doesnt belong and you need to take
> that back.  You wanna complain about me, great. bring it. But cut someone a
> break who is trying to do the right thing on both sides.
>
> On Monday, 9 May 2016, patrick keshishian  wrote:
>
>> if I didn't, someone else would.
>>
>> Index: theo.c
>> ===
>> RCS file: /cvs/src/usr.bin/mg/theo.c,v
>> retrieving revision 1.150
>> diff -u -p -u -p -r1.150 theo.c
>> --- theo.c  23 Dec 2015 19:37:34 -  1.150
>> +++ theo.c  10 May 2016 03:07:12 -
>> @@ -1,4 +1,4 @@
>> -/* $OpenBSD: theo.c,v 1.150 2015/12/23 19:37:34 lum Exp $  */
>> +/* $OpenBSD: theo.c,v 1.149 2015/11/14 21:24:11 sthen Exp $
>> */
>>  /*
>>   * Copyright (c) 2002 Artur Grabowski >
>>   * All rights reserved.
>> @@ -195,6 +195,7 @@ static const char *talk[] = {
>> "I want a new vax, one that's not so slow.",
>> "This sausage is made from unsound meat.",
>> "The people who wrote this code are not on your side.",
>> +   "Go take your opinions and give them to your BANKS INSTEAD.",
>>  };
>>
>>  static const int ntalk = sizeof(talk)/sizeof(talk[0]);
>>
>>
>> -- Forwarded message --
>> From: Theo de Raadt >
>> Date: Mon, 09 May 2016 15:39:17 -0600
>> Subject: Re: TLS now supported on openbsd.org?
>> To: Rubén Llorente >
>> Cc: m...@openbsd.org 
>>
>> > Giancarlo Razzolini > wrote:
>> > > It is really nice to finally see TLS on openbsd.org. How about
>> redirecting
>> > > http to https?
>> >
>> > I dislike the idea.
>>
>> Let me be more clear, both of you.
>>
>> Those decisions will made by the people (Bob et all) who maintain the
>> back end.
>>
>> They don't need your opinions.  Go take your opinions and give them
>> to your BANKS INSTEAD.
>>
>>
>

[patch] theo.c [was: TLS now supported on openbsd.org?]

[patrick keshishian]

if I didn't, someone else would.

Index: theo.c
===
RCS file: /cvs/src/usr.bin/mg/theo.c,v
retrieving revision 1.150
diff -u -p -u -p -r1.150 theo.c
--- theo.c  23 Dec 2015 19:37:34 -  1.150
+++ theo.c  10 May 2016 03:07:12 -
@@ -1,4 +1,4 @@
-/* $OpenBSD: theo.c,v 1.150 2015/12/23 19:37:34 lum Exp $  */
+/* $OpenBSD: theo.c,v 1.149 2015/11/14 21:24:11 sthen Exp $*/
 /*
  * Copyright (c) 2002 Artur Grabowski 
  * All rights reserved.
@@ -195,6 +195,7 @@ static const char *talk[] = {
"I want a new vax, one that's not so slow.",
"This sausage is made from unsound meat.",
"The people who wrote this code are not on your side.",
+   "Go take your opinions and give them to your BANKS INSTEAD.",
 };

 static const int ntalk = sizeof(talk)/sizeof(talk[0]);


-- Forwarded message --
From: Theo de Raadt 
Date: Mon, 09 May 2016 15:39:17 -0600
Subject: Re: TLS now supported on openbsd.org?
To: Rubén Llorente 
Cc: m...@openbsd.org

> Giancarlo Razzolini  wrote:
> > It is really nice to finally see TLS on openbsd.org. How about redirecting
> > http to https?
>
> I dislike the idea.

Let me be more clear, both of you.

Those decisions will made by the people (Bob et all) who maintain the
back end.

They don't need your opinions.  Go take your opinions and give them
to your BANKS INSTEAD.

Re: synaptics: two-finger scrolling and coasting

[patrick keshishian]

fwiw, I've been running this on my lenovo x120e for a the week.
--patrick

On 4/21/16, Ulf Brosziewski  wrote:
> Ping?
>
> It isn't a severe bug and it doesn't concern a spectacular feature,
> but shouldn't we fix it?
>
>
> On 04/16/2016 06:01 PM, Ulf Brosziewski wrote:
>> The changes I have introduced in wsconscomm.c recently can make the
>> transition from two-finger scrolling to coasting somewhat difficult.
>> Provided that you have scrolled quickly enough, coasting will start
>> if you finish the scroll gesture by lifting both fingers simultaneously,
>> but a short delay will prevent coasting. When the contact count drops
>> from 2 to 1 a RESET event will occur, and the handler clears the scroll
>> flag.
>>
>> The diff below is a fix for this problem. The new version won't interrupt
>> or stop scrolling, rather it ensures that the current coordinate deltas
>> won't affect its speed or direction. I have tested it with my Elantech-v4
>> touchpad.
>>
>> OK?
>>
>>
>> Index: src/wsconscomm.c
>> ===
>> RCS file: /cvs/xenocara/driver/xf86-input-synaptics/src/wsconscomm.c,v
>> retrieving revision 1.14
>> diff -u -p -r1.14 wsconscomm.c
>> --- src/wsconscomm.c 30 Mar 2016 23:33:34 -  1.14
>> +++ src/wsconscomm.c 16 Apr 2016 15:15:46 -
>> @@ -150,6 +150,21 @@ WSConsQueryHardware(InputInfoPtr pInfo)
>>  return WSConsIsTouchpad(pInfo, NULL);
>>  }
>>
>> +static void
>> +WSConsAdjustScrollCoords(SynapticsPrivate *priv, struct SynapticsHwState
>> *hw)
>> +{
>> +int dx, dy, i;
>> +
>> +dx = hw->x - priv->scroll.last_x;
>> +dy = hw->y - priv->scroll.last_y;
>> +priv->scroll.last_x = hw->x;
>> +priv->scroll.last_y = hw->y;
>> +for (i = 0; i < SYNAPTICS_MOVE_HISTORY; i++) {
>> +priv->move_hist[i].x += dx;
>> +priv->move_hist[i].y += dy;
>> +}
>> +}
>> +
>>  static Bool
>>  WSConsReadHwState(InputInfoPtr pInfo,
>>  struct CommData *comm, struct SynapticsHwState *hwRet)
>> @@ -158,7 +173,7 @@ WSConsReadHwState(InputInfoPtr pInfo,
>>  struct wsconscomm_proto_data *proto_data = priv->proto_data;
>>  struct SynapticsHwState *hw = comm->hwState;
>>  struct wscons_event *event;
>> -Bool v;
>> +Bool v, reset = FALSE;
>>
>>  while ((event = WSConsGetEvent(pInfo)) != NULL) {
>>  switch (event->type) {
>> @@ -229,15 +244,18 @@ WSConsReadHwState(InputInfoPtr pInfo,
>>  hw->fingerWidth = event->value;
>>  break;
>>  case WSCONS_EVENT_TOUCH_RESET:
>> -/*
>> - * The contact count or the active MT-slot has changed.
>> - * Suppress pointer motion and two-finger scrolling.
>> - */
>> -priv->count_packet_finger = 0;
>> -priv->vert_scroll_twofinger_on = FALSE;
>> -priv->horiz_scroll_twofinger_on = FALSE;
>> +/* The contact count or the active MT slot has changed. */
>> +reset = TRUE;
>>  break;
>>  case WSCONS_EVENT_SYNC:
>> +if (reset) {
>> +/* Ensure that pointer motion stops. */
>> +priv->count_packet_finger = 0;
>> +if (priv->vert_scroll_twofinger_on
>> +|| priv->horiz_scroll_twofinger_on) {
>> +WSConsAdjustScrollCoords(priv, hw);
>> +}
>> +}
>>  hw->millis = 1000 * event->time.tv_sec +
>>  event->time.tv_nsec / 100;
>>  SynapticsCopyHwState(hwRet, hw);
>>
>>
>
>

Re: Allow top(1) to search arguments

[patrick keshishian]

On Wed, Feb 10, 2016 at 11:04:18PM +, Edd Barrett wrote:
> Hey,
> 
> I'd like top(1)'s filter feature (-g) to search process arguments. This
> would make searching for (e.g.) Python scripts by name much easier. The
> current behaviour only searches the program name, which for scripts is
> the interpreter binary name (e.g. python2.7). Currently the best you
> could do to find a Python script by name is to filter for "python", then
> press "C" to enable argument display, then scan the args by eyeball.
> 
> Here is a diff that allows searching of arguments, but only if
> arguments are currently being displayed.
> 
> I know the timing is bad, but any comments?
> 
> 
> Index: machine.c
> ===
> RCS file: /home/edd/cvsync/src/usr.bin/top/machine.c,v
> retrieving revision 1.85
> diff -u -p -r1.85 machine.c
> --- machine.c 20 Aug 2015 22:32:42 -  1.85
> +++ machine.c 6 Feb 2016 15:12:42 -
> @@ -57,6 +57,8 @@
>  static int   swapmode(int *, int *);
>  static char  *state_abbr(struct kinfo_proc *);
>  static char  *format_comm(struct kinfo_proc *);
> +int  cmd_matches(struct kinfo_proc *proc, char *cmd);
> +static char  **get_proc_args(struct kinfo_proc *kp);
>  
>  /* get_process_info passes back a handle.  This is what it looks like: */
>  
> @@ -360,6 +362,54 @@ getprocs(int op, int arg, int *cnt)
>   return (procbase);
>  }
>  
> +static char **
> +get_proc_args(struct kinfo_proc *kp)
> +{
> + static char **s;
> + size_t  siz = 100;
> + int mib[4];
> +
> + for (;; siz *= 2) {
> + if ((s = realloc(s, siz)) == NULL)
> + err(1, NULL);
> + mib[0] = CTL_KERN;
> + mib[1] = KERN_PROC_ARGS;
> + mib[2] = kp->p_pid;
> + mib[3] = KERN_PROC_ARGV;
> + if (sysctl(mib, 4, s, &siz, NULL, 0) == 0)
> + break;
> + if (errno != ENOMEM)
> + return (NULL);
> + }
> + return (s);
> +}
> +
> +int
> +cmd_matches(struct kinfo_proc *proc, char *term)
> +{
> + extern int  show_args;
> + char**args = NULL, **arg = NULL;
> +
> + if (!term) {
> + /* No command filter set */
> + return (1);
> + } else {
> + /* Filter set, process name needs to contain term */
> + if (strstr(proc->p_comm, term)) {
> + return (1);
> + }
> + /* If showing arguments, search those as well */
> + if (show_args) {
> + args = get_proc_args(proc);
> + for (arg = args; *arg != NULL; arg++) {
 
NULL pointer dereference is possible here.

Also, unclear why you need both arg and args variables.

--patrick

> + if (strstr(*arg, term))
> + return (1);
> + }
> + }
> + }
> + return (0);
> +}
> +
>  caddr_t
>  get_process_info(struct system_info *si, struct process_select *sel,
>  int (*compare) (const void *, const void *))
> @@ -421,8 +471,7 @@ get_process_info(struct system_info *si,
>   (!hide_uid || pp->p_ruid != sel->huid) &&
>   (!show_uid || pp->p_ruid == sel->uid) &&
>   (!show_pid || pp->p_pid == sel->pid) &&
> - (!show_cmd || strstr(pp->p_comm,
> - sel->command))) {
> + (!show_cmd || cmd_matches(pp, sel->command))) {
>   *prefp++ = pp;
>   active_procs++;
>   }
> @@ -462,27 +511,17 @@ state_abbr(struct kinfo_proc *pp)
>  static char *
>  format_comm(struct kinfo_proc *kp)
>  {
> - static char **s, buf[MAX_COLS];
> - size_t siz = 100;
> - char **p;
> - int mib[4];
> + static char buf[MAX_COLS];
> + char**p, **s;
>   extern int show_args;
>  
>   if (!show_args)
>   return (kp->p_comm);
>  
> - for (;; siz *= 2) {
> - if ((s = realloc(s, siz)) == NULL)
> - err(1, NULL);
> - mib[0] = CTL_KERN;
> - mib[1] = KERN_PROC_ARGS;
> - mib[2] = kp->p_pid;
> - mib[3] = KERN_PROC_ARGV;
> - if (sysctl(mib, 4, s, &siz, NULL, 0) == 0)
> - break;
> - if (errno != ENOMEM)
> - return (kp->p_comm);
> - }
> + s = get_proc_args(kp);
> + if (s == NULL)
> + return (kp->p_comm);
> +
>   buf[0] = '\0';
>   for (p = s; *p != NULL; p++) {
>   if (p != s)
> Index: top.1
> ===
> RCS file: /home/edd/cvsync/src/usr.bin/top/top.1,v
> retrieving revision 1.66
> diff -u -p -r

Re: pkg_add.1

[patrick keshishian]

On Mon, Feb 08, 2016 at 07:28:24PM +0100, Marc Espie wrote:
> On Sun, Feb 07, 2016 at 09:42:32AM -0600, joshua stein wrote:
> > We don't recommend FTP mirrors anymore, installing a package via a
> > pipe doesn't seem to work anymore, and packages have to be signed to
> > be installed so the advice about miscreants is not very relevant.
> > 
> > 
> installing packages thru pipes should still work.
> surprised it got broken.
> 
> you can still install non-signed packages if you really try.

I haven't build ports in a while, but does this comment mean
that if I'm building my own ports (moving forward), the
resulting packages must be signed?

--patrick

Re: libc: getusershell, new implementation

[patrick keshishian]

On Sat, Dec 05, 2015 at 01:25:10PM -0500, Ted Unangst wrote:
> Tobias Stoeckmann wrote:
> > 
> > And I still think that the current code is a bit too permissive in parsing
> > things. I mean what's the point in allowing lines like:
> > 
> > sometextwithoutspace/bin/ksh should be used for logins # seriously!
> > 
> > Which would result in /bin/ksh, by the way.
> > 
> > Didn't notice the consequences that arise by keeping the descriptor open,
> > so I'm fine with an alternative approach. Yet we might make the code a
> > bit easier to review by not allowing such weird lines. What it should
> > expect and enforce:
> > 
> > - a valid line has to start with a slash
> > - comments are chopped off
> > - comments are supposed to be at the beginning of a line
> > 
> > So if somebody writes "/bin/ksh # comment", that actually leads to 
> > "/bin/ksh ",
> > with an additional whitespace at the end. Currently we couldn't even 
> > specify a
> > shell with a whitespace in its path.
> 
> ok. i was going to leave the behavior alone, but we can fix that too.
> 
> - use getline to read lines of any length.
> - only consider lines that start with a /.
> - truncate lines after a #, but not after spaces.
> 
> 
> Index: gen/getusershell.c
> ===
> RCS file: /cvs/src/lib/libc/gen/getusershell.c,v
> retrieving revision 1.16
> diff -u -p -r1.16 getusershell.c
> --- gen/getusershell.c14 Sep 2015 16:09:13 -  1.16
> +++ gen/getusershell.c5 Dec 2015 18:24:33 -
> @@ -28,14 +28,13 @@
>   * SUCH DAMAGE.
>   */
>  
> -#include 
> -
>  #include 
>  #include 
>  #include 
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  
>  /*
> @@ -44,7 +43,7 @@
>   */
>  
>  static char *okshells[] = { _PATH_BSHELL, _PATH_CSHELL, _PATH_KSHELL, NULL };
> -static char **curshell, **shells, *strings;
> +static char **curshell, **shells;
>  static char **initshells(void);
>  
>  /*
> @@ -66,11 +65,14 @@ getusershell(void)
>  void
>  endusershell(void)
>  {
> - 
> + char **s;
> +
> + if ((s = shells))
> + while (*s)
> + free(*s++);
>   free(shells);
>   shells = NULL;
> - free(strings);
> - strings = NULL;
> +
>   curshell = NULL;
>  }
>  
> @@ -84,48 +86,50 @@ setusershell(void)
>  static char **
>  initshells(void)
>  {
> - char **sp, *cp;
> + size_t nshells, nalloc, linesize;
> + char *line;
>   FILE *fp;
> - struct stat statb;
>  
>   free(shells);
>   shells = NULL;
> - free(strings);
> - strings = NULL;
> +
>   if ((fp = fopen(_PATH_SHELLS, "re")) == NULL)
>   return (okshells);
> - if (fstat(fileno(fp), &statb) == -1) {
> - (void)fclose(fp);
> - return (okshells);
> - }
> - if (statb.st_size > SIZE_MAX) {
> - (void)fclose(fp);
> - return (okshells);
> - }
> - if ((strings = malloc((size_t)statb.st_size)) == NULL) {
> - (void)fclose(fp);
> - return (okshells);
> - }
> - shells = calloc((size_t)(statb.st_size / 3 + 2), sizeof (char *));
> - if (shells == NULL) {
> - (void)fclose(fp);
> - free(strings);
> - strings = NULL;
> - return (okshells);
> - }
> - sp = shells;
> - cp = strings;
> - while (fgets(cp, PATH_MAX + 1, fp) != NULL) {
> - while (*cp != '#' && *cp != '/' && *cp != '\0')
> - cp++;
> - if (*cp == '#' || *cp == '\0')
> +
> + line = NULL;
> + nalloc = 10; // just an initial guess
> + nshells = 0;
> + shells = reallocarray(NULL, nalloc, sizeof (char *));
> + if (shells == NULL)
> + goto fail;
> + linesize = 0;
> + while (getline(&line, &linesize, fp) != -1) {
> + if (*line != '/')
>   continue;
> - *sp++ = cp;
> - while (!isspace((unsigned char)*cp) && *cp != '#' && *cp != 
> '\0')
> - cp++;
> - *cp++ = '\0';
> + line[strcspn(line, "#\n")] = '\0';
> + if (!(shells[nshells] = strdup(line)))
> + goto fail;
> +
> + nshells++;
> + if (nshells == nalloc) {
> + char **new = reallocarray(shells, nalloc * 2, 
> sizeof(char *));
> + if (!new)
> + goto fail;

This 'goto fail' will free() beyond allocated: shells[nshells--]
Better to check 'if (nshells + 1 == nalloc)' and increment nshells
afterward.

--patrick

> + shells = new;
> + nalloc *= 2;
> + }
>   }
> - *sp = NULL;
> + free(line);
> + shells[nshells] = NULL;
>   (void)fclose(fp);
>   return (shells);
> +
> +fail:
> + free(line);
> + while (nshells)
> + free(shells[nshells--]);
> + free(shells);
> + shells =

Re: kern_tame.c: fix strncmp call

[patrick keshishian]

Apologies, my eyes failed me on this.

On 8/23/15, patrick keshishian  wrote:
> On 8/23/15, Caspar Schutijser  wrote:
>> Patch below.
>>
>> Thanks,
>> Caspar Schutijser
>>
>>
>> Index: sys/kern/kern_tame.c
>> ===
>> RCS file: /cvs/src/sys/kern/kern_tame.c,v
>> retrieving revision 1.25
>> diff -u -p -r1.25 kern_tame.c
>> --- sys/kern/kern_tame.c 23 Aug 2015 19:32:20 -  1.25
>> +++ sys/kern/kern_tame.c 23 Aug 2015 21:22:38 -
>> @@ -423,7 +423,7 @@ tame_namei(struct proc *p, char *origpat
>>   */
>>  if ((p->p_p->ps_tame & _TM_TMPPATH) &&
>>  (p->p_tame_syscall == SYS_unlink) &&
>> -strncmp(path, "/tmp/", sizeof("/tmp") - 1) == 0) {
>> +strncmp(path, "/tmp/", sizeof("/tmp/") - 1) == 0) {
>
> you are confusing sizeof() with strlen(). former counts the byte
> required for the terminating NUL.
>
> $ cat >/tmp/a.c
> #include 
> #include 
>
> int main(int argc, char *argv[])
> {
>   printf("sizeof(\"/tmp\")=%zu\n", sizeof("/tmp"));
>   exit(0);
> }
> $ cc  /tmp/a.c -o /tmp/a
> $ /tmp/a
> sizeof("/tmp")=5
>
> --patrick
>
>>  return (0);
>>  }
>>
>>
>>
>

Re: kern_tame.c: fix strncmp call

[patrick keshishian]

On 8/23/15, Caspar Schutijser  wrote:
> Patch below.
>
> Thanks,
> Caspar Schutijser
>
>
> Index: sys/kern/kern_tame.c
> ===
> RCS file: /cvs/src/sys/kern/kern_tame.c,v
> retrieving revision 1.25
> diff -u -p -r1.25 kern_tame.c
> --- sys/kern/kern_tame.c  23 Aug 2015 19:32:20 -  1.25
> +++ sys/kern/kern_tame.c  23 Aug 2015 21:22:38 -
> @@ -423,7 +423,7 @@ tame_namei(struct proc *p, char *origpat
>*/
>   if ((p->p_p->ps_tame & _TM_TMPPATH) &&
>   (p->p_tame_syscall == SYS_unlink) &&
> - strncmp(path, "/tmp/", sizeof("/tmp") - 1) == 0) {
> + strncmp(path, "/tmp/", sizeof("/tmp/") - 1) == 0) {

you are confusing sizeof() with strlen(). former counts the byte
required for the terminating NUL.

$ cat >/tmp/a.c
#include 
#include 

int main(int argc, char *argv[])
{
printf("sizeof(\"/tmp\")=%zu\n", sizeof("/tmp"));
exit(0);
}
$ cc  /tmp/a.c -o /tmp/a
$ /tmp/a
sizeof("/tmp")=5

--patrick

>   return (0);
>   }
>
>
>

Re: [patch] vi: fix "file modified more recently than this copy ..." error

[patrick keshishian]

On 7/7/15, Todd C. Miller  wrote:
> I prefer this diff instead.

Thanks. I too like this better.
--patrick


>  - todd
>
> Index: usr.bin/vi/common/exf.c
> ===
> RCS file: /cvs/src/usr.bin/vi/common/exf.c,v
> retrieving revision 1.36
> diff -u -p -u -r1.36 exf.c
> --- usr.bin/vi/common/exf.c   24 Apr 2015 21:48:31 -  1.36
> +++ usr.bin/vi/common/exf.c   7 Jul 2015 15:53:28 -
> @@ -185,7 +185,8 @@ file_init(SCR *sp, FREF *frp, char *rcv_
>   (void)snprintf(tname, sizeof(tname),
>   "%s/vi.XX", O_STR(sp, O_TMP_DIRECTORY));
>   fd = mkstemp(tname);
> - if (fd == -1 || fchmod(fd, S_IRUSR | S_IWUSR) == -1) {
> + if (fd == -1 || fstat(fd, &sb) == -1 ||
> + fchmod(fd, S_IRUSR | S_IWUSR) == -1) {
>   msgq(sp, M_SYSERR,
>   "237|Unable to create temporary file");
>   if (fd != -1) {
> @@ -210,8 +211,6 @@ file_init(SCR *sp, FREF *frp, char *rcv_
>   psize = 1024;
>   if (!LF_ISSET(FS_OPENERR))
>   F_SET(frp, FR_NEWFILE);
> -
> - (void)clock_gettime(CLOCK_REALTIME, &ep->mtim);
>   } else {
>   /*
>* XXX
> @@ -226,16 +225,17 @@ file_init(SCR *sp, FREF *frp, char *rcv_
>   psize = 1;
>   psize *= 1024;
>
> - F_SET(ep, F_DEVSET);
> - ep->mdev = sb.st_dev;
> - ep->minode = sb.st_ino;
> -
> - ep->mtim = sb.st_mtim;
> -
>   if (!S_ISREG(sb.st_mode))
>   msgq_str(sp, M_ERR, oname,
>   "238|Warning: %s is not a regular file");
>   }
> +
> + /* Save device, inode and modification time. */
> + F_SET(ep, F_DEVSET);
> + ep->mdev = sb.st_dev;
> + ep->minode = sb.st_ino;
> +
> + ep->mtim = sb.st_mtim;
>
>   /* Set up recovery. */
>   memset(&oinfo, 0, sizeof(RECNOINFO));
>
>

Re: [patch] vi: fix "file modified more recently than this copy ..." error

[patrick keshishian]

ping?

On 6/9/15, patrick keshishian  wrote:
> Hi,
>
> Noticed a regression with vi and recent changes to timespec
> data types.
>
> To reproduce, run vi without a file name to edit. Try save buffer
> via ":w" and you'll be presented by following message:
>
> file modified more recently than this this copy; use ! to override
>
> Patch below fixes this.
>
> Hope this is acceptable.
>
> Thanks,
> --patrick
>
> Index: common/exf.c
> ===
> RCS file: /cvs/obsd/src/usr.bin/vi/common/exf.c,v
> retrieving revision 1.36
> diff -u -p -u -p -r1.36 exf.c
> --- common/exf.c  24 Apr 2015 21:48:31 -  1.36
> +++ common/exf.c  10 Jun 2015 03:55:58 -
> @@ -211,7 +211,10 @@ file_init(SCR *sp, FREF *frp, char *rcv_
>   if (!LF_ISSET(FS_OPENERR))
>   F_SET(frp, FR_NEWFILE);
>
> - (void)clock_gettime(CLOCK_REALTIME, &ep->mtim);
> + if (stat(tname, &sb))
> + (void)clock_gettime(CLOCK_REALTIME, &ep->mtim);
> + else
> + ep->mtim = sb.st_mtim;
>   } else {
>   /*
>* XXX
>
>

Re: PATCH: faq4.html edit the auto layout

[patrick keshishian]

Hi,

On 6/29/15, Juan Francisco Cantero Hurtado  wrote:
> It's surprising but many users don't know the "R" option. OK?
>
> The webpage with the patch applied:
> http://juanfra684.devio.us/tmp/faq4.html#InstDisks
>
> diff --git faq/faq4.html faq/faq4.html
> index 51606b3..f2b9486 100644
> --- faq/faq4.html
> +++ faq/faq4.html
> @@ -946,15 +946,18 @@ The installer has presented us with its proposed "Auto
> layout" for
>  OpenBSD partitions on our disk, which we are going to accept.
>
>  
> -If the proposed layout is not appropriate for your needs, you can,
> -of course, edit the default or customize it completely, more details
> -on the disklabel partitioning  id="BackMoredisklabel">below.
> +If the proposed layout is not appropriate for your needs, you can
> +(E)dit the sizes of the partitions with the R option in
> the
> +disklabel editor or customize it completely, more details on the disklabel
> +partitioning
> +below.
>
>  
>  NOTE for re-installers:
>  The new installer will not clear your old disklabel if you chose
> -"(C)ustom Layout", but you will need to re-specify each mount point
> -using the 'm' option in disklabel(8).

I think you meant the "n" option.

--patrick


> +(C)ustom Layout, but you will need to re-specify each mount point
> +using the m option in
> + href="http://www.openbsd.org/cgi-bin/man.cgi?query=disklabel&sektion=8";>disklabel(8).
>
>  
>  The installer now creates those partitions and creates file systems
>
>

Re: systrace patch for chflagsat

[patrick keshishian]

Hi,

On Sat, Jun 13, 2015 at 09:41:58PM -0700, Philip Guenther wrote:
> On Thu, 4 Jun 2015, patrick keshishian wrote:
> > With the cp(1) change to use chflagsat(), systrace needs to be taught 
> > about chflagsat syscall. databases/db/v4 port fails with 
> > USE_SYSTRACE=Yes due to a "$(CP) -pr" use during install_docs.
> > 
> > Could someone look over these changes and correct any mistakes. I am not 
> > 100% sure it is correct; specifically, I'm not sure about the 
> > print_atflags.
> 
> Hmm, the other *at calls don't seem to do special handling for the 
> atflags.  As a first pass, perhaps the chflagsat support should do the 
> same, with support for atflag matching as a second pass?
> 
> If that's sane, then I think the diff, modelling on the existing fchmodat 
> support, just becomes this, no?

I trust your judgement. Thanks for looking into this.

--patrick

p.s., Sorry for the late reply; I've had limit time with email.


> Philip Guenther
> 
> Index: register.c
> ===
> RCS file: /data/src/openbsd/src/bin/systrace/register.c,v
> retrieving revision 1.25
> diff -u -p -r1.25 register.c
> --- register.c16 Jan 2015 00:19:12 -  1.25
> +++ register.c8 Jun 2015 08:44:16 -
> @@ -258,6 +258,11 @@ systrace_initcb(void)
>   alias = systrace_new_alias("native", "fstatat", "native", "fsread");
>   systrace_alias_add_trans(alias, tl);
>  
> + X(intercept_register_sccb("native", "chflagsat", trans_cb, NULL));
> + intercept_register_translation("native", "chflagsat", 1,
> + &ic_translate_filenameatflag);
> + intercept_register_translation("native", "chflagsat", 2, &ic_fileflags);
> +
>   X(intercept_register_sccb("native", "linkat", trans_cb, NULL));
>   intercept_register_translation("native", "linkat", 1,
>   &ic_translate_unlinknameatflag);
> 

[patch] vi: fix "file modified more recently than this copy ..." error

[patrick keshishian]

Hi,

Noticed a regression with vi and recent changes to timespec
data types.

To reproduce, run vi without a file name to edit. Try save buffer
via ":w" and you'll be presented by following message:

file modified more recently than this this copy; use ! to override

Patch below fixes this.

Hope this is acceptable.

Thanks,
--patrick

Index: common/exf.c
===
RCS file: /cvs/obsd/src/usr.bin/vi/common/exf.c,v
retrieving revision 1.36
diff -u -p -u -p -r1.36 exf.c
--- common/exf.c24 Apr 2015 21:48:31 -  1.36
+++ common/exf.c10 Jun 2015 03:55:58 -
@@ -211,7 +211,10 @@ file_init(SCR *sp, FREF *frp, char *rcv_
if (!LF_ISSET(FS_OPENERR))
F_SET(frp, FR_NEWFILE);
 
-   (void)clock_gettime(CLOCK_REALTIME, &ep->mtim);
+   if (stat(tname, &sb))
+   (void)clock_gettime(CLOCK_REALTIME, &ep->mtim);
+   else
+   ep->mtim = sb.st_mtim;
} else {
/*
 * XXX

Re: clean the tree from superfluous "case '?': /* FALLTHROUGH */"

[patrick keshishian]

Hi,

On Tue, Jun 09, 2015 at 07:46:20PM +0200, Benjamin Baier wrote:
> Hello tech@
> 
> delete "case '?': /* FALLTHROUGH */" where it is already handled 
> by "default: usage();".

Not quite sure it is correct to remove the '?' case from
npppd.c, since there is no 'default' case.

Also, if the idea is to remove cases which simply fall through
'default', these few (10) have 'h' cases that do same (judging
from your diff):

> RCS file: /cvs/src/games/arithmetic/arithmetic.c,v
> RCS file: /cvs/src/games/banner/banner.c,v
> RCS file: /cvs/src/games/fish/fish.c,v
> RCS file: /cvs/src/games/grdc/grdc.c,v
> RCS file: /cvs/src/games/hangman/main.c,v
> RCS file: /cvs/src/games/morse/morse.c,v
> RCS file: /cvs/src/games/number/number.c,v
> RCS file: /cvs/src/games/ppt/ppt.c,v
> RCS file: /cvs/src/games/snake/snake.c,v
> RCS file: /cvs/src/games/worms/worms.c,v


--patrick


...
> Index: games/arithmetic/arithmetic.c
> ===
> RCS file: /cvs/src/games/arithmetic/arithmetic.c,v
> retrieving revision 1.18
> diff -u -p -r1.18 arithmetic.c
> --- games/arithmetic/arithmetic.c 29 Aug 2013 20:22:09 -  1.18
> +++ games/arithmetic/arithmetic.c 9 Jun 2015 17:28:09 -
> @@ -116,7 +116,6 @@ main(int argc, char *argv[])
>   if ((rangemax = atoi(optarg)) <= 0)
>   errx(1, "invalid range.");
>   break;
> - case '?':
>   case 'h':
>   default:
>   usage();
> Index: games/banner/banner.c
> ===
> RCS file: /cvs/src/games/banner/banner.c,v
> retrieving revision 1.17
> diff -u -p -r1.17 banner.c
> --- games/banner/banner.c 10 Feb 2015 13:50:58 -  1.17
> +++ games/banner/banner.c 9 Jun 2015 17:28:09 -
> @@ -1030,7 +1030,7 @@ main(int argc, char *argv[])
>   if (width <= 0 || width > DWIDTH)
>   errx(1, "illegal argument for -w option");
>   break;
> - case '?': case 'h':
> + case 'h':
>   default:
>   (void)fprintf(stderr,
>   "usage: banner [-w width] message ...\n");
...
> ===
> RCS file: /cvs/src/games/fish/fish.c,v
> retrieving revision 1.17
> diff -u -p -r1.17 fish.c
> --- games/fish/fish.c 18 Feb 2015 23:20:45 -  1.17
> +++ games/fish/fish.c 9 Jun 2015 17:28:09 -
> @@ -92,7 +92,6 @@ main(int argc, char *argv[])
>   case 'p':
>   promode = 1;
>   break;
> - case '?':
>   case 'h':
>   default:
>   usage();
>   }
...
> Index: games/grdc/grdc.c
> ===
> RCS file: /cvs/src/games/grdc/grdc.c,v
> retrieving revision 1.19
> diff -u -p -r1.19 grdc.c
> --- games/grdc/grdc.c 19 Nov 2014 03:27:45 -  1.19
> +++ games/grdc/grdc.c 9 Jun 2015 17:28:09 -
> @@ -77,7 +77,6 @@ main(int argc, char *argv[])
>   scrol = 1;
>   break;
>   case 'h':
> - case '?':
>   default:
>   usage();
>   }
> Index: games/hangman/main.c
> ===
> RCS file: /cvs/src/games/hangman/main.c,v
> retrieving revision 1.12
> diff -u -p -r1.12 main.c
> --- games/hangman/main.c  7 Feb 2015 01:37:30 -   1.12
> +++ games/hangman/main.c  9 Jun 2015 17:28:09 -
> @@ -56,7 +56,6 @@ main(int argc, char *argv[])
>   Dict_name = _PATH_KSYMS;
>   break;
>   case 'h':
> - case '?':
>   default:
>   usage();
>   }
> Index: games/morse/morse.c
> ===
> RCS file: /cvs/src/games/morse/morse.c,v
> retrieving revision 1.15
> diff -u -p -r1.15 morse.c
> --- games/morse/morse.c   27 Nov 2013 13:32:02 -  1.15
> +++ games/morse/morse.c   9 Jun 2015 17:28:09 -
> @@ -120,7 +120,7 @@ main(int argc, char *argv[])
>   case 's':
>   sflag = 1;
>   break;
> - case '?': case 'h':
> + case 'h':
>   default:
>   fprintf(stderr, "usage: morse [-d | -s] [string 
> ...]\n");
>   exit(1);
> Index: games/number/number.c
> ===
> RCS file: /cvs/src/games/number/number.c,v
> retrieving revision 1.14
> diff -u -p -r1.14 number.c
> --- games/number/number.c 27 Oct 2009 23:59:26 -  1.14
> +++ games/number/number.c 9 Jun 2015 17:28:09 -
> @@ -85

Re: file(1): is this reasonable?

[patrick keshishian]

On Thu, Jun 04, 2015 at 11:37:33PM +0100, Nicholas Marriott wrote:
> file shouldn't need chflagsat?

Yes. The chflagsat was for cp(1). I'm just combining the diff
from earlier (sent to ports[1]), so it does not get lost.

Thanks,
--patrick

[1] http://marc.info/?l=openbsd-ports&m=143340587303885&w=2


> Otherwise I think this is fine.
> 
> 
> On Thu, Jun 04, 2015 at 03:29:06PM -0700, patrick keshishian wrote:
> > On Thu, Jun 04, 2015 at 11:06:38PM +0100, Nicholas Marriott wrote:
> > > /usr/ports/infrastructure/db/systrace.filter has these:
> > > 
> > > native-recvmsg: permit
> > > native-sendmsg: sockaddr match "/tmp" then permit
> > >   native-sendmsg: sockaddr match "/var/tmp" then permit
> > >   native-sendmsg: sockaddr match "/tmp" then permit
> > >   native-sendmsg: sockaddr match "/usr/ports/pobj/unzip-6.0" then permit
> > > native-sendmsg: sockaddr match "/: *" then 
> > > deny[enoent]
> > > 
> > > We could add this I think:
> > > 
> > > native-sendmsg: sockaddr eq "" then permit
> > 
> > If this is acceptable, then the file(1) patch reduces to simply
> > skipping the systrace set-up if STRIOCATTACH fails.
> > 
> > Patches follow for file(1) and ports' systrace.policy
> > 
> > 
> > Index: sandbox.c
> > ===
> > RCS file: /cvs/obsd/src/usr.bin/file/sandbox.c,v
> > retrieving revision 1.7
> > diff -u -p -u -p -r1.7 sandbox.c
> > --- sandbox.c   29 May 2015 15:58:34 -  1.7
> > +++ sandbox.c   4 Jun 2015 22:23:32 -
> > @@ -130,7 +130,7 @@ sandbox_fork(const char *user)
> > close(devfd);
> >  
> > if (ioctl(fd, STRIOCATTACH, &pid) == -1)
> > -   err(1, "ioctl(STRIOCATTACH)");
> > +   goto out;
> >  
> > memset(&policy, 0, sizeof policy);
> > policy.strp_op = SYSTR_POLICY_NEW;
> > @@ -150,7 +150,7 @@ sandbox_fork(const char *user)
> > err(1, "ioctl(STRIOCPOLICY/MODIFY)");
> > }
> >  
> > -   if (kill(pid, SIGCONT) != 0)
> > +out:   if (kill(pid, SIGCONT) != 0)
> > err(1, "kill(SIGCONT)");
> > return (pid);
> >  }
> > 
> > 
> > Index: systrace.filter
> > ===
> > RCS file: /cvs/obsd/ports/infrastructure/db/systrace.filter,v
> > retrieving revision 1.45
> > diff -u -p -u -p -r1.45 systrace.filter
> > --- systrace.filter 11 Sep 2014 10:33:44 -  1.45
> > +++ systrace.filter 4 Jun 2015 22:25:08 -
> > @@ -22,6 +22,7 @@
> > native-chflags: filename match "${TMPDIR}" then permit
> > native-chflags: filename match "${WRKDIR}" then permit
> > native-chflags: filename match "/: *" then 
> > deny[enoent]
> > +   native-chflagsat: filename match "${WRKDIR}" then permit
> > native-chmod: filename match "/tmp" then permit
> > native-chmod: filename match "/var/tmp" then permit
> > native-chmod: filename match "${TMPDIR}" then permit
> > @@ -93,6 +94,7 @@
> > native-futimes: permit
> > native-futimens: permit
> > native-getdents: permit
> > +   native-getdtablecount: permit
> > native-getegid: permit
> > native-getentropy: permit
> > native-geteuid: permit
> > @@ -196,6 +198,7 @@
> > native-sendmsg: sockaddr match "${TMPDIR}" then permit
> > native-sendmsg: sockaddr match "${WRKDIR}" then permit
> > native-sendmsg: sockaddr match "/: *" then 
> > deny[enoent]
> > +   native-sendmsg: sockaddr eq "" then permit
> > native-sendsyslog: permit
> > native-sendto: permit
> > native-setegid: permit
> > 
> > 
> > 
> > > 
> > > On Thu, Jun 04, 2015 at 10:47:47PM +0100, Nicholas Marriott wrote:
> > > > Hi
> > > > 
> > > > On Thu, Jun 04, 2015 at 03:39:45PM -0600, Theo de Raadt wrote:
> > > > > > Is it just to avoid adding sendmsg to the ports systrace policy? 
> > > > > > Why not
> > > > > > add it - maybe not globally but just for file?
> > > > > 
> > > > > sendmsg with a CMSG fd passing in/out of such a jail is a bad thing.
> > > > 
> > > > The systrace policy already allows recvmsg(). So we can get new fds in,
> > > > why not send them out?
> > > > 
> > > > Any fd we have inside to send out will have had to have passed the
> > > > open(), bind() etc systrace rules already.
> > > > 
> > > > > 
> > > > > However.
> > > > > 
> > > > > It is likely that a ports configure test may try to test this 
> > > > > interface.
> > > > > Not just CMSG, but sendmsg itself.
> > > > > 
> > > > > It suspect it needs to find that it works.
> > > > > 
> > > > > I doubt this is a system call that can be blocked.
> > > > > 
> > > > > It sounds like a great idea to limit the build environment 
> > > > > substantially,
> > > > > but an eye must be kept on fallout from being too strict.  That's the
> > > > > problem with systrace; it is too easy to return an 'error' and a 
> > > > > program
> > > > > will continue...
> > > > > 
> > > 
> 

Re: file(1): is this reasonable?

[patrick keshishian]

On Thu, Jun 04, 2015 at 11:06:38PM +0100, Nicholas Marriott wrote:
> /usr/ports/infrastructure/db/systrace.filter has these:
> 
> native-recvmsg: permit
> native-sendmsg: sockaddr match "/tmp" then permit
>   native-sendmsg: sockaddr match "/var/tmp" then permit
>   native-sendmsg: sockaddr match "/tmp" then permit
>   native-sendmsg: sockaddr match "/usr/ports/pobj/unzip-6.0" then permit
> native-sendmsg: sockaddr match "/: *" then 
> deny[enoent]
> 
> We could add this I think:
> 
> native-sendmsg: sockaddr eq "" then permit

If this is acceptable, then the file(1) patch reduces to simply
skipping the systrace set-up if STRIOCATTACH fails.

Patches follow for file(1) and ports' systrace.policy


Index: sandbox.c
===
RCS file: /cvs/obsd/src/usr.bin/file/sandbox.c,v
retrieving revision 1.7
diff -u -p -u -p -r1.7 sandbox.c
--- sandbox.c   29 May 2015 15:58:34 -  1.7
+++ sandbox.c   4 Jun 2015 22:23:32 -
@@ -130,7 +130,7 @@ sandbox_fork(const char *user)
close(devfd);
 
if (ioctl(fd, STRIOCATTACH, &pid) == -1)
-   err(1, "ioctl(STRIOCATTACH)");
+   goto out;
 
memset(&policy, 0, sizeof policy);
policy.strp_op = SYSTR_POLICY_NEW;
@@ -150,7 +150,7 @@ sandbox_fork(const char *user)
err(1, "ioctl(STRIOCPOLICY/MODIFY)");
}
 
-   if (kill(pid, SIGCONT) != 0)
+out:   if (kill(pid, SIGCONT) != 0)
err(1, "kill(SIGCONT)");
return (pid);
 }


Index: systrace.filter
===
RCS file: /cvs/obsd/ports/infrastructure/db/systrace.filter,v
retrieving revision 1.45
diff -u -p -u -p -r1.45 systrace.filter
--- systrace.filter 11 Sep 2014 10:33:44 -  1.45
+++ systrace.filter 4 Jun 2015 22:25:08 -
@@ -22,6 +22,7 @@
native-chflags: filename match "${TMPDIR}" then permit
native-chflags: filename match "${WRKDIR}" then permit
native-chflags: filename match "/: *" then 
deny[enoent]
+   native-chflagsat: filename match "${WRKDIR}" then permit
native-chmod: filename match "/tmp" then permit
native-chmod: filename match "/var/tmp" then permit
native-chmod: filename match "${TMPDIR}" then permit
@@ -93,6 +94,7 @@
native-futimes: permit
native-futimens: permit
native-getdents: permit
+   native-getdtablecount: permit
native-getegid: permit
native-getentropy: permit
native-geteuid: permit
@@ -196,6 +198,7 @@
native-sendmsg: sockaddr match "${TMPDIR}" then permit
native-sendmsg: sockaddr match "${WRKDIR}" then permit
native-sendmsg: sockaddr match "/: *" then 
deny[enoent]
+   native-sendmsg: sockaddr eq "" then permit
native-sendsyslog: permit
native-sendto: permit
native-setegid: permit



> 
> On Thu, Jun 04, 2015 at 10:47:47PM +0100, Nicholas Marriott wrote:
> > Hi
> > 
> > On Thu, Jun 04, 2015 at 03:39:45PM -0600, Theo de Raadt wrote:
> > > > Is it just to avoid adding sendmsg to the ports systrace policy? Why not
> > > > add it - maybe not globally but just for file?
> > > 
> > > sendmsg with a CMSG fd passing in/out of such a jail is a bad thing.
> > 
> > The systrace policy already allows recvmsg(). So we can get new fds in,
> > why not send them out?
> > 
> > Any fd we have inside to send out will have had to have passed the
> > open(), bind() etc systrace rules already.
> > 
> > > 
> > > However.
> > > 
> > > It is likely that a ports configure test may try to test this interface.
> > > Not just CMSG, but sendmsg itself.
> > > 
> > > It suspect it needs to find that it works.
> > > 
> > > I doubt this is a system call that can be blocked.
> > > 
> > > It sounds like a great idea to limit the build environment substantially,
> > > but an eye must be kept on fallout from being too strict.  That's the
> > > problem with systrace; it is too easy to return an 'error' and a program
> > > will continue...
> > > 
> 

Re: file(1): is this reasonable?

[patrick keshishian]

On Thu, Jun 04, 2015 at 10:20:35PM +0100, Nicholas Marriott wrote:
> Hi
> 
> I think having two different main()s is silly. Why not keep the privsep
> and everything else but just skip the systrace bits?

Well, part of the problem is to avoid the imsg interface
between parent and child. Theo's suggestion was to solve
this issue with systrace(1)-ed file(1) invocation.

Or did I miss something?

--patrick


> 
> On Thu, Jun 04, 2015 at 02:09:43PM -0700, patrick keshishian wrote:
> > On Thu, Jun 04, 2015 at 12:46:14PM -0600, Theo de Raadt wrote:
> > > > Trying to get file to work under systrace(1).
> > > > 
> > > > Is this a reasonable patch?
> > > 
> > > Semi-reasonable.
> > > 
> > > I think if the ioctl fails, it should be entirely silent.
> > > 
> > > > file(1) still fails with systrace because of sendmsg(2), any
> > > > help with correcting systrace.policy for this case would be
> > > > appreciated.
> > > 
> > > Perhaps this situation can be tested earlier, using an ioctl, and then
> > > the privsep model can be skipped.  Trusting in the non-fragility of
> > > the code, and that it is already operating with some containment.
> > 
> > Nice suggestion. I borrowed from top(1); reason it is in
> > separate file is to avoid Coypright clutter.
> > 
> > Hope this is OK, or at least close to being acceptable.
> > The port build seems to be moving along now.
> > 
> > Also cursory tests (e.g., file -, file *.c, file -c) seem to be
> > doing what they are supposed to do.
> > 
> > --patrick
> > 
> > 
> > Index: Makefile
> > ===
> > RCS file: /cvs/obsd/src/usr.bin/file/Makefile,v
> > retrieving revision 1.15
> > diff -u -p -u -p -r1.15 Makefile
> > --- Makefile27 Apr 2015 13:52:17 -  1.15
> > +++ Makefile4 Jun 2015 21:01:40 -
> > @@ -1,8 +1,8 @@
> >  # $OpenBSD: Makefile,v 1.14 2015/04/27 13:41:45 nicm Exp $
> >  
> >  PROG=   file
> > -SRCS=   file.c magic-dump.c magic-load.c magic-test.c magic-common.c 
> > sandbox.c \
> > -   text.c xmalloc.c
> > +SRCS=   file.c magic-dump.c magic-load.c magic-test.c magic-common.c \
> > +   proc.c sandbox.c text.c xmalloc.c
> >  MAN=   file.1 magic.5
> >  
> >  LDADD= -lutil
> > Index: file.c
> > ===
> > RCS file: /cvs/obsd/src/usr.bin/file/file.c,v
> > retrieving revision 1.45
> > diff -u -p -u -p -r1.45 file.c
> > --- file.c  30 May 2015 06:25:35 -  1.45
> > +++ file.c  4 Jun 2015 21:01:40 -
> > @@ -79,6 +79,7 @@ static int read_message(struct imsgbuf 
> >  static void read_link(struct input_msg *, const char *);
> >  
> >  static __dead void child(int, pid_t, int, char **);
> > +static __dead void solo(int, char **);
> >  
> >  static void test_file(struct input_file *, size_t);
> >  
> > @@ -188,6 +189,9 @@ main(int argc, char **argv)
> > if (magicfp == NULL)
> > err(1, "%s", magicpath);
> >  
> > +   if (being_traced())
> > +   solo(argc, argv);
> > +
> > parent = getpid();
> > if (socketpair(AF_UNIX, SOCK_STREAM, PF_UNSPEC, pair) != 0)
> > err(1, "socketpair");
> > @@ -637,4 +641,60 @@ test_file(struct input_file *inf, size_t
> >  
> > if (inf->mapped && inf->base != NULL)
> > munmap(inf->base, inf->size);
> > +}
> > +
> > +static __dead void
> > +solo(int argc, char *argv[])
> > +{
> > +   struct stat  sb;
> > +   struct input_msg msg;
> > +   struct input_fileinf;
> > +   struct magic*m;
> > +   int  fd, idx;
> > +   size_t   len, width = 0;
> > +
> > +   m = magic_load(magicfp, magicpath, cflag || Wflag);
> > +   if (cflag) {
> > +   magic_dump(m);
> > +   exit(0);
> > +   }
> > +
> > +   for (idx = 0; idx < argc; idx++) {
> > +   len = strlen(argv[idx]) + 1;
> > +   if (len > width)
> > +   width = len;
> > +   }
> > +
> > +   for (idx = 0; idx < argc; idx++) {
> > +   memset(&msg, 0, sizeof msg);
> > +   msg.idx = idx;
> > +
> > +   if (strcmp(argv[idx], "-") == 0) {
> > +   i

Re: file(1): is this reasonable?

[patrick keshishian]

On Thu, Jun 04, 2015 at 12:46:14PM -0600, Theo de Raadt wrote:
> > Trying to get file to work under systrace(1).
> > 
> > Is this a reasonable patch?
> 
> Semi-reasonable.
> 
> I think if the ioctl fails, it should be entirely silent.
> 
> > file(1) still fails with systrace because of sendmsg(2), any
> > help with correcting systrace.policy for this case would be
> > appreciated.
> 
> Perhaps this situation can be tested earlier, using an ioctl, and then
> the privsep model can be skipped.  Trusting in the non-fragility of
> the code, and that it is already operating with some containment.

Nice suggestion. I borrowed from top(1); reason it is in
separate file is to avoid Coypright clutter.

Hope this is OK, or at least close to being acceptable.
The port build seems to be moving along now.

Also cursory tests (e.g., file -, file *.c, file -c) seem to be
doing what they are supposed to do.

--patrick


Index: Makefile
===
RCS file: /cvs/obsd/src/usr.bin/file/Makefile,v
retrieving revision 1.15
diff -u -p -u -p -r1.15 Makefile
--- Makefile27 Apr 2015 13:52:17 -  1.15
+++ Makefile4 Jun 2015 21:01:40 -
@@ -1,8 +1,8 @@
 # $OpenBSD: Makefile,v 1.14 2015/04/27 13:41:45 nicm Exp $
 
 PROG=   file
-SRCS=   file.c magic-dump.c magic-load.c magic-test.c magic-common.c sandbox.c 
\
-   text.c xmalloc.c
+SRCS=   file.c magic-dump.c magic-load.c magic-test.c magic-common.c \
+   proc.c sandbox.c text.c xmalloc.c
 MAN=   file.1 magic.5
 
 LDADD= -lutil
Index: file.c
===
RCS file: /cvs/obsd/src/usr.bin/file/file.c,v
retrieving revision 1.45
diff -u -p -u -p -r1.45 file.c
--- file.c  30 May 2015 06:25:35 -  1.45
+++ file.c  4 Jun 2015 21:01:40 -
@@ -79,6 +79,7 @@ static int read_message(struct imsgbuf 
 static void read_link(struct input_msg *, const char *);
 
 static __dead void child(int, pid_t, int, char **);
+static __dead void solo(int, char **);
 
 static void test_file(struct input_file *, size_t);
 
@@ -188,6 +189,9 @@ main(int argc, char **argv)
if (magicfp == NULL)
err(1, "%s", magicpath);
 
+   if (being_traced())
+   solo(argc, argv);
+
parent = getpid();
if (socketpair(AF_UNIX, SOCK_STREAM, PF_UNSPEC, pair) != 0)
err(1, "socketpair");
@@ -637,4 +641,60 @@ test_file(struct input_file *inf, size_t
 
if (inf->mapped && inf->base != NULL)
munmap(inf->base, inf->size);
+}
+
+static __dead void
+solo(int argc, char *argv[])
+{
+   struct stat  sb;
+   struct input_msg msg;
+   struct input_fileinf;
+   struct magic*m;
+   int  fd, idx;
+   size_t   len, width = 0;
+
+   m = magic_load(magicfp, magicpath, cflag || Wflag);
+   if (cflag) {
+   magic_dump(m);
+   exit(0);
+   }
+
+   for (idx = 0; idx < argc; idx++) {
+   len = strlen(argv[idx]) + 1;
+   if (len > width)
+   width = len;
+   }
+
+   for (idx = 0; idx < argc; idx++) {
+   memset(&msg, 0, sizeof msg);
+   msg.idx = idx;
+
+   if (strcmp(argv[idx], "-") == 0) {
+   if (fstat(STDIN_FILENO, &sb) == -1) {
+   fd = -1;
+   msg.error = errno;
+   } else
+   fd = STDIN_FILENO;
+   } else if (lstat(argv[idx], &sb) == -1) {
+   fd = -1;
+   msg.error = errno;
+   } else {
+   fd = open(argv[idx], O_RDONLY|O_NONBLOCK);
+   if (fd == -1 && (errno == ENFILE || errno == EMFILE))
+   err(1, "open");
+   if (S_ISLNK(sb.st_mode))
+   read_link(&msg, argv[idx]);
+   }
+
+   memset(&inf, 0, sizeof inf);
+   inf.m = m;
+   inf.msg = &msg;
+   inf.path = argv[idx];
+   inf.fd = fd;
+
+   test_file(&inf, width);
+   if (STDIN_FILENO != fd)
+   close(fd);
+   }
+   exit(0);
 }
Index: file.h
===
RCS file: /cvs/obsd/src/usr.bin/file/file.h,v
retrieving revision 1.29
diff -u -p -u -p -r1.29 file.h
--- file.h  27 Apr 2015 13:52:17 -  1.29
+++ file.h  4 Jun 2015 21:01:40 -
@@ -32,4 +32,7 @@ intsandbox_fork(const char *);
 const char *text_get_type(const void *, size_t);
 const char *text_try_words(const void *, size_t, int);
 
+/* proc.c */
+int being_traced(void);
+
 #endif /* FILE_H */
Index: proc.c
=

file(1): is this reasonable?

[patrick keshishian]

Trying to get file to work under systrace(1).

Is this a reasonable patch?

According to systrace(4) ioctl(STRIOCATTACH) can fail for one of
five reasons. In context of file(1), the only possible one is
number "3. It's being traced already".


file(1) still fails with systrace because of sendmsg(2), any
help with correcting systrace.policy for this case would be
appreciated.

--patrick


Index: sandbox.c
===
RCS file: /cvs/obsd/src/usr.bin/file/sandbox.c,v
retrieving revision 1.7
diff -u -p -u -p -r1.7 sandbox.c
--- sandbox.c   29 May 2015 15:58:34 -  1.7
+++ sandbox.c   4 Jun 2015 18:29:55 -
@@ -129,8 +129,11 @@ sandbox_fork(const char *user)
err(1, "ioctl(STRIOCCLONE)");
close(devfd);
 
-   if (ioctl(fd, STRIOCATTACH, &pid) == -1)
-   err(1, "ioctl(STRIOCATTACH)");
+   if (ioctl(fd, STRIOCATTACH, &pid) == -1) {
+   /* Already attached; i.e., running under systrace */
+   warn("ioctl(STRIOCATTACH)");
+   goto out;
+   }
 
memset(&policy, 0, sizeof policy);
policy.strp_op = SYSTR_POLICY_NEW;
@@ -150,7 +153,7 @@ sandbox_fork(const char *user)
err(1, "ioctl(STRIOCPOLICY/MODIFY)");
}
 
-   if (kill(pid, SIGCONT) != 0)
+out:   if (kill(pid, SIGCONT) != 0)
err(1, "kill(SIGCONT)");
return (pid);
 }

systrace patch for chflagsat

[patrick keshishian]

Hi,

With the cp(1) change to use chflagsat(), systrace needs to be
taught about chflagsat syscall. databases/db/v4 port fails with
USE_SYSTRACE=Yes due to a "$(CP) -pr" use during install_docs.

Could someone look over these changes and correct any mistakes.
I am not 100% sure it is correct; specifically, I'm not sure
about the print_atflags.

The extra white-space in register.c diff is to help you eyeball
any obvious (not to me) mistakes; otherwise, the two empty lines
should be removed to maintain uniformity.

Thanks,
--patrick

p.s., I'll send a patch for systrace.filter change to ports@.


Index: register.c
===
RCS file: /cvs/obsd/src/bin/systrace/register.c,v
retrieving revision 1.25
diff -u -p -u -p -r1.25 register.c
--- register.c  16 Jan 2015 00:19:12 -  1.25
+++ register.c  4 Jun 2015 07:43:26 -
@@ -126,6 +126,13 @@ systrace_initcb(void)
X(intercept_register_sccb("native", "chflags", trans_cb, NULL));
intercept_register_transfn("native", "chflags", 0);
intercept_register_translation("native", "chflags", 1, &ic_fileflags);
+
+   X(intercept_register_sccb("native", "chflagsat", trans_cb, NULL));
+   intercept_register_translation("native", "chflagsat", 0, &ic_fdt);
+   intercept_register_transfn("native", "chflagsat", 1);
+   intercept_register_translation("native", "chflagsat", 2, &ic_fileflags);
+   intercept_register_translation("native", "chflagsat", 3, &ic_atflags);
+
X(intercept_register_sccb("native", "readlink", trans_cb, NULL));
tl = intercept_register_translation("native", "readlink", 0,
&ic_translate_unlinkname);
Index: systrace-translate.c
===
RCS file: /cvs/obsd/src/bin/systrace/systrace-translate.c,v
retrieving revision 1.25
diff -u -p -u -p -r1.25 systrace-translate.c
--- systrace-translate.c16 Jan 2015 00:19:12 -  1.25
+++ systrace-translate.c4 Jun 2015 07:43:26 -
@@ -67,6 +67,7 @@ static int print_signame(char *, size_t,
 static int print_fcntlcmd(char *, size_t, struct intercept_translate *);
 static int print_memprot(char *, size_t, struct intercept_translate *);
 static int print_fileflags(char *, size_t, struct intercept_translate *);
+static int print_atflags(char *, size_t, struct intercept_translate *);
 static int get_argv(struct intercept_translate *, int, pid_t, void *);
 static int print_argv(char *, size_t, struct intercept_translate *);
 
@@ -559,6 +560,35 @@ print_fileflags(char *buf, size_t buflen
 }
 
 static int
+print_atflags(char *buf, size_t buflen, struct intercept_translate *tl)
+{
+   int flags = (intptr_t)tl->trans_addr;
+   char lbuf[64];
+
+   *buf = '\0';
+
+   while (flags) {
+   if (*buf)
+   strlcat(buf, "|", buflen);
+
+   if (flags & AT_SYMLINK_NOFOLLOW) {
+   strlcat(buf, "AT_SYMLINK_NOFOLLOW", buflen);
+   flags &= ~AT_SYMLINK_NOFOLLOW;
+   continue;
+   }
+
+   if (flags) {
+   snprintf(lbuf, sizeof(lbuf), "", flags);
+   strlcat(buf, lbuf, buflen);
+   flags = 0;
+   continue;
+   }
+   }
+
+   return (0);
+}
+
+static int
 get_argv(struct intercept_translate *trans, int fd, pid_t pid, void *addr)
 {
char *arg;
@@ -688,4 +718,9 @@ struct intercept_translate ic_linux_memp
 struct intercept_translate ic_fileflags = {
"flags",
NULL, print_fileflags,
+};
+
+struct intercept_translate ic_atflags = {
+   "atflags",
+   NULL, print_atflags,
 };
Index: systrace.h
===
RCS file: /cvs/obsd/src/bin/systrace/systrace.h,v
retrieving revision 1.27
diff -u -p -u -p -r1.27 systrace.h
--- systrace.h  2 Jul 2006 12:34:15 -   1.27
+++ systrace.h  4 Jun 2015 07:43:26 -
@@ -245,6 +245,7 @@ extern struct intercept_translate ic_fcn
 extern struct intercept_translate ic_memprot;
 extern struct intercept_translate ic_linux_memprot;
 extern struct intercept_translate ic_fileflags;
+extern struct intercept_translate ic_atflags;
 
 extern struct intercept_translate ic_linux_oflags;
 

Re: ftp(1) rewrite

[patrick keshishian]

Hi,

quick comments inline.

On 6/1/15, Sunil Nimmagadda  wrote:
> On Thu, May 21, 2015 at 11:16:09PM -0400, Ted Unangst wrote:
>> Sunil Nimmagadda wrote:
>> > Hi,
>> >
>> > The idea is to start with the subset of ftp(1) functionality needed
>> > by pkg_add(1):
>> >
>> > ftp [-o output] url ...
>> >
>> > i.e., should be able to download files over HTTP(S) and FTP.
>> >
>> > This implementation works as FETCH_CMD for pkg_add(1) over HTTP(S).
>> >
>> > FTP is not yet done, but thinking of implementing just PASV and
>> > RETR commands and drop command interpreter compeletely.
>> >
>> > The SMALL variant drops HTTPS and FTP support.
>> >
>> > Comments?
>>
>> screw ftp. just make a new util http, that just does http.
>
> Hi,
>
> Here is a work in progress version 2. Changes from previous diff...
> 1. Drop ftp provision and rename the utility as http(1).
> 2. HTTP redirect handling.
> 3. Proxy support.
> 4. Resume interrupted file transfer.
> 5. Basic and Proxy Authorization.
>
> Todo: progressmeter, cookies, RFC1738 URL encoding/decoding and
> maybe some more.
>
> Comments?
>
[...]
> Index: http.c
> ===
> RCS file: http.c
> diff -N http.c
> --- /dev/null 1 Jan 1970 00:00:00 -
> +++ http.c1 Jun 2015 09:02:59 -
[...]
> +int
> +proxy_connect(int s, struct url *url, struct url *proxy)
> +{
> + FILE*fp;
> + char hostname[HOST_NAME_MAX+1], *buf;
> + const char  *proxy_auth = NULL;
> + size_t   len;
> + int  code;
> +
> + if ((fp = fdopen(s, "r+")) == NULL)
> + err(1, "http_connect: fdopen");
> +
> + if (gethostname(hostname, sizeof(hostname)) != 0)
> + hostname[0] = '\0';

Pretty sure the "Host:" header should indicate the remote host,
not local.

> + if (proxy->user[0] || proxy->pass[0])
> + proxy_auth = base64_encode(proxy->user, proxy->pass);
> +
> + fprintf(fp,
> + "CONNECT %s:%s HTTP/1.0\r\n"
> + "Host: %s\r\n"
> + "User-Agent: %s\r\n"
> + "%s%s"
> + "\r\n",
> + url->host,
> + (url->port[0]) ? url->port : "80",
> + hostname,
> + ua,
> + (proxy_auth) ? "Proxy-Authorization: Basic " : "",
> + (proxy_auth) ? proxy_auth : "");
> +
> + fflush(fp);
> + if ((buf = http_getline(fp, &len)) == NULL)
> + return (-1);
> +
> + if ((code = http_response_code(buf)) == -1) {
> + free(buf);
> + return (-1);
> + }
> +
> + free(buf);
> + if (code != 200)
> + errx(1, "Error retrieving file: %s", errstr(code));
> +
> + return (0);
> +}
> +
> +const char *
> +base64_encode(const char *user, const char *pass)
> +{
> + static char  basic_auth[BUFSIZ];
> + char*creds, b64_creds[BUFSIZ];
> + int  ret;
> +
> + if ((ret = asprintf(&creds, "%s:%s", user, pass)) == -1)
> + errx(1, "base64_encode: asprintf failed");
> +
> + if (b64_ntop(creds, ret, b64_creds, sizeof(b64_creds)) == -1)
> + errx(1, "error in base64 encoding");
> +
> + free(creds);
> + ret = snprintf(basic_auth, sizeof(basic_auth), "%s\r\n", b64_creds);
> + if (ret == -1 || ret > sizeof(basic_auth))
> + errx(1, "base64_encode: basic_auth overflow");
> +
> + return (basic_auth);
> +}
> +
> +int
> +http_get(int s, struct url *url, const char *fn, int resume,
> +struct headers *hdrs)
> +{
> + struct stat  sb;
> + char hostname[HOST_NAME_MAX+1], range[BUFSIZ];
> + const char  *basic_auth = NULL;
> + FILE*fin;
> + char*buf;
> + size_t   len;
> + int  fd, flags, res = -1;
> +
> + if ((fin = fdopen(s, "r+")) == NULL)
> + err(1, "http_get: fdopen");
> +
> + if (gethostname(hostname, sizeof(hostname)) != 0)
> + hostname[0] = '\0';

Same here.

But afaik HTTP/1.0 doesn't require that header.

--patrick


> + if (stat(fn, &sb) == -1)
> + resume = 0;
> + else {
> + snprintf(range, sizeof(range),
> + "Range: bytes=%lld-\r\n", sb.st_size);
> + }
> +
> + if (url->user[0] || url->pass[0])
> + basic_auth = base64_encode(url->user, url->pass);
> +
> + fprintf(fin,
> + "GET %s HTTP/1.0\r\n"
> + "Host: %s\r\n"
> + "User-Agent: %s\r\n"
> + "%s"
> + "%s%s"
> + "\r\n",
> + (url->path[0]) ? url->path : "/",
> + hostname,
> + ua,
> + (resume) ? range : "",
> + (basic_auth) ? "Authorization: Basic " : "",
> + (basic_auth) ? basic_auth : "");
[...]

Re: [patch 3/3] ksh: add overflow checking for memory allocations

[patrick keshishian]

On 5/24/15, Theo Buehler  wrote:
>
>
> On Sun, May 24, 2015 at 09:10:08PM +0200, Martin Natano wrote:
>> I highly doubt any part of those three diffs is authored by yourself.
>> (Merely renaming functions doesn't count as original work.) As pointed
>> out before, the efficient overflow checking code is copied from Otto's
>> code and the rest has been written by me. In case you don't remember the
>> location where you found those diffs, here it is:
>> https://github.com/bitrig/bitrig/commit/6b917fe675fc8a81d35ae383675da0ec0161965c
>> https://github.com/bitrig/bitrig/commit/a355e613ee35a4c678f2a059781682d6998d9a01
>>
>> The BSD/ISC style licenses actively encourage code reuse, but still the
>> original authors have to be given credit. I'm startled that has to be
>> explained!

So, wait, any if() statement matching the pattern:

if (a > 0 && c / a < b) { ... }

should cite above work?
or is the really clever bit begging citation, the part where that
check is wrapped in a function?

Otto's sqrt() optimization added to the if() statement is
something (i think) original and clever, and should be cited.
but the simple if pattern above ... that's really reaching.

--patrick



>>
>> natano
>
> Dear Martin
>
> I believe you are exaggerating.  Yes, there is quite a bit of overlap
> between the patches you linked to and the ones I sent, but I honestly
> only saw your work after checking your links.
>
> I immediately acknowledged that the overflow checking was based on
> Otto's code (but it wasn't copied).  But you are going a step further
> and you are accusing me of plagiarism, which I am not ready to admit,
> because I didn't do it.
>
> The first of your links has nothing to do with what I mailed besides
> being a related change to the same file and the second is mostly a more
> or less mechanical expansion (and you removed a number of casts, which I
> didn't do).
>
> I am sorry to have offended you, and I merely tried to help improve the
> code base, that's all.  I never intended to sell any work of others as
> my own.
>
> Cheers,
>
> Theo
>
>

Re: /etc/daily /tmp purge mods; skip open files with fstat test

[patrick keshishian]

On 5/14/15, Craig Skinner  wrote:
[...]
> Here's a diff of a modified /etc/daily /tmp purge portion:
>
>  o replace test(1) '-L' with '-h' due to:
>-L ... "Do not rely on its existence; use -h instead"

Interesting that FreeBSD[1] and MacOS X say the opposite.
SUSv4 (one I have handy) has the exact same text for both
options:

True if pathname resolves to an existing
directory entry for a symbolic link. False if
pathname cannot be resolved, or if pathname
resolves to an existing directory entry for a
file that is not a symbolic link. If the final
component of pathname is a symbolic link, that
symbolic link is not followed

--patrick

[1] 
https://www.freebsd.org/cgi/man.cgi?query=test&apropos=0&sektion=0&manpath=FreeBSD+10.1-RELEASE&arch=default&format=html


>  o don't cd nor find(1) execdir, rather full path find.
>  o file find stage;-
>o read found & skip directories for rm(1),
>  check found item isn't open with fstat.
>o securely random pattern overwrite stale files.
>  o directory find stage;-
>o find only empty directories for rmdir(1).
>o 5 day stale directories.
>  o similarily order ignores of .X11-unix, .ICE-unix & portslocks.
>  o also purge stale;-
>o pipes.
>o sockets.
>o dangling symlinks.
>
>
>
> Index: daily
> ===
> RCS file: /cvs/src/etc/daily,v
> retrieving revision 1.83
> diff -u -p -r1.83 daily
> --- daily 29 Apr 2015 00:10:44 -  1.83
> +++ daily 14 May 2015 15:53:00 -
> @@ -45,16 +45,32 @@ start_part "Running daily.local:"
>  run_script "daily.local"
>
>  next_part "Removing scratch and junk files:"
> -if [ -d /tmp -a ! -L /tmp ]; then
> - cd /tmp && {
> - find -x . \
> - \( -path './ssh-*' -o -path ./.X11-unix -o -path ./.ICE-unix \
> - -o -path ./portslocks -o -path './tmux-*' \) \
> - -prune -o -type f -atime +7 -execdir rm -f -- {} \; 2>/dev/null
> - find -x . -type d -mtime +1 ! -path ./vi.recover ! -path ./.X11-unix \
> - ! -path ./.ICE-unix ! -path ./portslocks ! -name . \
> - -execdir rmdir -- {} \; >/dev/null 2>&1; }
> -fi
> +[[ -d /tmp && ! -h /tmp ]] &&
> +{
> + # stale files, not still held open:
> + find -x /tmp \( -path '/tmp/ssh-*' -o -path '/tmp/tmux-*' \
> + -o -path /tmp/.X11-unix -o -path /tmp/.ICE-unix \
> + -o -path /tmp/portslocks \) -prune \
> + -o -type f -atime +7 | while read found
> + do
> + [[ -d ${found} ]] && continue
> + fstat ${found} | grep -q ${found}$ || rm -P -- ${found}
> + done
> +
> + # stale dangling symlinks:
> + find -Lx /tmp -type l -ctime +14 \
> + -exec rm -- {} \;
> +
> + # stale pipes & sockets:
> + find -x /tmp \( -type p -o -type s \) -ctime +40 \
> + -exec rm -- {} \;
> +
> + # stale directories:
> + find -x /tmp -type d -mtime +5 -empty \
> + ! -name /tmp ! -path /tmp/vi.recover \
> + ! -path /tmp/.X11-unix ! -path /tmp/.ICE-unix \
> + ! -path /tmp/portslocks -exec rmdir -- {} \;
> +}
>
>  # Additional junk directory cleanup would go like this:
>  #if [ -d /scratch -a ! -L /scratch ]; then
>
>
> Would it be a good idea to move the /scratch example out of the
> /etc/daily script, and into daily(8), as an example for daily.local?
>
>

Re: [patch] rtl8188eu support for urtwn(4)

[patrick keshishian]

On 4/26/15, Mikhail  wrote:
> On 21:22 26-Apr 2015 Mikhail wrote:
>> On 20:20 26-Apr 2015 Stefan Sperling wrote:
>> > On Sun, Apr 26, 2015 at 01:31:17PM +0200, Stefan Sperling wrote:
>> > > On Sun, Apr 19, 2015 at 11:48:32PM +0300, Mikhail wrote:
>> > > > Bellow new version of the patch with above things fixed, also I've
>> > > > fixed
>> > > > detection of ETV chip in urtwn_attach(), nothing else is changed.
>> > >
>> > > I'm seeing very low data transmission rates with your patch and a
>> > > TP-Link TL-WN725N device. In both 11b and 11g mode, the data rate
>> > > remains very low (less than 100Kbit/s). A different urtwn(4) device
>> > > (with 8188CUS chip) has much better throughput.
>> > >
>> > > Are you seeing this, too?
>> >
>> > The chunk below is wrong for OpenBSD since it sets the intitial
>> > transmit
>> > rate to an 11n rate. 0x13 corresponds to the "MCS7" 11n rate,
>> > see linux/drivers/net/wireless/rtlwifi/rtl8188ee/def.h enum
>> > rtl_desc92c_rate.
>> > The value 11 corresponds to OFDM 54Mbit which is fine for OpenBSD.
>> > We only support 11a/b/g at present.
>> >
>> > --- sys/dev/usb/if_urtwn.c 14 Mar 2015 03:38:49 -  1.43
>> > +++ sys/dev/usb/if_urtwn.c 19 Apr 2015 20:27:41 -
>> > @@ -1813,7 +2011,10 @@ urtwn_tx(struct urtwn_softc *sc, struct
>> >txd->txdw4 |= htole32(SM(R92C_TXDW4_RTSRATE, 8));
>> >txd->txdw5 |= htole32(0x0001ff00);
>> >/* Send data at OFDM54. */
>> > -  txd->txdw5 |= htole32(SM(R92C_TXDW5_DATARATE, 11));
>> > +  if (sc->chip & URTWN_CHIP_88E)
>> > +  txd->txdw5 |= htole32(0x13 & 0x3f);
>> > +  else
>> > +  txd->txdw5 |= htole32(SM(R92C_TXDW5_DATARATE, 11));
>> >
>> >} else {
>> >txd->txdw1 |= htole32(
>> >
>> > Reverting this change doesn't fix the transmit speed problem,
>> > unfortunately.
>> >
>> > I wonder if we're making some mistake while setting up the TX
>> > descriptor?
>> > There are several differences in the TX descriptors of 88E vs 92C.
>> > For example, 88E has third antenna C available.
>>
>> Hello, in urtwn_init(), in part:
>>
>> if (sc->chip & URTWN_CHIP_88E)
>> urtwn_write_1(sc, R92C_RXDMA_AGG_PG_TH + 1, 4);
>> else
>> urtwn_write_1(sc, R92C_USB_DMA_AGG_TO, 4);
>>
>> comment first 3 lines. It fixes speed problem for me, but I need to
>> understand the issue further, before proposing any solution.
>
> The issue isn't in those lines, but a little bit higher - the driver
> enables usb and dma aggregations, but native linux implementation
> enables only dma and disables usb one.
>
> I've submitted bug report to FreeBSD[1] with a patch, which I see as a
> proper solution:
>
> urtwn_write_1(sc, R92C_USB_SPECIAL_OPTION,
> urtwn_read_1(sc, R92C_USB_SPECIAL_OPTION) |
> -   R92C_USB_SPECIAL_OPTION_AGG_EN);
> +   (sc->chip & URTWN_CHIP_88E ?
> +   ~R92C_USB_SPECIAL_OPTION_AGG_EN :

umm... maybe 0, as it is being ORed with what read returns.
But maybe the check should be against URTWN_CHIP_92C since
the option is specific to it (or at least implied to be so).

--patrick

> +R92C_USB_SPECIAL_OPTION_AGG_EN));
>
> I'd suggest to wait some time for feedback. Testing is welcome, of
> course. Thank you for pointing this out and review in general.
>
> [1] - https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=199718
>
>

Re: Do you need/prefer the non-DUID option in the installer?

[patrick keshishian]

On 3/15/15, Michael W. Lucas  wrote:
> On Sun, Mar 15, 2015 at 01:06:37PM -0600, Theo de Raadt wrote:
>> Look, if people keep being unspecific on how DUIDs interfere with
>> their usage patterns, then the non-DUID configuration mode is going
>> to go away.
>>
>> WHY must be use the non-DUID option in the installer??!?!?!
>
> As someone who recently had several OpenBSD boxes in production, in a
> variety of roles:
>
> I can't imagine why DUIDs wouldn't work.
>
> We defaulted to DUIDs the moment they became available. They worked
> fine. Even for the Linux guys.
>
> If someone has a particular dislike of DUIDs, they can easily change
> them back. Anyone who has a whole bunch of OpenBSD boxes probably has
> uses a post-install script, and a couple lines of
> sed/awk/perl/whatever will make them happy.

The ridiculousness of this point is beyond ... beyond ... well,
it is silly.

The installation script right now, as everyone is used to, asks
a simple question. With a "yes" (the default) or a "no" answer
everyone's preference and need are met.

You are arguing to make more work -- which certainly means
more time, more effort and less convenience, with potential
introduction of errors --  for a single group of people, just so
the other group isn't bothered to press the enter key to accept
the default "yes" answer during installation?

Is this the problem you are trying to solve? One less press
of the enter key for you?

Are you serious?!

--patrick


> If you have one OpenBSD box, and you just don't like DUIDs, again,
> it's really easy to revert.
>
> ==ml
>
> PS: Yes, I still have OpenBSD hosts. But I'm no longer a pro sysadmin,
> so I can't claim to be running a server farm or anything like that.
>
> --
> Michael W. Lucas  -  mwlu...@michaelwlucas.com, Twitter @mwlauthor
> http://www.MichaelWLucas.com/, http://blather.MichaelWLucas.com/
>
>

Re: Do you need/prefer the non-DUID option in the installer?

[patrick keshishian]

On Sun, Mar 15, 2015 at 11:24:32AM -0400, Kenneth Westerback wrote:
> Using DUIDs in the installed /etc/fstab has been the default for some time 
> now.
> 
> We'd like to eliminate the question in the installer and just use
> DUIDs unconditionally.
> 
> But first we need to know you are aware of any circumstances where
> people need or prefer to use the non-DUID option when installing?

I prefer not using DUIDs.

--patrick

Re: From lines and mail.local

[patrick keshishian]

On Fri, Feb 06, 2015 at 03:09:57PM +, Martin Brandenburg wrote:
> When mail comes in for local delivery with two consecutive lines
> beginning 'From ' only the first is escaped with a >. Two non-
> consecutive 'From ' lines in one message does not cause this.
> 
> Connect to the local mail server and send
> > From blah blah
> > From blah blah
> 
> And you see in /var/mail/...
> > >From blah blah
> > From blah blah
> 
> Needless to say, this confuses practically everything handling mbox
> files.
> 
> I'm not sure what eline was supposed to accomplish below, but it doesn't
> seem to help since all 'From ' lines need to be escaped.

Are you certain about this? My understanding is a new message
in the mbox is indicated by a blank line followed by "From ".
Therefore each "From " that appears in the message, with a
preceeding blank line gets ">" prepended (by convention).

also: http://en.wikipedia.org/wiki/Mbox

--patrick


> -- Martin
> 
> Index: mail.local.c
> ===
> RCS file: /cvs/src/libexec/mail.local/mail.local.c,v
> retrieving revision 1.33
> diff -u -p -r1.33 mail.local.c
> --- mail.local.c  16 Jan 2015 06:39:50 -  1.33
> +++ mail.local.c  6 Feb 2015 15:00:13 -
> @@ -117,7 +117,7 @@ storemail(char *from)
>  {
>   FILE *fp = NULL;
>   time_t tval;
> - int fd, eline;
> + int fd;
>   size_t len;
>   char *line, *tbuf;
>  
> @@ -131,7 +131,7 @@ storemail(char *from)
>   (void)time(&tval);
>   (void)fprintf(fp, "From %s %s", from, ctime(&tval));
>  
> - for (eline = 1, tbuf = NULL; (line = fgetln(stdin, &len));) {
> + for (tbuf = NULL; (line = fgetln(stdin, &len));) {
>   /* We have to NUL-terminate the line since fgetln does not */
>   if (line[len - 1] == '\n')
>   line[len - 1] = '\0';
> @@ -143,14 +143,8 @@ storemail(char *from)
>   tbuf[len] = '\0';
>   line = tbuf;
>   }
> - if (line[0] == '\0')
> - eline = 1;
> - else {
> - if (eline && line[0] == 'F' && len > 5 &&
> - !memcmp(line, "From ", 5))
> - (void)putc('>', fp);
> - eline = 0;
> - }
> + if (line[0] == 'F' && len > 5 && !memcmp(line, "From ", 5))
> + (void)putc('>', fp);
>   (void)fprintf(fp, "%s\n", line);
>   if (ferror(fp))
>   break;
> 

[patch] xdm doesn't clear wtmp/utmp entry after log out

[patrick keshishian]

Hi,

I've noticed this issue for sometime now. I was hoping one
of the Xorg updates would fix it, so finally I decided to
read some man pages, specifically sessreg.

Evidently the Xservers path is not correct (or has changed)
in OBSD's install. No longer found at:

/usr/X11R6/lib/X11/xdm/Xservers
is at:
/etc/X11/xdm/Xservers

[pre-patch]
Log into X using xdm, then log out. Alt+Ctrl+F1 login and
run last as such:
$ last -5
sidster   : Tue Jan 13 12:01   still logged in
sidster   ttyC0 Sat Jan 10 16:35   still logged in
reboot~ Sat Jan 10 16:35
shutdown  ~ Sat Jan 10 16:34
root  ttyC0 Sat Jan 10 16:28 - shutdown  (00:06)

[post-patch below (loally modified files)]
Same procedure as above:
$ last -5
sidster   : Tue Jan 13 13:23 - 13:24  (00:00)
sidster   : Tue Jan 13 12:01 - 13:23  (01:22)
sidster   ttyC0 Sat Jan 10 16:35   still logged in
reboot~ Sat Jan 10 16:35
shutdown  ~ Sat Jan 10 16:34

This is on:
kern.version=OpenBSD 5.7-beta (GENERIC) #701: Sat Jan 10 07:52:06 MST 2015
dera...@amd64.openbsd.org:/usr/src/sys/arch/amd64/compile/GENERIC


Index: GiveConsole
===
RCS file: /cvs/obsd/xenocara/app/xdm/config/GiveConsole,v
retrieving revision 1.4
diff -u -p -u -p -r1.4 GiveConsole
--- GiveConsole 18 Nov 2013 20:39:48 -  1.4
+++ GiveConsole 13 Jan 2015 21:26:24 -
@@ -11,4 +11,4 @@ chown $USER /dev/console
 if [ -c /dev/drm0 ]; then
 chown $USER /dev/drm0
 fi
-/usr/X11R6/bin/sessreg -a -l $DISPLAY -u none -x 
/usr/X11R6/lib/X11/xdm/Xservers $USER
+/usr/X11R6/bin/sessreg -a -l $DISPLAY -u none -x /etc/X11/xdm/Xservers $USER
Index: TakeConsole
===
RCS file: /cvs/obsd/xenocara/app/xdm/config/TakeConsole,v
retrieving revision 1.4
diff -u -p -u -p -r1.4 TakeConsole
--- TakeConsole 18 Nov 2013 20:39:48 -  1.4
+++ TakeConsole 13 Jan 2015 21:26:24 -
@@ -8,4 +8,4 @@ chown root /dev/console
 if [ -c /dev/drm0 ]; then
 chown root /dev/drm0
 fi
-/usr/X11R6/bin/sessreg -d -l $DISPLAY -u /var/run/utmp -x 
/usr/X11R6/lib/X11/xdm/Xservers $USER
+/usr/X11R6/bin/sessreg -d -l $DISPLAY -u /var/run/utmp -x 
/etc/X11/xdm/Xservers $USER

Re: chmod: range checks

[patrick keshishian]

On Fri, Dec 12, 2014 at 10:45:04AM -0700, Todd C. Miller wrote:
> On Fri, 12 Dec 2014 18:16:35 +0100, Tobias Stoeckmann wrote:
> 
> > chmod doesn't check if the program name is at least 3 characters long
> > before checking its index 2.
> 
> OK.

Just throwing this out there: will this program ever get
installed with filename shorter than ch{grp,mod,own,flags}?

--patrick


> > Also, there is a compiler warning about signed vs unsigned when "val"
> > is used.  In one instance, it's used with strtoul, in another with strtol,
> > checking its ranges.  It's okay due to automatic casting but definitely
> > no clean code.
> 
> OK.  This is still a comparision between signed and unsigned but
> since INT_MAX is a constant the compiler can tell that it is safe.
> 
>  - todd
> 

Re: Violating randomization standards

[patrick keshishian]

Hello,

Small comment below.

On Mon, Dec 08, 2014 at 01:55:47PM -0700, Theo de Raadt wrote:
> I have spent the last week researching all the uses of the srand(),
> srandom(), and srand48() subsystems in the ports tree.
[...]
> RAND(3)Library Functions ManualRAND(3)
> 
> NAME
>  rand, rand_r, srand, srand_deterministic - bad pseudo-random number
>  generator
> 
> SYNOPSIS
>  #include 
> 
>  void
>  srand(unsigned int seed);
> 
>  void
>  srand_deterministic(unsigned int seed);
> 
>  int
>  rand(void);
> 
>  int
>  rand_r(unsigned int *seed);
> 
> DESCRIPTION
>  Standards insist that this interface return deterministic results.
>  Unsafe usage is very common, so OpenBSD changed the subsystem to return
>  non-deterministic results by default.
> 
>  To satisfy portable code, srand() may be called to initialize the
>  subsystem.  In OpenBSD the seed variable is ignored, and strong random
>  number results will be provided from arc4random(3.) In other systems, the
>  seed variable primes a simplistic deterministic algorithm.
> 
>  If the standardized behavior is required srand_deterministic() can be
>  substituted for srand(), then subsequent rand() calls will return results
>  using the deterministic algorithm.
> 
>  The rand() function returns a result in the range of 0 to RAND_MAX.  By
>  default, this result comes from arc4random(3).  If srand_deterministic()
>  was called, the result will be computed using the deterministic
>  algorithm.
> 
>  The rand_r() is a thread-safe version of rand().  Storage for the seed
>  must be provided through the seed argument, and needs to have been
>  initialized by the caller.  It always operates using the deterministic
>  algorithm.
> 
> SEE ALSO
>  arc4random(3), rand48(3), random(3)
> 
> STANDARDS
>  The rand() function conforms to ANSI X3.159-1989 (``ANSI C89'').
> 
>  The rand_r() function conforms to IEEE Std 1003.1-2008 (``POSIX.1'').
> 
>  The srand() function does not conform to ANSI X3.159-1989 (``ANSI C89''),
>  intentionally.
> 
>  The srand_deterministic() function is an OpenBSD extension.
> 
> HISTORY
>  The functions rand() and srand() first appeared in Version 3 AT&T UNIX.
> 
> OpenBSD 5.6November 25, 2014   OpenBSD 5.6
[...]
> Index: lib/libc/stdlib/rand.c
> ===
> RCS file: /cvs/src/lib/libc/stdlib/rand.c,v
> retrieving revision 1.10
> diff -u -p -u -r1.10 rand.c
> --- lib/libc/stdlib/rand.c1 Aug 2013 19:42:08 -   1.10
> +++ lib/libc/stdlib/rand.c8 Dec 2014 03:50:34 -
> @@ -30,6 +30,7 @@
>  #include 
>  #include 
>  
> +static int rand_deterministic;
>  static u_int next = 1;
>  
>  int
> @@ -47,6 +48,8 @@ __warn_references(rand_r,
>  int
>  rand(void)
>  {
> + if (rand_deterministic)
> + return (arc4random() % ((u_int)RAND_MAX + 1));

I think, based on man page change you sent, the if()
statement should check for '0 == rand_deterministic'
No?

--patrick


>   return (rand_r(&next));
>  }
>  
> @@ -58,10 +61,12 @@ __warn_references(rand,
>  void
>  srand(u_int seed)
>  {
> - next = seed;
> + rand_deterministic = 0;
>  }
>  
> -#if defined(APIWARN)
> -__warn_references(srand,
> -"warning: srand() seed choices are invariably poor");
> -#endif
> +void
> +srand_deterministic(u_int seed)
> +{
> + rand_deterministic = 1;
> + next = seed;
> +}

Re: httpd errata

[patrick keshishian]

Hi,

On Thu, Nov 20, 2014 at 06:24:23PM -0500, Ted Unangst wrote:
> [on behalf of reyk]
> 
> Many people want to test the new httpd in OpenBSD 5.6; so we decided
> to provide various improvements from -current for 5.6.
> See the description below for more details.

[...]

> Index: usr.sbin/httpd/httpd.c
> ===
> RCS file: /cvs/src/usr.sbin/httpd/httpd.c,v
> retrieving revision 1.17
> diff -u -p -r1.17 httpd.c
> --- usr.sbin/httpd/httpd.c5 Aug 2014 15:36:59 -   1.17
> +++ usr.sbin/httpd/httpd.c18 Nov 2014 15:02:54 -

[...]

> @@ -623,6 +678,40 @@ socket_rlimit(int maxfd)
>   rl.rlim_cur = MAX(rl.rlim_max, (rlim_t)maxfd);
>   if (setrlimit(RLIMIT_NOFILE, &rl) == -1)
>   fatal("socket_rlimit: failed to set resource limit");
> +}
> +
> +char *
> +evbuffer_getline(struct evbuffer *evb)
> +{
> + u_int8_t*ptr = EVBUFFER_DATA(evb);
> + size_t   len = EVBUFFER_LENGTH(evb);
> + char*str;
> + u_inti;
> +
> + /* Safe version of evbuffer_readline() */
> + if ((str = get_string(ptr, len)) == NULL)
> + return (NULL);
> +
> + for (i = 0; str[i] != '\0'; i++) {
> + if (str[i] == '\r' || str[i] == '\n')
> + break;
> + }
> +
> + if (i == len) {
> + free(str);
> + return (NULL);
> + }
> +
> + str[i] = '\0';
> +
> + if ((i + 1) < len) {
> + if (ptr[i] == '\r' && ptr[i + 1] == '\n')
> + i++;
> + }

any concern over 'u_int i' vs 'size_t len' type-mismatch?

--patrick

Re: locate - change sizeof(char **) to sizeof(char *)

[patrick keshishian]

On Sat, Nov 15, 2014 at 10:12:17AM -0800, patrick keshishian wrote:
> On Sat, Nov 15, 2014 at 06:42:34PM +0100, Nicolas Bedos wrote:
> > In usr.bin/locate/locate/locate.c and util.c the variable dbv is defined
> > as a pointer to char * and is used to access the path to every database
> > provided to locate. E.g. when running
> > 
> > locate -d /path/to/db1 -d /path/to/db2 -d /path/to/db3 *
> > 
> > *dbv points to '/path/to/db1'
> > *(dbv+1) points to '/path/to/db2'
> > *(dbv+2) points to '/path/to/db3'
> > 
> > However dbv is initialized with the following call
> > 
> > dbv = malloc(sizeof(char **))
> > 
> > which I believe should be 
> > 
> > dbv = malloc(sizeof(char *))
> 
> No. That's not how the code reads. dbv is an array of
> "char *"-s, not a "char *". For each element in "path"
> new "char *" of "slen + 1" is allocated, and "dbv" is
> grown and the newly allocated string (char *) is stored.

Crap. I'm looking an a stale source before reallocarray()
change. Sorry for the noise.


> 
> Best,
> --patrick
> 
> 
> > The same goes for the variable newdbv. Please find the corresponding
> > diff below. No binary change on amd64 since sizeof(char **) ==
> > sizeof(char *). 
> > 
> > Nicolas Bedos
> > 
> > 
> > 
> > Index: usr.bin/locate/locate/util.c
> > ===
> > RCS file: /cvs/src/usr.bin/locate/locate/util.c,v
> > retrieving revision 1.11
> > diff -u -p -u -r1.11 util.c
> > --- usr.bin/locate/locate/util.c8 Oct 2014 04:04:37 -   1.11
> > +++ usr.bin/locate/locate/util.c15 Nov 2014 14:08:15 -
> > @@ -89,7 +89,7 @@ colon(dbv, path, dot)
> > char **pv;
> >  
> > if (dbv == NULL) {
> > -   if ((dbv = malloc(sizeof(char **))) == NULL)
> > +   if ((dbv = malloc(sizeof(char *))) == NULL)
> > err(1, "malloc");
> > *dbv = NULL;
> > }
> > @@ -123,7 +123,7 @@ colon(dbv, path, dot)
> > }
> > /* increase dbv with element p */
> > if ((newdbv = reallocarray(dbv, vlen + 2,
> > -   sizeof(char **))) == NULL)
> > +   sizeof(char *))) == NULL)
> > err(1, "realloc");
> > dbv = newdbv;
> > *(dbv + vlen) = p;
> > 
> 

Re: locate - change sizeof(char **) to sizeof(char *)

[patrick keshishian]

On Sat, Nov 15, 2014 at 06:42:34PM +0100, Nicolas Bedos wrote:
> In usr.bin/locate/locate/locate.c and util.c the variable dbv is defined
> as a pointer to char * and is used to access the path to every database
> provided to locate. E.g. when running
> 
> locate -d /path/to/db1 -d /path/to/db2 -d /path/to/db3 *
> 
> *dbv points to '/path/to/db1'
> *(dbv+1) points to '/path/to/db2'
> *(dbv+2) points to '/path/to/db3'
> 
> However dbv is initialized with the following call
> 
> dbv = malloc(sizeof(char **))
> 
> which I believe should be 
> 
> dbv = malloc(sizeof(char *))

No. That's not how the code reads. dbv is an array of
"char *"-s, not a "char *". For each element in "path"
new "char *" of "slen + 1" is allocated, and "dbv" is
grown and the newly allocated string (char *) is stored.

Best,
--patrick


> The same goes for the variable newdbv. Please find the corresponding
> diff below. No binary change on amd64 since sizeof(char **) ==
> sizeof(char *). 
> 
> Nicolas Bedos
> 
> 
> 
> Index: usr.bin/locate/locate/util.c
> ===
> RCS file: /cvs/src/usr.bin/locate/locate/util.c,v
> retrieving revision 1.11
> diff -u -p -u -r1.11 util.c
> --- usr.bin/locate/locate/util.c  8 Oct 2014 04:04:37 -   1.11
> +++ usr.bin/locate/locate/util.c  15 Nov 2014 14:08:15 -
> @@ -89,7 +89,7 @@ colon(dbv, path, dot)
>   char **pv;
>  
>   if (dbv == NULL) {
> - if ((dbv = malloc(sizeof(char **))) == NULL)
> + if ((dbv = malloc(sizeof(char *))) == NULL)
>   err(1, "malloc");
>   *dbv = NULL;
>   }
> @@ -123,7 +123,7 @@ colon(dbv, path, dot)
>   }
>   /* increase dbv with element p */
>   if ((newdbv = reallocarray(dbv, vlen + 2,
> - sizeof(char **))) == NULL)
> + sizeof(char *))) == NULL)
>   err(1, "realloc");
>   dbv = newdbv;
>   *(dbv + vlen) = p;
> 

Re: tetris(6): fix select() -> poll() conversion

[patrick keshishian]

On Wed, Nov 05, 2014 at 02:44:54PM -0800, patrick keshishian wrote:
> On Wed, Nov 05, 2014 at 08:45:07PM +0100, Theo Buehler wrote:
> > Pausing a tetris game currently causes a segfault due to a a null
> > pointer dereference.
> > 
> > Fix this by checking that s is non-NULL before accessing its members.
> > 
> > A number of comments and an error message still refer to select()
> > instead of poll(). Correct this as well.
> > 
> > 
> > Index: input.c
> > ===
> > RCS file: /cvs/src/games/tetris/input.c,v
> > retrieving revision 1.13
> > diff -u -p -r1.13 input.c
> > --- input.c 3 Nov 2014 22:14:54 -   1.13
> > +++ input.c 5 Nov 2014 19:39:30 -
> > @@ -64,12 +64,12 @@
> > }
> >  
> >  /*
> > - * Do a `read wait': select for reading from stdin, with timeout *tvp.
> > + * Do a `read wait': poll for reading from stdin, with timeout *tvp.
> >   * On return, modify *tvp to reflect the amount of time spent waiting.
> >   * It will be positive only if input appeared before the time ran out;
> >   * otherwise it will be zero or perhaps negative.
> >   *
> > - * If tvp is nil, wait forever, but return if select is interrupted.
> > + * If tvp is nil, wait forever, but return if poll is interrupted.
> >   *
> >   * Return 0 => no input, 1 => can read() from stdin
> >   */
> > @@ -90,14 +90,15 @@ rwait(struct timeval *tvp)
> >  again:
> > pfd[0].fd = STDIN_FILENO;
> > pfd[0].events = POLLIN;
> > -   switch (poll(pfd, 1, s->tv_sec * 1000 + s->tv_usec / 1000)) {
> > +   switch (poll(pfd, 1, s ? s->tv_sec * 1000 + s->tv_usec / 1000 :
> 
> 
> I propose getting rid of the s pointer all together as such:

After replacing select() with poll(), not removing
`struct timeval *s' seems an oversight; Its use was
solely for select()'s benefit.

Once more, proposing removal of `struct timeval *s' and
using an `int timo' for the time-out value, that gets fed
into poll(). It also improves readability of the code;
rumors floating around that that is a good thing.

Index: input.c
===
RCS file: /cvs/obsd/src/games/tetris/input.c,v
retrieving revision 1.14
diff -u -p -u -r1.14 input.c
--- input.c 5 Nov 2014 20:23:38 -   1.14
+++ input.c 7 Nov 2014 19:37:07 -
@@ -76,7 +76,8 @@
 int
 rwait(struct timeval *tvp)
 {
-   struct timeval starttv, endtv, *s;
+   int timo = INFTIM;
+   struct timeval starttv, endtv;
struct pollfd pfd[1];
 
 #defineNILTZ ((struct timezone *)0)
@@ -84,15 +85,12 @@ rwait(struct timeval *tvp)
if (tvp) {
(void) gettimeofday(&starttv, NILTZ);
endtv = *tvp;
-   s = &endtv;
-   } else
-   s = NULL;
+   timo = endtv.tv_sec * 1000 + endtv.tv_usec / 1000;
+   }
 again:
pfd[0].fd = STDIN_FILENO;
pfd[0].events = POLLIN;
-   switch (poll(pfd, 1, s ? s->tv_sec * 1000 + s->tv_usec / 1000 :
-   INFTIM)) {
-
+   switch (poll(pfd, 1, timo)) {
case -1:
if (tvp == 0)
return (-1);


> 
> Index: input.c
> ===
> RCS file: /cvs/obsd/src/games/tetris/input.c,v
> retrieving revision 1.13
> diff -u -p -u -p -r1.13 input.c
> --- input.c   3 Nov 2014 22:14:54 -   1.13
> +++ input.c   5 Nov 2014 22:40:47 -
> @@ -64,19 +64,20 @@
>   }
>  
>  /*
> - * Do a `read wait': select for reading from stdin, with timeout *tvp.
> + * Do a `read wait': poll for reading from stdin, with timeout *tvp.
>   * On return, modify *tvp to reflect the amount of time spent waiting.
>   * It will be positive only if input appeared before the time ran out;
>   * otherwise it will be zero or perhaps negative.
>   *
> - * If tvp is nil, wait forever, but return if select is interrupted.
> + * If tvp is nil, wait forever, but return if poll is interrupted.
>   *
>   * Return 0 => no input, 1 => can read() from stdin
>   */
>  int
>  rwait(struct timeval *tvp)
>  {
> - struct timeval starttv, endtv, *s;
> + int timo = INFTIM;
> + struct timeval starttv, endtv;
>   struct pollfd pfd[1];
>  
>  #define  NILTZ ((struct timezone *)0)
> @@ -84,20 +85,19 @@ rwait(struct timeval *tvp)
>   if (tvp) {
>   (void) gettimeofday(&starttv, NILTZ);
>   endtv = *tvp;
> - s = &endtv;
> - } else
> - s = NULL;
> + timo =

Re: tetris(6): fix select() -> poll() conversion

[patrick keshishian]

On Wed, Nov 05, 2014 at 08:45:07PM +0100, Theo Buehler wrote:
> Pausing a tetris game currently causes a segfault due to a a null
> pointer dereference.
> 
> Fix this by checking that s is non-NULL before accessing its members.
> 
> A number of comments and an error message still refer to select()
> instead of poll(). Correct this as well.
> 
> 
> Index: input.c
> ===
> RCS file: /cvs/src/games/tetris/input.c,v
> retrieving revision 1.13
> diff -u -p -r1.13 input.c
> --- input.c   3 Nov 2014 22:14:54 -   1.13
> +++ input.c   5 Nov 2014 19:39:30 -
> @@ -64,12 +64,12 @@
>   }
>  
>  /*
> - * Do a `read wait': select for reading from stdin, with timeout *tvp.
> + * Do a `read wait': poll for reading from stdin, with timeout *tvp.
>   * On return, modify *tvp to reflect the amount of time spent waiting.
>   * It will be positive only if input appeared before the time ran out;
>   * otherwise it will be zero or perhaps negative.
>   *
> - * If tvp is nil, wait forever, but return if select is interrupted.
> + * If tvp is nil, wait forever, but return if poll is interrupted.
>   *
>   * Return 0 => no input, 1 => can read() from stdin
>   */
> @@ -90,14 +90,15 @@ rwait(struct timeval *tvp)
>  again:
>   pfd[0].fd = STDIN_FILENO;
>   pfd[0].events = POLLIN;
> - switch (poll(pfd, 1, s->tv_sec * 1000 + s->tv_usec / 1000)) {
> + switch (poll(pfd, 1, s ? s->tv_sec * 1000 + s->tv_usec / 1000 :


I propose getting rid of the s pointer all together as such:

Index: input.c
===
RCS file: /cvs/obsd/src/games/tetris/input.c,v
retrieving revision 1.13
diff -u -p -u -p -r1.13 input.c
--- input.c 3 Nov 2014 22:14:54 -   1.13
+++ input.c 5 Nov 2014 22:40:47 -
@@ -64,19 +64,20 @@
}
 
 /*
- * Do a `read wait': select for reading from stdin, with timeout *tvp.
+ * Do a `read wait': poll for reading from stdin, with timeout *tvp.
  * On return, modify *tvp to reflect the amount of time spent waiting.
  * It will be positive only if input appeared before the time ran out;
  * otherwise it will be zero or perhaps negative.
  *
- * If tvp is nil, wait forever, but return if select is interrupted.
+ * If tvp is nil, wait forever, but return if poll is interrupted.
  *
  * Return 0 => no input, 1 => can read() from stdin
  */
 int
 rwait(struct timeval *tvp)
 {
-   struct timeval starttv, endtv, *s;
+   int timo = INFTIM;
+   struct timeval starttv, endtv;
struct pollfd pfd[1];
 
 #defineNILTZ ((struct timezone *)0)
@@ -84,20 +85,19 @@ rwait(struct timeval *tvp)
if (tvp) {
(void) gettimeofday(&starttv, NILTZ);
endtv = *tvp;
-   s = &endtv;
-   } else
-   s = NULL;
+   timo = endtv.tv_sec * 1000 + endtv.tv_usec / 1000;
+   }
 again:
pfd[0].fd = STDIN_FILENO;
pfd[0].events = POLLIN;
-   switch (poll(pfd, 1, s->tv_sec * 1000 + s->tv_usec / 1000)) {
+   switch (poll(pfd, 1, timo)) {
 
case -1:
if (tvp == 0)
return (-1);
if (errno == EINTR)
goto again;
-   stop("select failed, help");
+   stop("poll failed, help");
/* NOTREACHED */
 
case 0: /* timed out */
@@ -115,7 +115,7 @@ again:
 }
 
 /*
- * `sleep' for the current turn time (using select).
+ * `sleep' for the current turn time (using poll).
  * Eat any input that might be available.
  */
 void



--patrick









> + INFTIM)) {
>  
>   case -1:
>   if (tvp == 0)
>   return (-1);
>   if (errno == EINTR)
>   goto again;
> - stop("select failed, help");
> + stop("poll failed, help");
>   /* NOTREACHED */
>  
>   case 0: /* timed out */
> @@ -115,7 +116,7 @@ again:
>  }
>  
>  /*
> - * `sleep' for the current turn time (using select).
> + * `sleep' for the current turn time (using poll).
>   * Eat any input that might be available.
>   */
>  void
> 

Re: deobfuscate disk subr gpt

[patrick keshishian]

On Mon, Nov 03, 2014 at 11:56:25AM -0500, Ted Unangst wrote:
> On Sat, Nov 01, 2014 at 22:45, Ted Unangst wrote:
> > Pull out a few common subexpressions. I think this makes the code
> > easier to read. Some byte swaps are left, when they are only used once.
> > 
> > Then use mallocarray for bounds checking.
> > 
> > Also observe the following:
> > +   if (ghsize < GPTMINHDRSIZE && ghsize > DEV_BSIZE)
> > I'm pretty sure that should be an ||, otherwise it's never true.
> 
> The rest is committed. Now for the real fix. Check my work?

The change matches the comment. Should the comment be updated
to use the #define GPTMINHDRSIZE instead of 92?

--patrick

> Index: subr_disk.c
> ===
> RCS file: /cvs/src/sys/kern/subr_disk.c,v
> retrieving revision 1.172
> diff -u -p -r1.172 subr_disk.c
> --- subr_disk.c   3 Nov 2014 16:55:21 -   1.172
> +++ subr_disk.c   3 Nov 2014 16:55:59 -
> @@ -702,7 +702,7 @@ readgptlabel(struct buf *bp, void (*stra
>* Header size must be greater than or equal to 92 and less
>* than or equal to the logical block size.
>*/
> - if (ghsize < GPTMINHDRSIZE && ghsize > DEV_BSIZE)
> + if (ghsize < GPTMINHDRSIZE || ghsize > DEV_BSIZE)
>   return (EINVAL);
>  
>   if (letoh64(gh.gh_lba_start) >= DL_GETDSIZE(lp) ||
> 

Re: swap affinity?

[patrick keshishian]

On Sat, Sep 20, 2014 at 08:53:58AM +0200, Otto Moerbeek wrote:
> On Fri, Sep 19, 2014 at 10:05:35PM -0700, patrick keshishian wrote:
> 
> > Hi,
> > 
> > Just ran into something strange. I was running 'sudo cdio cdrip'
> > and in another shell running a memory-hungry perl script, this
> > caused a bunch of processes to swap out (namely Xorg). Once
> > the cdrip was done, and I killed the perl script, and let the
> > system "calm down", I still noticed lag in moving the mouse
> > around in X. Looking at top, Xorg (and other processes, which
> > were swapped, ones that show 4K RES) seem to want to stay swapped.
> > Playing in X, trying to force it to unswap, I see its RES increase,
> > but once I leave it alone, I can see in top the its RES goes
> > down, until I start doing things in X. Same with other processes.
> > cu is at 4K RES ATM, If I start typing in its session, RES
> > goes up to 250K, then back down to 4K.
> > 
> > live example of this xterm I'm typing in:
> > 
> > 13634 sidster20 3120K  352K sleep select0:02  0.00% xterm
> > 
> > stop typing and:
> > 
> > 13634 sidster20 3120K4K sleep select0:02  0.00% xterm
> > 
> > What other info can i provide that would be useful? Before
> > I reboot this box (cause I can't stand working like this)?
> 
> Lacking informationm, it looks like your system still has shortage of
> memory, i.e. large processes are still there. I never saw the
> phenomenon you are describing after the memory hungry processes were gone,

Wish I knew what other info to provide. This is top output
before I rebooted the box:

load averages:  0.08,  0.08,  0.08   amd64.local 23:07:02
43 processes: 42 idle, 1 on processor
CPU states:  5.9% user,  0.1% nice,  3.3% system,  0.0% interrupt, 90.7% idle
Memory: Real: 6692K/461M act/tot Free: 22M Cache: 37M Swap: 47M/2055M

  PID USERNAME PRI NICE  SIZE   RES STATE WAIT  TIMECPU COMMAND
23352 sidster20 6828K 3364K sleep kqread3:58  0.44% tmux
15110 _sndio 2  -20  856K4K idle  poll 12:04  0.00% sndiod
26246 _pflogd40  740K  140K sleep bpf   0:04  0.00% pflogd
16142 _x11   20   28M  992K idle  select0:03  0.00% Xorg
1 root  100  552K4K idle  wait  0:01  0.00% init
 6818 sidster20 1712K 2540K sleep poll  0:01  0.00% top
 1256 sidster20 1200K  824K idle  select0:01  0.00% ssh
 3702 sidster   180 1056K4K idle  pause 0:00  0.00% ksh
 2758 sidster   180  776K  720K sleep pause 0:00  0.00% ksh
 8700 _syslogd   20  912K  536K sleep poll  0:00  0.00% syslogd
24187 root   20 1460K  116K idle  poll  0:00  0.00% xdm
26400 root   20  992K  496K sleep select0:00  0.00% cron
 3037 uucp   20  748K  436K idle  kqread0:00  0.00% cu
 3915 sidster20 1092K4K idle  select0:00  0.00% ssh
29334 sidster30  760K4K idle  ttyin 0:00  0.00% ksh
 3468 _ntp   2  -20  844K  720K idle  poll  0:00  0.00% ntpd
 2982 _smtpd 20 1744K4K idle  kqread0:00  0.00% smtpd
15793 _smtpq 20 1600K4K idle  kqread0:00  0.00% smtpd
13434 root  180  752K4K idle  pause 0:00  0.00% xdm
 4428 sidster   180  672K   20K idle  pause 0:00  0.00% ksh
19015 root   20 1552K4K idle  kqread0:00  0.00% smtpd
20298 _x11   20  940K4K idle  poll  0:00  0.00% xconsole
19314 _smtpd 20 1560K4K idle  kqread0:00  0.00% smtpd
25006 _smtpd 20 1676K4K idle  kqread0:00  0.00% smtpd
 7575 root   2  -20  788K  532K idle  poll  0:00  0.00% ntpd
 6229 sidster   180  776K4K idle  pause 0:00  0.00% ksh
13594 root   20 2540K4K idle  netio 0:00  0.00% Xorg
32714 _smtpd 20 1404K4K idle  kqread0:00  0.00% smtpd
 3018 _smtpd 20 1332K4K idle  kqread0:00  0.00% smtpd
13887 sidster20  968K  380K idle  kqread0:00  0.00% tmux
 6124 sidster   180  764K4K idle  pause 0:00  0.00% ksh
19081 sidster   180  620K4K idle  pause 0:00  0.00% ksh
14425 root   30  424K4K idle  ttyin 0:00  0.00% getty
26697 root   30  568K4K idle  ttyin 0:00  0.00% getty
30470 root   30  492K4K idle  ttyin 0:00  0.00% getty
29335 root   30  648K4K idle  ttyin 0:00  0.00% getty
19748 root   20  824K4K idle  select0:00  0.00% lpd
31411 root   20  676K4K idle  netio 0:00  0.00% pflogd
 8289 root   20  912K4K idle  netio 0:00  0.00% syslogd
32

swap affinity?

[patrick keshishian]

Hi,

Just ran into something strange. I was running 'sudo cdio cdrip'
and in another shell running a memory-hungry perl script, this
caused a bunch of processes to swap out (namely Xorg). Once
the cdrip was done, and I killed the perl script, and let the
system "calm down", I still noticed lag in moving the mouse
around in X. Looking at top, Xorg (and other processes, which
were swapped, ones that show 4K RES) seem to want to stay swapped.
Playing in X, trying to force it to unswap, I see its RES increase,
but once I leave it alone, I can see in top the its RES goes
down, until I start doing things in X. Same with other processes.
cu is at 4K RES ATM, If I start typing in its session, RES
goes up to 250K, then back down to 4K.

live example of this xterm I'm typing in:

13634 sidster20 3120K  352K sleep select0:02  0.00% xterm

stop typing and:

13634 sidster20 3120K4K sleep select0:02  0.00% xterm

What other info can i provide that would be useful? Before
I reboot this box (cause I can't stand working like this)?

Cheers,
--patrick


$ sysctl kern.version
kern.version=OpenBSD 5.6-current (GENERIC) #345: Tue Sep  9 00:22:24 MDT 2014
t...@amd64.openbsd.org:/usr/src/sys/arch/amd64/compile/GENERIC


OpenBSD 5.6-current (GENERIC) #345: Tue Sep  9 00:22:24 MDT 2014
t...@amd64.openbsd.org:/usr/src/sys/arch/amd64/compile/GENERIC
real mem = 520028160 (495MB)
avail mem = 497561600 (474MB)
mpath0 at root
scsibus0 at mpath0: 256 targets
mainbus0 at root
bios0 at mainbus0: SMBIOS rev. 2.3 @ 0xf0100 (43 entries)
bios0: vendor Award Software International, Inc. version "F9" date 05/11/2006
bios0: Gigabyte Technology Co., Ltd. GA-K8N-SLi / GA-K8N-SLi-RH
acpi0 at bios0: rev 0
acpi0: sleep states S0 S1 S4 S5
acpi0: tables DSDT FACP MCFG APIC
acpi0: wakeup devices HUB0(S5) XVR0(S5) XVR1(S5) XVR2(S5) XVR3(S5) USB0(S3) 
USB2(S3) MMAC(S5) MMCI(S5) UAR1(S5)
acpitimer0 at acpi0: 3579545 Hz, 24 bits
acpimcfg0 at acpi0 addr 0xd000, bus 0-255
acpimadt0 at acpi0 addr 0xfee0: PC-AT compat
cpu0 at mainbus0: apid 0 (boot processor)
cpu0: AMD Athlon(tm) 64 Processor 3200+, 2010.53 MHz
cpu0: 
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,MMX,FXSR,SSE,SSE2,SSE3,NXE,MMXX,FFXSR,LONG,3DNOW2,3DNOW,LAHF
cpu0: 64KB 64b/line 2-way I-cache, 64KB 64b/line 2-way D-cache, 512KB 64b/line 
16-way L2 cache
cpu0: ITLB 32 4KB entries fully associative, 8 4MB entries fully associative
cpu0: DTLB 32 4KB entries fully associative, 8 4MB entries fully associative
cpu0: AMD erratum 89 present, BIOS upgrade may be required
mtrr: Pentium Pro MTRR support, 8 var ranges, 88 fixed ranges
cpu0: apic clock running at 201MHz
ioapic0 at mainbus0: apid 2 pa 0xfec0, version 11, 24 pins
acpiprt0 at acpi0: bus 0 (PCI0)
acpiprt1 at acpi0: bus 1 (HUB0)
acpicpu0 at acpi0: PSS
acpibtn0 at acpi0: PWRB
cpu0: Cool'n'Quiet K8 2010 MHz: speeds: 2000 1000 MHz
pci0 at mainbus0 bus 0
"NVIDIA nForce4 DDR" rev 0xa3 at pci0 dev 0 function 0 not configured
pcib0 at pci0 dev 1 function 0 "NVIDIA nForce4 ISA" rev 0xa3
nviic0 at pci0 dev 1 function 1 "NVIDIA nForce4 SMBus" rev 0xa2
iic0 at nviic0
spdmem0 at iic0 addr 0x50: 512MB DDR SDRAM non-parity PC3200CL3.0
iic1 at nviic0
iic1: addr 0x4e 01=06 02=10 03=72 05=80 06=0f 0a=ff 0e=e0 0f=ff words 00=00ff 
01=06ff 02=10ff 03=72ff 04=00ff 05=80ff 06=0fff 07=00ff
ohci0 at pci0 dev 2 function 0 "NVIDIA nForce4 USB" rev 0xa2: apic 2 int 10, 
version 1.0, legacy support
ehci0 at pci0 dev 2 function 1 "NVIDIA nForce4 USB" rev 0xa3: apic 2 int 11
usb0 at ehci0: USB revision 2.0
uhub0 at usb0 "NVIDIA EHCI root hub" rev 2.00/1.00 addr 1
auich0 at pci0 dev 4 function 0 "NVIDIA nForce4 AC97" rev 0xa2: apic 2 int 5, 
nForce4 AC97
ac97: codec id 0x414c4790 (Avance Logic ALC850 rev 0)
audio0 at auich0
pciide0 at pci0 dev 6 function 0 "NVIDIA nForce4 IDE" rev 0xf2: DMA, channel 0 
configured to compatibility, channel 1 configured to compatibility
wd0 at pciide0 channel 0 drive 0: 
wd0: 16-sector PIO, LBA48, 238474MB, 488395055 sectors
wd0(pciide0:0:0): using PIO mode 4, Ultra-DMA mode 5
pciide0: channel 1 disabled (no drives)
pciide1 at pci0 dev 7 function 0 "NVIDIA nForce4 SATA" rev 0xf3: DMA
pciide1: using apic 2 int 11 for native-PCI interrupt
atapiscsi0 at pciide1 channel 0 drive 0
scsibus1 at atapiscsi0: 2 targets
cd0 at scsibus1 targ 0 lun 0:  ATAPI 5/cdrom 
removable
cd0(pciide1:0:0): using PIO mode 4, Ultra-DMA mode 6
pciide2 at pci0 dev 8 function 0 "NVIDIA nForce4 SATA" rev 0xf3: DMA
pciide2: using apic 2 int 10 for native-PCI interrupt
ppb0 at pci0 dev 9 function 0 "NVIDIA nForce4" rev 0xa2
pci1 at ppb0 bus 1
dc0 at pci1 dev 7 function 0 "Lite-On PNIC" rev 0x20: apic 2 int 5, address 
00:02:e3:07:cc:df
bmtphy0 at dc0 phy 1: BCM5201 10/100 PHY, rev. 2
nfe0 at pci0 dev 10 function 0 "NVIDIA CK804 LAN" rev 0xa3: apic 2 int 11, 
address 00:16:e6:82:17:da
ciphy0 at nfe0 phy 7: CS8201 10/100/1000TX PHY, rev. 3
ppb1 at pci0 dev 11 function 0 "NVIDIA nForce4 PCIE" re

Re: autonetd, Wi-Fi automated configuration

[patrick keshishian]

Hi,

Since you did ask for input.

On Wed, Aug 13, 2014 at 07:06:23PM +0400, Vadim Zhukov wrote:
> Hello all.
> 
> I won't describe the problem, you all new it: when you switch between
> hotspots, your network interface doesn't follow you. Also, you
> probably want to have some sort of fallback configuration.
> 
> Maybe you have a bunch of shell scripts, or whatever, - all this stuff
> that rely on ifconfig(8) output, and thus tend to break on crazy

I did when I worked for a company and my laptop went to work
and came back home. They were simple scripts, did not do any
auto-anything. I would simply do:

$ sh workwifi.sh# at employer
$ sh homewifi.sh# at home
$ sh momdadwifi.sh  # at my parents' home

You can get fancier than this if you really need to be.
All the necessary tools exist on the base OS.

> network names. Sometimes you probably asked yourself, why there is no
> NetworkManager analog in OpenBSD? - So here is a try of constructing
> an answer to ths question.

No, never. In fact, the opposite. Every time I come across
a daemon/"manager" I wonder "is this really necessary?"

To me, the elegance of OpenBSD has always been in its
simplicity. It provides all necessary tools to its users.
The users are free to build the more sophisticated tools
(e.g., ones found in ports/packages).

This leaves OpenBSD uncluttered. Unlike some other OSes,
e.g., MacOS X, inundated with countless processes, busy
doing ... something, I'm sure.  And what and why in the
world are "those two" sending and receiving over my network?

Every time I take a look at the process list on my MBP
I get this depressed, grossed-out, disgusted feeling.


And on that high note...
--patrick

p.s., My plea: Please not for the base OS.


> The internal logic is simple: scan networks, find known one and try to
> run ifconfig(8) (and, probably, dhclient(8) and/or route(8)) on it. I
> tried to avoid creating another rich parser syntax - all of the lines
> you set up in configuration file became command-line arguments of
> corresponding utilities.
> 
> I invite you to look at the current source code here:
> 
> cvs -d anon...@anoncvs.ohvost.ru:/cvs checkout autonetd
> 
> Maybe this will evolve into backend of some GUI, or whatever. Maybe
> not. But at least it works much better than what I've had before.
> 
> Any input is highly appreciated. Thank you for your time!
> 
> --
>   WBR,
>   Vadim Zhukov
> 

Re: Print strings with double quotes safely in ifconfig(8)

[patrick keshishian]

On 8/10/14, Vadim Zhukov  wrote:
> This changes the way ifconfig(8) to print lines like 'crazy "nwid',
> i.e., containing double quotes inside the data being output.
> At the present, such lines will be printed in the following way:
>
> "crazy "nwid"
>
> And this makes everything that tries to parse such lines go crazy
> in their turn. I propose to force unambigious hexadecimal output
> in this case.

Caution: Slippery slope ahead!

Any other "weird" characters that may confuse parsers? I see a
bunch of networks with single-quotes in them in my area. What
about back-slashes, back-ticks, exclamation-marks, hash-marks,
...?

--patrick


> Objections/okays?
> --
> WBR,
>   Vadim Zhukov
>
>
> Index: ifconfig.c
> ===
> RCS file: /cvs/src/sbin/ifconfig/ifconfig.c,v
> retrieving revision 1.287
> diff -u -p -r1.287 ifconfig.c
> --- ifconfig.c12 Jul 2014 19:58:17 -  1.287
> +++ ifconfig.c10 Aug 2014 21:11:03 -
> @@ -1499,8 +1499,8 @@ print_string(const u_int8_t *buf, int le
>
>   if (len < 2 || buf[0] != '0' || tolower(buf[1]) != 'x') {
>   for (; i < len; i++) {
> - /* Only print 7-bit ASCII keys */
> - if (buf[i] & 0x80 || !isprint(buf[i]))
> + /* Only print 7-bit ASCII keys, excluding double quote 
> */
> + if (buf[i] & 0x80 || !isprint(buf[i]) || buf[i] == '"')
>   break;
>   if (isspace(buf[i]))
>   hasspc++;
>
>

Re: [PATCH]unused NULL check before calling free

[patrick keshishian]

On Wed, Jul 30, 2014 at 10:14:54PM +0200, Fritjof Bornebusch wrote:
> Hi tech,
> 
> there is an unnecessary NULL check before calling free.
> 
> fritjof
> 
> Index: xmalloc.c
> ===
> RCS file: /cvs/src/usr.bin/rcs/xmalloc.c,v
> retrieving revision 1.4
> diff -u -p -r1.4 xmalloc.c
> --- xmalloc.c   7 Jun 2009 08:39:13 -   1.4
> +++ xmalloc.c   31 May 2014 19:19:18 -
> @@ -76,8 +76,6 @@ xrealloc(void *ptr, size_t nmemb, size_t
>  void
>  xfree(void *ptr)
>  {
> -   if (ptr == NULL)
> -   errx(1, "xfree: NULL pointer given as argument");
> free(ptr);
>  }


Going by this context, a quick grep in src/usr.bin/rcs, and
especially the error message, the NULL check's purpose is to
catach this condition.

The code you claim to correct:

> there is an unnecessary NULL check before calling free.

would have been of the form:

if (ptr != NULL)-or-if (ptr == NULL)
free(ptr);  return;

--patrick

Re: lsearch.c

[patrick keshishian]

On 7/17/14, Matthew Dempsky  wrote:
> Hrm.  It seems silly to me to change it to require a non-const pointer
> argument,

Silly even though, the description of lsearch says it will modify
("it shall be added at the end of") the table, for which "base
argument points to the first element"?

analogy: dst in strlcat() being declared const char *?

--patrick


> but POSIX, Linux, Solaris, FreeBSD, and NetBSD all use "void
> *base" so it doesn't seem like we should have any ports tree issues
> from changing it to be compatible either.
>
> On Thu, Jul 17, 2014 at 6:04 PM, enh  wrote:
>> For me (Android) the practical consequence is compiler warnings unless I
>> change search.h to be wrong.
>>
>> I'm going through the non-Open BSD stuff I have working out why. In this
>> case, this seems to be the reason we have the NetBSD search.h
>> implementation. The majority of our BSD code is OpenBSD so my preference
>> would be to switch over as much as possible.
>>
>> On Jul 17, 2014 5:59 PM, "Matthew Dempsky"  wrote:
>>>
>>> Hm, is there a practical consequence of this?  Seems like it would
>>> only affect applications trying to store a reference to lsearch in a
>>> function pointer of type "void (*)(const void *, void *, size_t *,
>>> size_t, int (*)(const void *, const void *))"; do those exist?
>>>
>>> On Thu, Jul 17, 2014 at 5:38 PM, enh  wrote:
>>> > POSIX says lsearch's 'base' is void*, not const void*.
>>> >
>>> > http://pubs.opengroup.org/onlinepubs/9699919799/functions/lsearch.html
>>> >
>>> > openbsd gets this wrong in search.h and lsearch.c. (but lfind is
>>> > correct --- that should be const void*.)
>>> >
>>> >  -e
>>> >
>
>

Re: Probably you already know but http://www.openbsd.org/cgi-bin/cvsweb/ is returning HTTP 403 Forbidden

[patrick keshishian]

On 7/16/14, Bob Beck  wrote:
> I've disabled it due to persistant DOS attacks. It may come back later.

wait ... what?

you helping'em? :P

--patrick


> On Wed, Jul 16, 2014 at 12:11 PM, Rafael Neves 
> wrote:
>> Hi Beck,
>>
>> Probably you already know  is
>> returning HTTP 403 Forbidden. This is recent, maybe two or three days.
>>
>> Is there any idea when ti will become available again?
>>
>> Thanks.
>>
>> Regards,
>> Rafael
>
>

[patch] sys/dev/ic/mfi.c

[patrick keshishian]

Hi,

I think is a bug in sys/dev/ic/mfi.c noticed during
"PATCH: further kernel malloc -> mallocarray" review[1]

I see the mallocarray() patch seems to have been applied.
Want to make sure if this is in fact a bug, that it is
not overlooked.

Ignore if this is noise.

Cheers,
--patrick

[1] http://marc.info/?t=14054867071&r=1&w=2


Index: mfi.c
===
RCS file: /cvs/src/sys/dev/ic/mfi.c,v
retrieving revision 1.154
diff -u -p -u -p -r1.154 mfi.c
--- mfi.c   13 Jul 2014 23:10:23 -  1.154
+++ mfi.c   16 Jul 2014 17:57:04 -
@@ -2149,7 +2149,7 @@ mfi_create_sensors(struct mfi_softc *sc)
}
 
sc->sc_bbu_status = malloc(sizeof(*sc->sc_bbu_status) *
-   sizeof(mfi_bbu_indicators), M_DEVBUF, M_WAITOK | M_ZERO);
+   nitems(mfi_bbu_indicators), M_DEVBUF, M_WAITOK | M_ZERO);
 
for (i = 0; i < nitems(mfi_bbu_indicators); i++) {
sc->sc_bbu_status[i].type = SENSOR_INDICATOR;

Re: PATCH: further kernel malloc -> mallocarray

[patrick keshishian]

Question, comment and a potential bug ...

On Wed, Jul 16, 2014 at 04:54:49AM +, Doug Hogan wrote:
> ===
> RCS file: /cvs/src/sys/arch/amd64/amd64/est.c,v
> retrieving revision 1.33
> diff -u -p -d -r1.33 est.c
> --- sys/arch/amd64/amd64/est.c12 Jul 2014 18:44:41 -  1.33
> +++ sys/arch/amd64/amd64/est.c15 Jul 2014 23:48:21 -
[...]
> @@ -381,8 +381,8 @@ est_init(struct cpu_info *ci)
>   }
>  
>  
> - if ((fake_table = malloc(sizeof(struct est_op) * 3, M_DEVBUF,
> -  M_NOWAIT)) == NULL) {
> + if ((fake_table = mallocarray(3, sizeof(struct est_op),
> +  M_DEVBUF, M_NOWAIT)) == NULL) {

For obvious cases such as this, is it worth converting?
This is almost suggesting following conversion:

- ptr = malloc(sizeof(struct mystruct), ...);
+ ptr = mallocarray(1, sizeof(struct mystruct), ...);


> ===
> RCS file: /cvs/src/sys/arch/i386/i386/est.c,v
> retrieving revision 1.43
> diff -u -p -d -r1.43 est.c
> --- sys/arch/i386/i386/est.c  12 Jul 2014 18:44:41 -  1.43
> +++ sys/arch/i386/i386/est.c  15 Jul 2014 23:48:23 -
[...]
> @@ -1140,8 +1140,8 @@ est_init(struct cpu_info *ci, int vendor
>   }
>  
>  
> - if ((fake_table = malloc(sizeof(struct est_op) * 3, M_DEVBUF,
> -  M_NOWAIT)) == NULL) {
> + if ((fake_table = mallocarray(3, sizeof(struct est_op),
> +  M_DEVBUF, M_NOWAIT)) == NULL) {
[...]
> ===
> RCS file: /cvs/src/sys/arch/sparc/dev/obio.c,v
> retrieving revision 1.22
> diff -u -p -d -r1.22 obio.c
> --- sys/arch/sparc/dev/obio.c 5 Sep 2010 18:10:10 -   1.22
> +++ sys/arch/sparc/dev/obio.c 15 Jul 2014 23:48:26 -
> @@ -310,7 +310,7 @@ vmesattach(parent, self, args)
>   printf("\n");
>  
>   if (vmeints == NULL) {
> - vmeints = malloc(256 * sizeof(struct intrhand *), M_TEMP,
> + vmeints = mallocarray(256, sizeof(struct intrhand *), M_TEMP,
>   M_NOWAIT | M_ZERO);
>   if (vmeints == NULL)
>   panic("vmesattach: can't allocate intrhand");
> @@ -332,7 +332,7 @@ vmelattach(parent, self, args)
>   printf("\n");
>  
>   if (vmeints == NULL) {
> - vmeints = malloc(256 * sizeof(struct intrhand *), M_TEMP,
> + vmeints = mallocarray(256, sizeof(struct intrhand *), M_TEMP,
>   M_NOWAIT | M_ZERO);
>   if (vmeints == NULL)
>   panic("vmelattach: can't allocate intrhand");
[...]
> Index: sys/arch/sparc64/dev/vdsk.c
> ===
> RCS file: /cvs/src/sys/arch/sparc64/dev/vdsk.c,v
> retrieving revision 1.39
> diff -u -p -d -r1.39 vdsk.c
> --- sys/arch/sparc64/dev/vdsk.c   12 Jul 2014 18:44:43 -  1.39
> +++ sys/arch/sparc64/dev/vdsk.c   15 Jul 2014 23:48:27 -
> @@ -298,7 +298,7 @@ vdsk_attach(struct device *parent, struc
>   printf(", can't allocate dring\n");
>   goto free_map;
>   }
> - sc->sc_vsd = malloc(32 * sizeof(*sc->sc_vsd), M_DEVBUF, M_NOWAIT);
> + sc->sc_vsd = mallocarray(32, sizeof(*sc->sc_vsd), M_DEVBUF, M_NOWAIT);
>   if (sc->sc_vsd == NULL) {
>   printf(", can't allocate software ring\n");
>   goto free_dring;
[...]
> Index: sys/arch/sparc64/dev/vnet.c
> ===
> RCS file: /cvs/src/sys/arch/sparc64/dev/vnet.c,v
> retrieving revision 1.33
> diff -u -p -d -r1.33 vnet.c
> --- sys/arch/sparc64/dev/vnet.c   12 Jul 2014 18:44:43 -  1.33
> +++ sys/arch/sparc64/dev/vnet.c   15 Jul 2014 23:48:27 -
> @@ -1381,7 +1381,7 @@ vnet_init(struct ifnet *ifp)
>   sc->sc_vd = vnet_dring_alloc(sc->sc_dmatag, 128);
>   if (sc->sc_vd == NULL)
>   return;
> - sc->sc_vsd = malloc(128 * sizeof(*sc->sc_vsd), M_DEVBUF, M_NOWAIT);
> + sc->sc_vsd = mallocarray(128, sizeof(*sc->sc_vsd), M_DEVBUF, M_NOWAIT);
>   if (sc->sc_vsd == NULL)
>   return;
>  
[...]
> Index: sys/dev/usb/uaudio.c
> ===
> RCS file: /cvs/src/sys/dev/usb/uaudio.c,v
> retrieving revision 1.104
> diff -u -p -d -r1.104 uaudio.c
> --- sys/dev/usb/uaudio.c  12 Jul 2014 18:48:52 -  1.104
> +++ sys/dev/usb/uaudio.c  15 Jul 2014 23:48:35 -
[...]
> @@ -1962,7 +1962,8 @@ uaudio_identify_ac(struct uaudio_softc *
>   ibufend = ibuf + aclen;
>   dp = (const usb_descriptor_t *)ibuf;
>   ndps = 0;
> - iot = malloc(sizeof(struct io_terminal) * 256, M_TEMP, M_NOWAIT | 
> M_ZERO);
> + iot = mallocarray(256, sizeof(struct io_terminal), M_TEMP,
> + M_NOWAIT | M_ZERO);
>   i

Re: mallocarray() in sys/dev, first pass

[patrick keshishian]

On Sun, Jul 13, 2014 at 11:29:22AM -0600, dera...@cvs.openbsd.org wrote:
> This is the first pass of mallocarray() in sys/dev.  Please proofread.

[...]
> ===
> RCS file: /cvs/src/sys/dev/softraid.c,v
> retrieving revision 1.334
> diff -u -p -u -r1.334 softraid.c
> --- softraid.c12 Jul 2014 18:48:51 -  1.334
> +++ softraid.c13 Jul 2014 15:48:56 -
> @@ -252,7 +252,7 @@ sr_meta_attach(struct sr_discipline *sd,
>  
>   /* we have a valid list now create an array index */
>   cl = &sd->sd_vol.sv_chunk_list;
> - sd->sd_vol.sv_chunks = malloc(sizeof(struct sr_chunk *) * chunk_no,
> + sd->sd_vol.sv_chunks = mallocarray(chunk_no, sizeof(struct sr_chunk *),
>   M_DEVBUF, M_WAITOK | M_ZERO);
>  
>   /* fill out chunk array */
> @@ -1284,13 +1284,13 @@ sr_boot_assembly(struct sr_softc *sc)
>   }
>  
>   /* Allocate memory for device and ondisk version arrays. */
> - devs = malloc(BIOC_CRMAXLEN * sizeof(dev_t), M_DEVBUF,
> + devs = mallocarray(BIOC_CRMAXLEN, sizeof(dev_t), M_DEVBUF,
>   M_NOWAIT | M_CANFAIL);
>   if (devs == NULL) {
>   printf("%s: failed to allocate device array\n", DEVNAME(sc));
>   goto unwind;
>   }
> - ondisk = malloc(BIOC_CRMAXLEN * sizeof(u_int64_t), M_DEVBUF,
> + ondisk = mallocarray(BIOC_CRMAXLEN, sizeof(u_int64_t), M_DEVBUF,
>   M_NOWAIT | M_CANFAIL);
>   if (ondisk == NULL) {
>   printf("%s: failed to allocate ondisk array\n", DEVNAME(sc));
> @@ -1934,8 +1934,9 @@ sr_ccb_alloc(struct sr_discipline *sd)
>   if (sd->sd_ccb)
>   return (1);
>  
> - sd->sd_ccb = malloc(sizeof(struct sr_ccb) *
> - sd->sd_max_wu * sd->sd_max_ccb_per_wu, M_DEVBUF, M_WAITOK | M_ZERO);
> + sd->sd_ccb = mallocarray(sd->sd_max_wu,
> + sd->sd_max_ccb_per_wu * sizeof(struct sr_ccb),
> + M_DEVBUF, M_WAITOK | M_ZERO);

I don't have the knowledge, but I'd hope sd->sd_max_ccb_per_wu
is smaller than sd->sd_max_wu?


>   TAILQ_INIT(&sd->sd_ccb_freeq);
>   for (i = 0; i < sd->sd_max_wu * sd->sd_max_ccb_per_wu; i++) {
>   ccb = &sd->sd_ccb[i];

[...]
> Index: ic/ciss.c
> ===
> RCS file: /cvs/src/sys/dev/ic/ciss.c,v
> retrieving revision 1.69
> diff -u -p -u -r1.69 ciss.c
> --- ic/ciss.c 12 Jul 2014 18:48:17 -  1.69
> +++ ic/ciss.c 13 Jul 2014 16:20:29 -
> @@ -347,7 +347,7 @@ ciss_attach(struct ciss_softc *sc)
>   return -1;
>   }
>  
> - if (!(sc->sc_lds = malloc(sc->maxunits * sizeof(*sc->sc_lds),
> + if (!(sc->sc_lds = mallocarray(sc->maxunits, sizeof(*sc->sc_lds),
>   M_DEVBUF, M_NOWAIT | M_ZERO))) {
>   bus_dmamem_free(sc->dmat, sc->cmdseg, 1);
>   bus_dmamap_destroy(sc->dmat, sc->cmdmap);
> @@ -385,7 +385,7 @@ ciss_attach(struct ciss_softc *sc)
>  
>   sc->sc_flags |= CISS_BIO;
>  #ifndef SMALL_KERNEL
> - sc->sensors = malloc(sizeof(struct ksensor) * sc->maxunits,
> + sc->sensors = mallocarray(sc->maxunits, sizeof(struct ksensor),
>   M_DEVBUF, M_NOWAIT | M_ZERO);
>   if (sc->sensors) {
>   struct device *dev;
> @@ -1311,7 +1311,7 @@ ciss_pdscan(struct ciss_softc *sc, int l
>   if (!k)
>   return NULL;
>  
> - ldp = malloc(sizeof(*ldp) + (k-1), M_DEVBUF, M_NOWAIT);
> + ldp = mallocarray(k-1, sizeof(*ldp), M_DEVBUF, M_NOWAIT);

Oops!
That's not a multiplication. It's an addition.
... or is/was the original code incorrect?


>   if (!ldp)
>   return NULL;
>  

[...]
> Index: ic/qla.c
> ===
> RCS file: /cvs/src/sys/dev/ic/qla.c,v
> retrieving revision 1.42
> diff -u -p -u -r1.42 qla.c
> --- ic/qla.c  12 Jul 2014 18:48:17 -  1.42
> +++ ic/qla.c  13 Jul 2014 16:02:02 -
> @@ -2547,7 +2547,7 @@ qla_alloc_ccbs(struct qla_softc *sc)
>   mtx_init(&sc->sc_port_mtx, IPL_BIO);
>   mtx_init(&sc->sc_mbox_mtx, IPL_BIO);
>  
> - sc->sc_ccbs = malloc(sizeof(struct qla_ccb) * sc->sc_maxcmds,
> + sc->sc_ccbs = mallocarray(sizeof(struct qla_ccb), sc->sc_maxcmds,
>   M_DEVBUF, M_WAITOK | M_CANFAIL | M_ZERO);

typically put count as first and size as second param?


>   if (sc->sc_ccbs == NULL) {
>   printf("%s: unable to allocate ccbs\n", DEVNAME(sc));
> Index: ic/qlw.c
> ===
> RCS file: /cvs/src/sys/dev/ic/qlw.c,v
> retrieving revision 1.23
> diff -u -p -u -r1.23 qlw.c
> --- ic/qlw.c  12 Jul 2014 18:48:17 -  1.23
> +++ ic/qlw.c  13 Jul 2014 16:01:53 -
> @@ -1699,7 +1699,7 @@ qlw_alloc_ccbs(struct qlw_softc *sc)
>   mtx_init(&sc->sc_ccb_mtx, IPL_BIO);
>   mtx_init(&sc->sc_queue_mtx, IPL_BIO);
>  
> - sc->sc_ccbs = malloc(sizeof(struct qlw_ccb) * sc->sc_maxccbs,
> + 

Re: lynx: disable old protocols

[patrick keshishian]

On 7/11/14, Theo de Raadt  wrote:
> If lynx was removed from base, and only available in ports... how many of
> you would even know of it's existance and use it?

asking rhetorically?
either way, yes, I would install lynx if it wasn't in base.
I use it on a daily basis.

--patrick

Re: cvs admin for commitids

[patrick keshishian]

On Sun, Jun 29, 2014 at 07:39:59PM -0500, joshua stein wrote:
> On Sun, 29 Jun 2014 at 19:37:18 -0500, joshua stein wrote:
> > This adds a -C option to cvs's admin command which can add, change,
> > or delete a revision's commitid.  I couldn't find any similar
> > functionality in any other CVS implementations, nor any using 'admin
> > -C'.
> 
> Oops, here's the proper diff that actually deletes empty commitids.

This may be a silly question, but how do commitids get
set/generated/applied?

only one paragraph in cvs man page mentions it, but does
not indicate how they are gotten.

--patrick


> 
> 
> Index: gnu/usr.bin/cvs/src/admin.c
> ===
> RCS file: /cvs/src/gnu/usr.bin/cvs/src/admin.c,v
> retrieving revision 1.2
> diff -u -p -u -p -r1.2 admin.c
> --- gnu/usr.bin/cvs/src/admin.c   11 May 2008 12:16:00 -  1.2
> +++ gnu/usr.bin/cvs/src/admin.c   30 Jun 2014 00:39:08 -
> @@ -27,6 +27,7 @@ static const char *const admin_usage[] =
>  "\t-A fileAppend another file's access list.\n",
>  "\t-b[rev]Set default branch (highest branch on trunk if 
> omitted).\n",
>  "\t-c string  Set comment leader.\n",
> +"\t-C rev:id  Replace revision's commit id.\n",
>  "\t-e[users]  Remove (comma-separated) user names from access list\n",
>  "\t   (all names if omitted).\n",
>  "\t-I Run interactively.\n",
> @@ -167,7 +168,7 @@ admin (argc, argv)
>  optind = 0;
>  only_k_option = 1;
>  while ((c = getopt (argc, argv,
> - "+ib::c:a:A:e::l::u::LUn:N:m:o:s:t::IqxV:k:")) != -1)
> + "+ib::c:C:a:A:e::l::u::LUn:N:m:o:s:t::IqxV:k:")) != -1)
>  {
>   if (c != 'k')
>   only_k_option = 0;
> @@ -209,6 +210,11 @@ admin (argc, argv)
>   strcat (admin_data.comment, optarg);
>   break;
>  
> + case 'C':
> + /* Change or add commitid. */
> + arg_add (&admin_data, 'C', optarg);
> + break;
> +
>   case 'a':
>   arg_add (&admin_data, 'a', optarg);
>   break;
> @@ -689,7 +695,7 @@ admin_fileproc (callerdat, finfo)
>  for (i = 0; i < admin_data->ac; ++i)
>  {
>   char *arg;
> - char *p, *rev, *revnum, *tag, *msg;
> + char *p, *rev, *revnum, *tag, *msg, *commitid;
>   char **users;
>   int argc, u;
>   Node *n;
> @@ -736,6 +742,50 @@ admin_fileproc (callerdat, finfo)
>   for (u = 0; u < argc; ++u)
>   RCS_addaccess (rcs, users[u]);
>   free_names (&argc, users);
> + break;
> + case 'C':
> + p = strchr (arg, ':');
> + if (p == NULL)
> + {
> + error (0, 0, "%s: -C option lacks commitid", rcs->path);
> + status = 1;
> + continue;
> + }
> + *p = '\0';
> + rev = RCS_gettag (rcs, arg + 2, 0, NULL);
> + if (rev == NULL)
> + {
> + error (0, 0, "%s: no such revision %s", rcs->path, rev);
> + status = 1;
> + continue;
> + }
> + *p++ = ':';
> + commitid = p;
> +
> + n = findnode (rcs->versions, rev);
> + free (rev);
> + delta = (RCSVers *) n->data;
> +
> + if (delta->other_delta == NULL)
> + delta->other_delta = getlist();
> +
> + n = findnode (delta->other_delta, "commitid");
> + if (n == NULL) {
> + if (strlen(commitid)) {
> + n = getnode();
> + n->type = RCSFIELD;
> + n->key = xstrdup ("commitid");
> + n->data = xstrdup(commitid);
> + addnode (delta->other_delta, n);
> + }
> + } else {
> + if (strlen(commitid)) {
> + free (n->data);
> + n->data = xstrdup(commitid);
> + } else
> + delnode(n);
> + }
> +
>   break;
>   case 'n': /* fall through */
>   case 'N':
> 

simple fstab.5 diff

[patrick keshishian]

fix missing forward-slash.

Index: fstab.5
===
RCS file: /cvs/obsd/src/share/man/man5/fstab.5,v
retrieving revision 1.48
diff -u -p -u -p -r1.48 fstab.5
--- fstab.5 6 Jan 2014 00:52:21 -   1.48
+++ fstab.5 27 Jun 2014 05:52:08 -
@@ -248,7 +248,7 @@ a value of zero is returned and
 will assume that the filesystem does not need to be checked.
 .Bd -literal
 #defineFSTAB_RW"rw"/* read/write device */
-#defineFSTAB_RQ"rq"/* read/write with quotas *
+#defineFSTAB_RQ"rq"/* read/write with quotas */
 #defineFSTAB_RO"ro"/* read-only device */
 #defineFSTAB_SW"sw"/* swap device */
 #defineFSTAB_XX"xx"/* ignore totally */

Re: ANONCVS MIRROR MAINTAINERS.. YOU NEED TO READ THIS!

[patrick keshishian]

On Thu, Jun 26, 2014 at 10:19:09AM +0100, Stuart Henderson wrote:
> On 2014/06/26 11:02, Mike Belopuhov wrote:
> > On 26 June 2014 08:53, patrick keshishian  wrote:
> > > On Wed, Jun 25, 2014 at 10:01:06PM -0700, patrick keshishian wrote:
> > >> On Thu, Jun 26, 2014 at 06:37:00AM +0200, Alexander Hall wrote:
> > >> > On 06/25/14 20:52, Bob Beck wrote:
> > >> > >If you or someone you love runs an anoncvs server, they need to see 
> > >> > >this.
> > >> > >
> > >> > >As you know we recently added commitid support to cvs, and we had
> > >> > >you update your cvsync binary.
> > >> > >
> > >> > >Unfortunately, the fix wasn't quite right.  We ran into problems
> > >> > >with the synching of commitid files. naddy managed to cook
> > >> > >a correct fix for us.
> > >> > >
> > >> > >anoncvs.ca (the fanout machine) has been fixed - again. What you
> > >> > >need to do is to update your cvsync to the latest one that was
> > >> > >just committed into ports (cvsync-0.25.0pre0 or later). Remove your
> > >> > >scanfile (if any).
> > >> > >
> > >> > >Once you do that, re-run cvsync and you should be back in business.
> > >> >
> > >> > Is it known whether this requires a fixed upstream cvsync first, or if
> > >> > it is ok to apply the fix and your repo will become fine whenever the
> > >> > upstream is fixed?
> > >> >
> > >> > Anyone with insight, please feel free to chime in.
> > >>
> > >>
> > >> The commit message suggests, at least, the server must be
> > >> updated:
> > >>
> > >> | Add full support for commitid and bump protocol version
> > >> | Old clients will receive updates with commitid stripped out.
> > >
> > >
> > > It seems, running updated cvsync against an "un-updated"
> > > cvsyncd (I assume) results in failure:
> > >
> > > Connecting to anoncvs1.usa.openbsd.org port 
> > > Connected to 149.20.54.217 port 
> > > Running...
> > > Updating (collection openbsd-src/rcs)
> > > Updater(RCS): UPDATE(delta): error
> > > Updater(RCS): UPDATE: 
> > > /cvs/anoncvs/cvs/obsd/src/lib/libc/string/timingsafe_bcmp.3,v
> > > Updater: RCS Error
> > > Socket Error: recv: 2 residue 2
> > > Receiver Error: recv
> > > Mux(SEND) Error: not running: 0
> > > DirScan: RCS Error
> > > Mux(SEND) Error: not running: 1
> > > FileScan(RCS): UPDATE /cvs/anoncvs/cvs/obsd/src/sbin/pfctl/parse.y,v
> > > FileScan: RCS Error
> > > Failed
> > >
> > >
> > 
> > removing offending files locally and restarting cvsync helps.
> > 
> 
> I suspect this may only help until they receive another commit.

I was thinking it would be bit of a pain to keep running
cvsync, have it fail, remove files, run it again, repeat.
I stopped after three cycles.

> The server *must* be updated - though it looks like there may still be
> some problems.

Run with -v option per Stuart Henderson's comment.

Parsing a file /etc/obsd-cvsync...
Connecting to anoncvs1.usa.openbsd.org port 
Connected to 149.20.54.217 port 
Protocol: 0.25
Hash: MD5
Exchanging collection list...
 collection name "openbsd-src" release "rcs" umask 002
 collection name "openbsd-xenocara" release "rcs" umask 002
 collection name "openbsd-ports" release "rcs" umask 002
Compression: zlib
Trying to establish the multiplexed channel...
Running...
Updating (collection openbsd-src/rcs)
Updater(RCS): UPDATE(delta): error
Updater(RCS): UPDATE: 
/cvs/anoncvs/cvs/obsd/src/lib/libcrypto/crypto/getentropy_linux.c,v
Updater: RCS Error
Socket Error: recv: 1691 residue 334
Receiver(DATA) Error: recv
Mux(SEND) Error: not running: 1
FileScan(RCS): UPDATE 
/cvs/anoncvs/cvs/obsd/src/usr.sbin/installboot/i386_installboot.h,v
FileScan: RCS Error
Mux(SEND) Error: not running: 0
DirScan: RCS Error
Failed

I suspect this is because 'there still are some problems'.

Is there anther way to update one's source trees for the
time being?

Best,
--patrick

Re: ANONCVS MIRROR MAINTAINERS.. YOU NEED TO READ THIS!

[patrick keshishian]

On Wed, Jun 25, 2014 at 10:01:06PM -0700, patrick keshishian wrote:
> On Thu, Jun 26, 2014 at 06:37:00AM +0200, Alexander Hall wrote:
> > On 06/25/14 20:52, Bob Beck wrote:
> > >If you or someone you love runs an anoncvs server, they need to see this.
> > >
> > >As you know we recently added commitid support to cvs, and we had
> > >you update your cvsync binary.
> > >
> > >Unfortunately, the fix wasn't quite right.  We ran into problems
> > >with the synching of commitid files. naddy managed to cook
> > >a correct fix for us.
> > >
> > >anoncvs.ca (the fanout machine) has been fixed - again. What you
> > >need to do is to update your cvsync to the latest one that was
> > >just committed into ports (cvsync-0.25.0pre0 or later). Remove your
> > >scanfile (if any).
> > >
> > >Once you do that, re-run cvsync and you should be back in business.
> > 
> > Is it known whether this requires a fixed upstream cvsync first, or if 
> > it is ok to apply the fix and your repo will become fine whenever the 
> > upstream is fixed?
> > 
> > Anyone with insight, please feel free to chime in.
> 
> 
> The commit message suggests, at least, the server must be
> updated:
> 
> | Add full support for commitid and bump protocol version
> | Old clients will receive updates with commitid stripped out.


It seems, running updated cvsync against an "un-updated"
cvsyncd (I assume) results in failure:

Connecting to anoncvs1.usa.openbsd.org port 
Connected to 149.20.54.217 port 
Running...
Updating (collection openbsd-src/rcs)
Updater(RCS): UPDATE(delta): error
Updater(RCS): UPDATE: 
/cvs/anoncvs/cvs/obsd/src/lib/libc/string/timingsafe_bcmp.3,v
Updater: RCS Error
Socket Error: recv: 2 residue 2
Receiver Error: recv
Mux(SEND) Error: not running: 0
DirScan: RCS Error
Mux(SEND) Error: not running: 1
FileScan(RCS): UPDATE /cvs/anoncvs/cvs/obsd/src/sbin/pfctl/parse.y,v
FileScan: RCS Error
Failed


ouch :-)

--patrick


> HTH,
> --patrick
> 
> 
> > /Alexander
> > 
> > >to check a server do:
> > >cvs -d anon...@yourserver.org:/cvs log /usr/src/libexec/ld.so/malloc.c
> > >|  less  and see if it has r 1.3
> > >
> > >Thanks, and sorry for the disruption,
> > >
> > >-Bob
> > >
> > 
> 

Re: ANONCVS MIRROR MAINTAINERS.. YOU NEED TO READ THIS!

[patrick keshishian]

On Thu, Jun 26, 2014 at 06:37:00AM +0200, Alexander Hall wrote:
> On 06/25/14 20:52, Bob Beck wrote:
> >If you or someone you love runs an anoncvs server, they need to see this.
> >
> >As you know we recently added commitid support to cvs, and we had
> >you update your cvsync binary.
> >
> >Unfortunately, the fix wasn't quite right.  We ran into problems
> >with the synching of commitid files. naddy managed to cook
> >a correct fix for us.
> >
> >anoncvs.ca (the fanout machine) has been fixed - again. What you
> >need to do is to update your cvsync to the latest one that was
> >just committed into ports (cvsync-0.25.0pre0 or later). Remove your
> >scanfile (if any).
> >
> >Once you do that, re-run cvsync and you should be back in business.
> 
> Is it known whether this requires a fixed upstream cvsync first, or if 
> it is ok to apply the fix and your repo will become fine whenever the 
> upstream is fixed?
> 
> Anyone with insight, please feel free to chime in.


The commit message suggests, at least, the server must be
updated:

| Add full support for commitid and bump protocol version
| Old clients will receive updates with commitid stripped out.

HTH,
--patrick


> /Alexander
> 
> >to check a server do:
> >cvs -d anon...@yourserver.org:/cvs log /usr/src/libexec/ld.so/malloc.c
> >|  less  and see if it has r 1.3
> >
> >Thanks, and sorry for the disruption,
> >
> >-Bob
> >
> 

Re: [PATCH]: Convert select() to poll() in amd/info_hes.c

[patrick keshishian]

On Wed, May 07, 2014 at 02:47:32PM +0100, Dimitris Papastamos wrote:
> Hi,
> 
> This piece of code now uses poll() instead of select().
> I have not got round to test this yet, but I will as soon as I have
> a working setup.
> 
> Thoughts?
> 
> ===
> RCS file: /cvs/src/usr.sbin/amd/amd/info_hes.c,v
> retrieving revision 1.13
> diff -u -p -r1.13 info_hes.c
> --- info_hes.c24 Feb 2012 06:19:00 -  1.13
> +++ info_hes.c7 May 2014 13:38:21 -
> @@ -66,7 +66,7 @@
>  #endif
>  
>  static int soacnt;
> -static struct timeval hs_timeout;
> +static int hs_timeout;
>  static int servernum;
>  #endif /* HAS_HESIOD_RELOAD */
>  
> @@ -261,12 +261,11 @@ hs_res_send(char *buf, int buflen, char 
>*/
>   for (retry = _res.retry; retry > 0; retry--) {
>   for (ns = 0; ns < hs_nscount; ns++) {
> - hs_timeout.tv_sec =
> - (_res.retrans << (_res.retry - retry))
> - / hs_nscount;
> - if (hs_timeout.tv_sec <= 0)
> - hs_timeout.tv_sec = 1;
> - hs_timeout.tv_usec = 0;
> + hs_timeout = _res.retrans << (_res.retry - retry);
> + hs_timeout /= hs_nscount;
> + hs_timeout *= 1000;
> + if (hs_timeout <= 0)
> + hs_timeout = 1000;

I imagine the values used here are small in scale, but
I'm not sure. Any concerns of overflow given the new
code is:

 - using an int type for calculations and storage vs
   time_t in the original.
 - breaking the bit-shift, divide and assign statement
   into two: a) bit-shift / assign, b) divide / assign.


--patrick

Re: malloc in libssl/src/apps

[patrick keshishian]

On Sun, May 04, 2014 at 03:50:06PM -0400, Jean-Philippe Ouellet wrote:
> On Sun, May 04, 2014 at 12:17:16PM -0600, Theo de Raadt wrote:
> > We are going to completely ignore diffs which change multiple idioms
> > at once.
> 
> Okay.
> 
> > That is how mistakes get made.
> 
> Yep, more true than I realized.
> 
> 
> Here's a simpler one:
> 
[...]
> Index: speed.c
> ===
> RCS file: /cvs/src/lib/libssl/src/apps/speed.c,v
> retrieving revision 1.38
> diff -u -p -r1.38 speed.c
> --- speed.c   2 May 2014 17:06:46 -   1.38
> +++ speed.c   4 May 2014 19:36:00 -
> @@ -2178,7 +2178,7 @@ do_multi(int multi)
>   int *fds;
>   static char sep[] = ":";
>  
> - fds = malloc(multi * sizeof *fds);
> + fds = reallocarray(NULL, multi, sizeof(int));

how about a check for return value here?
similar to other malloc -> reallocarray changes.

--patrick

>   for (n = 0; n < multi; ++n) {
>   if (pipe(fd) == -1) {
>   fprintf(stderr, "pipe failure\n");
> Index: srp.c
> ===
> RCS file: /cvs/src/lib/libssl/src/apps/srp.c,v
> retrieving revision 1.10
> diff -u -p -r1.10 srp.c
> --- srp.c 24 Apr 2014 12:22:22 -  1.10
> +++ srp.c 4 May 2014 19:36:00 -
> @@ -176,7 +176,8 @@ update_index(CA_DB * db, BIO * bio, char
>   char **irow;
>   int i;
>  
> - if ((irow = (char **) malloc(sizeof(char *) * (DB_NUMBER + 1))) == 
> NULL) {
> + irow = reallocarray(NULL, DB_NUMBER + 1, sizeof(char *));
> + if (irow == NULL)
>   BIO_printf(bio_err, "Memory allocation failure\n");
>   return 0;
>   }
> 

Re: malloc in libssl/src/apps

[patrick keshishian]

On Sun, May 04, 2014 at 02:38:40AM -0400, Jean-Philippe Ouellet wrote:
> Hello,
> 
> I've gone through lib/libssl/src/apps with the goal of making {m,c,re}alloc
> uses more idiomatic, adding error checking in some places where missing,
> and some minor style unification.
> 
> Feedback appreciated, better patches to come after the semester ends.
> 
> 
> Index: apps.c
> ===
> RCS file: /cvs/src/lib/libssl/src/apps/apps.c,v
> retrieving revision 1.45
> diff -u -p -r1.45 apps.c
> --- apps.c3 May 2014 16:03:54 -   1.45
> +++ apps.c4 May 2014 06:07:45 -
> @@ -209,13 +209,12 @@ chopup_args(ARGS * arg, char *buf, int *
>   *argc = 0;
>   *argv = NULL;
>  
> - i = 0;
>   if (arg->count == 0) {
>   arg->count = 20;
> - arg->data = (char **)malloc(sizeof(char *) * arg->count);
> + arg->data = calloc(arg->count, sizeof(char *));
> + if (arg->data == NULL)
> + return 0;
>   }
> - for (i = 0; i < arg->count; i++)
> - arg->data[i] = NULL;
>  
>   num = 0;
>   p = buf;
> @@ -232,8 +231,7 @@ chopup_args(ARGS * arg, char *buf, int *
>   if (num >= arg->count) {
>   char **tmp_p;
>   int tlen = arg->count + 20;
> - tmp_p = (char **) realloc(arg->data,
> - sizeof(char *) * tlen);
> + tmp_p = reallocarray(arg->data, tlen, sizeof(char *));
>   if (tmp_p == NULL)
>   return 0;
>   arg->data = tmp_p;
> @@ -266,7 +264,7 @@ chopup_args(ARGS * arg, char *buf, int *
>   }
>   *argc = num;
>   *argv = arg->data;
> - return (1);
> + return 1;
>  }
>  
>  int
> @@ -404,7 +402,28 @@ password_callback(char *buf, int bufsiz,
>   ok = UI_add_input_string(ui, prompt, ui_flags, buf,
>   PW_MIN_LENGTH, bufsiz - 1);
>   if (ok >= 0 && verify) {
> - buff = (char *) malloc(bufsiz);
> + buff = malloc(bufsiz);
> + /*
> +  * NULL returns appear to be handled by the following:
> +  *
> +  * UI_add_verify_string(x, x, x, buff=NULL, x, x, x) ->
> +  *   general_allocate_string(x, x, x, UIT_VERIFY, x, \
> +  *   result_buf=NULL, x, x, x) ->
> +  * general_allocate_prompt(x, x, x, x, x, NULL) ->
> +  *   ret = NULL;
> +  *   if (type == UIT_VERIFY && result_buf == NULL)
> +  * UIerr(...); and dont touch ret
> +  * returns NULL
> +  *   returns -1
> +  * returns -1
> +  *
> +  * So, we /should/ (maybe) be good. Not calling UIerr()
> +  * could very well have some well-hidden side-effects
> +  * that I would definitly not notice myself, so I'll
> +  * leave this as is without an explicit check here.
> +  * Maybe somebody who knows better than I has a better
> +  * suggestion?
> +  */
>   ok = UI_add_verify_string(ui, prompt, ui_flags, buff,
>   PW_MIN_LENGTH, bufsiz - 1, buf);
>   }
> @@ -1830,26 +1849,30 @@ parse_yesno(const char *str, int def)
>  X509_NAME *
>  parse_name(char *subject, long chtype, int multirdn)
>  {
> - size_t buflen = strlen(subject) + 1;/* to copy the types and
> -  * values into. due to
> -  * escaping, the copy can
> -  * only become shorter */
> - char *buf = malloc(buflen);
> - size_t max_ne = buflen / 2 + 1; /* maximum number of name elements */
> - char **ne_types = malloc(max_ne * sizeof(char *));
> - char **ne_values = malloc(max_ne * sizeof(char *));
> - int *mval = malloc(max_ne * sizeof(int));
> + size_t buflen, max_ne;
> + char **ne_types, **ne_values, *buf, *sp, *bp;
> + int *mval, i, nid, ne_num = 0;
> + X509_NAME *n = NULL;
>  
> - char *sp = subject, *bp = buf;
> - int i, ne_num = 0;
> + /* Due to escaping, buf can never be bigger than subject. */
> + buflen = strlen(subject + 1);
   
In addition to already mentioned mistake in above strlen()

> - X509_NAME *n = NULL;
> - int nid;
> + /* maximum number of name elements */
> + max_ne = buflen / 2 + 1;
> +
> + buf = malloc(buflen);
> + ne_types = malloc(max_ne);
> + ne_values = malloc(max_ne);

above two malloc() calls are inconsistent with origi

Re: malloc in libssl/src/apps

[patrick keshishian]

On Sun, May 04, 2014 at 01:26:18AM -0700, Philip Guenther wrote:
> On Sunday, May 4, 2014, patrick keshishian  wrote:
> 
> > On Sun, May 04, 2014 at 12:29:59AM -0700, Philip Guenther wrote:
> > > On Sun, May 4, 2014 at 12:21 AM, patrick keshishian 
> > > 
> > >wrote:
> > >
> > > > On Sun, May 04, 2014 at 02:38:40AM -0400, Jean-Philippe Ouellet wrote:
> > > >
> > > ...
> > >
> > > > > - if ((irow = (char **)malloc(sizeof(char *) *
> > > > > - (DB_NUMBER + 1))) == NULL) {
> > > > > + irow = reallocarray(NULL, DB_NUMBER + 1, sizeof(char
> > *));
> > > >
> > > > why not use calloc(2)?
> > > >
> > >
> > > Please justify your belief that the existing code is wrong by lack of
> > > memset/bzero.  Please do so for *ALL* the places where you suggested
> > using
> > > calloc() where Jean-Philippe Ouellet's patch suggest reallocarray().
> >
> > How did you deduce my "belief" to be such?
> 
> 
> The difference between reallocarray(NULL, x,  y) and calloc(x, y) is that
> the latter zeros the allocated array. Ergo, your suggestion to use calloc()
> instead of reallocarray() indicates a belief that the array should be
> zeroed, meaning the existing code is wrong.
> 
> 
> 
> > The code was changing malloc(n * size) to reallocarray().
> > Typical "correction" for this usage has been to convert to
> > calloc(2), "in order to avoid possible integer overflows".
> 
> 
> > Seeing that "reallocarray() appeared in OpenBSD 5.6", I am
> > sure it is an equally fine interface.
> 
> 
> Please *support* the use of reallocarray() in code that would otherwise
> have to do the overflow check itself (and possibly screw it up) or use
> calloc() (and zero possibly large chunks of memory unnecessarily)!

That sentence does not make sense. I have read it multiple
times.

I have no idea what you are driving at, other than diverting
attention away from the strlen() mistake (possibly typo) in
the original suggested patch.

That's the real issue any one serious should care about.
Unless, of course, you want to tell me the following two
statements are identical:

1. strlen(subject) + 1;/* original code */
2. strlen(subject + 1);/* suggested patch */

--patrick

Re: malloc in libssl/src/apps

[patrick keshishian]

On Sun, May 04, 2014 at 12:29:59AM -0700, Philip Guenther wrote:
> On Sun, May 4, 2014 at 12:21 AM, patrick keshishian 
> wrote:
> 
> > On Sun, May 04, 2014 at 02:38:40AM -0400, Jean-Philippe Ouellet wrote:
> >
> ...
> 
> > > - if ((irow = (char **)malloc(sizeof(char *) *
> > > - (DB_NUMBER + 1))) == NULL) {
> > > + irow = reallocarray(NULL, DB_NUMBER + 1, sizeof(char *));
> >
> > why not use calloc(2)?
> >
> 
> Please justify your belief that the existing code is wrong by lack of
> memset/bzero.  Please do so for *ALL* the places where you suggested using
> calloc() where Jean-Philippe Ouellet's patch suggest reallocarray().

How did you deduce my "belief" to be such?

The code was changing malloc(n * size) to reallocarray().
Typical "correction" for this usage has been to convert to
calloc(2), "in order to avoid possible integer overflows".

Seeing that "reallocarray() appeared in OpenBSD 5.6", I am
sure it is an equally fine interface.

Any concern over the first strlen() change I pointed out?

--patrick


> Either:
> a) the existing code's lack of memset/bzero is a bug (details?), OR
> b) using calloc() instead of reallocarray() is *wrong*.
> 
> 
> Philip Guenther

Re: malloc in libssl/src/apps

[patrick keshishian]

On Sun, May 04, 2014 at 02:38:40AM -0400, Jean-Philippe Ouellet wrote:
> Hello,
> 
> I've gone through lib/libssl/src/apps with the goal of making {m,c,re}alloc
> uses more idiomatic, adding error checking in some places where missing,
> and some minor style unification.
> 
> Feedback appreciated, better patches to come after the semester ends.
> 
> 
> Index: apps.c
> ===
> RCS file: /cvs/src/lib/libssl/src/apps/apps.c,v
> retrieving revision 1.45
> diff -u -p -r1.45 apps.c
> --- apps.c3 May 2014 16:03:54 -   1.45
> +++ apps.c4 May 2014 06:07:45 -
> @@ -209,13 +209,12 @@ chopup_args(ARGS * arg, char *buf, int *
>   *argc = 0;
>   *argv = NULL;
>  
> - i = 0;
>   if (arg->count == 0) {
>   arg->count = 20;
> - arg->data = (char **)malloc(sizeof(char *) * arg->count);
> + arg->data = calloc(arg->count, sizeof(char *));
> + if (arg->data == NULL)
> + return 0;
>   }
> - for (i = 0; i < arg->count; i++)
> - arg->data[i] = NULL;
>  
>   num = 0;
>   p = buf;
> @@ -232,8 +231,7 @@ chopup_args(ARGS * arg, char *buf, int *
>   if (num >= arg->count) {
>   char **tmp_p;
>   int tlen = arg->count + 20;
> - tmp_p = (char **) realloc(arg->data,
> - sizeof(char *) * tlen);
> + tmp_p = reallocarray(arg->data, tlen, sizeof(char *));
>   if (tmp_p == NULL)
>   return 0;
>   arg->data = tmp_p;
> @@ -266,7 +264,7 @@ chopup_args(ARGS * arg, char *buf, int *
>   }
>   *argc = num;
>   *argv = arg->data;
> - return (1);
> + return 1;
>  }
>  
>  int
> @@ -404,7 +402,28 @@ password_callback(char *buf, int bufsiz,
>   ok = UI_add_input_string(ui, prompt, ui_flags, buf,
>   PW_MIN_LENGTH, bufsiz - 1);
>   if (ok >= 0 && verify) {
> - buff = (char *) malloc(bufsiz);
> + buff = malloc(bufsiz);
> + /*
> +  * NULL returns appear to be handled by the following:
> +  *
> +  * UI_add_verify_string(x, x, x, buff=NULL, x, x, x) ->
> +  *   general_allocate_string(x, x, x, UIT_VERIFY, x, \
> +  *   result_buf=NULL, x, x, x) ->
> +  * general_allocate_prompt(x, x, x, x, x, NULL) ->
> +  *   ret = NULL;
> +  *   if (type == UIT_VERIFY && result_buf == NULL)
> +  * UIerr(...); and dont touch ret
> +  * returns NULL
> +  *   returns -1
> +  * returns -1
> +  *
> +  * So, we /should/ (maybe) be good. Not calling UIerr()
> +  * could very well have some well-hidden side-effects
> +  * that I would definitly not notice myself, so I'll
> +  * leave this as is without an explicit check here.
> +  * Maybe somebody who knows better than I has a better
> +  * suggestion?
> +  */
>   ok = UI_add_verify_string(ui, prompt, ui_flags, buff,
>   PW_MIN_LENGTH, bufsiz - 1, buf);
>   }
> @@ -1830,26 +1849,30 @@ parse_yesno(const char *str, int def)
>  X509_NAME *
>  parse_name(char *subject, long chtype, int multirdn)
>  {
> - size_t buflen = strlen(subject) + 1;/* to copy the types and
> -  * values into. due to
> -  * escaping, the copy can
> -  * only become shorter */
> - char *buf = malloc(buflen);
> - size_t max_ne = buflen / 2 + 1; /* maximum number of name elements */
> - char **ne_types = malloc(max_ne * sizeof(char *));
> - char **ne_values = malloc(max_ne * sizeof(char *));
> - int *mval = malloc(max_ne * sizeof(int));
> + size_t buflen, max_ne;
> + char **ne_types, **ne_values, *buf, *sp, *bp;
> + int *mval, i, nid, ne_num = 0;
> + X509_NAME *n = NULL;
>  
> - char *sp = subject, *bp = buf;
> - int i, ne_num = 0;
> + /* Due to escaping, buf can never be bigger than subject. */
> + buflen = strlen(subject + 1);
   
Fail!

>  
> - X509_NAME *n = NULL;
> - int nid;
> + /* maximum number of name elements */
> + max_ne = buflen / 2 + 1;
> +
> + buf = malloc(buflen);
> + ne_types = malloc(max_ne);
> + ne_values = malloc(max_ne);
> + mval = reallocarray(NULL, max_ne, sizeof(int));

why not use calloc(2)?

>  
>   if (!buf 

Re: vfs references to strncpy and MFSNAMELEN

[patrick keshishian]

On 4/29/14, H??ctor Luis Gimbatti  wrote:
> The constant MFSNAMELEN as defined in: 
> 
> lib/libc/sys/getfsstat.2:#define MFSNAMELEN   16 
> lib/libc/sys/statfs.2:#define MFSNAMELEN   16
> sys/sys/mount.h: #define MFSNAMELEN  16
> 
> defines the fs type name and, according to comments, it includes nul
> terminating character.
> 
>  The following code makes uses of strncpy and involves MFSNAMELEN
> 
> ./sys/kern/vfs_subr.c:225:  strncpy(mp->mnt_stat.f_fstypename,
> vfsp->vfc_name, MFSNAMELEN);
> ./sys/kern/vfs_subr.c:2272: strncpy(sbp->f_fstypename,
> mp->mnt_vfc->vfc_name, MFSNAMELEN);
> ./sys/kern/vfs_syscalls.c:243:  strncpy(mp->mnt_stat.f_fstypename,
> vfsp->vfc_name, MFSNAMELEN);
> ./sys/miscfs/procfs/procfs_vfsops.c:190:strncpy(sbp->f_fstypename,
> mp->mnt_vfc->vfc_name, MFSNAMELEN);
> ./sys/msdosfs/msdosfs_vfsops.c:669: strncpy(sbp->f_fstypename,
> mp->mnt_vfc->vfc_name, MFSNAMELEN);
> ./sys/nfs/nfs_vfsops.c:646: strncpy(&mp->mnt_stat.f_fstypename[0],
> mp->mnt_vfc->vfc_name, MFSNAMELEN);
> ./sys/ntfs/ntfs_vfsops.c:628:   strncpy(sbp->f_fstypename,
> mp->mnt_vfc->vfc_name, MFSNAMELEN);
> ./sys/ufs/ext2fs/ext2fs_vfsops.c:704:   strncpy(sbp->f_fstypename,
> mp->mnt_vfc->vfc_name, MFSNAMELEN);
> ./sys/ufs/mfs/mfs_vfsops.c:222: strncpy(&sbp->f_fstypename[0],
> mp->mnt_vfc->vfc_name, MFSNAMELEN);
> 
> 
> Can be those replace safely by strlcpy without any modification to
> MFSNAMELEN constant? 

I thought I had seen someone reply to this on tech@ but
I can't find any evidence of that. So... *ping*?


Most places use strn{cmp,cpy} except for compat/linux/linux_misc.c
so it "seems" OK. However, OP's grep missed the following usage
which (I imagine) expects a nul-terminated c-string:

sys/kern/vfs_subr.c in vfs_mount_print()

(*pr)("  fstype \"%s\" mnton \"%s\" mntfrom \"%s\" mntspec \"%s\"\n",
mp->mnt_stat.f_fstypename, mp->mnt_stat.f_mntonname,
mp->mnt_stat.f_mntfromname, mp->mnt_stat.f_mntfromspec);

Called from ddb/db_command.c: db_show_all_{mount,vnode}s()

Producing a patch as OP suggested is simple enough, but
testing for regression is a bit of an unknown to me.

--patrick

Re: [PATCH| zero a freed pointer passed in a struct, to prevent reuse after free

[patrick keshishian]

On Tue, Apr 22, 2014 at 10:33:55PM -0600, Bob Beck wrote:
> Yes, ok
> 
> committed
> 
> 
> On Wed, Apr 23, 2014 at 03:55:19AM +0200, Dirk Engling wrote:
> > Index: x_x509.c
> > ===
> > RCS file: /cvs/src/lib/libssl/src/crypto/asn1/x_x509.c,v
> > retrieving revision 1.12
> > diff -u -r1.12 x_x509.c
> > --- x_x509.c18 Apr 2014 11:20:32 -  1.12
> > +++ x_x509.c23 Apr 2014 01:54:03 -
> > @@ -125,6 +125,7 @@
> >  #endif
> > if (ret->name != NULL)
> > free(ret->name);
> > +   ret->name = NULL;
> > break;
> > }

Why not kill the 'if (ret->name != NULL)' check while at it?

Re: segfault in dhclient 5.4 please help

[patrick keshishian]

On 4/14/14, sven falempin  wrote:
[..]
>>> OpenBSD 5.4 (GENERIC) #37: Tue Jul 30 12:05:01 MDT 2013
>>> dera...@i386.openbsd.org:/usr/src/sys/arch/i386/compile/GENERIC
[...]
> so i got gdb back to the machine because i cannot reproduce outside of the
> box.
> gdb too old cannot gcore.
>
> The state is nasty, but i do get the trace of the dhcp transaction.
>
> [..]
> DHCPREQUEST on trunk0 to 255.255.255.255 port 67
> DHCPACK from 10.0.0.254 (96:4f:87:9c:ad:67)
>
> Program received signal SIGSEGV, Segmentation fault.
> 0x1c005b26 in add_classless_static_routes (rdomain=13684944,
> classless_static_routes=0x0) at /usr/src/sbin/dhclient/dhclient.c:2408
> 2408/usr/src/sbin/dhclient/dhclient.c: No such file or directory.
> in /usr/src/sbin/dhclient/dhclient.c
> (gdb) bt
> #0  0x1c005b26 in add_classless_static_routes (rdomain=13684944,
> classless_static_routes=0x0) at /usr/src/sbin/dhclient/dhclient.c:2408

that rdomain value looks awful funny.

You aren't by chance mixing binaries pre/post time_t
change?

But, don't mind me too much. Wait for someone with
actual knowledge in this area.

--patrick

Re: Switch OpenBSD manuals to DocBook

[patrick keshishian]

On 4/1/14, Hendrickson, Kenneth  wrote:
>
> On 4/1/2014, Patrick Keshishian wrote:
>> sorry to crash your party, but i think you've got something there
>> with the usage() example. this could reduce man executable to
>> a one line shell script (or a builtin):
>>
>> $ cat /usr/bin/man
>> #!/bin/sh
>> while [ $# -gt 0 ] ; do $1 -h | ${PAGER:-more} ; shift ; done
>>
>> Yeah?
>
> Not for man -k (apropros).

Craps! ... maybe someone smarter can figure out a solution
for that, like run through $PATH, and recursively run man on
each executable found, pipe through fancy grep & sed ...

regardless, I don't think we should give up on this! This could
possibly be a GSoC project for someone ... even.

Re: Switch OpenBSD manuals to DocBook

[patrick keshishian]

On 4/1/14, Theo de Raadt  wrote:
>> In the short-run, can't you achieve your goal the same way with this:
>>
>>   pod2mdoc -> doclifter -> docbook2mdoc -> man
>>
>> Eventually, we can re-write man to accept more sophisticated formats
>> directly instead of being tied to mdoc(7), but this would be transparent
>> to end-users.
>>
>> Thoughts?
>
> Another approach is to extend the usage() in every program so that it
> provides more information.
>
> For example, ssh-keygen(1)'s usage spits out:
>
> % ssh-keygen -?
> usage: ssh-keygen [options]
> Options:
>   -A  Generate non-existent host keys for all key types.
>   -a number   Number of KDF rounds for new key format or moduli primality
> tests.
>   -B  Show bubblebabble digest of key file.
>   -b bits Number of bits in the key to create.
>   -C comment  Provide new comment.
>   -c  Change comment in private and public key files.
>   -D pkcs11   Download public key from pkcs11 token.
>   -e  Export OpenSSH to foreign format key file.
>   -F hostname Find hostname in known hosts file.
>   -f filename Filename of the key file.
>   -G file Generate candidates for DH-GEX moduli.
>   -g  Use generic DNS resource record format.
>   -H  Hash names in known_hosts file.
>   -h  Generate host certificate instead of a user certificate.
>   -I key_id   Key identifier to include in certificate.
>   -i  Import foreign format to OpenSSH key file.
>   -J number   Screen this number of moduli lines.
>   -j number   Start screening moduli at specified line.
>   -K checkpt  Write checkpoints to this file.
>   -k  Generate a KRL file.
>   -L  Print the contents of a certificate.
>   -l  Show fingerprint of key file.
>   -M memory   Amount of memory (MB) to use for generating DH-GEX moduli.
>   -m key_fmt  Conversion format for -e/-i (PEM|PKCS8|RFC4716).
>   -N phrase   Provide new passphrase.
>   -n name,... User/host principal names to include in certificate
>   -O option   Specify a certificate option.
>   -o  Enforce new private key format.
>   -P phrase   Provide old passphrase.
>   -p  Change passphrase of private key file.
>   -Q  Test whether key(s) are revoked in KRL.
>   -q  Quiet.
>   -R hostname Remove host from known_hosts file.
>   -r hostname Print DNS resource record.
>   -S startStart point (hex) for generating DH-GEX moduli.
>   -s ca_key   Certify keys with CA key.
>   -T file Screen candidates for DH-GEX moduli.
>   -t type Specify type of key to create.
>   -u  Update KRL rather than creating a new one.
>   -V from:to  Specify certificate validity interval.
>   -v  Verbose.
>   -W gen  Generator to use for generating DH-GEX moduli.
>   -y  Read private key file and print public key.
>   -Z cipher   Specify a cipher for new private key format.
>   -z serial   Specify a serial number.
>
> It probably is not much more work to add the remaining bits from
> the old (crufty) manual page to the usage() code in the program.

sorry to crash your party, but i think you've got something there
with the usage() example. this could reduce man executable to
a one line shell script (or a builtin):

$ cat /usr/bin/man
#!/bin/sh
while [ $# -gt 0 ] ; do $1 -h | ${PAGER:-more} ; shift ; done


Yeah?

--patrick


> Then the semantic markup would all be in one place -- in the source
> files.  There would be no need for seperate manual pages, and the
> pipeline could be simply:
>
> ssh-keygen -? -> usage2pod -> pod2mdoc -> doclifter -> docbook2mdoc ->
> man
>
> More food for thought.
>
>

Re: dd oldebcdic is old

[patrick keshishian]

On 3/24/14, Ted Unangst  wrote:
> Last call, convert your oldebcdic tapes (not to be confused with old
> ebcdic tapes) now before it's too late.

Just curious why this removal? Saving space?

--patrick


> Index: args.c
> ===
> RCS file: /cvs/src/bin/dd/args.c,v
> retrieving revision 1.23
> diff -u -p -r1.23 args.c
> --- args.c24 Mar 2014 21:42:41 -  1.23
> +++ args.c24 Mar 2014 21:56:08 -
> @@ -275,9 +275,6 @@ static const struct conv {
>   { "ebcdic", C_EBCDIC,   C_ASCII,a2e_POSIX },
>   { "ibm",C_EBCDIC,   C_ASCII,a2ibm_POSIX },
>   { "lcase",  C_LCASE,C_UCASE,NULL },
> - { "oldascii",   C_ASCII,C_EBCDIC,   e2a_32V },
> - { "oldebcdic",  C_EBCDIC,   C_ASCII,a2e_32V },
> - { "oldibm", C_EBCDIC,   C_ASCII,a2ibm_32V },
>   { "osync",  C_OSYNC,C_BS,   NULL },
>   { "swab",   C_SWAB, 0,  NULL },
>   { "sync",   C_SYNC, 0,  NULL },
> Index: conv_tab.c
> ===
> RCS file: /cvs/src/bin/dd/conv_tab.c,v
> retrieving revision 1.5
> diff -u -p -r1.5 conv_tab.c
> --- conv_tab.c27 Oct 2009 23:59:21 -  1.5
> +++ conv_tab.c24 Mar 2014 21:56:08 -
> @@ -37,11 +37,7 @@
>  #include 
>
>  /*
> - * There are currently six tables:
> - *
> - *   ebcdic  -> ascii32V conv=oldascii
> - *   ascii   -> ebcdic   32V conv=oldebcdic
> - *   ascii   -> ibm ebcdic   32V conv=oldibm
> + * There are currently three tables:
>   *
>   *   ebcdic  -> asciiPOSIX/S5conv=ascii
>   *   ascii   -> ebcdic   POSIX/S5conv=ebcdic
> @@ -52,8 +48,7 @@
>   *
>   * Tables used for conversions to/from IBM and EBCDIC to support an
> extension
>   * to POSIX P1003.2/D11. The tables referencing POSIX contain data
> extracted
> - * from tables 4-3 and 4-4 in P1003.2/Draft 11.  The historic tables were
> - * constructed by running against a file with all possible byte values.
> + * from tables 4-3 and 4-4 in P1003.2/Draft 11.
>   *
>   * More information can be obtained in "Correspondences of 8-Bit and
> Hollerith
>   * Codes for Computer Environments-A USASI Tutorial", Communications of
> the
> @@ -61,114 +56,6 @@
>   */
>
>  u_char casetab[256];
> -
> -/* EBCDIC to ASCII -- 32V compatible. */
> -const u_char e2a_32V[] = {
> - , 0001, 0002, 0003, 0234, 0011, 0206, 0177, /*  */
> - 0227, 0215, 0216, 0013, 0014, 0015, 0016, 0017, /* 0010 */
> - 0020, 0021, 0022, 0023, 0235, 0205, 0010, 0207, /* 0020 */
> - 0030, 0031, 0222, 0217, 0034, 0035, 0036, 0037, /* 0030 */
> - 0200, 0201, 0202, 0203, 0204, 0012, 0027, 0033, /* 0040 */
> - 0210, 0211, 0212, 0213, 0214, 0005, 0006, 0007, /* 0050 */
> - 0220, 0221, 0026, 0223, 0224, 0225, 0226, 0004, /* 0060 */
> - 0230, 0231, 0232, 0233, 0024, 0025, 0236, 0032, /* 0070 */
> - 0040, 0240, 0241, 0242, 0243, 0244, 0245, 0246, /* 0100 */
> - 0247, 0250, 0133, 0056, 0074, 0050, 0053, 0041, /* 0110 */
> - 0046, 0251, 0252, 0253, 0254, 0255, 0256, 0257, /* 0120 */
> - 0260, 0261, 0135, 0044, 0052, 0051, 0073, 0136, /* 0130 */
> - 0055, 0057, 0262, 0263, 0264, 0265, 0266, 0267, /* 0140 */
> - 0270, 0271, 0174, 0054, 0045, 0137, 0076, 0077, /* 0150 */
> - 0272, 0273, 0274, 0275, 0276, 0277, 0300, 0301, /* 0160 */
> - 0302, 0140, 0072, 0043, 0100, 0047, 0075, 0042, /* 0170 */
> - 0303, 0141, 0142, 0143, 0144, 0145, 0146, 0147, /* 0200 */
> - 0150, 0151, 0304, 0305, 0306, 0307, 0310, 0311, /* 0210 */
> - 0312, 0152, 0153, 0154, 0155, 0156, 0157, 0160, /* 0220 */
> - 0161, 0162, 0313, 0314, 0315, 0316, 0317, 0320, /* 0230 */
> - 0321, 0176, 0163, 0164, 0165, 0166, 0167, 0170, /* 0240 */
> - 0171, 0172, 0322, 0323, 0324, 0325, 0326, 0327, /* 0250 */
> - 0330, 0331, 0332, 0333, 0334, 0335, 0336, 0337, /* 0260 */
> - 0340, 0341, 0342, 0343, 0344, 0345, 0346, 0347, /* 0270 */
> - 0173, 0101, 0102, 0103, 0104, 0105, 0106, 0107, /* 0300 */
> - 0110, 0111, 0350, 0351, 0352, 0353, 0354, 0355, /* 0310 */
> - 0175, 0112, 0113, 0114, 0115, 0116, 0117, 0120, /* 0320 */
> - 0121, 0122, 0356, 0357, 0360, 0361, 0362, 0363, /* 0330 */
> - 0134, 0237, 0123, 0124, 0125, 0126, 0127, 0130, /* 0340 */
> - 0131, 0132, 0364, 0365, 0366, 0367, 0370, 0371, /* 0350 */
> - 0060, 0061, 0062, 0063, 0064, 0065, 0066, 0067, /* 0360 */
> - 0070, 0071, 0372, 0373, 0374, 0375, 0376, 0377, /* 0370 */
> 

[diff] calendar.birthday

[patrick keshishian]

Index: calendar.birthday
===
RCS file: /cvs/obsd/src/usr.bin/calendar/calendars/calendar.birthday,v
retrieving revision 1.57
diff -u -p -u -p -r1.57 calendar.birthday
--- calendar.birthday   11 Feb 2014 12:20:34 -  1.57
+++ calendar.birthday   6 Mar 2014 04:37:08 -
@@ -77,6 +77,7 @@
 02/17  T. J. Watson, Sr. born, 1874
 02/17  Marion Anderson born, 1902
 02/18  Ernst Mach born, 1838, philosopher & optics pioneer
+02/18  Michelangelo Buonarroti dies in Rome, Italy, 1564
 02/19  Nicolas Copernicus born in Thorn, Poland, 1473
 02/20  Ludwig Boltzmann born, 1844, atomic physics pioneer
 02/21  Alexis De Rochon born, 1838, developed the spyglass

Re: panic: __mp_lock_held(&sched_lock) 2013-DEC-27 snapshot

[patrick keshishian]

On Wed, Jan 29, 2014 at 07:50:34AM +0100, J??r??mie Courr??ges-Anglas wrote:
> patrick keshishian  writes:
> 
> > On Tue, Jan 28, 2014 at 09:02:31PM -0800, Philip Guenther wrote:
> >> On Tue, Jan 28, 2014 at 4:55 PM, patrick keshishian  
> >> wrote:
> >> > Side note 1:
> >> > Incidentally, I couldn't figure out how to correctly leave
> >> > a symbol unresolved while compiling myapp, until dlopen()
> >> > time.
> >> 
> >> If you can include the shared-object/library in the link command line,
> >> then the symbol can be referenced at link time.  That lets you refer
> >> to the symbols directly as if they were in the executable itself,
> >> though you must still declare them for the compiler to know what type
> >> they are, etc.
> >
> > A bit of a chicken-and-egg situation for this case. Unless
> > I do some ugly "hopping" around in Makefiles, it would be
> > difficult to include the object on the link line for the
> > main program.
> >
> >> If you need to delay the loading of the shared-object until after
> >> process startup via dlopen(), then the executable should use dlsym()
> >> to get the symbol address, passing it the handle that dlopen()
> >> returned.
> >
> > Yes. I do that right now with a bit of "dance". However,
> > reading a bit about __attribute__((weak)) on-line, I
> > thought, there may be an easier way to achieve this. But,
> > I quite possibly misunderstood the use of that attribute.
> 
> It sounds like you're not using dlsym as intended, then.  If you used:
> 
>   void (*my_function)(void);
> 
>   my_function = dlsym(handle, "target_function");
>   if (my_function == NULL)
>   oops();
>   (*my_function)();

Hi,

That is what I am doing right now. This method, allows
the main program to compile just fine as the storage is
reserved for the "my_function" (in your example). While,
I had initially hoped one could have objects (e.g., int
types, or structs) defined in the yet-to-be-loaded .so
object file, referenced in the main program sources,
and be able get the main program to link; then at run
time, the .so object is loaded, and symbols updated/
resolved, just like the (system) dynamic linker
(implicitly) does at program start up.

Thanks for all your replies!

--patrick