Re: [PATCH] New pickle for TAR files.

[Mohammad-Reza Nabipoor]

Hi Kostas.


On Wed, Aug 09, 2023 at 05:46:47PM +0300, Kostas Chasialis wrote:
> Hello Mohammad.
> 
> Thank you for your comments and your patience to point them twice. I 
> mistakenly
> oversaw them.
> Sorry for sending such a sloppy patch. I hope this one makes up for it.
> 
> Please find attached the revised version.
> 


Thank you for your new revision.
Tests are green :)
There are a few minor things to fix; please find the comments below.

When you fix these issues (esp. make sure `make syntax-check` is happy),
then you can push it to master, if Jose doesn't have any objections.
Thank you!


Jose, any objections for pushing to master?


Regards,
Mohammad-Reza



2023-09-06  Konstantinos Chasialis  <koschasialis@gmail.com>

        * pickles/tar.px: New pickle for TAR files.


s/px/pk/ :)


        * testsuite/poke.pickles/tar-1-test.pk: New test.
        * testsuite/poke.pickles/tar-2-test.pk: Likewise.
        * testsuite/Makefile.am (EXTRA_DIST): Add new tests.
---
diff --git a/ChangeLog b/ChangeLog
index 25bcdefe..33edddbd 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,10 @@
+2023-09-06  Konstantinos Chasialis  <koschasialis@gmail.com>
+
+       * pickles/tar.px: New pickle for TAR files.


Ditto.


+       * testsuite/poke.pickles/tar-1-test.pk: New test.
+       * testsuite/poke.pickles/tar-2-test.pk: Likewise.
+       * testsuite/Makefile.am (EXTRA_DIST): Add new tests.
+
 2023-08-06  Mohammad-Reza Nabipoor  <mnabipoor@gnu.org>
 
        * liboke/ios.c (ios_write_int_common): Fix the function when `bits'
diff --git a/pickles/tar.pk b/pickles/tar.pk
new file mode 100644
index 00000000..8d25a1f6
--- /dev/null
+++ b/pickles/tar.pk
@@ -0,0 +1,234 @@
+type TAR_Thdr =
+  struct
+  {
+    uint<8>[100] name_chars; /* File name.  */
+    uint<8>[8] mode_chars; /* File mode.  */
+    uint<8>[8] uid_chars;  /* Owner's numeric user ID.  */
+    uint<8>[8] gid_chars;  /* Group's numeric user ID.  */
+    uint<8>[12] size_chars;  /* File size in bytes.  */
+    uint<8>[12] mtime_chars;  /* Last modification time in numeric Unix time 
format.  */
+    uint<8>[8] chksum_chars;  /* Checksum for header record.  */
+    uint<8> typeflag_char : typeflag_char in ['0', '\0', '1', '2', '3', '4', 
'5', '6', '7', 'x', 'g'];  /* Link indicator (file type).  */
+    uint<8>[100] linkname_chars;  /* Name of linked file.  */
+    uint<8>[8] magic_chars == ['u', 's', 't', 'a', 'r', ' ', ' ', '\0'];  /* 
Magic number.  */


I think using `string magic_string == "ustar  ";' is a easier to read,
but from performance perspective (esp. for invalid files), the bounded-array
approach you're using is better.

You decide to keep the array or change it to string!


+    uint<8>[32] uname_chars;  /* Owner user name.  */
+    uint<8>[32] gname_chars;  /* Owner group name.  */
+    uint<8>[8] devmajor_chars;  /* Device major number.  */
+    uint<8>[8] devminor_chars;  /* Device minor number.  */
+    uint<8>[155] prefix_chars;  /* Filename prefix.  */
+    uint<8>[12] @ OFFSET;  /* Padding.  */


What's the purpose of this `@ OFFSET'?
`uint<8>[12]; /* Padding.  */' shoul


+  


This line has whitespace which makes `make syntax-check' unhappy:

```
[...]
../pickles/tar.pk
maint.mk: empty line(s) or no newline at EOF
```

Please remove these blanks :)
You can search them using this regexp: `  *$'.
You have to add a newline (press Enter key at the very end of the file)
to make `make syntax-check'.  It's important to keep it happy :D


+    computed string name;
+    computed uint<21> mode;
+    computed uint<21> uid;
+    computed uint<21> gid;
+    computed offset<uint<33>, B> size;
------------------------------^

