[PATCH 3/8] ch_monitor: Update virCHMonitorGet to handle accept a response

[Daniel P. Berrangé]

On Tue, Aug 31, 2021 at 09:08:21PM +0000, Douglas, William wrote:
> On Fri, 2021-08-27 at 17:30 +0100, Daniel P. Berrangé wrote:
> > On Thu, Aug 26, 2021 at 02:59:17PM -0700, William Douglas wrote:
> > > The virCHMonitorGet function needed to be able to return data from
> > > the
> > > hypervisor. This functionality is needed in order for the driver to
> > > support PTY enablement and getting details about the VM state.
> > > 
> > > Signed-off-by: William Douglas <william.douglas at intel.com>
> > > ---
> > >  src/ch/ch_monitor.c | 44
> > > ++++++++++++++++++++++++++++++++++++++++++--
> > >  1 file changed, 42 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c
> > > index b4bc10bfcf..ee7e4683e3 100644
> > > --- a/src/ch/ch_monitor.c
> > > +++ b/src/ch/ch_monitor.c
> > > @@ -611,12 +611,36 @@ virCHMonitorPutNoContent(virCHMonitor *mon,
> > > const char *endpoint)
> > >      return ret;
> > >  }
> > >  
> > > +struct curl_data {
> > > +    char *content;
> > > +    size_t size;
> > > +};
> > > +
> > > +static size_t
> > > +curl_callback(void *contents, size_t size, size_t nmemb, void
> > > *userp)
> > > +{
> > > +    size_t content_size = size * nmemb;
> > > +    struct curl_data *data = (struct curl_data *)userp;
> > > +
> > > +    if (content_size == 0)
> > > +        return content_size;
> > > +
> > > +    data->content = g_realloc(data->content, data->size +
> > > content_size);
> > > +
> > > +    memcpy(&(data->content[data->size]), contents, content_size);
> > > +    data->size += content_size;
> > > +
> > > +    return content_size;
> > > +}
> > > +
> > >  static int
> > > -virCHMonitorGet(virCHMonitor *mon, const char *endpoint)
> > > +virCHMonitorGet(virCHMonitor *mon, const char *endpoint,
> > > virJSONValue **response)
> > >  {
> > >      g_autofree char *url = NULL;
> > >      int responseCode = 0;
> > >      int ret = -1;
> > > +    struct curl_slist *headers = NULL;
> > > +    struct curl_data data = {0};
> > >  
> > >      url = g_strdup_printf("%s/%s", URL_ROOT, endpoint);
> > >  
> > > @@ -628,12 +652,28 @@ virCHMonitorGet(virCHMonitor *mon, const char
> > > *endpoint)
> > >      curl_easy_setopt(mon->handle, CURLOPT_UNIX_SOCKET_PATH, mon-
> > > >socketpath);
> > >      curl_easy_setopt(mon->handle, CURLOPT_URL, url);
> > >  
> > > +    if (response) {
> > > +        headers = curl_slist_append(headers, "Accept:
> > > application/json");
> > > +        headers = curl_slist_append(headers, "Content-Type:
> > > application/json");
> > > +        curl_easy_setopt(mon->handle, CURLOPT_HTTPHEADER,
> > > headers);
> > > +        curl_easy_setopt(mon->handle, CURLOPT_WRITEFUNCTION,
> > > curl_callback);
> > > +        curl_easy_setopt(mon->handle, CURLOPT_WRITEDATA, (void
> > > *)&data);
> > 
> > This bit feels dodgy to me.   mon->handle has its lifetime tied to
> > the virCHMonitor object, but '&data' is allocated on the stack.
> > IOW, this pointer is persistent, but it goes out of scope.
> > 
> > I guess you're relying on the fact that calls to this method are
> > serialized, and we re-write the bad pointer on every call, but
> > if 'response' is NULL on the next call we're not going to do that
> > re-write.
> > 
> > Can all these options be set unconditionally when the mon->handle
> > is first created ?
> 
> They could be but the use of curl_easy_reset (which is just outside the
> context of this patch I'm noticing, sorry) will cause anything we
> initialize the handle with on creation to be reset anyway.
> 
> All the calls using the handle lock the mon and then do a
> curl_easy_reset on the handle prior to use.
> 
> I still understand the dislike for having the handle with potentially
> bad pointers in it though. Another option would just be having the
> handle exist on the stack though we then lose the live connections but
> that wouldn't be an issue for this use case.
> 
> Thoughts?

I'd be happy if we just cleared the bad point at the end of this
method, or even just added another call to easy_reset at end of
this method.


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 :|