src/qemu/qemu_domain.c | 36 ++++++++++++++++++++---------------- src/qemu/qemu_domain.h | 6 ++++-- src/qemu/qemu_process.c | 4 ++-- 3 files changed, 26 insertions(+), 20 deletions(-)
---
src/qemu/qemu_domain.c | 36 ++++++++++++++++++++----------------
src/qemu/qemu_domain.h | 6 ++++--
src/qemu/qemu_process.c | 4 ++--
3 files changed, 26 insertions(+), 20 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 3d3f796d85..0d2edf4dbe 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -150,7 +150,7 @@ qemuDomainObjFromDomain(virDomainPtr domain)
struct _qemuDomainLogContext {
- virObject parent;
+ GObject parent;
int writefd;
int readfd; /* Only used if manager == NULL */
@@ -160,37 +160,46 @@ struct _qemuDomainLogContext {
virLogManagerPtr manager;
};
-static virClassPtr qemuDomainLogContextClass;
+G_DEFINE_TYPE(qemuDomainLogContext, qemu_domain_log_context, G_TYPE_OBJECT);
static virClassPtr qemuDomainSaveCookieClass;
-static void qemuDomainLogContextDispose(void *obj);
+static void qemuDomainLogContextFinalize(GObject *obj);
static void qemuDomainSaveCookieDispose(void *obj);
static int
qemuDomainOnceInit(void)
{
- if (!VIR_CLASS_NEW(qemuDomainLogContext, virClassForObject()))
- return -1;
-
if (!VIR_CLASS_NEW(qemuDomainSaveCookie, virClassForObject()))
return -1;
return 0;
}
+static void qemu_domain_log_context_init(qemuDomainLogContext *logctxt G_GNUC_UNUSED)
+{
+}
+
+static void qemu_domain_log_context_class_init(qemuDomainLogContextClass *klass)
+{
+ GObjectClass *obj = G_OBJECT_CLASS(klass);
+
+ obj->finalize = qemuDomainLogContextFinalize;
+}
+
VIR_ONCE_GLOBAL_INIT(qemuDomain);
static void
-qemuDomainLogContextDispose(void *obj)
+qemuDomainLogContextFinalize(GObject *object)
{
- qemuDomainLogContextPtr ctxt = obj;
+ qemuDomainLogContextPtr ctxt = QEMU_DOMAIN_LOG_CONTEXT(object);
VIR_DEBUG("ctxt=%p", ctxt);
virLogManagerFree(ctxt->manager);
VIR_FREE(ctxt->path);
VIR_FORCE_CLOSE(ctxt->writefd);
VIR_FORCE_CLOSE(ctxt->readfd);
+ G_OBJECT_CLASS(qemu_domain_log_context_parent_class)->finalize(object);
}
const char *
@@ -10557,13 +10566,7 @@ qemuDomainLogContextPtr qemuDomainLogContextNew(virQEMUDriverPtr driver,
qemuDomainLogContextMode mode)
{
g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
- qemuDomainLogContextPtr ctxt = NULL;
-
- if (qemuDomainInitialize() < 0)
- return NULL;
-
- if (!(ctxt = virObjectNew(qemuDomainLogContextClass)))
- return NULL;
+ qemuDomainLogContextPtr ctxt = QEMU_DOMAIN_LOG_CONTEXT(g_object_new(QEMU_TYPE_DOMAIN_LOG_CONTEXT, NULL));
VIR_DEBUG("Context new %p stdioLogD=%d", ctxt, cfg->stdioLogD);
ctxt->writefd = -1;
@@ -10632,7 +10635,8 @@ qemuDomainLogContextPtr qemuDomainLogContextNew(virQEMUDriverPtr driver,
return ctxt;
error:
- virObjectUnref(ctxt);
+ if (ctxt)
+ g_object_unref(ctxt);
return NULL;
}
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 3929ee9ca1..3c270b87a2 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -37,6 +37,8 @@
#include "virmdev.h"
#include "virchrdev.h"
#include "virobject.h"
+#include "internal.h"
+#include <glib-object.h>
#include "logging/log_manager.h"
#include "virdomainmomentobjlist.h"
#include "virenum.h"
@@ -598,9 +600,9 @@ struct qemuProcessEvent {
void qemuProcessEventFree(struct qemuProcessEvent *event);
-typedef struct _qemuDomainLogContext qemuDomainLogContext;
+#define QEMU_TYPE_DOMAIN_LOG_CONTEXT qemu_domain_log_context_get_type()
+G_DECLARE_FINAL_TYPE(qemuDomainLogContext, qemu_domain_log_context, QEMU, DOMAIN_LOG_CONTEXT, GObject);
typedef qemuDomainLogContext *qemuDomainLogContextPtr;
-G_DEFINE_AUTOPTR_CLEANUP_FUNC(qemuDomainLogContext, virObjectUnref);
typedef struct _qemuDomainSaveCookie qemuDomainSaveCookie;
typedef qemuDomainSaveCookie *qemuDomainSaveCookiePtr;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 499d39a920..9f7c245c7e 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -1925,7 +1925,7 @@ static void
qemuProcessMonitorLogFree(void *opaque)
{
qemuDomainLogContextPtr logCtxt = opaque;
- virObjectUnref(logCtxt);
+ g_clear_object(&logCtxt);
}
@@ -1978,7 +1978,7 @@ qemuConnectMonitor(virQEMUDriverPtr driver, virDomainObjPtr vm, int asyncJob,
driver);
if (mon && logCtxt) {
- virObjectRef(logCtxt);
+ g_object_ref(logCtxt);
qemuMonitorSetDomainLog(mon,
qemuProcessMonitorReportLogError,
logCtxt,
--
2.24.1
On Tue, Mar 10, 2020 at 17:30:01 +0530, Gaurav Agrawal wrote:
> ---
> src/qemu/qemu_domain.c | 36 ++++++++++++++++++++----------------
> src/qemu/qemu_domain.h | 6 ++++--
> src/qemu/qemu_process.c | 4 ++--
> 3 files changed, 26 insertions(+), 20 deletions(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 3d3f796d85..0d2edf4dbe 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -150,7 +150,7 @@ qemuDomainObjFromDomain(virDomainPtr domain)
[...]
> static int
> qemuDomainOnceInit(void)
> {
> - if (!VIR_CLASS_NEW(qemuDomainLogContext, virClassForObject()))
> - return -1;
> -
> if (!VIR_CLASS_NEW(qemuDomainSaveCookie, virClassForObject()))
> return -1;
>
> return 0;
> }
>
> +static void qemu_domain_log_context_init(qemuDomainLogContext *logctxt G_GNUC_UNUSED)
> +{
> +}
There's no reason to break coding style rules in this kind of refactor
nor to make it inconsistent in span of 20 lines.
> +
> +static void qemu_domain_log_context_class_init(qemuDomainLogContextClass *klass)
> +{
> + GObjectClass *obj = G_OBJECT_CLASS(klass);
> +
> + obj->finalize = qemuDomainLogContextFinalize;
> +}
> +
On Tue, Mar 10, 2020 at 01:33:42PM +0100, Peter Krempa wrote:
> On Tue, Mar 10, 2020 at 17:30:01 +0530, Gaurav Agrawal wrote:
> > ---
> > src/qemu/qemu_domain.c | 36 ++++++++++++++++++++----------------
> > src/qemu/qemu_domain.h | 6 ++++--
> > src/qemu/qemu_process.c | 4 ++--
> > 3 files changed, 26 insertions(+), 20 deletions(-)
> >
> > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> > index 3d3f796d85..0d2edf4dbe 100644
> > --- a/src/qemu/qemu_domain.c
> > +++ b/src/qemu/qemu_domain.c
> > @@ -150,7 +150,7 @@ qemuDomainObjFromDomain(virDomainPtr domain)
>
> [...]
>
> > static int
> > qemuDomainOnceInit(void)
> > {
> > - if (!VIR_CLASS_NEW(qemuDomainLogContext, virClassForObject()))
> > - return -1;
> > -
> > if (!VIR_CLASS_NEW(qemuDomainSaveCookie, virClassForObject()))
> > return -1;
> >
> > return 0;
> > }
> >
> > +static void qemu_domain_log_context_init(qemuDomainLogContext *logctxt G_GNUC_UNUSED)
> > +{
> > +}
>
> There's no reason to break coding style rules in this kind of refactor
> nor to make it inconsistent in span of 20 lines.
If you're refering to the use of underscores, then this is expected
and indeed required because these method names are auto-generated
by the GObject macros. This should only be the case for three
specific methods (_init, _class_init and _get_type), with the rest
all following normal style. See the virIdentity conversion for
the prior example of this.
>
> > +
> > +static void qemu_domain_log_context_class_init(qemuDomainLogContextClass *klass)
> > +{
> > + GObjectClass *obj = G_OBJECT_CLASS(klass);
> > +
> > + obj->finalize = qemuDomainLogContextFinalize;
> > +}
> > +
>
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 :|
On Tue, Mar 10, 2020 at 12:42:47 +0000, Daniel Berrange wrote:
> On Tue, Mar 10, 2020 at 01:33:42PM +0100, Peter Krempa wrote:
> > On Tue, Mar 10, 2020 at 17:30:01 +0530, Gaurav Agrawal wrote:
> > > ---
> > > src/qemu/qemu_domain.c | 36 ++++++++++++++++++++----------------
> > > src/qemu/qemu_domain.h | 6 ++++--
> > > src/qemu/qemu_process.c | 4 ++--
> > > 3 files changed, 26 insertions(+), 20 deletions(-)
> > >
> > > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> > > index 3d3f796d85..0d2edf4dbe 100644
> > > --- a/src/qemu/qemu_domain.c
> > > +++ b/src/qemu/qemu_domain.c
> > > @@ -150,7 +150,7 @@ qemuDomainObjFromDomain(virDomainPtr domain)
> >
> > [...]
> >
> > > static int
> > > qemuDomainOnceInit(void)
> > > {
> > > - if (!VIR_CLASS_NEW(qemuDomainLogContext, virClassForObject()))
> > > - return -1;
> > > -
> > > if (!VIR_CLASS_NEW(qemuDomainSaveCookie, virClassForObject()))
> > > return -1;
> > >
> > > return 0;
> > > }
> > >
> > > +static void qemu_domain_log_context_init(qemuDomainLogContext *logctxt G_GNUC_UNUSED)
> > > +{
> > > +}
> >
> > There's no reason to break coding style rules in this kind of refactor
> > nor to make it inconsistent in span of 20 lines.
>
> If you're refering to the use of underscores, then this is expected
> and indeed required because these method names are auto-generated
> by the GObject macros. This should only be the case for three
> specific methods (_init, _class_init and _get_type), with the rest
> all following normal style. See the virIdentity conversion for
> the prior example of this.
Yeah, I meant mainly that (also the newline after the type).
Can't we at least keep camel-case for the type itself?
'qemuDomainLogContext_init'
That way at least looking for the symbol name will be less painful.
On Tue, Mar 10, 2020 at 01:48:39PM +0100, Peter Krempa wrote:
> On Tue, Mar 10, 2020 at 12:42:47 +0000, Daniel Berrange wrote:
> > On Tue, Mar 10, 2020 at 01:33:42PM +0100, Peter Krempa wrote:
> > > On Tue, Mar 10, 2020 at 17:30:01 +0530, Gaurav Agrawal wrote:
> > > > ---
> > > > src/qemu/qemu_domain.c | 36 ++++++++++++++++++++----------------
> > > > src/qemu/qemu_domain.h | 6 ++++--
> > > > src/qemu/qemu_process.c | 4 ++--
> > > > 3 files changed, 26 insertions(+), 20 deletions(-)
> > > >
> > > > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> > > > index 3d3f796d85..0d2edf4dbe 100644
> > > > --- a/src/qemu/qemu_domain.c
> > > > +++ b/src/qemu/qemu_domain.c
> > > > @@ -150,7 +150,7 @@ qemuDomainObjFromDomain(virDomainPtr domain)
> > >
> > > [...]
> > >
> > > > static int
> > > > qemuDomainOnceInit(void)
> > > > {
> > > > - if (!VIR_CLASS_NEW(qemuDomainLogContext, virClassForObject()))
> > > > - return -1;
> > > > -
> > > > if (!VIR_CLASS_NEW(qemuDomainSaveCookie, virClassForObject()))
> > > > return -1;
> > > >
> > > > return 0;
> > > > }
> > > >
> > > > +static void qemu_domain_log_context_init(qemuDomainLogContext *logctxt G_GNUC_UNUSED)
> > > > +{
> > > > +}
> > >
> > > There's no reason to break coding style rules in this kind of refactor
> > > nor to make it inconsistent in span of 20 lines.
> >
> > If you're refering to the use of underscores, then this is expected
> > and indeed required because these method names are auto-generated
> > by the GObject macros. This should only be the case for three
> > specific methods (_init, _class_init and _get_type), with the rest
> > all following normal style. See the virIdentity conversion for
> > the prior example of this.
>
> Yeah, I meant mainly that (also the newline after the type).
>
> Can't we at least keep camel-case for the type itself?
>
> 'qemuDomainLogContext_init'
>
> That way at least looking for the symbol name will be less painful.
We had considered that in the original virIdentity conversion, but
decided mixing two different naming styles in one identifier was
even more ugly.
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 :|
On Tue, Mar 10, 2020 at 12:53:59 +0000, Daniel Berrange wrote: > On Tue, Mar 10, 2020 at 01:48:39PM +0100, Peter Krempa wrote: > > On Tue, Mar 10, 2020 at 12:42:47 +0000, Daniel Berrange wrote: > > > On Tue, Mar 10, 2020 at 01:33:42PM +0100, Peter Krempa wrote: > > > > On Tue, Mar 10, 2020 at 17:30:01 +0530, Gaurav Agrawal wrote: > > > > > --- > > > > > src/qemu/qemu_domain.c | 36 ++++++++++++++++++++---------------- > > > > > src/qemu/qemu_domain.h | 6 ++++-- > > > > > src/qemu/qemu_process.c | 4 ++-- > > > > > 3 files changed, 26 insertions(+), 20 deletions(-) [...] > > > > > > > > There's no reason to break coding style rules in this kind of refactor > > > > nor to make it inconsistent in span of 20 lines. > > > > > > If you're refering to the use of underscores, then this is expected > > > and indeed required because these method names are auto-generated > > > by the GObject macros. This should only be the case for three > > > specific methods (_init, _class_init and _get_type), with the rest > > > all following normal style. See the virIdentity conversion for > > > the prior example of this. > > > > Yeah, I meant mainly that (also the newline after the type). > > > > Can't we at least keep camel-case for the type itself? > > > > 'qemuDomainLogContext_init' > > > > That way at least looking for the symbol name will be less painful. > > We had considered that in the original virIdentity conversion, but > decided mixing two different naming styles in one identifier was > even more ugly. While ugly I still think that for greppability/searchability it's way better to keep the object name using camel case. In the end as you've pointed out it's just for the specific methods named above. Making it look ugly is IMO less worse than missing something when trying to understand how the code works.
On Tue, Mar 10, 2020 at 02:02:47PM +0100, Peter Krempa wrote: > On Tue, Mar 10, 2020 at 12:53:59 +0000, Daniel Berrange wrote: > > On Tue, Mar 10, 2020 at 01:48:39PM +0100, Peter Krempa wrote: > > > On Tue, Mar 10, 2020 at 12:42:47 +0000, Daniel Berrange wrote: > > > > On Tue, Mar 10, 2020 at 01:33:42PM +0100, Peter Krempa wrote: > > > > > On Tue, Mar 10, 2020 at 17:30:01 +0530, Gaurav Agrawal wrote: > > > > > > --- > > > > > > src/qemu/qemu_domain.c | 36 ++++++++++++++++++++---------------- > > > > > > src/qemu/qemu_domain.h | 6 ++++-- > > > > > > src/qemu/qemu_process.c | 4 ++-- > > > > > > 3 files changed, 26 insertions(+), 20 deletions(-) > > [...] > > > > > > > > > > > There's no reason to break coding style rules in this kind of refactor > > > > > nor to make it inconsistent in span of 20 lines. > > > > > > > > If you're refering to the use of underscores, then this is expected > > > > and indeed required because these method names are auto-generated > > > > by the GObject macros. This should only be the case for three > > > > specific methods (_init, _class_init and _get_type), with the rest > > > > all following normal style. See the virIdentity conversion for > > > > the prior example of this. > > > > > > Yeah, I meant mainly that (also the newline after the type). > > > > > > Can't we at least keep camel-case for the type itself? > > > > > > 'qemuDomainLogContext_init' > > > > > > That way at least looking for the symbol name will be less painful. > > > > We had considered that in the original virIdentity conversion, but > > decided mixing two different naming styles in one identifier was > > even more ugly. > > While ugly I still think that for greppability/searchability it's way > better to keep the object name using camel case. In the end as you've > pointed out it's just for the specific methods named above. Making it > look ugly is IMO less worse than missing something when trying to > understand how the code works. IIRC, Ján raised the issue of avoiding mixed camelcase/snakecase namings originally, so CC'ing to see if he's ok with changing back again ? 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 :|
On a Tuesday in 2020, Daniel P. Berrangé wrote: >On Tue, Mar 10, 2020 at 02:02:47PM +0100, Peter Krempa wrote: >> On Tue, Mar 10, 2020 at 12:53:59 +0000, Daniel Berrange wrote: >> > On Tue, Mar 10, 2020 at 01:48:39PM +0100, Peter Krempa wrote: >> > > On Tue, Mar 10, 2020 at 12:42:47 +0000, Daniel Berrange wrote: >> > > > On Tue, Mar 10, 2020 at 01:33:42PM +0100, Peter Krempa wrote: >> > > > > On Tue, Mar 10, 2020 at 17:30:01 +0530, Gaurav Agrawal wrote: >> > > > > > --- >> > > > > > src/qemu/qemu_domain.c | 36 ++++++++++++++++++++---------------- >> > > > > > src/qemu/qemu_domain.h | 6 ++++-- >> > > > > > src/qemu/qemu_process.c | 4 ++-- >> > > > > > 3 files changed, 26 insertions(+), 20 deletions(-) >> >> [...] >> >> > > > > >> > > > > There's no reason to break coding style rules in this kind of refactor >> > > > > nor to make it inconsistent in span of 20 lines. >> > > > >> > > > If you're refering to the use of underscores, then this is expected >> > > > and indeed required because these method names are auto-generated >> > > > by the GObject macros. This should only be the case for three >> > > > specific methods (_init, _class_init and _get_type), with the rest >> > > > all following normal style. See the virIdentity conversion for >> > > > the prior example of this. >> > > >> > > Yeah, I meant mainly that (also the newline after the type). >> > > >> > > Can't we at least keep camel-case for the type itself? >> > > >> > > 'qemuDomainLogContext_init' >> > > >> > > That way at least looking for the symbol name will be less painful. >> > >> > We had considered that in the original virIdentity conversion, but >> > decided mixing two different naming styles in one identifier was >> > even more ugly. >> >> While ugly I still think that for greppability/searchability it's way >> better to keep the object name using camel case. In the end as you've >> pointed out it's just for the specific methods named above. Making it >> look ugly is IMO less worse than missing something when trying to >> understand how the code works. > >IIRC, Ján raised the issue of avoiding mixed camelcase/snakecase >namings originally, so CC'ing to see if he's ok with changing >back again ? I don't care about it enough to send another e-mail after this one :) However the case convetion is a part of the macro documentation: https://developer.gnome.org/gobject/unstable/gobject-Type-Information.html#G-DEFINE-TYPE:CAPS Jano > >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 :| >
On Tue, Mar 10, 2020 at 18:23:42 +0100, Ján Tomko wrote: > On a Tuesday in 2020, Daniel P. Berrangé wrote: [...] > > > While ugly I still think that for greppability/searchability it's way > > > better to keep the object name using camel case. In the end as you've > > > pointed out it's just for the specific methods named above. Making it > > > look ugly is IMO less worse than missing something when trying to > > > understand how the code works. > > > > IIRC, Ján raised the issue of avoiding mixed camelcase/snakecase > > namings originally, so CC'ing to see if he's ok with changing > > back again ? > > I don't care about it enough to send another e-mail after this one :) > > However the case convetion is a part of the macro documentation: > https://developer.gnome.org/gobject/unstable/gobject-Type-Information.html#G-DEFINE-TYPE:CAPS Hmm, sorry I didn't really look into that in the first place so thanks for teaching me. While I don't like yet another convention, I'm okay with the naming in this patch based on the above.
On a Tuesday in 2020, Gaurav Agrawal wrote: >--- > src/qemu/qemu_domain.c | 36 ++++++++++++++++++++---------------- > src/qemu/qemu_domain.h | 6 ++++-- > src/qemu/qemu_process.c | 4 ++-- > 3 files changed, 26 insertions(+), 20 deletions(-) > [...] >@@ -10632,7 +10635,8 @@ qemuDomainLogContextPtr qemuDomainLogContextNew(virQEMUDriverPtr driver, > return ctxt; > > error: >- virObjectUnref(ctxt); >+ if (ctxt) >+ g_object_unref(ctxt); g_object_unref is safe to call with a NULL argument, the "if (ctxt)" check is not needed here. > return NULL; > } > >diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h >index 3929ee9ca1..3c270b87a2 100644 >--- a/src/qemu/qemu_domain.h >+++ b/src/qemu/qemu_domain.h >@@ -37,6 +37,8 @@ > #include "virmdev.h" > #include "virchrdev.h" > #include "virobject.h" >+#include "internal.h" The "internal.h" addition is not necessary for this patch - all the types are in glib-object. In the virIdentity conversion, other parts of the include file relied on "internal.h" being included indirectly through "virobject.h". >+#include <glib-object.h> Please put the includes in angle brackets at the beginning of the file: https://libvirt.org/hacking.html#includes Otherwise the patch looks good to me. Jano > #include "logging/log_manager.h" > #include "virdomainmomentobjlist.h" > #include "virenum.h"
On Tue, Mar 10, 2020 at 06:20:49PM +0100, Ján Tomko wrote: > On a Tuesday in 2020, Gaurav Agrawal wrote: > > --- > > src/qemu/qemu_domain.c | 36 ++++++++++++++++++++---------------- > > src/qemu/qemu_domain.h | 6 ++++-- > > src/qemu/qemu_process.c | 4 ++-- > > 3 files changed, 26 insertions(+), 20 deletions(-) > > > > [...] > > > @@ -10632,7 +10635,8 @@ qemuDomainLogContextPtr qemuDomainLogContextNew(virQEMUDriverPtr driver, > > return ctxt; > > > > error: > > - virObjectUnref(ctxt); > > + if (ctxt) > > + g_object_unref(ctxt); > > g_object_unref is safe to call with a NULL argument, the "if (ctxt)" > check is not needed here. I'm not so sure on that. g_clear_object API docs explicitly say that it is OK if the object is NULL: https://developer.gnome.org/gobject/stable/gobject-The-Base-Object-Type.html#g-clear-object but the g_object_unref docs are completely silent on this matter: https://developer.gnome.org/gobject/stable/gobject-The-Base-Object-Type.html#g-object-ref Thus I've always assumed a NULL check was required for g_object_unref 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 :|
On Tue, Mar 10, 2020 at 17:30:28 +0000, Daniel Berrange wrote: > On Tue, Mar 10, 2020 at 06:20:49PM +0100, Ján Tomko wrote: > > On a Tuesday in 2020, Gaurav Agrawal wrote: > > > --- > > > src/qemu/qemu_domain.c | 36 ++++++++++++++++++++---------------- > > > src/qemu/qemu_domain.h | 6 ++++-- > > > src/qemu/qemu_process.c | 4 ++-- > > > 3 files changed, 26 insertions(+), 20 deletions(-) > > > > > > > [...] > > > > > @@ -10632,7 +10635,8 @@ qemuDomainLogContextPtr qemuDomainLogContextNew(virQEMUDriverPtr driver, > > > return ctxt; > > > > > > error: > > > - virObjectUnref(ctxt); > > > + if (ctxt) > > > + g_object_unref(ctxt); > > > > g_object_unref is safe to call with a NULL argument, the "if (ctxt)" > > check is not needed here. > > I'm not so sure on that. > > g_clear_object API docs explicitly say that it is OK if the object is NULL: > > https://developer.gnome.org/gobject/stable/gobject-The-Base-Object-Type.html#g-clear-object I'd prefer we agree on using this one globally on the same basis we had for using VIR_FREE even on cleanup paths.
On a Wednesday in 2020, Peter Krempa wrote: >On Tue, Mar 10, 2020 at 17:30:28 +0000, Daniel Berrange wrote: >> On Tue, Mar 10, 2020 at 06:20:49PM +0100, Ján Tomko wrote: >> > On a Tuesday in 2020, Gaurav Agrawal wrote: >> > > --- >> > > src/qemu/qemu_domain.c | 36 ++++++++++++++++++++---------------- >> > > src/qemu/qemu_domain.h | 6 ++++-- >> > > src/qemu/qemu_process.c | 4 ++-- >> > > 3 files changed, 26 insertions(+), 20 deletions(-) >> > > >> > >> > [...] >> > >> > > @@ -10632,7 +10635,8 @@ qemuDomainLogContextPtr qemuDomainLogContextNew(virQEMUDriverPtr driver, >> > > return ctxt; >> > > >> > > error: >> > > - virObjectUnref(ctxt); >> > > + if (ctxt) >> > > + g_object_unref(ctxt); >> > >> > g_object_unref is safe to call with a NULL argument, the "if (ctxt)" >> > check is not needed here. >> >> I'm not so sure on that. >> The code itself does check for NULL, it's hidden in the g_return_if_fail (G_IS_OBJECT (object)); call which boils down to g_type_check_instance_is_fundamentally_a Even though the documentation does not mention NULL is safe, it has to be, since it's used as a cleanup function in gobject/gobject-autocleanups.h >> g_clear_object API docs explicitly say that it is OK if the object is NULL: >> >> https://developer.gnome.org/gobject/stable/gobject-The-Base-Object-Type.html#g-clear-object > >I'd prefer we agree on using this one globally on the same basis we had >for using VIR_FREE even on cleanup paths. Sounds good. Most of the unrefs will be handled by g_auto anyway. Jano
On Wed, Mar 11, 2020 at 10:26:24AM +0100, Ján Tomko wrote: > On a Wednesday in 2020, Peter Krempa wrote: > > On Tue, Mar 10, 2020 at 17:30:28 +0000, Daniel Berrange wrote: > > > On Tue, Mar 10, 2020 at 06:20:49PM +0100, Ján Tomko wrote: > > > > On a Tuesday in 2020, Gaurav Agrawal wrote: > > > > > --- > > > > > src/qemu/qemu_domain.c | 36 ++++++++++++++++++++---------------- > > > > > src/qemu/qemu_domain.h | 6 ++++-- > > > > > src/qemu/qemu_process.c | 4 ++-- > > > > > 3 files changed, 26 insertions(+), 20 deletions(-) > > > > > > > > > > > > > [...] > > > > > > > > > @@ -10632,7 +10635,8 @@ qemuDomainLogContextPtr qemuDomainLogContextNew(virQEMUDriverPtr driver, > > > > > return ctxt; > > > > > > > > > > error: > > > > > - virObjectUnref(ctxt); > > > > > + if (ctxt) > > > > > + g_object_unref(ctxt); > > > > > > > > g_object_unref is safe to call with a NULL argument, the "if (ctxt)" > > > > check is not needed here. > > > > > > I'm not so sure on that. > > > > > The code itself does check for NULL, it's hidden in the > g_return_if_fail (G_IS_OBJECT (object)); > call which boils down to > g_type_check_instance_is_fundamentally_a Yes, I think I can see this now, but I wonder if that's an intentionale guarantee or not. > Even though the documentation does not mention NULL is safe, it has to be, since it's > used as a cleanup function in gobject/gobject-autocleanups.h The G_DEFINE_AUTOPTR_CLEANUP_FUNC macro expands to code which has an explicit "if(ptr)" check in it, so the actual func doesn't require this. > > > https://developer.gnome.org/gobject/stable/gobject-The-Base-Object-Type.html#g-clear-object > > > > I'd prefer we agree on using this one globally on the same basis we had > > for using VIR_FREE even on cleanup paths. > > Sounds good. Most of the unrefs will be handled by g_auto anyway. Yep, makes sense in majority of cases. 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 :|
© 2016 - 2024 Red Hat, Inc.