Re: [libvirt] [PATCH v6 8/9] backup: Introduce virDomainBackup APIs

On 3/26/19 1:13 PM, Eric Blake wrote: TL;DR: After more thought, I'm reluctant to make a domain job an opaque type. I'm also leaning towards withdrawing virDomainListJobIds() from the set of APIs I'm proposing for 5.2, on the grounds that with a documented magic job id of '0' that means "the currently running job" for both virDomainBackupGetXMLDesc() and virDomainBackupEnd(), we don't lose any functionality over what qemu currently provides (of no parallel jobs) while still having a sane API that can be properly used with non-zero job ids in the future when we do add parallel job support. Actually, it turns out to make life a bit more difficult if the type is opaque. My demo at KVM Forum showed the following: $ virsh backup-begin $dom backup_pull_full.xml Backup id 1 created backup used description from 'backup_pull_full.xml' $ # do thired-party copy $ virsh backup-end $dom 1 Backup id 1 completed Note the time and process separation between creation and completion; if we make virDomainBackupEnd() take an opaque pointer, then the first virsh instance has to call virDomainJobFree(job) prior to exiting (well, that depends a lot on whether we make virDomainJobPtr light-weight scalar-only that just records type/id where a leak is harmless, or heavy-weight where it also records the virConnectionPtr and/or virDomainPtr it was associated for better API safety, but where freeing the pointer is important in a long-running client). Then, the second virsh call would have to call virDomainJobLookupId(domain, 1) to re-obtain the proper virDomainJobPtr object to hand to virDomainBackupEnd(domain, job), and then call virDomainJobFree(ptr) again to conclude things. Thus, making it an opaque type adds a lot of API overhead to using things, compared to the interface that virsh wants to give the user of just printing an integer and letting the user re-input that integer; we also have to worry about whether multiple consecutive API calls can result in TOCTTOU races that could confuse things. You raised the question of whether storing type/id as an opaque type would add any safety to prevent the user from requesting an API on an unrelated job ID. But after thinking about it more, my argument is that it doesn't really help - when calling virDomainBackupEnd(domain, 1), we've already implied that we want to operate on backup job 1 (and throw an error if there is no backup job with id 1, even if there is some other job type that overlaps namespaces and has an id 1). Calling virDomainBackupEnd(domain, virDomainJobCompose(domain, VIR_DOMAIN_JOB_BACKUP, 1)) adds verbosity, but I don't see it adding safety. Along those lines we can look at file descriptors as an example - they are plain integers, but still serve as an opaque type (you use fcntl(F_GETFD/F_GETFL) to learn things about the fd, and have various errors like ESPIPE or EISDIR when operating on the wrong id compared to the function trying to do something on an fd). Then there's the question of what precedence we have for opaque vs. user-visible types in public headers. We already expose virTypedParameter as a user-visible type rather than opaque, and the more time I've spent on the code this afternoon playing with the idea of making jobs full-bodied and opaque, the more I'm finding it to be too much overhead. For example, the python generator can easily support ints, but needs a lot more modifications to support a new opaque type. If we insist on making virDomainBackupBegin() return an opaque type instead of an integer id, I'm very doubtful on whether the changes to libvirt-python would be made in time for making the 5.2 release. Thinking for the future, when we eventually do get around to adding full parallel job ID support, it will touch a lot of APIs: - virDomainGetJobInfo(), virDomainGetJobStats(), and virDomainAbortJob() - currently operate on a default job (in particular, the only job actually affected by these are jobs started by virDomainMigrate() and friends) - virDomainBlockJobAbort(), virDomainGetBlockJobInfo(), virDomainBlockJobSetSpeed() - currently act on a single job per disk, where multiple jobs can be run in parallel if you have multiple disks, but again with no id; affects jobs started by virDomainBlockCopy(), virDomainBlockCommit(), virDomainBlockPull(), and friends - my new virDomainBlockBackup() - which I'm trying to design for the future by returning a job id - we may want to add a virDomainJobGetDefaultID(domain) or even virDomainBlockJobGetID(domain, diskname) to make it easy to reverse-engineer which job id was utilized by the legacy APIs, but at the same time would want new APIs that return the job id directly when creating one of those jobs - we may want to add a virDomainJobSetDefaultID(domain, id) to select which job (out of multiple running in parallel) will be targetted by the existing job functions that don't take a job id - side note: virDomainAbortJob() vs. virDomainBlockJobAbort() is confusing; it's hard to remember which name form to use So, that said, what are the options for 5.2? If we commit virDomainBackupEnd(virDomainPtr, int job, flags) in time for Wednesday's freeze, we can still change the API (go with a different parameter type) and/or revert the addition up until Friday. If the release happens with virDomainBackupEnd() but no virDomainListAllJobs(), we can still run backup jobs (you just can't easily query what job id was chosen, but have to assume a default job). But I could also document that although virDomainBackupBegin() always returns a non-zero job id on success, we guarantee that the reserved job id '0' stands for "the currently-running job". If there is no active backup job, or if there are more than one active backup jobs, then id "0" will fail - but we can also document that if a server does not support the virDomainListAllJobs() API, then that server does not support parallel active backup jobs. Thus, the use of a job id is a formality for 5.2 (we know that qemu won't have parallel jobs, so we can blindly pass 0 for the job id to all calls that document a job id - but still code virsh to allow the user to pass job id '1' or whatever the output from backup-begin said if they don't want to rely on the default of job '0'). At the same time, down the road (in release 6.0 or whenever we finally add parallel job ids) that we DO finally add virDomainListAllJobs() and all the other APIs needed for true job management, we've avoided needing to add wrapper APIs to fix virDomainBackupBegin/End not working with job ids (even though we have to add wrappers for all the other legacy job-related APIs that did not work with ids from the get-go). We could decide that a job id is too much for right now, and state that virDomainBackupBegin() returns 0 on success rather than a job id (for exactly one job possible), and where virDomainBackupEnd() no longer needs a job id parameter (in other words, making it more like existing virDomainMigrate()/virDomainAbortJob()). Then future parallel jobs would have to add a new API for backup as well as everything else (less work now, and at least consistent with other APIs, but more work down the road). I'm less in favor of this option, because it feels like adding technical debt. If the release happens now with both virDomainBackupEnd() taking a job id, AND the addition of a virDomainListAllJobs() API, then we are stuck supporting the virDomainListAllJobs() API even if it turns out we don't like the design it locked us into. But if we think we have a sufficiently extensible design for that one API, even without the rest of a nice parallel job framework, and feel we can reasonably add remaining job API enhancements later, then more power to us for getting it right. So having said all that, I'm leaning towards keeping job ids as an integer, having no virDomainListAllJobs() in this release, and adding sufficient documentation and magic around a reserved job id '0' so that callers can start experimenting with backup jobs in 5.2. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org