Re: [Xen-devel] Re: [PATCH V6 3/3] libxl, Introduce a QMP client

[Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>]

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