On 12/7/18 4:13 PM, patrick keshishian wrote:
> 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/sedXXXXXXXXXX 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<repeat to infinity>/
>> $ 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.c 6 Dec 2018 20:16:04 -0000 1.39
>> +++ main.c 7 Dec 2018 07:31:54 -0000
>> @@ -35,6 +35,7 @@
>>
>> #include <sys/types.h>
>> #include <sys/ioctl.h>
>> +#include <sys/param.h>
>> #include <sys/stat.h>
>>
>> #include <ctype.h>
>> 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) {
>> + if (*oldfname == '\0')
>> + unlink(oldfname);
>
> This looks a bit odd.
That's because it's a type-O. Thanks for spotting.
I also noticed I forgot to close the outfile.
Updated diff below.
>
>
>> *oldfname = '\0';
>> }
>> - if (*tmpfname != '\0') {
>> - if (outfile != NULL && outfile != stdout)
>> - fclose(outfile);
>> - outfile = NULL;
>> - rename(tmpfname, fname);
>> - *tmpfname = '\0';
>> - }
>> outfname = NULL;
>> }
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.c 6 Dec 2018 20:16:04 -0000 1.39
+++ main.c 8 Dec 2018 11:47:28 -0000
@@ -35,6 +35,7 @@
#include <sys/types.h>
#include <sys/ioctl.h>
+#include <sys/param.h>
#include <sys/stat.h>
#include <ctype.h>
@@ -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,47 @@ 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);
- }
+ fclose(outfile);
+ if (inplace != NULL) {
+ if (*oldfname != '\0')
+ unlink(oldfname);
*oldfname = '\0';
}
- if (*tmpfname != '\0') {
- if (outfile != NULL && outfile != stdout)
- fclose(outfile);
- outfile = NULL;
- rename(tmpfname, fname);
- *tmpfname = '\0';
- }
outfname = NULL;
}
}
@@ -341,7 +363,7 @@ finish_file(void)
int
mf_fgets(SPACE *sp, enum e_spflag spflag)
{
- struct stat sb;
+ struct stat sb, isb;
size_t len;
char *p;
int c, fd;
@@ -369,10 +391,18 @@ mf_fgets(SPACE *sp, enum e_spflag spflag
outfname = "stdout";
break;
}
+ infile = fopen(fname, inplace == NULL ? "r" : "r+");
+ if (infile == NULL) {
+ warning("%s", strerror(errno));
+ rval = 1;
+ continue;
+ }
if (inplace != NULL) {
- if (lstat(fname, &sb) != 0)
+ /* inplace reads from copy */
+ outfile = infile;
+ if (fstat(fileno(outfile), &sb) != 0)
error(FATAL, "%s: %s", fname,
- strerror(errno ? errno : EIO));
+ strerror(errno));
if (!S_ISREG(sb.st_mode))
error(FATAL, "%s: %s %s", fname,
"in-place editing only",
@@ -382,32 +412,48 @@ mf_fgets(SPACE *sp, enum e_spflag spflag
sizeof(oldfname));
len = strlcat(oldfname, inplace,
sizeof(oldfname));
- if (len > sizeof(oldfname))
- error(FATAL, "%s: name too long",
fname);
- }
- len = snprintf(tmpfname, sizeof(tmpfname),
"%s/sedXXXXXXXXXX",
- dirname(fname));
- if (len >= sizeof(tmpfname))
- error(FATAL, "%s: name too long", fname);
- if ((fd = mkstemp(tmpfname)) == -1)
- error(FATAL, "%s: %s", fname, strerror(errno));
- if ((outfile = fdopen(fd, "w")) == NULL) {
- unlink(tmpfname);
- error(FATAL, "%s", fname);
+ if (len >= sizeof(oldfname))
+ error(FATAL, "backup %s: name too long",
+ fname);
+ (void)unlink(oldfname);
+ if ((infile = fopen(oldfname, "w+")) == NULL)
+ error(FATAL, "can't create backup "
+ "file: %s", oldfname);
+ if (fstat(fileno(infile), &isb) != 0)
+ error(FATAL, "%s: %s", oldfname,
+ strerror(errno));
+ if (!S_ISREG(isb.st_mode))
+ error(FATAL, "backup file %s not a "
+ "regular file and can't be "
+ "replaced", oldfname);
+ fchown(fileno(infile), sb.st_uid, sb.st_gid);
+ fchmod(fileno(infile), sb.st_mode & ALLPERMS);
+ } else {
+ (void)strlcpy(oldfname, "/tmp/sedXXXXXXXXXX",
+ sizeof(oldfname));
+ if ((fd = mkstemp(oldfname)) == -1)
+ error(FATAL, "%s: %s", fname,
+ strerror(errno));
+ if ((infile = fdopen(fd, "a+")) == NULL) {
+ unlink(oldfname);
+ error(FATAL, "%s", fname);
+ }
}
- fchown(fileno(outfile), sb.st_uid, sb.st_gid);
- fchmod(fileno(outfile), sb.st_mode & ALLPERMS);
- outfname = tmpfname;
+ outfname = fname;
linenum = 0;
resetstate();
+ if (!copy(outfile, infile)) {
+ unlink(oldfname);
+ error(FATAL, "%s: failed to create backup copy "
+ "to %s: %s", fname, oldfname,
+ strerror(errno));
+ }
+ ftruncate(fileno(outfile), 0);
+ rewind(outfile);
+ rewind(infile);
} else {
outfile = stdout;
outfname = "stdout";
- }
- if ((infile = fopen(fname, "r")) == NULL) {
- warning("%s", strerror(errno));
- rval = 1;
- continue;
}
}
Index: process.c
===================================================================
RCS file: /cvs/src/usr.bin/sed/process.c,v
retrieving revision 1.34
diff -u -p -r1.34 process.c
--- process.c 14 Nov 2018 10:59:33 -0000 1.34
+++ process.c 8 Dec 2018 11:47:28 -0000
@@ -75,6 +75,7 @@ static int sdone; /* If any substitutes
/* Iov structure for 'w' commands. */
static regex_t *defpreg;
size_t maxnsub;
+extern char oldfname[PATH_MAX];
regmatch_t *match;
#define OUT() do {\
@@ -473,7 +474,11 @@ flush_appends(void)
break;
}
if (ferror(outfile))
- error(FATAL, "%s: %s", outfname, strerror(errno ? errno : EIO));
+ error(FATAL, "%s: %s%s%s%s", outfname,
+ strerror(errno ? errno : EIO),
+ *oldfname == '\0' ? "" : " (backup at ",
+ *oldfname == '\0' ? "" : oldfname,
+ *oldfname == '\0' ? "" : ")");
appendx = sdone = 0;
}
@@ -513,7 +518,11 @@ lputs(char *s)
(void)fputc('$', outfile);
(void)fputc('\n', outfile);
if (ferror(outfile))
- error(FATAL, "%s: %s", outfname, strerror(errno ? errno : EIO));
+ error(FATAL, "%s: %s%s%s%s", outfname,
+ strerror(errno ? errno : EIO),
+ *oldfname == '\0' ? "" : " (backup at ",
+ *oldfname == '\0' ? "" : oldfname,
+ *oldfname == '\0' ? "" : ")");
}
static inline int
Index: sed.1
===================================================================
RCS file: /cvs/src/usr.bin/sed/sed.1,v
retrieving revision 1.57
diff -u -p -r1.57 sed.1
--- sed.1 14 Nov 2018 10:59:33 -0000 1.57
+++ sed.1 8 Dec 2018 11:47:28 -0000
@@ -102,10 +102,6 @@ Edit files in place, saving backups with
If a zero length
.Ar extension
is given, no backup will be saved.
-It is not recommended to give a zero length
-.Ar extension
-when in place editing files, as it risks corruption or partial content
-in situations where disk space is exhausted, etc.
In
.Fl i
mode, the hold space, line numbers, and ranges are reset between files.
