Re: [Qemu-devel] [PATCH v5 3/6] add backup related monitor commands

[Markus Armbruster]

Dietmar Maurer <address@hidden> writes:

>> > You can easily extend the API to allow multiple backups (In a fully
>> > backwards compatible way). So there is no need to change that now.
>> 
>> I disagree.
>> 
>> You propose something like:
>> 
>> 1.  -> { "execute": "backup",
>>          "arguments": { "backup-file": "foo.vma" } }
>>     <- { "return": { "deb8e5da-5b69-46d1-86c6-1b715a22809f" } }
>> 
>>     This *deletes* any prior BackupStatus.  Clearly inappropriate with
>>     multiple backups.  How exactly do you propose to extend it in a
>>     backwards compatible way?
>
> It currently deletes the BackupStatus. But this is related to the
> current implementation, not the API definition.

What exactly makes BackupStatus get deleted is as API as it gets :)

>> 2.  -> { "execute": "query-backup" }
>> 
>>     This is specified to return exactly one BackupStatus.
>> 
>>     How exactly do you propose to extend it for multiple backups?
>
> Add an optional 'uuid' parameter. We can auto-select the job if there
> is only one job running

Adds a bit of complexity for no discernable gain.

>> 3.  -> { "execute": "backup-cancel" }
>> 
>>     This is specified to cancel "the current execute backup process".
>> 
>>     This becomes *ill-defined* when multiple backup processes are
>>     executing.
>
> Again, we can add an optional 'uuid' parameter.

Yes, but:

>>     With multiple backup support, we still need to keep it working
>>     unchanged when no or just one backup is executing.  When more are
>>     executing, we have to make it fail, unless an optional argument is
>>     given.

Again, unnecessary complexity.

Moreover, omitting the "optional" argument will break hard unless you
control all monitors.  Because if you don't, you never know whether
there's another backup in flight.

Evolution of APIs occasionally gets us "optional" parameters that are
really mandatory.  Tolerable.  But when a little foresight lets us avoid
such warts, we should.