Acked-by: Hugh Greenberg <[EMAIL PROTECTED]>
On Mon, 2008-10-06 at 18:59 -0600, Abhishek Kulkarni wrote:
> This patch improves the error handling in xget such that it can handle
> the accidental export of procfs, devfs, sysfs or anything else.
>
> It does the following things
>
> a) While the xget server sets up files to be served, if it cannot
> open/read a file it just skips it and goes on to serve the other files.
>
> b) Some special files (from procfs) are set up properly by the xget
> server but when it tries to read from them, it returns an EINVAL.
> Now if the server cannot handle a read request on a file, it simply
> prints an error in the log, and the clients exit saying that the file
> cannot be downloaded.
>
> c) another minor bug in which the client segfaulted while unmounting the
> filesystem during an error.
>
> Part of the patch is based on some improvements Hugh was doing with the
> error handling.
>
> Signed-off-by: Abhishek Kulkarni <[EMAIL PROTECTED]>
>
> Index: xget.c
> ===================================================================
> --- xget.c (revision 710)
> +++ xget.c (working copy)
> @@ -484,8 +484,21 @@
> if (f->finished == 1)
> file_finalize(f, 0);
>
> - if(f->finished == 2)
> + if (f->finished == 2)
> continue;
> +
> + if (f->finished == 4) {
> + debug(Dbgfn, "Error downloading file %s
> \n", f->lname);
> + if (f->datafid) {
> + spc_close(f->datafid);
> + f->datafid = NULL;
> + }
> +
> + if (f->fs && f->fs != masterfs)
> +
> spc_umount(f->fs);
> +
> + f->finished = 2;
> + }
>
> fdone = 0;
> if (time(NULL) - f->progress > retrytime) {
> @@ -656,7 +669,8 @@
> continue;
>
> debug(Dbgfn, "Adding file: %s\n", buf);
> - if (localfileread(dir, buf) == NULL)
> + localfileread(dir, buf);
> + if (sp_haserror())
> goto error;
> }
> closedir(d);
> @@ -714,10 +728,9 @@
> par = dir->parent;
> parent_remove(dir);
> dir_remove(dir);
> - if ((ret = localfileread(par, name)) == NULL) {
> - sp_uerror(errno);
> + ret = localfileread(par, name);
> + if (sp_haserror())
> goto error;
> - }
> }
> else if (S_ISDIR(st.st_mode))
> ret = updatedir(dir, name);
> @@ -871,6 +884,8 @@
> Spfile *file;
>
> f = sp_malloc(sizeof(*f) + strlen(nname) + strlen(lname) + 2);
> + if (!f)
> + return NULL;
> qp = qpath++ * 16;
> f->nname = ((char *) f) + sizeof(*f);
> strcpy(f->nname, nname);
> @@ -879,6 +894,7 @@
> f->datasize = datasize;
> f->datalen = datalen;
> f->numworkers = 0;
> + f->fs = NULL;
> f->firstworker = NULL;
> f->lastworker = NULL;
> f->nextworker = NULL;
> @@ -911,13 +927,13 @@
> static Spfile*
> localfileread(Spfile *parent, char *filename)
> {
> - char *name, *nextf;
> + char *name, *nextf, *ename;
> struct stat st;
> File *f;
> Spfile *dir, *ret;
> DIR *dirstr;
> struct dirent *de;
> - int fd;
> + int fd, ecode;
> u32 crc, npmode;
> Spuser *usr;
> Spgroup *grp;
> @@ -927,6 +943,8 @@
> ret = NULL;
> usr = NULL;
> grp = NULL;
> + nextf = NULL;
> +
> if ((name = strrchr(filename, '/')))
> name++;
> else
> @@ -934,7 +952,7 @@
>
> if (lstat(filename, &st) < 0) {
> sp_uerror(errno);
> - return NULL;
> + goto error;
> }
>
> npmode = umode2npmode(st.st_mode);
> @@ -956,14 +974,19 @@
> goto error;
>
> close(fd);
> +
> + if (sp_haserror())
> + goto error;
> +
> debug(Dbgsrvfn, "Added file: %s\n", f->dir->name);
> ret = f->dir;
> +
> } else if (S_ISDIR(st.st_mode)) {
> if (strcmp(path, filename)) {
> dir = create_file(parent, name, npmode,
> qpath++ * 16, &dir_ops, usr, grp,
> NULL);
> if (!dir)
> - return NULL;
> + goto error;
>
> debug(Dbgsrvfn, "Added dir: %s\n", dir->name);
> }
> @@ -980,22 +1003,42 @@
> continue;
>
> nextf = (char *) sp_malloc(strlen(filename) +
> - strlen(de->d_name) + 2);
> + strlen(de->d_name) +
> 2);
> +
> + if (!nextf)
> + goto error;
> +
> sprintf(nextf, "%s/%s", filename, de->d_name);
> localfileread(dir, nextf);
> +
> free(nextf);
> + nextf = NULL;
> }
>
> closedir(dirstr);
> }
> ret->mtime = ret->atime = st.st_mtime;
> - }
> -
> + if (sp_haserror())
> + goto error;
> + } else
> + goto error;
> +
> return ret;
>
> error:
> + if (sp_haserror()) {
> + sp_rerror(&ename, &ecode);
> + debug(Dbgfn, "Error: Skipping %s : %s\n",
> + filename, ename);
> + sp_werror(NULL, 0);
> + }
> +
> if (fd > -1)
> close(fd);
> +
> + if (nextf)
> + free(nextf);
> +
> return NULL;
> }
>
> @@ -1060,11 +1103,16 @@
> return;
>
> error:
> - xget_uerror(errno);
> + debug(Dbgfn, "Error: Cannot process request for file %s : %s\n",
> + f->lname, strerror(errno));
> +
> if (buf)
> free(buf);
> if (fd > 0)
> close(fd);
> +
> + rc = sp_create_rflush();
> + sp_respond(req->req, rc);
> }
>
> static void
> @@ -1104,6 +1152,9 @@
> readsize = fd->iounit;
> buf = sp_malloc(readsize);
> n = spcfd_read(fd, buf, readsize);
> + if (n < 0)
> + goto error;
> +
> if ((lfd = open(f->lname, O_CREAT | O_RDWR, 0600)) == -1) {
> xget_uerror(errno);
> goto error;
> @@ -1142,12 +1193,15 @@
> return;
>
> error:
> + f->finished = 4;
> +
> if (st)
> free(st);
> if (buf)
> free(buf);
> if (lfd > -1)
> close(lfd);
> +
> return;
> }
>
> @@ -1359,6 +1413,7 @@
> }
>
> free(fnames[i]);
> + fnames[i] = NULL;
> }
>
> free(fnames);
> @@ -1374,6 +1429,10 @@
>
> done:
> spc_close(fid);
> + for (i=0; i < n; i++) {
> + if (fnames[i])
> + free(fnames[i]);
> + }
> return ret;
> }
>
> @@ -1756,10 +1815,8 @@
> chdir("/");
> }
>
> - if (!masterfs) {
> - if (localfileread(root, path) == NULL)
> - goto error;
> - }
> + if (!masterfs)
> + localfileread(root, path);
>
> while (1) {
> if (xget_haserror())
> @@ -1797,7 +1854,7 @@
> if (f->availfid)
> spc_close(f->availfid);
>
> - if (f->fs && f->fs != masterfs)
> + if (netaddress && f->fs && f->fs != masterfs)
> spc_umount(f->fs);
>
> f1 = f->next;
>
