> I'm coming into this review late, so I'm not sure what your series is adding
> that cannot already be done with external snapshots and migration to file.
This series try to do backup more efficient (see docu in [PATCH v2 1/6]).
> But any time someone proposes new QMP commands that libvirt might have
> to interact with, I try to chime in:
libvirt does not need to use any of those commands.
> > +# returned, no backup process has been initiated
> > +#
> > +# @errmsg: #optional error message (only returned if status is
> > +'error') # # @total: #optional total amount of bytes involved in the
> > +backup process # # @transferred: #optional amount of bytes already
> > +backed up.
> > +#
> > +# @zero-bytes: #optional amount of 'zero' bytes detected.
> > +#
> > +# @start-time: #optional time (epoch) when backup job started.
>
> Should you account for sub-second resolution here
Why (I doubt we can backup within a second).
> > +# @backupfile: the backup file name
>
> I guess the fdset mechanism is how libvirt would pass in a pipe fd rather than
> supplying an actual file name. Does your implementation allow for pipes, or
> must it be seekable?
You can pass /dev/fdset/XXX.
My implementation allows pipes.
(I will post the necessary changes for /dev/fdset/ next time).
> > +# @format: format of the backup file
>
> Rather than letting this be a free-form string, it would be nicer as an enum
> of
> actually supported formats.
ok
> > +# @config-filename: #optional name of a configuration file to include
> > +into # the backup archive.
> > +#
> > +# @speed: #optional the maximum speed, in bytes per second # #
> > +Returns: the uuid of the backup job
>
> UUID in raw byte format, or in ASCII format? (Assuming the latter)
ASCII
> > +#
> > +# Since: 1.4.0
> > +##
> > +{ 'command': 'backup', 'data': { 'backupfile': 'str', '*format':
> > +'str',
>
> backupfile sounds like a run-on; is it any better to name it backup-file?
>
> > + '*config-filename': 'str',
>
> especially when compared with config-filename
ok. Many thanks for the review.
