Date: Mon, 16 Jan 2012 11:14:32 -0600 From: Guy Helmer <guy.helmer@palisadesystems.com> To: Bruce Evans <brde@optusnet.com.au> Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org, Pawel Jakub Dawidek <pjd@FreeBSD.org> Subject: Re: svn commit: r230037 - head/lib/libutil Message-ID: <52A73054-9960-403B-B2FE-857C8801D129@palisadesystems.com> In-Reply-To: <20120115073823.O843@besplex.bde.org> References: <201201122249.q0CMnaZe030200@svn.freebsd.org> <20120114204720.Q1458@besplex.bde.org> <20120114182758.GJ1694@garage.freebsd.pl> <20120115073823.O843@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Jan 14, 2012, at 3:02 PM, Bruce Evans wrote:
> On Sat, 14 Jan 2012, Pawel Jakub Dawidek wrote:
>=20
>> On Sat, Jan 14, 2012 at 09:59:27PM +1100, Bruce Evans wrote:
>>> ...
>>> It's good to declare mode_t, since pidfile_open() uses it and we =
want
>>> to remove the dependency on <sys/param.h>. However, this definition
>>> doesn't follow KNF or the style of all the other typedef =
declarations
>>> in the file, since all the others follow KNF and thus have a space
>>> instead of a tab after #define and also after typedef.
>>=20
>> I think you mixed space with tab. All the others have a tab after
>> #define and typedef. I fully agree this should be consistent.
>=20
> Oops.
>=20
>>>> -#ifdef _SYS_PARAM_H_
>>>> int pidfile_close(struct pidfh *_pfh);
>>>> int pidfile_fileno(const struct pidfh *_pfh);
>>>> struct pidfh *
>>>> pidfile_open(const char *_path, mode_t _mode, pid_t *_pidptr);
>>>> int pidfile_remove(struct pidfh *_pfh);
>>>> int pidfile_write(struct pidfh *_pfh);
>>>> -#endif
>>>=20
>>> Now these are unsorted, since a separate section to hold them is not
>>> needed. It was used just to make the ifdef easier to read (we don't
>>> want to split up the main list with blank lines around each ifdef, =
and
>>> without such blank lines the ifdefs are harder to read).
>>=20
>> I'd prefer not to change that. All those functions are part of =
pidfile(3)
>> API and it would be better, IMHO, to keep them together here too.
>=20
> The functions have a unique prefix, so they are grouped nicely when =
sorted
> into a long list.
>=20
> While I'm here, I'll complain about the verboseness of that prefix =
:-).
> Other APIs in the file mostly use short prefixes:
> - kinfo_. Should have been ki_ like its struct member names. pidfile =
uses
> a good prefix for its struct member names too.
> - properties_/property_. Bad, like the rest of the API.
> - uu_. A weird nondescriptive name for serial device locking =
protocol.
> Is it from uucp? But its weirdness makes it memorable, unlike a
> generic English word like `property'. Better yet, I don't have to
> quote it here.
> - f. Stdio's prefix meaning `file'. To fit indentifiers in 8 =
characters,
> it can't even have an underscore.
> - pw_. Old prefix/abbrieviation for `password'. It's more readable =
than
> `password' once you are used to it.
> - gr_. Newer prefix for `group'. More verbose than the g in gid.
> - quota_. At least the English word is short.
>=20
> Just noticed some more disorder: the groups of the defines at the end
> are in random (mostly historical) order (U*, HO*, F*, PW*, HN* (for
> the last parameter of humanize_number()), HN* (for the second last
> parameter...), HD*.
>=20
> If the pidfile API had defines and if the API is kept in its own
> section, its defines should be in that section. Most of the other =
APIs
> that have a man page are large enough to deserve the same treatment
> if it is done for pidfile. Some like dehumanize^Wscientificize^W
> humanize_number() are larger although they have fewer functions, since
> they have lots of defines.
>=20
> Bruce
I've pasted the diff below that I think captures the majority of the =
issues you have brought up. I have not attempted to tackle the =
property.3/properties.3 issues, nor the objections to the prefixes that =
I think would take considerably more effort to resolve -- I wanted to =
concentrate on the issues that can be isolated to libutil. I hope the =
diff was pasted OK, especially WRT <tab> characters.
Guy
Index: lib/libutil/property.3
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- lib/libutil/property.3 (revision 230221)
+++ lib/libutil/property.3 (working copy)
@@ -36,7 +36,6 @@
.Sh LIBRARY
.Lb libutil
.Sh SYNOPSIS
-.In sys/types.h
.In libutil.h
.Ft properties
.Fn properties_read "int fd"
Index: lib/libutil/libutil.h
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- lib/libutil/libutil.h (revision 230221)
+++ lib/libutil/libutil.h (working copy)
@@ -49,8 +49,8 @@
#endif
=20
#ifndef _MODE_T_DECLARED
-typedef __mode_t mode_t;
-#define _MODE_T_DECLARED
+typedef __mode_t mode_t;
+#define _MODE_T_DECLARED
#endif
=20
#ifndef _PID_T_DECLARED
@@ -68,8 +68,8 @@
#define _UID_T_DECLARED
#endif
=20
-#define PROPERTY_MAX_NAME 64
-#define PROPERTY_MAX_VALUE 512
+#define PROPERTY_MAX_NAME 64
+#define PROPERTY_MAX_VALUE 512
=20
/* for properties.c */
typedef struct _property {
@@ -80,9 +80,6 @@
=20
/* Avoid pulling in all the include files for no need */
struct in_addr;
-struct kinfo_file;
-struct kinfo_proc;
-struct kinfo_vmentry;
struct pidfh;
struct sockaddr;
struct termios;
@@ -114,6 +111,12 @@
int login_tty(int _fd);
int openpty(int *_amaster, int *_aslave, char *_name,
struct termios *_termp, struct winsize *_winp);
+int pidfile_close(struct pidfh *_pfh);
+int pidfile_fileno(const struct pidfh *_pfh);
+struct pidfh *
+ pidfile_open(const char *_path, mode_t _mode, pid_t *_pidptr);
+int pidfile_remove(struct pidfh *_pfh);
+int pidfile_write(struct pidfh *_pfh);
void properties_free(properties _list);
char *property_find(properties _list, const char *_name);
properties
@@ -170,13 +173,6 @@
int gr_tmp(int _mdf);
#endif
=20
-int pidfile_close(struct pidfh *_pfh);
-int pidfile_fileno(const struct pidfh *_pfh);
-struct pidfh *
- pidfile_open(const char *_path, mode_t _mode, pid_t *_pidptr);
-int pidfile_remove(struct pidfh *_pfh);
-int pidfile_write(struct pidfh *_pfh);
-
#ifdef _UFS_UFS_QUOTA_H_
struct fstab;
struct quotafile;
@@ -199,22 +195,6 @@
=20
__END_DECLS
=20
-#define UU_LOCK_INUSE (1)
-#define UU_LOCK_OK (0)
-#define UU_LOCK_OPEN_ERR (-1)
-#define UU_LOCK_READ_ERR (-2)
-#define UU_LOCK_CREAT_ERR (-3)
-#define UU_LOCK_WRITE_ERR (-4)
-#define UU_LOCK_LINK_ERR (-5)
-#define UU_LOCK_TRY_ERR (-6)
-#define UU_LOCK_OWNER_ERR (-7)
-
-/* return values from realhostname() */
-#define HOSTNAME_FOUND (0)
-#define HOSTNAME_INCORRECTNAME (1)
-#define HOSTNAME_INVALIDADDR (2)
-#define HOSTNAME_INVALIDNAME (3)
-
/* fparseln(3) */
#define FPARSELN_UNESCESC 0x01
#define FPARSELN_UNESCCONT 0x02
@@ -222,26 +202,43 @@
#define FPARSELN_UNESCREST 0x08
#define FPARSELN_UNESCALL 0x0f
=20
-/* pw_scan() */
-#define PWSCAN_MASTER 0x01
-#define PWSCAN_WARN 0x02
-
-/* humanize_number(3) */
-#define HN_DECIMAL 0x01
-#define HN_NOSPACE 0x02
-#define HN_B 0x04
-#define HN_DIVISOR_1000 0x08
-#define HN_IEC_PREFIXES 0x10
-
-/* maxscale =3D 0x07 */
-#define HN_GETSCALE 0x10
-#define HN_AUTOSCALE 0x20
-
-/* hexdump(3) */
+/* Flags for hexdump(3) */
#define HD_COLUMN_MASK 0xff
#define HD_DELIM_MASK 0xff00
#define HD_OMIT_COUNT (1 << 16)
#define HD_OMIT_HEX (1 << 17)
#define HD_OMIT_CHARS (1 << 18)
=20
+/* Flags for humanize_number(3) flags */
+#define HN_DECIMAL 0x01
+#define HN_NOSPACE 0x02
+#define HN_B 0x04
+#define HN_DIVISOR_1000 0x08
+#define HN_IEC_PREFIXES 0x10
+
+/* Flags for humanize_number(3) scale */
+#define HN_GETSCALE 0x10
+#define HN_AUTOSCALE 0x20
+
+/* return values from realhostname() */
+#define HOSTNAME_FOUND 0
+#define HOSTNAME_INCORRECTNAME 1
+#define HOSTNAME_INVALIDADDR 2
+#define HOSTNAME_INVALIDNAME 3
+
+/* Flags for pw_scan() */
+#define PWSCAN_MASTER 0x01
+#define PWSCAN_WARN 0x02
+
+/* Return values from uu_lock() */
+#define UU_LOCK_INUSE 1
+#define UU_LOCK_OK 0
+#define UU_LOCK_OPEN_ERR -1
+#define UU_LOCK_READ_ERR -2
+#define UU_LOCK_CREAT_ERR -3
+#define UU_LOCK_WRITE_ERR -4
+#define UU_LOCK_LINK_ERR -5
+#define UU_LOCK_TRY_ERR -6
+#define UU_LOCK_OWNER_ERR -7
+
#endif /* !_LIBUTIL_H_ */
Index: lib/libutil/realhostname.3
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- lib/libutil/realhostname.3 (revision 230221)
+++ lib/libutil/realhostname.3 (working copy)
@@ -33,8 +33,6 @@
.Sh LIBRARY
.Lb libutil
.Sh SYNOPSIS
-.In sys/types.h
-.In netinet/in.h
.In libutil.h
.Ft int
.Fn realhostname "char *host" "size_t hsize" "const struct in_addr *ip"
Index: lib/libutil/pidfile.3
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- lib/libutil/pidfile.3 (revision 230221)
+++ lib/libutil/pidfile.3 (working copy)
@@ -36,7 +36,6 @@
.Sh LIBRARY
.Lb libutil
.Sh SYNOPSIS
-.In sys/param.h
.In libutil.h
.Ft "struct pidfh *"
.Fn pidfile_open "const char *path" "mode_t mode" "pid_t *pidptr"
--------
This message has been scanned by ComplianceSafe, powered by Palisade's PacketSure.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?52A73054-9960-403B-B2FE-857C8801D129>