Re: [Qemu-block] [PATCH v4] qemu-io: add pattern file for write command

[Denis Plotnikov]

On 31.05.2019 10:14, Stefano Garzarella wrote:
> On Fri, May 31, 2019 at 06:21:13AM +0000, Denis Plotnikov wrote:
>>
>>
>> On 30.05.2019 11:26, Stefano Garzarella wrote:
>>> On Thu, May 30, 2019 at 11:10:55AM +0300, Denis Plotnikov wrote:
>>>> The patch allows to provide a pattern file for write
>>>> command. There was no similar ability before.
>>>>
>>>> Signed-off-by: Denis Plotnikov <address@hidden>
>>>> ---
>>>
>>> Hi Denis,
>>> for next versions I suggest to describe here, after the ---, the changes
>>> with previous versions. In this way the review should be easier.
>> ---
>> Sure
>>>
>>>>    qemu-io-cmds.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++----
>>>>    1 file changed, 75 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
>>>> index 09750a23ce..1c6a5e4faf 100644
>>>> --- a/qemu-io-cmds.c
>>>> +++ b/qemu-io-cmds.c
>>>> @@ -21,6 +21,7 @@
>>>>    #include "qemu/option.h"
>>>>    #include "qemu/timer.h"
>>>>    #include "qemu/cutils.h"
>>>> +#include "string.h"
>>>>    
>>>>    #define CMD_NOFILE_OK   0x01
>>>>    
>>>> @@ -343,6 +344,61 @@ static void *qemu_io_alloc(BlockBackend *blk, size_t 
>>>> len, int pattern)
>>>>        return buf;
>>>>    }
>>>>    
>>>> +static void *qemu_io_alloc_from_file(BlockBackend *blk, size_t len,
>>>> +                                     char *file_name)
>>>> +{
>>>> +    char *buf, *buf_pos;
>>>> +    FILE *f = fopen(file_name, "r");
>>>> +    int l;
>>>> +
>>>> +    if (!f) {
>>>> +        printf("'%s': %s\n", file_name, strerror(errno));
>>>> +        return NULL;
>>>> +    }
>>>> +
>>>> +    if (qemuio_misalign) {
>>>> +        len += MISALIGN_OFFSET;
>>>> +    }
>>>> +    buf = blk_blockalign(blk, len);
>>>> +    memset(buf, 0, len);
>>>> +
>>>> +    buf_pos = buf;
>>>> +
>>>> +    while (len > 0) {
>>>> +        l = fread(buf_pos, sizeof(char), len, f);
>>>> +
>>>> +        if (feof(f)) {
>>>> +            rewind(f);
>>>> +        }
>>>> +
>>>> +        if (ferror(f)) {
>>>> +            printf("'%s': %s\n", file_name, strerror(errno));
>>>> +            goto error;
>>>> +        }
>>>> +
>>>> +        if (l == 0) {
>>>> +            printf("'%s' is empty\n", file_name);
>>>> +            goto error;
>>>> +        }
>>>
>>> Could it happen that we read some bytes, than we reached the EOF, so we made
>>> the rewind() and fread() returns 0?
>>> If that can happen, maybe we should change it in this way:
>>>
>>>           if (l == 0 && buf_pos == buf) {
>>>               printf("'%s' is empty\n", file_name);
>>>               goto error;
>>>           }
>> This shouldn't happen. To get that situation we need to read exactly the
>> file length (from the current position to the end) on the first step,
>> then rewind, read again and in that case get l == 0 and feof == true.
>> But reading the exact length means that we asked for that size, so the
>> buffer is fully populated, thus len == 0 and we leave the loop.
>>
>> So, l == 0 is only when we read an empty file on the first iteration.
>>
> 
> Right!
> 
>>>> @@ -983,8 +1039,9 @@ static int write_f(BlockBackend *blk, int argc, char 
>>>> **argv)
>>>>       /* Some compilers get confused and warn if this is not initialized.  
>>>> */
>>>>       int64_t total = 0;
>>>>       int pattern = 0xcd;
>>>> +    char *file_name;
> 
> I think we should initialize file_name to NULL here to silent the compiler.
> I applied this patch and I had this error:
> /home/stefano/repos/qemu/qemu-io-cmds.c: In function ‘write_f’:
> /home/stefano/repos/qemu/qemu-io-cmds.c:351:15: error: ‘file_name’ may be 
> used uninitialized in this function [-Werror=maybe-uninitialized]
>       FILE *f = fopen(file_name, "r");
>                 ^~~~~~~~~~~~~~~~~~~~~
> /home/stefano/repos/qemu/qemu-io-cmds.c:1042:11: note: ‘file_name’ was 
> declared here
>       char *file_name;
>             ^~~~~~~~~
> cc1: all warnings being treated as errors
> make: *** [/home/stefano/repos/qemu/rules.mak:69: qemu-io-cmds.o] Error 1
> make: *** Waiting for unfinished jobs....
ok, will do
> 
> Thanks,
> Stefano
> 

-- 
Best,
Denis