Questions on README

[ming1]

Hello,

API review, I'm not happy with DeviceParams.

Care to provide more details?

Better errors.
Sadly the kernel driver returns the same error for different error situations. And in
some cases we need to send multiple messages, e.g., we can only send
SetParams if the device' state is DEAD, this requires to first send GetDevInfo to get > the status.

Right, GetDevInfo() is available anytime. And in which cases, you need to send
multiple messages?

The original imlementation supports to set each param in single command, but
that way makes things a bit complicated, but the control interface can be extended,
and we just need to understand the use case.

Thanks,
Ming

[germag]

Hello,

API review, I'm not happy with DeviceParams.

Care to provide more details?

Because it's not very Rusty, currently to create a DeviceParams you need to :

    let params = DeviceParams {
        attrs: Default::default(),
        logical_bs_shift: 9,
        ...
   }

A more rust-idiomatic approach could be using a builder pattern, like the one used in DeviceOptions.
Also I want to know if some fields are related, for example, if dev_sectors always dev size << logical_bs_shift, in that case I can hide some fields, and expose others (e.g., block_size), and do the shifts during object creation.

Better errors.
Sadly the kernel driver returns the same error for different error situations. And in
some cases we need to send multiple messages, e.g., we can only send
SetParams if the device' state is DEAD, this requires to first send GetDevInfo to get > the status.

Right, GetDevInfo() is available anytime. And in which cases, you need to send multiple messages?

Is just a nit, I want to return a descriptive error if the user tries to set parameters to a device in a LIVE state, so I need to send a GetDevInfo() first to get the state. I don't think it's a problem since is just the control path, so one extra command will make no difference.

The original imlementation supports to set each param in single command, but that way makes things a bit complicated, but the control interface can be extended, and we just need to understand the use case.

Thanks, Ming

[ming1]

Hello,

API review, I'm not happy with DeviceParams.

Care to provide more details?

Because it's not very Rusty, currently to create a DeviceParams you need to :

    let params = DeviceParams {
        attrs: Default::default(),
        logical_bs_shift: 9,
        ...
   }

A more rust-idiomatic approach could be using a builder pattern, like the one used in DeviceOptions.

Like DeviceInfo, DeviceParams is ABI between linux kernel and libublksrv,
also DeviceInfo length will not be extended in future, but it will be easy to
see new param added to end of DeviceParams, which is one variable
& extendable ABI data.

DeviceOptions should be just one rust/ublk internal data for building DeviceInfo, DeviceParams, and other KABI data for sending control
commands.

IMO, DeviceInfo, DeviceParams follows the usual kernel ABI pattern,
and this data can't be changed at will any more, and new change
has to be done in extendable style.

Not sure I understand your exact question, if not, please explain in
a bit details.

Also I want to know if some fields are related, for example, if dev_sectors always dev size << logical_bs_shift, in that case I can hide some fields, and expose others (e.g., block_size), and do the shifts during object creation.

Yes, dev_sectors is always same with dev size << logical_bs_shift.

Better errors.
Sadly the kernel driver returns the same error for different error situations. And in
some cases we need to send multiple messages, e.g., we can only send
SetParams if the device' state is DEAD, this requires to first send GetDevInfo to get > the status.

Right, GetDevInfo() is available anytime. And in which cases, you need to send multiple messages?

Is just a nit, I want to return a descriptive error if the user tries to set parameters to a device in a LIVE state, so I need to send a GetDevInfo() first to get the state. I don't think it's a problem since is just the control path, so one extra command will make no difference.

OK, got it. Also if set params command is sent to one device in LIVE
state, -EACCESS is returned to userpace, do you think this return
value isn't accurate enough for user? Or maybe -EBUSY is better?

Thanks,
Ming

[germag]

Like DeviceInfo, DeviceParams is ABI between linux kernel and libublksrv, also DeviceInfo length will not be extended in future, but it will be easy to see new param added to end of DeviceParams, which is one variable & extendable ABI data.

DeviceOptions should be just one rust/ublk internal data for building DeviceInfo, DeviceParams, and other KABI data for sending control commands.

IMO, DeviceInfo, DeviceParams follows the usual kernel ABI pattern, and this data can't be changed at will any more, and new change has to be done in extendable style.

Not sure I understand your exact question, if not, please explain in a bit details.

I think we are talking about two different things, sorry, I should have explained myself better.

user <- (1) -> ublk-rs <- (2) -> kernel

