Re: [PATCH RFC 1/4] qemu_agent: move agent into util

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 03/03/2017 11:35 AM, Daniel P. Berrange wrote:
> On Fri, Mar 03, 2017 at 11:36:25AM +0000, Joao Martins wrote:
>>
>>
>> On 03/02/2017 05:58 PM, Jim Fehlig wrote:
>>> Sorry for the review delay.
>>>
>>> On 02/08/2017 09:44 AM, Joao Martins wrote:
>>>> As it could be shared with libxl which now allows channels to
>>>> be created. Also changed filename to match others in the same
>>>> directory namely to virqemuagent.{h,c}
>>>>
>>>> Signed-off-by: Joao Martins <joao.m.martins@xxxxxxxxxx>
>>>> ---
>>>>  po/POTFILES.in               |    2 +-
>>>>  src/Makefile.am              |    2 +-
>>>>  src/libvirt_private.syms     |   21 +
>>>>  src/qemu/qemu_agent.c        | 2248 ------------------------------------------
>>>>  src/qemu/qemu_agent.h        |  123 ---
>>>>  src/qemu/qemu_domain.h       |    2 +-
>>>>  src/qemu/qemu_driver.c       |    2 +-
>>>>  src/util/virqemuagent.c      | 2248 ++++++++++++++++++++++++++++++++++++++++++
>>>>  src/util/virqemuagent.h      |  123 +++
>>>>  tests/qemuagenttest.c        |    2 +-
>>>>  tests/qemumonitortestutils.c |    2 +-
>>>>  tests/qemumonitortestutils.h |    2 +-
>>>>  12 files changed, 2399 insertions(+), 2378 deletions(-)
>>>>  delete mode 100644 src/qemu/qemu_agent.c
>>>>  delete mode 100644 src/qemu/qemu_agent.h
>>>>  create mode 100644 src/util/virqemuagent.c
>>>>  create mode 100644 src/util/virqemuagent.h
>>>
>>> I hope others will opine on this change. It seems reasonable to me and I'm 
>>> surprised the qemu driver only needed tiny changes to accommodate moving all 
>>> this code.
>>
>> Yeap, I too was surprised on how the changes were non disruptive wrt to qemu
>> driver :)
>>
>>>
>>> [...]
>>>> diff --git a/src/util/virqemuagent.h b/src/util/virqemuagent.h
>>>> new file mode 100644
>>>> index 0000000..2e81020
>>>> --- /dev/null
>>>> +++ b/src/util/virqemuagent.h
>>>> @@ -0,0 +1,123 @@
>>>> +/*
>>>> + * virqemuagent.h: interaction with QEMU guest agent
>>>> + *
>>>> + * Copyright (C) 2006-2012 Red Hat, Inc.
>>>> + * Copyright (C) 2006 Daniel P. Berrange
>>>> + *
>>>> + * This library is free software; you can redistribute it and/or
>>>> + * modify it under the terms of the GNU Lesser General Public
>>>> + * License as published by the Free Software Foundation; either
>>>> + * version 2.1 of the License, or (at your option) any later version.
>>>> + *
>>>> + * This library is distributed in the hope that it will be useful,
>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>>>> + * Lesser General Public License for more details.
>>>> + *
>>>> + * You should have received a copy of the GNU Lesser General Public
>>>> + * License along with this library.  If not, see
>>>> + * <http://www.gnu.org/licenses/>.
>>>> + *
>>>> + * Author: Daniel P. Berrange <berrange@xxxxxxxxxx>
>>>> + */
>>>> +
>>>> +
>>>> +#ifndef __QEMU_AGENT_H__
>>>> +# define __QEMU_AGENT_H__
>>>> +
>>>> +# include "internal.h"
>>>> +# include "domain_conf.h"
>>>
>>> ISTR a recent discussion on the list frowning on using code from src/conf in 
>>> src/util, although virthostdev.h also includes domain_conf.h.
>>>
>>> BTW, compilation fails here
>>>
>>> In file included from util/virqemuagent.c:35:0:
>>> util/virqemuagent.h:29:26: fatal error: domain_conf.h: No such file or directory
>>>   # include "domain_conf.h"
>>>
>> Hmm, will have to fix it for v1. This series were applied on top of commit
>> 2841e67 ("qemu: propagate bridge MTU into qemu "host_mtu" option") and I didn't
>> observe compilation  issues (neither make {syntax-check,check} IIRC).
>>
>>>> +
>>>> +typedef struct _qemuAgent qemuAgent;
>>>> +typedef qemuAgent *qemuAgentPtr;
>>>> +
>>>> +typedef struct _qemuAgentCallbacks qemuAgentCallbacks;
>>>> +typedef qemuAgentCallbacks *qemuAgentCallbacksPtr;
>>>> +struct _qemuAgentCallbacks {
>>>> +    void (*destroy)(qemuAgentPtr mon,
>>>> +                    virDomainObjPtr vm);
>>>> +    void (*eofNotify)(qemuAgentPtr mon,
>>>> +                      virDomainObjPtr vm);
>>>> +    void (*errorNotify)(qemuAgentPtr mon,
>>>> +                        virDomainObjPtr vm);
>>>> +};
>>>> +
>>>> +
>>>> +qemuAgentPtr qemuAgentOpen(virDomainObjPtr vm,
>>>> +                           const virDomainChrSourceDef *config,
>>>> +                           qemuAgentCallbacksPtr cb);
>>>> +
>>>> +void qemuAgentClose(qemuAgentPtr mon);
>>>
>>> Other files in src/util prefix structs and functions with "vir". I'm not sure 
>>> how picky folks are about that. If the consensus is towards the "vir" prefix, 
>>> perhaps it would be easier done with a follow-up after the move?
>> Yeah I noticed the pattern. Though given the big move of code into util making
>> as a follow-up patch would easy review perhaps.
> 
> It would be desirable to use the vir name prefix here, but that should
> certainly be done as a separate patch
BTW I said follow-up when I actually meant it in the context of this series i.e.
patch 2/5 would rename these functions to be prefixed by vir.

> - it is bad practice to move and
> rename code at the same time, as it makes diffs harder to review.

/nods

Joao

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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux