On Fri, Dec 22, 2017 at 11:06:12AM +0100, Markus Armbruster wrote:
> "Daniel P. Berrange" <berra...@redhat.com> writes:
> > +
> > +/* Syms in libqemustub.a are discarded at .o file granularity.
> > + * To replace monitor_get_fd() we must ensure everything in
> > + * stubs/monitor.c is defined, to make sure monitor.o is discarded
> > + * otherwise we get duplicate syms at link time.
> > + */
> > +Monitor *cur_mon = NULL;
> > +void monitor_init(Chardev *chr, int flags) {}
> > +
> > +/* If a monitor is active (ie cur_mon != NULL), then
> > + * we should be able to use fd=<NAME> syntax
> > + */
> > +static void char_socket_fdpass_mon_test(void)
> > +{
> > + Chardev *chr;
> > + const char *optstr;
> > + QemuOpts *opts;
> > + int fd;
> > +
> > + fd = char_socket_listener();
> > + mon_fd = fd;
> > + cur_mon = g_malloc(1); /* Pretend we have a mon available */
>
> Feels unnecessarily dirty. Suggest to define cur_mon like this:
>
> static Monitor dummy_mon;
> Monitor *cur_mon = &dummy_mon; /* Pretend we have a mon available */
>
> Or in case cur_mon must remain null outside this function, set it like
> this:
>
> Monitor dummy_mon = {0};
> cur_mon = &dummy_mon; /* Pretend we have a mon available */
>
> More of the same below.
FYI, I didn't do that because 'struct Monitor' is defined inside
monitor.c, not exposed in header files. I felt it would be worse
to pollute the header file with what's supposed to be a private
struct definition, just for sake of tests, particularly since
we don't actually need any of the Monitor object contents.
We could create a monitor-internal.h for the "struct Monitor"
definition, if you feel strongly we should take this approach
in the tests instead of my hack here ?
> > diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> > index 1d23f0b742..9400f9a940 100644
> > --- a/util/qemu-sockets.c
> > +++ b/util/qemu-sockets.c
> > @@ -1046,7 +1046,26 @@ int socket_connect(SocketAddress *addr, Error **errp)
> > break;
> >
> > case SOCKET_ADDRESS_TYPE_FD:
> > - fd = monitor_get_fd(cur_mon, addr->u.fd.str, errp);
> > + if (cur_mon) {
> > + fd = monitor_get_fd(cur_mon, addr->u.fd.str, errp);
> > + if (fd < 0) {
> > + return -1;
> > + }
> > + } else {
> > + unsigned long i;
>
> Naming a long @i is bad taste. Let's rename to @ul.
>
> > + if (qemu_strtoul(addr->u.fd.str, NULL, 10, &i) < 0) {
> > + error_setg_errno(errp, errno,
> > + "Unable to parse FD number %s",
> > + addr->u.fd.str);
> > + return -1;
> > + }
> > + fd = i;
>
> Truncates silently. Shouldn't you check for range?
>
> If the parent process screws up passing the file descriptor, fd can
> hijack some random internal file. I'd ask you to catch that if I had
> any idea how to do that easily.
I guess it is just a matter of defining yet another qemu_strtoNN variant
that takes an "int" parameter instead of "long", and does range checking.
> Outside monitor context, you can now use numeric fds, and only numeric
> fds. Makes sense, because named fds are associated with a monitor.
> Note that before the patch, we crashed in monitor_get_fd() dereferencing
> cur_mon.
Yeah, that is fun, but I don't think there's any code path that could
trigger it.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
