Date: Wed, 18 Jan 2012 17:10:04 -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: <8DD4D210-28DA-453F-86D0-6FC4FB84AC04@palisadesystems.com> In-Reply-To: <20120117201411.B1819@besplex.bde.org> References: <201201122249.q0CMnaZe030200@svn.freebsd.org> <20120114204720.Q1458@besplex.bde.org> <20120114182758.GJ1694@garage.freebsd.pl> <20120115073823.O843@besplex.bde.org> <52A73054-9960-403B-B2FE-857C8801D129@palisadesystems.com> <20120116175627.GA1674@garage.freebsd.pl> <20120117201411.B1819@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Jan 17, 2012, at 3:58 AM, Bruce Evans wrote:
> On Mon, 16 Jan 2012, Pawel Jakub Dawidek wrote:
>=20
>> On Mon, Jan 16, 2012 at 11:14:32AM -0600, Guy Helmer wrote:
>>> 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.
>>=20
>> The patch looks mostly good, one nit mentioned below and also one
>> question for Bruce.
>>=20
>>> +/* Flags for hexdump(3) */
>>> +/* Flags for humanize_number(3) flags */
>>> +/* Flags for humanize_number(3) scale */
>>> +/* return values from realhostname() */
>>> +/* Flags for pw_scan() */
>>> +/* Return values from uu_lock() */
>>=20
>> All those sentences are missing period and one doesn't start with
>> capital letter.
>=20
> I decided not to worry about the termination since it was fairly =
consistent
> in the file. The most recent commit fixed these, but made all the =
others
> inconsistent:
>=20
> % /* for properties.c */
> % /* Avoid pulling in all the include files for no need */
> % #ifdef _STDIO_H_ /* avoid adding new includes */
>=20
> This is now the only comment to the right of code. Comments to the
> right of ifdefs are especially hard format nicely
> (the normal comment indentation to 32 or 40 columns doesn't work
> well; I normally use a single space; the above indents to 24 =
columns),
> so the should be avoided. The same treatment is used for 3 other =
includes,
> but there is no comment for the others. The simplest fix is to remove
> this comment. Otherwise, put a meta-comment about them all somewhere.
> It's hard to place this so that it clearly covers them all, but =
anywhere
> is better than to the right of 1 of their ifdefs.
>=20
> % /* fparseln(3) */
>=20
> This is also missing the new, otherwise (too) uniform wording
> "Flags for..."
>=20
>> I noticed also one more inconsistency:
>>=20
>> struct kinfo_file *
>> kinfo_getfile(pid_t _pid, int *_cntp);
>> struct passwd
>> *pw_dup(const struct passwd *_pw);
>>=20
>> Sometimes * is on the same line as function type and sometimes it is =
in
>> the line below. Former is definiately better.
>=20
> Indded.
>=20
OK, one more round of proposed changes:
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 230318)
+++ lib/libutil/libutil.h (working copy)
@@ -71,14 +71,14 @@
#define PROPERTY_MAX_NAME 64
#define PROPERTY_MAX_VALUE 512
=20
-/* for properties.c */
+/* For properties.c. */
typedef struct _property {
struct _property *next;
char *name;
char *value;
} *properties;
=20
-/* Avoid pulling in all the include files for no need */
+/* Avoid pulling in all the include files for no need. */
struct in_addr;
struct pidfh;
struct sockaddr;
@@ -132,7 +132,11 @@
int uu_unlock(const char *_ttyname);
int uu_lock_txfr(const char *_ttyname, pid_t _pid);
=20
-#ifdef _STDIO_H_ /* avoid adding new includes */
+/*
+ * Conditionally prototype the following functions if the include
+ * files upon which they depend have been included.
+ */
+#ifdef _STDIO_H_
char *fparseln(FILE *_fp, size_t *_len, size_t *_lineno,
const char _delim[3], int _flags);
#endif
@@ -150,26 +154,26 @@
char *pw_make_v7(const struct passwd *_pw);
int pw_mkdb(const char *_user);
int pw_lock(void);
-struct passwd
- *pw_scan(const char *_line, int _flags);
-const char
- *pw_tempname(void);
+struct passwd *
+ pw_scan(const char *_line, int _flags);
+const char *
+ pw_tempname(void);
int pw_tmp(int _mfd);
#endif
=20
#ifdef _GRP_H_
int gr_copy(int __ffd, int _tfd, const struct group *_gr,
struct group *_old_gr);
-struct group
- *gr_dup(const struct group *_gr);
+struct group *
+ gr_dup(const struct group *_gr);
int gr_equal(const struct group *_gr1, const struct group *_gr2);
void gr_fini(void);
int gr_init(const char *_dir, const char *_master);
int gr_lock(void);
char *gr_make(const struct group *_gr);
int gr_mkdb(void);
-struct group
- *gr_scan(const char *_line);
+struct group *
+ gr_scan(const char *_line);
int gr_tmp(int _mdf);
#endif
=20
@@ -209,18 +213,18 @@
#define HD_OMIT_HEX (1 << 17)
#define HD_OMIT_CHARS (1 << 18)
=20
-/* Flags for humanize_number(3) flags. */
+/* Values for humanize_number(3)'s flags parameter. */
#define HN_DECIMAL 0x01
#define HN_NOSPACE 0x02
#define HN_B 0x04
#define HN_DIVISOR_1000 0x08
#define HN_IEC_PREFIXES 0x10
=20
-/* Flags for humanize_number(3) scale. */
+/* Values for humanize_number(3)'s scale parameter. */
#define HN_GETSCALE 0x10
#define HN_AUTOSCALE 0x20
=20
-/* return values from realhostname(). */
+/* Return values from realhostname(). */
#define HOSTNAME_FOUND 0
#define HOSTNAME_INCORRECTNAME 1
#define HOSTNAME_INVALIDADDR 2
@@ -233,12 +237,12 @@
/* 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
+#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)
=20
#endif /* !_LIBUTIL_H_ */
--------
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?8DD4D210-28DA-453F-86D0-6FC4FB84AC04>