Re: [libvirt] [PATCHv4] libvirt: do not mix internal flags into public API
On 07/15/2011 05:12 PM, Eric Blake wrote: There were two API in driver.c that were silently masking flags bits prior to calling out to the drivers, and several others that were explicitly masking flags bits. This is not forward-compatible - if we ever have that many flags in the future, then talking to an old server that masks out the flags would be indistinguishable from talking to a new server that can honor the flag. In general, libvirt.c should forward _all_ flags on to drivers, and only the drivers should reject unknown flags. In the case of virDrvSecretGetValue, the solution is to separate the internal driver callback function to have two parameters instead of one, with only one parameter affected by the public API. On second thought, I don't like this. It's nicer to have all the driver callbacks match the public API as much as possible, and to instead create a new entry point in libvirt_private.syms that qemu can use for the case where it needs to call the internal API instead of the public virSecretGetValue. v5 coming up. -- Eric Blake ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv4] libvirt: do not mix internal flags into public API
On 07/18/2011 01:27 PM, Eric Blake wrote:
On 07/15/2011 05:12 PM, Eric Blake wrote:
There were two API in driver.c that were silently masking flags
bits prior to calling out to the drivers, and several others
that were explicitly masking flags bits. This is not
forward-compatible - if we ever have that many flags in the
future, then talking to an old server that masks out the
flags would be indistinguishable from talking to a new server
that can honor the flag. In general, libvirt.c should forward
_all_ flags on to drivers, and only the drivers should reject
unknown flags.
In the case of virDrvSecretGetValue, the solution is to separate
the internal driver callback function to have two parameters
instead of one, with only one parameter affected by the public
API.
On second thought, I don't like this. It's nicer to have all the driver
callbacks match the public API as much as possible, and to instead
create a new entry point in libvirt_private.syms that qemu can use for
the case where it needs to call the internal API instead of the public
virSecretGetValue. v5 coming up.
Here's the interdiff for v5:
diff --git i/src/driver.h w/src/driver.h
index 9d0d3de..759150d 100644
--- i/src/driver.h
+++ w/src/driver.h
@@ -1258,6 +1258,10 @@ typedef int
typedef unsigned char *
(*virDrvSecretGetValue) (virSecretPtr secret,
size_t *value_size,
+ unsigned int flags);
+typedef unsigned char *
+(*virDrvSecretGetValueInternal) (virSecretPtr secret,
+ size_t *value_size,
unsigned int flags,
unsigned int internalFlags);
typedef int
@@ -1295,6 +1299,7 @@ struct _virSecretDriver {
virDrvSecretGetXMLDesc getXMLDesc;
virDrvSecretSetValue setValue;
virDrvSecretGetValue getValue;
+virDrvSecretGetValueInternal getValueInternal;
virDrvSecretUndefine undefine;
};
diff --git i/src/libvirt.c w/src/libvirt.c
index 39e2041..34acede 100644
--- i/src/libvirt.c
+++ w/src/libvirt.c
@@ -12680,7 +12680,7 @@ virSecretGetValue(virSecretPtr secret, size_t
*value_size, unsigned int flags)
if (conn-secretDriver != NULL conn-secretDriver-getValue !=
NULL) {
unsigned char *ret;
-ret = conn-secretDriver-getValue(secret, value_size, flags, 0);
+ret = conn-secretDriver-getValue(secret, value_size, flags);
if (ret == NULL)
goto error;
return ret;
diff --git i/src/qemu/qemu_process.c w/src/qemu/qemu_process.c
index 448b06e..cb7575f 100644
--- i/src/qemu/qemu_process.c
+++ w/src/qemu/qemu_process.c
@@ -257,7 +257,7 @@ qemuProcessGetVolumeQcowPassphrase(virConnectPtr conn,
if (conn-secretDriver == NULL ||
conn-secretDriver-lookupByUUID == NULL ||
-conn-secretDriver-getValue == NULL) {
+conn-secretDriver-getValueInternal == NULL) {
qemuReportError(VIR_ERR_NO_SUPPORT, %s,
_(secret storage not supported));
goto cleanup;
@@ -276,8 +276,8 @@ qemuProcessGetVolumeQcowPassphrase(virConnectPtr conn,
enc-secrets[0]-uuid);
if (secret == NULL)
goto cleanup;
-data = conn-secretDriver-getValue(secret, size, 0,
-
VIR_SECRET_GET_VALUE_INTERNAL_CALL);
+data = conn-secretDriver-getValueInternal(secret, size, 0,
+
VIR_SECRET_GET_VALUE_INTERNAL_CALL);
virUnrefSecret(secret);
if (data == NULL)
goto cleanup;
diff --git i/src/remote/remote_driver.c w/src/remote/remote_driver.c
index c2f8bbd..d3b5df9 100644
--- i/src/remote/remote_driver.c
+++ w/src/remote/remote_driver.c
@@ -3174,7 +3174,7 @@ remoteSecretClose (virConnectPtr conn)
static unsigned char *
remoteSecretGetValue (virSecretPtr secret, size_t *value_size,
- unsigned int flags, unsigned int internalFlags)
+ unsigned int flags)
{
unsigned char *rv = NULL;
remote_secret_get_value_args args;
@@ -3183,12 +3183,6 @@ remoteSecretGetValue (virSecretPtr secret, size_t
*value_size,
remoteDriverLock (priv);
-/* internalFlags intentionally do not go over the wire */
-if (internalFlags) {
-remoteError(VIR_ERR_NO_SUPPORT, %s, _(no internalFlags
support));
-goto done;
-}
-
make_nonnull_secret (args.secret, secret);
args.flags = flags;
diff --git i/src/secret/secret_driver.c w/src/secret/secret_driver.c
index 02cdbb9..87e2b83 100644
--- i/src/secret/secret_driver.c
+++ w/src/secret/secret_driver.c
@@ -873,8 +873,9 @@ cleanup:
}
static unsigned char *
-secretGetValue(virSecretPtr obj, size_t *value_size, unsigned int flags,
- unsigned int internalFlags)
+secretGetValueInternal(virSecretPtr obj, size_t *value_size,
+ unsigned int flags,
+ unsigned
Re: [libvirt] [PATCHv4] libvirt: do not mix internal flags into public API
On 07/18/2011 02:39 PM, Eric Blake wrote: On second thought, I don't like this. It's nicer to have all the driver callbacks match the public API as much as possible, and to instead create a new entry point in libvirt_private.syms that qemu can use for the case where it needs to call the internal API instead of the public virSecretGetValue. v5 coming up. Here's the interdiff for v5: Except that this break docs/hvsupport.pl, since that file doesn't know that the new function is for internal use only. At this point, I'm leaning back towards just using v4 as is, with a single driver.h callback for secretGetValue, instead of trying to have two different driver callbacks, one public and one for internal only. If we change our minds later, then further touching up of secretGetValue can be an independent patch. -- Eric Blake ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv4] libvirt: do not mix internal flags into public API
On 07/18/2011 02:53 PM, Laine Stump wrote: On 07/15/2011 07:12 PM, Eric Blake wrote: There were two API in driver.c that were silently masking flags bits prior to calling out to the drivers, and several others that were explicitly masking flags bits. This is not forward-compatible - if we ever have that many flags in the future, then talking to an old server that masks out the flags would be indistinguishable from talking to a new server that can honor the flag. In general, libvirt.c should forward _all_ flags on to drivers, and only the drivers should reject unknown flags. +++ b/src/interface/netcf_driver.c @@ -344,6 +344,8 @@ static char *interfaceGetXMLDesc(virInterfacePtr ifinfo, virInterfaceDefPtr ifacedef = NULL; char *ret = NULL; + virCheckFlags(VIR_INTERFACE_XML_INACTIVE, NULL); + If I understand correctly, this is here to put the burden on the driver to check/reject flags, rather than letting the client do it. Correct. Older drivers didn't reject unknown flags, so there is a situation where a newer client talking to a pre-0.9.4 server will misbehave, but the only way to fix that would be to teach 0.9.4 client codes to reject any flags that did not exist prior to 0.9.4 before making the RPC. But from 0.9.4 onwards, all drivers will all reject unknown flags, so the RPC can blindly pass all flags through to let the remote driver decide whether or not a flag is known, instead of rejecting the flag prematurely. - ret = conn-secretDriver-getValue(secret, value_size, flags); + ret = conn-secretDriver-getValue(secret, value_size, flags, 0); So the getValue() callback has an extra internalFlags arg, but that is only non-0 if get Value is called from the driver side (e.g. from qemu), and never sent over the wire. A bit odd, but I can live with it (or you can add the v5 interdiff - either way is fine with me). I ended up pushing v4 as is, and saving the separation of driver functions for a separate patch, which I'm still not convinced whether to keep or drop. ACK, either with or without the v5 interdiff added. Having a separate callback seems cleaner, but may have other implications I'm not aware of. Now pushed. -- Eric Blake ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
