[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v2 02/15] blockjob: Decouple the ID from the dev
From: |
Alberto Garcia |
Subject: |
Re: [Qemu-block] [PATCH v2 02/15] blockjob: Decouple the ID from the device name in the BlockJob struct |
Date: |
Wed, 22 Jun 2016 16:42:02 +0200 |
User-agent: |
Notmuch/0.18.2 (http://notmuchmail.org) Emacs/24.4.1 (i586-pc-linux-gnu) |
On Wed 22 Jun 2016 02:42:24 PM CEST, Kevin Wolf wrote:
>> We want block jobs to have unique, arbitrary identifiers that are not
>> tied to a block device, so this patch decouples the ID from the
>> device name in the BlockJob structure.
>>
>> The ID is generated automatically for the moment, in later patches
>> we'll allow the user to set it.
>
> This approach requires us to keep track of both a device name and an
> ID in order to maintain compatibility, and we always need to check
> both when a job is referenced. This model is already a pain with
> node-name and BB name, I wouldn't want to introduce more instances.
>
> Why don't we go the easy route and make the ID configurable, but still
> default to the device name?
That was my first approach when I started to write the series, but I
discarded it for several reasons:
- More confusing API: we'd have many instances of fields and parameters
named 'device' holding something that is not a device name or anything
similar.
We'd have e.g. block-commit(id, device), and the created job would
have a 'device' that is not the user-specified 'device' but the
user-specified 'id', which has no relation to devices.
- More risk of collisions: with the current system if no ID is specified
a new one is be generated. That's guaranteed to be valid and unique.
If we default to the device name we have no such guarantee.
- Potential compatibility break: there's a field that used to hold the
name of the device but it no longer does.
I don't think any of them is a showstopper, and as you said if a client
is recent enough to understand job IDs it should also expect the device
field to hold something different from a device name. But I'm not a big
fan of this kind of scenarios where the semantics of a returned value
depend on what we can assume that the caller is able to handle.
I thought adding a new 'ID' field was simpler. The device name is still
a device name (where it makes sense). The default ID is guaranteed to be
valid and guaranteed not to clash with user-defined IDs. The API is (in
my opinion) more clear.
The only problems that I can think of:
- BlockJobInfo and the events expose the 'device' field which is
superfluous.
- block-job-{pause,resume,...} can take an ID or a device name.
All that said, I'm open to change the implementation, but I'd like to
make sure that the problems of keeping the ID and device name merged are
considered.
Berto
- Re: [Qemu-block] [PATCH v2 01/15] stream: Fix prototype of stream_start(), (continued)
- [Qemu-block] [PATCH v2 03/15] blockjob: Add block_job_get(), Alberto Garcia, 2016/06/22
- [Qemu-block] [PATCH v2 07/15] backup: Add 'job-id' parameter to 'blockdev-backup' and 'drive-backup', Alberto Garcia, 2016/06/22
- [Qemu-block] [PATCH v2 06/15] mirror: Add 'job-id' parameter to 'blockdev-mirror' and 'drive-mirror', Alberto Garcia, 2016/06/22
- [Qemu-block] [PATCH v2 08/15] stream: Add 'job-id' parameter to 'block-stream', Alberto Garcia, 2016/06/22
- [Qemu-block] [PATCH v2 05/15] blockjob: Add 'job_id' parameter to block_job_create(), Alberto Garcia, 2016/06/22
- [Qemu-block] [PATCH v2 02/15] blockjob: Decouple the ID from the device name in the BlockJob struct, Alberto Garcia, 2016/06/22
[Qemu-block] [PATCH v2 09/15] commit: Add 'job-id' parameter to 'block-commit', Alberto Garcia, 2016/06/22
[Qemu-block] [PATCH v2 14/15] blockjob: Add 'id' parameter to 'block-job-complete', Alberto Garcia, 2016/06/22
[Qemu-block] [PATCH v2 04/15] block: Simplify find_block_job() and make it accept a job ID, Alberto Garcia, 2016/06/22
[Qemu-block] [PATCH v2 13/15] blockjob: Add 'id' parameter to 'block-job-resume', Alberto Garcia, 2016/06/22
[Qemu-block] [PATCH v2 12/15] blockjob: Add 'id' parameter to 'block-job-pause', Alberto Garcia, 2016/06/22