From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42557) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YbDOD-00073y-C3 for qemu-devel@nongnu.org; Thu, 26 Mar 2015 15:24:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YbDO9-0000eW-6V for qemu-devel@nongnu.org; Thu, 26 Mar 2015 15:24:25 -0400 Received: from mx1.redhat.com ([209.132.183.28]:56910) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YbDO8-0000eM-Sa for qemu-devel@nongnu.org; Thu, 26 Mar 2015 15:24:21 -0400 Message-ID: <55145CE2.50801@redhat.com> Date: Thu, 26 Mar 2015 13:24:18 -0600 From: Eric Blake MIME-Version: 1.0 References: <1427286325-42082-1-git-send-email-itamar@guardicore.com> In-Reply-To: <1427286325-42082-1-git-send-email-itamar@guardicore.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="JnBCMOwk3WrmMTgaMWbUj5asCdCLWpEH0" Subject: Re: [Qemu-devel] [PATCH 3/3] qga support process list, netstat and file stat/delete List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: itamar.tal4@gmail.com, qemu-devel@nongnu.org Cc: ori@guardicore.com, ariel@guardicore.com, mdroth@linux.vnet.ibm.com, pavel@guardicore.com, Itamar Tal This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --JnBCMOwk3WrmMTgaMWbUj5asCdCLWpEH0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 03/25/2015 06:25 AM, itamar.tal4@gmail.com wrote: > From: Itamar Tal 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. >=20 > this patch add support for some more functionality in the qemu-guest-ag= ent, > 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 >=20 > Itamar, > Guardicore > itamar@guardicore.com >=20 > --- > 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? > + =20 > +## > +# @guest-file-delete: > +# > +# Delete a file in the guest's operating system > +# > +# Returns:=20 > +# > +# 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' } > +=20 > +## > +# @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': 's= tr', > + '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? > + =20 > +## > +# @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' ] } > + =20 > +## > +# @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': 'GuestTcp= ProtocolState', > + 'if-family': 'GuestIpAddressType', 'protocol': 'GuestIpProtocol',= > + 'start-time': 'uint64'}} > +=20 > + ## > +# @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'] } > +=20 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. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --JnBCMOwk3WrmMTgaMWbUj5asCdCLWpEH0 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJVFFziAAoJEKeha0olJ0NqkskH/ivqPO85oygk+yMwW0NITQrg EPfd6Dq34P3hkHHrc9NiZdz6ov58qEBVxzGn+JSxpdGd7ZMdROpBeQlw8UELCGAY EN3ASfdFzWTd//8wKsK+mEymEOj0etSZ2knSiegZfEM01gsa8i1YC+tk8NckhEXF XoNoQ5MHY9bm1R1pJMkmDdLGKL7xT9rD//y9F5yPpe+CoahSFQCLr2mNQmtYPQHD WVSvdYwdnX/wkDQl708vPiKFrwDZbk+1HdAyFaE7W/4R6ocWrwPmxMc16tak6w/Z 9JBGOu6kBUZdZPWckCusYU9/afrR0E6an6ORIK67zEtx6ozsUwsEgfOS80H5f98= =V8M3 -----END PGP SIGNATURE----- --JnBCMOwk3WrmMTgaMWbUj5asCdCLWpEH0--