Re: [Qemu-devel] [PATCH 1/2] Split out dump-guest-memory memory mapping code

[Andreas Färber]

Am 23.04.2013 17:54, schrieb Jens Freimann:
> On Tue, Apr 23, 2013 at 09:41:43AM -0600, Eric Blake wrote:
>> On 04/23/2013 09:30 AM, Jens Freimann wrote:
>>> Split out dump-guest-memory memory mapping code to allow dumping without
>>> memory mapping
>>>
>>> The qemu dump.c code currently requires CONFIG_HAVE_CORE_DUMP as well as
>>> CONFIG_HAVE_GET_MEMORY_MAPPING. This allows for dumping with and without 
>>> paging.
>>> Some architectures will provide only the non-paging case. This patch allows 
>>> an
>>> architecture to provide dumping even when CONFIG_HAVE_GET_MEMORY_MAPPING is 
>>> not
>>> available. To do that, we split out the common code and provide stub 
>>> functions
>>> for the non-paging case. If -p is specified on a target that doesn't 
>>> support it,
>>> we will pass an error to the calling code.
>>>
>>> Signed-off-by: Ekaterina Tumanova <address@hidden>
>>> Signed-off-by: Jens Freimann <address@hidden>
>>> ---
>>
>>> +++ b/include/qapi/qmp/qerror.h
>>> @@ -249,4 +249,7 @@ void assert_no_error(Error *err);
>>>  #define QERR_SOCKET_CREATE_FAILED \
>>>      ERROR_CLASS_GENERIC_ERROR, "Failed to create socket"
>>>  
>>> +#define QERR_UNSUPPORTED_COMMAND_OPTION \
>>> +    ERROR_CLASS_GENERIC_ERROR, "Option(s) %s of %s command not supported 
>>> for %s"
>>
>> Rather than adding a new QERR_* constant here, just use error_setg() in
>> qmp_dump_guest_memory() in the first place.
> 
> ok, will fix

Any progress on this? I'd be happy to take an updated version of this
generic patch and leave the s390x part for Alex et al. to review.

If you resend, note that Git complained about a trailing white line in
the new file; otherwise I'd be fine with a diff+Sob or suggestion for
the error_setg() text. Thanks.

>> This raises an interesting question about introspection - how will
>> management apps (such as libvirt) be able to determine whether the
>> paging command is supported for a given architecture?  Do we need to
>> expand the 'MachineInfo' QMP datatype so that 'query-machines' can tell
>> us whether a given machine will support or reject attempts to set
>> 'paging':true during 'dump-guest-memory'?
> 
> sounds reasonable to me.

This discussion seems orthogonal since we are not changing behavior.
AFAICS we were only hiding the documentation of the dump-guest-memory
command but not shielding its implementation and the stub raised
QERR_UNSUPPORTED, so raising an error when the implementation doesn't
support paging doesn't seem a regression to me and can be revisited when
we actually add a second implementation, whether s390x or arm.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg