Re: [libvirt] [PATCH v2 0/5] Guest filesystem information API

On 19.11.2014 23:58, John Ferlan wrote: > > > On 11/17/2014 06:26 PM, Tomoki Sekiyama wrote: >> Hi, >> >> This is v2 of patchset to add virDomainGetFSInfo API. >> >> * changes in v1->v2: >> -[all] removed redundant NULL element at the last of returned info array >> -[3/5] make error messages in qemu_agent.c consistent with other commands >> -[4/5] added a test case for 2 items in info->devAliases >> -[5/5] added a pod document for virsh domfsinfo command >> (v1: http://www.redhat.com/archives/libvir-list/2014-October/msg00001.html ) >> >> * summary >> This series implements a new virDomainGetFSInfo API, that returns a list of >> mounted filesystems information in the guest, collected via the guest agent. >> >> The returned info contains mountpoints and disk device alias named in >> libvirt, so we can know which mountpoints should be frozen by >> virDomainFSFreeze to take snapshots of a part of disks. >> >> --- >> Tomoki Sekiyama (5): >> Implement public API for virDomainGetFSInfo >> remote: Implement the remote protocol for virDomainGetFSInfo >> qemu: Implement the qemu driver for virDomainGetFSInfo >> qemu: add test for qemuAgentGetFSInfo >> virsh: expose virDomainGetFSInfo >> >> >> daemon/remote.c | 117 ++++++++++++++++++++ >> include/libvirt/libvirt-domain.h | 21 ++++ >> src/conf/domain_conf.c | 71 ++++++++++++ >> src/conf/domain_conf.h | 6 + >> src/driver-hypervisor.h | 6 + >> src/libvirt.c | 66 +++++++++++ >> src/libvirt_private.syms | 1 >> src/libvirt_public.syms | 6 + >> src/qemu/qemu_agent.c | 178 ++++++++++++++++++++++++++++++ >> src/qemu/qemu_agent.h | 2 >> src/qemu/qemu_driver.c | 48 ++++++++ >> src/remote/remote_driver.c | 92 ++++++++++++++++ >> src/remote/remote_protocol.x | 32 +++++ >> src/remote_protocol-structs | 21 ++++ >> src/rpc/gendispatch.pl | 1 >> tests/Makefile.am | 1 >> tests/qemuagentdata/qemuagent-fsinfo.xml | 39 +++++++ >> tests/qemuagenttest.c | 143 ++++++++++++++++++++++++ >> tools/virsh-domain.c | 74 ++++++++++++ >> tools/virsh.pod | 9 ++ >> 20 files changed, 933 insertions(+), 1 deletion(-) >> create mode 100644 tests/qemuagentdata/qemuagent-fsinfo.xml >> >> -- >> >> Tomoki Sekiyama >> > > > I reviewed the 'libvirt' specific changes - had a few comments and have > made changes to my review branch as specified. As long as you're OK > with those changes I will get these pushed. > > I'm also hoping someone else (eblake?) can look at the remote_protocol.x > changes to ensure they encompass everything they are supposed to. Also > that the usage of QEMU_JOB_QUERY not _MODIFY for the GetFSInfo seems > more appropriate and is in line with the various remote_protocol.x > settings (@acl/@generate stuff settings). @generate is correct, since both, client and server implementations are provided. @acl looks consistent to the rest. Correct, for querying domain info you need to have read permission and that's it. And yes, the job should be _QUERY since we are querying info, not modifying domain in any way. However, for some reason I see this build error after 2/5: make[3]: Entering directory '/home/zippy/work/libvirt/libvirt.git/tools/wireshark/src' CC libvirt_la-packet-libvirt.lo In file included from libvirt/protocol.h:5:0, from packet-libvirt.h:112, from packet-libvirt.c:36: ./libvirt/remote.h: In function 'dissect_xdr_remote_typed_param_value': ./libvirt/remote.h:470:5: error: unknown type name 'remote_nonnull_string' remote_nonnull_string type = 0; ^ ./libvirt/remote.h:473:5: warning: implicit declaration of function 'xdr_remote_nonnull_string' [-Wimplicit-function-declaration] if (!xdr_remote_nonnull_string(xdrs, &type)) ^ Makefile:1934: recipe for target 'libvirt_la-packet-libvirt.lo' failed make[3]: *** [libvirt_la-packet-libvirt.lo] Error 1