From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56409) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bFjMc-0004bs-QM for qemu-devel@nongnu.org; Wed, 22 Jun 2016 10:42:48 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bFjMX-0000kr-Qt for qemu-devel@nongnu.org; Wed, 22 Jun 2016 10:42:45 -0400 From: Alberto Garcia In-Reply-To: <20160622124224.GG6134@noname.redhat.com> References: <6034f1747fb838368a057df8463084b6a2326fe4.1466598035.git.berto@igalia.com> <20160622124224.GG6134@noname.redhat.com> Date: Wed, 22 Jun 2016 16:42:02 +0200 Message-ID: MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v2 02/15] blockjob: Decouple the ID from the device name in the BlockJob struct List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: qemu-devel@nongnu.org, qemu-block@nongnu.org, Max Reitz , Eric Blake , Jeff Cody 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