Re: [Qemu-devel] [PATCH 2/6] bitmap dump code via QAPI framework

[Sanidhya Kashyap]

>> +#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