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

[Daniel P. Berrangé <berrange@xxxxxxxxxx>]

On Thu, Sep 09, 2021 at 09:48:48AM +0100, Daniel P. Berrangé wrote:
> On Wed, Sep 08, 2021 at 11:01:17AM -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@xxxxxxxxx>
> > ---
> >  src/ch/ch_monitor.c | 46 +++++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 44 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c
> > index b4bc10bfcf..44b99ef07a 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;
> 
> FWIW, the type cast here is redundant as 'void *' can be directly
> assigned to/from any other type.
> 
> > +
> > +    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,30 @@ 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);
> > +    }
> > +
> >      responseCode = virCHMonitorCurlPerform(mon->handle);
> >  
> >      virObjectUnlock(mon);
> >  
> > -    if (responseCode == 200 || responseCode == 204)
> > +    if (responseCode == 200 || responseCode == 204) {
> >          ret = 0;
> > +        if (response) {
> > +            data.content = g_realloc(data.content, data.size + 1);
> > +            data.content[data.size] = 0;
> > +            *response = virJSONValueFromString(data.content);
> 
> Oh, well need to add:
> 
>     if (!*response)
>        return -1;
> 
> in case JSON parsing fails

Actually slight more to avoid a leak.

@@ -665,14 +665,17 @@ virCHMonitorGet(virCHMonitor *mon, const char *endpoint, virJSONValue **response
     virObjectUnlock(mon);
 
     if (responseCode == 200 || responseCode == 204) {
-        ret = 0;
         if (response) {
             data.content = g_realloc(data.content, data.size + 1);
             data.content[data.size] = 0;
             *response = virJSONValueFromString(data.content);
+            if (!*response)
+                goto cleanup;
         }
+        ret = 0;
     }
 
+ cleanup:
     g_free(data.content);
     /* reset the libcurl handle to avoid leaking a stack pointer to data */
     curl_easy_reset(mon->handle);



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