On Wed, May 8, 2019 at 11:09 AM Pavel Bar <pbar@redhat.com> wrote:

Thanks for reviewing.

On Tue, May 7, 2019 at 1:38 AM Nir Soffer <nirsof@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@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_errot" - shouldn't it be "log_error"?

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")
-@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