Re: [Xen-changelog] If the 'cdrom=' option is specified in the definition file but media is

Re: [Xen-changelog] If the 'cdrom=' option is specified in the definition file but media is

[Anthony Liguori]

Xen patchbot -3.0-testing wrote:
> # HG changeset patch
> # User kaf24@firebug.cl.cam.ac.uk
> # Node ID fd526926e0d1c0671295aa7f4b952186c9345173
> # Parent  408f51a850f47af4db20f43f281935909d502511
> If the 'cdrom=' option is specified in the definition file but media is 
> not found in the CD drive then main() in vl.c exits and the guest appears 
> to hang.  This patch modifies vl.c slightly to check for the presents of 
> media.  If the cdrom cannot be opened then the cd entry is removed from 
> hd_filename[] and bs_table[] allowing the guest to continue initializing. 
> If the guest requires the CD media then the guest should report, gracefully
> or otherwise, that it's missing.
>
> From: Ross Maxfield <rmaxfiel@novell.com>
>
> Signed-off-by: Keir Fraser <keir@xensource.com>
>   

Doesn't this need a Signed-off-by: Ross Maxfield <rmaxfiel@novell.com>?

Regards,

Anthony Liguori

> diff -r 408f51a850f4 -r fd526926e0d1 tools/ioemu/vl.c
> --- a/tools/ioemu/vl.c	Mon Apr 10 16:14:36 2006
> +++ b/tools/ioemu/vl.c	Mon Apr 10 16:17:07 2006
> @@ -3243,8 +3243,17 @@
>      /* we always create the cdrom drive, even if no disk is there */
>      bdrv_init();
>      if (has_cdrom) {
> -        bs_table[2] = bdrv_new("cdrom");
> -        bdrv_set_type_hint(bs_table[2], BDRV_TYPE_CDROM);
> +        int fd;
> +        if ( (fd = open(hd_filename[2], O_RDONLY | O_BINARY)) < 0) {
> +                hd_filename[2]=NULL;
> +                bs_table[2]=NULL;
> +                fprintf(logfile, "Could not open CD %s.\n", hd_filename[i]);
> +        }
> +        else {
> +                close(fd);
> +                bs_table[2] = bdrv_new("cdrom");
> +                bdrv_set_type_hint(bs_table[2], BDRV_TYPE_CDROM);
> +        }
>      }
>  
>      /* open the virtual block devices */
>
> _______________________________________________
> Xen-changelog mailing list
> Xen-changelog@lists.xensource.com
> http://lists.xensource.com/xen-changelog
>   

Re: Re: [Xen-changelog] If the 'cdrom=' option is specified in the definition file but media is

[Ewan Mellor]

On Mon, Apr 10, 2006 at 01:29:13PM -0500, Anthony Liguori wrote:

> Xen patchbot -3.0-testing wrote:
> ># HG changeset patch
> ># User kaf24@firebug.cl.cam.ac.uk
> ># Node ID fd526926e0d1c0671295aa7f4b952186c9345173
> ># Parent  408f51a850f47af4db20f43f281935909d502511
> >If the 'cdrom=' option is specified in the definition file but media is 
> >not found in the CD drive then main() in vl.c exits and the guest appears 
> >to hang.  This patch modifies vl.c slightly to check for the presents of 
> >media.  If the cdrom cannot be opened then the cd entry is removed from 
> >hd_filename[] and bs_table[] allowing the guest to continue initializing. 
> >If the guest requires the CD media then the guest should report, gracefully
> >or otherwise, that it's missing.
> >
> >From: Ross Maxfield <rmaxfiel@novell.com>
> >
> >Signed-off-by: Keir Fraser <keir@xensource.com>
> >  
> 
> Doesn't this need a Signed-off-by: Ross Maxfield <rmaxfiel@novell.com>?

People have been complaining that a patch should not retain the Signed-off-by
line if the patch has been modified, because they do not sign-off the modified
patch.  If a patch needs minor changes before it can be committed, we can
either bounce it back to the author, which seems unnecessarily heavyweight, or
do what Keir's done here, and sign-off the patch himself.  The From: line
retains the audit trail, credit, and copyright, and it's clear that Keir
himself thinks that this patch is acceptable.

Ewan.

Re: Re: [Xen-changelog] If the 'cdrom=' option is specified in the definition file but media is

