>> +#define QERR_LOG_DIRTY_BITMAP_ACTIVE \
>> + ERROR_CLASS_GENERIC_ERROR, "Dirty bitmap dump already in progress"
>> +
>
> Please don't add new ERROR_CLASS macros. Instead, just use error_setg()
> and directly output the error message at the call site that produces the
> error.
>
will keep in mind in my next version of the patch.
>> +++ b/qapi-schema.json
>> @@ -4700,3 +4700,13 @@
>> 'btn' : 'InputBtnEvent',
>> 'rel' : 'InputMoveEvent',
>> 'abs' : 'InputMoveEvent' } }
>> +##
>> +# @log-dirty-bitmap
>> +#
>> +# provides the dirty bitmap for time t if specified.
>
> Too light on the specification. You should document all 4 parameters,
> and the default value for when the parameters are omitted.
>
> 'time t' isn't mentioned in any of the parameter names, so I'm assuming
> the docs should mention its significance.
>
ohh!, a bit messy statement, will rectify it.
> Missing a 'Since: 2.1' designation.
>
>> +##
>> +{ 'command' : 'log-dirty-bitmap',
>> + 'data' : { 'filename' : 'str',
>> + '*epochs' : 'int',
>> + '*frequency' : 'int',
>> + '*readable' : 'bool' } }
>
> Seems okay. As long as you use qemu_open(), it should also allow
> management to pass in an fd with the add-fd mechanism.
>
haven't used qemu_open, will make appropriate changes.
>> +
>> +Arguments:
>> +
>> +- "filename": name of the file, in which the bitmap will be saved
>> +- "epochs": number of times, the memory will be logged
>
> s/times,/times/
>
>> +- "frequency": time difference in milliseconds between each epoch
>> +- "readable": dumps the bitmap in hex format (non-binary)
>
> Possibly a poor choice of naming (binary output IS machine-readable; in
> fact, programs prefer parsing binary output over decoding human-readable
> text); maybe 'pretty' or 'text' would have been better; or even invert
> the sense and have it be 'binary' and default to true. I also wonder if
> we even need it - I'd rather have the QMP command always output raw
> binary, and then rely on post-processing to turn it into whatever format
> the user wants to see (od works wonders), than to bloat qemu with having
> to generate a human-readable variant.
>
will remove the readable (poor naming convention) part from the code and will
only dump the data in machine-readable format.
>
>> +static inline void check_epochs_value(int64_t *epochs, Error **errp)
>> +{
>> + if (*epochs <= 0) {
>> + error_setg(errp, "epoch size must be greater than 0, setting"
>> + " a value of 3\n");
>> + *epochs = 3;
>
> This error reporting won't work. Since this function returns void, if
> errp is set at all, the caller must assume failure, rather than hoping
> that you replaced the user's bad input with a saner default value. If
> it was worth reporting the error, then it is not worth trying to fix up
> the value.
>
will correct it.
>> +/*
>> + * copied from arch_init.c file (below function)
>> + */
>> +
>> +static void dirty_bitmap_logging_sync_range(ram_addr_t start,
>> + ram_addr_t length)
>> +{
>
> Rather than copying a function, can you make the original non-static and
> share it between both callers? Otherwise, the two will get out of sync
> if a bug is fixed in one.
>
will have to make certain changes in both files in order for the file
to be generic. will do that.
>> + if (readable) {
>
> readable is undefined if has_readable is false. You are missing
> something like:
>
> if (!has_readable) {
> readable = false;
> }
missed it. will keep this in mind.
>> + if (!has_epochs) {
>> + epochs = 3;
>> + }
>> + if (!has_frequency) {
>> + frequency = 40;
>
> Magic numbers; use a named constant.
>
> --
> Eric Blake eblake redhat com +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>
Thanks for the quick response. Will address all the above issues in my
second version of the patch.
--
Sanidhya
