New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rbd-nbd: don't ignore --read-only option in BLKROSET ioctl #13944

Merged
merged 2 commits into from Mar 16, 2017

Conversation

Projects
None yet
2 participants
@liupan1111
Contributor

liupan1111 commented Mar 13, 2017

No description provided.

@liupan1111

This comment has been minimized.

Contributor

liupan1111 commented Mar 13, 2017

@trociny , please help take a look.

@trociny

This comment has been minimized.

Contributor

trociny commented Mar 13, 2017

@liupan1111 I am not sure about the redundant line break. It separates rbd-nbd specific options from ceph generic options, and this looks ok to me. Also this is what i.g. ceph-osd does.

I would more happy if one decreases number of space characters between an option name and its description. This can be done in the same commit where you fix "an image" (I don't see a point in having many commits for updating help info).

As for "readonly" redundant setting, I suppose it was done this way because NBD_SET_FLAGS may fail on older kernels, so this is like a safety guard. See how it is handled in qemu [1]. I would do similarly or leave it as is now.

[1] https://github.com/qemu/qemu/blob/master/nbd/client.c#L669

@liupan1111

This comment has been minimized.

Contributor

liupan1111 commented Mar 14, 2017

I am not sure about the redundant line break. It separates rbd-nbd specific options from ceph generic options, and this looks ok to me. Also this is what i.g. ceph-osd does.

OK, agree, let's keep it.

I would more happy if one decreases number of space characters between an option name and its description. This can be done in the same commit where you fix "an image" (I don't see a point in having many commits for updating help info).

Sure, I will do it in same commit with "an".

As for "readonly" redundant setting, I suppose it was done this way because NBD_SET_FLAGS may fail on older kernels, so this is like a safety guard. See how it is handled in qemu [1]. I would do similarly or leave it as is now.

I agree with this double check, and ok, we could leave it. But there are still two issues for this check:

  1. for NBD_SET_FLAGS, it will both check snapname and readonly option. But for this "double check", it only checks snapname, so I think it is a bug and will change the right value.
  2. In the qemu code you referred, it will set NBD_SET_FLAGS first, if failed, then call this "double check". How about we keep the style?
@trociny

This comment has been minimized.

Contributor

trociny commented Mar 14, 2017

for NBD_SET_FLAGS, it will both check snapname and readonly option. But for this "double check", it only checks snapname, so I think it is a bug and will change the right value.

Agreed.

In the qemu code you referred, it will set NBD_SET_FLAGS first, if failed, then call this "double check". How about we keep the style?

You can only fix setting read_only properly or if you like do similar to what qemu does. Both ways are ok to me.

liupan1111 added some commits Mar 14, 2017

rbd-nbd: fixed typo and format in help info.
Signed-off-by: Pan Liu <liupan1111@gmail.com>
rbd-nbd: should set read_only when --readonly is set.
Signed-off-by: Pan Liu <liupan1111@gmail.com>

@liupan1111 liupan1111 changed the title from rbd-nbd: remove redundant check for readonly, since it is already checked and set by NBD_SET_FLAGS to rbd-nbd: should set read_only when --readonly is set. Mar 14, 2017

@liupan1111 liupan1111 changed the title from rbd-nbd: should set read_only when --readonly is set. to rbd-nbd: should set read_only when --read-only is set. Mar 14, 2017

@liupan1111

This comment has been minimized.

Contributor

liupan1111 commented Mar 14, 2017

@trociny updated.

@trociny trociny self-assigned this Mar 14, 2017

@trociny

LGTM

@trociny trociny changed the title from rbd-nbd: should set read_only when --read-only is set. to rbd-nbd: don't ignore --read-only option in BLKROSET ioctl Mar 16, 2017

@trociny trociny merged commit 5f14439 into ceph:master Mar 16, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details

@liupan1111 liupan1111 deleted the liupan1111:wip-fix-nbd-issues branch Mar 16, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment