Re: [Qemu-devel] [PATCH 3/3] qga support process list, netstat and file stat/delete

[Eric Blake]

On 03/25/2015 06:25 AM, address@hidden wrote:
> From: Itamar Tal <address@hidden>

This patch says 3/3, but I see no 1/3 or 2/3 (let alone a 0/3 cover
letter) in the mail archives.

> 
> this patch add support for some more functionality in the qemu-guest-agent,
> both for windows and linux. Main added features are:
> - interface listing in Windows
> - Process list in Windows
> - network connections enumeration in Windows
> - file delete for both Windows and Posix
> - file stat() for both Windows and Posix
> - system uptime for both Windows and Posix
> 
> Itamar,
> Guardicore
> address@hidden
> 
> ---
>  qga/commands-posix.c    |  49 +++++
>  qga/commands-win32.c    | 552 
> +++++++++++++++++++++++++++++++++++++++++++++++-
>  qga/commands.c          |  25 +++
>  qga/guest-agent-core.h  |   2 +
>  qga/main.c              |   9 +
>  qga/qapi-schema.json    | 172 +++++++++++++++
>  qga/win32-definitions.h | 115 ++++++++++
>  qga/win32-iptypes.h     | 411 +++++++++++++++++++++++++++++++++++

That's awfully big.  Please split it up into one or two closely-related
qga commands per patch, rather than lots of commands all at once.

> +++ b/qga/qapi-schema.json
> @@ -891,3 +891,175 @@
>  ##
>  { 'command': 'guest-get-memory-block-info',
>    'returns': 'GuestMemoryBlockInfo' }
> +
> +##
> +# @GuestFileStat
> +#
> +# @st_mode: file access permissions mode
> +# @st_ino: file inode id
> +# @st_dev: file device node

Is this portable to be exposing?  Windows in particular is notorious for
not having decent ino/dev in its basic stat() call, and anything we do
to fake it is likely to break someone's use case.  I'd rather play it
conservative and go with fewer fields that we know are portable than to
try and expose all of struct stat, if we can't prove that the extra
fields would be useful.

> +# @st_nlink: number of links pointing to the file
> +# @st_uid: file user id
> +# @st_gid: file group id
> +# @st_atime: file access time

Insufficient.  Please report timestamps as a struct containing seconds
since Epoch (1970) AND nanoseconds.  Truncating timestamps to the
nearest second by returning only an int is not friendly.

> +# @st_mtime: file modification time
> +# @st_ctime: file creation time

Absolutely not.  ctime is NOT creation time, but last change time.
Creation time is typically reported as Birthtime on decent Unix-y
systems.  Yes, many (but not all) filesystems track FOUR times per file,
and even though POSIX only requires three timestamps, all of BSD, Linux,
and Cygwin have their struct stat() report all four timestamps (where
birthtime is an obvious all-0s for a filesystem that doesn't track it).
(The fact that windows tracks BOTH birth time AND change time, but
reports ctime as birthtime in its stat() call, doesn't help matters).

> +# @st_size: file size in bytes

The names documented here...[1]

> +#
> +# Since: 2.3

You missed 2.3.  The earliest this can go in is 2.4.

> +##
> +{ 'type': 'GuestFileStat',
> +  'data': {'mode': 'int', 'inode': 'int', 'dev': 'int',
> +                     'nlink': 'int', 'uid': 'int', 'gid': 'int',

TAB damage.  Please make sure your patch passes ./scripts/checkpatch.pl
before submitting.

> +                     'size': 'uint64', 'atime': 'int', 'mtime': 'int',
> +                     'ctime': 'int'

[1]...don't match the names actually used here.

> +           }}
> +
> +##
> +# @guest-file-stat:
> +#
> +# Get the stat() for a file in the guest's operating system
> +#
> +# Returns: hostname string.

Wrong copy-and-paste.

> +#
> +# Since 2.3

2.4.  (Won't mention it again)

> +##
> +{ 'command': 'guest-file-stat',
> +  'data': { 'path': 'str' },
> +  'returns': 'GuestFileStat' }

Are you planning on an fstat counterpart of an open handle?  Are you
planning on handling the symlink aspect of stat vs. lstat?  Should you
be providing more of an fstatat() interface where you can specify a name
relative to an already-open directory handle?

> +  
> +##
> +# @guest-file-delete:
> +#
> +# Delete a file in the guest's operating system
> +#
> +# Returns: 
> +#
> +# Since 2.3
> +##
> +{ 'command': 'guest-file-delete',
> +  'data': { 'path': 'str' }}

Can this delete directories?  If so, should it be named remove?  Should
it provide removeat() functionality of specifying a name relative to an
already-open directory handle?

> +
> +##
> +# @guest-get-hostname:
> +#
> +# Get the hostname of the guest's operating system
> +#
> +# Returns: hostname string.
> +#
> +# Since 2.3
> +##
> +{ 'command': 'guest-get-hostname',
> +  'returns': 'str' }
> + 
> +##
> +# @guest-uptime:
> +#
> +# Get the time in seconds since the guest machine operating system was 
> started
> +#
> +# Returns: uptime in seconds

Is this sufficient, or should you provide more resolution?

> +#
> +# Since 2.3
> +##
> +{ 'command': 'guest-uptime',
> +  'returns': 'uint64' }
> +
> +##
> +# @GuestProcessInfo
> +#
> +# @process-id: the process unique id
> +# @parent-id: the process parent unique id
> +# @process-name: the name of the process
> +# @image-path: full path of the process image
> +# @session-id: the session id of the process
> +#
> +# Since: 2.3
> +##
> +{ 'type': 'GuestProcessInfo',
> +  'data': {'process-id': 'int', 'parent-id': 'int', 'process-name': 'str',
> +                     'image-path': 'str', 'session-id': 'int'}}

Do you really need this, or is the existing guest-file-open on /proc/...
sufficient for getting at the same information without needing a new
command?

> +  
> +##
> +# @guest-get-process-list:
> +#
> +# Get the list of active processes on the guest operating system
> +#
> +# Returns: array of active processes
> +#
> +# Since 2.3
> +##
> +{ 'command': 'guest-get-process-list',
> +  'returns': ['GuestProcessInfo'] }

Almost guaranteed to be stale (processes will probably come and go
during the time that the guest agent is collecting data to return), and
rather expensive to collect.  Should it take an optional argument for
filtering the results to a specific pid?

> +
> +##
> +# @GuestIpProtocol:
> +#
> +# An enumeration of supported IP protocols
> +#
> +# @tcp: TCP
> +#
> +# @udp: UDP
> +#
> +# Since: 2.3
> +##
> +{ 'enum': 'GuestIpProtocol',
> +  'data': [ 'tcp', 'udp' ] }
> +  
> +##
> +# @GuestTcpProtocolState:
> +#
> +# An enumeration of TCP connection state
> +#
> +# @closed: CLOSED
> +# @listen: LISTEN
> +# @syn-sent: SYN_SENT
> +# @syn-rcvd: SYN_RCVD
> +# @established: ESTABLISHED
> +# @fin-wait1: FIN_WAIT1
> +# @fin-wait2: FIN_WAIT2
> +# @close-wait: CLOSE_WAIT
> +# @closing: CLOSING
> +# @last-ack: LAST_ACK
> +# @time-wait: TIME_WAIT
> +# @delete-tcb: DELETE_TCB
> +#
> +# Since: 2.3
> +##
> +{ 'enum': 'GuestTcpProtocolState',
> +  'data': [ 'closed', 'listen', 'syn-sent', 'syn-rcvd', 'established',
> +                     'fin-wait1', 'fin-wait2', 'close-wait', 'closing',
> +                     'last-ack', 'time-wait', 'delete-tcb' ] }
> +
> +##
> +# @GuestActiveConnection
> +#
> +# @if-family: 4 / 6
> +# @protocol: TCP / UDP
> +# @source-addr: the source IP address of the connection
> +# @source-port: the source port of the connection
> +# @dest-addr: the destination IP address of the connection
> +# @dest-port: the destination port of the connection
> +# @owner-process_id: the process unique id for the connection owner
> +# @state: connection protocol state
> +# @start-time: time where bind() was called for the connection
> +#
> +# Since: 2.3
> +##
> +{ 'type': 'GuestActiveConnection',
> +  'data': {  'source-addr': 'str', 'source-port': 'int', 'dest-addr': 'str',
> +                     'dest-port': 'int', 'owner-process_id': 'int', 'state': 
> 'GuestTcpProtocolState',
> +                     'if-family': 'GuestIpAddressType', 'protocol': 
> 'GuestIpProtocol',
> +                     'start-time': 'uint64'}}
> + 
> + ##
> +# @guest-get-active-connections:
> +#
> +# Get the list of active connections on the guest operating system
> +#
> +# Returns: array of active connections
> +#
> +# Since 2.3
> +##
> +{ 'command': 'guest-get-active-connections',
> +  'returns': ['GuestActiveConnection'] }
> + 

I'm not sure if all of these commands make sense to add, but I _am_ sure
that you are trying to add too much in one commit.  Please break this up
before resubmitting.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature