On Mon, Apr 10, 2017 at 10:08:21AM +0300, Amarnath Valluri wrote: > > > On 07.04.2017 17:41, Daniel P. Berrange wrote: > > On Fri, Apr 07, 2017 at 05:30:31PM +0300, Amarnath Valluri wrote: > > > This change introduces a new TPM backend driver that can communicate with > > > swtpm(software TPM emulator) using unix domain socket interface. > > > > > > Swtpm uses two unix sockets, one for plain TPM commands and responses, > > > and one > > > for out-of-band control messages. > > > > > > The swtpm and associated tools can be found here: > > > https://github.com/stefanberger/swtpm > > > > > > Usage: > > > # setup TPM state directory > > > mkdir /tmp/mytpm > > > chown -R tss:root /tmp/mytpm > > > /usr/bin/swtpm_setup --tpm-state /tmp/mytpm --createek > > > > > > # Ask qemu to use TPM emulator with given tpm state directory > > > qemu-system-x86_64 \ > > > [...] \ > > > -tpmdev > > > emulator,id=tpm0,tpmstatedir=/tmp/mytpm,logfile=/tmp/swtpm.log \ > > > -device tpm-tis,tpmdev=tpm0 \ > > > [...] > > > > > > Signed-off-by: Amarnath Valluri <amarnath.vall...@intel.com> > > > --- > > > configure | 15 +- > > > hmp.c | 21 ++ > > > hw/tpm/Makefile.objs | 1 + > > > hw/tpm/tpm_emulator.c | 927 > > > ++++++++++++++++++++++++++++++++++++++++++++++++++ > > > hw/tpm/tpm_ioctl.h | 243 +++++++++++++ > > > qapi-schema.json | 36 +- > > > qemu-options.hx | 53 ++- > > > tpm.c | 2 +- > > > 8 files changed, 1289 insertions(+), 9 deletions(-) > > > create mode 100644 hw/tpm/tpm_emulator.c > > > create mode 100644 hw/tpm/tpm_ioctl.h > > > +static int tpm_emulator_spawn_emulator(TPMEmulator *tpm_pt) > > > +{ > > > + int fds[2] = { -1, -1 }; > > > + int ctrl_fds[2] = { -1, -1 }; > > > + pid_t cpid; > > > + > > > + if (!tpm_pt->ops->has_data_path) { > > > + if (socketpair(AF_UNIX, SOCK_SEQPACKET, 0, fds) < 0) { > > > + return -1; > > > + } > > > + } > > > + > > > + if (!tpm_pt->ops->has_ctrl_path) { > > > + if (socketpair(AF_UNIX, SOCK_SEQPACKET, 0, ctrl_fds) < 0) { > > > + if (!tpm_pt->ops->has_data_path) { > > > + closesocket(fds[0]); > > > + closesocket(fds[1]); > > > + } > > > + return -1; > > > + } > > > + } > > > + > > > + cpid = qemu_fork(NULL); > > > + if (cpid < 0) { > > > + error_report("tpm-emulator: Fork failure: %s", strerror(errno)); > > > + if (!tpm_pt->ops->has_data_path) { > > > + closesocket(fds[0]); > > > + closesocket(fds[1]); > > > + } > > > + if (!tpm_pt->ops->has_ctrl_path) { > > > + closesocket(ctrl_fds[0]); > > > + closesocket(ctrl_fds[1]); > > > + } > > > + return -1; > > > + } > > > + > > > + if (cpid == 0) { /* CHILD */ > > > + enum { > > > + PARAM_PATH, > > > + PARAM_IFACE, > > > + PARAM_SERVER, PARAM_SERVER_ARGS, > > > + PARAM_CTRL, PARAM_CTRL_ARGS, > > > + PARAM_STATE, PARAM_STATE_ARGS, > > > + PARAM_PIDFILE, PARAM_PIDFILE_ARGS, > > > + PARAM_LOG, PARAM_LOG_ARGS, > > > + PARAM_MAX > > > + }; > > > + > > > + int i; > > > + int data_fd = -1, ctrl_fd = -1; > > > + char *argv[PARAM_MAX+1]; > > > + > > > + /* close all unused inherited sockets */ > > > + if (fds[0] >= 0) > > > + closesocket(fds[0]); > > > + if (ctrl_fds[0] >= 0) > > > + closesocket(ctrl_fds[0]); > > The 'if' checks are pointless - its already guaranteed by the > > fact you check socketpair() status. > socketpairs might not be created in case of data-path & ctrl-path provided, > so i feel these checks are needed. > > > > > + i = STDERR_FILENO + 1; > > > + if (fds[1] >= 0) { > > > + data_fd = dup2(fds[1], i++); > > > + if (data_fd < 0) { > > > + error_report("tpm-emulator: dup2() failure - %s", > > > + strerror(errno)); > > > + goto exit_child; > > > + } > > > + } > > > + if (ctrl_fds[1] >= 0) { > > > + ctrl_fd = dup2(ctrl_fds[1], i++); > > > + if (ctrl_fd < 0) { > > > + error_report("tpm-emulator: dup2() failure - %s", > > > + strerror(errno)); > > > + goto exit_child; > > > + } > > > + } > > > + for ( ; i < _SC_OPEN_MAX; i++) { > > Errr, _SC_OPEN_MAX is not the maximum number of FDs - it is parameter to > > use with sysconf() to query the number of files - you must call sysconf(). > Ya, thanks for educating me, i will change this. > > > > > + closesocket(i); > > close, not closesocket - you can't assume these are all sockets. > Does this change makes any difference, as per include/sysemu/os-posix.h, > closesocket() is define as close(), and this backend is targeted only for > "Linux" targets. Please let me know if i am missing something.
Using closesocket() is misleading even if it does happen to call close() on Linux. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
