On Fri, 2011-07-01 at 16:04 +0100, Anthony PERARD wrote:
> On Fri, Jul 1, 2011 at 09:39, Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx> wrote:
> > On Thu, 2011-06-30 at 18:30 +0100, Anthony PERARD wrote:
> >> QMP stands for QEMU Monitor Protocol and it is used to query information
> >> from QEMU or to control QEMU.
> >>
> >> This implementation will ask QEMU the list of chardevice and store the
> >> path to serial0 in xenstored. So we will be able to use xl console with
> >> QEMU upstream.
> >>
> >> In order to connect to the QMP server, a socket file is created in
> >> /var/run/xen/qmp-$(domid).
> >
> > Should include "libxl" in the name somewhere to differentiate from other
> > potential connections in the future?
>
> Well, this socket belong to QEMU (the creator). So I'm not sure that
> "libxl" in the name is appropriate.
It's created due to libxl asking it too was how I was thinking about it.
> But did you prefer to see "qmp-libxl-$(domid)" as a socket file name ?
Something like that, yeah.
> [...]
>
> >> +/*
> >> + * Handler functions
> >> + */
> >> +
> >> +static libxl__qmp_handler *qmp_init_handler(libxl_ctx *ctx, uint32_t
> >> domid)
> >
> > libxl__qmp_init_handler?
>
> "libxl__" event if it's intern to this file?
I guess it's not required, I'm just used to seeing it. The use of
libxl__gc for internal functions is more important.
> > Generally internal functions should take a libxl__gc *gc rather than a
> > ctx (applies to a bunch of functions).
> >
> >> +{
> >> + libxl__qmp_handler *qmp = NULL;
> >> +
> >> + qmp = calloc(1, sizeof (libxl__qmp_handler));
> >
> > Could be attached to the libxl__gc infrastructure instead of using an
> > explicit free?
>
> So, I should use the garbage collector for the handler, but also for
> the callback ? For the callback, I know when I can free them because
> they will not be used anymore. But if I use the gc, the callbacks will
> not be freed before the dustman is called by the caller.
I don't think using the gc infrastructure should be mandatory, just do
what leads to the cleanest code IMHO.
> > BTW, I was wondering the same thing as I read the parser stuff in the
> > previous patch but that would involve plumbing a *gc through and careful
> > thought about whether or not a given data field could ever reach the
> > libxl user (e.g. JSON_STRING's could ultimately get returned to the
> > caller).
>
> Well, a json_string should be strdup because, with or without gc,
> everything malloc for a json_object will be free with this object.
>
> I will try to use the gc with the json parsing stuff.
>
> >> +static int qmp_open(libxl__qmp_handler *qmp, const char *qmp_socket_path,
> >> + int timeout)
> >> +{
> >> + int ret;
> >> + int i = 0;
> >> +
> >> + qmp->qmp_fd = socket(AF_UNIX, SOCK_STREAM | SOCK_NONBLOCK, 0);
> >> + if (qmp->qmp_fd < 0) {
> >> + return -1;
> >> + }
> >> +
> >> + memset(&qmp->addr, 0, sizeof (&qmp->addr));
> >> + qmp->addr.sun_family = AF_UNIX;
> >> + strncpy(qmp->addr.sun_path, qmp_socket_path,
> >> + sizeof (qmp->addr.sun_path));
> >> +
> >> + do {
> >> + ret = connect(qmp->qmp_fd, (struct sockaddr *) &qmp->addr,
> >> + sizeof (qmp->addr));
> >> + if (ret == 0)
> >> + break;
> >> + if (errno == ENOENT || errno == ECONNREFUSED) {
> >> + /* ENOENT : Socket may not have shown up yet
> >> + * ECONNREFUSED : Leftover socket hasn't been removed yet */
> >> + continue;
> >> + }
> >> + return -1;
> >> + } while ((++i / 5 <= timeout) && (usleep(200 * 1000) <= 0));
> >
> > If we exit this loop because we timed out do we need to set errno to
> > ETIMEDOUT or something similar to indicate this? Or is the existing err
> > errno (one of ENOENT or ECONNREFUSED?) a sufficient indicator?
>
> I think it's should be a ETIMEDOUT. ENOENT or ECONNREFUSED explain
> just why we timeout.
>
> >> +
> >> + return ret;
> >> +}
> >> +
> >> +static void qmp_close(libxl__qmp_handler *qmp)
> >> +{
> >> + callback_id_pair *pp = NULL;
> >> + callback_id_pair *tmp = NULL;
> >> +
> >> + close(qmp->qmp_fd);
> >> + SIMPLEQ_FOREACH(pp, &qmp->callback_list, next) {
> >> + if (tmp)
> >> + free(tmp);
> >> + tmp = pp;
> >
> > That seems like an odd construct, but I suppose it is necessary to work
> > around the lack of a SIMPLEQ_FOREACH variant which is safe against
> > removing the iterator from the list.
>
> Yes.
>
> >> +libxl__qmp_handler *libxl__qmp_initialize(libxl_ctx *ctx, uint32_t domid)
> >> +{
> >> + int ret = 0;
> >> + libxl__qmp_handler *qmp = NULL;
> >> + char *qmp_socket;
> >> + libxl__gc gc = LIBXL_INIT_GC(ctx);
> >> +
> >> + qmp = qmp_init_handler(ctx, domid);
> >> +
> >> + qmp_socket = libxl__sprintf(&gc, "%s/qmp-%d",
> >> + libxl_run_dir_path(), domid);
> >> + if ((ret = qmp_open(qmp, qmp_socket, QMP_SOCKET_CONNECT_TIMEOUT)) <
> >> 0) {
> >> + LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "Connection error");
> >> + qmp_free_handler(qmp);
> >> + return NULL;
> >> + }
> >> +
> >> + LIBXL__LOG(qmp->ctx, LIBXL__LOG_DEBUG, "connected to %s", qmp_socket);
> >> +
> >> + /* Wait for the response to qmp_capabilities */
> >> + while (!qmp->connected) {
> >> + if ((ret = qmp_next(qmp)) < 0) {
> >> + break;
> >
> > Is it an error condition not to get in to the connected state? Should we
> > therefore qmp_free_handler and return NULL? That would save callers
> > checking qmp->connected since they can just assume it is true...
>
> That meen that QEMU did not respond to the "qmp_capabilities" command.
> And it's needed in order send other commands. So yes, it's an error. I
> will return NULL.
>
> > [...]
> >> +int libxl__qmp_initializations(libxl_ctx *ctx, uint32_t domid)
> >> +{
> >> + libxl__qmp_handler *qmp = NULL;
> >> + int ret = 0;
> >> +
> >> + qmp = libxl__qmp_initialize(ctx, domid);
> >> + if (!qmp)
> >> + return -1;
> >> + if (qmp->connected) {
> >
> > ... like here.
> >
> >> + ret = libxl__qmp_query_serial(qmp);
> >> + }
> >> + libxl__qmp_close(qmp);
> >> + return ret;
> >> +}
>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home