(Re-send because missing reply-to-all)
Really appreciate your time and valuable input Eric. I almost didn't submit
any patches, so there is quite a lot to learn about it.
I'd agree with you on this, but memsave/pmemsave function are using stdio,
thus I was trying to be consistent with them.
> > + if (!f) {
> > + error_setg_file_open(errp, errno, filename);
> > + return;
> > + }
> > +
> > + while (size != 0) {
> > + l = fread(buf, 1, sizeof(buf), f);
> > + if (l > size)
> > + l = size;
>
> How about error handling as below, the same error handling is used by
memsave/pmemsave.
l = fread(buf, 1, sizeof(buf), f);
if (l ! = sizeof(buf)) {
error_set(errp, QERR_IO_ERROR);
goto eixt;
}
> This is lousy at error detection. Again, you should be using read(),
> not fread(), and properly erroring out on read failures rather than
> silently ignoring them.
>
>
> > + .name = "pmemload",
> > + .args_type = "val:l,size:i,filename:s",
>
> Would it make sense to put filename as the FIRST argument, with val and
> size as optional arguments (defaulting to reading in at address 0 and
> for the size determined by the length of the input file), rather than
> forcing the user to pass in a size up front?
>
> Same reason as above, I though it could be easier if pmemload takes the
same arguments as memsave/pmemsave.
Again, really thanks for your patient.
Best Regards
On Tue, Apr 8, 2014 at 12:42 PM, Eric Blake <ebl...@redhat.com> wrote:
> On 04/08/2014 01:30 PM, Baojun Wang wrote:
>
> Your subject line is extremely long. In general, we strive for
> something less than 60 characters, so that 'git shortlog -30' can
> display the entire line in an 80-column terminal. Also, the subject
> mentions pmemsave, but your commit mentions pmemload - it's very
> misleading to provide a patch where the commit message doesn't even
> mention the name of the command the patch is adding. I would suggest a
> subject line of:
>
> qmp: add pmemload command
>
> > I found this could be useful to have qemu-softmmu as a cross debugger
> (launch
> > with -s -S command line option), then if we can have a command to load
> guest
> > physical memory, we can use cross gdb to do some target debug which gdb
> cannot
> > do directly.
> >
>
> Your patch is lacking a Signed-off-by designation, therefore we cannot
> accept it yet.
>
> > Many thanks to Eric Blake for review the patch.
>
> This comment may be useful to reviewers, but is not part of the commit
> itself, so it belongs better...
>
> >
> > ---
>
> ...here, after the --- separator.
>
>
> >
> > +void qmp_pmemload(int64_t addr, int64_t size, const char *filename,
> > + Error **errp)
> > +{
> > + FILE *f;
> > + uint32_t l;
> > + uint8_t buf[1024];
> > +
> > + f = fopen(filename, "rb");
>
> Rather than using fopen() and FILE* operations, I'd prefer you use
> qemu_open() and lower-level read() operations. In particular, this will
> automatically make it possible for management applications to pass in
> '/dev/fdset/1' to reuse a file descriptor that they passed in to qemu,
> rather than forcing qemu to directly open the file.
>
> > + if (!f) {
> > + error_setg_file_open(errp, errno, filename);
> > + return;
> > + }
> > +
> > + while (size != 0) {
> > + l = fread(buf, 1, sizeof(buf), f);
> > + if (l > size)
> > + l = size;
>
> This is lousy at error detection. Again, you should be using read(),
> not fread(), and properly erroring out on read failures rather than
> silently ignoring them.
>
>
> > + .name = "pmemload",
> > + .args_type = "val:l,size:i,filename:s",
>
> Would it make sense to put filename as the FIRST argument, with val and
> size as optional arguments (defaulting to reading in at address 0 and
> for the size determined by the length of the input file), rather than
> forcing the user to pass in a size up front?
>
> > +++ b/qapi-schema.json
> > @@ -1708,6 +1708,26 @@
> > 'data': {'val': 'int', 'size': 'int', 'filename': 'str'} }
> >
> > ##
> > +# @pmemload:
> > +#
> > +# Load a portion of guest physical memory from a file.
> > +#
> > +# @val: the physical address of the guest to start from
> > +#
> > +# @size: the size of memory region to load
> > +#
> > +# @filename: the file to load the memory from as binary data
> > +#
> > +# Returns: Nothing on success
> > +#
> > +# Since: 2.1
> > +#
> > +# Notes: Errors were not reliably returned until 2.1
>
> Delete this line. I guess I didn't make it clear in my first review
> that this line is bogus copy and paste. You don't need it. pmemsave
> needed it, because pmemsave went through some iterations where earlier
> versions were buggy. But your pmemload command is brand new, and should
> not have bugs worth worrying about.
>
> --
> Eric Blake eblake redhat com +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>
>