I'm ok with the ABI between ublk-rs and the kernel (2), I'm not talking about theDeviceParams that goes to the kernel, I'm talking about the rust object api exposed to the user (1). I'm not talking to change struct ublk_param_basic just the ublk-rs API.

For example, instead of creating it as:

let params = DeviceParams {
        attrs: Default::default(),
        logical_bs_shift: 9,
        ...
   }

We can do it "the rust way", using the builder pattern:

let params = DeviceParams
        .sector_size(512)
        ...

or

let params = DeviceParams
        .logical_bs_shift(9)
        ...

or something similar.

Also I want to know if some fields are related, for example, if dev_sectors always dev size << logical_bs_shift, in that case I can hide some fields, and expose others (e.g., block_size), and do the shifts during object creation.

Yes, dev_sectors is always same with dev size << logical_bs_shift.

This is the type of things that I want to "hide" in the API to avoid misusing, I want to make it impossible to define:

let params = DeviceParams {
        logical_bs_shift: 9,
        dev_sectors: dev_size >> 8,
        ...
   }

Better errors.
Sadly the kernel driver returns the same error for different error situations. And in
some cases we need to send multiple messages, e.g., we can only send
SetParams if the device' state is DEAD, this requires to first send GetDevInfo to get > the status.

Right, GetDevInfo() is available anytime. And in which cases, you need to send multiple messages?

Is just a nit, I want to return a descriptive error if the user tries to set parameters to a device in a LIVE state, so I need to send a GetDevInfo() first to get the state. I don't think it's a problem since is just the control path, so one extra command will make no difference.

OK, got it. Also if set params command is sent to one device in LIVE state, -EACCESS is returned to userpace, do you think this return value isn't accurate enough for user? Or maybe -EBUSY is better?

Probably EBUSY is a bit better, but I don't think it's a big deal to send an info command to check the state. I was talking about the rust Errors returned as part of Result<>

[ming1]

Like DeviceInfo, DeviceParams is ABI between linux kernel and libublksrv, also DeviceInfo length will not be extended in future, but it will be easy to see new param added to end of DeviceParams, which is one variable & extendable ABI data.
DeviceOptions should be just one rust/ublk internal data for building DeviceInfo, DeviceParams, and other KABI data for sending control commands.
IMO, DeviceInfo, DeviceParams follows the usual kernel ABI pattern, and this data can't be changed at will any more, and new change has to be done in extendable style.
Not sure I understand your exact question, if not, please explain in a bit details.

I think we are talking about two different things, sorry, I should have explained myself better.

user <- (1) -> ublk-rs <- (2) -> kernel

I'm ok with the ABI between ublk-rs and the kernel (2), I'm not talking about theDeviceParams that goes to the kernel, I'm talking about the rust object api exposed to the user (1). I'm not talking to change struct ublk_param_basic just the ublk-rs API.

OK, looks I misunderstood the point, and now ublk-rs API is completely
in your hand, :-)

For example, instead of creating it as:

let params = DeviceParams {
        attrs: Default::default(),
        logical_bs_shift: 9,
        ...
   }

We can do it "the rust way", using the builder pattern:

let params = DeviceParams
        .sector_size(512)
        ...

or

let params = DeviceParams
        .logical_bs_shift(9)
        ...

or something similar.

Also I want to know if some fields are related, for example, if dev_sectors always dev size << logical_bs_shift, in that case I can hide some fields, and expose others (e.g., block_size), and do the shifts during object creation.

Yes, dev_sectors is always same with dev size << logical_bs_shift.

This is the type of things that I want to "hide" in the API to avoid misusing, I want to make it impossible to define:

let params = DeviceParams {
        logical_bs_shift: 9,
        dev_sectors: dev_size >> 8,
        ...
   }

Better errors.
Sadly the kernel driver returns the same error for different error situations. And in
some cases we need to send multiple messages, e.g., we can only send
SetParams if the device' state is DEAD, this requires to first send GetDevInfo to get > the status.

Right, GetDevInfo() is available anytime. And in which cases, you need to send multiple messages?

Is just a nit, I want to return a descriptive error if the user tries to set parameters to a device in a LIVE state, so I need to send a GetDevInfo() first to get the state. I don't think it's a problem since is just the control path, so one extra command will make no difference.

OK, got it. Also if set params command is sent to one device in LIVE state, -EACCESS is returned to userpace, do you think this return value isn't accurate enough for user? Or maybe -EBUSY is better?

Probably EBUSY is a bit better, but I don't think it's a big deal to send an info command to check the state. I was talking about the rust Errors returned as part of Result<>

OK, looks still one ublk-rs specific issue.

Thanks,