On Wed, May 8, 2019 at 11:09 AM Pavel Bar <pbar(a)redhat.com> wrote:
Thanks for reviewing.
On Tue, May 7, 2019 at 1:38 AM Nir Soffer <nirsof(a)gmail.com> wrote:
> If sanlock_read_resource_owners() is called with wrong sector size, fail
> instead of falling back to actual sector size. This is more consistent
> with sanlock_read_resource() and other other functions.
>
> Signed-off-by: Nir Soffer <nsoffer(a)redhat.com>
> ---
> src/resource.c | 17 ++++++++++++++---
> tests/python_test.py | 1 -
> 2 files changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/src/resource.c b/src/resource.c
> index a783106..f75ea71 100644
> --- a/src/resource.c
> +++ b/src/resource.c
> @@ -167,19 +167,19 @@ int read_resource_owners(struct task *task, struct
> token *token,
> uint64_t host_id;
> uint32_t checksum;
> char *lease_buf_dblock;
> char *lease_buf = NULL;
> char *hosts_buf = NULL;
> + int sector_size = token->sector_size;
> int align_size;
> int host_count = 0;
> int i, rv;
>
> disk = &token->disks[0];
>
> /*
> - * We don't know the sector_size of the resource until the leader
> - * record has been read, start with the smaller size.
> + * If we don't know the sector_size start with the smaller size.
> */
>
> if (!token->sector_size) {
> token->sector_size = 512;
> token->align_size = sector_size_to_align_size_old(512);
> @@ -204,13 +204,24 @@ int read_resource_owners(struct task *task, struct
> token *token,
>
> align_size = leader_align_size_from_flag(leader.flags);
> if (!align_size)
> align_size =
> sector_size_to_align_size_old(leader.sector_size);
>
> + /* If caller specified value was incorrect, fail. */
> + if (sector_size && sector_size != leader.sector_size) {
>
If "leader.sector_size" is non-zero, you can skip the first sub-condition
"sector_size &&"
I think that leader.sector_size is always non-zero, but we cannot skip
sector_size check.
sector_size is initialized to the user specified sector size - what we set
in the python bindings
sector argument.
If sector_size is set, it must match the actual sector size on storage
(leader.sector_size)
written when a resource was created (write_resource). This is the only case
when using
the python bindings, because we use default value of 512 - keeping the old
behavior.
However if this function is called from the command line (e.g. sanlock
client ...), or directly
using sanlock_read_resource_owners() (e.g. someone writing application
using sanlock C API),
sector will be set only if you added SANLK_RES_SECTOR512 or
SANLK_RES_SECTOR4K
flags the res.flags. In this case sanlock try to be nice and guess the
value.
I'm not sure why David added this option, maybe to keep backward
compatibility.
+ log_errot(token, "read_resource_owners invalid
> sector_size: %d actual: %d",
>
"log_erro*t*" - shouldn't it be "log_erro*r*"?
It looks like a typo, but this is the name of the macro :-)
// src/log.h
46 #define log_errot(token, fmt, args...) log_level(token->space_id,
token->res_id, NULL, LOG_ERR, fmt, ##args)
> + sector_size, leader.sector_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) ||
> (token->align_size != align_size)) {
> - /* initial sizes were wrong */
> log_debug("read_resource_owners rereading with correct
> sizses");
>
Although you didn't change this code, I see typo "sizses".
Why not send a patch fixing it?
token->sector_size = leader.sector_size;
> token->align_size = align_size;
> free(lease_buf);
> lease_buf = NULL;
> diff --git a/tests/python_test.py b/tests/python_test.py
> index aa1f7f5..e8a34c0 100644
> --- a/tests/python_test.py
> +++ b/tests/python_test.py
> @@ -246,11 +246,10 @@ def
> test_read_resource_4k_invalid_sector_size(sanlock_daemon):
>
>
> @pytest.mark.skipif(
> "SANLOCK_4K_PATH" not in os.environ,
> reason="Requires user specified path using 4k block size")
> -(a)pytest.mark.xfail(reason="fallback hides wrong value from caller")
> def test_read_resource_owners_4k_invalid_sector_size(sanlock_daemon):
> path = os.environ["SANLOCK_4K_PATH"]
> disks = [(path, 0)]
>
> sanlock.write_resource(
> --
> 2.17.2
>
>