util: Use virStorageSourceNew in virStorageFileMetadataNew

[libvirt] [PATCH] util: Use virStorageSourceNew in virStorageFileMetadataNew

Posted by Peter Krempa 6 years, 11 months ago

Commit dcda2bf4c110 forgot to fix this one instance.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 src/util/virstoragefile.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 5927140a66..b2e308d81d 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -1119,22 +1119,20 @@ static virStorageSourcePtr
 virStorageFileMetadataNew(const char *path,
                           int format)
 {
-    virStorageSourcePtr def = NULL;
+    VIR_AUTOUNREF(virStorageSourcePtr) def = NULL;
+    virStorageSourcePtr ret = NULL;

-    if (VIR_ALLOC(def) < 0)
+    if (!(def = virStorageSourceNew()))
         return NULL;

     def->format = format;
     def->type = VIR_STORAGE_TYPE_FILE;

     if (VIR_STRDUP(def->path, path) < 0)
-        goto error;
-
-    return def;
+        return NULL;

- error:
-    virObjectUnref(def);
-    return NULL;
+    VIR_STEAL_PTR(ret, def);
+    return ret;
 }


-- 
2.20.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] util: Use virStorageSourceNew in virStorageFileMetadataNew

Posted by John Ferlan 6 years, 11 months ago


On 2/18/19 7:27 AM, Peter Krempa wrote:
> Commit dcda2bf4c110 forgot to fix this one instance.
> 
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
>  src/util/virstoragefile.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 

Right fix as far as I'm concerned; however, I am curious to know whether
this passes the MinGW build test because this is exactly where things
started to break down for VIR_AUTOPTR usage.

VIR_AUTOUNREF could also be used more liberally in this module...

> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> index 5927140a66..b2e308d81d 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
> @@ -1119,22 +1119,20 @@ static virStorageSourcePtr
>  virStorageFileMetadataNew(const char *path,
>                            int format)
>  {
> -    virStorageSourcePtr def = NULL;
> +    VIR_AUTOUNREF(virStorageSourcePtr) def = NULL;
> +    virStorageSourcePtr ret = NULL;

Erik prefers the usage of VIR_AUTO* defs last (IOW, swap these).

As long as this gets through the MinGW build test, then

Reviewed-by: John Ferlan <jferlan@redhat.com>

John

Altering other virStorageSource refs here to use VIR_AUTOUNREF would be
the next step (and of course getting them through MinGW build test).

> 
> -    if (VIR_ALLOC(def) < 0)
> +    if (!(def = virStorageSourceNew()))
>          return NULL;
> 
>      def->format = format;
>      def->type = VIR_STORAGE_TYPE_FILE;
> 
>      if (VIR_STRDUP(def->path, path) < 0)
> -        goto error;
> -
> -    return def;
> +        return NULL;
> 
> - error:
> -    virObjectUnref(def);
> -    return NULL;
> +    VIR_STEAL_PTR(ret, def);
> +    return ret;
>  }
> 
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] util: Use virStorageSourceNew in virStorageFileMetadataNew

Posted by Peter Krempa 6 years, 11 months ago

On Mon, Feb 18, 2019 at 07:38:34 -0500, John Ferlan wrote:
> 
> 
> On 2/18/19 7:27 AM, Peter Krempa wrote:
> > Commit dcda2bf4c110 forgot to fix this one instance.
> > 
> > Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> > ---
> >  src/util/virstoragefile.c | 14 ++++++--------
> >  1 file changed, 6 insertions(+), 8 deletions(-)
> > 
> 
> Right fix as far as I'm concerned; however, I am curious to know whether
> this passes the MinGW build test because this is exactly where things
> started to break down for VIR_AUTOPTR usage.

We'll see after I push it. I don't have mingw deployed and don't care
enough about that platform to do so.

> 
> VIR_AUTOUNREF could also be used more liberally in this module...

I'll not pursue this refactor.

> 
> > diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> > index 5927140a66..b2e308d81d 100644
> > --- a/src/util/virstoragefile.c
> > +++ b/src/util/virstoragefile.c
> > @@ -1119,22 +1119,20 @@ static virStorageSourcePtr
> >  virStorageFileMetadataNew(const char *path,
> >                            int format)
> >  {
> > -    virStorageSourcePtr def = NULL;
> > +    VIR_AUTOUNREF(virStorageSourcePtr) def = NULL;
> > +    virStorageSourcePtr ret = NULL;
> 
> Erik prefers the usage of VIR_AUTO* defs last (IOW, swap these).

Well I prefer if the returned variable is last and if the longer lines
are first.

> As long as this gets through the MinGW build test, then
> 
> Reviewed-by: John Ferlan <jferlan@redhat.com>
> 
> John
> 
> Altering other virStorageSource refs here to use VIR_AUTOUNREF would be
> the next step (and of course getting them through MinGW build test).
> 
> > 
> > -    if (VIR_ALLOC(def) < 0)
> > +    if (!(def = virStorageSourceNew()))
> >          return NULL;
> > 
> >      def->format = format;
> >      def->type = VIR_STORAGE_TYPE_FILE;
> > 
> >      if (VIR_STRDUP(def->path, path) < 0)
> > -        goto error;
> > -
> > -    return def;
> > +        return NULL;
> > 
> > - error:
> > -    virObjectUnref(def);
> > -    return NULL;
> > +    VIR_STEAL_PTR(ret, def);
> > +    return ret;
> >  }
> > 
> > 
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] util: Use virStorageSourceNew in virStorageFileMetadataNew

