Re: [Qemu-devel] [PATCH v2] qga: Add 'mountpoints' argument to guest-fsfreeze-freeze command

[Tomoki Sekiyama]

Hi Eric, thank you for the comments.

On 5/20/14 18:46 , "Eric Blake" <address@hidden> wrote:

>On 05/20/2014 04:01 PM, Tomoki Sekiyama wrote:
>> The patch below is for the command to get filesystems list.
>> 
>> ===
>> From: Tomoki Sekiyama <address@hidden>
>
>You'll want to resend it as a series of patches as a top-level thread;
>not everyone notices a patch buried as a reply.

OK. I will resend as a series (with a fix below).

>> 
>> qga: Add guest-get-fs-info command
>> 
>> Add command to get mounted filesystems information in the guest.
>> The returned value contains a list of mountpoint paths and
>> corresponding disks info such as disk bus type, drive address,
>> and the disk controllers' PCI addresses, so that management layer
>> such as libvirt can resolve the disk backends.
>> In Linux guest, the disk information is resolved from sysfs.
>> 
>> guest-get-fs-info command will return the following result:
>> 
>>     {"return":
>>      [{"name":"dm-1",
>>        "mountpoint":"/mnt/test",
>>        "disk":[
>>         {"bus-type":"scsi","bus":0,"unit":1,"target":0,
>>          "pci-controller":{"bus":0,"slot":10,"domain":0,"function":0}},
>>         {"bus-type":"ide","bus":0,"unit":0,"target":0,
>>          "pci-controller":{"bus":0,"slot":1,"domain":0,"function":1}}],
>>        "type":"xfs"},
>
>Definitely looks like a useful set of information.  I have not reviewed
>the patch closely, but it looks like you have a good API for Linux.  I'm
>a bit worried that it might be hard to translate this API into non-Linux
>guests, but it still seems generic enough; and the argument also holds
>that if this command is not implemented, the guest does not support
>partial freezing.
>>+++ b/qga/qapi-schema.json
>> @@ -646,3 +646,80 @@
>>  { 'command': 'guest-set-vcpus',
>>    'data':    {'vcpus': ['GuestLogicalProcessor'] },
>
>> +
>> +##
>> +# @GuestFilesystemInfo
>> +#
>> +# @name: disk name
>> +# @mountpoint: mount point path
>> +# @type: file system type string
>> +# @disk: an array of disk hardware informations that the volume lies on
>
>s/informations/information/

Will fix it.

>> +#
>> +# Since: 2.1
>> +##
>> +{ 'type': 'GuestFilesystemInfo',
>> +  'data': {'name': 'str', 'mountpoint': 'str', 'type': 'str',
>> +           '*disk': ['GuestDiskAddress']} }
>
>Why is the array optional? Is it something where an empty array has
>different meaning than omitting the array altogether?

Not actually. Currently it omits the array when the disk type is not
supported (non-PCI device like floppy disks or SD cards). But it can
be replaced with an empty array if preferable.

>>+
>> +##
>> +# @guest-get-fs-info:
>> +#
>> +# Returns: The list of filesystems information mounted in the guest.
>> +#          The returned mountpoints may be specified to
>>@guest-fsfreeze-freeze.
>> +#          Network filesystems (such as CIFS and NFS) are not listed.
>> +#
>> +# Since: 2.1
>> +##
>> +{ 'command': 'guest-get-fs-info',
>> +  'returns': ['GuestFilesystemInfo'] }
>
>Should this command support an optional input argument '*name':'str'
>that filters the results to a one-element array about that one named
>mountpoint?

Hmm, I don't have specific use-cases for the name argument.
As other commands like 'guest-network-get-interfaces' don't have a such
filter, I don't have a reason to give a filter for this command.
Anyway, the caller can easily filter the result by him/herself.

Regards,
Tomoki Sekiyama