[Anthony Liguori]

Ewan Mellor wrote:
> On Mon, Apr 10, 2006 at 01:29:13PM -0500, Anthony Liguori wrote:
>  
>   
>> Doesn't this need a Signed-off-by: Ross Maxfield <rmaxfiel@novell.com>?
>>     
>
> People have been complaining that a patch should not retain the Signed-off-by
> line if the patch has been modified, because they do not sign-off the modified
> patch.  If a patch needs minor changes before it can be committed, we can
> either bounce it back to the author, which seems unnecessarily heavyweight, or
> do what Keir's done here, and sign-off the patch himself.  The From: line
> retains the audit trail, credit, and copyright, and it's clear that Keir
> himself thinks that this patch is acceptable.
>   

I won't speak for Hollis (although I will CC him :-)) but my 
understanding is that the appropriate thing to do is check in the patch 
with the original Signed-off-by and then check in another patch on top 
of that with the necessary changes (this time, with just Keir's 
Signed-off-by).

I think the intention is that the original submitter needs to have a 
Signed-off-by to signify that the origin of the code is kosher (which is 
something Keir cannot do on his own if the code didn't originate from 
him).  Is this how other people understand it?

Regards,

Anthony Liguori

> Ewan.
>   

Re: Signed-off-by again

[Hollis Blanchard]

On Mon, 2006-04-10 at 15:54 -0500, Anthony Liguori wrote:
> Ewan Mellor wrote:
> > On Mon, Apr 10, 2006 at 01:29:13PM -0500, Anthony Liguori wrote:
> >  
> >   
> >> Doesn't this need a Signed-off-by: Ross Maxfield <rmaxfiel@novell.com>?
> >>     
> >
> > People have been complaining that a patch should not retain the Signed-off-by
> > line if the patch has been modified, because they do not sign-off the modified
> > patch.  If a patch needs minor changes before it can be committed, we can
> > either bounce it back to the author, which seems unnecessarily heavyweight, or
> > do what Keir's done here, and sign-off the patch himself.  The From: line
> > retains the audit trail, credit, and copyright, and it's clear that Keir
> > himself thinks that this patch is acceptable.
> >   
> 
> I won't speak for Hollis (although I will CC him :-)) but my 
> understanding is that the appropriate thing to do is check in the patch 
> with the original Signed-off-by and then check in another patch on top 
> of that with the necessary changes (this time, with just Keir's 
> Signed-off-by).
> 
> I think the intention is that the original submitter needs to have a 
> Signed-off-by to signify that the origin of the code is kosher (which is 
> something Keir cannot do on his own if the code didn't originate from 
> him).  Is this how other people understand it?

Actually, now I'm confused about the DCO
(http://www.osdl.org/newsroom/press_releases/2004/2004_05_24_dco.html).
The terms seem to allow adding Signed-off-by lines when making
modifications, but that seems obviously in conflict point of the system.

See also Linus's mail at http://kerneltrap.org/node/3180:
        It also allows middle parties to edit the patch without somehow
        "losing" their names - quite often the patch that reaches the
        final kernel is not exactly the same as the original one, as it
        has gone through a few layers of people.

So in the Linux system, it is OK for Keir to modify (not rewrite) and
add his Signed-off-by after all.

The reason I don't like that is this example:
- Mary submits a clean patch with signed-off-by line
- Joe adds some bad IP to Mary's patch (e.g. some proprietary
copyrighted code), adds his signed-off-by line, and forwards on
- patch is checked in
- lawyers find the infringing patch, and look, there's Mary apparently
signing off on it

We could follow the Linux system, or something stronger (i.e. no
modifications to other people's patches allowed). I guess it's up to the
maintainers...

Whatever is chosen, it needs to be *documented* (beyond just the DCO URL
above) and adhered to.

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: Signed-off-by again

[Mike D. Day]

Hollis Blanchard wrote:

> 
> So in the Linux system, it is OK for Keir to modify (not rewrite) and
> add his Signed-off-by after all.

Yes after reading the email reference I agree with Hollis , and I was 
wrong to say that Keir's method was against linux DCO guidelines (it was 
within guidelines).