Posted by John Ferlan 6 years, 11 months ago


On 2/18/19 7:51 AM, Peter Krempa wrote:
> On Mon, Feb 18, 2019 at 07:38:34 -0500, John Ferlan wrote:
>>
>>
>> On 2/18/19 7:27 AM, Peter Krempa wrote:
>>> Commit dcda2bf4c110 forgot to fix this one instance.
>>>
>>> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
>>> ---
>>>  src/util/virstoragefile.c | 14 ++++++--------
>>>  1 file changed, 6 insertions(+), 8 deletions(-)
>>>
>>
>> Right fix as far as I'm concerned; however, I am curious to know whether
>> this passes the MinGW build test because this is exactly where things
>> started to break down for VIR_AUTOPTR usage.
> 
> We'll see after I push it. I don't have mingw deployed and don't care
> enough about that platform to do so.
> 

Cannot disagree about the relevance of the importance of MinGW. Still I
note it because it was something that caused previous changes to add
VIR_AUTOPTR for this module to not be pushed. I was pointed in the
direction of Andrea and the lcitool, but TBH that didn't help me much.
Eventually I noted that Erik had run a build via some link between
travis-ci.org and a github repo. I was able to do something similar and
found a similar failure.

>>
>> VIR_AUTOUNREF could also be used more liberally in this module...
> 
> I'll not pursue this refactor.
> 
>>
>>> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
>>> index 5927140a66..b2e308d81d 100644
>>> --- a/src/util/virstoragefile.c
>>> +++ b/src/util/virstoragefile.c
>>> @@ -1119,22 +1119,20 @@ static virStorageSourcePtr
>>>  virStorageFileMetadataNew(const char *path,
>>>                            int format)
>>>  {
>>> -    virStorageSourcePtr def = NULL;
>>> +    VIR_AUTOUNREF(virStorageSourcePtr) def = NULL;
>>> +    virStorageSourcePtr ret = NULL;
>>
>> Erik prefers the usage of VIR_AUTO* defs last (IOW, swap these).
> 
> Well I prefer if the returned variable is last and if the longer lines
> are first.
> 

Picking and choosing which review comments to follow is an interesting
decision - hopefully it's not contagious.

Consistency wise, VIR_AUTO* defs have been last. If it's that important
I suppose per usual someone can come in afterwards and propose another
patch as well as either a rule in/for make check or adjustment to the
hacking guide.

John

>> As long as this gets through the MinGW build test, then
>>
>> Reviewed-by: John Ferlan <jferlan@redhat.com>
>>
>> John
>>
>> Altering other virStorageSource refs here to use VIR_AUTOUNREF would be
>> the next step (and of course getting them through MinGW build test).
>>
>>>
>>> -    if (VIR_ALLOC(def) < 0)
>>> +    if (!(def = virStorageSourceNew()))
>>>          return NULL;
>>>
>>>      def->format = format;
>>>      def->type = VIR_STORAGE_TYPE_FILE;
>>>
>>>      if (VIR_STRDUP(def->path, path) < 0)
>>> -        goto error;
>>> -
>>> -    return def;
>>> +        return NULL;
>>>
>>> - error:
>>> -    virObjectUnref(def);
>>> -    return NULL;
>>> +    VIR_STEAL_PTR(ret, def);
>>> +    return ret;
>>>  }
>>>
>>>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] util: Use virStorageSourceNew in virStorageFileMetadataNew

Posted by Peter Krempa 6 years, 11 months ago

On Mon, Feb 18, 2019 at 08:28:54 -0500, John Ferlan wrote:
> 
> 
> On 2/18/19 7:51 AM, Peter Krempa wrote:
> > On Mon, Feb 18, 2019 at 07:38:34 -0500, John Ferlan wrote:
> >>
> >>
> >> On 2/18/19 7:27 AM, Peter Krempa wrote:
> >>> Commit dcda2bf4c110 forgot to fix this one instance.
> >>>
> >>> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> >>> ---
> >>>  src/util/virstoragefile.c | 14 ++++++--------
> >>>  1 file changed, 6 insertions(+), 8 deletions(-)
> >>>
> >>
> >> Right fix as far as I'm concerned; however, I am curious to know whether
> >> this passes the MinGW build test because this is exactly where things
> >> started to break down for VIR_AUTOPTR usage.
> > 
> > We'll see after I push it. I don't have mingw deployed and don't care
> > enough about that platform to do so.
> > 
> 
> Cannot disagree about the relevance of the importance of MinGW. Still I
> note it because it was something that caused previous changes to add
> VIR_AUTOPTR for this module to not be pushed. I was pointed in the
> direction of Andrea and the lcitool, but TBH that didn't help me much.
> Eventually I noted that Erik had run a build via some link between
> travis-ci.org and a github repo. I was able to do something similar and
> found a similar failure.

