Changes since the first send:
- split to five separate patches
- fixed a bug in cache_get_filehandle() that made result parsing not work
- fixed to check result of write() calls
The review mentioned my patches adding:
nfssvc.c:71:8: warning: ignoring return value of 'system', declared with attribute warn_unused_result [-Wunused-result]
nfssvc.c:325:9: warning: ignoring return value of 'write', declared with attribute warn_unused_result [-Wunused-result]
but this does not make any sense: I'm only adding one #include there.
Is that perhaps uncovering some other issues?
I'm not sure if the NFSv3 related issues were caused by the cache_get_filehandle()
issue or not, so this still needs testing. Another potential cause is that the
kernel is sending to user land requests longer than RPC_CHAN_BUF_SIZE bytes, but
that does not seem likely.
Timo Teräs (5):
Add string.h to source files that need it
mountd: talk to kernel using file descriptors instead of FILE
gssd: talk to kernel using file descriptors instead of FILE
nfsexport: talk to kernel using file descriptors instead of FILE
nfslib: remove now unused FILE helpers
support/include/exportfs.h | 1 +
support/include/nfslib.h | 7 -
support/include/nfsrpc.h | 1 +
support/nfs/cacheio.c | 111 +------------
support/nfs/nfsexport.c | 77 +++++----
utils/gssd/gssd_proc.c | 9 +-
utils/gssd/svcgssd.h | 2 +-
utils/gssd/svcgssd_main_loop.c | 9 +-
utils/gssd/svcgssd_proc.c | 51 +++---
utils/gssd/write_bytes.h | 1 +
utils/mountd/cache.c | 343 ++++++++++++++++++++++-------------------
utils/nfsd/nfssvc.c | 1 +
12 files changed, 270 insertions(+), 343 deletions(-)
--
2.1.2
Signed-off-by: Timo Teräs <[email protected]>
---
support/include/exportfs.h | 1 +
support/include/nfsrpc.h | 1 +
support/nfs/cacheio.c | 1 +
utils/gssd/write_bytes.h | 1 +
utils/nfsd/nfssvc.c | 1 +
5 files changed, 5 insertions(+)
diff --git a/support/include/exportfs.h b/support/include/exportfs.h
index 9021fae..32711e4 100644
--- a/support/include/exportfs.h
+++ b/support/include/exportfs.h
@@ -10,6 +10,7 @@
#define EXPORTFS_H
#include <netdb.h>
+#include <string.h>
#include "sockaddr.h"
#include "nfslib.h"
diff --git a/support/include/nfsrpc.h b/support/include/nfsrpc.h
index 1bfae7a..fbbdb6a 100644
--- a/support/include/nfsrpc.h
+++ b/support/include/nfsrpc.h
@@ -23,6 +23,7 @@
#ifndef __NFS_UTILS_NFSRPC_H
#define __NFS_UTILS_NFSRPC_H
+#include <string.h>
#include <rpc/types.h>
#include <rpc/clnt.h>
diff --git a/support/nfs/cacheio.c b/support/nfs/cacheio.c
index 61e07a8..2726d8f 100644
--- a/support/nfs/cacheio.c
+++ b/support/nfs/cacheio.c
@@ -18,6 +18,7 @@
#include <nfslib.h>
#include <stdio.h>
#include <stdio_ext.h>
+#include <string.h>
#include <ctype.h>
#include <unistd.h>
#include <sys/types.h>
diff --git a/utils/gssd/write_bytes.h b/utils/gssd/write_bytes.h
index 4fc72cc..b3f342b 100644
--- a/utils/gssd/write_bytes.h
+++ b/utils/gssd/write_bytes.h
@@ -32,6 +32,7 @@
#define _WRITE_BYTES_H_
#include <stdlib.h>
+#include <string.h>
#include <sys/types.h>
#include <netinet/in.h> /* for ntohl */
diff --git a/utils/nfsd/nfssvc.c b/utils/nfsd/nfssvc.c
index 0675b6a..027e5ac 100644
--- a/utils/nfsd/nfssvc.c
+++ b/utils/nfsd/nfssvc.c
@@ -20,6 +20,7 @@
#include <fcntl.h>
#include <errno.h>
#include <stdlib.h>
+#include <string.h>
#include "nfslib.h"
#include "xlog.h"
--
2.1.2
All access to kernel is now done using file descriptors.
Signed-off-by: Timo Teräs <[email protected]>
---
support/include/nfslib.h | 7 ---
support/nfs/cacheio.c | 110 -----------------------------------------------
2 files changed, 117 deletions(-)
diff --git a/support/include/nfslib.h b/support/include/nfslib.h
index ce4b14b..9fd22ac 100644
--- a/support/include/nfslib.h
+++ b/support/include/nfslib.h
@@ -152,11 +152,6 @@ struct nfs_fh_len * getfh(const struct sockaddr_in *sin, const char *path);
struct nfs_fh_len * getfh_size(const struct sockaddr_in *sin,
const char *path, int const size);
-void qword_print(FILE *f, char *str);
-void qword_printhex(FILE *f, char *str, int slen);
-void qword_printint(FILE *f, int num);
-int qword_eol(FILE *f);
-int readline(int fd, char **buf, int *lenp);
int qword_get(char **bpp, char *dest, int bufsize);
int qword_get_int(char **bpp, int *anint);
void cache_flush(int force);
@@ -167,8 +162,6 @@ void qword_addint(char **bpp, int *lp, int n);
void qword_adduint(char **bpp, int *lp, unsigned int n);
void qword_addeol(char **bpp, int *lp);
int qword_get_uint(char **bpp, unsigned int *anint);
-void qword_printuint(FILE *f, unsigned int num);
-void qword_printtimefrom(FILE *f, unsigned int num);
void closeall(int min);
diff --git a/support/nfs/cacheio.c b/support/nfs/cacheio.c
index 2726d8f..42e2502 100644
--- a/support/nfs/cacheio.c
+++ b/support/nfs/cacheio.c
@@ -121,74 +121,6 @@ void qword_addeol(char **bpp, int *lp)
(*lp)--;
}
-static char qword_buf[8192];
-void qword_print(FILE *f, char *str)
-{
- char *bp = qword_buf;
- int len = sizeof(qword_buf);
- qword_add(&bp, &len, str);
- if (fwrite(qword_buf, bp-qword_buf, 1, f) != 1) {
- xlog_warn("qword_print: fwrite failed: errno %d (%s)",
- errno, strerror(errno));
- }
-}
-
-void qword_printhex(FILE *f, char *str, int slen)
-{
- char *bp = qword_buf;
- int len = sizeof(qword_buf);
- qword_addhex(&bp, &len, str, slen);
- if (fwrite(qword_buf, bp-qword_buf, 1, f) != 1) {
- xlog_warn("qword_printhex: fwrite failed: errno %d (%s)",
- errno, strerror(errno));
- }
-}
-
-void qword_printint(FILE *f, int num)
-{
- fprintf(f, "%d ", num);
-}
-
-void qword_printuint(FILE *f, unsigned int num)
-{
- fprintf(f, "%u ", num);
-}
-
-void qword_printtimefrom(FILE *f, unsigned int num)
-{
- fprintf(f, "%lu ", time(0) + num);
-}
-
-int qword_eol(FILE *f)
-{
- int err;
-
- err = fprintf(f,"\n");
- if (err < 0) {
- xlog_warn("qword_eol: fprintf failed: errno %d (%s)",
- errno, strerror(errno));
- } else {
- err = fflush(f);
- if (err) {
- xlog_warn("qword_eol: fflush failed: errno %d (%s)",
- errno, strerror(errno));
- }
- }
- /*
- * We must send one line (and one line only) in a single write
- * call. In case of a write error, libc may accumulate the
- * unwritten data and try to write it again later, resulting in a
- * multi-line write. So we must explicitly ask it to throw away
- * any such cached data. But we return any original error
- * indication to the caller.
- */
- __fpurge(f);
- fflush(f);
- return err;
-}
-
-
-
#define isodigit(c) (isdigit(c) && c <= '7')
int qword_get(char **bpp, char *dest, int bufsize)
{
@@ -266,48 +198,6 @@ int qword_get_uint(char **bpp, unsigned int *anint)
return 0;
}
-#define READLINE_BUFFER_INCREMENT 2048
-
-int readline(int fd, char **buf, int *lenp)
-{
- /* read a line into *buf, which is malloced *len long
- * realloc if needed until we find a \n
- * nul out the \n and return
- * 0 on eof, 1 on success
- */
- int len;
-
- if (*lenp == 0) {
- char *b = malloc(READLINE_BUFFER_INCREMENT);
- if (b == NULL)
- return 0;
- *buf = b;
- *lenp = READLINE_BUFFER_INCREMENT;
- }
- len = read(fd, *buf, *lenp);
- if (len <= 0)
- return 0;
- while ((*buf)[len-1] != '\n') {
- /* now the less common case. There was no newline,
- * so we have to keep reading after re-alloc
- */
- char *new;
- int nl;
- *lenp += READLINE_BUFFER_INCREMENT;
- new = realloc(*buf, *lenp);
- if (new == NULL)
- return 0;
- *buf = new;
- nl = read(fd, *buf + len, *lenp - len);
- if (nl <= 0)
- return 0;
- len += nl;
- }
- (*buf)[len-1] = '\0';
- return 1;
-}
-
-
/* Check if we should use the new caching interface
* This succeeds iff the "nfsd" filesystem is mounted on
* /proc/fs/nfs
--
2.1.2
Signed-off-by: Timo Teräs <[email protected]>
---
utils/mountd/cache.c | 343 +++++++++++++++++++++++++++------------------------
1 file changed, 183 insertions(+), 160 deletions(-)
diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
index 663a52a..c23d384 100644
--- a/utils/mountd/cache.c
+++ b/utils/mountd/cache.c
@@ -61,15 +61,13 @@ enum nfsd_fsid {
* Record is terminated with newline.
*
*/
-static int cache_export_ent(char *domain, struct exportent *exp, char *p);
+static int cache_export_ent(char *buf, int buflen, char *domain, struct exportent *exp, char *path);
#define INITIAL_MANAGED_GROUPS 100
-char *lbuf = NULL;
-int lbuflen = 0;
extern int use_ipaddr;
-static void auth_unix_ip(FILE *f)
+static void auth_unix_ip(int f)
{
/* requests are
* class IP-ADDR
@@ -78,23 +76,26 @@ static void auth_unix_ip(FILE *f)
*
* "nfsd" IP-ADDR expiry domainname
*/
- char *cp;
char class[20];
char ipaddr[INET6_ADDRSTRLEN + 1];
char *client = NULL;
struct addrinfo *tmp = NULL;
- if (readline(fileno(f), &lbuf, &lbuflen) != 1)
- return;
+ char buf[RPC_CHAN_BUF_SIZE], *bp;
+ int blen;
+
+ blen = read(f, buf, sizeof(buf));
+ if (blen <= 0 || buf[blen-1] != '\n') return;
+ buf[blen-1] = 0;
- xlog(D_CALL, "auth_unix_ip: inbuf '%s'", lbuf);
+ xlog(D_CALL, "auth_unix_ip: inbuf '%s'", buf);
- cp = lbuf;
+ bp = buf;
- if (qword_get(&cp, class, 20) <= 0 ||
+ if (qword_get(&bp, class, 20) <= 0 ||
strcmp(class, "nfsd") != 0)
return;
- if (qword_get(&cp, ipaddr, sizeof(ipaddr) - 1) <= 0)
+ if (qword_get(&bp, ipaddr, sizeof(ipaddr) - 1) <= 0)
return;
tmp = host_pton(ipaddr);
@@ -113,16 +114,20 @@ static void auth_unix_ip(FILE *f)
freeaddrinfo(ai);
}
}
- qword_print(f, "nfsd");
- qword_print(f, ipaddr);
- qword_printtimefrom(f, DEFAULT_TTL);
+ bp = buf; blen = sizeof(buf);
+ qword_add(&bp, &blen, "nfsd");
+ qword_add(&bp, &blen, ipaddr);
+ qword_adduint(&bp, &blen, time(0) + DEFAULT_TTL);
if (use_ipaddr) {
memmove(ipaddr + 1, ipaddr, strlen(ipaddr) + 1);
ipaddr[0] = '$';
- qword_print(f, ipaddr);
+ qword_add(&bp, &blen, ipaddr);
} else if (client)
- qword_print(f, *client?client:"DEFAULT");
- qword_eol(f);
+ qword_add(&bp, &blen, *client?client:"DEFAULT");
+ qword_addeol(&bp, &blen);
+ if (blen <= 0 || write(f, buf, bp - buf) != bp - buf)
+ xlog(L_ERROR, "auth_unix_ip: error writing reply");
+
xlog(D_CALL, "auth_unix_ip: client %p '%s'", client, client?client: "DEFAULT");
free(client);
@@ -130,7 +135,7 @@ static void auth_unix_ip(FILE *f)
}
-static void auth_unix_gid(FILE *f)
+static void auth_unix_gid(int f)
{
/* Request are
* uid
@@ -144,7 +149,8 @@ static void auth_unix_gid(FILE *f)
gid_t *more_groups;
int ngroups;
int rv, i;
- char *cp;
+ char buf[RPC_CHAN_BUF_SIZE], *bp;
+ int blen;
if (groups_len == 0) {
groups = malloc(sizeof(gid_t) * INITIAL_MANAGED_GROUPS);
@@ -156,11 +162,12 @@ static void auth_unix_gid(FILE *f)
ngroups = groups_len;
- if (readline(fileno(f), &lbuf, &lbuflen) != 1)
- return;
+ blen = read(f, buf, sizeof(buf));
+ if (blen <= 0 || buf[blen-1] != '\n') return;
+ buf[blen-1] = 0;
- cp = lbuf;
- if (qword_get_uint(&cp, &uid) != 0)
+ bp = buf;
+ if (qword_get_uint(&bp, &uid) != 0)
return;
pw = getpwuid(uid);
@@ -180,15 +187,19 @@ static void auth_unix_gid(FILE *f)
}
}
}
- qword_printuint(f, uid);
- qword_printtimefrom(f, DEFAULT_TTL);
+
+ bp = buf; blen = sizeof(buf);
+ qword_adduint(&bp, &blen, uid);
+ qword_adduint(&bp, &blen, time(0) + DEFAULT_TTL);
if (rv >= 0) {
- qword_printuint(f, ngroups);
+ qword_adduint(&bp, &blen, ngroups);
for (i=0; i<ngroups; i++)
- qword_printuint(f, groups[i]);
+ qword_adduint(&bp, &blen, groups[i]);
} else
- qword_printuint(f, 0);
- qword_eol(f);
+ qword_adduint(&bp, &blen, 0);
+ qword_addeol(&bp, &blen);
+ if (blen <= 0 || write(f, buf, bp - buf) != bp - buf)
+ xlog(L_ERROR, "auth_unix_gid: error writing reply");
}
#if USE_BLKID
@@ -659,14 +670,13 @@ static struct addrinfo *lookup_client_addr(char *dom)
return ret;
}
-static void nfsd_fh(FILE *f)
+static void nfsd_fh(int f)
{
/* request are:
* domain fsidtype fsid
* interpret fsid, find export point and options, and write:
* domain fsidtype fsid expiry path
*/
- char *cp;
char *dom;
int fsidtype;
int fsidlen;
@@ -678,24 +688,27 @@ static void nfsd_fh(FILE *f)
nfs_export *exp;
int i;
int dev_missing = 0;
+ char buf[RPC_CHAN_BUF_SIZE], *bp;
+ int blen;
- if (readline(fileno(f), &lbuf, &lbuflen) != 1)
- return;
+ blen = read(f, buf, sizeof(buf));
+ if (blen <= 0 || buf[blen-1] != '\n') return;
+ buf[blen-1] = 0;
- xlog(D_CALL, "nfsd_fh: inbuf '%s'", lbuf);
+ xlog(D_CALL, "nfsd_fh: inbuf '%s'", buf);
- cp = lbuf;
+ bp = buf;
- dom = malloc(strlen(cp));
+ dom = malloc(blen);
if (dom == NULL)
return;
- if (qword_get(&cp, dom, strlen(cp)) <= 0)
+ if (qword_get(&bp, dom, blen) <= 0)
goto out;
- if (qword_get_int(&cp, &fsidtype) != 0)
+ if (qword_get_int(&bp, &fsidtype) != 0)
goto out;
if (fsidtype < 0 || fsidtype > 7)
goto out; /* unknown type */
- if ((fsidlen = qword_get(&cp, fsid, 32)) <= 0)
+ if ((fsidlen = qword_get(&bp, fsid, 32)) <= 0)
goto out;
if (parse_fsid(fsidtype, fsidlen, fsid, &parsed))
goto out;
@@ -796,12 +809,13 @@ static void nfsd_fh(FILE *f)
}
if (found)
- if (cache_export_ent(dom, found, found_path) < 0)
+ if (cache_export_ent(buf, sizeof(buf), dom, found, found_path) < 0)
found = 0;
- qword_print(f, dom);
- qword_printint(f, fsidtype);
- qword_printhex(f, fsid, fsidlen);
+ bp = buf; blen = sizeof(buf);
+ qword_add(&bp, &blen, dom);
+ qword_addint(&bp, &blen, fsidtype);
+ qword_addhex(&bp, &blen, fsid, fsidlen);
/* The fsid -> path lookup can be quite expensive as it
* potentially stats and reads lots of devices, and some of those
* might have spun-down. The Answer is not likely to
@@ -810,20 +824,21 @@ static void nfsd_fh(FILE *f)
* timeout. Maybe this should be configurable on the command
* line.
*/
- qword_printint(f, 0x7fffffff);
+ qword_addint(&bp, &blen, 0x7fffffff);
if (found)
- qword_print(f, found_path);
- qword_eol(f);
- out:
+ qword_add(&bp, &blen, found_path);
+ qword_addeol(&bp, &blen);
+ if (blen <= 0 || write(f, buf, bp - buf) != bp - buf)
+ xlog(L_ERROR, "nfsd_fh: error writing reply");
+out:
if (found_path)
free(found_path);
freeaddrinfo(ai);
free(dom);
xlog(D_CALL, "nfsd_fh: found %p path %s", found, found ? found->e_path : NULL);
- return;
}
-static void write_fsloc(FILE *f, struct exportent *ep)
+static void write_fsloc(char **bp, int *blen, struct exportent *ep)
{
struct servers *servers;
@@ -833,20 +848,20 @@ static void write_fsloc(FILE *f, struct exportent *ep)
servers = replicas_lookup(ep->e_fslocmethod, ep->e_fslocdata);
if (!servers)
return;
- qword_print(f, "fsloc");
- qword_printint(f, servers->h_num);
+ qword_add(bp, blen, "fsloc");
+ qword_addint(bp, blen, servers->h_num);
if (servers->h_num >= 0) {
int i;
for (i=0; i<servers->h_num; i++) {
- qword_print(f, servers->h_mp[i]->h_host);
- qword_print(f, servers->h_mp[i]->h_path);
+ qword_add(bp, blen, servers->h_mp[i]->h_host);
+ qword_add(bp, blen, servers->h_mp[i]->h_path);
}
}
- qword_printint(f, servers->h_referral);
+ qword_addint(bp, blen, servers->h_referral);
release_replicas(servers);
}
-static void write_secinfo(FILE *f, struct exportent *ep, int flag_mask)
+static void write_secinfo(char **bp, int *blen, struct exportent *ep, int flag_mask)
{
struct sec_entry *p;
@@ -857,45 +872,52 @@ static void write_secinfo(FILE *f, struct exportent *ep, int flag_mask)
return;
}
fix_pseudoflavor_flags(ep);
- qword_print(f, "secinfo");
- qword_printint(f, p - ep->e_secinfo);
+ qword_add(bp, blen, "secinfo");
+ qword_addint(bp, blen, p - ep->e_secinfo);
for (p = ep->e_secinfo; p->flav; p++) {
- qword_printint(f, p->flav->fnum);
- qword_printint(f, p->flags & flag_mask);
+ qword_addint(bp, blen, p->flav->fnum);
+ qword_addint(bp, blen, p->flags & flag_mask);
}
}
-static int dump_to_cache(FILE *f, char *domain, char *path, struct exportent *exp)
+static int dump_to_cache(int f, char *buf, int buflen, char *domain, char *path, struct exportent *exp)
{
- qword_print(f, domain);
- qword_print(f, path);
+ char *bp = buf;
+ int blen = buflen;
+ time_t now = time(0);
+
+ qword_add(&bp, &blen, domain);
+ qword_add(&bp, &blen, path);
if (exp) {
int different_fs = strcmp(path, exp->e_path) != 0;
int flag_mask = different_fs ? ~NFSEXP_FSID : ~0;
- qword_printtimefrom(f, exp->e_ttl);
- qword_printint(f, exp->e_flags & flag_mask);
- qword_printint(f, exp->e_anonuid);
- qword_printint(f, exp->e_anongid);
- qword_printint(f, exp->e_fsid);
- write_fsloc(f, exp);
- write_secinfo(f, exp, flag_mask);
- if (exp->e_uuid == NULL || different_fs) {
- char u[16];
- if (uuid_by_path(path, 0, 16, u)) {
- qword_print(f, "uuid");
- qword_printhex(f, u, 16);
- }
- } else {
- char u[16];
- get_uuid(exp->e_uuid, 16, u);
- qword_print(f, "uuid");
- qword_printhex(f, u, 16);
- }
+ qword_adduint(&bp, &blen, now + exp->e_ttl);
+ qword_addint(&bp, &blen, exp->e_flags & flag_mask);
+ qword_addint(&bp, &blen, exp->e_anonuid);
+ qword_addint(&bp, &blen, exp->e_anongid);
+ qword_addint(&bp, &blen, exp->e_fsid);
+ write_fsloc(&bp, &blen, exp);
+ write_secinfo(&bp, &blen, exp, flag_mask);
+ if (exp->e_uuid == NULL || different_fs) {
+ char u[16];
+ if (uuid_by_path(path, 0, 16, u)) {
+ qword_add(&bp, &blen, "uuid");
+ qword_addhex(&bp, &blen, u, 16);
+ }
+ } else {
+ char u[16];
+ get_uuid(exp->e_uuid, 16, u);
+ qword_add(&bp, &blen, "uuid");
+ qword_addhex(&bp, &blen, u, 16);
+ }
} else
- qword_printtimefrom(f, DEFAULT_TTL);
- return qword_eol(f);
+ qword_adduint(&bp, &blen, now + DEFAULT_TTL);
+ qword_addeol(&bp, &blen);
+ if (blen <= 0) return -1;
+ if (write(f, buf, bp - buf) != bp - buf) return -1;
+ return 0;
}
static nfs_export *
@@ -1245,27 +1267,27 @@ static struct exportent *lookup_junction(char *dom, const char *pathname,
return exp;
}
-static void lookup_nonexport(FILE *f, char *dom, char *path,
+static void lookup_nonexport(int f, char *buf, int buflen, char *dom, char *path,
struct addrinfo *ai)
{
struct exportent *eep;
eep = lookup_junction(dom, path, ai);
- dump_to_cache(f, dom, path, eep);
+ dump_to_cache(f, buf, buflen, dom, path, eep);
if (eep == NULL)
return;
exportent_release(eep);
free(eep);
}
#else /* !HAVE_NFS_PLUGIN_H */
-static void lookup_nonexport(FILE *f, char *dom, char *path,
+static void lookup_nonexport(int f, char *buf, int buflen, char *dom, char *path,
struct addrinfo *UNUSED(ai))
{
- dump_to_cache(f, dom, path, NULL);
+ dump_to_cache(f, buf, buflen, dom, path, NULL);
}
#endif /* !HAVE_NFS_PLUGIN_H */
-static void nfsd_export(FILE *f)
+static void nfsd_export(int f)
{
/* requests are:
* domain path
@@ -1273,26 +1295,28 @@ static void nfsd_export(FILE *f)
* domain path expiry flags anonuid anongid fsid
*/
- char *cp;
char *dom, *path;
nfs_export *found = NULL;
struct addrinfo *ai = NULL;
+ char buf[RPC_CHAN_BUF_SIZE], *bp;
+ int blen;
- if (readline(fileno(f), &lbuf, &lbuflen) != 1)
- return;
+ blen = read(f, buf, sizeof(buf));
+ if (blen <= 0 || buf[blen-1] != '\n') return;
+ buf[blen-1] = 0;
- xlog(D_CALL, "nfsd_export: inbuf '%s'", lbuf);
+ xlog(D_CALL, "nfsd_export: inbuf '%s'", buf);
- cp = lbuf;
- dom = malloc(strlen(cp));
- path = malloc(strlen(cp));
+ bp = buf;
+ dom = malloc(blen);
+ path = malloc(blen);
if (!dom || !path)
goto out;
- if (qword_get(&cp, dom, strlen(lbuf)) <= 0)
+ if (qword_get(&bp, dom, blen) <= 0)
goto out;
- if (qword_get(&cp, path, strlen(lbuf)) <= 0)
+ if (qword_get(&bp, path, blen) <= 0)
goto out;
auth_reload();
@@ -1306,14 +1330,14 @@ static void nfsd_export(FILE *f)
found = lookup_export(dom, path, ai);
if (found) {
- if (dump_to_cache(f, dom, path, &found->m_export) < 0) {
+ if (dump_to_cache(f, buf, sizeof(buf), dom, path, &found->m_export) < 0) {
xlog(L_WARNING,
"Cannot export %s, possibly unsupported filesystem"
" or fsid= required", path);
- dump_to_cache(f, dom, path, NULL);
+ dump_to_cache(f, buf, sizeof(buf), dom, path, NULL);
}
} else
- lookup_nonexport(f, dom, path, ai);
+ lookup_nonexport(f, buf, sizeof(buf), dom, path, ai);
out:
xlog(D_CALL, "nfsd_export: found %p path %s", found, path ? path : NULL);
@@ -1325,15 +1349,14 @@ static void nfsd_export(FILE *f)
struct {
char *cache_name;
- void (*cache_handle)(FILE *f);
- FILE *f;
- char vbuf[RPC_CHAN_BUF_SIZE];
+ void (*cache_handle)(int f);
+ int f;
} cachelist[] = {
- { "auth.unix.ip", auth_unix_ip, NULL, ""},
- { "auth.unix.gid", auth_unix_gid, NULL, ""},
- { "nfsd.export", nfsd_export, NULL, ""},
- { "nfsd.fh", nfsd_fh, NULL, ""},
- { NULL, NULL, NULL, ""}
+ { "auth.unix.ip", auth_unix_ip, -1 },
+ { "auth.unix.gid", auth_unix_gid, -1 },
+ { "nfsd.export", nfsd_export, -1 },
+ { "nfsd.fh", nfsd_fh, -1 },
+ { NULL, NULL, -1 }
};
extern int manage_gids;
@@ -1350,11 +1373,7 @@ void cache_open(void)
if (!manage_gids && cachelist[i].cache_handle == auth_unix_gid)
continue;
sprintf(path, "/proc/net/rpc/%s/channel", cachelist[i].cache_name);
- cachelist[i].f = fopen(path, "r+");
- if (cachelist[i].f != NULL) {
- setvbuf(cachelist[i].f, cachelist[i].vbuf, _IOLBF,
- RPC_CHAN_BUF_SIZE);
- }
+ cachelist[i].f = open(path, O_RDWR);
}
}
@@ -1366,8 +1385,8 @@ void cache_set_fds(fd_set *fdset)
{
int i;
for (i=0; cachelist[i].cache_name; i++) {
- if (cachelist[i].f)
- FD_SET(fileno(cachelist[i].f), fdset);
+ if (cachelist[i].f >= 0)
+ FD_SET(cachelist[i].f, fdset);
}
}
@@ -1380,11 +1399,11 @@ int cache_process_req(fd_set *readfds)
int i;
int cnt = 0;
for (i=0; cachelist[i].cache_name; i++) {
- if (cachelist[i].f != NULL &&
- FD_ISSET(fileno(cachelist[i].f), readfds)) {
+ if (cachelist[i].f >= 0 &&
+ FD_ISSET(cachelist[i].f, readfds)) {
cnt++;
cachelist[i].cache_handle(cachelist[i].f);
- FD_CLR(fileno(cachelist[i].f), readfds);
+ FD_CLR(cachelist[i].f, readfds);
}
}
return cnt;
@@ -1397,14 +1416,14 @@ int cache_process_req(fd_set *readfds)
* % echo $domain $path $[now+DEFAULT_TTL] $options $anonuid $anongid $fsid > /proc/net/rpc/nfsd.export/channel
*/
-static int cache_export_ent(char *domain, struct exportent *exp, char *path)
+static int cache_export_ent(char *buf, int buflen, char *domain, struct exportent *exp, char *path)
{
- int err;
- FILE *f = fopen("/proc/net/rpc/nfsd.export/channel", "w");
- if (!f)
- return -1;
+ int f, err;
+
+ f = open("/proc/net/rpc/nfsd.export/channel", O_WRONLY);
+ if (f < 0) return -1;
- err = dump_to_cache(f, domain, exp->e_path, exp);
+ err = dump_to_cache(f, buf, buflen, domain, exp->e_path, exp);
if (err) {
xlog(L_WARNING,
"Cannot export %s, possibly unsupported filesystem or"
@@ -1445,13 +1464,13 @@ static int cache_export_ent(char *domain, struct exportent *exp, char *path)
continue;
dev = stb.st_dev;
path[l] = 0;
- dump_to_cache(f, domain, path, exp);
+ dump_to_cache(f, buf, buflen, domain, path, exp);
path[l] = c;
}
break;
}
- fclose(f);
+ close(f);
return err;
}
@@ -1462,27 +1481,25 @@ static int cache_export_ent(char *domain, struct exportent *exp, char *path)
*/
int cache_export(nfs_export *exp, char *path)
{
- char buf[INET6_ADDRSTRLEN];
- int err;
- FILE *f;
+ char ip[INET6_ADDRSTRLEN];
+ char buf[RPC_CHAN_BUF_SIZE], *bp;
+ int blen, f;
- f = fopen("/proc/net/rpc/auth.unix.ip/channel", "w");
- if (!f)
+ f = open("/proc/net/rpc/auth.unix.ip/channel", O_WRONLY);
+ if (f < 0)
return -1;
-
- qword_print(f, "nfsd");
- qword_print(f,
- host_ntop(get_addrlist(exp->m_client, 0), buf, sizeof(buf)));
- qword_printtimefrom(f, exp->m_export.e_ttl);
- qword_print(f, exp->m_client->m_hostname);
- err = qword_eol(f);
-
- fclose(f);
-
- err = cache_export_ent(exp->m_client->m_hostname, &exp->m_export, path)
- || err;
- return err;
+ bp = buf, blen = sizeof(buf);
+ qword_add(&bp, &blen, "nfsd");
+ qword_add(&bp, &blen, host_ntop(get_addrlist(exp->m_client, 0), ip, sizeof(ip)));
+ qword_adduint(&bp, &blen, time(0) + exp->m_export.e_ttl);
+ qword_add(&bp, &blen, exp->m_client->m_hostname);
+ qword_addeol(&bp, &blen);
+ if (blen <= 0 || write(f, buf, bp - buf) != bp - buf) blen = -1;
+ close(f);
+ if (blen < 0) return -1;
+
+ return cache_export_ent(buf, sizeof(buf), exp->m_client->m_hostname, &exp->m_export, path);
}
/**
@@ -1501,27 +1518,33 @@ int cache_export(nfs_export *exp, char *path)
struct nfs_fh_len *
cache_get_filehandle(nfs_export *exp, int len, char *p)
{
- FILE *f = fopen("/proc/fs/nfsd/filehandle", "r+");
- char buf[200];
- char *bp = buf;
- int failed;
static struct nfs_fh_len fh;
+ char buf[RPC_CHAN_BUF_SIZE], *bp;
+ int blen, f;
+
+ f = open("/proc/fs/nfsd/filehandle", O_RDWR);
+ if (f < 0) {
+ f = open("/proc/fs/nfs/filehandle", O_RDWR);
+ if (f < 0) return NULL;
+ }
- if (!f)
- f = fopen("/proc/fs/nfs/filehandle", "r+");
- if (!f)
+ bp = buf, blen = sizeof(buf);
+ qword_add(&bp, &blen, exp->m_client->m_hostname);
+ qword_add(&bp, &blen, p);
+ qword_addint(&bp, &blen, len);
+ qword_addeol(&bp, &blen);
+ if (blen <= 0 || write(f, buf, bp - buf) != bp - buf) {
+ close(f);
return NULL;
+ }
+ bp = buf;
+ blen = read(f, buf, sizeof(buf));
+ close(f);
- qword_print(f, exp->m_client->m_hostname);
- qword_print(f, p);
- qword_printint(f, len);
- failed = qword_eol(f);
-
- if (!failed)
- failed = (fgets(buf, sizeof(buf), f) == NULL);
- fclose(f);
- if (failed)
+ if (blen <= 0 || buf[blen-1] != '\n')
return NULL;
+ buf[blen-1] = 0;
+
memset(fh.fh_handle, 0, sizeof(fh.fh_handle));
fh.fh_size = qword_get(&bp, (char *)fh.fh_handle, NFS3_FHSIZE);
return &fh;
--
2.1.2
Signed-off-by: Timo Teräs <[email protected]>
---
support/nfs/nfsexport.c | 77 ++++++++++++++++++++++++++++---------------------
1 file changed, 44 insertions(+), 33 deletions(-)
diff --git a/support/nfs/nfsexport.c b/support/nfs/nfsexport.c
index f129fd2..afd7c90 100644
--- a/support/nfs/nfsexport.c
+++ b/support/nfs/nfsexport.c
@@ -18,6 +18,7 @@
#include <fcntl.h>
#include "nfslib.h"
+#include "misc.h"
/* if /proc/net/rpc/... exists, then
* write to it, as that interface is more stable.
@@ -32,62 +33,72 @@
static int
exp_unexp(struct nfsctl_export *exp, int export)
{
- FILE *f;
+ char buf[RPC_CHAN_BUF_SIZE], *bp;
struct stat stb;
__u32 fsid;
char fsidstr[8];
__u16 dev;
__u32 inode;
- int err;
+ int err = 0, f, blen;
+ f = open("/proc/net/rpc/nfsd.export/channel", O_WRONLY);
+ if (f < 0) return -1;
- f = fopen("/proc/net/rpc/nfsd.export/channel", "w");
- if (f == NULL) return -1;
- qword_print(f, exp->ex_client);
- qword_print(f, exp->ex_path);
+ bp = buf; blen = sizeof(buf);
+ qword_add(&bp, &blen, exp->ex_client);
+ qword_add(&bp, &blen, exp->ex_path);
if (export) {
- qword_printint(f, 0x7fffffff);
- qword_printint(f, exp->ex_flags);
- qword_printint(f, exp->ex_anon_uid);
- qword_printint(f, exp->ex_anon_gid);
- qword_printint(f, exp->ex_dev);
+ qword_addint(&bp, &blen, 0x7fffffff);
+ qword_addint(&bp, &blen, exp->ex_flags);
+ qword_addint(&bp, &blen, exp->ex_anon_uid);
+ qword_addint(&bp, &blen, exp->ex_anon_gid);
+ qword_addint(&bp, &blen, exp->ex_dev);
} else
- qword_printint(f, 1);
-
- err = qword_eol(f);
- fclose(f);
+ qword_addint(&bp, &blen, 1);
+ qword_addeol(&bp, &blen);
+ if (blen <= 0 || write(f, buf, bp - buf) != bp - buf)
+ err = -1;
+ close(f);
if (stat(exp->ex_path, &stb) != 0)
return -1;
- f = fopen("/proc/net/rpc/nfsd.fh/channel", "w");
- if (f==NULL) return -1;
+
+ f = open("/proc/net/rpc/nfsd.fh/channel", O_WRONLY);
+ if (f < 0) return -1;
if (exp->ex_flags & NFSEXP_FSID) {
- qword_print(f,exp->ex_client);
- qword_printint(f,1);
+ bp = buf; blen = sizeof(buf);
+ qword_add(&bp, &blen, exp->ex_client);
+ qword_addint(&bp, &blen, 1);
fsid = exp->ex_dev;
- qword_printhex(f, (char*)&fsid, 4);
+ qword_addhex(&bp, &blen, (char*)&fsid, 4);
if (export) {
- qword_printint(f, 0x7fffffff);
- qword_print(f, exp->ex_path);
+ qword_addint(&bp, &blen, 0x7fffffff);
+ qword_add(&bp, &blen, exp->ex_path);
} else
- qword_printint(f, 1);
-
- err = qword_eol(f) || err;
+ qword_addint(&bp, &blen, 1);
+ qword_addeol(&bp, &blen);
+ if (blen <= 0 || write(f, buf, bp - buf) != bp - buf)
+ err = -1;
}
- qword_print(f,exp->ex_client);
- qword_printint(f,0);
+
+ bp = buf; blen = sizeof(buf);
+ qword_add(&bp, &blen, exp->ex_client);
+ qword_addint(&bp, &blen, 0);
dev = htons(major(stb.st_dev)); memcpy(fsidstr, &dev, 2);
dev = htons(minor(stb.st_dev)); memcpy(fsidstr+2, &dev, 2);
inode = stb.st_ino; memcpy(fsidstr+4, &inode, 4);
- qword_printhex(f, fsidstr, 8);
+ qword_addhex(&bp, &blen, fsidstr, 8);
if (export) {
- qword_printint(f, 0x7fffffff);
- qword_print(f, exp->ex_path);
+ qword_addint(&bp, &blen, 0x7fffffff);
+ qword_add(&bp, &blen, exp->ex_path);
} else
- qword_printint(f, 1);
- err = qword_eol(f) || err;
- fclose(f);
+ qword_addint(&bp, &blen, 1);
+ qword_addeol(&bp, &blen);
+ if (blen <= 0 || write(f, buf, bp - buf) != bp - buf)
+ err = -1;
+ close(f);
+
return err;
}
--
2.1.2
Signed-off-by: Timo Teräs <[email protected]>
---
utils/gssd/gssd_proc.c | 9 +++++---
utils/gssd/svcgssd.h | 2 +-
utils/gssd/svcgssd_main_loop.c | 9 ++++----
utils/gssd/svcgssd_proc.c | 51 ++++++++++++++++++++++--------------------
4 files changed, 38 insertions(+), 33 deletions(-)
diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
index 121feb1..1d8e6a7 100644
--- a/utils/gssd/gssd_proc.c
+++ b/utils/gssd/gssd_proc.c
@@ -78,6 +78,7 @@
#include "nfsrpc.h"
#include "nfslib.h"
#include "gss_names.h"
+#include "misc.h"
/*
* pollarray:
@@ -1250,7 +1251,7 @@ void
handle_gssd_upcall(struct clnt_info *clp)
{
uid_t uid;
- char *lbuf = NULL;
+ char lbuf[RPC_CHAN_BUF_SIZE];
int lbuflen = 0;
char *p;
char *mech = NULL;
@@ -1260,11 +1261,14 @@ handle_gssd_upcall(struct clnt_info *clp)
printerr(1, "handling gssd upcall (%s)\n", clp->dirname);
- if (readline(clp->gssd_fd, &lbuf, &lbuflen) != 1) {
+ lbuflen = read(clp->gssd_fd, lbuf, sizeof(lbuf));
+ if (lbuflen <= 0 || lbuf[lbuflen-1] != '\n') {
printerr(0, "WARNING: handle_gssd_upcall: "
"failed reading request\n");
return;
}
+ lbuf[lbuflen-1] = 0;
+
printerr(2, "%s: '%s'\n", __func__, lbuf);
/* find the mechanism name */
@@ -1362,7 +1366,6 @@ handle_gssd_upcall(struct clnt_info *clp)
}
out:
- free(lbuf);
free(mech);
free(enctypes);
free(target);
diff --git a/utils/gssd/svcgssd.h b/utils/gssd/svcgssd.h
index 9a2e2e8..02b5c7a 100644
--- a/utils/gssd/svcgssd.h
+++ b/utils/gssd/svcgssd.h
@@ -35,7 +35,7 @@
#include <sys/queue.h>
#include <gssapi/gssapi.h>
-void handle_nullreq(FILE *f);
+void handle_nullreq(int f);
void gssd_run(void);
#define GSSD_SERVICE_NAME "nfs"
diff --git a/utils/gssd/svcgssd_main_loop.c b/utils/gssd/svcgssd_main_loop.c
index 2b4111c..b5681ce 100644
--- a/utils/gssd/svcgssd_main_loop.c
+++ b/utils/gssd/svcgssd_main_loop.c
@@ -54,19 +54,18 @@ void
gssd_run()
{
int ret;
- FILE *f;
+ int f;
struct pollfd pollfd;
#define NULLRPC_FILE "/proc/net/rpc/auth.rpcsec.init/channel"
- f = fopen(NULLRPC_FILE, "rw");
-
- if (!f) {
+ f = open(NULLRPC_FILE, O_RDWR);
+ if (f < 0) {
printerr(0, "failed to open %s: %s\n",
NULLRPC_FILE, strerror(errno));
exit(1);
}
- pollfd.fd = fileno(f);
+ pollfd.fd = f;
pollfd.events = POLLIN;
while (1) {
int save_err;
diff --git a/utils/gssd/svcgssd_proc.c b/utils/gssd/svcgssd_proc.c
index 5bdb438..72ec254 100644
--- a/utils/gssd/svcgssd_proc.c
+++ b/utils/gssd/svcgssd_proc.c
@@ -73,36 +73,35 @@ struct svc_cred {
int cr_ngroups;
gid_t cr_groups[NGROUPS];
};
-static char vbuf[RPC_CHAN_BUF_SIZE];
static int
do_svc_downcall(gss_buffer_desc *out_handle, struct svc_cred *cred,
gss_OID mech, gss_buffer_desc *context_token,
int32_t endtime, char *client_name)
{
- FILE *f;
- int i;
+ char buf[RPC_CHAN_BUF_SIZE], *bp;
+ int i, f, err, blen;
char *fname = NULL;
- int err;
printerr(1, "doing downcall\n");
if ((fname = mech2file(mech)) == NULL)
goto out_err;
- f = fopen(SVCGSSD_CONTEXT_CHANNEL, "w");
- if (f == NULL) {
+
+ f = open(SVCGSSD_CONTEXT_CHANNEL, O_WRONLY);
+ if (f < 0) {
printerr(0, "WARNING: unable to open downcall channel "
"%s: %s\n",
SVCGSSD_CONTEXT_CHANNEL, strerror(errno));
goto out_err;
}
- setvbuf(f, vbuf, _IOLBF, RPC_CHAN_BUF_SIZE);
- qword_printhex(f, out_handle->value, out_handle->length);
+ bp = buf, blen = sizeof(buf);
+ qword_addhex(&bp, &blen, out_handle->value, out_handle->length);
/* XXX are types OK for the rest of this? */
/* For context cache, use the actual context endtime */
- qword_printint(f, endtime);
- qword_printint(f, cred->cr_uid);
- qword_printint(f, cred->cr_gid);
- qword_printint(f, cred->cr_ngroups);
+ qword_addint(&bp, &blen, endtime);
+ qword_addint(&bp, &blen, cred->cr_uid);
+ qword_addint(&bp, &blen, cred->cr_gid);
+ qword_addint(&bp, &blen, cred->cr_ngroups);
printerr(2, "mech: %s, hndl len: %d, ctx len %d, timeout: %d (%d from now), "
"clnt: %s, uid: %d, gid: %d, num aux grps: %d:\n",
fname, out_handle->length, context_token->length,
@@ -110,19 +109,21 @@ do_svc_downcall(gss_buffer_desc *out_handle, struct svc_cred *cred,
client_name ? client_name : "<null>",
cred->cr_uid, cred->cr_gid, cred->cr_ngroups);
for (i=0; i < cred->cr_ngroups; i++) {
- qword_printint(f, cred->cr_groups[i]);
+ qword_addint(&bp, &blen, cred->cr_groups[i]);
printerr(2, " (%4d) %d\n", i+1, cred->cr_groups[i]);
}
- qword_print(f, fname);
- qword_printhex(f, context_token->value, context_token->length);
+ qword_add(&bp, &blen, fname);
+ qword_addhex(&bp, &blen, context_token->value, context_token->length);
if (client_name)
- qword_print(f, client_name);
- err = qword_eol(f);
- if (err) {
+ qword_add(&bp, &blen, client_name);
+ qword_addeol(&bp, &blen);
+ err = 0;
+ if (blen <= 0 || write(f, buf, bp - buf) != bp - buf) {
printerr(1, "WARNING: error writing to downcall channel "
"%s: %s\n", SVCGSSD_CONTEXT_CHANNEL, strerror(errno));
+ err = -1;
}
- fclose(f);
+ close(f);
return err;
out_err:
printerr(1, "WARNING: downcall failed\n");
@@ -317,7 +318,7 @@ print_hexl(const char *description, unsigned char *cp, int length)
#endif
void
-handle_nullreq(FILE *f) {
+handle_nullreq(int f) {
/* XXX initialize to a random integer to reduce chances of unnecessary
* invalidation of existing ctx's on restarting svcgssd. */
static u_int32_t handle_seq = 0;
@@ -339,19 +340,21 @@ handle_nullreq(FILE *f) {
u_int32_t maj_stat = GSS_S_FAILURE, min_stat = 0;
u_int32_t ignore_min_stat;
struct svc_cred cred;
- static char *lbuf = NULL;
- static int lbuflen = 0;
- static char *cp;
+ char lbuf[RPC_CHAN_BUF_SIZE];
+ int lbuflen = 0;
+ char *cp;
int32_t ctx_endtime;
char *hostbased_name = NULL;
printerr(1, "handling null request\n");
- if (readline(fileno(f), &lbuf, &lbuflen) != 1) {
+ lbuflen = read(f, lbuf, sizeof(lbuf));
+ if (lbuflen <= 0 || lbuf[lbuflen-1] != '\n') {
printerr(0, "WARNING: handle_nullreq: "
"failed reading request\n");
return;
}
+ lbuf[lbuflen-1] = 0;
cp = lbuf;
--
2.1.2
On 10/02/2014 09:41 AM, Timo Teräs wrote:
> Changes since the first send:
> - split to five separate patches
> - fixed a bug in cache_get_filehandle() that made result parsing not work
> - fixed to check result of write() calls
>
> The review mentioned my patches adding:
> nfssvc.c:71:8: warning: ignoring return value of 'system', declared with attribute warn_unused_result [-Wunused-result]
> nfssvc.c:325:9: warning: ignoring return value of 'write', declared with attribute warn_unused_result [-Wunused-result]
> but this does not make any sense: I'm only adding one #include there.
> Is that perhaps uncovering some other issues?
>
> I'm not sure if the NFSv3 related issues were caused by the cache_get_filehandle()
> issue or not, so this still needs testing. Another potential cause is that the
> kernel is sending to user land requests longer than RPC_CHAN_BUF_SIZE bytes, but
> that does not seem likely.
>
> Timo Teräs (5):
> Add string.h to source files that need it
> mountd: talk to kernel using file descriptors instead of FILE
> gssd: talk to kernel using file descriptors instead of FILE
> nfsexport: talk to kernel using file descriptors instead of FILE
> nfslib: remove now unused FILE helpers
>
> support/include/exportfs.h | 1 +
> support/include/nfslib.h | 7 -
> support/include/nfsrpc.h | 1 +
> support/nfs/cacheio.c | 111 +------------
> support/nfs/nfsexport.c | 77 +++++----
> utils/gssd/gssd_proc.c | 9 +-
> utils/gssd/svcgssd.h | 2 +-
> utils/gssd/svcgssd_main_loop.c | 9 +-
> utils/gssd/svcgssd_proc.c | 51 +++---
> utils/gssd/write_bytes.h | 1 +
> utils/mountd/cache.c | 343 ++++++++++++++++++++++-------------------
> utils/nfsd/nfssvc.c | 1 +
> 12 files changed, 270 insertions(+), 343 deletions(-)
>
Committed and tested!
steved.
Hi,
it seems that the "rework access to /proc/net/rpc" patchset removed
dynamic buffers in favour of static, fixed size, buffers. That seems
like a step backwards to me?
At least the readline() function could be implemented using read/write
(instead of fread/fwrite) and a dynamic buffer...no?
//David
On Tue, 09 Dec 2014 09:16:59 +0100
David Härdeman <[email protected]> wrote:
> it seems that the "rework access to /proc/net/rpc" patchset removed
> dynamic buffers in favour of static, fixed size, buffers. That seems
> like a step backwards to me?
Depends a bit on your view. On read() side, readline() like
functionality is removed yes. Though, my understanding is so that this
is not needed with the kernel API. Maybe someone can correct me if I'm
wrong. The removal simplifies memory management, overall code size. As
probably has a positive impact on speed too (probably not too big, but
this communication is used all overall, so it might be useful).
On write() side the old code was completely wrong. It did several
assumptions on how FILE buffering works, most of them being incorrect
in general, but also glibc. It only worked because no large messages
have been sent to kernel.
> At least the readline() function could be implemented using
> read/write (instead of fread/fwrite) and a dynamic buffer...no?
It's extra complexity. I'd rather not add it unless it's required. My
understanding about the communication mechanism with kernel is that
it's not required. Why have code that would never be used?
Thanks,
Timo
On 2014-12-09 09:42, Timo Teras wrote:
> On Tue, 09 Dec 2014 09:16:59 +0100
> David Härdeman <[email protected]> wrote:
>
>> it seems that the "rework access to /proc/net/rpc" patchset removed
>> dynamic buffers in favour of static, fixed size, buffers. That seems
>> like a step backwards to me?
>
> Depends a bit on your view. On read() side, readline() like
> functionality is removed yes. Though, my understanding is so that this
> is not needed with the kernel API. Maybe someone can correct me if I'm
> wrong. The removal simplifies memory management, overall code size. As
> probably has a positive impact on speed too (probably not too big, but
> this communication is used all overall, so it might be useful).
And it makes the buffer size static, introducing an arbitrary limitation
(although a rather large one...32KB allocated on the stack IIRC)
> On write() side the old code was completely wrong. It did several
> assumptions on how FILE buffering works, most of them being incorrect
> in general, but also glibc. It only worked because no large messages
> have been sent to kernel.
I didn't really check the write() side, it was just the readline() that
I was interested in actually...
>
>> At least the readline() function could be implemented using
>> read/write (instead of fread/fwrite) and a dynamic buffer...no?
>
> It's extra complexity. I'd rather not add it unless it's required. My
> understanding about the communication mechanism with kernel is that
> it's not required. Why have code that would never be used?
I agree that it depends on your view. I tend to be very sceptical of
arbitrary limitations unless they have a very good reason (like
measurable and relevant performance impact), I doubt that's the case
here.
It's up to the maintainer though, I just wanted to point it out :)
Regards,
David
On 12/09/2014 09:01 AM, David Härdeman wrote:
> On 2014-12-09 09:42, Timo Teras wrote:
>> On Tue, 09 Dec 2014 09:16:59 +0100
>> David Härdeman <[email protected]> wrote:
>>
>>> it seems that the "rework access to /proc/net/rpc" patchset removed
>>> dynamic buffers in favour of static, fixed size, buffers. That seems
>>> like a step backwards to me?
>>
>> Depends a bit on your view. On read() side, readline() like
>> functionality is removed yes. Though, my understanding is so that this
>> is not needed with the kernel API. Maybe someone can correct me if I'm
>> wrong. The removal simplifies memory management, overall code size. As
>> probably has a positive impact on speed too (probably not too big, but
>> this communication is used all overall, so it might be useful).
>
> And it makes the buffer size static, introducing an arbitrary limitation (although a rather large one...32KB allocated on the stack IIRC)
>
>> On write() side the old code was completely wrong. It did several
>> assumptions on how FILE buffering works, most of them being incorrect
>> in general, but also glibc. It only worked because no large messages
>> have been sent to kernel.
>
> I didn't really check the write() side, it was just the readline() that I was interested in actually...
>
>>
>>> At least the readline() function could be implemented using
>>> read/write (instead of fread/fwrite) and a dynamic buffer...no?
>>
>> It's extra complexity. I'd rather not add it unless it's required. My
>> understanding about the communication mechanism with kernel is that
>> it's not required. Why have code that would never be used?
>
> I agree that it depends on your view. I tend to be very sceptical of arbitrary
> limitations unless they have a very good reason (like measurable and relevant
> performance impact), I doubt that's the case here.
Your skeptical-ability of arbitrary limitations has become very clear in
the last few hours... ;-) I guess I'm indifferent about it... From reading
your gssd patch set, it is a bit more artful not to use fixed size buffers
but again, I'm indifferent... That said... if patches appear removing these
fixed buffers they definitely would be considered...
>
> It's up to the maintainer though, I just wanted to point it out :)
My understanding these patches were needed to make nfs-utils compatible with the musl c-library.
That is the case, correct?
steved.
On Tue, Dec 09, 2014 at 11:08:39AM -0500, Steve Dickson wrote:
>On 12/09/2014 09:01 AM, David H?rdeman wrote:
>> On 2014-12-09 09:42, Timo Teras wrote:
>>> On Tue, 09 Dec 2014 09:16:59 +0100
>>> David H?rdeman <[email protected]> wrote:
>>>> At least the readline() function could be implemented using
>>>> read/write (instead of fread/fwrite) and a dynamic buffer...no?
>>>
>>> It's extra complexity. I'd rather not add it unless it's required. My
>>> understanding about the communication mechanism with kernel is that
>>> it's not required. Why have code that would never be used?
>>
>> I agree that it depends on your view. I tend to be very sceptical of arbitrary
>> limitations unless they have a very good reason (like measurable and relevant
>> performance impact), I doubt that's the case here.
>Your skeptical-ability of arbitrary limitations has become very clear in
>the last few hours... ;-) I guess I'm indifferent about it... From reading
>your gssd patch set, it is a bit more artful not to use fixed size buffers
>but again, I'm indifferent... That said... if patches appear removing these
>fixed buffers they definitely would be considered...
>
>>
>> It's up to the maintainer though, I just wanted to point it out :)
>My understanding these patches were needed to make nfs-utils compatible with the musl c-library.
>That is the case, correct?
The fread/fwrite removal seems reasonable, yes. The removal of the
readline() function though (which could be implemented using normal
read/malloc/realloc) seems less so.....IMHO.
--
David H?rdeman
On 12/09/2014 03:26 PM, David H?rdeman wrote:
> On Tue, Dec 09, 2014 at 11:08:39AM -0500, Steve Dickson wrote:
>> On 12/09/2014 09:01 AM, David H?rdeman wrote:
>>> On 2014-12-09 09:42, Timo Teras wrote:
>>>> On Tue, 09 Dec 2014 09:16:59 +0100
>>>> David H?rdeman <[email protected]> wrote:
>>>>> At least the readline() function could be implemented using
>>>>> read/write (instead of fread/fwrite) and a dynamic buffer...no?
>>>>
>>>> It's extra complexity. I'd rather not add it unless it's required. My
>>>> understanding about the communication mechanism with kernel is that
>>>> it's not required. Why have code that would never be used?
>>>
>>> I agree that it depends on your view. I tend to be very sceptical of arbitrary
>>> limitations unless they have a very good reason (like measurable and relevant
>>> performance impact), I doubt that's the case here.
>> Your skeptical-ability of arbitrary limitations has become very clear in
>> the last few hours... ;-) I guess I'm indifferent about it... From reading
>> your gssd patch set, it is a bit more artful not to use fixed size buffers
>> but again, I'm indifferent... That said... if patches appear removing these
>> fixed buffers they definitely would be considered...
>>
>>>
>>> It's up to the maintainer though, I just wanted to point it out :)
>> My understanding these patches were needed to make nfs-utils compatible with the musl c-library.
>> That is the case, correct?
>
> The fread/fwrite removal seems reasonable, yes. The removal of the
> readline() function though (which could be implemented using normal
> read/malloc/realloc) seems less so.....IMHO.
>
Patches welcome! 8-)
steved.
On Tue, 09 Dec 2014 16:30:52 -0500
Steve Dickson <[email protected]> wrote:
>
>
> On 12/09/2014 03:26 PM, David Härdeman wrote:
> > On Tue, Dec 09, 2014 at 11:08:39AM -0500, Steve Dickson wrote:
> >> On 12/09/2014 09:01 AM, David Härdeman wrote:
> >>> On 2014-12-09 09:42, Timo Teras wrote:
> >>>> On Tue, 09 Dec 2014 09:16:59 +0100
> >>>> David Härdeman <[email protected]> wrote:
> >>>>> At least the readline() function could be implemented using
> >>>>> read/write (instead of fread/fwrite) and a dynamic buffer...no?
> >>>>
> >>>> It's extra complexity. I'd rather not add it unless it's
> >>>> required. My understanding about the communication mechanism
> >>>> with kernel is that it's not required. Why have code that would
> >>>> never be used?
> >>>
> >>> I agree that it depends on your view. I tend to be very sceptical
> >>> of arbitrary limitations unless they have a very good reason
> >>> (like measurable and relevant performance impact), I doubt that's
> >>> the case here.
> >> Your skeptical-ability of arbitrary limitations has become very
> >> clear in the last few hours... ;-) I guess I'm indifferent about
> >> it... From reading your gssd patch set, it is a bit more artful
> >> not to use fixed size buffers but again, I'm indifferent... That
> >> said... if patches appear removing these fixed buffers they
> >> definitely would be considered...
> >>
> >>>
> >>> It's up to the maintainer though, I just wanted to point it out :)
> >> My understanding these patches were needed to make nfs-utils
> >> compatible with the musl c-library. That is the case, correct?
Yes. It is because musl FILE implementation uses writev() and readv()
with multiple buffers, and the kernel side does not handle that.
Also, the write side was very brittle even with glibc, it probably was
broken with large messages.
> > The fread/fwrite removal seems reasonable, yes. The removal of the
> > readline() function though (which could be implemented using normal
> > read/malloc/realloc) seems less so.....IMHO.
> >
> Patches welcome! 8-)
Normally having upper limits is also a security feature. It means the
other end cannot force the client code to receive so large messages
that it runs out of memory. Though, in this case the other end is
kernel and to be trusted.
Though, it probably helps catch potential errors. The messages from
kernel really cannot exceed that length. If they do, it's an error
anyway. The message structure pretty much ensures this.
In my opinion the dynamic allocation is a step backward, rather then
forwards. It adds potential failure (out of memory), is not required,
and it does not add any features either.
IMHO, "just because it used to be so" is a bad excuse. And it would
just cause additional code making harder to debug and easier to fail.
Why add complexity when it can be done simpler?
just my five cents,
Timo
On 2014-12-10 07:09, Timo Teras wrote:
> On Tue, 09 Dec 2014 16:30:52 -0500
> Steve Dickson <[email protected]> wrote:
>>>> My understanding these patches were needed to make nfs-utils
>>>> compatible with the musl c-library. That is the case, correct?
>
> Yes. It is because musl FILE implementation uses writev() and readv()
> with multiple buffers, and the kernel side does not handle that.
I should probably note in that case that my patches to gssd include a
call to fscanf, I'm guessing that'd be a problem for you?
> In my opinion the dynamic allocation is a step backward, rather then
> forwards. It adds potential failure (out of memory), is not required,
> and it does not add any features either.
>
> IMHO, "just because it used to be so" is a bad excuse. And it would
> just cause additional code making harder to debug and easier to fail.
> Why add complexity when it can be done simpler?
I think PATH_MAX is a good counter-example.
But I think we can at least agree that we're discussing coding style
now, which is a bit like discussing Emacs vs vi, and I doubt we'll ever
reach an agreement... :)
Regards,
David