> 
> We could follow the Linux system, or something stronger (i.e. no
> modifications to other people's patches allowed). I guess it's up to the
> maintainers...

I still say the right thing in this case is to bounce the patch back to 
the author (preferably with comments). That way all of us gain the 
benefit of the dialog.


Mike

Re: Signed-off-by again

[Muli Ben-Yehuda]

On Mon, Apr 10, 2006 at 04:24:20PM -0500, Hollis Blanchard wrote:

> So in the Linux system, it is OK for Keir to modify (not rewrite) and
> add his Signed-off-by after all.

You have to consider the intent: it's ok and reasonable for a
maintainer to adapt a patch in a trivial way for another change he has
in his tree (e.g., a function prototype changed), but it's neither ok
nor reasonable for the maintainer to rewrite the patch and keep the
original SOB.

As for the bad IP example - anyone who adds his SOB beyond the
original author is one of the maintainers, and maintainers are
implicitly trusted. If you have maintainers who start adding bad IP
...

Cheers,
Muli
-- 
Muli Ben-Yehuda
http://www.mulix.org | http://mulix.livejournal.com/

Re: Signed-off-by again

[Anthony Liguori]

Muli Ben-Yehuda wrote:
> On Mon, Apr 10, 2006 at 04:24:20PM -0500, Hollis Blanchard wrote:
>
>   
>> So in the Linux system, it is OK for Keir to modify (not rewrite) and
>> add his Signed-off-by after all.
>>     
>
> You have to consider the intent: it's ok and reasonable for a
> maintainer to adapt a patch in a trivial way for another change he has
> in his tree (e.g., a function prototype changed), but it's neither ok
> nor reasonable for the maintainer to rewrite the patch and keep the
> original SOB.
>   

Having another changeset is so trivial that IMHO one should always just 
hg import the original patch and then add another one on top of it.

Personally, I'm very interested to see the things about a patch that 
warrant modification so I can avoid doing them in my own patches.  
Having that expressed as a separate changeset is very useful (especially 
if it has a nice commit message explaining the reasons for the 
modification).

Regards,

Anthony Liguori

> As for the bad IP example - anyone who adds his SOB beyond the
> original author is one of the maintainers, and maintainers are
> implicitly trusted. If you have maintainers who start adding bad IP
> ...
>
> Cheers,
> Muli
>   

Re: Re: [Xen-changelog] If the 'cdrom=' option is specified in the definition file but media is

[Mike D. Day]

Ewan Mellor wrote:
>>> From: Ross Maxfield <rmaxfiel@novell.com>
>>>
>>> Signed-off-by: Keir Fraser <keir@xensource.com>
>>>  
>> Doesn't this need a Signed-off-by: Ross Maxfield <rmaxfiel@novell.com>?
> 
> People have been complaining that a patch should not retain the Signed-off-by
> line if the patch has been modified, because they do not sign-off the modified
> patch.  If a patch needs minor changes before it can be committed, we can
> either bounce it back to the author, which seems unnecessarily heavyweight, or
> do what Keir's done here, and sign-off the patch himself.  The From: line
> retains the audit trail, credit, and copyright, and it's clear that Keir
> himself thinks that this patch is acceptable.

This is not acceptable practice in my opinion. An alternative would be 
to add an Acked-by: Ross line to verify that the code is still ok'd per 
the DCO guidelines by the original author.

What would be even better is to follow the accepted practice for linux 
development, which is to bounce it back to the author with comments. In 
fact, please explain why we don't follow that practice again? If the 
answer is that it is too heavyweight please think again.

Mike Day

Re: Re: [Xen-changelog] If the 'cdrom=' option is specified in the definition file but media is

[Anthony Liguori]

Ewan Mellor wrote:
> On Mon, Apr 10, 2006 at 01:29:13PM -0500, Anthony Liguori wrote:
>  
>   
>> Doesn't this need a Signed-off-by: Ross Maxfield <rmaxfiel@novell.com>?
>>     
>
> People have been complaining that a patch should not retain the Signed-off-by
> line if the patch has been modified, because they do not sign-off the modified
> patch.  If a patch needs minor changes before it can be committed, we can
> either bounce it back to the author, which seems unnecessarily heavyweight, or
> do what Keir's done here, and sign-off the patch himself.  The From: line
> retains the audit trail, credit, and copyright, and it's clear that Keir
> himself thinks that this patch is acceptable.
>   

I won't speak for Hollis (although I will CC him :-)) but my 
understanding is that the appropriate thing to do is check in the patch 
with the original Signed-off-by and then check in another patch on top 
of that with the necessary changes (this time, with just Keir's 
Signed-off-by).

I think the intention is that the original submitter needs to have a 
Signed-off-by to signify that the origin of the code is kosher (which is 
something Keir cannot do on his own if the code didn't originate from 
him).  Is this how other people understand it?

Regards,

Anthony Liguori

> Ewan.
>   

Re: Re: [Xen-changelog] If the 'cdrom=' option is specified in the definition file but media is

[Anthony Liguori]

Ewan Mellor wrote:
> On Mon, Apr 10, 2006 at 01:29:13PM -0500, Anthony Liguori wrote:
>
>   
>> Xen patchbot -3.0-testing wrote:
>>     
>>> # HG changeset patch
>>> # User kaf24@firebug.cl.cam.ac.uk
>>> # Node ID fd526926e0d1c0671295aa7f4b952186c9345173
>>> # Parent  408f51a850f47af4db20f43f281935909d502511
>>> If the 'cdrom=' option is specified in the definition file but media is 
>>> not found in the CD drive then main() in vl.c exits and the guest appears 
>>> to hang.  This patch modifies vl.c slightly to check for the presents of 
>>> media.  If the cdrom cannot be opened then the cd entry is removed from 
>>> hd_filename[] and bs_table[] allowing the guest to continue initializing. 
>>> If the guest requires the CD media then the guest should report, gracefully
>>> or otherwise, that it's missing.
>>>
>>> From: Ross Maxfield <rmaxfiel@novell.com>
>>>
>>> Signed-off-by: Keir Fraser <keir@xensource.com>
>>>  
>>>       
>> Doesn't this need a Signed-off-by: Ross Maxfield <rmaxfiel@novell.com>?
>>     
>
> People have been complaining that a patch should not retain the Signed-off-by
> line if the patch has been modified, because they do not sign-off the modified
> patch.  If a patch needs minor changes before it can be committed, we can
> either bounce it back to the author, which seems unnecessarily heavyweight, or
> do what Keir's done here, and sign-off the patch himself.  The From: line
> retains the audit trail, credit, and copyright, and it's clear that Keir
> himself thinks that this patch is acceptable.
>   

Just so this doesn't get lost in the discussion,  I meant to say that I 
was referring to the original submission.  The original patch submission 
did not contain a Signed-off-by line.

Regardless of the interpretation of the DCO, Ross should resubmit the 
patch with a Signed-off-by.

Regards,

Anthony Liguori

> Ewan.
>   

RE: Signed-off-by again

[Ian Pratt]

> Having another changeset is so trivial that IMHO one should 
> always just hg import the original patch and then add another 
> one on top of it.
> 
> Personally, I'm very interested to see the things about a 
> patch that warrant modification so I can avoid doing them in 
> my own patches.  
> Having that expressed as a separate changeset is very useful 
> (especially if it has a nice commit message explaining the 
> reasons for the modification).

In most cases the modifications are just to fix merge conflicts, or make
trivial formating fixes. Bouncing these back to the submitter is time
consuming for the maintainer, adds latency, and can lead to patch
lossage.

In the case of a merge conflict, it's not actually possible with
mercurial to checkin the original patch and the fix. In the cases where
there has been other trivial changes, it seems quite heavy weight to
create a 2nd changeset: having unnecessary changesets causes unnecessary
xenrt validation runs, slows down binary-chop bug hunting, and
complicates back porting patches. 

My preferance would be to authorize maintainers to make 'simple'
modifications to patches before checkin as a single changset. Anything
more 'semantic' should be a second changest.

I fully agree that once we agree the finer points of the process we
should document it and stick to it religously. We need a "patch
submission process" document on the wiki.

Ian 

Re: Signed-off-by again

[Muli Ben-Yehuda]

On Tue, Apr 11, 2006 at 05:59:50PM +0100, Ian Pratt wrote:

> My preferance would be to authorize maintainers to make 'simple'
> modifications to patches before checkin as a single changset. Anything
> more 'semantic' should be a second changest.

Sounds great to me, and fits in with Linux's usage of the DCO.

Cheers,
Muli
-- 
Muli Ben-Yehuda
http://www.mulix.org | http://mulix.livejournal.com/