I've looked at the ci.centos.org page and the tests were aborted after
90 mins without any error:

This is the last output:

  CC       virnettlshelpers.o
  CC       pkix_asn1_tab.o
  CC       virnettlssessiontest.o
  CC       virdbustest-virdbustest.o
  CC       virdbustest-testutils.o
Build timed out (after 90 minutes). Marking the build as aborted.
Build was aborted
Finished: ABORTED

Seems to me that it's more an environment or compiler problem.

> >> VIR_AUTOUNREF could also be used more liberally in this module...
> > 
> > I'll not pursue this refactor.
> > 
> >>
> >>> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> >>> index 5927140a66..b2e308d81d 100644
> >>> --- a/src/util/virstoragefile.c
> >>> +++ b/src/util/virstoragefile.c
> >>> @@ -1119,22 +1119,20 @@ static virStorageSourcePtr
> >>>  virStorageFileMetadataNew(const char *path,
> >>>                            int format)
> >>>  {
> >>> -    virStorageSourcePtr def = NULL;
> >>> +    VIR_AUTOUNREF(virStorageSourcePtr) def = NULL;
> >>> +    virStorageSourcePtr ret = NULL;
> >>
> >> Erik prefers the usage of VIR_AUTO* defs last (IOW, swap these).
> > 
> > Well I prefer if the returned variable is last and if the longer lines
> > are first.
> > 
> 
> Picking and choosing which review comments to follow is an interesting
> decision - hopefully it's not contagious.
> 
> Consistency wise, VIR_AUTO* defs have been last. If it's that important
> I suppose per usual someone can come in afterwards and propose another
> patch as well as either a rule in/for make check or adjustment to the
> hacking guide.

Any other variable addition after the block of the VIR_AUTO*stuff will
be forgotten sooner or later during review, so if isn't enforced by
a syntax check rule, it's pointless to even think to open the editor
and change it.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] util: Use virStorageSourceNew in virStorageFileMetadataNew

Posted by Erik Skultety 6 years, 11 months ago

On Mon, Feb 18, 2019 at 08:28:54AM -0500, John Ferlan wrote:
>
>
> On 2/18/19 7:51 AM, Peter Krempa wrote:
> > On Mon, Feb 18, 2019 at 07:38:34 -0500, John Ferlan wrote:
> >>
> >>
> >> On 2/18/19 7:27 AM, Peter Krempa wrote:
> >>> Commit dcda2bf4c110 forgot to fix this one instance.
> >>>
> >>> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> >>> ---
> >>>  src/util/virstoragefile.c | 14 ++++++--------
> >>>  1 file changed, 6 insertions(+), 8 deletions(-)
> >>>
> >>
> >> Right fix as far as I'm concerned; however, I am curious to know whether
> >> this passes the MinGW build test because this is exactly where things
> >> started to break down for VIR_AUTOPTR usage.
> >
> > We'll see after I push it. I don't have mingw deployed and don't care
> > enough about that platform to do so.
> >
>
> Cannot disagree about the relevance of the importance of MinGW. Still I
> note it because it was something that caused previous changes to add
> VIR_AUTOPTR for this module to not be pushed. I was pointed in the
> direction of Andrea and the lcitool, but TBH that didn't help me much.
> Eventually I noted that Erik had run a build via some link between
> travis-ci.org and a github repo. I was able to do something similar and
> found a similar failure.

This actually passes the build on MinGW:
https://travis-ci.org/eskultety/libvirt

>
> >>
> >> VIR_AUTOUNREF could also be used more liberally in this module...
> >
> > I'll not pursue this refactor.
> >
> >>
> >>> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> >>> index 5927140a66..b2e308d81d 100644
> >>> --- a/src/util/virstoragefile.c
> >>> +++ b/src/util/virstoragefile.c
> >>> @@ -1119,22 +1119,20 @@ static virStorageSourcePtr
> >>>  virStorageFileMetadataNew(const char *path,
> >>>                            int format)
> >>>  {
> >>> -    virStorageSourcePtr def = NULL;
> >>> +    VIR_AUTOUNREF(virStorageSourcePtr) def = NULL;
> >>> +    virStorageSourcePtr ret = NULL;
> >>
> >> Erik prefers the usage of VIR_AUTO* defs last (IOW, swap these).
> >
> > Well I prefer if the returned variable is last and if the longer lines
> > are first.
> >
>
> Picking and choosing which review comments to follow is an interesting
> decision - hopefully it's not contagious.
>
> Consistency wise, VIR_AUTO* defs have been last. If it's that important
> I suppose per usual someone can come in afterwards and propose another
> patch as well as either a rule in/for make check or adjustment to the
> hacking guide.

I am obviously in favour of consistency and I'd like to us to follow it, but of'
course as a reviewer you can't really force the author to do that :/..
well, wechnically we could abuse perl once again for "the rescue" and create a
syntax-check rule, but HELL no...I agree that we might want to mention this in
the HACKING guide.

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list