On Sun, May 12, 2019 at 6:54 PM Nir Soffer <nirsof@gmail.com> wrote:
Previously if sector size was not set, we modified both sector size and
align size, dropping the caller align size silently.

This can cause two issues:

- User align size was correct, but sanlock guessed the wrong value -
  in this case sanlock logs a debug message and perform another read.

- User align size was incorrect, but sanlock guess was correct, hiding a
  bug in caller code.

Change the behaviour to be more consistent with the way we validate
sector size, and reject invalid value set by the user.

Signed-off-by: Nir Soffer <nsoffer@redhat.com>
---
 src/resource.c       | 17 +++++++++++++----
 tests/python_test.py |  1 -
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/src/resource.c b/src/resource.c
index 3959b35..e0e07b6 100644
--- a/src/resource.c
+++ b/src/resource.c
@@ -168,22 +168,24 @@ int read_resource_owners(struct task *task, struct token *token,
        uint32_t checksum;
        char *lease_buf_dblock;
        char *lease_buf = NULL;
        char *hosts_buf = NULL;
        int sector_size_set = token->sector_size != 0;
probably: const int sector_size_set = token->sector_size != 0;
+       int align_size_set = token->align_size != 0;
probably: const int align_size_set = token->align_size != 0;
        int align_size;
        int host_count = 0;
        int i, rv;

        disk = &token->disks[0];

        /* If sector size not set, start with the smaller one. */
-
-       if (!sector_size_set) {
+       if (!sector_size_set)
                token->sector_size = 512;
-               token->align_size = sector_size_to_align_size_old(512);
-       }
+
+       /* If align size not set, default to the older align. */
+       if (!align_size_set)
+               token->align_size = sector_size_to_align_size_old(token->sector_size);

        /* we could in-line paxos_read_buf here like we do in read_mode_block */
  retry:
        rv = paxos_read_buf(task, token, &lease_buf);
        if (rv < 0) {
@@ -211,10 +213,17 @@ int read_resource_owners(struct task *task, struct token *token,
                          token->sector_size, leader.sector_size);
                rv = -EINVAL;
                goto out;
        }

+       if (align_size_set && token->align_size != align_size) {
+               log_errot(token, "read_resource_owners invalid align_size: %d actual: %d",
+                         token->align_size, align_size);
+               rv = -EINVAL;
+               goto out;
+       }
+
        /*
         * If the caller did not specify sector size and our guess was wrong,
         * retry with the actual value
         */
        if ((token->sector_size != leader.sector_size) ||
diff --git a/tests/python_test.py b/tests/python_test.py
index d5b81ae..85e4829 100644
--- a/tests/python_test.py
+++ b/tests/python_test.py
@@ -235,11 +235,10 @@ def test_read_resource_owners_4k_invalid_sector_size(
         sanlock.read_resource_owners(
             "ls_name", "res_name", disks, sector=SECTOR_SIZE_512)
     assert e.value.errno == errno.EINVAL


-@pytest.mark.xfail(reason="fallback hides invalid user value")
 def test_read_resource_owners_invalid_align_size(tmpdir, sanlock_daemon):
     path = str(tmpdir.join("path"))
     util.create_file(path, 1024**3)
     disks = [(path, 0)]

--
2.17.2