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

[Dietmar Maurer]

> 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.