On Wed, Oct 05, 2011 at 05:04:28PM -0300, Marcelo Moreira de Mello wrote:
> On 10/05/2011 04:03 AM, Jan Pazdziora wrote:
> > Nack.
> >
> > The first part of your patch reverts change for bug 452956. Unless we
> > know *exactly* why that change was done for that bug (in other words,
> > reproduce that original bug, without that patch), we risk a regression
> > here.
> >
> > The second part of your patch seems like a noop to me since
> > StringUtils.defaultString is defined as
> >
> >     Returns either the passed in String, or if the String is null,
> >     an empty String ("").
> >
> > already.
> 
>   After reviewing the patch follow attach a better looking patch for
> this bug. Doing our tests, this patch does not include a regression for
> bug 452956.

>  .../frontend/xmlrpc/serializer/DmiSerializer.java  |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git 
> a/java/code/src/com/redhat/rhn/frontend/xmlrpc/serializer/DmiSerializer.java 
> b/java/code/src/com/redhat/rhn/frontend/xmlrpc/serializer/DmiSerializer.java
> index 7fb29ad..1710c2d 100644
> --- 
> a/java/code/src/com/redhat/rhn/frontend/xmlrpc/serializer/DmiSerializer.java
> +++ 
> b/java/code/src/com/redhat/rhn/frontend/xmlrpc/serializer/DmiSerializer.java
> @@ -60,7 +60,7 @@ public class DmiSerializer implements 
> XmlRpcCustomSerializer {
>          Dmi dmi = (Dmi) value;
>  
>          if (dmi.getSystem() == null) {
> -            return;
> +            dmi.setSystem("");
>          }
>  
>          bean.add("vendor", StringUtils.defaultString(dmi.getVendor()));

I still don't think the patch is correct. There is no point in
dmi.setSystem'ing the system -- the StringUtils.defaultString will do
the trick for the output, without modifying the value, which could
potentially cause hibernate want to store it back to the database.

I believe the correct fix is to remove that

        if (dmi.getSystem() == null) {

test and branch altogether but if you do that, you need to find out
what is the alternate fix for the bug 452956 in the first place.

It might be the bios thing, it might be something different.

-- 
Jan Pazdziora
Principal Software Engineer, Satellite Engineering, Red Hat

_______________________________________________
Spacewalk-devel mailing list
Spacewalk-devel@redhat.com
https://www.redhat.com/mailman/listinfo/spacewalk-devel

Reply via email to