Re: [PATCH 3/8] ch_monitor: Update virCHMonitorGet to handle accept a response
On Tue, Aug 31, 2021 at 09:08:21PM +, 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
> > > ---
> > > 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
> > > *));
> >
> > This bit feels dodgy to me. mon->handle has its lifetime tied to
> > the virCHMonitor object, but '' 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 :|
Re: [PATCH 3/8] ch_monitor: Update virCHMonitorGet to handle accept a response
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
> > ---
> > 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
> > *));
>
> This bit feels dodgy to me. mon->handle has its lifetime tied to
> the virCHMonitor object, but '' 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?
>
> > + }
> > +
> > 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);
> > + }
> > + }
> > +
> > + g_free(data.content);
> >
> > return ret;
> > }
>
> Regards,
> Daniel
Re: [PATCH 3/8] ch_monitor: Update virCHMonitorGet to handle accept a response
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
> ---
> 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 *));
This bit feels dodgy to me. mon->handle has its lifetime tied to
the virCHMonitor object, but '' 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 ?
> +}
> +
> 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);
> +}
> +}
> +
> +g_free(data.content);
>
> return ret;
> }
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 :|
[PATCH 3/8] ch_monitor: Update virCHMonitorGet to handle accept a response
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
---
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 *));
+}
+
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);
+}
+}
+
+g_free(data.content);
return ret;
}
--
2.31.1