Please remove extra whitespace before `B`: `offset<uint<33>,B>'.


+
+    method get_size = offset<uint<33>, B>:


Ditto.


+    {
+      var x = atoi (catos (size_chars), 8);
+
+      return x#B;
+    }
+
+    method set_size = (offset<uint<33>, B> size) void:


Ditto.


+    {
+      stoca (format ("%u33o", size'magnitude), size_chars);
+    }
+
+    method get_mtime = uint<33>:
+    {
+      return atoi (catos (mtime_chars), 8);
+    }
+
+    method set_mtime = (uint<33> mtime) void:
+    {
+      stoca (format ("%u33o", mtime), mtime_chars);
+    }
+
+    method get_typeflag = string:
+    {
+      return typeflag_char as string;
+    }
+
+    method set_typeflag = (string typeflag) void:
+    {
+      typeflag_char = typeflag[0];
+    }    


Extra whitespace at the end of line.


+
+type TAR_Tfile =
+  struct
+  {
+    TAR_Thdr thdr;
+    var fsize = thdr.get_size;
+    var file_data_offset = OFFSET;
+    // end of data  padding for alignment
+    uint<8>[0] @ file_data_offset + fsize + alignto (fsize, 512#B);
+
+    method get_file_data_offset = offset<int<64>,b>:
+    {
+        return file_data_offset;
+    }
+
+    method get_eof_offset = offset<int<64>,b>:
+    {
+      return file_data_offset + fsize + alignto (fsize, 512#B);
+    }
+  };
+
+fun get_file_contents = (TAR_Tfile[] tfiles, int<32> idx) uint<8>[]:
+{
+  assert (idx < tfiles'length && idx > 0);
+
+  var contents_offset = tfiles[idx].get_file_data_offset;
+  var tidx = idx - 1;
+  while (tidx >= 0)
+  {
+    contents_offset += tfiles[tidx].get_eof_offset;
+    tidx -= 1;
+  }
+
+  return uint<8>[tfiles[idx].thdr.get_size] @ contents_offset;
+}
\ No newline at end of file


With this line, `git' is also telling you that there's no new line at end of
this file :)


diff --git a/testsuite/poke.pickles/tar-1-test.pk 
b/testsuite/poke.pickles/tar-1-test.pk
new file mode 100644
index 00000000..420d3912
--- /dev/null
+++ b/testsuite/poke.pickles/tar-1-test.pk
@@ -0,0 +1,1325 @@
+/* tar-1-test.pk - Tests for the TAR pickle.  */
+
+/* Copyright (C) 2023 Konstantinos Chasialis.  */
+
+/* This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+ 


One extra whitespace :)
Please remove it.


diff --git a/testsuite/poke.pickles/tar-2-test.pk 
b/testsuite/poke.pickles/tar-2-test.pk
new file mode 100644
index 00000000..ab45854d
--- /dev/null
+++ b/testsuite/poke.pickles/tar-2-test.pk
@@ -0,0 +1,2723 @@
+/* tar-1-test.pk - Tests for the TAR pickle.  */


s/tar-1-test/tar-2-test/ :)


+
+/* Copyright (C) 2023 Konstantinos Chasialis.  */
+
+/* This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+ 


Ditto.


+
+var tests = [
+  PkTest {
+    name = "project tar",
+    func = lambda (string name) void:
+      {
+        var tfiles = TAR_Tfile[] @ data : 0#B;
+
+        assert (tfiles'length == 7UL);
+        


Ditto.


+        assert (tfiles[0].thdr.get_name == "project/");
+        assert (tfiles[0].thdr.get_size == 0#B);
+        assert (tfiles[0].thdr.get_magic == "ustar  ");
+        assert (tfiles[0].thdr.get_mode == 0o0000775);
+
+        assert (tfiles[1].thdr.get_name == "project/src/");
+        assert (tfiles[1].thdr.get_size == 0#B);
+        assert (tfiles[1].thdr.get_magic == "ustar  ");
+        assert (tfiles[1].thdr.get_mode == 0o0000775);
+
+        assert (tfiles[2].thdr.get_name == "project/src/lib.c");
+        assert (catos(get_file_contents(tfiles, 2)) == CONTENT1);
+        assert (tfiles[2].thdr.get_magic == "ustar  ");
+        assert (tfiles[2].thdr.get_mode == 0o0000600);
+
+        assert (tfiles[3].thdr.get_name == "project/src/lib.h");
+        assert (catos(get_file_contents(tfiles, 3)) == CONTENT2);
+        assert (tfiles[3].thdr.get_magic == "ustar  ");
+        assert (tfiles[3].thdr.get_mode == 0o0000666);
+
+        assert (tfiles[4].thdr.get_name == "project/src/main.c");
+        assert (catos(get_file_contents(tfiles, 4)) == CONTENT3);
+        assert (tfiles[4].thdr.get_magic == "ustar  ");
+        assert (tfiles[4].thdr.get_mode == 0o0000770);
+
+        assert (tfiles[5].thdr.get_name == "project/doc/");
+        assert (tfiles[5].thdr.get_size == 0#B);
+        assert (tfiles[5].thdr.get_magic == "ustar  ");
+        assert (tfiles[5].thdr.get_mode == 0o0000775);
+
+        assert (tfiles[6].thdr.get_name == "project/doc/README.md");
+        assert (catos(get_file_contents(tfiles, 6)) == CONTENT4);             



Ditto.


+        assert (tfiles[6].thdr.get_magic == "ustar  ");
+        assert (tfiles[6].thdr.get_mode == 0o0000777);
+      },
+  },
+];
+
+var ok = pktest_run (tests);
+
+close (data);
+exit (ok ? 0 : 1);
-- 
2.34.1