Re: [PATCH v2 6/6] sanlock: Fail read_resource_owners with incorrect align size

[Amit Bawer]

On Sun, May 12, 2019 at 6:54 PM Nir Soffer <nirsof(a)gmail.com&gt; 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(a)redhat.com&gt;
>  ---
>   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
>
>
>  -(a)pytest.mark.xfail(reason=&quot;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
>
>